Skip to content

call MIDI input script functions with appropriate "this" object #919

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
rryan merged 7 commits into from
Apr 13, 2016
Merged

call MIDI input script functions with appropriate "this" object #919

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
4732970
call MIDI input script functions with appropriate "this" object
Be-ing Apr 7, 2016
131ed5f
Revert "call MIDI input script functions with appropriate "this" object"
Be-ing Apr 10, 2016
0db778c
treat <key> fields in MIDI mapping XML files for script functions as …
Be-ing Apr 11, 2016
7ffefe0
add numberOfArgs argument to ControllerEngine::wrapFunctionCode
Be-ing Apr 11, 2016
9d3df51
always cache function in ControllerEngine::wrapFunctionCode
Be-ing Apr 11, 2016
d49025a
return QScriptValue Error objects from ControllerEngine::wrapFunction…
Be-ing Apr 11, 2016
66bc5dc
only evaluate codeSnippet once in ControllerEngine::wrapFunctionCode
Be-ing Apr 13, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/controllers/controller.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ void Controller::receive(const QByteArray data, mixxx::Duration timestamp) {
continue;
}
function.append(".incomingData");
QScriptValue incomingData = m_pEngine->resolveFunction(function);
QScriptValue incomingData = m_pEngine->wrapFunctionCode(function, 2);
Copy link
Copy Markdown
Member

@daschuer daschuer Apr 11, 2016

Choose a reason for hiding this comment

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

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;
    }
}

if (!m_pEngine->execute(incomingData, data, timestamp)) {
qWarning() << "Controller: Invalid script function" << function;
}
Expand Down
95 changes: 61 additions & 34 deletions src/controllers/controllerengine.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,30 +98,38 @@ void ControllerEngine::callFunctionOnObjects(QList<QString> scriptFunctionPrefix
}
}

/* -------- ------------------------------------------------------
Purpose: Resolves a function name to a QScriptValue including
OBJECT.Function calls
Input: -
Output: -
-------- ------------------------------------------------------ */
QScriptValue ControllerEngine::resolveFunction(const QString& function) const {
QHash<QString, QScriptValue>::const_iterator i =
m_scriptValueCache.find(function);
if (i != m_scriptValueCache.end()) {
return i.value();
}
/* ------------------------------------------------------------------
Purpose: Turn a snippet of JS into a QScriptValue function.
Wrapping it in an anonymous function allows any JS that
evaluates to a function to be used in MIDI mapping XML files
and ensures the function is executed with the correct
'this' object.
Input: QString snippet of JS that evaluates to a function,
int number of arguments that the function takes
Output: QScriptValue of JS snippet wrapped in an anonymous function
------------------------------------------------------------------- */
QScriptValue ControllerEngine::wrapFunctionCode(const QString& codeSnippet,
int numberOfArgs) {
QScriptValue wrappedFunction;

QScriptValue object = m_pEngine->globalObject();
QStringList parts = function.split(".");
QHash<QString, QScriptValue>::const_iterator i =
m_scriptWrappedFunctionCache.find(codeSnippet);

for (int i = 0; i < parts.size(); i++) {
object = object.property(parts.at(i));
if (!object.isValid()) {
break;
if (i != m_scriptWrappedFunctionCache.end()) {
wrappedFunction = i.value();
} else {
QStringList wrapperArgList;
for (int i = 1; i <= numberOfArgs; i++) {
wrapperArgList << QString("arg%1").arg(i);
}
}
m_scriptValueCache[function] = object;
return object;
QString wrapperArgs = wrapperArgList.join(",");
QString wrappedCode = "(function (" + wrapperArgs + ") { (" +
codeSnippet + ")(" + wrapperArgs + "); })";
wrappedFunction = m_pEngine->evaluate(wrappedCode);
checkException();
m_scriptWrappedFunctionCache[codeSnippet] = wrappedFunction;
Copy link
Copy Markdown
Member

@rryan rryan Apr 11, 2016

Choose a reason for hiding this comment

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

resolveFunction cached failed lookups but since we return early the next time this MIDI message comes in we'll repeat the syntax check / evaluate / failure dialog, etc. I think we should probably cache failures too here. WDYT?

Copy link
Copy Markdown
Contributor Author

@Be-ing Be-ing Apr 11, 2016

Choose a reason for hiding this comment

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

I thought about that, but I realized it could be possible for a failed lookup to work the next time if it references script variables that the script has changed between MIDI messages received.

Copy link
Copy Markdown
Member

@rryan rryan Apr 11, 2016

Choose a reason for hiding this comment

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

Gotcha -- I could be wrong but I think evaluating the wrapper can only fail if there is a syntax error -- for example: https://repl.it/CEMr

Undefined variables or bad references would be exceptions at the time we call the wrapper function.

And I can't think of a syntax error you can write that will become not a syntax error based on changed in the environment.

Copy link
Copy Markdown
Contributor Author

@Be-ing Be-ing Apr 11, 2016

Choose a reason for hiding this comment

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

Evaluating the wrapper fails when the passed code snippet is a valid object but not a function. The script could reassign the object to a function between MIDI messages received.

Copy link
Copy Markdown
Member

@rryan rryan Apr 11, 2016

Choose a reason for hiding this comment

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

Huh, that doesn't match my understanding. For example: https://repl.it/CENd
Evaluating the wrapper succeeds without a problem. Could you post an example of this?

Copy link
Copy Markdown
Contributor Author

@Be-ing Be-ing Apr 11, 2016

Choose a reason for hiding this comment

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

https://repl.it/CENh/4

Map some button's input to P32.button[1] and another's to P32.button[2]. If button 1 is pressed first, evaluating the wrapper would throw an exception. If button 2 is pressed before button 1 is pressed again, pressing button 1 should work.

Copy link
Copy Markdown
Contributor Author

@Be-ing Be-ing Apr 11, 2016

Choose a reason for hiding this comment

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

This is the error I get from Mixxx:

Uncaught exception at line 1 in passed code: TypeError: Result of expression '(P32.whatever)' [stuff] is not a function.
Backtrace:
<anonymous>()@:1

P32.whatever is set to the string 'stuff'.

Copy link
Copy Markdown
Member

@rryan rryan Apr 11, 2016

Choose a reason for hiding this comment

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

Hm, are you sure that error occurred in evaluating the wrapper function or was it executing the wrapper function?

I wrote a quick test that suggests evaluating the wrapper would work fine in that case:
rryan@7036720

Output:

[ RUN      ] ControllerEngineTest.Evaluate
Loading resources from  "/Users/rjryan/Code/mixxx/res/"
ConfigObject: Could not read "/Users/rjryan/Code/mixxx/src/test/test_data/test.cfg"
SetUp
ControllerEngine: Loading "/var/folders/qr/klrx2z4s0l37vkk8y5lq5718002y8v/T/qt_temp.M56742"
TypeError: Result of expression '(P32.whatever)' [stuff] is not a function.
whatever works!
TearDown
ControllerEngine shutting down...
[       OK ] ControllerEngineTest.Evaluate (1 ms)

In this case evaluating the wrapper (the value we're caching) succeeds without exception. It's calling the wrapper when P32.whatever equals 'stuff' that produces the exception.

Copy link
Copy Markdown
Contributor Author

@Be-ing Be-ing Apr 11, 2016

Choose a reason for hiding this comment

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

Ah, you are correct. I misunderstood where the exception was occuring. It is indeed when calling the function in internalExecute(). So yes, it does make sense to always cache the function.

}
return wrappedFunction;
}

/* -------- ------------------------------------------------------
Expand Down Expand Up @@ -158,8 +166,8 @@ void ControllerEngine::gracefulShutdown() {
}
}

// Clear the Script Value cache
m_scriptValueCache.clear();
// Clear the cache of function wrappers
m_scriptWrappedFunctionCache.clear();

// Free all the control object threads
QList<ConfigKey> keys = m_controlCache.keys();
Expand Down Expand Up @@ -297,19 +305,11 @@ bool ControllerEngine::evaluate(const QString& filepath) {
return ret;
}

/* -------- ------------------------------------------------------
Purpose: Evaluate & run script code
Input: 'this' object if applicable, Code string
Output: false if an exception
-------- ------------------------------------------------------ */
bool ControllerEngine::internalExecute(QScriptValue thisObject,
const QString& scriptCode) {
// A special version of safeExecute since we're evaluating strings, not actual functions
// (execute() would print an error that it's not a function every time a timer fires.)
if (m_pEngine == nullptr)
bool ControllerEngine::syntaxIsValid(const QString& scriptCode) {
if (m_pEngine == nullptr) {
return false;
}

// Check syntax
QScriptSyntaxCheckResult result = m_pEngine->checkSyntax(scriptCode);
QString error = "";
switch (result.state()) {
Expand All @@ -332,6 +332,25 @@ bool ControllerEngine::internalExecute(QScriptValue thisObject,
scriptErrorDialog(error);
return false;
}
return true;
}

/* -------- ------------------------------------------------------
Purpose: Evaluate & run script code
Input: 'this' object if applicable, Code string
Output: false if an exception
-------- ------------------------------------------------------ */
bool ControllerEngine::internalExecute(QScriptValue thisObject,
const QString& scriptCode) {
// A special version of safeExecute since we're evaluating strings, not actual functions
// (execute() would print an error that it's not a function every time a timer fires.)
if (m_pEngine == nullptr) {
return false;
}

if (!syntaxIsValid(scriptCode)) {
Copy link
Copy Markdown
Member

@daschuer daschuer Apr 11, 2016

Choose a reason for hiding this comment

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

This call along with the wrapper generation should be done earlier, just after evaluating the QScriptValue as string places like:
ControllerEngine::beginTimer

return false;
}

QScriptValue scriptFunction = m_pEngine->evaluate(scriptCode);

Expand Down Expand Up @@ -360,6 +379,12 @@ bool ControllerEngine::internalExecute(QScriptValue thisObject, QScriptValue fun
return false;
}

if (functionObject.isError()) {
qDebug() << "ControllerEngine::internalExecute:"
<< functionObject.toString();
Copy link
Copy Markdown
Contributor Author

@Be-ing Be-ing Apr 11, 2016

Choose a reason for hiding this comment

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

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.

return false;
}

// If it's not a function, we're done.
if (!functionObject.isFunction()) {
qDebug() << "ControllerEngine::internalExecute:"
Expand Down Expand Up @@ -441,6 +466,8 @@ bool ControllerEngine::checkException() {
scriptErrorDialog(ControllerDebug::enabled() ?
QString("%1\nBacktrace:\n%2")
.arg(errorText, backtrace.join("\n")) : errorText);

m_pEngine->clearExceptions();
return true;
}
return false;
Expand Down
7 changes: 4 additions & 3 deletions src/controllers/controllerengine.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,8 @@ class ControllerEngine : public QObject {
m_bPopups = bPopups;
}

// Resolve a function name to a QScriptValue.
QScriptValue resolveFunction(const QString& function) const;
// Wrap a snippet of JS code in an anonymous function
QScriptValue wrapFunctionCode(const QString& codeSnippet, int numberOfArgs);

// Look up registered script function prefixes
const QList<QString>& getScriptFunctionPrefixes() { return m_scriptFunctionPrefixes; };
Expand Down Expand Up @@ -148,6 +148,7 @@ class ControllerEngine : public QObject {
void errorDialogButton(const QString& key, QMessageBox::StandardButton button);

private:
bool syntaxIsValid(const QString& scriptCode);
bool evaluate(const QString& scriptName, QList<QString> scriptPaths);
bool internalExecute(QScriptValue thisObject, const QString& scriptCode);
bool internalExecute(QScriptValue thisObject, QScriptValue functionObject,
Expand Down Expand Up @@ -193,7 +194,7 @@ class ControllerEngine : public QObject {
QVarLengthArray<bool> m_ramp, m_brakeActive;
QVarLengthArray<AlphaBetaFilter*> m_scratchFilters;
QHash<int, int> m_scratchTimers;
mutable QHash<QString, QScriptValue> m_scriptValueCache;
QHash<QString, QScriptValue> m_scriptWrappedFunctionCache;
// Filesystem watcher for script auto-reload
QFileSystemWatcher m_scriptWatcher;
QList<QString> m_lastScriptPaths;
Expand Down
4 changes: 2 additions & 2 deletions src/controllers/midi/midicontroller.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,7 +288,7 @@ void MidiController::processInputMapping(const MidiInputMapping& mapping,
return;
}

QScriptValue function = pEngine->resolveFunction(mapping.control.item);
QScriptValue function = pEngine->wrapFunctionCode(mapping.control.item, 5);
Copy link
Copy Markdown
Member

@daschuer daschuer Apr 11, 2016

Choose a reason for hiding this comment

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

This call should be moved to
void MidiController::learnTemporaryInputMappings and store result to a new mapping element.

Copy link
Copy Markdown
Member

@daschuer daschuer Apr 11, 2016

Choose a reason for hiding this comment

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

And it looks like we have to fill this new element somewhere after MidiController::visit().

Copy link
Copy Markdown
Member

@rryan rryan Apr 11, 2016

Choose a reason for hiding this comment

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

IMO this would violate abstraction barriers and instead needs some somewhat careful refactoring to gain the performance wins you're talking about. (Script values escaping the ControllerEngine as they did with resolveFunction was already an abstraction violation and storing the values in the MidiController/preset is taking it a step further :) ).

For example -- we need to invalidate the wrapper script values when the filesystem watcher notices the script has changed. If the QScriptValue was stored in the MidiInputMapping (or an m_incomingDataFunctions list) there is no current easy way for the script engine to reach out into the MidiMapping/MidiController to do this. I'm not even sure if it's safe to use a QScriptValue after its QScriptEngine has been deleted (gracefulShutdown destroys the current engine).

Copy link
Copy Markdown
Member

@daschuer daschuer Apr 11, 2016

Choose a reason for hiding this comment

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

OK, the script value holds a pointer of the script engine and vice versa. So it can be dangerous to spread QStringValues around.

How about turn the current cache solution into a shared pointer cache. The mapping can hold a weak pointer.
Turing a weak pointer into a shared pointer and execute the QScriptValue function should be much faster then looking up a hash. If the strong cache is gone, all stored weak pointers are invalid and the mapping has to evaluate its functions again.

QHash<QString, QSharedPointer<QScriptValue> > m_scriptWrappedFunctionCache
...
QSharedPointer<QScriptValue> = pFunction(new QScriptValue(m_pEngine->evaluate(codeSnippet)));
m_scriptWrappedFunctionCache[codeSnippet] = pFunction; 
...

...
QWeakPointer<QScriptValue> scriptfunction;
...
mapping.scriptfunction = pFunction; 
...

QSharedPointer<QScriptValue> strong = mapping.scriptfunction.toStrongRef();
if (strong)
    // execute
else
   // evaluate again

Copy link
Copy Markdown
Member

@rryan rryan Apr 11, 2016

Choose a reason for hiding this comment

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

That works -- though it creates more complexity / tight coupling between the controller and the script engine. We could create an opaque reference/token or something.

Unless @Be-ing wants to go for "extra credit" here from my POV this branch is a strict improvement over master though.

Copy link
Copy Markdown
Contributor Author

@Be-ing Be-ing Apr 11, 2016

Choose a reason for hiding this comment

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

I'd prefer to merge this sooner and move on to other projects at this point.

Copy link
Copy Markdown
Member

@daschuer daschuer Apr 11, 2016

Choose a reason for hiding this comment

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

Yes, right. This brach includes already enough improvements to be mergable. Well done. :-)

if (!pEngine->execute(function, channel, control, value, status,
mapping.control.group, timestamp)) {
qDebug() << "MidiController: Invalid script function"
Expand Down Expand Up @@ -547,7 +547,7 @@ void MidiController::processInputMapping(const MidiInputMapping& mapping,
if (pEngine == NULL) {
return;
}
QScriptValue function = pEngine->resolveFunction(mapping.control.item);
QScriptValue function = pEngine->wrapFunctionCode(mapping.control.item, 2);
if (!pEngine->execute(function, data, timestamp)) {
qDebug() << "MidiController: Invalid script function"
<< mapping.control.item;
Expand Down
2 changes: 1 addition & 1 deletion src/test/controllerengine_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ class ControllerEngineTest : public MixxxTest {
}

bool execute(const QString& functionName) {
QScriptValue function = cEngine->resolveFunction(functionName);
QScriptValue function = cEngine->wrapFunctionCode(functionName, 0);
return cEngine->internalExecute(QScriptValue(), function,
QScriptValueList());
}
Expand Down