Pref > Controller: allow creating a new mapping with 'No Mapping' selected #4905

ronso0 · 2022-08-20T14:07:54Z

'Learning Wizard' button and in/output tabs are now enabled/disabled after applying a mapping (previously this was only in slotUpdate() when re-opening the preferences)
this also allows starting the Wizard with 'No Mapping' selected
fix creating a new mapping with the wizard by creating the I/O table models & assigning them to the new, empty mapping
fix false-positive isDirty when starting the Wizard with 'No Mapping' selected after opening the peferences

ferranpujolcamins

LGTM

daschuer

This fix is unfortunately not consistent with the behavior of the button after changing a mapping. The learning wizard button has already the pop up to ask for apply when the mapping has changed, this is bypassing the gray out state AFTER apply:

open the mapping pane
Select No Mapping
Click Learn Assistant
Confirm Applying Popup
Now the leaning Wizzart has "No Mapping" to modify.

Solutions

Remove Graying out and add a Pop up "Select an existing mapping to modify frist";
Gray out the Learn assistant if the "No Mapping" is selected (before apply)

ronso0 · 2022-08-24T09:42:16Z

Thanks for testing!

open the mapping pane

Select No Mapping

Click Learn Assistant

Confirm Applying Popup

Now the leaning Wizzart has "No Mapping" to modify.

Well, this fixes #10540
(uuuh yeah, 'Issues' are imported 🎉 )
So we only need to take care of the "No mapping" case in saveMapping() IINM

Btw there is also the issue that enumerateMappings overrides the mapping selection:

("Mapping123" selected)
make changes (don't apply)
select "MappinXY"
"Apply?" yes, choose Overwrite or Save as new..
"Mapping123" selected

ronso0 · 2022-08-24T11:49:13Z

Thanks for testing!

open the mapping pane

Select No Mapping

Click Learn Assistant

Confirm Applying Popup

Now the leaning Wizzart has "No Mapping" to modify.

Well, this fixes #10540 .. So we only need to take care of the "No mapping" case in saveMapping() IINM

That could fix #10540 though there's much more to it.
I'll simply disable the Wizard + IO tabs when 'No Mapping' is selected.

Prior to that I added two fixup commits. @daschuer Please give me a ✔️ when you reviewed those so I can squash them.

src/controllers/dlgprefcontroller.cpp

ronso0 · 2022-08-25T00:05:38Z

src/controllers/dlgprefcontroller.cpp

+        // shortuct to create and assign required IO table models (no GUI changes)
+        slotShowMapping(m_pMapping);


... otherwise there's no input table the recorded mappings can be applied to.

we may as well split the table model creation into a separate function, though for the 'new mapping' case there's no overhead in using slotShowMapping (it just updates the mapping info section, which is empty anyway)

ronso0 · 2022-08-25T00:10:03Z

src/controllers/scripting/legacy/controllerscriptenginelegacy.cpp

+    // creating a new, empty mapping LegacyMidiControllerMapping with the wizard
+    if (m_pJSEngine) {
+        callFunctionOnObjects(m_scriptFunctionPrefixes, "shutdown");
+    }


otherwise we'd hit debug assertions in callFunctionOnObjects and ControllerScriptEngineBase::shutdown() when the new mapping applied.

(peviously/2.3 both methods simly returned when there was no js engine)

daschuer · 2022-08-25T05:24:51Z

Just tested the current state of this PR:

After restart with "No Mapping" the Learn wizzard is active.

Is it intended to allow to learn a new mapping from the scratch with this PR?
Do we want to create a new Entry "Create New Mapping"?
What is the use case for "No Mapping" by the way? Just the default after new installation? In this case we may replaces it with a Dummy Item "Select Mapping ..." which can be removed after the user has selected one.

daschuer · 2022-08-25T05:32:20Z

Is there a reason the we show the populated IO Tabs grayed out after selection a mapping? You cannot even adjust the columns to make the content fully visible and they are hard to read. I propose to just gray out the edit buttons, if required.

Maybe we should consider to auto adjust the columns width.

daschuer · 2022-08-25T05:47:02Z

This PR has a creeping scope. Did you consider to polish the original bug-fix for 2.3 and move the features into another branch?
Just an idea ...

… mapping

ronso0 · 2022-08-25T10:52:12Z

Did you consider to polish the original bug-fix for 2.3

No problem, I extracted some fixes for 2.3, see #10821
This branch is now cleaner.

ronso0 · 2022-08-25T10:53:25Z

About greying out the I/O tabs, I'm not sure what the intention is. I don't mind if we remove that entirely.
The 'Add ...' buttons don't work with no mapping anyway, so ~~I don't see a~~ it's just a minor UX regression. Need to double-check.

Auto-adjust columns is a nice-to-have item for later on.

ronso0 · 2022-08-25T10:57:33Z

Just tested the current state of this PR:
* After restart with "No Mapping" the Learn wizzard is active.

Is it intended to allow to learn a new mapping from the scratch with this PR?

This PR just fixes the behaviour of 2.3 which was broken because the Wizard button was disabled with 'No Mapping' and the I/O table models were not linked to the mapping when starting the wizard with 'No Mapping'.

Do we want to create a new Entry "Create New Mapping"? What is the use case for "No Mapping" by the way? Just the default after new installation? In this case we may replaces it with a Dummy Item "Select Mapping ..." which can be removed after the user has selected one.

IMO "No Mapping" serves these cases:

no mapping selected (none selected, or configured mapping not found / invalid)
clear UI to be 100% sure there is no mapping loaded
create a new mapping from scratch

Thus, I'm not tempted to make the code & UX more complex.
As long as the Wizard button is available with 'No Mapping' I think it's obvious that it should be clicked to start a new mapping.

…th 'No Mapping'

ronso0 · 2022-09-01T12:24:19Z

open the mapping pane

Select No Mapping

Click Learn Assistant

Confirm Applying Popup

Now the leaning Wizzart has "No Mapping" to modify.

Btw this is also fixed, the name field is empty.

ronso0 · 2022-09-11T14:36:26Z

Do we want to create a new Entry "Create New Mapping"?

When "No mapping" is selected we could show a hint where the mapping description is displayed:
"You can use the MIDI Wizard to create a new mapping from scratch"

links to the manual or wiki maybe

daschuer

Thank you, works good now.

ronso0 added this to the 2.4.0 milestone Aug 20, 2022

ronso0 added controller mappings ui preferences labels Aug 20, 2022

ferranpujolcamins approved these changes Aug 21, 2022

View reviewed changes

mixxxbot mentioned this pull request Aug 23, 2022

midi mapping inputs/outputs not appearing in preferences #10539

Closed

daschuer requested changes Aug 23, 2022

View reviewed changes

Swiftb0y reviewed Aug 24, 2022

View reviewed changes

src/controllers/dlgprefcontroller.cpp Outdated Show resolved Hide resolved

ronso0 marked this pull request as draft August 24, 2022 23:57

ronso0 force-pushed the pref-controller-apply-wizard branch 2 times, most recently from 8fa2c5d to ca57abf Compare August 25, 2022 00:03

ronso0 commented Aug 25, 2022

View reviewed changes

This was linked to issues Aug 25, 2022

can't create new mapping with wizard from scratch #10540

Closed

midi mapping inputs/outputs not appearing in preferences #10539

Closed

ronso0 added 5 commits August 25, 2022 12:36

Pref Controller: update GUI after applying mapping

dcbec45

Pref Controller: disable mapping GUI for 'No Mapping'

8e001f1

Pref Controller: always enable Wizard button for MIDI controllers

12f6338

Pref Controller: fix creating new mapping with 'No Mapping'

3c1f16e

ControllerScriptEngineLegacy: avoid debug asserts with new, file-less…

06ac0df

… mapping

ronso0 force-pushed the pref-controller-apply-wizard branch from ca57abf to 06ac0df Compare August 25, 2022 10:45

ronso0 changed the title ~~Pref > Controller: update GUI after mapping was applied~~ Pref > Controller: update GUI after applying mapping, fix new mapping with 'No Mapping' Aug 25, 2022

Pref Controller: fix false-positive 'isDirty' when starting Wizard wi…

ea61944

…th 'No Mapping'

ronso0 marked this pull request as ready for review August 25, 2022 12:55

ronso0 added the regression label Aug 30, 2022

ronso0 changed the title ~~Pref > Controller: update GUI after applying mapping, fix new mapping with 'No Mapping'~~ Pref > Controller: allow creating a new mapping with 'No Mapping' selected Aug 30, 2022

daschuer approved these changes Sep 17, 2022

View reviewed changes

daschuer merged commit eddade7 into mixxxdj:main Sep 17, 2022

ronso0 deleted the pref-controller-apply-wizard branch September 21, 2022 22:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pref > Controller: allow creating a new mapping with 'No Mapping' selected #4905

Pref > Controller: allow creating a new mapping with 'No Mapping' selected #4905

ronso0 commented Aug 20, 2022 •

edited

Loading

ferranpujolcamins left a comment

daschuer left a comment

ronso0 commented Aug 24, 2022

ronso0 commented Aug 24, 2022

ronso0 Aug 25, 2022

ronso0 Aug 25, 2022

ronso0 Aug 25, 2022

ronso0 Aug 28, 2022

daschuer commented Aug 25, 2022

daschuer commented Aug 25, 2022

daschuer commented Aug 25, 2022

ronso0 commented Aug 25, 2022

ronso0 commented Aug 25, 2022 •

edited

Loading

ronso0 commented Aug 25, 2022 •

edited

Loading

ronso0 commented Sep 1, 2022

ronso0 commented Sep 11, 2022

daschuer left a comment

		// shortuct to create and assign required IO table models (no GUI changes)
		slotShowMapping(m_pMapping);

Pref > Controller: allow creating a new mapping with 'No Mapping' selected #4905

Pref > Controller: allow creating a new mapping with 'No Mapping' selected #4905

Conversation

ronso0 commented Aug 20, 2022 • edited Loading

ferranpujolcamins left a comment

Choose a reason for hiding this comment

daschuer left a comment

Choose a reason for hiding this comment

ronso0 commented Aug 24, 2022

ronso0 commented Aug 24, 2022

ronso0 Aug 25, 2022

Choose a reason for hiding this comment

ronso0 Aug 25, 2022

Choose a reason for hiding this comment

ronso0 Aug 25, 2022

Choose a reason for hiding this comment

ronso0 Aug 28, 2022

Choose a reason for hiding this comment

daschuer commented Aug 25, 2022

daschuer commented Aug 25, 2022

daschuer commented Aug 25, 2022

ronso0 commented Aug 25, 2022

ronso0 commented Aug 25, 2022 • edited Loading

ronso0 commented Aug 25, 2022 • edited Loading

ronso0 commented Sep 1, 2022

ronso0 commented Sep 11, 2022

daschuer left a comment

Choose a reason for hiding this comment

ronso0 commented Aug 20, 2022 •

edited

Loading

ronso0 commented Aug 25, 2022 •

edited

Loading

ronso0 commented Aug 25, 2022 •

edited

Loading