Skip to content

Conversation

@tytan652
Copy link
Collaborator

@tytan652 tytan652 commented Feb 18, 2025

Description

Began to split transitions and transition duration storage from the UI.
Also makes the frontend code rely more on transitions UUID even if those are not preserved between sessions for now.

While this PR remove any kind of reliance on index of a transition, it does not overhaul things to rely only on UUID so there is still reliance on the transition name as an identifier. Same for retaining which transition was inserted last.

Motivation and Context

After multiple tries, I finally managed to separated the transitions data from this combobox, now it needs reviews and testings…

The awaited sequel of #7278, but it's the prequel of the expected sequel.

How Has This Been Tested?

Please tests

Types of changes

  • Tweak (non-breaking change to improve existing functionality)

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.

@tytan652 tytan652 added Seeking Testers Build artifacts on CI Enhancement Improvement to existing functionality UI/UX Anything to do with changes or additions to UI/UX elements. labels Feb 18, 2025
@tytan652
Copy link
Collaborator Author

tytan652 commented Feb 18, 2025

@PatTheMav, please just say to avoid mutator even if we check if present beforehand.

Using diffs for every mutator use make the review less readable and uses too much space that github ends up hiding stuff.

@tytan652 tytan652 marked this pull request as draft February 18, 2025 18:24
@PatTheMav
Copy link
Member

@PatTheMav, please just say to avoid mutator even if we check if present beforehand.

Using diffs for every mutator use make the review less readable and uses too much space that github ends up hiding stuff.

I disagree, it is the correct tool for the job to highlight requested changes exactly where they are requested, including the context of the change. Expanding all changes takes a single click and that's a small price to pay for the improved clarity of changes and keeping discussions local to specific changes.

@tytan652
Copy link
Collaborator Author

I think you are mixing up the code selected and the diff that you explicitly type in the review. I'm talking about the second.

@tytan652 tytan652 force-pushed the transitions_dock_separation_part_1 branch from 68ec976 to 6985616 Compare February 18, 2025 19:22
@tytan652 tytan652 marked this pull request as ready for review February 18, 2025 19:22
@tytan652 tytan652 force-pushed the transitions_dock_separation_part_1 branch from 6985616 to 0d91fd9 Compare February 18, 2025 19:27
@tytan652 tytan652 requested a review from PatTheMav February 18, 2025 19:43
@tytan652 tytan652 force-pushed the transitions_dock_separation_part_1 branch from 0d91fd9 to 4894a29 Compare February 21, 2025 15:50
@tytan652 tytan652 requested a review from PatTheMav March 1, 2025 18:41
Copy link
Member

@PatTheMav PatTheMav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think all my comments were addressed, right?

@tytan652
Copy link
Collaborator Author

I think all my comments were addressed, right?

The conversation(s) I did not mark as resolved by myself were waiting for your feedback that if the changes to solve it were good for you.

@tytan652 tytan652 force-pushed the transitions_dock_separation_part_1 branch from 4894a29 to e1e6f76 Compare April 3, 2025 13:41
@tytan652
Copy link
Collaborator Author

tytan652 commented Apr 3, 2025

Just a rebase, I still did not found the source of my issues with the signal handler mutex and websocket…

Copy link
Member

@Warchamp7 Warchamp7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As noted by tytan above, this currently crashes when run. I'm putting a review to block merging as an extra safety.

@tytan652 tytan652 force-pushed the transitions_dock_separation_part_1 branch 2 times, most recently from e098544 to e26a134 Compare April 19, 2025 13:51
@tytan652 tytan652 requested a review from Warchamp7 April 19, 2025 13:52
@tytan652
Copy link
Collaborator Author

The issue at startup should be now resolved.

@tytan652 tytan652 force-pushed the transitions_dock_separation_part_1 branch from e26a134 to 2f93c48 Compare April 19, 2025 14:16
@tytan652 tytan652 added this to the OBS Studio 32.0 milestone Jun 16, 2025
@tytan652 tytan652 force-pushed the transitions_dock_separation_part_1 branch from 2f93c48 to 7fe4981 Compare July 18, 2025 05:36
@tytan652 tytan652 force-pushed the transitions_dock_separation_part_1 branch from 7fe4981 to 7b3b06e Compare August 6, 2025 11:45
Copy link
Member

@PatTheMav PatTheMav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just tested this locally and for some reason the button to delete a transition is always disabled, even for transitions past the default "Cut" and "Fade" ones.

After relaunching OBS, the button to reach the transition properties or to rename it also became disabled for all of them.

@PatTheMav PatTheMav moved this from Ready to In review in OBS Studio 32.0 PR Considerations Aug 7, 2025
@tytan652 tytan652 force-pushed the transitions_dock_separation_part_1 branch from 7b3b06e to c78a87b Compare August 8, 2025 05:22
@tytan652
Copy link
Collaborator Author

tytan652 commented Aug 8, 2025

I feel dumb to not have caught that I forgot to "set transition" while setting the current transition.

Edit: I will squash the two last commit ("reduce" and "set the transition" with "replace combobox direct access") once everything is good.

@tytan652 tytan652 requested a review from PatTheMav August 8, 2025 05:24
@tytan652 tytan652 force-pushed the transitions_dock_separation_part_1 branch from c78a87b to fe947fd Compare August 8, 2025 05:38
@tytan652 tytan652 force-pushed the transitions_dock_separation_part_1 branch from 36802c8 to 2128d1f Compare August 8, 2025 14:19
Copy link
Member

@PatTheMav PatTheMav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm afraid this still doesn't work right, as deleted transitions are retained in the dropdown:

  • Given transitions A,B,C,D, and E
  • Deleting E retains E in the list
  • Deleting D retains D in the list
  • Trying to delete D or E again asks for confirmation to delete C

Indeed the rename and properties actions also are associated with transition C, even when the retained transition D or E are selected.

@tytan652 tytan652 force-pushed the transitions_dock_separation_part_1 branch from acede6c to 9c085c8 Compare August 9, 2025 18:06
@tytan652 tytan652 force-pushed the transitions_dock_separation_part_1 branch from 9c085c8 to b12c7d8 Compare August 9, 2025 18:22
@PatTheMav PatTheMav moved this from In review to Done in OBS Studio 32.0 PR Considerations Aug 11, 2025
@RytoEX
Copy link
Member

RytoEX commented Aug 20, 2025

I'm afraid this still doesn't work right, as deleted transitions are retained in the dropdown:

Has this changed since then?

@tytan652
Copy link
Collaborator Author

Yes, it was fixed

@tytan652 tytan652 force-pushed the transitions_dock_separation_part_1 branch from b12c7d8 to 29903d0 Compare August 22, 2025 04:28
@RytoEX RytoEX dismissed Warchamp7’s stale review August 22, 2025 20:02

The crash was reportedly fixed.

@RytoEX RytoEX merged commit ffcc3ac into obsproject:master Aug 22, 2025
15 checks passed
@tytan652 tytan652 deleted the transitions_dock_separation_part_1 branch August 23, 2025 05:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Enhancement Improvement to existing functionality Seeking Testers Build artifacts on CI UI/UX Anything to do with changes or additions to UI/UX elements.

Projects

No open projects

Development

Successfully merging this pull request may close these issues.

4 participants