Skip to content

feat: enable controller setting on MIDI#13045

Merged
JoergAtGithub merged 1 commit into
mixxxdj:mainfrom
acolombier:feat/enable-settings-midi-controller-engine
Apr 16, 2024
Merged

feat: enable controller setting on MIDI#13045
JoergAtGithub merged 1 commit into
mixxxdj:mainfrom
acolombier:feat/enable-settings-midi-controller-engine

Conversation

@acolombier
Copy link
Copy Markdown
Member

The original plan was to also support settings when parsing the XML mapping definition, thus why a follow up PR was dedicated for this feature.

As the latter is likely not happening, this PR exposes the settings in the JS engine only.

I don't have a MIDI controller available for testing yet, can anyone give it a go please?

@ronso0
Copy link
Copy Markdown
Member

ronso0 commented Apr 3, 2024

How exactly can we test?

@acolombier
Copy link
Copy Markdown
Member Author

It can be tested by adding settings definition and querying them from the engine like for the S4 MK3

@ronso0
Copy link
Copy Markdown
Member

ronso0 commented Apr 3, 2024

So they do go to the xml? (to create the settings GUI)

@acolombier
Copy link
Copy Markdown
Member Author

Yes, that's right. The setting definition still goes in XML. The thing that was dropped is conditional mapping definition in XML

@ronso0
Copy link
Copy Markdown
Member

ronso0 commented Apr 4, 2024

Haha, easy one : )
Will give it a go soonish.

@ronso0
Copy link
Copy Markdown
Member

ronso0 commented Apr 4, 2024

I added the S4 Mk3 snip to a VirMIDI mapping and there seems to be an issue with the checkbox labal and rich text:
the inline style is not recognized, thus the string is looooooong and makes the settings page expand that much I don't get a full view with FHD.

Seems this is not based on main?

@ronso0
Copy link
Copy Markdown
Member

ronso0 commented Apr 4, 2024

Yeah, putting this commit on top of main fixes the string issue.

But the checkbox wrapper has a margin now.
before (broken, pure checkbox):
image

on top of main:
image

@ronso0
Copy link
Copy Markdown
Member

ronso0 commented Apr 4, 2024

Btw I think we should add a 'button' style for the label strings, so that the mapping only contains
This will light up the :btn:SYNC:/btn: (or whatever syntax fits)
and c++ creates the style depending on the GUI's dark/light flag (Qt theme).

Any other style templates that would simplify the xml strings?
italic?
bold?

@ronso0
Copy link
Copy Markdown
Member

ronso0 commented Apr 4, 2024

I.e. I consider this ready to go if rebased onto main for easy testing.

Copy link
Copy Markdown
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.

Code LGTM ;)
Didn't do any testing. I may find some time to test this out with some of my mappings.

@ronso0
Copy link
Copy Markdown
Member

ronso0 commented Apr 4, 2024

Btw I think we should add a 'button' style for the label strings, so that the mapping only contains This will light up the :btn:SYNC:/btn: (or whatever syntax fits)

:hwbtn:LABEL?
#13046

@acolombier acolombier force-pushed the feat/enable-settings-midi-controller-engine branch from 4fd2de6 to c47c7d2 Compare April 5, 2024 18:02
@acolombier
Copy link
Copy Markdown
Member Author

Merge?

@ronso0
Copy link
Copy Markdown
Member

ronso0 commented Apr 13, 2024

Sorry, I only checked the GUI so far, not the actual settings (I expect that to work, though someone should check it).

@JoergAtGithub
Copy link
Copy Markdown
Member

I will check this on Monday. I need to set up a MIDI device first.

Copy link
Copy Markdown
Member

@JoergAtGithub JoergAtGithub left a comment

Choose a reason for hiding this comment

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

I just tested this with a MIDI device. The settings functionality itself works as expected, but not the Restore Defaults button. It unloads the whole mapping instead of resetting the setting values.

@acolombier
Copy link
Copy Markdown
Member Author

image

Could you share a video of the behaviour? It sound like the expected behaviour if there is no exact mapping match

@JoergAtGithub
Copy link
Copy Markdown
Member

JoergAtGithub commented Apr 15, 2024

MidiRestoreDefaultsAufzeichnung.2024-04-15.233537.mp4

Here the mapping files:
Reloop_KUT.zip

@acolombier
Copy link
Copy Markdown
Member Author

acolombier commented Apr 16, 2024

This looks like expected behaviour when the device doesn't match 100% the mapping definition. Could you confirm the mapping combobox reset to No mapping too?

The custom settings of the current mapping are staged for reset, but when you re-select it in the combobox, you undo the reset (as per intended). If you want to reset the setting, you should:

  • Click the Restore Defaults
  • Click Ok/Apply

Then reselecting the mapping will use the default values.

I guess we could change this behaviour but that's out of scope for this PR. If you think the flow should be different, could you create an issue detailing specifically behaviours for different usecase? I'd be happy to address them in a separate PR.

@JoergAtGithub
Copy link
Copy Markdown
Member

This looks like expected behaviour when the device doesn't match 100% the mapping definition. Could you confirm the mapping combobox reset to No mapping too?

Yes, can confirm it's exactly the behavior you described for a device that doesn't match 100% the mapping definition.

But matching can't be true for a MIDI device due to:

bool MidiController::matchMapping(const MappingInfo& mapping) {
// Product info mapping not implemented for MIDI devices yet
Q_UNUSED(mapping);
return false;
}

Therefor this PR is correct and the controller ettings code will work as soon as MidiController::matchMapping get implemented. The behavior without MidiController::matchMapping implemented is safe and just a UI glitch.

@JoergAtGithub JoergAtGithub merged commit 9db6089 into mixxxdj:main Apr 16, 2024
@JoergAtGithub
Copy link
Copy Markdown
Member

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants