Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reorganize content into tabs of controller preferences #14006

Merged
merged 12 commits into from
Feb 1, 2025

Conversation

JoergAtGithub
Copy link
Member

  • Convert groupBoxSettings/groupBoxScreens into tabs
  • Show only the tabs, which have valid content in the current context
  • Sort tabs that settingsTab is shown by default, if controller settings are defined in mapping
  • Moved controller tabs below info and warning section
  • Made hyperlink warning label clickable and warning text select- and copy-able
  • Made warning label collapsing in height
  • Hide 'Mapping Info' fields if there is no content

This is a preparatory GUI cleanup, prior to the addition of further properties in additional tabs.

Before:
grafik

This PR:
grafik

Show only the tabs, which have valid content in the current context
Sort tabs that settingsTab is shown by default, if controller settings are defined in mapping
Moved controller tabs below info and warning section
Made hyperlink warning label clickable and warning text select- and copy-able
Made warning label collapsing in height
Hide 'Mapping Info' fields if there is no content
Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM otherwise. Thank you.

src/controllers/dlgprefcontroller.h Show resolved Hide resolved
Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Waiting for CI. @ronso0 do you have interest to take a quick look at the .ui changes?

@ronso0
Copy link
Member

ronso0 commented Dec 11, 2024

Umm, looking at .ui diffs is pointless here. Before and after grid/layout items are in random order (that's why I usually reorder them so GUI order matches .ui order).
And I can't built main currently because I don't have hidapi 0.14.0

So if the screenshots reflect the current state of this PR and UX (correct tabstops etc.) is the same this LGTM 👍
Just the grids/2-column-layouts in Device Info / Mapping Info could be optimised so that eg. Mapping -> Description can use the available space.

Added tabstop section
Unified some names
Simplified top level frame/grid structure to a single grid
@JoergAtGithub JoergAtGithub force-pushed the controller_pref_tab_layout branch from b0a87f9 to e1f8a6b Compare December 14, 2024 20:34
@JoergAtGithub
Copy link
Member Author

I reordered the .ui file in GUI order.
I added a tabstop section.
But I could not find a way to use all the available vertical space in the grids/2-column-layouts in Device Info / Mapping Info. I tried for hours, and the only partially working solution was to set the spacer height from C++, which I consider more ugly than a little uneccessary whitespace.

@ronso0
Copy link
Member

ronso0 commented Dec 14, 2024

I'll take a look asap, can build again with a libhidapi backport.

@ronso0 ronso0 added this to the 2.6-beta milestone Dec 28, 2024
Copy link
Member

@ronso0 ronso0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my first review, will take another look soonish

src/controllers/dlgprefcontroller.cpp Outdated Show resolved Hide resolved
src/controllers/dlgprefcontroller.cpp Outdated Show resolved Hide resolved
@ronso0
Copy link
Member

ronso0 commented Dec 30, 2024

A regression I noticed is that now the Input/Output tabs can't use the entire height of the dialog as before, mapping and device info will always occupy space at the top.

One solution would be to allow collapsing groupboxes.

  1. subclass QGroupBox, add collapse/expand buttons, which I'd appreciate also for mappings with many options where I'm interested only in one or two settings catgeories.
    Not trivial though..
  2. add an expand/collapse toggle above Device / Mapping Info.
    Much easier, but the toggle itself would occupy a row

@ronso0
Copy link
Member

ronso0 commented Dec 30, 2024

Btw
QCheckBox "chkEnabledDevice" and QLabel "labelLoadMapping" are both in row="1" column="0".
This will probably never cause issues (overlap) because the checkbox is left-aligned (default) and the label is right-aligned, only if the entire dialog is very narrow and all device info fields are empty and the big, bold device name is very short and the mapping description is not empty.
Just saying..

on second thought, maybe that's intentional 🤷‍♂️

@JoergAtGithub
Copy link
Member Author

Btw QCheckBox "chkEnabledDevice" and QLabel "labelLoadMapping" are both in row="1" column="0".

I also noticed this, but this is not new in this PR.

Added methods to count and find the first visible tab.
@JoergAtGithub JoergAtGithub requested a review from ronso0 December 30, 2024 14:06
Copy link
Member

@ronso0 ronso0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I'll do a test with some HID and MIDI gear... next year ; )

@JoergAtGithub
Copy link
Member Author

Thanks, I'll do a test with some HID and MIDI gear... next year ; )

@ronso0 Did you already found the time to test this?

@ronso0
Copy link
Member

ronso0 commented Jan 26, 2025

Sorry, this got shifted down in my TODO list.
Will try to test tonight.

Copy link
Member

@ronso0 ronso0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM otherwise!

I opened JoergAtGithub#3 with some fixes:

  • Screens tab was always visible when built with QML off
  • finally managed to fix the random layout changes / v-stretch of the device / mapping info groupboxes
    (this took hours, literally... sometimes Qt widget/layout props are a PITA)
  • nits

Tested with MIDI Through and Traktor S4mk3 (HID).
All looking good now with the fixup PR.
image

src/controllers/dlgprefcontroller.cpp Outdated Show resolved Hide resolved
int firstVisibleTabIndex = getIndexOfFirstVisibleTabs();
if (firstVisibleTabIndex >= 0) {
m_ui.controllerTabs->setCurrentIndex(firstVisibleTabIndex);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ux nit: when changing mappings, this would always reset to first visible tab.

We could save and (try to) restore the last selected tab.
This might be helpful for eg. quickly compare screens of two mappings.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see your use case. But I think another use case is more important:
If a (new) user steps/scrolls through the list of mappings. Than it's likely that there was a mapping without controller settings beneth them. This means Midi In becomes the default - an expert setting, not intendend for average users. For them we introduced the controller settings, which should appear as default therefore.

Comment on lines +308 to +313

// Store the index of the input and output mappings tabs
m_inputMappingsTabIndex = m_ui.controllerTabs->indexOf(m_ui.inputMappingsTab);
m_outputMappingsTabIndex = m_ui.controllerTabs->indexOf(m_ui.outputMappingsTab);
m_settingsTabIndex = m_ui.controllerTabs->indexOf(m_ui.settingsTab);
m_screensTabIndex = m_ui.controllerTabs->indexOf(m_ui.screensTab);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All tabs are visible by default (indices are correct), so let's move this to the top of the ctor, above the first call of slotShowMapping()?

Copy link
Member

@ronso0 ronso0 Jan 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any side effects, just saying..
When slotUpdate() is called to actually show the preferences indices are set.

@ronso0
Copy link
Member

ronso0 commented Feb 1, 2025

Okay, let's merge it then.
Thanks!

@ronso0 ronso0 merged commit f54d3f6 into mixxxdj:main Feb 1, 2025
13 checks passed
@JoergAtGithub JoergAtGithub deleted the controller_pref_tab_layout branch February 2, 2025 01:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants