11242: Docks should remember their position

[krasko78]

Resolves: #11242 Resolves: #18345 (however the height is not retained because the Mixer auto-calculates it) Resolves: #11218 Resolves: #26678 Resolves: #21148 (partially; the pen width is a different issue)

I am proposing a couple of simple changes to fix the problem with the docks not remembering their position which is a big nuisance:

1. When an undocked panel is closed and reopen, it will not reappear undocked. When a docked panel, docked into a sidebar that is not its default, is closed and reopen, it will reappear docked but in its default sidebar. The problem here is that when a panel is to be shown, we simply search for a suitable panel that can host it and move it there. I discovered that if we simply call the open method on the panel like we do with the non-panels, this will call the show method on the widget. From there KDockWidgets has all the functionality to restore the widget to it position prior to it getting closed. So I am proposing that we call the open method and bypass our destination-panel-search logic with the following two exceptions:

• When a widget was not closed but is not the active tab, simply calling the show() method is not enough: it would not change the active tab although the contents of the widget would become visible. So I've added a check for this case and we just need to have the frame activate the tab with the widget. Otherwise we call the show() method which will do its best to restore the widget and will also activate it in the frame.
• I've added a check for whether the widget being shown, has a last position stored for it before it got closed. If it does, we are good but if it does not, there is no way we can restore it to its previous position. I am not sure if we will ever hit this scenario, most likely when starting the app for the first time or after resetting to factory settings but even then maybe our default workspace has a last position for each widget. I don't know. In any case I decided to handle this scenario. The show() method in this case would show the widget undocked with some size which makes sense. However, in this case it is better to fallback to our destination-panel-search logic instead. To this end, I unfortunately had to add a small method to DockWidgetBase which is third-party code but I didn't find another way to do it - everything around the last position is private and there are no no virtual methods to hook into...

2. When an undocked dock is closed and reopen, it won't return to its position provided it is restored as an undocked widget (this is the case with non-panels such as the toolbars and with panels for which a destination panel cannot be found, e.g. the Mixer on my machine). The cause is that when we are closing the widget, we call forceClose() which turns on a flag called m_isForceClosing in DockWidgetBase.cpp::873 and this later on prevents the code from capturing its current position (i.e. its last position) in DockWidgetBase::Private::close() around line 650. I am proposing that we call close() instead of forceClose() so the current position is captured correctly before the widget is closed so it can be restored to it later on when shown.

Most widgets have size constraints and others also have some auto-size logic built into them. For example, the vertical panels are restricted to 300px in width meaning that if you resize them more or less than this width when undocked, if you close and reopen them, they will return to this width. The horizontal panels, such as the Mixer, have a maximum height of 520px. Moreover, the Mixer panel and the Playback controls have additional size constraints. [Personally I am not a fan of these constraints (more freedom would be nice). There is an unfinished story to lift those constraints or bring them to less limiting values.] So when restoring a closed widget it may not have its pre-closure size, but at least the position should be retained.

• [X] I signed the CLA
• [X] The title of the PR describes the problem it addresses
• [X] Each commit's message describes its purpose and effects, and references the issue it resolves
• [X] If changes are extensive, there is a sequence of easily reviewable commits
• [X] The code in the PR follows the coding rules
• [X] There are no unnecessary changes
• [X] The code compiles and runs on my machine, preferably after each commit individually
• [ ] I created a unit test or vtest to verify the changes I made (if applicable)

[zacjansheski]

In this PR, after factory reset, percussion panel and mixer panel can be stacked as opposed to tabs in same dock area

https://github.com/user-attachments/assets/c5cda344-453a-4e35-81a3-986cbb6463d3

[zacjansheski]

Undo/Redo spans right side of window vertically after "restore default layout" I've seen this bug before, but it seems easier to reproduce in this PR (I don't actually need to put a panel on the right, just open a score and "Restore Default Layout")

https://github.com/user-attachments/assets/1003839a-1014-4c7f-a44d-56d545d3ebee

[krasko78]

Thanks, Zac. The undo/redo issue is reproducible without this PR too.

As far as the other issue is concerned: restoring a dock at lastPositions if valid has now uncovered this behavior. If all horizontal panels are initially hidden, loadPanels cannot find a destination panel for them and uses addDock to add them according to their locations. Unfortunately addDock won't tab any docks. So in the end all horizontal panels end up stacked one over the other (the issue in Zac's video).

I tried a bunch of things and the only thing that seems to work well is in the new commit I just added. Simply the idea is to add all panels as visible so they can be positioned properly and then the hidden panels to be closed:

1. loadPanels will make addDock add the panel as visible initially even if it is hidden. This is so a frame gets created for it and the panel can then be returned by findDestinationForPanel as the destination panel for the other panels that should belong together (same group name and location).

2. In addPanelAsTab I had to remove the check whether the panel being added is not visible.

3. After all panels have been loaded, the hidden ones are closed in reverse order to preserve the order in which they were tabbed.

4. I also changed loadPanels and findDestinationForPanel so that when loading a panel, only the previously added panels are searched as the potential destination. This is to preserve the order of the panels. So for example if the Mixer panel is hidden and followed by the Keyboard panel which is set to be visible, Keyboard will be tabbed to the Mixer and not the other way around. The difference should become visible when the Mixer panel gets shown: with this change it will appear before the Keyboard (original order is preserved) whereas without this change, it will appear after the Keyboard (order in which they are defined in the QML is not preserved).

This PR also breaks new (unknown) panels for which the saved JSON layout does not know about: any unknown panels will still appear docked thanks to this PR but they won't be tabbed with the other panels they belong to if: a) all are hidden; and b) there exists a previoualy saved layout (i.e. this is not a clean start). I think this should not be a show-stopper for this PR though as the benefits are worth this inconvenience.

[cbjeukendrup]

@krasko78 Sorry, another rebase is needed...

I've reviewed this, and have no objections, but I'm not confident enough to approve it, also considering the consequences for new unknown dock widgets, mentioned above. So @mathesoncalum it would be great if you could take a look as well.

[krasko78]

Fair enough, Casper. Docks are tricky. I'll rebase and review. Maybe now I'll be able to look at it with fresh eyes and figure something out.

[krasko78]

@cbjeukendrup @mathesoncalum Next round! So the problem was in that the horizontal panels (Mixer, Piano keybord, etc.) are all hidden by default so in DockWindow::loadPanels() they would all be added with addDock since no suitable destination panel would be found. addDock would not tab so all horizontal panels would end up stacked instead of tabbed. My solution for this was to add all panels as visible and then close the ones that should be initially closed. This worked for the existing panels but not for any unknown panels which still suffered from the same problem: if all of their buddies were closed, no destination panel would be found for them and they would be stacked instead of tabbed to any panel.

To me, this all means that we need a way to be able to tab a panel to a closed one and I have found a way to do it. The core change I have made is to DockPanelView::addPanelAsTab which now:

1. Will copy the placeholder/layout item of the destination panel into the panel being added as a tab. The placeholder/layout item is essentially where the widget is hosted. When two or more widgets are tabbed, they share the same placeholder/layout item.
2. In addition to a host, we also need to set the tabIndex of the widget within its host and to make sure that the widget is set as docked rather than floating. This is taken care of by the saveTabIndex call.

lastPositions is a core part of every widget and stores all the data needed to restore the widget to its last position when it gets open so we just manipulate it properly.

The only thing left for addPanelAsTab to do is call restoreToPreviousPosition if the panel being added should be open. This will materialize/move the panel to what is stored in lastPositions. restoreToPreviousPosition takes care of everything: no matter if the panel is floating or not, or hosted in another placeholder/layout item, it will be properly docked and tabbed into the placeholder/layout item of the destination panel. It will also automatically create a frame if no other widget in that placeholder/layout item is currently open. Both DockWidgetBase::show() and DockWidgetBase::setFloating use restoreToPreviousPosition.

Another change worth mentioning is in DockPanelView::isTabAllowed where we had if (floating()) {. During my testing, I discovered this was not working properly. One of my test cases was to have the mixer panel floating. In this case my expectation was that an unknown panel would not be tabbed to it but interestingly floating() was returning false. So I've come up with a more complex condition for whether the destination panel is floating or not (lines 282-288).

In DockWindow.cpp: loadPanels() will still look for a destination panel for the current panel only among the previously processed panels. Since now addPanelAsTab is able to add to closed panels, it is no longer necessary for loadPanels to add all panels as visible and then close the hidden ones. It is sufficient to find even a closed matching panel and tab the current panel to it. So what happens is:

• the Palettes panel will be added via addDock since it is the first panel in its group and then all vertical panels will be tabbed to it (i.e. all vertical panels will be tabbed together) no matter which ones are visible and which ones are hidden.
• for the horizontal panels the Mixer panel will be created with addDock (first in the group) and then all others tabbed to it.
• in handleUnknownDock we do the same: look for a suitable destination panel even if it is closed and tab the unknown panel to it (and its group).

In handleUnknownDock I added a null check for unknownDock because I was seeing a null pointer dereference warning on line 531 (where we have unknownDock->location()).

I have performed extensive testing of as many scenarios as occurred to me, including when no layout is present in the workspace (factory reset). I tested with an unknown panel both set to initially visible and hidden, reseting to the default layout, floating and docked horizontal panels. It looks good to me.

QA: You can test with an unknown panel by hacking the layout. Unzip your workspace file, open ui_states in a text editor, look for a panel name, e.g. instrumentsPanel and change it to something else that does not contain instrumentsPanel in it. So for instance "wabadabadoo" and "instrumentsPan" will work but "instrumentsPanel2" and "instrumentsPanelFake" won't work. Make sure to replace all instances of the panel name! Finally zip back everything as the original workspace file and replace it with your version.

[krasko78]

Did we solve the issue with stacking panels? This was the first thing I saw when I tried the build today (just by opening the mixer, percussion, and keyboard panels):

The latest changes you just reviewed address exactly this issue. I was to reprodute it only once when I switched to my default workspace which I don't use. My assumption is that this workspace had the panels saved as stacked from some time before my latest changes and they just got restored this way from the saved layout. I was not able to reproduce it once I tabbed them manually, then switched back and forth between branches, reverted to factory setting, nor did I ever see it in my non-default workspace (once I added the latest changes). Is it possible that you had them saved as stacked? I don't have a clue what I can do here, maybe just keep an eye and see if it will happen again for you, me or Zac?

[krasko78]

Sure! In the meantime I'll do some more testing to see if I'll be able to reproduce it again.

[zacjansheski]

To get the panels to stack, you can restore default layout in nightly then open this PR

https://github.com/user-attachments/assets/29f013a8-ef20-4d1d-ac15-246bc4479cd3

[krasko78]

You guys do enjoy torturing me, don't you? Alright, alright... 😁

[krasko78]

@mathesoncalum @zacjansheski Here's what's happening here:

Prior to this PR:

1. DockWindow::loadPanels cannot tab closed panels so it stacks them. This applies to all bottom panels because they are all hidden in the default layout.
2. When opening a closed panel, MuseScore unconditionally tabs the new panel into the first matching visible panel (see DockPageView::setDockOpen). This has the following consequences:

• a. It hides the fact that the closed panels have been stacked; it ignores the saved layout and just forces the new panel as a tab
• b. It always opens a closed panel as tabbed no matter whether it was floating or not and where it was docked/floating before it got closed. This is what provoked this current issue 27695. Bottom line is: the panels do not remember their last positions.

This PR: 3. DockWindow::loadPanels can now tab closed panels into another closed panel. For the default layout, all bottom panels will be tabbed with the Mixer panel since it is the first to be processed. 4. When opening a closed panel, MuseScore respects the saved layout and restores the panel to the exact same state and place where it was before it got closed.

Combining both: Now imagine what happens if you restore the default layout in a build without this PR and then start MuseScore with this PR. When you restore the default layout, the panels will be closed and stacked - see point 1. above. When you close MuseScore, the layout will be saved into the workspace. When you then open MuseScore with this PR, it respects any saved layout and will show the panels as stacked because this is how the other build saved them. Again, the fact that the panels appear as tabbed in the other build is only due to its totally incorrect behavior of unconditionally tabbing a panel on open.

Conclusions: I don't think there is anything we can do here but accept it as a one-time thing when switching between non-this PR MuseScore and this-PR MuseScore. When we release this PR (I hope we will :), we can warn the users of the possibility of encountering this issue. If this happens, it will be sufficient for the users to either tab all panels together manually or restore the default layout. This will be a one-time thing to do. From then on, everything should be fine. I don't believe this will be an issue for many users because most of them will have arranged the panels as they want, meaning they won't have the default layout. Instead, they will have their own layout which will be saved in the workspace and respected by the new build. Only the panels that the users never opened since the last time they restored the default layout, will appear as stacked.

[bkunda]

I’ve been discussing this PR with the internal team, and there are a few concerns that mean we are unfortunately reluctant move forward with merging it right now.

One key issue is that this change can cause all panels to appear stacked by default. On upgrade, that could—for some users—leave much of the score obscured. If that’s the first impression after updating, it risks sending an undesirable message, even if the workaround is relatively simple.

That said, I want to emphasize how much I appreciate the work you’ve put into this PR, @krasko78 (as with all your excellent contributions!). The challenge here isn’t so much with your implementation, but with the limitations of KDDockWidgets. Until we’re able to update that dependency, we’re unfortunately constrained in how we can tackle issues like this in a robust way.

We’ll keep this PR in consideration, but for now our focus has to remain on the 4.6 release. I wanted to share this update so you weren’t left waiting any longer.

[krasko78]

You are telling me that a minor one-time cosmetic inconvenience for some users that is super easy to fix is a bigger problem and sends a worse message than the panels not remembering their positions for years on end and more time to come? Fair enough. You make MuseScore's decisions.

[mrbumpy409]

Yeah, I'm far more annoyed by the fact that I have to move my note entry palette to the left side of the window every single time I use the app. There has to be some way to mitigate the initial cosmetic convenience when users upgrade to the MuseScore version containing the fix. Perhaps on first upgrade, docks could just be set to their default positions? I mean, that's what's happening to me every time I start the app anyway...