call MIDI input script functions with appropriate "this" object

[Be-ing]

Fixing bug https://bugs.launchpad.net/mixxx/+bug/1567203

[Pegasus-RPG]

Won't this cause performance problems if a script makes liberal use of the "this" keyword? Or is resolveThisObject() intended to be called just during initialization?

[Be-ing]

I modified the commit since opening the PR. I realized there wasn't a need for a new function at all. resolveFunction's caching works this way and the function can still write properties to the "this" object. I don't know why the caching I tried when I had it in a separate resolveThisObject function didn't work.

[Be-ing]

Do you think it would make sense to rename resolveFunction() to resolveObject()?

[daschuer]

Thank you for working on this.

I think we need to to some more refactoring to not introduce a performance regression.

It looks like for your approach ControllerEngine::resolveFunction is broken.
How about return function and Object as output parameter? The Object and the function should be both cached in the m_scriptValueCache.

It looks like resolveFunction should return m_pEngine->globalObject() , if there is no obj found in the "obj.func" schema.

[Be-ing]

What is the performance concern with the way it is? Making two function calls rather than one? How would you recommend returning two values, with a std::pair? tuple? QHash?

[rryan]

Won't this cause performance problems if a script makes liberal use of the "this" keyword? Or is resolveThisObject() intended to be called just during initialization?

The function body (script usage of this) doesn't matter since this is already resolved at that point -- the only performance concern is that it adds an additional environment inspection per incoming MIDI message.

[daschuer]

resolveFunction() might be longer than the final script call by dimensions.
All QString function calls shoulder be reduced to a minimum.
Since the can be very short we should squeeze the lookup code as much as possible.

[daschuer]

Don't use a std::pair, tuple or similar anonymous structs. These are only useful for obfuscation.

In this case you may consider to call the execute from inside the resolveFunction or use output parameters.

void resolveFunction(const QString& function,  QScriptValue* pThisObject, QScriptValue* pFunction ) const;

[rryan]

Thanks for working on this! I've also been bothered by the right way to get 'this' pointing to something that makes sense in script handlers.

I think eventually we want to get away from resolveFunction and instead eval whatever expression the XML gave us. If the result of that eval is a function then we call/cache it. Doing dot-based environment traversal is kind of doing the work of eval but probably inaccurately. The "function prefixes" we use for init/shutdown are particularly hacky too and I'd like to see them go.

I think we might also encourage script writers to properly bind their event handlers to the object the handler lives in using Function.prototype.bind. We could even provide a helper method to bind handlers in our common Controller class we provide. Then it wouldn't matter what context we execute the method with.

Anyhow.. what we do is roughly the equivalent of:

function Controller() {
  this.name = "FancyController";
}

var MyController = new Controller();
MyController.handler = function() {
  console.log(this.name);
}

Mixxx then does the rough equivalent of:

var theHandler = eval("MyController.handler");
theHandler();

And you get:

Cannot read property 'name' of undefined

OTOH if you do:

eval("MyController.handler()");

You get"

FancyController

If the function were bound:

MyController.handler = function() {
  console.log(this.name);
}
MyController.handler = MyController.handler.bind(MyController);

var theHandler = eval("MyController.handler");
theHandler();

Then it would work as expected:

FancyController

[Be-ing]

All QString function calls shoulder be reduced to a minimum.

How about storing the QScriptValue for the "this" object for each function name in a cache along with the QScriptValue for the function?

[Be-ing]

I think we might also encourage script writers to properly bind their event handlers to the object the handler lives in using Function.prototype.bind. We could even provide a helper method to bind handlers in our common Controller class we provide. Then it wouldn't matter what context we execute the method with.

I'd rather that Mixxx found the appropriate this object itself. My broader goal is to make writing complex scripts more convenient with a minimum of repetitive boilerplate code. IMO binding every callback function is annoying. I can test again, but I think I did try binding a callback function but it didn't do anything (probably because the C++ called the function with this explicitly set to the global object anyway).

[Be-ing]

I think eventually we want to get away from resolveFunction and instead eval whatever expression the XML gave us. If the result of that eval is a function then we call/cache it. Doing dot-based environment traversal is kind of doing the work of eval but probably inaccurately. The "function prefixes" we use for init/shutdown are particularly hacky too and I'd like to see them go.

I agree. I think this could also resolve https://bugs.launchpad.net/mixxx/+bug/1565377 . It could also allow snippets of JS to be entered in the mapping GUI.

[daschuer]

Yes, we should do this.
It would be even better to evaluate the strings only once and store the resolved script values along with the strings. Don't know if this can be part of this PR or not.

We have actually real performance issues with High resolution jog wheels here:
For example the SCS.1d uses 3xmidi speed which results in an update rate of
3125 messages per second

[Be-ing]

We have actually real performance issues with High resolution jog wheels here:
For example the SCS.1d uses 3xmidi speed which results in an update rate of
3125 messages per second

Yes, I am aware. I do not have any SCS controllers or controllers with jog wheels though, so others would have to test if changes impact real usability in such demanding contexts.

[Be-ing]

Hm, I realized that it's kinda silly to handle this upon receipt of every MIDI message. It would make more sense to store the QScriptValues in a cache upon loading the mapping, then refer to that when MIDI messages are received.

[daschuer]

We do not need a SCS to check the performance changes.
Just counting the function calls parsing the string will give a good estimation.
To proof it, you may set up a ScopedTimer like we did in other places.

It is obvious that every "." in the sting is an extra cache lookup.
If we can replace the cache, by a storage along with the string, it allows us to use things like
P32.leftDeck.hotcueButtons[2].input with less pain.

[rryan]

Here's how the standard says this should work:
http://www.ecma-international.org/ecma-262/5.1/#sec-11.2.3
http://www.ecma-international.org/ecma-262/5.1/#sec-8.7

So this describes why

foo.bar(1, 2, 3)

calls bar with this set to foo but

var f = foo.bar;
f(1, 2, 3);

calls bar with this set to the global object.

The grammar production rule for function calls distinguishes between an object reference and a free standing value. When foo.bar is part of the expression bar is an object reference but when it's a standalone value it's not.

Our traversal of the environment and call of bar is equivalent to the latter example. To get normal javascript behavior, we probably want the former.

Using the prior value in the dot traversal would mostly approximate the GetBase function defined by the standard for object references so this is a pretty reasonable fix! However it wouldn't work for

foo.bar[foo.currentDeck].handler

For that we probably need to build a Javascript expression parser to do it right (notice how splitting on dots no longer works for that case).

[rryan]

Also note that if we want to support:

foo.bar[foo.currentDeck].handler

this means caching is no longer an option since we need to re-evaluate foo.currentDeck each time to be correct

[rryan]

please also update sysex handling (line 551) for consistency

[rryan]

So I would be in favor of merging this as a short term fix. To minimize perf impact, resolveFunction and the script value cache should probably be updated to return a std::pair of (context, value) where context is the last value in the dot chain before value -- that way we'd be doing essentially no more work than we already are per MIDI message.

[Be-ing]

Okay, I'll work on that as a short-term fix. I think trying to implement fully correct JS behavior by parsing strings in C++ would likely end in other hacks that don't really work with the full flexibility of JS. I think to really take advantage of all JS has to offer, the input handler callbacks need to be registered by scripts (generally in their init functions). Then getting the correct this object would be easy, like it is done in https://github.com/mixxxdj/mixxx/blob/master/src/controllers/controllerengine.cpp#L751

[daschuer]

Ok, I understand we cannot cache the object because it may vary.

But foo.bar[foo.currentDeck].handler is anyway too complex to compile it from a string on each midi update.

I think there are some possible ways to solve it:

• disallow such expressions where the "this" context varies.
• detect the case (multi dots or arrays) and wrap it into an anonymous function

Pseudo code:

foo.mapping8044 = function (channel, control, value, status, group) {
    foo.bar[foo.currentDeck].handler(channel, control, value, status, group);
}

QString wrapper = 
        "foo.mapping8044 = function (channel, control, value, status, group) {" +
        mapping.control.item +
        " (channel, control, value, status, group); }";
m_pEngine->evaluate(wrapper); 

// resove foo
// resove mapping8044
// Store both QScriptValues along with "mapping.control.item"

If we found out the compiler is smart enough to ditch the extra call, we can always use the evaluate.
We may also allow to inline whole script functions to the xml file.

---

should probably be updated to return a std::pair

never use std::pair is a source of later confusion. These are good alternatives

• output parameters
• define your own struct / class

[rryan]

never use std::pair is a source of later confusion. These are good alternatives

output parameters
define your own struct / class

Hm, I disagree with "never use std::pair". It's not too hard to read the function documentation to learn what it returns -- it's the same as reading the function prototype to learn what the output parameters are or reading the struct definition to learn what members it has. I agree that a pair isn't good to use as a data model that is prevalent across the codebase but for an internal implementation detail it's fine.

[rryan]

wrap it into an anonymous function
...
If we found out the compiler is smart enough to ditch the extra call, we can always use the evaluate.

Unless we have data showing an extra pre-evaluated-by-the-interpreter function call is worse than what we have now with resolveFunction poking at the environment repeatedly from the QtScript API then I wouldn't worry about it. Javascript interpreters are fast from years of optimizations trying to get fancy webapps to work faster and I'm not sure our QtScript API and string manipulation would win against a JS interpreter's evaluation of the user provided expression -- especially if the code is already parsed (which it would be after our one-time evaluate) and better yet it would be a correct implementation of the JS spec!

On the whole wrapping every script handler in an anonymous function is a pretty nice solution and only requires the string formatting once up front.

Pro:

• we only have to evaluate the wrapper once -- no need for resolveFunction
• the variables pointed to by the expression provided by the user can change or not.
• complicated expressions work as long as they return a function: MyController.deck[MyController.activeDeck].Handler("hotcue_10") is valid if it returns a function.
• we get the "real" Javascript foo.bar() behavior where bar is executed with this="foo".

Con:

• None demonstrated so far?

[daschuer]

There are situations where pair helps, but they are very rare. You can use std::tie to make it less scary. But even that involves some anonymous boiler plate which should be avoided.

In this case this reads well:

QScriptValue thisObject; 
QScriptValue function;
resolveFunction(mapping.control.item, &thisObject, &function );
if (!pEngine->execute(function, thisObject, channel, control, value, status,
                              mapping.control.group, timestamp)) {

If the pointers are to scary for you we may use

struct ScriptMemberFunction{
    QScriptValue thisObject; 
    QScriptValue function;
}

ScriptMemberFunction memberFunction = resolveFunction(mapping.control.item, pThisObject, pFunction );
if (!pEngine->execute(memberFunction.function, memberFunction.thisObject, channel, control, value, status,
                              mapping.control.group, timestamp)) {

This is far more self explaining than

std::pair<QScriptValue, QScriptValue> memberFunction = resolveFunction(mapping.control.item, pThisObject, pFunction );
if (!pEngine->execute(memberFunction.fist, memberFunction.second, channel, control, value, status,
                              mapping.control.group, timestamp)) {

[rryan]

So here's my proposal based on @daschuer's -- at mapping initialization time turn every handler into an anonymous function and evaluate it once then and not when MIDI messages come in:

QString wrapper = "function (channel, control, value, status, group) { (" + 
    mapping.control.item + ")(channel, control,value, status, group); }";
m_pEngine->evaluate(wrapper); 

[rryan]

There are situations where pair helps, but they are very rare. You can use std::tie to make it less scary. But even that involves some anonymous boiler plate which should be avoided.

I agree that if the type will spread beyond the file it lives in then it's best to define a type for it. However I think we're going to have to agree to disagree on whether it's ok to use std::pair in local contexts.

[rryan]

Backtrace:

Could you type thread apply all bt once it crashes to get the full stack trace? That's just the line it crashed on.

[Be-ing]

Full backtrace attached
test-crash.txt

[Be-ing]

I'm using qt-4.8.7-12.fc23.x86_64

[Be-ing]

Okay, wrapFunctionCode() always caches the QScriptValue function now.

[daschuer]

This loop should be moved to:
ControllerEngine::initializeScripts and fill a simple QList m_incommongDataFunctions
which is only called once.

Here we can use than a simple .. without any string arithmetic:

for (const auto& incomingData: m_pEngine->m_incommongDataFunctions()) {
    if (!m_pEngine->execute(incomingData, data, timestamp)) {
        qWarning() << "Controller: Invalid script function" << function;
    }
}

[Be-ing]

Okay, I found a way around having to deal with escaping JS within JS within C++. This still gives meaningful error messages on every button press in the log, but only pops up an attention-grabbing dialog the first time the button is pressed. Note that this requires doing the m_pEngine->evaluate() before checking the syntax because m_pEngine->checkSyntax() does not generate an exception.

[Be-ing]

It would be easy to call scriptErrorDialog(functionObject.toString()) here, but IMO popping up a dialog repeatedly is annoying. When debugging a mapping, I'd rather look through the log than have to click to close a bunch of dialogs.

[rryan]

Full backtrace attached
test-crash.txt

Thanks -- that's an odd one:

Thread 1 (Thread 0x7ffff7f798c0 (LWP 19496)):
#0  0x00007ffff47805da in QString::fromLocal8Bit(char const*, int) (str=0x21 <error: Cannot access memory at address 0x21>, size=size@entry=-1)
    at tools/qstring.cpp:3960
#1  0x00007ffff48466a1 in QCoreApplication::arguments() () at kernel/qcoreapplication.cpp:2345

I think this is probably related to google-test parsing of commandline flags (it mutates argc) and QApplication's use of command line flags. Can you try messing with this line:
https://github.com/mixxxdj/mixxx/blob/master/src/test/main.cpp#L27

For example:

• printing out the argv array before and after testing::InitGoogleTest
• passing 1 as the argc to applicationScope

Not really sure what's going on. Maybe google test is dropping argc to 0. QApplication says argc needs to be at least 1. I tried on Mac and Linux and argc is always 1 when I invoke mixxx-test as "./mixxx-test".

[Be-ing]

Actually, I don't think wrapFunctionCode() needs to check syntax. The "Parse error" message is generic, but MidiController::processInputMapping prints the offending code snippet in the log with each button press. Passing the QScriptValue Error to execute() then internalExecute() prints "Parse error" along with each button press as well. That may be difficult for many users to understand, but I think mapping developers would know to look at the log and then they'd see quite clearly what happened.

[Be-ing]

The exception error messages from evaluating the wrapper look pretty much the same as those for just executing codeSnippet, so yes, it is fine to just evaluate the wrapper.

[rryan]

Actually, I don't think wrapFunctionCode() needs to check syntax. The "Parse error" message is generic, but MidiController::processInputMapping prints the offending code snippet in the log with each button press.

Cool, ok -- this LGTM. Thanks for the changes!

[Be-ing]

Great! I'll add a couple more things to the P32 mapping not depending on this that will be compatible with 2.0, then I'd like to branch off from that and take advantage of this.

[rryan]

Not really sure what's going on. Maybe google test is dropping argc to 0. QApplication says argc needs to be at least 1. I tried on Mac and Linux and argc is always 1 when I invoke mixxx-test as "./mixxx-test".

I think I fixed the problem. Uggggggh. 8e905c7