add WCollapsibleGroupBox, use for controller mapping info boxes and settings #14324

ronso0 · 2025-02-12T17:19:20Z

This adds a QGroupBox-derived WCollapsibleGroupBox that can be collapsed/expanded by clicking its title.

IMHO this fixes the regression with the new tab'ed controller page where the device and mapping info would push the setting and I/O tabs down.
It also allows to focus on a certain settings group when adjusting the mapping.

Down/Right arrow like on treeview branches
state of Device / Mapping info boxes is persistent per session (per controller)
state of settings boxes is restored / persists as long as the mapping is selected
(they are expanded every time the controller page is shown because the controller settings are rebuild each time)

WCollapsibleGroupBox abuses QGroupBox::setChecked() mechanism. Originally this is a checkbox in the title bar and toggling it enables/disables all children.
Adopting this saves us from reimplementing the checked state tracking, signal etc.
This is not touched but also irrelevant since the content is now hidden anyway when it's collapsed.
Collapsing is achieved by simply setting the maximum height to height of the title row.
Inspired by ctkCollapsibleGroupBox, just without the complexity of hiding/showing child widgets (which requires some small, low-risk hacks to get the appearance right under all circumstances).

I imagine this groupbox can be helpful in other preferences pages, too.
(as quick fix for long pages, compared to restructuring into tab or otherwise splitting pages)

cr7pt0gr4ph7

LGTM 👍 Just a few small comment-related nitpicks. No major objections.

src/widget/wcollapsiblegroupbox.h

src/controllers/dlgprefcontroller.cpp

cr7pt0gr4ph7 · 2025-02-14T14:58:02Z

src/controllers/dlgprefcontroller.cpp

+                const QString title = pBox->title();
+                pBox->setCheckable(true);
+                if (m_settingsCollapsedStates.contains(title)) {
+                    pBox->setChecked(m_settingsCollapsedStates.value(title));
+                }
+
+                connect(pBox,
+                        &WCollapsibleGroupBox::toggled,
+                        this,
+                        [this, title](bool checked) {
+                            m_settingsCollapsedStates.insert(title, checked);
+                        });


This will slightly misbehave when multiple groups have the same title. I think that's OK, but maybe we should add a comment similar to this:

// Note: If multiple groups happen to have the same name (which should // not normally happen in well-behaved controller mappings, but is not // strictly prohibited), the last one to be expanded/collapsed determines // the state that will be restored.

how about we just store whether the groupbox is collapsed in the groupbox itself?

IIUC, the MIDI learning wizard can cause the mapping to be reloaded while the preferences dialog is open, and this is an attempt to restore the expansion state in that case (because reloading also means that the mapping-defined GUI widgets are destroyed and recreated).

Yes, that, too. The primary reason we need (want) to restore the state in the first place is that the settings widgets are rebuild every time the Preferences are shown. Same when pressing Cancel.

We may re-show the mapping only if it has been changed (I/O mapping, setting(s)). Eg. we could compare each setting's value with its saved value (when a setting is toggled on and of again) and somehow link that to mapping->setDirty().
But that seems a lot of work compared to the QHash implementation.

src/controllers/dlgprefcontroller.cpp

src/widget/wcollapsiblegroupbox.cpp

ronso0 · 2025-02-14T16:05:40Z

Thanks for your review!

I pushed the fixup and will squash before merge.

Swiftb0y

couple thoughts

Swiftb0y · 2025-02-14T20:48:47Z

src/controllers/dlgprefcontroller.cpp

+                const QString title = pBox->title();
+                pBox->setCheckable(true);
+                if (m_settingsCollapsedStates.contains(title)) {
+                    pBox->setChecked(m_settingsCollapsedStates.value(title));
+                }
+
+                connect(pBox,
+                        &WCollapsibleGroupBox::toggled,
+                        this,
+                        [this, title](bool checked) {
+                            m_settingsCollapsedStates.insert(title, checked);
+                        });


how about we just store whether the groupbox is collapsed in the groupbox itself?

Swiftb0y · 2025-02-14T20:49:50Z

src/controllers/legacycontrollersettingslayout.cpp

-    auto pContainer = make_parented<QGroupBox>(m_label, pParent);
+    auto pContainer = make_parented<WCollapsibleGroupBox>(m_label, pParent);


wdyt of making this configurable in the settings XML? Shouldn't be hard IMO.

Do you mean with an extra xml node?
If yes, I 'd rather not put that work on mapping developers. I mean collapsing top-level groups is desirable, and on lower levels it only makes the GUI more complex.

But maybe we can check if the groupbox' parent is a WCollapsibleGroupBox, too, and auto-set checkable if it's not.
Will check..

Naa, we already query the groups in DlgPrefController so that's the place to set the checkable IMO.

Do you mean with an extra xml node?

Either that or add an attribute to the node

If yes, I 'd rather not put that work on mapping developers.

Right. On the flipside (and why I was suggesting to make it configurable), what if devs really want some settings to be easily visible and hide others (more advances ones for instance)?

Ah okay, so you're proposing to allow setting the default collapsed state in xml (not the collapsible feature in general).

yeah, that works too.

I like the idea of the collapsed default state! I was looking at adding some kin d of depends or condition to help with the large amount of settings in #13653, but this looks like anm elegant solution to address this with a minimum effort.

Thanks for the feedback!

Though I'd love to merge the basic implementation first.

acolombier

Thank you for adding this, this is looking very good! Just some minor comments and feedbacks

acolombier · 2025-02-15T13:03:24Z

src/controllers/legacycontrollersettingslayout.cpp

-    auto pContainer = make_parented<QGroupBox>(m_label, pParent);
+    auto pContainer = make_parented<WCollapsibleGroupBox>(m_label, pParent);


I like the idea of the collapsed default state! I was looking at adding some kin d of depends or condition to help with the large amount of settings in #13653, but this looks like anm elegant solution to address this with a minimum effort.

src/widget/wcollapsiblegroupbox.cpp

src/widget/wcollapsiblegroupbox.h

cr7pt0gr4ph7 · 2025-02-15T14:09:48Z

I already have an additional use case for WCollapsibleGroupBox as soon as its merged: Making the File Info section of the track info dialog collapsible, as the latter has the tendency to exceed the screen height on wide but "height-challenged"¹ laptop screens when combined with even slightly larger font sizes.

Also, could you make sure/test that the WCollapsibleGroupBox is well-behaved when using keyboard navigation?

The header should be reachable using the keyboard, both from outside the groupbox, and from the inside (when expanded), and should have suitable visual feedback when focused via the keyboard.
Clicking on the header should move the focus to it.
Spacebar should expand/collapse the groupbox when the header is focused.
Controls inside the box should be reachable via Tab when expanded, and skipped over when collapsed.
(I'm not 100% sure what the correct behavior should be when the groupbox is programmatically collapsed while keyboard focus is in it. Should the focus stay on the previously focused but now invisible control, or should focus move to the groupbox header? Disabling the controls inside collapsed groupbox seems to actually be a good thing because it prevents making accidental edits to those controls.)

A question for the native English speakers: What's the correct adjective for the opposite of tall when you specifically mean a small height instead of a small overall size? It's wide vs. narrow, and tall vs. ...? I know there is short, but a short screen does not really sound right to me. ↩

cr7pt0gr4ph7 · 2025-02-15T16:10:46Z

src/widget/wcollapsiblegroupbox.cpp

+        } else if (pEvent->type() == QEvent::LayoutRequest ||
+                pEvent->type() == QEvent::ContentsRectChange ||
+                pEvent->type() == QEvent::Resize) {


It might make sense to replace this if/else blocks with a switch.

ronso0 · 2025-02-16T22:09:27Z

The header should be reachable using the keyboard, both from outside the groupbox, and from the inside (when expanded), and should have suitable visual feedback when focused via the keyboard.

Clicking on the header should move the focus to it.

Spacebar should expand/collapse the groupbox when the header is focused.

Controls inside the box should be reachable via Tab when expanded, and skipped over when collapsed.

✔️ since we just hook up to the 'checked' state this is all handled correctly by QGroupBox
(now that I removed the wrong setFocusPolicy(Qt::NoFocus); ; )

(I'm not 100% sure what the correct behavior should be when the groupbox is programmatically collapsed while keyboard focus is in it. Should the focus stay on the previously focused but now invisible control, or should focus move to the groupbox header? Disabling the controls inside collapsed groupbox seems to actually be a good thing because it prevents making accidental edits to those controls.)

The same here: QGroupBox handles this. The original behavior of setChecked() is to enable/disable all children and we simply expect that to work.

ronso0 · 2025-02-16T22:15:33Z

Btw I fixed soe other keyboard focus issue in the controller page (settings checkbox, more tabstops, don't focus hidden tabs ..)

JoergAtGithub · 2025-02-17T00:20:35Z

src/controllers/dlgprefcontrollerdlg.ui

-       <widget class="QLabel" name="labelDataHandlingProtocolValue">
+     <layout class="QVBoxLayout" name="deviceInfoGroupLayout">
+      <item>
+       <widget class="WCollapsibleGroupBox" name="groupBoxDeviceInfo">


Please ensure that groupBoxDeviceInfo and groupBoxMappingInfoare always expanded, if no mapping is loaded to a device, as these boxes contain the information needed by the end user to select the correct mapping for the right device.

By default these groups are expanded and remain so until the user explicitly collapses them.
And the most important info is the device name which is the big label in the top left corner.
What other info do you refer to?

The text in the big label is not unique, this is why Device Info was implemented for 2.6. On my system I have 3 devices with exactly the same string:

This is normal because this string depends on assumptions that are only sometimes true. This was addressed by adding the device info box. If this information is hidden, the user has no chance to distinguish the devices.
There is no use case for collapsing these 2 boxes of valuable information needed for the mapping selection, until a mapping is loaded. This is because the tabs below are hidden at this time and enough space is available to show them.

I understand. Still, the groups are expanded by default and the state is persistent per controller page. And the device order in the sidebar is also persistent. Collapsing is optional and done manually. So if you don't need it, don't use it.
In the case of multiple devices with the same display name, why would a user collapse the info if they need it to make sure it's the correct device?

I mean, sure we can somehow force-expand the device info when 'No mapping' is selected but which exact use case would that cover?
I'm not a fan of GUI elements being shifted / shown with for obvious reason.

acolombier · 2025-02-17T19:27:38Z

src/widget/wcollapsiblegroupbox.cpp

-          m_minH(-1),
-          m_maxH(10000) {
+          m_minHeight(-1),
+          m_maxHeight(QWIDGETSIZE_MAX) {


Nice 💯 ! I didn't know about this macro

acolombier

Code LGTM! Will take that for a spin in a bit

acolombier

There is a weird glitch when expending a collapsed group. tested on Fedora 41 (Wayland)

Screencast.From.2025-02-17.23-12-28.mp4

ronso0 · 2025-02-18T00:39:21Z

I'm seeing this, too, if I look for it.
Will check..

ronso0 · 2025-02-18T13:18:22Z

Hmm, I traced the layout / resize events and it seems there is no easy fix:
when a group is expanded (and also sometimes when it's collapsed??) the parent layout is resized, too, but apparently doesn't keep up with the resize events of its children. The groups above are shrunk and regrown temporarily in 1-2 steps until all is setlled and the desired layout is established.

Maybe sequential hiding/showing of the group children fixes it but I fear this also adds more complexity / more stuff to care about. Bit like it's done in ctkCollapsibleGroupBox which also resizes via setMaximumHeight(), but I didn't build with that (Qt-derived?) framework.

ronso0 · 2025-02-18T14:13:57Z

Maybe sequential hiding/showing of the group children fixes it

No, it doesn't.
Rejecting the "wrong"/undesired shrink events also doesn't help: the groups are resized anyway (and tbh I don't feel comfortbale interferring with Qt at this level..)
I suspect it's an resize issue with the parent layout, or someQSizePolicy glitch QLayout/QTabWidget.

the parent layout is resized, too, but apparently doesn't keep up with the resize events of its children.

The weird thing is: it also happens when a group is collapsed.. so in that case the layout seems to shrink before the box is actually collapsed, squeezing the groups above.

These are extemely hard to debug/trace and I wasted countless hours already fixing (or trying to) some Mixxx dialogs.
I'll give it another go (looking at the settings layout) but tbh I don't expect we can do much about it 🤷‍♂️

ronso0 · 2025-02-18T15:08:27Z

Yeah, I'll stop working on this (fix).
It's the order of the events (resize, layou update) and the glitch appears randomly (but is easier to reproduce if the (actually static) group above holds QCheckBoxes).

Btw when collapsing a group, the parent QScrollArea might toggle the vertical scrollbars which is another resize reason.

I think we either have to live with this Qt glitch, or delay this PR until someone finds a fix.

acolombier · 2025-02-18T19:20:09Z

Tbh, I'm happy living with this bug. It's a small discomfort, but doesn't fundamentally prevent or break anything. Thanks for trying anyway!

ronso0 · 2025-02-19T14:50:41Z

Btw I think it's not related to the settings layout, I also see the glitch in when I use WCollapsibleGroupBox in Deck preferences if the expanded groups don't fit in the page, ie. scrollbars are toggled.

FWIW I think we should fix the abnormally large layout padding in the controller settings:
Library

Controller

…hide

ronso0 · 2025-02-20T03:22:59Z

Alright then, thanks for your review!
Squashed and main merged, ready to roll.

@JoergAtGithub Could I dispel your concerns re the Device/Mapping groups?

github-actions bot added ui build controller backend labels Feb 12, 2025

ronso0 force-pushed the controller-sett-group-hide branch 3 times, most recently from eb616c7 to bbe7749 Compare February 13, 2025 19:04

ronso0 marked this pull request as ready for review February 13, 2025 20:08

ronso0 added preferences and removed build controller backend labels Feb 13, 2025

cr7pt0gr4ph7 approved these changes Feb 14, 2025

View reviewed changes

github-actions bot added build controller backend labels Feb 14, 2025

Swiftb0y reviewed Feb 14, 2025

View reviewed changes

ronso0 force-pushed the controller-sett-group-hide branch from c91d5d5 to 6d6e02a Compare February 14, 2025 21:49

acolombier reviewed Feb 15, 2025

View reviewed changes

cr7pt0gr4ph7 reviewed Feb 15, 2025

View reviewed changes

JoergAtGithub requested changes Feb 17, 2025

View reviewed changes

acolombier reviewed Feb 17, 2025

View reviewed changes

acolombier approved these changes Feb 17, 2025

View reviewed changes

acolombier requested changes Feb 17, 2025

View reviewed changes

acolombier approved these changes Feb 18, 2025

View reviewed changes

ronso0 added 5 commits February 20, 2025 04:20

add WCollapsibleGroupBox, use for controller mapping info boxes

28a7a31

use WCollapsibleGroupBox for controller settings groups

f6566cf

Controller pref: disable hidden tabs so they can get keyboard focus

b757f08

Controller pref: add tabstops for mapping links

595951a

Merge remote-tracking branch 'mixxx/main' into controller-sett-group-…

4beaa90

…hide

ronso0 force-pushed the controller-sett-group-hide branch from f06af1e to 4beaa90 Compare February 20, 2025 03:20

		auto pContainer = make_parented<QGroupBox>(m_label, pParent);
		auto pContainer = make_parented<WCollapsibleGroupBox>(m_label, pParent);

add WCollapsibleGroupBox, use for controller mapping info boxes and settings #14324

Are you sure you want to change the base?

add WCollapsibleGroupBox, use for controller mapping info boxes and settings #14324

Conversation

ronso0 commented Feb 12, 2025 • edited Loading

cr7pt0gr4ph7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronso0 commented Feb 14, 2025

Swiftb0y left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acolombier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cr7pt0gr4ph7 commented Feb 15, 2025

Footnotes

Choose a reason for hiding this comment

ronso0 commented Feb 16, 2025

ronso0 commented Feb 16, 2025

Choose a reason for hiding this comment

ronso0 Feb 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acolombier left a comment

Choose a reason for hiding this comment

acolombier left a comment • edited Loading

Choose a reason for hiding this comment

ronso0 commented Feb 18, 2025

ronso0 commented Feb 18, 2025

ronso0 commented Feb 18, 2025 • edited Loading

ronso0 commented Feb 18, 2025

acolombier commented Feb 18, 2025

ronso0 commented Feb 19, 2025

ronso0 commented Feb 20, 2025

ronso0 commented Feb 12, 2025 •

edited

Loading

ronso0 Feb 17, 2025 •

edited

Loading

acolombier left a comment •

edited

Loading

ronso0 commented Feb 18, 2025 •

edited

Loading