Undocked panels should remember their position/size

[krasko78]

Resolves partially: #11242 & #26678 (partially because docked panels are still a problem)
Resolves: #18345 (however the height is not retained because the Mixer auto-calculates it)
Resolves: #27570

---

NOTE: Below is the original comment for this PR, prior to various force-pushes and discussions.

---

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.

• I signed the CLA
• The title of the PR describes the problem it addresses
• Each commit's message describes its purpose and effects, and references the issue it resolves
• If changes are extensive, there is a sequence of easily reviewable commits
• The code in the PR follows the coding rules
• There are no unnecessary changes
• 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)

[mathesoncalum]

Thanks very much @krasko78!

Ready for testing now @zacjansheski - need to make sure we don't reintroduce #26009 or similar.

[zacjansheski]

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

video1283134326.mp4

[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")

video1103005246.mp4

[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.

[mathesoncalum]

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):

[mathesoncalum]

Ok, now that 4.6.4 is behind us we can return to this...

For context, behind the scenes we have been actively investigating the best approach to a dock system update. As you can probably imagine it's quite a big/tricky task with a lot of uncertainties but one thing we'd really like to aim for is a system that doesn't involve our own hacked/patched version of KDDockWidgets.

For this reason, and reasons stated previously in this PR thread, I think we can leave the lastPositions-related stuff for now, drop the original second commit, and simply do the forceClose -> close change for now.

This will solve one of, if not the, most frequently reported issues relating to dock positions and minimizes the risk of us having to do follow-up patches to KDDW.

Huge thanks for your work on this one @krasko78

[cbjeukendrup]

Happy to approve this, although it's sad that this is all that remains after the long history of this PR. But the future is bright (hopefully 😅).

[krasko78]

Did I write most of the above comments? Oh, boy, it is really time to stop the drugs or change the dealer. Hopefully this will earn me the next Nobel prize for Literature.

Changing forceClose to force is a step in the right direction but alone it is not sufficient for anything. It is DockPageView::setDockOpen that forces the panels to open as tabs ignoring their last positions. So even if we are now properly saving the last positions, if we are ignoring them when opening the panels, what's the point?

[mathesoncalum]

@krasko78 if you look at #18345 and #27570 (probably more), these issues relate to undocked panels only and the root cause is forceClose. The first issue specifies "the mixer returns to the original undocked position", the second issue specifies "it appears on the 2nd display". Changing to close is sufficient to solve this issue in these cases. Additional steps would indeed be required to prevent the undocked panels from tabbing when you reopen them - but that's a different root cause/issue.

Quick video to demonstrate what this actually solves - sound on (voice reveal!):

forceClose.mov

[krasko78]

Ah, I see. Looks like this is the case when all vertical or horizontal panels are either undocked or invisible. I am glad at least something is getting fixed. Thanks, @mathesoncalum!

[zacjansheski]

Tested on MacOS 15, Windows 11, Ubuntu 22.04.3. Approved
#27570 FIXED