[Warchamp7]

Description

A medley of clean up to the SceneTree widget.

Pile o' commits:

• Split out scene tree grid calculation
  • Moved this logic into it's own function rather than the hacky approach of forcing a resizeEvent which might have other side effects
• Remove empty eventFilter
  • Function is empty and always has been since it was added? Perhaps a misunderstanding of Qt from years ago.
• Remove deferred scenesReordered signal
  • This was added in UI: Fix scene tree event handling #3398 to hack around an issue where the order was incorrect in the multiview, which I believe was due to...
• Block signals while updating the grid list
  • This also fixes a bug where reordering the grid list caused OBS to swap to the scene at the target index and then immediately swap back to the scene being dragged. Visually this bug was invisible until libobs: Fix interrupted transitions #13088 fixed interrupted transitions.
• Restore old safety guard on selectionChanged
  • An old QTBUG allowed the scene to be deselected by clicking in the empty area of the QList. This was (allegedly) fixed at some point, however it is still possible to deselect the scene by clicking and dragging in the empty area now sooo we need this guard again.
• Simplify grid reordering logic
  • There's a lot going on here and I will type it up later.
• Adjust style refresh logic
  • Polish the widget instead of setting/clearing a stylesheet.
• Codestyle cleanup

Motivation and Context

Originally I was trying to chase down a UI deadlock a friend ran into when changing scenes. In the DMP I received from them, the main thread was stuck inside the SceneTree's drag event. I have yet to determine if the SceneTree was actually responsible in any way for that lock up, but looking around at what could have potentially caused it led me to the resizeEvent stuff.

How Has This Been Tested?

Tested all the functionality I changed. Reordered scenes in grid mode, adjusted the scene tree styling locally. Resized the dock before and after reordering items. Ensured Multiview behaved correctly in list and grid mode when making changes.

Types of changes

• Bug fix (non-breaking change which fixes an issue)

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

• Code cleanup (non-breaking change which makes code smaller or more readable)

Checklist:

• I have read the contributing document.
• My code has been run through clang-format.
• My code follows the project's style guidelines
• My code has been tested.
• All commit messages are properly formatted and commits squashed where appropriate.
• I have included updates to all appropriate documentation.

[PatTheMav]

Note

This is in context of commit 4dbc053. As stated in our contribution guidelines, all commits need to be "atomic" and represent a whole unit of change.

Just so I understand the logic right:

• You first take a pointer to the first of all selected items (because one could select multiple non-consecutive items and drag them).
  • What is the order of items in the list returned by selectedIndexes()? Is it order of actual selection or ordered by their row numbers in the original data model?
• Then you insert just this single item after the lastTargetRow, which will be -1 by default, thus representing the top of the list.
• Then you set the inserted item as the "current" item (so it is selected by default, which will also trigger a whole lot of other "selection changed" events, scene changes, etc.)

At a cursory glance (and proven by testing this code change), this leads to an endless loop: There is always a selected item in the scene list, so you take the current item and reinsert it at the first position.

This triggers the resizeEvent again, which... reinserts the same item and triggers the resizeEvent, and on and on it goes until it corrupts the stack pointer.

At the same time this also triggers scene switch events over and over again. With just two scenes I get this in my log:

info: User switched to scene 'Scene 2'
info: User switched to scene 'Scene'
info: User switched to scene 'Scene 2'
info: User switched to scene 'Scene'
info: User switched to scene 'Scene 2'
info: User switched to scene 'Scene'

And that's just from switching to "Grid Mode" without any reordering involved.

[Warchamp7]

The signal blocker should be preventing that sort of recursion. I'll investigate.

[PatTheMav]

Right, but it looks like commit 4dbc053 leaves the project in a broken state because only a later commit introduces the signal blocker changes. So you'd have to squash these commits to ensure that you still end up in a working state e.g. during bisecting.

[Warchamp7]

Ah okay, are you saying that commit has these issues or the PR as a whole exhibits this infinite loop? I was probably gonna squash the commits.

[PatTheMav]

Ah okay, are you saying that commit has these issues or the PR as a whole exhibits this infinite loop? I was probably gonna squash the commits.

Yeah squashing them in the right way will probably resolve this automatically. But per our contribution guidelines I'm checking code commit by commit as that's the expectation for initial (and final) reviews.

[Warchamp7]

In regards to the actual problem itself:

At a cursory glance (and proven by testing this code change), this leads to an endless loop: There is always a selected item in the scene list, so you take the current item and reinsert it at the first position.

Welcome to the wonderful world of Current Behaviour! We were getting lucky before by offloading this logic into the resizeEvent. Reordering scenes in grid mode on stable actually switches to the scene in the index after the dragged item, and then immediately back to the dragged item. Code that works vs code that doesn't break etc.

Commits have been squashed and should be properly atomic now.

[PatTheMav]

Will give it another quick spin, though IIRC it all worked on the last commit before.