MIDI for light: add settings ui#13721
Conversation
|
Thank you for this PR. Do you mind attaching a screenshot of the new settings? |
|
I allready signed the agreement. Here is a screenshot: |
|
Okay, thank you, I didn't remember you already worked on MIDI for lights. The VU checkboxes could use more human-readable labels, not just the internal variable names, no? I didn't work with the controller settings, yet, wasn't there an option/flag that would arange group items horizontally if the preference page allows it? |
|
I didnt work with the controller settings either, If there is a documentation for it i can set other flags. Leaving the Orientation to unset flipps checkboxes to a row while resizing the settings wich is really destroying the layout: |
@acolombier Do we have this described in the manual? The best reference is the mapping for the S4 Mk3: #12995 |
|
The Mapping xml file is described at https://github.com/mixxxdj/mixxx/wiki/MIDI%20controller%20mapping%20file%20format but not the ui Stuff. I Used the mentioned diff as reference. |
|
The |
|
Is anything missing or is everyone OK with the design? |
|
I'm not familiar with controller settings. |
|
The settings syntax looks goot to me. But I'm not familar with MIDI for Light. |
| var enable_vu_right_average_meter = false; // set to false if you not need VU right average meter | ||
| var deck_ending_time = 15; // set a time (in seconds) in which the playing track is considered to be ending | ||
| var deck_ending_priority_factor = 0.9; // decrease the priority of the ending track by this factor | ||
| var midi_channel = engine.getSetting("midi_channel"); // set midi_channel. Valid range: 1 to 16. |
There was a problem hiding this comment.
In previous mappings we've "defensively" added the default values here too (either using the nullish coalescing operator ?? or ||), even though we've already declared them in the XML.
I don't think that should be necessary, so this looks good to me, but just to confirm, is this safe @acolombier?
There was a problem hiding this comment.
AFAIK, the default values there were only necessarry for backward compatibility of the mapping to 2.4 without the controller settings feature.
There was a problem hiding this comment.
Ah interesting, so engine.getSetting was already available in 2.4, but invocations always returned undefined?
There was a problem hiding this comment.
It does not exist in 2.4, and is therefore undefined.
There was a problem hiding this comment.
You can't call undefined without getting a TypeError though.
There was a problem hiding this comment.
I can confirm the mapping does not work with the proposed fallback. Changing the line to
var midi_channel = engine.getSetting("midi_channel") || 1;
causes an error in Version 2.4:
Uncaught exception: file:///C:/Program Files/Mixxx/controllers/Midi_for_light-scripts.js:19: TypeError: Property 'getSetting' of object ControllerScriptInterfaceLegacy(0x16b9d336520) is not a function
There was a problem hiding this comment.
var midi_channel = engine.getSetting ? engine.getSetting("midi_channel") : 1; seems to work
There was a problem hiding this comment.
Depending on how new the JS engine is you might be able to do
engine.getSetting?.("midi_channel") ?? 1But tbh if we're merging this to 2.5, it shouldn't be needed anyway (and I don't think there'll be many 2.4 releases anymore apart from minor bug fixes)
|
I have added some documentation in the wiki, hopefully this can help refining the UI a bit. |
|
So, should I
|
|
I'd say targeting 2.5 is fine, along with keeping everything as is. From a quick glance the PR looks good, I'll see if I find some time to test it soon. |
|
I have tested this in QLC+1 and it seems to work well. Thanks! Footnotes
|
5 participants
Adds Setting definition for UI of the Midi for Light Script