Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

libobs: Deprecate obs_get_transition_by_[name|uuid] #11206

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gxalpha
Copy link
Member

@gxalpha gxalpha commented Aug 28, 2024

Description

obs_get_transition_by_name is very problematic because transitions are always private and their names aren't unique. This means that the method iterates over all private sources and then takes the first that both is a transition and matches the name we're looking for. However, this could be from anywhere - it could be a frontend transition, but also a source transition, quick transition, or even one from third-party plugins. This is always never what is intended.

As such, this method (which should never have been added in the first place) needs to go. In its place, obs_frontend_get_transitions returns a list of all frontend transitions (which is usually what people are looking for), and alternatively obs_get_source_by_uuid also provides access to private sources.

While we're at it, obs_get_transition_by_uuid is basically a wrapper for obs_get_source_by_uuid and not really necessary. UUID's are unique and a source doesn't suddenly change its type, so if you have a transition's UUID you can be pretty sure that when you do obs_get_source_by_uuid, it will still be a transition.

I optimistically set 31.0 as the deprecation version in the docs but will update once that is no longer possible.

Motivation and Context

#11200 revealed that this method did make it in (#5504, c36a5ae). While its usages were quickly removed after the accompanying browser PR got merged (because of the aforementioned problems), the method stayed (I really should have removed it, this is my fault. Or better yet, not even have written it. Three years ago me was a bad programmer (me in three years will probably say the same about current me!)).

How Has This Been Tested?

Checked GitHub code search to find no usages of these methods (besides the usual forks and bindings), which is good.
Checked the PR build of the documentation, looks correct.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • Code cleanup (non-breaking change which makes code smaller or more readable)

Checklist:

  • My code has been run through clang-format.
  • I have read the contributing document.
  • My code is not on the master branch.
  • The code has been tested.
  • All commit messages are properly formatted and commits squashed where appropriate.
  • I have included updates to all appropriate documentation.

@gxalpha gxalpha added Bug Fix Non-breaking change which fixes an issue Code Cleanup Non-breaking change which makes code smaller or more readable labels Aug 28, 2024
@WizardCM
Copy link
Member

I absolutely agree with deprecating obs_get_transition_by_name, but have less confidence in deprecating obs_get_transition_by_uuid, as the internals could change in the future and having them separate could be useful. Generally if a user is interacting with a transition, the API scope should reflect that.

@gxalpha
Copy link
Member Author

gxalpha commented Sep 1, 2024

as the internals could change in the future

Which internals are you thinking of here specifically? If source_by_uuid suddenly stops returning internal sources that would be quite the breaking change, so would UUIDs changing internally. UUID's are part of the public API, they're not allowed to just randomly change without warning.
And as far as I can tell, transition_by_uuid was only added in the first place because transition_by_name existed (which it shouldn't).

Generally if a user is interacting with a transition, the API scope should reflect that.

By that logic, we should also add filter_by_uuid, scene_by_uuid, and input_by_uuid (so that all source types are covered) and remove/internalize source_by_uuid completely. But having a UUID solves the issue of ambiguity already, I don't think any of that is necessary.

@gxalpha gxalpha force-pushed the dont-get-transition-by-name branch from 5863da5 to 61308b4 Compare October 5, 2024 20:46
obs_get_transition_by_name is very problematic because transitions are
always private and their names aren't unique. This means that the method
iterates over all private sources and then takes the first that both is
a transition and matches the name we're looking for. However, this could
be from anywhere - it could be a frontend transition, but also a source
transition, quick transition, or even one from third-party plugins. This
is always never what is intended.

As such, this method (which should never have been added in the first
place) needs to go. In its place, obs_frontend_get_transitions returns a
list of all frontend transitions (which is usually what people are
looking for), and alternatively obs_get_source_by_uuid also provides
access to private sources.

While we're at it, obs_get_transition_by_uuid is basically a wrapper for
obs_get_source_by_uuid and not really necessary. UUID's are unique and
a source doesn't suddenly change its type, so if you have a transition's
UUID you can be pretty sure that when you do obs_get_source_by_uuid, it
will still be a transition.
@gxalpha gxalpha force-pushed the dont-get-transition-by-name branch from 61308b4 to 99234f6 Compare October 5, 2024 21:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Fix Non-breaking change which fixes an issue Code Cleanup Non-breaking change which makes code smaller or more readable
Projects
Status: Backlog
Development

Successfully merging this pull request may close these issues.

2 participants