Fix for Issue 11218: Improve workflow of Panels #15675

hanoixan · 2023-01-01T20:21:10Z

Resolves: #11218
Resolves: #11242

This PR changes the behavior of toggling and open/closing tabbed panels. It affects the open/close/toggle behavior of both horizontal and vertical tabbed panels. New behavior:

Hotkey toggles (served by toggleDock()) will only close an already created tabbed panel if it is selected. If it is not selected, it will be selected. If it has not been created, it will be added as a new tab and selected.
General panel behavior (served by setDockOpen()) will always close a panel, but for opening, it will only create a new panel if it doesn't yet exist. Otherwise it will select an existing tabbed panel on open.

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)

hanoixan · 2023-01-02T16:36:52Z

Note: there was a previous attempt at fixing this issue. I provided this fix because it looks like that person is no longer active with the issue. My fix covers both horizontal and vertical panels, and does not require any special case for tabs--it's meant to be a general solution for any tab.

src/appshell/view/dockwindow/dockpanelview.h

hanoixan · 2023-01-05T02:59:35Z

src/appshell/view/dockwindow/dockpageview.cpp

 DockPanelView* panel = dynamic_cast<DockPanelView*>(dock);
 if (panel) {
- DockPanelView* destinationPanel = findPanelForTab(panel);


There was an issue with using findPanelForTab, as it returns the default location for a tab (the first it finds in the allowed panels), not its current location. I've renamed this method findFirstPanelForTab(), and created a new method findCurrentPanelForTab(), which searches for the tab in all valid panels.

hanoixan · 2023-01-05T03:01:18Z

src/appshell/view/dockwindow/dockpageview.cpp

- return;
+ if (panel->isOpen()) {
+ if (panel->floating()) {
+ shouldOpen = !panel->hasFocus();


The calls to hasFocus() are to meet the requirement that the hotkey should first focus the panel (if it's open and not focused), and only then is it ok to close it.

hanoixan · 2023-01-05T03:02:23Z

src/appshell/view/dockwindow/dockpageview.cpp

- } else if (tabNotCurrent) {
- destinationPanel->setCurrentTabIndex(tabIndex);
+ if (!panel->isOpen()) {
+ panel->open();


As it turns out, if you simply open a closed panel, it will open at its last closed location, which is great.

hanoixan · 2023-01-05T03:03:47Z

src/appshell/view/dockwindow/internal/dockbase.cpp

@@ -557,11 +557,9 @@ void DockBase::applySizeConstraints()
 m_dockWidget->setMaximumSize(maximumSize);

 if (KDDockWidgets::FloatingWindow* window = m_dockWidget->floatingWindow()) {
- window->setMinimumSize(minimumSize);


If these lines aren't removed, they will limit the size of the opened floating window to be smaller than it was last sized for.

hanoixan · 2023-01-05T03:05:37Z

thirdparty/KDDockWidgets/src/private/DragController.cpp

@@ -329,6 +329,12 @@ void StateDragging::onEntry()
 void StateDragging::onExit()
 {
 m_maybeCancelDrag.stop();
+


Previously saveLastFloatingGeometry() was only called when starting the drag. This means only the previous rect was saved. So, save it when the drag is finished as well.

hanoixan · 2023-01-05T03:07:22Z

thirdparty/KDDockWidgets/src/private/WidgetResizeHandler.cpp

@@ -128,6 +129,14 @@ bool WidgetResizeHandler::eventFilter(QObject *o, QEvent *e)
 updateCursor(CursorPosition_Undefined);
 auto mouseEvent = static_cast<QMouseEvent *>(e);

+ // When we stop dragging a floating window which has a single dock widget, we save the position


It also turned out that resizing the window wouldn't update the saved rect, so I added this.

bkunda · 2023-01-05T14:52:51Z

Ok this is starting to look very good!

I've made my observations here: https://www.dropbox.com/s/w255zb15o6h5mrk/Screen%20Recording%202023-01-05%20at%203.41.42%20pm.mov?dl=0

I would also add that I do not believe that a selected/opened panel is receiving keyboard focus at the moment, but I'd really like @shoogle to have a look at this as well to ensure everything still meets our keyboard navigation rules.

And it's probably worth mentioning that I'm currently only testing this on macOS. It should also be tested on Windows and Linux before we merge it. Tagging @DmitryArefiev and @abariska to have a look at as well when they are back from vacation.

Many thanks!

cbjeukendrup · 2023-01-06T00:42:36Z

It looks like something strange happened while rebasing... I see some commits that shouldn't appear in this PR. I would recommend doing git pull --rebase upstream master (if upstream points to the "musescore/MuseScore" repo), followed by git push -f. (In general, for pull request branches, we prefer rebasing over merging.)

Regarding the changes in thirdparty/KDDockWidgets: normally we don't like to make changes to thirdparty code, but in this case it is obviously required. It would be good to open a pull request for these changes at the KDDockWidgets repository too (https://github.com/KDAB/KDDockWidgets), probably against their 1.7 branch. They also have a 2.0 branch, which is such a big rewrite of the whole library that the changes are probably not applicable anymore.

hanoixan · 2023-01-06T05:53:59Z

@bkunda That vertical dock bug is interesting. It appears to be caused by the way that KDDockWidgets' layout algorithm works. It attempts to steal space from immediate neighbors when inserting an item back into a list of items. I think that only works if you add the items in the reverse order that you removed them.

For example, consider a vertical list of panel items A, B, C (top to bottom) each sized to be 300px high.

remove B, and it will give equally to its neighbors, so A and C are now both 450px high.
remove A, and you're left with C being 900px high.
now add back in C, and it's 900px high (correct)
but now instead of adding A then B (the proper reverse order), add B then A.
B goes in. It's last size was 300px, so it will steal it from C. B is now 300px, C is 600px high.
A goes in. It should be inserted at the top, and it's last size was 450px. So it will attempt to steal from its neighbor.... BUT, the neighbor is only 300px, so it zeroes the neighbor B, and takes an additional 150px from C.
And that's how you end up with a 450px A, a 0px B, and 450px C.

You can try the above using, e.g., Properties, Palette, and Instruments panels for A, B, C, and you'll end up with a squished Palette.

So, IMO KDDockWidgets has a bug in how layout happens on re-adding an item. I think a solution would be for each item to steal from the largest of all neighbors until the neighbors are equal sized, at which point it steals from all proportionally. That would be run on each side of the insertion.

I'm new to this code, so I could be missing some other way to configure the behavior.

@cbjeukendrup I'll take a look, thanks for bringing that up. Looks like I may have more changes to make to that package. I want to make sure I'm not fixing behavior that MS currently depends on, but these do feel like bugs.

bkunda · 2023-01-06T09:28:56Z

I think a solution would be for each item to steal from the largest of all neighbors until the neighbors are equal sized, at which point it steals from all proportionally. That would be run on each side of the insertion.

I'll look forward to seeing how this works in practice! Keep referring to @cbjeukendrup for technical questions as
needed. And thanks again 🙏🏻🙂

src/appshell/view/dockwindow/dockpageview.cpp

…istence.

hanoixan · 2023-01-07T21:29:47Z

@bkunda Here's a video with the new LongestNeighboursFirst strategy for adding in a panel. I think it's questionable whether this is better or worse than just equalizing all relative sizes when adding a new panel (which can be added with less code changes). I think it's better because it favors the new panel that you want to see, but I can see the case against adding a new strategy for third party maintenance reasons.

If you think it's worth committing, I can do that next.

Longestneighborsfirstdemo-Ms-11218-1.mp4

bkunda · 2023-01-09T15:33:11Z

@hanoixan it's a nifty solution, but I wonder how it would go if the selection filter were also included in the mix?

The more I see this in action, the more I think that we should – in the case of vertically-docked panels that all end up being closed – simply re-open them in their default (i.e. full-height) position. The necessity of returning a half-docked panel to its former position is arguably voided when all the panels are closed, because you can't reopen a single previously half-docked panel in any position other than full height.

So I don't think we need to commit this latest solution. Your previous solution was all working fine, and just needs the vertically-docked panels to be reopened at full-height if they are part of a set in which all panels are closed 🙂.

hanoixan · 2023-01-10T03:57:01Z

@bkunda It works ok with the selection filter. When you say and just needs the vertically-docked panels to be reopened at full-height if they are part of a set in which all panels are closed, it should be doing that right now as is. Adding a dock when there's nothing else opened there always makes it full height. Did you mean something else?

The other option I thought of was that when a dock gets opened, it would just force an equalization of all open docks after it's added. Given that toggling a panel closed from the middle of two neighbors steals from them equally, this almost feels like a symmetric behavior.

bkunda · 2023-01-10T15:22:51Z

it should be doing that right now as is.

Yes, except without squashing any panels down to the very bottom of the window (as it does in my video at 1:41).

In terms of solving the problem I've identified in my video above, I think having all the panels open at full height in this particular situation is likely to be simplest. In particular, I'm thinking that:

If a docked panel (A) in the lower vertical position is closed, and then its ‘parent‘ (i.e. full-height) panel (B) is also closed, when the user then opens panel A it makes more sense for it to assume the full-height position, because we don't support panels being opened at half-position without a full-height parent (B in this example has not yet been re-opened).
In this example, if panel B was the first one to be re-opened (at full-height), panel A – being the next one to be re-opened – should not then assume the lower vertical position (i.e. the reverse order of the starting position). The user never set this custom configuration, so we shouldn't assume it for them.

The only exception to this might be if the user opens the panels in the reverse order to which they initially closed them, but this really requires the user to have memorised their position before carefully triggering the right sequence of shortcuts to close and reopen them. I don't think it is necessary to support this kind of behaviour at the present minute (although my mind always remains open to changing if enough of a demand is shown). Our workspaces feature should be the place where users configure their precise arrangement of panels.

hanoixan · 2023-01-10T15:43:21Z

@bkunda I need some clarity on this. It sounds like what you're saying is that we would have these two situations and behaviors:

A panel is added to its original vertical place where there are no panels currently open -> it assumes full height.
A panel is added to its original vertical place where there is at least 1 panel open -> we make it full height by turning it into a new tab in that space.

If this is what you mean, I like it. It would feel clear that when you close a tab, it loses its exact representation. When you reopen it, you want to see it fully by default, but it's then up to you to set up anything more custom.

This aligns with the fact that this widget system doesn't let us recreate exact historical layouts easily (as shows in my video), so perhaps we don't try to promise that we can?

bkunda · 2023-01-10T15:53:13Z

A panel is added to its original vertical place where there are no panels currently open -> it assumes full height.

A panel is added to its original vertical place where there is at least 1 panel open -> we make it full height by turning it into a new tab in that space.

Yes...almost. I wouldn't want to lose what you've already built though, whereby if two panels are stacked on top of one another, and the user closes the lower panel: if they immediately then re-open that panel, it opens in its last position, not at full-height. Re-opening at full height only makes sense if the parent panel is no longer open.

hanoixan · 2023-01-12T04:55:24Z

@cbjeukendrup Following @bkunda's request above, I would like to be able to query whether the last parent that owned a panel is the current parent that will own it, when calling DockBase::open(). I'm not able to follow the relationship, and wondering if you have any tips on where I should start?

edit:
I went back and read your advice on Discord: So it's not like that one dock widget is the parent of another; they are all equal and the "frame" is their common parent. Does this mean that we have no way presently of tracking when a "parent panel" changes, because the docked panel above another isn't its parent, it's just a sibling under that larger dock space?

If that's true, it seems like we either have to invent some model for tracking that, or take the easier road of appending the tab to the larger space's tabs on any re-open. Open to other options of course.

cbjeukendrup · 2023-01-12T21:32:10Z

Does this mean that we have no way presently of tracking when a "parent panel" changes, because the docked panel above another isn't its parent, it's just a sibling under that larger dock space?

That's right, if I understand you correctly. The panels are all siblings, regardless of whether one is tabbed together with another or that they are stacked vertically or next to each other horizontally. The Frame (which is a concept that's never really visible to the user) is their parent. (Well, that is, there is a hierarchy of frames, and basically adjacent panels that from one row or column or tab group are in the same frame.)
Maybe you can check whether the Frame is different?

hanoixan · 2023-01-13T04:36:14Z

@bkunda

I think I've been thrown off a bit by the parent panel requirement. I think this can be much simpler: for a given frame, if the panel being opened was the last one closed for that frame, open it as usual, because you're guaranteed that it will show up in the right place. Otherwise, open full size as a new tab. Does that sound right?
My changes that first establish dock focus before closing are incorrect I think. I currently use DockBase::hasFocus() and ::setFocus(), and I don't think these are tied into any managed focus system. It gets set and stays at that value, even if I select other windows. I think there's probably something else I should be using. In the current feature branch, you can repro this by trying to toggle something right after startup. It will require two toggles to disappear a panel that is visible on startup, and then it will only ever require one for that panel no matter what you focus on.

But on a related note, when I think about having to toggle twice to make a panel disappear, because it wasn't in focus on the first press, it makes me wonder if it will be perceived as a bug, and annoy users.

hanoixan · 2023-01-18T15:52:09Z

@bkunda Ping from above message. I just want to know:

whether you think the following would be an equivalent implementation for panel opening behavior: if the panel being opened was the last one closed for that frame, open it as usual, because you're guaranteed that it will show up in the right place. Otherwise, open full size as a new tab.
if you're ok with me removing the current focus implementation because I'm pretty sure it's incorrect given that everything thinks its focused even when its not.

Just trying to get this issue done, and maybe leave the focus issue as a separate one.

bkunda · 2023-01-20T10:56:11Z

Hi @hanoixan, sorry for the delay.

if the panel being opened was the last one closed for that frame, open it as usual, because you're guaranteed that it will show up in the right place. Otherwise, open full size as a new tab.

If "open as usual" means "open in its last opened position", then yes. This sounds right.

if you're ok with me removing the current focus implementation because I'm pretty sure it's incorrect

If you're not happy with your solution here, then yes perhaps remove it. We may need to fix the focus implementation as a separate issue (@shoogle might have more to say on this).

Thank you again!

DmitryArefiev · 2023-01-23T17:17:00Z

@hanoixan @bkunda Hi, I've tested PR's build on Win10 briefly.

As I understand, we should fix this bug before merging, right? (sorry if I missed something in the thread)

bandicam.2023-01-23.19-11-24-121.mp4

bkunda · 2023-01-24T10:09:18Z

As I understand, we should fix this bug before merging, right? (sorry if I missed something in the thread)

Yes that does need to be fixed before merging.

hanoixan · 2023-01-26T03:47:39Z

@DmitryArefiev That is the issue with adding panels back in the same order they were removed.

Similar to your example, consider two panels that each take up 500px of height in a frame, P1 (top) and P2 (bottom). P2 is removed retaining a size of 500px. P1 now has 1000px (expanding in P2's absence) when it is removed. P2 is brought back at 500px, but expands to 1000px to fill the height. P1 is going to be added back at its last known 1000px, and because of the neighbor growth strategy in KDDockWidgets, it will push P2 most of the way to the bottom. A more complex example is above at #15675 (comment).

The solution that I want to implement this weekend is to handle the following cases:

If the panel being opened again was the last panel in that frame to be closed, then let it open where it was.
If it's not, open it as a new tab in that frame.

There may be a better solution here, but I don't think there's a way to configure KDDockWidgets to fix this. It seems like this will need an extension to that lib's behavior.

A separate change is that I want to remove the focus behavior, because it's not working as expected.

DmitryArefiev · 2023-01-27T11:07:51Z

@hanoixan Got it. Thanks!

hanoixan · 2023-01-29T19:49:51Z

I've spent some time trying to find a way around this, but any changes to the rules for how widgets are inserted into tab layouts vs. adding new tabs is handled within KDDockWidgets, behind a private implementation. I don't think I can change the behavior unless I change KDDockWidgets.

That seems like something out of scope for this issue, and would take coordination from a higher level engineer who has more ownership over this dependency.

@bkunda @DmitryArefiev Unless someone can point me at a solution, I don't think there's anything more I can do here besides pull out the current focus handling code, and submit that.

DmitryArefiev · 2023-01-31T16:58:10Z

@RomanPudashkin Maybe you can help

cbjeukendrup · 2023-02-12T00:41:39Z

I was going through some old issues and came across #11242, which also seems to be fixed by this PR.

bkunda · 2023-02-14T15:38:14Z

I was going through some old issues and came across #11242, which also seems to be fixed by this PR.

It would be good if this fix does address that. I've assigned it to the 4.x shortlist. I think it might just need some input from @RomanPudashkin (when he's ready) to tidy-up one remaining aspect.

worldwideweary · 2023-07-13T02:59:42Z

In MS4.1, [properties panel] will revert position to left-sided tabs if the user previously situated it to the RHS of the screen and disabled then re-enabled its view. Will this PR fix that?...

cbjeukendrup reviewed Jan 2, 2023

View reviewed changes

src/appshell/view/dockwindow/dockpanelview.h Outdated Show resolved Hide resolved

cbjeukendrup requested a review from RomanPudashkin January 2, 2023 18:07

bkunda mentioned this pull request Jan 3, 2023

[MU4 Issue] Improving workflow of Panels (Palettes, Properties, Instruments and Selection Filter) #11218

Open

hanoixan commented Jan 5, 2023

View reviewed changes

cbjeukendrup reviewed Jan 6, 2023

View reviewed changes

src/appshell/view/dockwindow/dockpageview.cpp Outdated Show resolved Hide resolved

hanoixan added 14 commits January 7, 2023 09:46

Change toggle behavior of tabbed panels

b06e8aa

Fixed whitespace and braces issue.

559dd47

Modified names of new DockPanelView methods to remove "get".

55dd6e4

Panels now retain placement. Also fixed floating window geometry pers…

c1186f3

…istence.

Change toggle behavior of tabbed panels

0202c76

Fixed whitespace and braces issue.

2bb786a

Modified names of new DockPanelView methods to remove "get".

34e42e5

Fixed crash due to draggable being used after drop.

a5f9a1d

Panels now retain placement. Also fixed floating window geometry pers…

2dff068

…istence.

Fixed crash due to draggable being used after drop.

6da1aa9

Fixed rebase

2102044

Fixed rebase

3a11268

Panels now retain placement. Also fixed floating window geometry pers…

da493f9

…istence.

Fixed crash due to draggable being used after drop.

8e7545f

hanoixan force-pushed the 11218-toggle-panels branch from eeb027f to 8e7545f Compare January 7, 2023 17:29

DmitryArefiev self-assigned this Jan 20, 2023

bkunda added this to In progress in 4.x SHORTLIST via automation Feb 14, 2023

bkunda added the P2 Priority: Medium label Feb 14, 2023

worldwideweary mentioned this pull request Jul 31, 2023

[MU4 Issue] Palette search command should open palette if not already visible #12873

Open

igorkorsukov force-pushed the master branch 6 times, most recently from fa1f8d3 to 525a11a Compare February 14, 2024 09:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for Issue 11218: Improve workflow of Panels #15675

Fix for Issue 11218: Improve workflow of Panels #15675

hanoixan commented Jan 1, 2023 •

edited by cbjeukendrup

Loading

hanoixan commented Jan 2, 2023

hanoixan Jan 5, 2023

hanoixan Jan 5, 2023

hanoixan Jan 5, 2023

hanoixan Jan 5, 2023

hanoixan Jan 5, 2023 •

edited

Loading

hanoixan Jan 5, 2023

bkunda commented Jan 5, 2023

cbjeukendrup commented Jan 6, 2023

hanoixan commented Jan 6, 2023

bkunda commented Jan 6, 2023

hanoixan commented Jan 7, 2023 •

edited

Loading

bkunda commented Jan 9, 2023

hanoixan commented Jan 10, 2023 •

edited

Loading

bkunda commented Jan 10, 2023

hanoixan commented Jan 10, 2023 •

edited

Loading

bkunda commented Jan 10, 2023

hanoixan commented Jan 12, 2023 •

edited

Loading

cbjeukendrup commented Jan 12, 2023

hanoixan commented Jan 13, 2023 •

edited

Loading

hanoixan commented Jan 18, 2023

bkunda commented Jan 20, 2023

DmitryArefiev commented Jan 23, 2023

bkunda commented Jan 24, 2023

hanoixan commented Jan 26, 2023 •

edited

Loading

DmitryArefiev commented Jan 27, 2023

hanoixan commented Jan 29, 2023 •

edited

Loading

DmitryArefiev commented Jan 31, 2023

cbjeukendrup commented Feb 12, 2023

bkunda commented Feb 14, 2023

worldwideweary commented Jul 13, 2023

Fix for Issue 11218: Improve workflow of Panels #15675

Are you sure you want to change the base?

Fix for Issue 11218: Improve workflow of Panels #15675

Conversation

hanoixan commented Jan 1, 2023 • edited by cbjeukendrup Loading

hanoixan commented Jan 2, 2023

hanoixan Jan 5, 2023

Choose a reason for hiding this comment

hanoixan Jan 5, 2023

Choose a reason for hiding this comment

hanoixan Jan 5, 2023

Choose a reason for hiding this comment

hanoixan Jan 5, 2023

Choose a reason for hiding this comment

hanoixan Jan 5, 2023 • edited Loading

Choose a reason for hiding this comment

hanoixan Jan 5, 2023

Choose a reason for hiding this comment

bkunda commented Jan 5, 2023

cbjeukendrup commented Jan 6, 2023

hanoixan commented Jan 6, 2023

bkunda commented Jan 6, 2023

hanoixan commented Jan 7, 2023 • edited Loading

bkunda commented Jan 9, 2023

hanoixan commented Jan 10, 2023 • edited Loading

bkunda commented Jan 10, 2023

hanoixan commented Jan 10, 2023 • edited Loading

bkunda commented Jan 10, 2023

hanoixan commented Jan 12, 2023 • edited Loading

cbjeukendrup commented Jan 12, 2023

hanoixan commented Jan 13, 2023 • edited Loading

hanoixan commented Jan 18, 2023

bkunda commented Jan 20, 2023

DmitryArefiev commented Jan 23, 2023

bkunda commented Jan 24, 2023

hanoixan commented Jan 26, 2023 • edited Loading

DmitryArefiev commented Jan 27, 2023

hanoixan commented Jan 29, 2023 • edited Loading

DmitryArefiev commented Jan 31, 2023

cbjeukendrup commented Feb 12, 2023

bkunda commented Feb 14, 2023

worldwideweary commented Jul 13, 2023

hanoixan commented Jan 1, 2023 •

edited by cbjeukendrup

Loading

hanoixan Jan 5, 2023 •

edited

Loading

hanoixan commented Jan 7, 2023 •

edited

Loading

hanoixan commented Jan 10, 2023 •

edited

Loading

hanoixan commented Jan 10, 2023 •

edited

Loading

hanoixan commented Jan 12, 2023 •

edited

Loading

hanoixan commented Jan 13, 2023 •

edited

Loading

hanoixan commented Jan 26, 2023 •

edited

Loading

hanoixan commented Jan 29, 2023 •

edited

Loading