Merge lp://qastaging/~mixxxdevelopers/mixxx/features_midiscript_improvements into lp://qastaging/~mixxxdevelopers/mixxx/trunk

Just FYI, the MIDI subsystem has been completely gutted in features_controllerAbtraction. This branch should still merge fine with it, but the changes will end up in the OLDmidi/ directory, and applicable improvements will need to be manually migrated to the classes in the midi/ directory.

I'm well aware of that. I'd like to get the branch reviewed though before I try to create one off of Controller Abstraction with these new features.

Looks good overall.. I've got some comments:

* I don't see any edits to timer related stuff?

* The filesystem watcher complicates MidiMapping (and MidiMapping was really freaking complicated to begin with). Thankfully MidiMapping doesn't exist anymore in the new branch. When you re-implement it please do it in a helper class and ideally from outside of the Controller/MidiController classes. I'm envisioning whatever code loads the preset notifying your helper class to "please notify me when thsi mapping changes so that I can reload the mapping on this device". I don't know exactly where this happens in the new branch -- Sean would know.

(In the new branch parsing/loading the mapping is still done in the Controller/MidiController. I've asked Sean to get that stuff out of there too.)

MidiMapping::resolveFunction()
  -- AFAIU (correct me if wrong) you resolve all script functions string names (the ones loaded from file) at script load time. Why not do it at call-time and allow those to be expressions? That way you could have an XML mapping that maps to a script expression e.g. "getHandler('keylock')". This would be kinda neat because the script could dynamically change the handler Mixxx invokes based on the result of whatever expression is in the .midi.xml. There are a whole lot of hoops that script writers (c.f. the SCS3d scripts) go to to dispatch the static function to the appropriate handler based on the current deck. We could eliminate the need for that with this sort of setup.

In midiscriptengine.cpp

QScriptValue MSE::resolveFunction(QString)
  -- Why not just eval() the string? This method would fail on an input like
     "MidiFighter.Decks[MidiFighter.Deck1].Handlers.PlayHandler"

-------

Later on you generate the valid script functions by iterating just 2 levels deep. What if we nested functions three levels deep? I think we should just stop checking whether functions are valid at all and deliver a warning at call time if we can't resolve a given expression to a function to call.

* Rename MidiScriptEngineControllerConnection -> ControlConnection in the new branch? That name is pretty long :)

----------

In connectControl:
  -- lookup of MSECC creates a copy of it.
In slotValueChanged
  -- again, copying MSECC
In MidiScriptEngineControllerConnectionScriptValueProxy(MidiScriptEngineControllerConnection conn)
  -- copying the input MSECC

The copying of the MSECC isn't a big deal unless you didn't mean for them to be copied. Edits to one aren't reflected in any of the others. I didn't see any point at which you mutated an MSECC after creating it. If you don't intend for anything to change, maybe it makes sense to make it const?

* the operator== method for MSECC says:
/* comparison function for ConfigKeys. Used by a QHash in ControlObject */
copypasta?

* I think it would be simpler if instead of having the MSECCSVP, you just passed the auto-generated id as the return value of connectControl. Then the user would just disconnectControl with that ID to disconnect it. This is a lot like the way startTimer works -- you get a timer ID and then to stop the timer you just call stop on that ID. Injecting QObject's into script la...

> MidiMapping::resolveFunction()
> -- AFAIU (correct me if wrong) you resolve all script functions string names (the ones loaded from file) at script
> load time. Why not do it at call-time and allow those to be expressions? That way you could have an XML mapping
> that maps to a script expression e.g. "getHandler('keylock')". This would be kinda neat because the script could
> dynamically change the handler Mixxx invokes based on the result of whatever expression is in the .midi.xml.
> There are a whole lot of hoops that script writers (c.f. the SCS3d scripts) go to to dispatch the static
> function to the appropriate handler based on the current deck. We could eliminate the need for that with
> this sort of setup.

The calling convention for MIDI Scripts added a group argument a while ago just for that. I do see how this would be convenient but I'd actually argue that extra parameters should be added as a second option. That way we can preserve the calling convention and change it if we need to under-the-hood without breaking the XML mapping.

> In midiscriptengine.cpp
>
> QScriptValue MSE::resolveFunction(QString)
> -- Why not just eval() the string? This method would fail on an input like
> "MidiFighter.Decks[MidiFighter.Deck1].Handlers.PlayHandler"

That is a handful. Who would be calling that? If we want something like this we should just dump the entire idea of MIDI Mappings and throw MIDI events straight into a single object/function or something and do the call resolution there.

> Later on you generate the valid script functions by iterating just 2 levels deep.
> What if we nested functions three levels deep? I think we should just stop checking
> whether functions are valid at all and deliver a warning at call time if we can't
> resolve a given expression to a function to call.

That code is cruft. I will be nuking it in short order.

> /* comparison function for ConfigKeys. Used by a QHash in ControlObject */
> copypasta?

Totally copypasta.

As for the whole MSECC debacle:

  * MSECC's are never mutated so I'll make them const in all other functions.
  * I think I could just resolve or cast the MSECC if passed to connectControl
    so you can treat the return as if it were an ID.
  * Doing the former I don't need any silly side hash hacks but still be pretty
    foolproof.

As a final conclusion, getting into the weird territory of eval()'ing each MIDI call is extremely problematic, variables can change values, parsing each time will not make it faster under any circumstance.

There are some pretty substantial indicators that V8 will be used in Qt5, if so direct calling will be blazingly fast and indirect calling will most likely become muuuuuuuuch slower.

Those are my comments for now.

And here I present a long diatribe concerning the invocation method of MIDI Script bindings:

But, whatever. I had this brilliant idea of nesting any expressions used for binding inside a function with a return statement. This would cover the current and most likely case where it is simply bound to the method of an object but also support weird, freaky syntax like: 'get("keylock");' or 'machine.handlers[deck].fader' or whatever else you can come up with. As usual, compile time errors fall on the end user. An example of what the function would look like for a binding:

function() {
    return get("keylock");
}

Useless, canoot be done, since we need to tack on the MIDI status, channel, value parameters as well as the group parameter. When executing the actual binding it will pass the arguments to the outermost function. Under the old system all you get with binding to 'get("keylock")' is the attempt to pass those arguments to a function returned by get("keylock"). On the other hand complicated, nested references to variables probably would work with the old invocation system. It does lead to the possibility though of having MIDI commands at some points cause errors and then at other times not without easily detecting the root of the problem. There are always to do this in Javascript though.

As an aside:

This blog posts describes how QML itself will try and optimize small expressions: http://cdumez.blogspot.ca/2010/12/ways-to-improve-qml-performance.html.

The URL for information concerning the V8 port of QtScript: http://qt-project.org/wiki/V8_Port.

The resolveFunction should be able to handle infinite levels of recursion

> * Rename MidiScriptEngineControllerConnection -> ControlConnection in the new branch?
> That name is pretty long :)

I admit it is long but I made it so to make sure it does not conflict with future code.

> The calling convention for MIDI Scripts added a group argument a while ago just for that. I do see
> how this would be convenient but I'd actually argue that extra parameters should be added as a
> second option. That way we can preserve the calling convention and change it if we need to
> under-the-hood without breaking the XML mapping.

Right -- but the group argument doesn't help for what the SCS3d does. The point is you don't know the group ahead of time -- it depends on the currently selected deck. getHandler('keylock') would do approximately this if you were using it for the SCS3d script:

function getHandler(key) {
  return SCS3d[SCS3d.currentDeck].Handlers[key];
}

This has nothing to do with the calling convention -- this is simply an expression you can use to resolve the handler function.

To be clear, the way we do this in practice today e.g. "<key>StantonSCS3m.cueL</key>" -- that is a Javascript expression which when evaluated, produces the handler function we will invoke when the bound command is received over MIDI. My main beef with this and your new implementation of resolveFunction is that it implements a subset of possible Javascript expressions (e.g. 'StantonSCS3m.cueL', 'StantonSCS3m.Deck1.cueL' and so on).

Why not allow anything to be in the <key> block as long as it is a valid Javascript expression which evaluated to a function that takes the appropriate # of arguments. For example:
"<key>getHandler('keylock')</key>"
"<key>function (channel, control, value, status, side) { print('hello world'); }</key>"
"<key>MidiFighter.Deck[MidiFighter.CurrentDeck].Handlers['keylock']</key>"
"<key>VCI100.ButtonA</key>"

These are all Javascript expressions which evaluate to a function that takes the appropriate number of arguments (the "calling convention"). Performance is the only reason I can think of not to but we haven't quantified that yet.

> That is a handful. Who would be calling that? If we want something like this we should just dump the
> entire idea of MIDI Mappings and throw MIDI events straight into a single object/function or
> something and do the call resolution there.

Well, someone who wanted to avoid having to write a selected-deck dispatch handler in script could do it this way. It would be more of an advanced use-case but could tighten up the SCS scripts a fair amount.

> As a final conclusion, getting into the weird territory of eval()'ing each MIDI call is extremely > problematic, variables can change values, parsing each time will not make it faster under any
> circumstance.

As I said -- I think dynamically resolving handlers could be useful to advanced script writers and basic script writers wouldn't notice. I agree there's no way this could make things faster. I'd be very curious to know whether it makes things significantly slower though.

> Useless, canoot be done, since we need to tack on the MIDI status, channel, value parameters as
> well as the group parameter.

Do you mean that we would stick

> function() {
> return get("keylock");
> }

In the <key> block in XML ? Or that you would give that a name in the JS file and stick that name in the <key> block. The former would work if you...

> > * Rename MidiScriptEngineControllerConnection -> ControlConnection in the new branch?
> > That name is pretty long :)

> I admit it is long but I made it so to make sure it does not conflict with future code.

K.. how about "ScriptControlConnection"? It won't be specific to MIDI anymore anyway in the HID branch.

I have anecdotal evidence that direct binding is just enough of a push to get my jogwheels on Mac OSX to work *PRECISELY* without unwanted follow through, etc...

Just to clarify, RJ's above example that ends up just returning a keylock status can _today_ be written:

function () {
  return get('keylock');
}

(assuming get() exists) since JS still calls the same function by name regardless of the signature. (The lack of parameters just means they are dropped.)

On Sun, Mar 18, 2012 at 7:21 PM, Sean M. Pappalardo <email address hidden>
 wrote:

> (assuming get() exists) since JS still calls the same function by name
> regardless of the signature. (The lack of parameters just means they are
> dropped.)
>

This isn't really what we're talking about. What I'm proposing is
definitely not possible today.

Maybe it would help if I showed an example of how this would work in
practice:

NumarkV7.DeckA.Play = function (channel, control, value, status, side) {
}
NumarkV7.DeckB.Play = function (channel, control, value, status, side) {
}

function getHandler(button) {
   // for an arg of 'play' returns DeckA.Play or DeckB.Play depending on
what the currently selected deck is
}

get('keylock') looks up a message handler and returns that -- it is not a
message handler itself. If you put in your XML mapping
"<key>getHandler('play')</key>" the idea is that the handler for that MIDI
message can change on the fly.