Registering MIDI Input Handlers From Javascript

[christophehenry]

This is a try at implementing the Registering MIDI Input Handlers From Javascript.

My C++ knowledge is close to zero so beware of the memory managment problems ahead.

The implementation is still very rough as I am discovering the code. Nothing has been tested yet. I'm opening this PR to get review as early as possible. I still have to connect the MidiInputHandlerController to the QJSEngine.

I will appreciate any hint on this feature.

[Swiftb0y]

Thank you for this early PR, it is going into the right direction.

[Swiftb0y]

I think you can get rid of the group string. That only really makes sense for handlers defined in XML. but the entire reason for registering handlers from JS is that you don't need the XML (at least not the handler definitions).

[christophehenry]

This is the signature of components.Component.input. I feel like it may be confusing for the user if this argument is undefined. Don't you think?

[Swiftb0y]

Good point. I'm not really sure either. It doesn't really make sense for componentJS either and just came from signature of the XML-bound handlers.
I imagine usage of this API to look like this:

midi.makeInputHandler(..., (channel, control, value, status) => {...} );

If this was bound to an XML-originating handler, you could just simply supply the group if you need it right then and there.

midi.makeInputHandler(..., (channel, control, value, status) => {
    someComponent.input(channel,control,value,status, someGroupFromEnclosingScope
} );

If thats too much to type, HOF got you covered:

// once somewhere
const makeLegacyWithGroup = (group, handler) => 
    (channel,control,value,status) => handler(channel,control,value,status,group);

// then use it
midi.makeInputHandler(..., makeLegacyWithGroup(someGroupFromEnclosingScope, someComponent.input));

As I see it, the only reason the group was introduce in the first place was that there was more information available in the manually created XML than in the corresponding JS handler. Now that all the information is already available in JS (since thats where the handler is created), that loophole is not needed anymore. Do you agree?

[christophehenry]

Sounds good to me. I'll just need to remember about documenting this behavior when this is merged.

[christophehenry]

So if we don't distinguish anymore from anonymous and named function, I have to reintroduce this argument. At least for documentation consistency.

[Swiftb0y]

yes... you're right, to some extend. I still don't think its super useful though. Why would the handler take a group, when there is no way to specify one from the handler creation function. I still don't think you should be able to specify one because its just unnecessary overhead in the C++ domain.

[Swiftb0y]

lets keep the group for now, but please add some jsdoc comments and mark the group as deprecated, wdyt?

[Swiftb0y]

Also please sign the Mixxx Contributor Agreement and comment here when you have done so. It gives us permission to distribute your contribution under the GPL v2 or later license and the Apple Mac App Store. It is also helpful for us to have contact information for contributors in case we may need it in the future.

[christophehenry]

I signed the contributor agreement after opening #12767.

[Swiftb0y]

I signed the contributor agreement after opening #12767.

Thank you, I just had assumed you hadn't because github still labels you as a first-time contributor. ;)

[christophehenry]

Oh? May that be because my email address is private on github?

[Swiftb0y]

idk, possibly related to the fact that this PR is targetting a different branch to the previous one. Our CLI is not connected to github in any way. I doubled checked and you signed, so we're all good.

[christophehenry]

I think I need some help on design, here. To be able to store arbitrary callback in LegacyMidiControllerMapping, I would need to change ConfigKey::item from QStringto QJSValue. Calling toString() on a QJSValue that represents a callable just converts to "function() { [native code] }"` which creates an error. But this is a significant change. A big portion of the code is impacted.

[Swiftb0y]

yeah. so the problem stems from the fact that if the MidiInputMapping is MidiOption::Script, the MidiInputMapping::control ConfigKey-key is representing a kind-off "path" on the global object which the correct midi input handler can be accessed from (see any JS-based controller mapping for examples).

Changing Configkey is not the right solution, also that would make the group unnecessary (see also our previous discussion). Instead, you'll probably want to change MidiInputMapping to be polymorphic. Since its a value-type, changing MidiInputMapping::control from the ConfigKey it is currently to a std::variant<ConfigKey, QJSValue> and then change the code that uses MidiInputMapping to deal with the new type would be the best option. Does that make sense?

[christophehenry]

Hmm… Yes. I think I can do that. Thank you for the help.

[christophehenry]

So I tried the template solution. For now, I can't test it. A few compilation errors remain that I don't know how to solve with my low C++ knowledge. I would love some help here.

[christophehenry]

I wonder if it wouldn't be simpler to just maintain a second hash of mappings.

[Swiftb0y]

So I tried the template solution. For now, I can't test it. A few compilation errors remain that I don't know how to solve with my low C++ knowledge. I would love some help here.

Sure. I'm afraid your most recent commit to a bit of a wrong term and we should try again (see review comments). I'm sorry that my suggestions haven't been clear enough. Can you stash your most recent commit away and we'll try again? I think the std::variant based solution will be simpler.

I wonder if it wouldn't be simpler to just maintain a second hash of mappings.

Yes, certainly.

[christophehenry]

Ok, I think I got to something correct. I haven't tested yet but it compiles, which is a promising first step.

[Swiftb0y]

Thank you, This is is much more the direction that I initially suggested.

[JoergAtGithub]

@daschuer Your proposal sounds good, but I think for the simple 1:1 relations the XML should be always the prefered solution, and we should continue to require this for official mappings, because:

• Better maintainability of sematic information (we can automatically convert mappings to any future mapping syntax like QML at any time, if we once lose the sematic information, we are in a dead end and must support this particular syntax forever)
• Users without programming skills are able to do custom mapping modifications of existing mappings using the wizard (e.g. reassign a button that they don't use to a function that they use)

[daschuer]

That makes sense. Let's lint the second point by being cooperative and not install a mapping form JS if the midi value has been mapped via wizard.

[christophehenry]

I'm sorry, what are learned mappings?

[Swiftb0y]

Mappings that have been created by using the controller wizard

[christophehenry]

I'm not sure I understand what I should do next.

[Swiftb0y]

I think this is a good next small feature:

skip adding a JS callback when midi value is already mapped by XML

[mxmilkiib]

I'm sorry, what are learned mappings?

🤦 I think you are owed a bit of apology; if you're asking that, at this point, whilst actively developing the controller system, then it means we have failed in our attempts to advise on what the point of this FR and now PR is, and the variety of ways people.. relate to controller mappings.

The "problem" [statement] this FR and now PR is solving is different in everyone's head.

It feels like a number of people think it would rock the boat too much to holistically point this out (and actively reassess the drawing board) and instead piecemeal work through any implementation and only inform the person doing the work of the ramifications and requirements one at a time. If anything, I think this is a bit disrespectful to @christophehenry.

The difference or overlaps between entry level user mapping creator, advanced user mapping creator, and dev mapping creator is not clear in this conversation. Heck, the fact that mappings can be created in Vim before any use starts, or in Mixxx during use (this being what "learned mappings" refers to), hasn't been made clear.

Let us, everyone, please, turn down the bolshie level, and be humble, and slow down, and admit that there are questions that need to be actively and deeply listened to and mindfully approached for various points of consensus to be achieved, and that there may possibly need to be some element of backtracking, at least widening in conceptions.

There's no magic path to everyone grokking everything that needs to be grokked, and for solutions to become obviously apparent.

I don't think there should be any shame in pointing all this out (n it's not fair to try intimate there is), or in pivoting to reassess understanding and implementation.

If it is feared that reassessing will slow things down, well I think it will actually be slower to not because energy will be shed at a far higher rate and development (which includes defining problem statements) will stall.

[christophehenry]

Is this feature planned for 2.5?

[Swiftb0y]

Depends on when we branched main (I don't remember of the top of my head). If we branched 2.5 after this pr got merged, it'll be in 2.5.

[mxmilkiib]

Would this be a method for [re]connecting pad MIDI presses like in this instance? I had assumed the ControllerJS input would work, but is it needing this feature? What kind of syntax would it be?

[christophehenry]

This is an alternative to registering MIDI handler from XML. Itks a implementation of https://github.com/mixxxdj/mixxx/wiki/Registering-Midi-Input-Handlers-From-Javascript

Edit: yes it seems to be what you're talking about. It's useful to me because the jog component from componentjs requires a deck number upon creation but on DN-S3700 deck number is retrieved with a sysex call that happen after input handlers creation.

[mxmilkiib]

I've got this currently, but now the pads aren't even lighting up. How far off the mark might it be?

edit; wait, that's just because of #13451 again.. I was using Arch binary before. Anyway, pads light up. update; oops that gist link was broken.

I fixed the script a bit but can't figure the logic to fix that bit..

[mxmilkiib]

I'm getting as far as getting this, but I can't find a way to fix the logic or encapsulation or whatever.

 controller.launchpadpromk3lppromk3midi.input:debug [Controller] "incoming: " "Launchpad Pro MK3 LPProMK3 MIDI: t:184167 ms status 0x90 (ch 1, opcode 0x9), ctrl 0x54, val 0x7F"
warning [Controller] ControlDoublePrivate::getControl returning NULL for ( "[Channel5]" , "hotcue_17_activate" )

Are there any code examples beyond the test? Cheers!

[Swiftb0y]

I think you're getting quite far. the problem now is likely that there is no [Channel5] created so it can't access a hotcue there. What skin are you using? You can can use the developer tools to check if [Channel5],hotcue_17_activate actually exists.

[mxmilkiib]

It doesn't, this is 4 decks, and whilst each deck has 32 or 36 or whatever hotcues, I'm only going up to 16.

I get an error when I try to use .bind to link the scope of the counter variables to that if the hotcue handler instantiations and not the prototype (if that's the right way to phrase it), the lack of ability to do so would thus clobber the values of the objects it creates, afaiu(?). Like with line 120 for the ComponentsJS method (and the reconnect subroutine did also?

Am I using the syntax wrong? Or does they new method, as it stands, not account for that requirement yet?

Late Night, though my own variation that provides 16 hotcues buttons for each deck.

[Swiftb0y]

right, but [Channel5] must be coming from somewhere, you probably have an off-by-one error somewhere. Having a quick look at the gist however, I can't tell you where exactly. Try adding some logmessages and see how that happens.

[Swiftb0y]

I think the problem is the mutability of the deck in line 155. try copying that into the handler closure on line 177.

[mxmilkiib]

Thanks for the feedback. I added the console output log, but indeed, that does sound about right. After the 4th loop, the loop counter has been ++ to 5, same with i to 17.

Ah yes, the deck and i loop counters should be made with let not var! Gosh, I've forgotten so much JS. This change appears to fix the main issue I was having. New hotcues are created, and existing ones get played.

The strange thing that still remains is that the deck UI play/pause button doesn't change after pressing a pad triggers a cue, needing two clicks to stop the playback.

[Swiftb0y]

Ah yes, the deck and i loop counters should be made with let not var! Gosh, I've forgotten so much JS. This change appears to fix the main issue I was having. New hotcues are created, and existing ones get played.

Yeah, just use const whenever you can, let whenever you must and var only in very very special edge cases (which you should probably consult with an expert on before).

The strange thing that still remains is that the deck UI play/pause button doesn't change after pressing a pad triggers a cue, needing two clicks to stop the playback.

yeah, thats probably because you simply reject the hotcuepad release message, leading mixxx to believe that you're still holding down the hotcuebutton even though you released it. If you simply delete the entire input definition of the HotcueButton in your mapping, it should work. The default hotcuebutton already does everything you want it to do. you only need to keep the sendRGB, group, midi and number definitions.

[mxmilkiib]

Aah, thanks for the feedback. I've commented out the input but I don't get any activity on pressing the lit pads now. I had only asked about it here and added that method because this was the case before. I had thus come to think the ComponentJS system was a bit weak in functionality, but if .HotcueButton is really meant to provide an input that, then I was wrong to think that, but I'm back to that square again, albeit mildly wiser.

[Swiftb0y]

ah, this is conflating multiple things now. So currently ComponentsJS is not yet setup to automatically bind input handlers. Currently you don't get any input because you commented out the code where the input handling connection is made. Until ComponentJS has native support here, you'll want:

this.hotcueButtons[i-1] = new components.HotcueButton({...});
this.hotcueButtons[i-1].inputHandlerHandle = midi.makeInputHandler(0x90, padAddress, someArray[i].input);

You can then use this.hotcueButtons[i-1].inputHandlerHandle.disconnect() IIRC if you ever want to unbind that handler again.