ui/sceneeditor: improve switching between view modes - #1914
Open
jpue wants to merge 2 commits into
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
BUG A
Describe the bug / To Reproduce
Generaltab, click on the “Fixtures used in this scene” list to deselect (!) all fixtures.Expected behaviour
The fixture tabs displayed should not depend on the fixtures selected in the list, but only on the fixtures added to the scene.
(If this is the desired behaviour, please let me know. Personally, I would find it counterintuitive.)
Problem Analysis
In the
SceneEditor::slotViewModeChangedfunction, the fixtures to be added when creating the views depend on the list returned bySceneEditor::selectedFixtures()which – as the name suggests – only returns the fixtures currently selected in the list.Proposed Solution (first commit of this PR)
Add an optional parameter of type
boolto theSceneEditor::selectedFixtures()function to retrieve the complete fixture list without major code changes.BUG B
Describe the bug / To Reproduce
Generaltab.→ After removal, the fixture will still appear in the
All fixturestab. (1)Generaltab.→ After adding, a new tab will be created for the fixture instead of adding it to the
All fixturestab/page. (2)Expected behaviour
(1) is annoying because the fixture will be added to the scene again, if you accidentally use the remaining fader to change the channel value.
Regarding (2), one could argue that this is a feature to inform the user about the newly added fixtures. However, I do not agree with this, and it is not mentioned in the documentation either. Therefore, I consider it a bug.
Problem Analysis
The corresponding functions
SceneEditor::addFixtureTabandSceneEditor::removeFixtureTabdo not differentiate between view modes and always assume the “tabbed fixture” mode.For this reason, these functions do not work properly in “All fixtures” mode.
Proposed Solution (second commit of this PR)
The simple solution would have been to always call
SceneEditor::slotViewModeChangedat the end ofSceneEditor::addFixtureTabandSceneEditor::removeFixtureTab, which would have completely regenerated all fixture tabs and therefore correctly reflected the changes.However, this would be computationally very expensive, especially for small changes (removing one fixture) to a large scene (with many fixtures).
For this reason, I implemented the more efficient solution, where only what needs to be changed is changed. To do this, I used code snippets from the existing methods
SceneEditor::addFixtureTab,SceneEditor::removeFixtureTabandSceneEditor::slotViewModeChanged.The resulting code duplication is not ideal, but I could not find a clean way to refactor this, as there are small differences in the control flows of the above functions.