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.

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.

acolombier · 2025-02-15T13:05:08Z

src/widget/wcollapsiblegroupbox.cpp

+WCollapsibleGroupBox::WCollapsibleGroupBox(QWidget* pParent)
+        : QGroupBox(pParent),
+          m_minH(-1),
+          m_maxH(10000) {


Could we use a constant here? Or maybe some std::numeric_limits?

I tried std::numeric_limits<int>::quiet_NaN() earlier but it didn't work as expected, and I was lazy / it was late ; )
Will check again..

Ah, that's only available for floating-point types.
so std::numeric_limits<T>::min seems to be the only applicable member?

acolombier · 2025-02-15T13:06:18Z

src/widget/wcollapsiblegroupbox.cpp

+}
+
+bool WCollapsibleGroupBox::event(QEvent* pEvent) {
+    if (isCheckable()) {


Could you please refactor some of these nested condition to flatten it and use early return instead? It is a bit hard to follow at the moment

Sure, I'll try.

Though early return isn't really applicable.

Suggested change

if (isCheckable()) {

if (!isCheckable()) {

// Only enable the special expand/collapse behaviors

// when the users has requested them via setCheckable(true).

// Otherwise, this should just behave like a normal QGroupBox.

QGroupBox::event(pEvent);

return;

}

Yes, that's the only one.

acolombier · 2025-02-15T13:06:43Z

src/widget/wcollapsiblegroupbox.h

+class CollapsibleGroupBoxStyle : public QProxyStyle {
+    Q_OBJECT
+  public:
+    explicit CollapsibleGroupBoxStyle(QStyle* pStyle = 0)


Should this be nullptr instead of 0?

Umm, right.
Seems I mixed up the original Qt implementation with that of ctkCollapsibleGroupBox

Suggested change

explicit CollapsibleGroupBoxStyle(QStyle* pStyle = 0)

explicit CollapsibleGroupBoxStyle(QStyle* pStyle = nullptr)

acolombier · 2025-02-15T13:07:21Z

src/widget/wcollapsiblegroupbox.h

+    int m_minH;
+    int m_maxH;


Does H stands for "height" here? How do you feel about making it explicit?

Sure, no prob: m_minHeight, m_maxHeight then

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.

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

ronso0 force-pushed the controller-sett-group-hide branch from 0166ab4 to 160c07b Compare February 13, 2025 15:43

add WCollapsibleGroupBox, use for controller mapping info boxes

a125f8f

ronso0 force-pushed the controller-sett-group-hide branch from 160c07b to eb616c7 Compare February 13, 2025 16:43

use WCollapsibleGroupBox for controller settings groups

bbe7749

ronso0 force-pushed the controller-sett-group-hide branch 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

ronso0 added 2 commits February 14, 2025 17:02

fixup! add WCollapsibleGroupBox, use for controller mapping info boxes

65ffa60

fixup! use WCollapsibleGroupBox for controller settings groups

6d6e02a

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ronso0 commented Feb 12, 2025 •

edited

Loading

cr7pt0gr4ph7 left a comment

cr7pt0gr4ph7 Feb 14, 2025

Swiftb0y Feb 14, 2025

cr7pt0gr4ph7 Feb 14, 2025

ronso0 Feb 14, 2025

ronso0 commented Feb 14, 2025

Swiftb0y left a comment

Swiftb0y Feb 14, 2025

Swiftb0y Feb 14, 2025

ronso0 Feb 14, 2025

ronso0 Feb 14, 2025

Swiftb0y Feb 15, 2025

ronso0 Feb 15, 2025

Swiftb0y Feb 15, 2025

acolombier Feb 15, 2025

acolombier left a comment

acolombier Feb 15, 2025

acolombier Feb 15, 2025

ronso0 Feb 15, 2025

ronso0 Feb 15, 2025

acolombier Feb 15, 2025

ronso0 Feb 15, 2025

ronso0 Feb 15, 2025

cr7pt0gr4ph7 Feb 15, 2025 •

edited

Loading

ronso0 Feb 15, 2025

acolombier Feb 15, 2025

ronso0 Feb 15, 2025

cr7pt0gr4ph7 Feb 15, 2025

acolombier Feb 15, 2025

ronso0 Feb 15, 2025

cr7pt0gr4ph7 commented Feb 15, 2025

cr7pt0gr4ph7 Feb 15, 2025

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

-    if (isCheckable()) {
+    if (!isCheckable()) {
+        // Only enable the special expand/collapse behaviors
+        // when the users has requested them via setCheckable(true).
+        // Otherwise, this should just behave like a normal QGroupBox.
+        QGroupBox::event(pEvent);
+        return;
+    }

	explicit CollapsibleGroupBoxStyle(QStyle* pStyle = 0)
	explicit CollapsibleGroupBoxStyle(QStyle* pStyle = nullptr)

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

acolombier 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

cr7pt0gr4ph7 Feb 15, 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

Choose a reason for hiding this 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 12, 2025 •

edited

Loading

cr7pt0gr4ph7 Feb 15, 2025 •

edited

Loading