Skip to content

Commit

Permalink
ControllerScriptEngine: generate wrappers for input callbacks
Browse files Browse the repository at this point in the history
This avoids calling two separate JS functions (one to convert the
ArrayBuffer to a Uint8Array then the callback) every time controller
input is received.
  • Loading branch information
Be-ing committed Jul 13, 2020
1 parent 9363b49 commit e25bf12
Show file tree
Hide file tree
Showing 4 changed files with 24 additions and 20 deletions.
30 changes: 16 additions & 14 deletions src/controllers/scripting/controllerscriptenginebase.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,20 @@ bool ControllerScriptEngineBase::initialize() {
"midi", m_pJSEngine->newQObject(controllerProxy));
}

m_byteArrayToScriptValueJSFunction = m_pJSEngine->evaluate(
"(function(arg1) { return new Uint8Array(arg1) })");
// Binary data is passed from the Controller as a QByteArray, which
// QJSEngine::toScriptValue converts to an ArrayBuffer in JavaScript.
// ArrayBuffer cannot be accessed with the [] operator in JS; it needs
// to be converted to a typed array (Uint8Array in this case) first.
// This function generates a wrapper function from a JS callback to do
// that conversion automatically.
m_makeArrayBufferWrapperFunction = m_pJSEngine->evaluate(QStringLiteral(
// arg2 is the timestamp for ControllerScriptModuleEngine.
// In ControllerScriptEngineLegacy it is the length of the array.
"(function(callback) {"
" return function(arrayBuffer, arg2) {"
" callback(new Uint8Array(arrayBuffer), arg2);"
" };"
"})"));

This comment has been minimized.

Copy link
@Holzhaus

Holzhaus Oct 25, 2020

Member

Can this be moved to controllerscriptenginelegacy? For the new controller system, I'd rather get rid of hacks like this and just provide that function as part of an actual JS module that can be imported.

This comment has been minimized.

Copy link
@Be-ing

Be-ing Oct 25, 2020

Author Contributor

This will be used by MidiDispatcher in the new system so I don't think it's a great idea to move it to ControllerScriptEngineLegacy.

This comment has been minimized.

Copy link
@Holzhaus

Holzhaus Oct 25, 2020

Member

But why does this has to be implemented in C++? We can't change the API for legacy script, but the new MidiDispatcher should just use an actual JS library, not a JS function wrapped in C++ code...

This comment has been minimized.

Copy link
@Be-ing

Be-ing Oct 25, 2020

Author Contributor

The code should not be duplicated.

This comment has been minimized.

Copy link
@Be-ing

Be-ing Oct 25, 2020

Author Contributor

I cannot anticipate the organization of future code that is blocked on this being merged, so going in circles preventing this code from being merged is IMO absurd.

This comment has been minimized.

Copy link
@Holzhaus

Holzhaus Oct 25, 2020

Member

Right, I'm arguing that we shouldn't put stuff in the base class if it's unclear that it's needed outside a specific child class.


return true;
}
Expand Down Expand Up @@ -235,16 +247,6 @@ void ControllerScriptEngineBase::throwJSError(const QString& message) {
#endif
}

QJSValue ControllerScriptEngineBase::byteArrayToScriptValue(
const QByteArray& byteArray) {
// The QJSEngine converts the QByteArray to an ArrayBuffer object.
QJSValue arrayBuffer = m_pJSEngine->toScriptValue(byteArray);
// Convert the ArrayBuffer to a Uint8 typed array so scripts can access its bytes
// with the [] operator.
QJSValue result =
m_byteArrayToScriptValueJSFunction.call(QJSValueList{arrayBuffer});
if (result.isError()) {
showScriptExceptionDialog(result);
}
return result;
QJSValue ControllerScriptEngineBase::wrapArrayBufferCallback(const QJSValue& callback) {
return m_makeArrayBufferWrapperFunction.call(QJSValueList{callback});
}
4 changes: 2 additions & 2 deletions src/controllers/scripting/controllerscriptenginebase.h
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ class ControllerScriptEngineBase : public QObject {

void scriptErrorDialog(const QString& detailedError, const QString& key, bool bFatal = false);

QJSValue byteArrayToScriptValue(const QByteArray& byteArray);
QJSValue wrapArrayBufferCallback(const QJSValue& callback);

bool m_bDisplayingExceptionDialog;
QJSEngine* m_pJSEngine;
Expand All @@ -58,7 +58,7 @@ class ControllerScriptEngineBase : public QObject {
void reload();

private:
QJSValue m_byteArrayToScriptValueJSFunction;
QJSValue m_makeArrayBufferWrapperFunction;

private slots:
void errorDialogButton(const QString& key, QMessageBox::StandardButton button);
Expand Down
4 changes: 2 additions & 2 deletions src/controllers/scripting/controllerscriptmoduleengine.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ bool ControllerScriptModuleEngine::initialize() {

QJSValue handleInputFunction = mod.property("handleInput");
if (handleInputFunction.isCallable()) {
m_handleInputFunction = handleInputFunction;
m_handleInputFunction = wrapArrayBufferCallback(handleInputFunction);
} else {
scriptErrorDialog(
"Controller JavaScript module exports no handleInput function.",
Expand Down Expand Up @@ -52,7 +52,7 @@ void ControllerScriptModuleEngine::shutdown() {
void ControllerScriptModuleEngine::handleInput(
QByteArray data, mixxx::Duration timestamp) {
QJSValueList args;
args << byteArrayToScriptValue(data);
args << m_pJSEngine->toScriptValue(data);
args << timestamp.toDoubleMillis();
executeFunction(m_handleInputFunction, args);
}
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,9 @@ bool ControllerScriptEngineLegacy::initialize() {
continue;
}
functionName.append(QStringLiteral(".incomingData"));
m_incomingDataFunctions.append(wrapFunctionCode(functionName, 2));
m_incomingDataFunctions.append(
wrapArrayBufferCallback(
wrapFunctionCode(functionName, 2)));
}

QJSValueList args;
Expand Down Expand Up @@ -139,7 +141,7 @@ bool ControllerScriptEngineLegacy::handleIncomingData(const QByteArray& data) {
}

QJSValueList args;
args << byteArrayToScriptValue(data);
args << m_pJSEngine->toScriptValue(data);
args << QJSValue(data.size());

for (const QJSValue& function : m_incomingDataFunctions) {
Expand Down

0 comments on commit e25bf12

Please sign in to comment.