From 966e572eeada07604c46d3a261ec2df5793dc641 Mon Sep 17 00:00:00 2001 From: Antoine C Date: Sat, 4 Nov 2023 20:15:11 +0000 Subject: [PATCH 1/6] feat: inter controller communication --- CMakeLists.txt | 2 + src/controllers/controller.h | 1 - src/controllers/controllermanager.cpp | 18 ++- src/controllers/controllermanager.h | 2 + src/controllers/controllerruntimedata.cpp | 3 + src/controllers/controllerruntimedata.h | 34 +++++ .../scripting/controllerscriptenginebase.h | 11 ++ .../legacy/controllerscriptenginelegacy.h | 1 + .../controllerscriptinterfacelegacy.cpp | 126 +++++++++++++++- .../legacy/controllerscriptinterfacelegacy.h | 17 +++ .../legacy/scriptconnectionjsproxy.cpp | 14 ++ .../legacy/scriptconnectionjsproxy.h | 21 ++- src/test/controller_runtimedata_test.cpp | 140 ++++++++++++++++++ 13 files changed, 384 insertions(+), 6 deletions(-) create mode 100644 src/controllers/controllerruntimedata.cpp create mode 100644 src/controllers/controllerruntimedata.h create mode 100644 src/test/controller_runtimedata_test.cpp diff --git a/CMakeLists.txt b/CMakeLists.txt index 70d4954448f..22d25fe0c68 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -739,6 +739,7 @@ add_library(mixxx-lib STATIC EXCLUDE_FROM_ALL src/controllers/controlpickermenu.cpp src/controllers/legacycontrollermappingfilehandler.cpp src/controllers/legacycontrollermapping.cpp + src/controllers/controllerruntimedata.cpp src/controllers/delegates/controldelegate.cpp src/controllers/delegates/midibytedelegate.cpp src/controllers/delegates/midichanneldelegate.cpp @@ -2076,6 +2077,7 @@ add_executable(mixxx-test src/test/controller_mapping_validation_test.cpp src/test/controller_mapping_settings_test.cpp src/test/controllerscriptenginelegacy_test.cpp + src/test/controller_runtimedata_test.cpp src/test/controlobjecttest.cpp src/test/controlobjectaliastest.cpp src/test/controlobjectscripttest.cpp diff --git a/src/controllers/controller.h b/src/controllers/controller.h index cd6e0fa5990..f17e56a0eae 100644 --- a/src/controllers/controller.h +++ b/src/controllers/controller.h @@ -136,7 +136,6 @@ class Controller : public QObject { const RuntimeLoggingCategory m_logOutput; private: // but used by ControllerManager - virtual int open() = 0; virtual int close() = 0; // Requests that the device poll if it is a polling device. Returns true diff --git a/src/controllers/controllermanager.cpp b/src/controllers/controllermanager.cpp index 7dbac102ef3..db382c9e1b5 100644 --- a/src/controllers/controllermanager.cpp +++ b/src/controllers/controllermanager.cpp @@ -6,7 +6,9 @@ #include "controllers/controller.h" #include "controllers/controllerlearningeventfilter.h" #include "controllers/controllermappinginfoenumerator.h" +#include "controllers/controllerruntimedata.h" #include "controllers/defs_controllers.h" +#include "controllers/scripting/legacy/controllerscriptenginelegacy.h" #include "moc_controllermanager.cpp" #include "util/cmdlineargs.h" #include "util/compatibility/qmutex.h" @@ -93,7 +95,8 @@ ControllerManager::ControllerManager(UserSettingsPointer pConfig) // its own event loop. m_pControllerLearningEventFilter(new ControllerLearningEventFilter()), m_pollTimer(this), - m_skipPoll(false) { + m_skipPoll(false), + m_pRuntimeData(std::make_shared(this)) { qRegisterMetaType>( "std::shared_ptr"); @@ -300,6 +303,12 @@ void ControllerManager::slotSetUpDevices() { qWarning() << "There was a problem opening" << name; continue; } + VERIFY_OR_DEBUG_ASSERT(pController->getScriptEngine()) { + qWarning() << "Unable to acquire the controller engine. Has the " + "controller open successfully?"; + continue; + } + pController->getScriptEngine()->setRuntimeData(m_pRuntimeData); pController->applyMapping(); } @@ -392,6 +401,13 @@ void ControllerManager::openController(Controller* pController) { // If successfully opened the device, apply the mapping and save the // preference setting. if (result == 0) { + VERIFY_OR_DEBUG_ASSERT(pController->getScriptEngine()) { + qWarning() << "Unable to acquire the controller engine. Has the " + "controller open successfully?"; + return; + } + pController->getScriptEngine()->setRuntimeData(m_pRuntimeData); + pController->applyMapping(); // Update configuration to reflect controller is enabled. diff --git a/src/controllers/controllermanager.h b/src/controllers/controllermanager.h index cfa040d496a..70b577dfbbf 100644 --- a/src/controllers/controllermanager.h +++ b/src/controllers/controllermanager.h @@ -14,6 +14,7 @@ class Controller; class ControllerLearningEventFilter; class MappingInfoEnumerator; class LegacyControllerMapping; +class ControllerRuntimeData; class ControllerEnumerator; /// Function to sort controllers by name @@ -86,4 +87,5 @@ class ControllerManager : public QObject { QSharedPointer m_pMainThreadUserMappingEnumerator; QSharedPointer m_pMainThreadSystemMappingEnumerator; bool m_skipPoll; + std::shared_ptr m_pRuntimeData; }; diff --git a/src/controllers/controllerruntimedata.cpp b/src/controllers/controllerruntimedata.cpp new file mode 100644 index 00000000000..83bc96ad66a --- /dev/null +++ b/src/controllers/controllerruntimedata.cpp @@ -0,0 +1,3 @@ +#include + +#include "moc_controllerruntimedata.cpp" diff --git a/src/controllers/controllerruntimedata.h b/src/controllers/controllerruntimedata.h new file mode 100644 index 00000000000..db3f72e4819 --- /dev/null +++ b/src/controllers/controllerruntimedata.h @@ -0,0 +1,34 @@ +#pragma once + +#include + +#include "util/assert.h" + +/// ControllerRuntimeData is a wrapper that allows controllers script runtimes +/// to share arbitrary data via a the JavaScript interface. It doesn't enforce +/// any type consistency and it is the script responsibility to use this data in +/// a considerate, non-destructive way (append to lists, extend to objects, +/// ...), as well as expecting that others won't do so. +class ControllerRuntimeData : public QObject { + Q_OBJECT + public: + ControllerRuntimeData(QObject* parent) + : QObject(parent), m_value() { + } + + const QVariant& get() const { + return m_value; + } + + public slots: + void set(const QVariant& value) { + m_value = value; + emit updated(m_value); + } + + signals: + void updated(const QVariant& value); + + private: + QVariant m_value; +}; diff --git a/src/controllers/scripting/controllerscriptenginebase.h b/src/controllers/scripting/controllerscriptenginebase.h index 85deba677f1..cc426267c4d 100644 --- a/src/controllers/scripting/controllerscriptenginebase.h +++ b/src/controllers/scripting/controllerscriptenginebase.h @@ -8,6 +8,7 @@ class Controller; class QJSEngine; +class ControllerRuntimeData; /// ControllerScriptEngineBase manages the JavaScript engine for controller scripts. /// ControllerScriptModuleEngine implements the current system using JS modules. @@ -40,6 +41,14 @@ class ControllerScriptEngineBase : public QObject { return m_bTesting; } + void setRuntimeData(std::shared_ptr runtimeData) { + m_pRuntimeData = std::move(runtimeData); + } + + std::shared_ptr getRuntimeData() const { + return m_pRuntimeData; + } + protected: virtual void shutdown(); @@ -48,6 +57,7 @@ class ControllerScriptEngineBase : public QObject { bool m_bDisplayingExceptionDialog; std::shared_ptr m_pJSEngine; + std::shared_ptr m_pRuntimeData; Controller* m_pController; const RuntimeLoggingCategory m_logger; @@ -63,4 +73,5 @@ class ControllerScriptEngineBase : public QObject { void errorDialogButton(const QString& key, QMessageBox::StandardButton button); friend class ColorMapperJSProxy; + friend class ControllerRuntimeDataTest; }; diff --git a/src/controllers/scripting/legacy/controllerscriptenginelegacy.h b/src/controllers/scripting/legacy/controllerscriptenginelegacy.h index 9bf379dc0cf..689539500e1 100644 --- a/src/controllers/scripting/legacy/controllerscriptenginelegacy.h +++ b/src/controllers/scripting/legacy/controllerscriptenginelegacy.h @@ -70,4 +70,5 @@ class ControllerScriptEngineLegacy : public ControllerScriptEngineBase { } friend class ControllerScriptEngineLegacyTest; + friend class ControllerRuntimeDataTest; }; diff --git a/src/controllers/scripting/legacy/controllerscriptinterfacelegacy.cpp b/src/controllers/scripting/legacy/controllerscriptinterfacelegacy.cpp index 3ee8d09a280..c8989286c6b 100644 --- a/src/controllers/scripting/legacy/controllerscriptinterfacelegacy.cpp +++ b/src/controllers/scripting/legacy/controllerscriptinterfacelegacy.cpp @@ -2,6 +2,7 @@ #include "control/controlobject.h" #include "control/controlobjectscript.h" +#include "controllers/controllerruntimedata.h" #include "controllers/scripting/legacy/controllerscriptenginelegacy.h" #include "controllers/scripting/legacy/scriptconnectionjsproxy.h" #include "mixer/playermanager.h" @@ -47,6 +48,11 @@ ControllerScriptInterfaceLegacy::ControllerScriptInterfaceLegacy( m_spinbackActive[i] = false; m_softStartActive[i] = false; } + + connect(m_pEngine->getRuntimeData().get(), + &ControllerRuntimeData::updated, + this, + &ControllerScriptInterfaceLegacy::onRuntimeDataUpdated); } ControllerScriptInterfaceLegacy::~ControllerScriptInterfaceLegacy() { @@ -164,6 +170,88 @@ void ControllerScriptInterfaceLegacy::setValue( } } +QJSValue ControllerScriptInterfaceLegacy::getRuntimeData() { + auto pJsEngine = m_pScriptEngineLegacy->jsEngine(); + VERIFY_OR_DEBUG_ASSERT(pJsEngine) { + return QJSValue(); + } + auto pRuntimeData = m_pScriptEngineLegacy->getRuntimeData(); + + if (!pRuntimeData) { + qWarning() << "Unable to fetch the runtime data"; + return QJSValue(); + } + + return pJsEngine->toScriptValue(pRuntimeData->get()); +} + +void ControllerScriptInterfaceLegacy::setRuntimeData(const QJSValue& value) { + auto pJsEngine = m_pScriptEngineLegacy->jsEngine(); + VERIFY_OR_DEBUG_ASSERT(pJsEngine) { + return; + } + auto pRuntimeData = m_pScriptEngineLegacy->getRuntimeData(); + + if (!pRuntimeData) { + qWarning() << "Unable to fetch the runtime data"; + return; + } + + pRuntimeData->set(value.toVariant()); + qDebug() << "runtime data set successfully"; +} + +QJSValue ControllerScriptInterfaceLegacy::onRuntimeDataUpdate(const QJSValue& callback) { + if (!callback.isCallable()) { + m_pScriptEngineLegacy->throwJSError( + "Tried to connect runtime data update handler" + " to an invalid callback. Make sure that your code contains no " + "syntax errors."); + return QJSValue(); + } + auto pJsEngine = m_pScriptEngineLegacy->jsEngine(); + VERIFY_OR_DEBUG_ASSERT(pJsEngine) { + return QJSValue(); + } + auto pRuntimeData = m_pScriptEngineLegacy->getRuntimeData(); + + if (!pRuntimeData) { + qWarning() << "Unable to fetch the runtime data"; + return QJSValue(); + } + + ScriptConnection connection; + connection.engineJSProxy = this; + connection.controllerEngine = m_pScriptEngineLegacy; + connection.callback = callback; + connection.id = QUuid::createUuid(); + + m_runtimeDataConnections.append(connection); + + return pJsEngine->newQObject( + new ScriptRuntimeConnectionJSProxy(m_runtimeDataConnections.last())); +} + +void ControllerScriptInterfaceLegacy::onRuntimeDataUpdated(const QVariant& value) { + auto pJsEngine = m_pScriptEngineLegacy->jsEngine(); + const auto args = QJSValueList{ + pJsEngine->toScriptValue(value), + }; + + for (const auto& connection : m_runtimeDataConnections) { + QJSValue func = connection.callback; // copy function because QJSValue::call is not const + QJSValue result = func.call(args); + if (result.isError()) { + if (m_pScriptEngineLegacy != nullptr) { + m_pScriptEngineLegacy->showScriptExceptionDialog(result); + } + qWarning() << "ControllerEngine: Invocation of connection " << connection.id.toString() + << "connected to runtime data failed:" + << result.toString(); + } + } +} + double ControllerScriptInterfaceLegacy::getParameter(const QString& group, const QString& name) { ControlObjectScript* coScript = getControlObjectScript(group, name); if (coScript == nullptr) { @@ -318,10 +406,11 @@ bool ControllerScriptInterfaceLegacy::removeScriptConnection( void ControllerScriptInterfaceLegacy::triggerScriptConnection( const ScriptConnection& connection) { - VERIFY_OR_DEBUG_ASSERT(m_pScriptEngineLegacy->jsEngine()) { + VERIFY_OR_DEBUG_ASSERT(m_pScriptEngineLegacy->jsEngine() && connection.key.isValid()) { return; } + // TODO handle runtimeData connection ControlObjectScript* coScript = getControlObjectScript(connection.key.group, connection.key.item); if (coScript == nullptr) { @@ -335,6 +424,41 @@ void ControllerScriptInterfaceLegacy::triggerScriptConnection( connection.executeCallback(coScript->get()); } +bool ControllerScriptInterfaceLegacy::removeRuntimeDataConnection( + const ScriptConnection& connection) { + if (m_pScriptEngineLegacy->jsEngine() == nullptr || + !m_runtimeDataConnections.contains(connection)) { + return false; + } + + m_runtimeDataConnections.removeAll(connection); + + return true; +} + +void ControllerScriptInterfaceLegacy::triggerRuntimeDataConnection( + const ScriptConnection& connection) { + VERIFY_OR_DEBUG_ASSERT(m_pScriptEngineLegacy->jsEngine() || + !m_runtimeDataConnections.contains(connection)) { + return; + } + auto pJsEngine = m_pScriptEngineLegacy->jsEngine(); + + QJSValue func = connection.callback; // copy function because QJSValue::call is not const + auto args = QJSValueList{ + pJsEngine->toScriptValue(m_pScriptEngineLegacy->getRuntimeData()->get()), + }; + QJSValue result = func.call(args); + if (result.isError()) { + if (m_pScriptEngineLegacy != nullptr) { + m_pScriptEngineLegacy->showScriptExceptionDialog(result); + } + qWarning() << "ControllerEngine: Invocation of runtime data connection " + << connection.id.toString() + << "connected to runtime data failed:" << result.toString(); + } +} + // This function is a legacy version of makeConnection with several alternate // ways of invoking it. The callback function can be passed either as a string of // JavaScript code that evaluates to a function or an actual JavaScript function. diff --git a/src/controllers/scripting/legacy/controllerscriptinterfacelegacy.h b/src/controllers/scripting/legacy/controllerscriptinterfacelegacy.h index 4d3e280a6ff..5bbf5a61c19 100644 --- a/src/controllers/scripting/legacy/controllerscriptinterfacelegacy.h +++ b/src/controllers/scripting/legacy/controllerscriptinterfacelegacy.h @@ -25,6 +25,11 @@ class ControllerScriptInterfaceLegacy : public QObject { Q_INVOKABLE QJSValue getSetting(const QString& name); Q_INVOKABLE double getValue(const QString& group, const QString& name); Q_INVOKABLE void setValue(const QString& group, const QString& name, double newValue); + + Q_INVOKABLE QJSValue getRuntimeData(); + Q_INVOKABLE void setRuntimeData(const QJSValue& value); + Q_INVOKABLE QJSValue onRuntimeDataUpdate(const QJSValue& callback); + Q_INVOKABLE double getParameter(const QString& group, const QString& name); Q_INVOKABLE void setParameter(const QString& group, const QString& name, double newValue); Q_INVOKABLE double getParameterForValue( @@ -74,9 +79,17 @@ class ControllerScriptInterfaceLegacy : public QObject { /// Execute a ScriptConnection's JS callback void triggerScriptConnection(const ScriptConnection& conn); + /// Disconnect and remove a ScriptConnection's RuntimeData JS callback + bool removeRuntimeDataConnection(const ScriptConnection& conn); + /// Execute a ScriptConnection's RuntimeData JS callback + void triggerRuntimeDataConnection(const ScriptConnection& conn); + /// Handler for timers that scripts set. virtual void timerEvent(QTimerEvent* event); + private slots: + void onRuntimeDataUpdated(const QVariant& value); + private: QJSValue makeConnectionInternal(const QString& group, const QString& name, @@ -109,4 +122,8 @@ class ControllerScriptInterfaceLegacy : public QObject { ControllerScriptEngineLegacy* m_pScriptEngineLegacy; const RuntimeLoggingCategory m_logger; + + QList m_runtimeDataConnections; + + friend class ControllerRuntimeDataTest; }; diff --git a/src/controllers/scripting/legacy/scriptconnectionjsproxy.cpp b/src/controllers/scripting/legacy/scriptconnectionjsproxy.cpp index d17e131c459..169e5cfa41d 100644 --- a/src/controllers/scripting/legacy/scriptconnectionjsproxy.cpp +++ b/src/controllers/scripting/legacy/scriptconnectionjsproxy.cpp @@ -13,3 +13,17 @@ bool ScriptConnectionJSProxy::disconnect() { void ScriptConnectionJSProxy::trigger() { m_scriptConnection.engineJSProxy->triggerScriptConnection(m_scriptConnection); } + +bool ScriptRuntimeConnectionJSProxy::disconnect() { + // if the removeRuntimeDataConnection succeeded, the connection has been + // successfully disconnected + bool success = + m_scriptConnection.engineJSProxy->removeRuntimeDataConnection( + m_scriptConnection); + m_isConnected = !success; + return success; +} + +void ScriptRuntimeConnectionJSProxy::trigger() { + m_scriptConnection.engineJSProxy->triggerRuntimeDataConnection(m_scriptConnection); +} diff --git a/src/controllers/scripting/legacy/scriptconnectionjsproxy.h b/src/controllers/scripting/legacy/scriptconnectionjsproxy.h index 2b80b5e4c08..58a601e552b 100644 --- a/src/controllers/scripting/legacy/scriptconnectionjsproxy.h +++ b/src/controllers/scripting/legacy/scriptconnectionjsproxy.h @@ -21,11 +21,26 @@ class ScriptConnectionJSProxy : public QObject { bool readIsConnected() const { return m_isConnected; } - Q_INVOKABLE bool disconnect(); - Q_INVOKABLE void trigger(); + Q_INVOKABLE virtual bool disconnect(); + Q_INVOKABLE virtual void trigger(); private: - ScriptConnection m_scriptConnection; QString m_idString; + + protected: + ScriptConnection m_scriptConnection; bool m_isConnected; }; + +/// ScriptRuntimeConnectionJSProxy provides scripts with an interface to +/// controller runtime update callback. +class ScriptRuntimeConnectionJSProxy : public ScriptConnectionJSProxy { + Q_OBJECT + public: + ScriptRuntimeConnectionJSProxy(const ScriptConnection& conn) + : ScriptConnectionJSProxy(conn) { + } + + Q_INVOKABLE bool disconnect() override; + Q_INVOKABLE void trigger() override; +}; diff --git a/src/test/controller_runtimedata_test.cpp b/src/test/controller_runtimedata_test.cpp new file mode 100644 index 00000000000..b6ebf557fa3 --- /dev/null +++ b/src/test/controller_runtimedata_test.cpp @@ -0,0 +1,140 @@ +#include +#include + +#include "control/controlobject.h" +#include "control/controlpotmeter.h" +#include "controllers/controllerruntimedata.h" +#include "controllers/scripting/legacy/controllerscriptenginelegacy.h" +#include "controllers/scripting/legacy/controllerscriptinterfacelegacy.h" +#include "controllers/scripting/legacy/scriptconnection.h" +#include "controllers/softtakeover.h" +#include "preferences/usersettings.h" +#include "test/mixxxtest.h" +#include "util/color/colorpalette.h" +#include "util/time.h" + +const RuntimeLoggingCategory logger(QString("test").toLocal8Bit()); + +class ControllerRuntimeDataTest : public MixxxTest { + protected: + void SetUp() override { + mixxx::Time::setTestMode(true); + mixxx::Time::setTestElapsedTime(mixxx::Duration::fromMillis(10)); + pRuntimeData = std::make_shared(nullptr); + cEngineA = new ControllerScriptEngineLegacy(nullptr, logger); + cEngineA->setRuntimeData(pRuntimeData); + cEngineA->initialize(); + cEngineB = new ControllerScriptEngineLegacy(nullptr, logger); + cEngineA->setRuntimeData(pRuntimeData); + cEngineB->initialize(); + } + + void TearDown() override { + delete cEngineA; + delete cEngineB; + mixxx::Time::setTestMode(false); + } + + QJSValue evaluateA(const QString& code) { + return cEngineA->jsEngine()->evaluate(code); + } + + QJSValue evaluateB(const QString& code) { + return cEngineA->jsEngine()->evaluate(code); + } + + std::shared_ptr jsEngineA() { + return cEngineA->jsEngine(); + } + + std::shared_ptr jsEngineB() { + return cEngineB->jsEngine(); + } + + const QList& runtimeDataConnectionsEngineA() { + return static_cast( + jsEngineA()->globalObject().property("engine").toQObject()) + ->m_runtimeDataConnections; + } + + const QList& runtimeDataConnectionsEngineB() { + return static_cast( + jsEngineB()->globalObject().property("engine").toQObject()) + ->m_runtimeDataConnections; + } + + ControllerScriptEngineLegacy* cEngineA; + ControllerScriptEngineLegacy* cEngineB; + + std::shared_ptr pRuntimeData; +}; + +TEST_F(ControllerRuntimeDataTest, getSetRuntimeData) { + pRuntimeData->set(QVariant("foobar")); + EXPECT_TRUE(!evaluateA(R"--( +let data = engine.getRuntimeData(); +if (data !== "foobar") throw "Something is wrong"; +engine.setRuntimeData("barfoo"); +)--") + .isError()); + auto data = pRuntimeData->get(); + EXPECT_TRUE(data.canConvert()); + EXPECT_EQ(data.toString(), "barfoo"); + + EXPECT_TRUE(!evaluateB(R"--( +let data = engine.getRuntimeData(); +if (data !== "barfoo") throw "Something is wrong"; +engine.setRuntimeData("bazfuu"); +)--") + .isError()); + data = pRuntimeData->get(); + EXPECT_TRUE(data.canConvert()); + EXPECT_EQ(data.toString(), "bazfuu"); +} + +TEST_F(ControllerRuntimeDataTest, runtimeDataCallback) { + EXPECT_TRUE(!evaluateA(R"--( +engine.onRuntimeDataUpdate((data) => { + if (data !== "foobar") throw "Something is wrong"; + engine.setRuntimeData("bazfuu") +}); +)--") + .isError()); + pRuntimeData->set(QVariant("foobar")); + application()->processEvents(); + + auto data = pRuntimeData->get(); + EXPECT_TRUE(data.canConvert()); + EXPECT_EQ(data.toString(), "bazfuu"); +} + +TEST_F(ControllerRuntimeDataTest, canTrigger) { + EXPECT_TRUE(!evaluateA(R"--( +engine.onRuntimeDataUpdate((data) => { + if (data) return; + engine.setRuntimeData("bazfuu") +}).trigger(); +)--") + .isError()); + application()->processEvents(); + + auto data = pRuntimeData->get(); + EXPECT_TRUE(data.canConvert()); + EXPECT_EQ(data.toString(), "bazfuu"); +} + +TEST_F(ControllerRuntimeDataTest, canConnectDisconnect) { + EXPECT_TRUE(!evaluateA(R"--( +let con = engine.onRuntimeDataUpdate((data) => { + throw "Something is wrong"; +}); +if (!con.isConnected) throw "Something is wrong"; +con.disconnect() +if (con.isConnected) throw "Something is wrong"; +)--") + .isError()); + pRuntimeData->set(QVariant("foobar")); + application()->processEvents(); + + EXPECT_TRUE(runtimeDataConnectionsEngineA().isEmpty()); +} From 3fcb10714c0e2ddaf42adaa27539d4c50c4e5b25 Mon Sep 17 00:00:00 2001 From: Antoine C Date: Fri, 29 Mar 2024 11:41:15 +0000 Subject: [PATCH 2/6] Update method naming to have better consistency with existing one --- CMakeLists.txt | 4 +- res/controllers/engine-api.d.ts | 37 +++++++++++++++++- src/controllers/controllermanager.cpp | 8 ++-- src/controllers/controllermanager.h | 4 +- src/controllers/controllerruntimedata.cpp | 3 -- src/controllers/controllershareddata.cpp | 3 ++ ...erruntimedata.h => controllershareddata.h} | 6 +-- .../scripting/controllerscriptenginebase.h | 10 ++--- .../legacy/controllerscriptenginelegacy.h | 2 +- .../controllerscriptinterfacelegacy.cpp | 20 +++++----- .../legacy/controllerscriptinterfacelegacy.h | 8 ++-- ...test.cpp => controllershareddata_test.cpp} | 39 ++++++++++--------- 12 files changed, 89 insertions(+), 55 deletions(-) delete mode 100644 src/controllers/controllerruntimedata.cpp create mode 100644 src/controllers/controllershareddata.cpp rename src/controllers/{controllerruntimedata.h => controllershareddata.h} (80%) rename src/test/{controller_runtimedata_test.cpp => controllershareddata_test.cpp} (80%) diff --git a/CMakeLists.txt b/CMakeLists.txt index 22d25fe0c68..0c55a3bebb1 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -739,7 +739,7 @@ add_library(mixxx-lib STATIC EXCLUDE_FROM_ALL src/controllers/controlpickermenu.cpp src/controllers/legacycontrollermappingfilehandler.cpp src/controllers/legacycontrollermapping.cpp - src/controllers/controllerruntimedata.cpp + src/controllers/controllershareddata.cpp src/controllers/delegates/controldelegate.cpp src/controllers/delegates/midibytedelegate.cpp src/controllers/delegates/midichanneldelegate.cpp @@ -2077,7 +2077,7 @@ add_executable(mixxx-test src/test/controller_mapping_validation_test.cpp src/test/controller_mapping_settings_test.cpp src/test/controllerscriptenginelegacy_test.cpp - src/test/controller_runtimedata_test.cpp + src/test/controllershareddata_test.cpp src/test/controlobjecttest.cpp src/test/controlobjectaliastest.cpp src/test/controlobjectscripttest.cpp diff --git a/res/controllers/engine-api.d.ts b/res/controllers/engine-api.d.ts index e40f67db0d2..b988f7233e2 100644 --- a/res/controllers/engine-api.d.ts +++ b/res/controllers/engine-api.d.ts @@ -25,6 +25,7 @@ declare interface ScriptConnection { declare namespace engine { type SettingValue = string | number | boolean; + type SharedDataValue = string | number | boolean | object | array | undefined; /** * Gets the value of a controller setting * The value is either set in the preferences dialog, @@ -72,6 +73,28 @@ declare namespace engine { */ function setParameter(group: string, name: string, newValue: number): void; + /** + * Gets the shared runtime data. + * @returns Runtime shared data value + */ + function getSharedData(): SharedDataValue; + + /** + * Override the the shared runtime data with a new value. + * + * It is suggested to make additive changes (e.g add new attribute to existing object) in order to ease integration with other controller mapping + * @param newValue Runtime shared data value to be set + */ + function setSharedData(newValue: SharedDataValue): void; + + /** + * Sets the control value specified with normalized range of 0..1 + * @param group Group of the control e.g. "[Channel1]" + * @param name Name of the control e.g. "play_indicator" + * @param newValue Value to be set, normalized to a range of 0..1 + */ + function setParameter(group: string, name: string, newValue: number): void; + /** * Normalizes a specified value using the range of the given control, * to the range of 0..1 @@ -112,6 +135,7 @@ declare namespace engine { function getDefaultParameter(group: string, name: string): number; type CoCallback = (value: number, group: string, name: string) => void + type RuntimeSharedDataCallback = (value: SharedDataValue) => void /** * Connects a specified Mixxx Control with a callback function, which is executed if the value of the control changes @@ -121,9 +145,18 @@ declare namespace engine { * @param group Group of the control e.g. "[Channel1]" * @param name Name of the control e.g. "play_indicator" * @param callback JS function, which will be called every time, the value of the connected control changes. - * @returns Returns script connection object on success, otherwise 'undefined'' + * @returns Returns script connection object on success, otherwise 'undefined' + */ + function makeConnection(group: string, name: string, callback: CoCallback): ScriptConnection | undefined; + + /** + * Register callback function to be triggered when the shared data is updated + * + * Note that local update will also trigger the callback. Make sure to make your callback safe against recursion. + * @param callback JS function, which will be called every time, the shared controller value changes. + * @returns Returns script connection object on success, otherwise 'undefined' */ - function makeConnection(group: string, name: string, callback: CoCallback): ScriptConnection |undefined; + function makeSharedDataConnection(callback: RuntimeSharedDataCallback): ScriptConnection | undefined; /** * Connects a specified Mixxx Control with a callback function, which is executed if the value of the control changes diff --git a/src/controllers/controllermanager.cpp b/src/controllers/controllermanager.cpp index db382c9e1b5..aabcfcff150 100644 --- a/src/controllers/controllermanager.cpp +++ b/src/controllers/controllermanager.cpp @@ -6,7 +6,7 @@ #include "controllers/controller.h" #include "controllers/controllerlearningeventfilter.h" #include "controllers/controllermappinginfoenumerator.h" -#include "controllers/controllerruntimedata.h" +#include "controllers/controllershareddata.h" #include "controllers/defs_controllers.h" #include "controllers/scripting/legacy/controllerscriptenginelegacy.h" #include "moc_controllermanager.cpp" @@ -96,7 +96,7 @@ ControllerManager::ControllerManager(UserSettingsPointer pConfig) m_pControllerLearningEventFilter(new ControllerLearningEventFilter()), m_pollTimer(this), m_skipPoll(false), - m_pRuntimeData(std::make_shared(this)) { + m_pRuntimeData(std::make_shared(this)) { qRegisterMetaType>( "std::shared_ptr"); @@ -308,7 +308,7 @@ void ControllerManager::slotSetUpDevices() { "controller open successfully?"; continue; } - pController->getScriptEngine()->setRuntimeData(m_pRuntimeData); + pController->getScriptEngine()->setSharedData(m_pRuntimeData); pController->applyMapping(); } @@ -406,7 +406,7 @@ void ControllerManager::openController(Controller* pController) { "controller open successfully?"; return; } - pController->getScriptEngine()->setRuntimeData(m_pRuntimeData); + pController->getScriptEngine()->setSharedData(m_pRuntimeData); pController->applyMapping(); diff --git a/src/controllers/controllermanager.h b/src/controllers/controllermanager.h index 70b577dfbbf..a012b1a2e69 100644 --- a/src/controllers/controllermanager.h +++ b/src/controllers/controllermanager.h @@ -14,7 +14,7 @@ class Controller; class ControllerLearningEventFilter; class MappingInfoEnumerator; class LegacyControllerMapping; -class ControllerRuntimeData; +class ControllerSharedData; class ControllerEnumerator; /// Function to sort controllers by name @@ -87,5 +87,5 @@ class ControllerManager : public QObject { QSharedPointer m_pMainThreadUserMappingEnumerator; QSharedPointer m_pMainThreadSystemMappingEnumerator; bool m_skipPoll; - std::shared_ptr m_pRuntimeData; + std::shared_ptr m_pRuntimeData; }; diff --git a/src/controllers/controllerruntimedata.cpp b/src/controllers/controllerruntimedata.cpp deleted file mode 100644 index 83bc96ad66a..00000000000 --- a/src/controllers/controllerruntimedata.cpp +++ /dev/null @@ -1,3 +0,0 @@ -#include - -#include "moc_controllerruntimedata.cpp" diff --git a/src/controllers/controllershareddata.cpp b/src/controllers/controllershareddata.cpp new file mode 100644 index 00000000000..ca93262658c --- /dev/null +++ b/src/controllers/controllershareddata.cpp @@ -0,0 +1,3 @@ +#include + +#include "moc_controllershareddata.cpp" diff --git a/src/controllers/controllerruntimedata.h b/src/controllers/controllershareddata.h similarity index 80% rename from src/controllers/controllerruntimedata.h rename to src/controllers/controllershareddata.h index db3f72e4819..893086816ed 100644 --- a/src/controllers/controllerruntimedata.h +++ b/src/controllers/controllershareddata.h @@ -4,15 +4,15 @@ #include "util/assert.h" -/// ControllerRuntimeData is a wrapper that allows controllers script runtimes +/// ControllerSharedData is a wrapper that allows controllers script runtimes /// to share arbitrary data via a the JavaScript interface. It doesn't enforce /// any type consistency and it is the script responsibility to use this data in /// a considerate, non-destructive way (append to lists, extend to objects, /// ...), as well as expecting that others won't do so. -class ControllerRuntimeData : public QObject { +class ControllerSharedData : public QObject { Q_OBJECT public: - ControllerRuntimeData(QObject* parent) + ControllerSharedData(QObject* parent) : QObject(parent), m_value() { } diff --git a/src/controllers/scripting/controllerscriptenginebase.h b/src/controllers/scripting/controllerscriptenginebase.h index cc426267c4d..712cff427ca 100644 --- a/src/controllers/scripting/controllerscriptenginebase.h +++ b/src/controllers/scripting/controllerscriptenginebase.h @@ -8,7 +8,7 @@ class Controller; class QJSEngine; -class ControllerRuntimeData; +class ControllerSharedData; /// ControllerScriptEngineBase manages the JavaScript engine for controller scripts. /// ControllerScriptModuleEngine implements the current system using JS modules. @@ -41,11 +41,11 @@ class ControllerScriptEngineBase : public QObject { return m_bTesting; } - void setRuntimeData(std::shared_ptr runtimeData) { + void setSharedData(std::shared_ptr runtimeData) { m_pRuntimeData = std::move(runtimeData); } - std::shared_ptr getRuntimeData() const { + std::shared_ptr getSharedData() const { return m_pRuntimeData; } @@ -57,7 +57,7 @@ class ControllerScriptEngineBase : public QObject { bool m_bDisplayingExceptionDialog; std::shared_ptr m_pJSEngine; - std::shared_ptr m_pRuntimeData; + std::shared_ptr m_pRuntimeData; Controller* m_pController; const RuntimeLoggingCategory m_logger; @@ -73,5 +73,5 @@ class ControllerScriptEngineBase : public QObject { void errorDialogButton(const QString& key, QMessageBox::StandardButton button); friend class ColorMapperJSProxy; - friend class ControllerRuntimeDataTest; + friend class ControllerSharedDataTest; }; diff --git a/src/controllers/scripting/legacy/controllerscriptenginelegacy.h b/src/controllers/scripting/legacy/controllerscriptenginelegacy.h index 689539500e1..4c9335eb0c1 100644 --- a/src/controllers/scripting/legacy/controllerscriptenginelegacy.h +++ b/src/controllers/scripting/legacy/controllerscriptenginelegacy.h @@ -70,5 +70,5 @@ class ControllerScriptEngineLegacy : public ControllerScriptEngineBase { } friend class ControllerScriptEngineLegacyTest; - friend class ControllerRuntimeDataTest; + friend class ControllerSharedDataTest; }; diff --git a/src/controllers/scripting/legacy/controllerscriptinterfacelegacy.cpp b/src/controllers/scripting/legacy/controllerscriptinterfacelegacy.cpp index c8989286c6b..fab25cbfabd 100644 --- a/src/controllers/scripting/legacy/controllerscriptinterfacelegacy.cpp +++ b/src/controllers/scripting/legacy/controllerscriptinterfacelegacy.cpp @@ -2,7 +2,7 @@ #include "control/controlobject.h" #include "control/controlobjectscript.h" -#include "controllers/controllerruntimedata.h" +#include "controllers/controllershareddata.h" #include "controllers/scripting/legacy/controllerscriptenginelegacy.h" #include "controllers/scripting/legacy/scriptconnectionjsproxy.h" #include "mixer/playermanager.h" @@ -49,8 +49,8 @@ ControllerScriptInterfaceLegacy::ControllerScriptInterfaceLegacy( m_softStartActive[i] = false; } - connect(m_pEngine->getRuntimeData().get(), - &ControllerRuntimeData::updated, + connect(m_pEngine->getSharedData().get(), + &ControllerSharedData::updated, this, &ControllerScriptInterfaceLegacy::onRuntimeDataUpdated); } @@ -170,12 +170,12 @@ void ControllerScriptInterfaceLegacy::setValue( } } -QJSValue ControllerScriptInterfaceLegacy::getRuntimeData() { +QJSValue ControllerScriptInterfaceLegacy::getSharedData() { auto pJsEngine = m_pScriptEngineLegacy->jsEngine(); VERIFY_OR_DEBUG_ASSERT(pJsEngine) { return QJSValue(); } - auto pRuntimeData = m_pScriptEngineLegacy->getRuntimeData(); + auto pRuntimeData = m_pScriptEngineLegacy->getSharedData(); if (!pRuntimeData) { qWarning() << "Unable to fetch the runtime data"; @@ -185,12 +185,12 @@ QJSValue ControllerScriptInterfaceLegacy::getRuntimeData() { return pJsEngine->toScriptValue(pRuntimeData->get()); } -void ControllerScriptInterfaceLegacy::setRuntimeData(const QJSValue& value) { +void ControllerScriptInterfaceLegacy::setSharedData(const QJSValue& value) { auto pJsEngine = m_pScriptEngineLegacy->jsEngine(); VERIFY_OR_DEBUG_ASSERT(pJsEngine) { return; } - auto pRuntimeData = m_pScriptEngineLegacy->getRuntimeData(); + auto pRuntimeData = m_pScriptEngineLegacy->getSharedData(); if (!pRuntimeData) { qWarning() << "Unable to fetch the runtime data"; @@ -201,7 +201,7 @@ void ControllerScriptInterfaceLegacy::setRuntimeData(const QJSValue& value) { qDebug() << "runtime data set successfully"; } -QJSValue ControllerScriptInterfaceLegacy::onRuntimeDataUpdate(const QJSValue& callback) { +QJSValue ControllerScriptInterfaceLegacy::makeSharedDataConnection(const QJSValue& callback) { if (!callback.isCallable()) { m_pScriptEngineLegacy->throwJSError( "Tried to connect runtime data update handler" @@ -213,7 +213,7 @@ QJSValue ControllerScriptInterfaceLegacy::onRuntimeDataUpdate(const QJSValue& ca VERIFY_OR_DEBUG_ASSERT(pJsEngine) { return QJSValue(); } - auto pRuntimeData = m_pScriptEngineLegacy->getRuntimeData(); + auto pRuntimeData = m_pScriptEngineLegacy->getSharedData(); if (!pRuntimeData) { qWarning() << "Unable to fetch the runtime data"; @@ -446,7 +446,7 @@ void ControllerScriptInterfaceLegacy::triggerRuntimeDataConnection( QJSValue func = connection.callback; // copy function because QJSValue::call is not const auto args = QJSValueList{ - pJsEngine->toScriptValue(m_pScriptEngineLegacy->getRuntimeData()->get()), + pJsEngine->toScriptValue(m_pScriptEngineLegacy->getSharedData()->get()), }; QJSValue result = func.call(args); if (result.isError()) { diff --git a/src/controllers/scripting/legacy/controllerscriptinterfacelegacy.h b/src/controllers/scripting/legacy/controllerscriptinterfacelegacy.h index 5bbf5a61c19..e5b3113e984 100644 --- a/src/controllers/scripting/legacy/controllerscriptinterfacelegacy.h +++ b/src/controllers/scripting/legacy/controllerscriptinterfacelegacy.h @@ -26,9 +26,9 @@ class ControllerScriptInterfaceLegacy : public QObject { Q_INVOKABLE double getValue(const QString& group, const QString& name); Q_INVOKABLE void setValue(const QString& group, const QString& name, double newValue); - Q_INVOKABLE QJSValue getRuntimeData(); - Q_INVOKABLE void setRuntimeData(const QJSValue& value); - Q_INVOKABLE QJSValue onRuntimeDataUpdate(const QJSValue& callback); + Q_INVOKABLE QJSValue getSharedData(); + Q_INVOKABLE void setSharedData(const QJSValue& value); + Q_INVOKABLE QJSValue makeSharedDataConnection(const QJSValue& callback); Q_INVOKABLE double getParameter(const QString& group, const QString& name); Q_INVOKABLE void setParameter(const QString& group, const QString& name, double newValue); @@ -125,5 +125,5 @@ class ControllerScriptInterfaceLegacy : public QObject { QList m_runtimeDataConnections; - friend class ControllerRuntimeDataTest; + friend class ControllerSharedDataTest; }; diff --git a/src/test/controller_runtimedata_test.cpp b/src/test/controllershareddata_test.cpp similarity index 80% rename from src/test/controller_runtimedata_test.cpp rename to src/test/controllershareddata_test.cpp index b6ebf557fa3..509a68aecf6 100644 --- a/src/test/controller_runtimedata_test.cpp +++ b/src/test/controllershareddata_test.cpp @@ -1,9 +1,10 @@ +#include "controllers/controllershareddata.h" + #include #include #include "control/controlobject.h" #include "control/controlpotmeter.h" -#include "controllers/controllerruntimedata.h" #include "controllers/scripting/legacy/controllerscriptenginelegacy.h" #include "controllers/scripting/legacy/controllerscriptinterfacelegacy.h" #include "controllers/scripting/legacy/scriptconnection.h" @@ -15,17 +16,17 @@ const RuntimeLoggingCategory logger(QString("test").toLocal8Bit()); -class ControllerRuntimeDataTest : public MixxxTest { +class ControllerSharedDataTest : public MixxxTest { protected: void SetUp() override { mixxx::Time::setTestMode(true); mixxx::Time::setTestElapsedTime(mixxx::Duration::fromMillis(10)); - pRuntimeData = std::make_shared(nullptr); + pRuntimeData = std::make_shared(nullptr); cEngineA = new ControllerScriptEngineLegacy(nullptr, logger); - cEngineA->setRuntimeData(pRuntimeData); + cEngineA->setSharedData(pRuntimeData); cEngineA->initialize(); cEngineB = new ControllerScriptEngineLegacy(nullptr, logger); - cEngineA->setRuntimeData(pRuntimeData); + cEngineA->setSharedData(pRuntimeData); cEngineB->initialize(); } @@ -66,15 +67,15 @@ class ControllerRuntimeDataTest : public MixxxTest { ControllerScriptEngineLegacy* cEngineA; ControllerScriptEngineLegacy* cEngineB; - std::shared_ptr pRuntimeData; + std::shared_ptr pRuntimeData; }; -TEST_F(ControllerRuntimeDataTest, getSetRuntimeData) { +TEST_F(ControllerSharedDataTest, getSetRuntimeData) { pRuntimeData->set(QVariant("foobar")); EXPECT_TRUE(!evaluateA(R"--( -let data = engine.getRuntimeData(); +let data = engine.getSharedData(); if (data !== "foobar") throw "Something is wrong"; -engine.setRuntimeData("barfoo"); +engine.setSharedData("barfoo"); )--") .isError()); auto data = pRuntimeData->get(); @@ -82,9 +83,9 @@ engine.setRuntimeData("barfoo"); EXPECT_EQ(data.toString(), "barfoo"); EXPECT_TRUE(!evaluateB(R"--( -let data = engine.getRuntimeData(); +let data = engine.getSharedData(); if (data !== "barfoo") throw "Something is wrong"; -engine.setRuntimeData("bazfuu"); +engine.setSharedData("bazfuu"); )--") .isError()); data = pRuntimeData->get(); @@ -92,11 +93,11 @@ engine.setRuntimeData("bazfuu"); EXPECT_EQ(data.toString(), "bazfuu"); } -TEST_F(ControllerRuntimeDataTest, runtimeDataCallback) { +TEST_F(ControllerSharedDataTest, runtimeDataCallback) { EXPECT_TRUE(!evaluateA(R"--( -engine.onRuntimeDataUpdate((data) => { +engine.makeSharedDataConnection((data) => { if (data !== "foobar") throw "Something is wrong"; - engine.setRuntimeData("bazfuu") + engine.setSharedData("bazfuu") }); )--") .isError()); @@ -108,11 +109,11 @@ engine.onRuntimeDataUpdate((data) => { EXPECT_EQ(data.toString(), "bazfuu"); } -TEST_F(ControllerRuntimeDataTest, canTrigger) { +TEST_F(ControllerSharedDataTest, canTrigger) { EXPECT_TRUE(!evaluateA(R"--( -engine.onRuntimeDataUpdate((data) => { +engine.makeSharedDataConnection((data) => { if (data) return; - engine.setRuntimeData("bazfuu") + engine.setSharedData("bazfuu") }).trigger(); )--") .isError()); @@ -123,9 +124,9 @@ engine.onRuntimeDataUpdate((data) => { EXPECT_EQ(data.toString(), "bazfuu"); } -TEST_F(ControllerRuntimeDataTest, canConnectDisconnect) { +TEST_F(ControllerSharedDataTest, canConnectDisconnect) { EXPECT_TRUE(!evaluateA(R"--( -let con = engine.onRuntimeDataUpdate((data) => { +let con = engine.makeSharedDataConnection((data) => { throw "Something is wrong"; }); if (!con.isConnected) throw "Something is wrong"; From 957a427f7fe502a61213706118b587938a11002d Mon Sep 17 00:00:00 2001 From: Antoine C Date: Mon, 13 May 2024 14:57:47 +0100 Subject: [PATCH 3/6] Add namespace isolation to prevent clash --- src/controllers/controller.cpp | 8 ++- src/controllers/controller.h | 3 +- src/controllers/controllermanager.cpp | 6 +-- src/controllers/controllershareddata.cpp | 18 +++++++ src/controllers/controllershareddata.h | 51 +++++++++++++++---- src/controllers/legacycontrollermapping.h | 11 ++++ .../legacycontrollermappingfilehandler.cpp | 6 +++ src/controllers/midi/midicontroller.cpp | 4 +- src/controllers/midi/midicontroller.h | 3 +- .../scripting/controllerscriptenginebase.cpp | 4 ++ .../scripting/controllerscriptenginebase.h | 12 ++--- .../controllerscriptinterfacelegacy.cpp | 16 +++--- .../controller_mapping_validation_test.cpp | 2 +- src/test/controllershareddata_test.cpp | 45 ++++++++++++---- 14 files changed, 147 insertions(+), 42 deletions(-) diff --git a/src/controllers/controller.cpp b/src/controllers/controller.cpp index 61a4bf8c002..0f8c87bd8d2 100644 --- a/src/controllers/controller.cpp +++ b/src/controllers/controller.cpp @@ -3,6 +3,7 @@ #include #include +#include "controllers/controllershareddata.h" #include "controllers/scripting/legacy/controllerscriptenginelegacy.h" #include "moc_controller.cpp" #include "util/cmdlineargs.h" @@ -62,7 +63,7 @@ void Controller::stopEngine() { m_pScriptEngineLegacy = nullptr; } -bool Controller::applyMapping() { +bool Controller::applyMapping(std::shared_ptr runtimeData) { qCInfo(m_logBase) << "Applying controller mapping..."; const std::shared_ptr pMapping = cloneMapping(); @@ -84,6 +85,11 @@ bool Controller::applyMapping() { m_pScriptEngineLegacy->setScriptFiles(scriptFiles); m_pScriptEngineLegacy->setSettings(pMapping->getSettings()); + + const auto& ns = pMapping->sharedDataNamespace(); + if (!ns.isEmpty() && runtimeData != nullptr) { + m_pScriptEngineLegacy->setSharedData(runtimeData->namespaced(ns)); + } return m_pScriptEngineLegacy->initialize(); } diff --git a/src/controllers/controller.h b/src/controllers/controller.h index f227f1f8a40..a5194551a34 100644 --- a/src/controllers/controller.h +++ b/src/controllers/controller.h @@ -8,6 +8,7 @@ class ControllerJSProxy; class ControllerScriptEngineLegacy; +class ControllerSharedData; /// This is a base class representing a physical (or software) controller. It /// must be inherited by a class that implements it on some API. Note that the @@ -70,7 +71,7 @@ class Controller : public QObject { // this if they have an alternate way of handling such data.) virtual void receive(const QByteArray& data, mixxx::Duration timestamp); - virtual bool applyMapping(); + virtual bool applyMapping(std::shared_ptr runtimeData); virtual void slotBeforeEngineShutdown(); // Puts the controller in and out of learning mode. diff --git a/src/controllers/controllermanager.cpp b/src/controllers/controllermanager.cpp index aabcfcff150..ba9602756ce 100644 --- a/src/controllers/controllermanager.cpp +++ b/src/controllers/controllermanager.cpp @@ -308,8 +308,7 @@ void ControllerManager::slotSetUpDevices() { "controller open successfully?"; continue; } - pController->getScriptEngine()->setSharedData(m_pRuntimeData); - pController->applyMapping(); + pController->applyMapping(m_pRuntimeData); } pollIfAnyControllersOpen(); @@ -406,9 +405,8 @@ void ControllerManager::openController(Controller* pController) { "controller open successfully?"; return; } - pController->getScriptEngine()->setSharedData(m_pRuntimeData); - pController->applyMapping(); + pController->applyMapping(m_pRuntimeData); // Update configuration to reflect controller is enabled. m_pConfig->setValue( diff --git a/src/controllers/controllershareddata.cpp b/src/controllers/controllershareddata.cpp index ca93262658c..895441b6131 100644 --- a/src/controllers/controllershareddata.cpp +++ b/src/controllers/controllershareddata.cpp @@ -1,3 +1,21 @@ #include #include "moc_controllershareddata.cpp" + +ControllerNamespacedSharedData* ControllerSharedData::namespaced(const QString& ns) { + return new ControllerNamespacedSharedData(this, ns); +} + +ControllerNamespacedSharedData::ControllerNamespacedSharedData( + ControllerSharedData* parent, const QString& ns) + : QObject(parent), m_namespace(ns) { + connect(parent, + &ControllerSharedData::updated, + this, + [this](const QString& ns, const QVariant& value) { + if (ns != m_namespace) { + return; + } + emit updated(value); + }); +} diff --git a/src/controllers/controllershareddata.h b/src/controllers/controllershareddata.h index 893086816ed..ce5021d972b 100644 --- a/src/controllers/controllershareddata.h +++ b/src/controllers/controllershareddata.h @@ -4,11 +4,13 @@ #include "util/assert.h" +class ControllerNamespacedSharedData; + /// ControllerSharedData is a wrapper that allows controllers script runtimes -/// to share arbitrary data via a the JavaScript interface. It doesn't enforce -/// any type consistency and it is the script responsibility to use this data in -/// a considerate, non-destructive way (append to lists, extend to objects, -/// ...), as well as expecting that others won't do so. +/// to share arbitrary data via a the JavaScript interface. Controllers don't +/// access this object directly, and instead uses the +/// ControllerNamespacedSharedData wrapper to isolate a specific namespace and +/// prevent potential clash class ControllerSharedData : public QObject { Q_OBJECT public: @@ -16,19 +18,50 @@ class ControllerSharedData : public QObject { : QObject(parent), m_value() { } - const QVariant& get() const { - return m_value; + QVariant get(const QString& ns) const { + return m_value.value(ns); + } + + /// @brief Create a a namespace wrapper that can be used by a controller. + /// The caller is owning the wrapper + /// @param ns The namespace to restrict access to + /// @return The pointer to the newly allocated wrapper + ControllerNamespacedSharedData* namespaced(const QString& ns); + + public slots: + void set(const QString& ns, const QVariant& value) { + m_value[ns] = value; + emit updated(ns, m_value[ns]); + } + + signals: + void updated(const QString& ns, const QVariant& value); + + private: + QHash m_value; +}; + +/// ControllerNamespacedSharedData is a wrapper that restrict access to a given +/// namespace. It doesn't held any data and can safely be deleted at all time, +/// but only provide the namespace abstraction for controller to interact with +/// via a the JavaScript interface +class ControllerNamespacedSharedData : public QObject { + Q_OBJECT + public: + ControllerNamespacedSharedData(ControllerSharedData* parent, const QString& ns); + + QVariant get() const { + return static_cast(parent())->get(m_namespace); } public slots: void set(const QVariant& value) { - m_value = value; - emit updated(m_value); + static_cast(parent())->set(m_namespace, value); } signals: void updated(const QVariant& value); private: - QVariant m_value; + QString m_namespace; }; diff --git a/src/controllers/legacycontrollermapping.h b/src/controllers/legacycontrollermapping.h index 0e556c9a11e..24e4f179288 100644 --- a/src/controllers/legacycontrollermapping.h +++ b/src/controllers/legacycontrollermapping.h @@ -22,6 +22,7 @@ class LegacyControllerMapping { : m_productMatches(other.m_productMatches), m_bDirty(other.m_bDirty), m_deviceId(other.m_deviceId), + m_sharedDataNamespace(other.m_sharedDataNamespace), m_filePath(other.m_filePath), m_name(other.m_name), m_author(other.m_author), @@ -161,6 +162,15 @@ class LegacyControllerMapping { return m_deviceId; } + void setSharedDataNamespace(QString sharedDataNamespace) { + m_sharedDataNamespace = std::move(sharedDataNamespace); + setDirty(true); + } + + const QString& sharedDataNamespace() const { + return m_sharedDataNamespace; + } + void setFilePath(const QString& filePath) { m_filePath = filePath; setDirty(true); @@ -277,6 +287,7 @@ class LegacyControllerMapping { bool m_bDirty; QString m_deviceId; + QString m_sharedDataNamespace; QString m_filePath; QString m_name; QString m_author; diff --git a/src/controllers/legacycontrollermappingfilehandler.cpp b/src/controllers/legacycontrollermappingfilehandler.cpp index b8c6ab94321..ebab5f92253 100644 --- a/src/controllers/legacycontrollermappingfilehandler.cpp +++ b/src/controllers/legacycontrollermappingfilehandler.cpp @@ -210,6 +210,12 @@ void LegacyControllerMappingFileHandler::addScriptFilesToMapping( QString deviceId = controller.attribute("id", ""); mapping->setDeviceId(deviceId); + // Empty namespace is forbidden. If a controller wants to use shared data, + // they must specify an non-empty string. + QString sharedDataNamespace = controller.attribute("namespace", ""); + if (!sharedDataNamespace.isEmpty()) { + mapping->setSharedDataNamespace(deviceId); + } // See TODO in LegacyControllerMapping::DeviceDirection - `direction` should // only be used as a workaround till the bulk integration gets refactored diff --git a/src/controllers/midi/midicontroller.cpp b/src/controllers/midi/midicontroller.cpp index f815cf5d0d2..9b97864ccbd 100644 --- a/src/controllers/midi/midicontroller.cpp +++ b/src/controllers/midi/midicontroller.cpp @@ -76,9 +76,9 @@ bool MidiController::matchMapping(const MappingInfo& mapping) { return false; } -bool MidiController::applyMapping() { +bool MidiController::applyMapping(std::shared_ptr runtimeData) { // Handles the engine - bool result = Controller::applyMapping(); + bool result = Controller::applyMapping(std::move(runtimeData)); // Only execute this code if this is an output device if (isOutputDevice()) { diff --git a/src/controllers/midi/midicontroller.h b/src/controllers/midi/midicontroller.h index dab0a4dd3cd..b36d334a926 100644 --- a/src/controllers/midi/midicontroller.h +++ b/src/controllers/midi/midicontroller.h @@ -9,6 +9,7 @@ #include "controllers/softtakeover.h" class MidiOutputHandler; +class ControllerSharedData; class MidiInputHandleJSProxy final : public QObject { Q_OBJECT @@ -81,7 +82,7 @@ class MidiController : public Controller { void slotBeforeEngineShutdown() override; private slots: - bool applyMapping() override; + bool applyMapping(std::shared_ptr) override; void learnTemporaryInputMappings(const MidiInputMappings& mappings); void clearTemporaryInputMappings(); diff --git a/src/controllers/scripting/controllerscriptenginebase.cpp b/src/controllers/scripting/controllerscriptenginebase.cpp index eca4c67d669..b6fa05d57e1 100644 --- a/src/controllers/scripting/controllerscriptenginebase.cpp +++ b/src/controllers/scripting/controllerscriptenginebase.cpp @@ -218,3 +218,7 @@ void ControllerScriptEngineBase::errorDialogButton( void ControllerScriptEngineBase::throwJSError(const QString& message) { m_pJSEngine->throwError(message); } + +void ControllerScriptEngineBase::setSharedData(ControllerNamespacedSharedData* runtimeData) { + m_runtimeData = std::unique_ptr(runtimeData); +} diff --git a/src/controllers/scripting/controllerscriptenginebase.h b/src/controllers/scripting/controllerscriptenginebase.h index 774244adc85..665afd63a18 100644 --- a/src/controllers/scripting/controllerscriptenginebase.h +++ b/src/controllers/scripting/controllerscriptenginebase.h @@ -4,11 +4,11 @@ #include #include +#include "controllers/controllershareddata.h" #include "util/runtimeloggingcategory.h" class Controller; class QJSEngine; -class ControllerSharedData; /// ControllerScriptEngineBase manages the JavaScript engine for controller scripts. /// ControllerScriptModuleEngine implements the current system using JS modules. @@ -41,12 +41,10 @@ class ControllerScriptEngineBase : public QObject { return m_bTesting; } - void setSharedData(std::shared_ptr runtimeData) { - m_pRuntimeData = std::move(runtimeData); - } + void setSharedData(ControllerNamespacedSharedData* runtimeData); - std::shared_ptr getSharedData() const { - return m_pRuntimeData; + ControllerNamespacedSharedData* getSharedData() { + return m_runtimeData.get(); } signals: void beforeShutdown(); @@ -59,7 +57,7 @@ class ControllerScriptEngineBase : public QObject { bool m_bDisplayingExceptionDialog; std::shared_ptr m_pJSEngine; - std::shared_ptr m_pRuntimeData; + std::unique_ptr m_runtimeData; Controller* m_pController; const RuntimeLoggingCategory m_logger; diff --git a/src/controllers/scripting/legacy/controllerscriptinterfacelegacy.cpp b/src/controllers/scripting/legacy/controllerscriptinterfacelegacy.cpp index 4898cdd8e59..611a05231ae 100644 --- a/src/controllers/scripting/legacy/controllerscriptinterfacelegacy.cpp +++ b/src/controllers/scripting/legacy/controllerscriptinterfacelegacy.cpp @@ -49,10 +49,12 @@ ControllerScriptInterfaceLegacy::ControllerScriptInterfaceLegacy( m_softStartActive[i] = false; } - connect(m_pEngine->getSharedData().get(), - &ControllerSharedData::updated, - this, - &ControllerScriptInterfaceLegacy::onRuntimeDataUpdated); + if (m_pEngine->getSharedData()) { + connect(m_pEngine->getSharedData(), + &ControllerNamespacedSharedData::updated, + this, + &ControllerScriptInterfaceLegacy::onRuntimeDataUpdated); + } } ControllerScriptInterfaceLegacy::~ControllerScriptInterfaceLegacy() { @@ -178,7 +180,7 @@ QJSValue ControllerScriptInterfaceLegacy::getSharedData() { auto pRuntimeData = m_pScriptEngineLegacy->getSharedData(); if (!pRuntimeData) { - qWarning() << "Unable to fetch the runtime data"; + qWarning() << "No runtime data available. Make sure a valid namespace is defined."; return QJSValue(); } @@ -193,7 +195,7 @@ void ControllerScriptInterfaceLegacy::setSharedData(const QJSValue& value) { auto pRuntimeData = m_pScriptEngineLegacy->getSharedData(); if (!pRuntimeData) { - qWarning() << "Unable to fetch the runtime data"; + qWarning() << "No runtime data available. Make sure a valid namespace is defined."; return; } @@ -216,7 +218,7 @@ QJSValue ControllerScriptInterfaceLegacy::makeSharedDataConnection(const QJSValu auto pRuntimeData = m_pScriptEngineLegacy->getSharedData(); if (!pRuntimeData) { - qWarning() << "Unable to fetch the runtime data"; + qWarning() << "No runtime data available. Make sure a valid namespace is defined."; return QJSValue(); } diff --git a/src/test/controller_mapping_validation_test.cpp b/src/test/controller_mapping_validation_test.cpp index 29be36d171c..964c90b11b7 100644 --- a/src/test/controller_mapping_validation_test.cpp +++ b/src/test/controller_mapping_validation_test.cpp @@ -123,7 +123,7 @@ bool LegacyControllerMappingValidationTest::testLoadMapping(const MappingInfo& m FakeController controller; controller.setMapping(pMapping); - bool result = controller.applyMapping(); + bool result = controller.applyMapping(std::shared_ptr(nullptr)); controller.stopEngine(); return result; } diff --git a/src/test/controllershareddata_test.cpp b/src/test/controllershareddata_test.cpp index 509a68aecf6..fc483e96878 100644 --- a/src/test/controllershareddata_test.cpp +++ b/src/test/controllershareddata_test.cpp @@ -23,10 +23,10 @@ class ControllerSharedDataTest : public MixxxTest { mixxx::Time::setTestElapsedTime(mixxx::Duration::fromMillis(10)); pRuntimeData = std::make_shared(nullptr); cEngineA = new ControllerScriptEngineLegacy(nullptr, logger); - cEngineA->setSharedData(pRuntimeData); + cEngineA->setSharedData(pRuntimeData->namespaced("testNS")); cEngineA->initialize(); cEngineB = new ControllerScriptEngineLegacy(nullptr, logger); - cEngineA->setSharedData(pRuntimeData); + cEngineB->setSharedData(pRuntimeData->namespaced("testNS")); cEngineB->initialize(); } @@ -71,14 +71,14 @@ class ControllerSharedDataTest : public MixxxTest { }; TEST_F(ControllerSharedDataTest, getSetRuntimeData) { - pRuntimeData->set(QVariant("foobar")); + pRuntimeData->set("testNS", QVariant("foobar")); EXPECT_TRUE(!evaluateA(R"--( let data = engine.getSharedData(); if (data !== "foobar") throw "Something is wrong"; engine.setSharedData("barfoo"); )--") .isError()); - auto data = pRuntimeData->get(); + auto data = pRuntimeData->get("testNS"); EXPECT_TRUE(data.canConvert()); EXPECT_EQ(data.toString(), "barfoo"); @@ -88,7 +88,7 @@ if (data !== "barfoo") throw "Something is wrong"; engine.setSharedData("bazfuu"); )--") .isError()); - data = pRuntimeData->get(); + data = pRuntimeData->get("testNS"); EXPECT_TRUE(data.canConvert()); EXPECT_EQ(data.toString(), "bazfuu"); } @@ -101,10 +101,10 @@ engine.makeSharedDataConnection((data) => { }); )--") .isError()); - pRuntimeData->set(QVariant("foobar")); + pRuntimeData->set("testNS", QVariant("foobar")); application()->processEvents(); - auto data = pRuntimeData->get(); + auto data = pRuntimeData->get("testNS"); EXPECT_TRUE(data.canConvert()); EXPECT_EQ(data.toString(), "bazfuu"); } @@ -119,7 +119,7 @@ engine.makeSharedDataConnection((data) => { .isError()); application()->processEvents(); - auto data = pRuntimeData->get(); + auto data = pRuntimeData->get("testNS"); EXPECT_TRUE(data.canConvert()); EXPECT_EQ(data.toString(), "bazfuu"); } @@ -134,8 +134,35 @@ con.disconnect() if (con.isConnected) throw "Something is wrong"; )--") .isError()); - pRuntimeData->set(QVariant("foobar")); + pRuntimeData->set("testNS", QVariant("foobar")); application()->processEvents(); EXPECT_TRUE(runtimeDataConnectionsEngineA().isEmpty()); } + +TEST_F(ControllerSharedDataTest, namespacePreventClash) { + pRuntimeData->set("testNS", QVariant("foobar")); + EXPECT_TRUE(!evaluateA(R"--( +let data = engine.getSharedData(); +if (data !== "foobar") throw "Something is wrong"; +engine.setSharedData("barfoo"); +)--") + .isError()); + auto data = pRuntimeData->get("testNS"); + EXPECT_TRUE(data.canConvert()); + EXPECT_EQ(data.toString(), "barfoo"); + + pRuntimeData->set("otherTestNS", QVariant("foobar")); + cEngineA->setSharedData(pRuntimeData->namespaced("otherTestNS")); + EXPECT_TRUE(!evaluateA(R"--( +let data = engine.getSharedData(); +if (data !== "foobar") throw "Something is wrong"; +engine.setSharedData("barfoo"); +)--") + .isError()); + data = pRuntimeData->get("otherTestNS"); + EXPECT_TRUE(data.canConvert()); + EXPECT_EQ(data.toString(), "barfoo"); +} + +// TODO test namespace From 172be8cf30884472fc917daaee13aa80589f0de4 Mon Sep 17 00:00:00 2001 From: Antoine C Date: Mon, 13 May 2024 16:28:58 +0100 Subject: [PATCH 4/6] Some more nits comming one by one --- .../scripting/controllerscriptenginebase.h | 1 + .../legacy/controllerscriptinterfacelegacy.cpp | 13 ++++--------- 2 files changed, 5 insertions(+), 9 deletions(-) diff --git a/src/controllers/scripting/controllerscriptenginebase.h b/src/controllers/scripting/controllerscriptenginebase.h index 665afd63a18..972b757ef37 100644 --- a/src/controllers/scripting/controllerscriptenginebase.h +++ b/src/controllers/scripting/controllerscriptenginebase.h @@ -41,6 +41,7 @@ class ControllerScriptEngineBase : public QObject { return m_bTesting; } + /// Takes ownership of `runtimeData` void setSharedData(ControllerNamespacedSharedData* runtimeData); ControllerNamespacedSharedData* getSharedData() { diff --git a/src/controllers/scripting/legacy/controllerscriptinterfacelegacy.cpp b/src/controllers/scripting/legacy/controllerscriptinterfacelegacy.cpp index 611a05231ae..563ec0a4762 100644 --- a/src/controllers/scripting/legacy/controllerscriptinterfacelegacy.cpp +++ b/src/controllers/scripting/legacy/controllerscriptinterfacelegacy.cpp @@ -240,9 +240,8 @@ void ControllerScriptInterfaceLegacy::onRuntimeDataUpdated(const QVariant& value pJsEngine->toScriptValue(value), }; - for (const auto& connection : m_runtimeDataConnections) { - QJSValue func = connection.callback; // copy function because QJSValue::call is not const - QJSValue result = func.call(args); + for (auto& connection : m_runtimeDataConnections) { + QJSValue result = connection.callback.call(args); if (result.isError()) { if (m_pScriptEngineLegacy != nullptr) { m_pScriptEngineLegacy->showScriptExceptionDialog(result); @@ -428,14 +427,10 @@ void ControllerScriptInterfaceLegacy::triggerScriptConnection( bool ControllerScriptInterfaceLegacy::removeRuntimeDataConnection( const ScriptConnection& connection) { - if (m_pScriptEngineLegacy->jsEngine() == nullptr || - !m_runtimeDataConnections.contains(connection)) { + VERIFY_OR_DEBUG_ASSERT(m_pScriptEngineLegacy->jsEngine()) { return false; } - - m_runtimeDataConnections.removeAll(connection); - - return true; + return m_runtimeDataConnections.removeAll(connection) > 0; } void ControllerScriptInterfaceLegacy::triggerRuntimeDataConnection( From 56e2d9f51923181210a1fd18828a50e74172e44f Mon Sep 17 00:00:00 2001 From: Antoine C Date: Tue, 14 May 2024 12:26:25 +0100 Subject: [PATCH 5/6] Fix the clang-tidy --- .../scripting/legacy/controllerscriptinterfacelegacy.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/controllers/scripting/legacy/controllerscriptinterfacelegacy.cpp b/src/controllers/scripting/legacy/controllerscriptinterfacelegacy.cpp index 563ec0a4762..ccde03a844e 100644 --- a/src/controllers/scripting/legacy/controllerscriptinterfacelegacy.cpp +++ b/src/controllers/scripting/legacy/controllerscriptinterfacelegacy.cpp @@ -177,7 +177,7 @@ QJSValue ControllerScriptInterfaceLegacy::getSharedData() { VERIFY_OR_DEBUG_ASSERT(pJsEngine) { return QJSValue(); } - auto pRuntimeData = m_pScriptEngineLegacy->getSharedData(); + auto* pRuntimeData = m_pScriptEngineLegacy->getSharedData(); if (!pRuntimeData) { qWarning() << "No runtime data available. Make sure a valid namespace is defined."; @@ -192,7 +192,7 @@ void ControllerScriptInterfaceLegacy::setSharedData(const QJSValue& value) { VERIFY_OR_DEBUG_ASSERT(pJsEngine) { return; } - auto pRuntimeData = m_pScriptEngineLegacy->getSharedData(); + auto* pRuntimeData = m_pScriptEngineLegacy->getSharedData(); if (!pRuntimeData) { qWarning() << "No runtime data available. Make sure a valid namespace is defined."; @@ -215,7 +215,7 @@ QJSValue ControllerScriptInterfaceLegacy::makeSharedDataConnection(const QJSValu VERIFY_OR_DEBUG_ASSERT(pJsEngine) { return QJSValue(); } - auto pRuntimeData = m_pScriptEngineLegacy->getSharedData(); + auto* pRuntimeData = m_pScriptEngineLegacy->getSharedData(); if (!pRuntimeData) { qWarning() << "No runtime data available. Make sure a valid namespace is defined."; From c0410fda94fcb0e6c6f54e115b0e7fc0b27e8dc6 Mon Sep 17 00:00:00 2001 From: Antoine C Date: Wed, 15 May 2024 12:44:20 +0100 Subject: [PATCH 6/6] Throw JS error instead of logging and prompting an error message --- src/controllers/controllershareddata.h | 2 +- .../controllerscriptinterfacelegacy.cpp | 20 ++++++++----------- 2 files changed, 9 insertions(+), 13 deletions(-) diff --git a/src/controllers/controllershareddata.h b/src/controllers/controllershareddata.h index ce5021d972b..0fe0a14b6f8 100644 --- a/src/controllers/controllershareddata.h +++ b/src/controllers/controllershareddata.h @@ -42,7 +42,7 @@ class ControllerSharedData : public QObject { }; /// ControllerNamespacedSharedData is a wrapper that restrict access to a given -/// namespace. It doesn't held any data and can safely be deleted at all time, +/// namespace. It doesn't hold any data and can safely be deleted at all time, /// but only provide the namespace abstraction for controller to interact with /// via a the JavaScript interface class ControllerNamespacedSharedData : public QObject { diff --git a/src/controllers/scripting/legacy/controllerscriptinterfacelegacy.cpp b/src/controllers/scripting/legacy/controllerscriptinterfacelegacy.cpp index ccde03a844e..6bc4bd0aad8 100644 --- a/src/controllers/scripting/legacy/controllerscriptinterfacelegacy.cpp +++ b/src/controllers/scripting/legacy/controllerscriptinterfacelegacy.cpp @@ -243,12 +243,10 @@ void ControllerScriptInterfaceLegacy::onRuntimeDataUpdated(const QVariant& value for (auto& connection : m_runtimeDataConnections) { QJSValue result = connection.callback.call(args); if (result.isError()) { - if (m_pScriptEngineLegacy != nullptr) { - m_pScriptEngineLegacy->showScriptExceptionDialog(result); - } - qWarning() << "ControllerEngine: Invocation of connection " << connection.id.toString() - << "connected to runtime data failed:" - << result.toString(); + m_pScriptEngineLegacy->logOrThrowError( + QStringLiteral("Invocation of runtime data connection %1 " + "failed: %2") + .arg(connection.id.toString(), result.toString())); } } } @@ -447,12 +445,10 @@ void ControllerScriptInterfaceLegacy::triggerRuntimeDataConnection( }; QJSValue result = func.call(args); if (result.isError()) { - if (m_pScriptEngineLegacy != nullptr) { - m_pScriptEngineLegacy->showScriptExceptionDialog(result); - } - qWarning() << "ControllerEngine: Invocation of runtime data connection " - << connection.id.toString() - << "connected to runtime data failed:" << result.toString(); + m_pScriptEngineLegacy->logOrThrowError( + QStringLiteral( + "Invocation of runtime data connection %1 failed: %2") + .arg(connection.id.toString(), result.toString())); } }