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

MIDI Input editor: allow selecting multiple Options #12348

Merged
merged 4 commits into from
May 22, 2024

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented Nov 24, 2023

Multiple options can be set in the learning dialog (not all available options are shown there #12336 (comment)) and by editing the mapping XML, of course. Though, editing multi-select in the mapping table was not possible since selecting an option would deselect all others.

This PR allows to set multiple options in the list view of the Options combobox.
Either click / press Enter on a selected option to toggle it and close the list, or press Space on an option to toggle it and keep the list open.
image

It took me a while to figure that the 'Normal' option should not be in options list now that it allows multiple options. Due to how the options flags work here 'Normal' is exclusive, meaning it's the implicit result of unchecking all other options, so showing it checked is pointless (and since it's a dumb combobox it's not unchecked immediately if others are checked). Also, clicking it does not uncheck all other options.

The second commit removes 'Normal' entirely. IMO the mapping table is much clearer without it, individual mappings that have options set are now much easier to spot.

The third commit (experimental) adds an 'Unset all' action. Please test if you think if that is helpful in real life. I didn't manipulate mappings on that level for very long.


I'm targeting main since this doesn't work with Qt5 as is unfortunately. Checkboxes are not shown and the check state can't be changed. I tried with setting QAction icons in c++ but handling the checkstate was a mess.
An alternative approach with a QMenu is to complex for this small improvement.

Closes #7416

@ronso0 ronso0 marked this pull request as draft December 7, 2023 14:25
@ronso0 ronso0 marked this pull request as ready for review December 8, 2023 21:29
@ronso0
Copy link
Member Author

ronso0 commented Dec 10, 2023

I adjusted the description to reflect the current state. It works smoothly now.

There's a tiny regression compared to the default combobox: usually, when closing the list view by clicking elsewhere, the (closed) combobox would show the seleted item. With multi-select the display string is updated only after the editor is closed (combobox hidden, data applied to model) so it would not reflect the new state. I choose to clear the index (no name displayed) to avoid confusion. I tried closing the editor as soon as the list is closed but that is cumbersome (subclass QComboBox, handle events) and may not work 100% as expected in all situations.

@daschuer
Copy link
Member

daschuer commented May 8, 2024

We have windows CI error.

@ronso0 ronso0 force-pushed the midi-dialog-multi-options branch from 4f36e9a to dfb7648 Compare May 8, 2024 09:40
@ronso0
Copy link
Member Author

ronso0 commented May 8, 2024

(force-pushed to trigger CI, logs have already been deleted)

@ronso0
Copy link
Member Author

ronso0 commented May 8, 2024

...and now also Windows builds it just fine.

const auto* pModel = static_cast<QStandardItemModel*>(pComboBox->model());
DEBUG_ASSERT(pModel);
for (int row = 0; row < pModel->rowCount(); row++) {
auto pItem = pModel->item(row, 0);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
auto pItem = pModel->item(row, 0);
auto *pItem = pModel->item(row, 0);

if (row == index) { // Actually it's the last item, but this is safer.
continue;
}
auto pItem = pModel->item(row, 0);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
auto pItem = pModel->item(row, 0);
auto *pItem = pModel->item(row, 0);

Copy link
Member Author

Choose a reason for hiding this comment

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

actually just pItem = pModel->item(row, 0); because it's already declared a few lines above.

@daschuer
Copy link
Member

It works better than before. However now concurrent bits can be checked. This can be don a in a follow up PR during beta or even later when you like. Else, we can merge after cleanup

@ronso0
Copy link
Member Author

ronso0 commented May 22, 2024

However now concurrent bits can be checked.

Hmm, so pretty much like it is possible via XML, just much easier.
I've filed #13254 and added a comment.

Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

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

LGTM, Thank you.

@daschuer daschuer merged commit 94805b1 into mixxxdj:main May 22, 2024
13 checks passed
@ronso0 ronso0 deleted the midi-dialog-multi-options branch May 22, 2024 22:30
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.

allow multiple MIDI option selection via GUI
3 participants