Merge pull request #3463 from Holzhaus/controllerengine_refactoring

ControllerEngine Refactoring

CMakeLists.txt

@@ -414,12 +414,14 @@ add_library(mixxx-lib STATIC EXCLUDE_FROM_ALL
   src/controllers/dlgprefcontrollerdlg.ui
   src/controllers/dlgprefcontrollers.cpp
   src/controllers/dlgprefcontrollersdlg.ui
-  src/controllers/engine/controllerengine.cpp
-  src/controllers/engine/controllerenginejsproxy.cpp
-  src/controllers/engine/colormapper.cpp
-  src/controllers/engine/colormapperjsproxy.cpp
-  src/controllers/engine/scriptconnection.cpp
-  src/controllers/engine/scriptconnectionjsproxy.cpp
+  src/controllers/scripting/controllerscriptenginebase.cpp
+  src/controllers/scripting/controllerscriptmoduleengine.cpp
+  src/controllers/scripting/colormapper.cpp
+  src/controllers/scripting/colormapperjsproxy.cpp
+  src/controllers/scripting/legacy/controllerscriptenginelegacy.cpp
+  src/controllers/scripting/legacy/controllerscriptinterfacelegacy.cpp
+  src/controllers/scripting/legacy/scriptconnection.cpp
+  src/controllers/scripting/legacy/scriptconnectionjsproxy.cpp
   src/controllers/keyboard/keyboardeventfilter.cpp
   src/controllers/learningutils.cpp
   src/controllers/midi/midicontroller.cpp
@@ -1389,7 +1391,7 @@ add_executable(mixxx-test
   src/test/compatibility_test.cpp
   src/test/configobject_test.cpp
   src/test/controller_preset_validation_test.cpp
-  src/test/controllerengine_test.cpp
+  src/test/controllerscriptenginelegacy_test.cpp
   src/test/controlobjecttest.cpp
   src/test/coverartcache_test.cpp
   src/test/coverartutils_test.cpp

src/control/controlobjectscript.cpp

@@ -2,10 +2,11 @@
 
 #include <QtDebug>
 
+#include "controllers/controllerdebug.h"
 #include "moc_controlobjectscript.cpp"
 
 ControlObjectScript::ControlObjectScript(const ConfigKey& key, QObject* pParent)
-        : ControlProxy(key, pParent, ControlFlag::NoAssertIfMissing) {
+        : ControlProxy(key, pParent, ControllerDebug::controlFlags()) {
 }
 
 bool ControlObjectScript::addScriptConnection(const ScriptConnection& conn) {

src/control/controlobjectscript.h

@@ -3,8 +3,7 @@
 #include <QVector>
 
 #include "control/controlproxy.h"
-#include "controllers/controllerdebug.h"
-#include "controllers/engine/scriptconnection.h"
+#include "controllers/scripting/legacy/scriptconnection.h"
 
 // this is used for communicate with controller scripts
 class ControlObjectScript : public ControlProxy {

src/controllers/bulk/bulkcontroller.cpp

@@ -47,8 +47,8 @@ void BulkReader::run() {
         if (result >= 0) {
             Trace process("BulkReader process packet");
             //qDebug() << "Read" << result << "bytes, pointer:" << data;
-            QByteArray outData((char*)data, transferred);
-            emit incomingData(outData, mixxx::Time::elapsed());
+            QByteArray byteArray(reinterpret_cast<char*>(data), transferred);
+            emit incomingData(byteArray, mixxx::Time::elapsed());
         }
     }
     qDebug() << "Stopped Reader";

src/controllers/controller.cpp

@@ -9,7 +9,7 @@
 #include "util/screensaver.h"
 
 Controller::Controller()
-        : m_pEngine(nullptr),
+        : m_pScriptEngineLegacy(nullptr),
           m_bIsOutputDevice(false),
           m_bIsInputDevice(false),
           m_bIsOpen(false),
@@ -29,31 +29,30 @@ ControllerJSProxy* Controller::jsProxy() {
 void Controller::startEngine()
 {
     controllerDebug("  Starting engine");
-    if (m_pEngine != nullptr) {
+    if (m_pScriptEngineLegacy) {
         qWarning() << "Controller: Engine already exists! Restarting:";
         stopEngine();
     }
-    m_pEngine = new ControllerEngine(this);
+    m_pScriptEngineLegacy = new ControllerScriptEngineLegacy(this);
 }
 
 void Controller::stopEngine() {
     controllerDebug("  Shutting down engine");
-    if (m_pEngine == nullptr) {
+    if (!m_pScriptEngineLegacy) {
         qWarning() << "Controller::stopEngine(): No engine exists!";
         return;
     }
-    m_pEngine->gracefulShutdown();
-    delete m_pEngine;
-    m_pEngine = nullptr;
+    delete m_pScriptEngineLegacy;
+    m_pScriptEngineLegacy = nullptr;
 }
 
-bool Controller::applyPreset(bool initializeScripts) {
+bool Controller::applyPreset() {
     qDebug() << "Applying controller preset...";
 
     const ControllerPreset* pPreset = preset();
 
     // Load the script code into the engine
-    if (m_pEngine == nullptr) {
+    if (!m_pScriptEngineLegacy) {
         qWarning() << "Controller::applyPreset(): No engine exists!";
         return false;
     }
@@ -64,19 +63,8 @@ bool Controller::applyPreset(bool initializeScripts) {
         return true;
     }
 
-    bool success = m_pEngine->loadScriptFiles(scriptFiles);
-    if (success && initializeScripts) {
-        m_pEngine->initializeScripts(scriptFiles);
-    }
-
-    // QFileInfo does not have a isValid/isEmpty/isNull method to check if it
-    // actually contains a reference, so we check if the filePath is empty as a
-    // workaround.
-    // See https://stackoverflow.com/a/45652741/1455128 for details.
-    if (initializeScripts && !pPreset->moduleFileInfo().filePath().isEmpty()) {
-        m_pEngine->loadModule(pPreset->moduleFileInfo());
-    }
-    return success;
+    m_pScriptEngineLegacy->setScriptFiles(scriptFiles);
+    return m_pScriptEngineLegacy->initialize();
 }
 
 void Controller::startLearning() {
@@ -113,7 +101,7 @@ void Controller::triggerActivity()
     }
 }
 void Controller::receive(const QByteArray& data, mixxx::Duration timestamp) {
-    if (m_pEngine == nullptr) {
+    if (!m_pScriptEngineLegacy) {
         //qWarning() << "Controller::receive called with no active engine!";
         // Don't complain, since this will always show after closing a device as
         //  queued signals flush out
@@ -122,7 +110,7 @@ void Controller::receive(const QByteArray& data, mixxx::Duration timestamp) {
     triggerActivity();
 
     int length = data.size();
-    if (ControllerDebug::enabled()) {
+    if (ControllerDebug::isEnabled()) {
         // Formatted packet display
         QString message = QString("%1: t:%2, %3 bytes:\n")
                                   .arg(m_sDeviceName,
@@ -145,14 +133,5 @@ void Controller::receive(const QByteArray& data, mixxx::Duration timestamp) {
         controllerDebug(message);
     }
 
-    foreach (QString function, m_pEngine->getScriptFunctionPrefixes()) {
-        if (function == "") {
-            continue;
-        }
-        function.append(".incomingData");
-        QJSValue incomingDataFunction = m_pEngine->wrapFunctionCode(function, 2);
-        m_pEngine->executeFunction(incomingDataFunction, data);
-    }
-
-    m_pEngine->handleInput(data, timestamp);
+    m_pScriptEngineLegacy->handleIncomingData(data);
 }

src/controllers/controller.h

@@ -1,19 +1,18 @@
 #pragma once
 
 #include <QElapsedTimer>
+#include <QTimerEvent>
 
 #include "controllers/controllerpreset.h"
 #include "controllers/controllerpresetfilehandler.h"
 #include "controllers/controllerpresetinfo.h"
 #include "controllers/controllerpresetvisitor.h"
 #include "controllers/controllervisitor.h"
-#include "controllers/engine/controllerengine.h"
+#include "controllers/scripting/legacy/controllerscriptenginelegacy.h"
 #include "util/duration.h"
 
 class ControllerJSProxy;
 
-/// Base class representing a physical (or software) controller.
-///
 /// 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
 /// subclass' destructor should call close() at a minimum.
@@ -85,13 +84,7 @@ class Controller : public QObject, ConstControllerPresetVisitor {
     // this if they have an alternate way of handling such data.)
     virtual void receive(const QByteArray& data, mixxx::Duration timestamp);
 
-    /// Apply the preset to the controller.
-    /// Initializes both controller engine and static output mappings.
-    ///
-    /// @param initializeScripts Can be set to false to skip script
-    /// initialization for unit tests.
-    /// @return Returns whether it was successful.
-    virtual bool applyPreset(bool initializeScripts = true);
+    virtual bool applyPreset();
 
     // Puts the controller in and out of learning mode.
     void startLearning();
@@ -108,17 +101,17 @@ class Controller : public QObject, ConstControllerPresetVisitor {
 
     // To be called in sub-class' open() functions after opening the device but
     // before starting any input polling/processing.
-    void startEngine();
+    virtual void startEngine();
 
     // To be called in sub-class' close() functions after stopping any input
     // polling/processing but before closing the device.
-    void stopEngine();
+    virtual void stopEngine();
 
     // To be called when receiving events
     void triggerActivity();
 
-    inline ControllerEngine* getEngine() const {
-        return m_pEngine;
+    inline ControllerScriptEngineLegacy* getScriptEngine() const {
+        return m_pScriptEngineLegacy;
     }
     inline void setDeviceName(const QString& deviceName) {
         m_sDeviceName = deviceName;
@@ -155,7 +148,7 @@ class Controller : public QObject, ConstControllerPresetVisitor {
     // Returns a pointer to the currently loaded controller preset. For internal
     // use only.
     virtual ControllerPreset* preset() = 0;
-    ControllerEngine* m_pEngine;
+    ControllerScriptEngineLegacy* m_pScriptEngineLegacy;
 
     // Verbose and unique device name suitable for display.
     QString m_sDeviceName;

src/controllers/controllerdebug.cpp

@@ -5,8 +5,9 @@
 
 //static
 bool ControllerDebug::s_enabled = false;
+bool ControllerDebug::s_testing = false;
 
 //static
-bool ControllerDebug::enabled() {
+bool ControllerDebug::isEnabled() {
     return s_enabled || CmdlineArgs::Instance().getMidiDebug();
 }

src/controllers/controllerdebug.h

@@ -2,6 +2,8 @@
 
 #include <QDebug>
 
+#include "control/control.h"
+
 // Specifies whether or not we should dump incoming data to the console at
 // runtime. This is useful for end-user debugging and script-writing.
 class ControllerDebug {
@@ -11,22 +13,36 @@ class ControllerDebug {
     static constexpr const char kLogMessagePrefix[] = "CDBG";
     static constexpr int kLogMessagePrefixLength = sizeof(kLogMessagePrefix) - 1;
 
-    static bool enabled();
+    static bool isEnabled();
 
     /// Override the command-line argument (for testing)
-    static void enable() {
-        s_enabled = true;
+    static void setEnabled(bool enabled) {
+        s_enabled = enabled;
     }
 
-    /// Override the command-line argument (for testing)
-    static void disable() {
-        s_enabled = false;
+    static void setTesting(bool isTesting) {
+        s_testing = isTesting;
+    }
+
+    /// Return the appropriate flag for ControlProxies in mappings.
+    ///
+    /// Normally, accessing an invalid control from a mapping should *not*
+    /// throw a debug assertion because controller mappings are considered
+    /// user data. If we're testing or controller debugging is enabled, we *do*
+    /// want assertions to prevent overlooking bugs in controller mappings.
+    static ControlFlags controlFlags() {
+        if (s_enabled || s_testing) {
+            return ControlFlag::None;
+        }
+
+        return ControlFlag::AllowMissingOrInvalid;
     }
 
   private:
     ControllerDebug() = delete;
 
     static bool s_enabled;
+    static bool s_testing;
 };
 
 // Usage: controllerDebug("hello" << "world");
@@ -39,7 +55,7 @@ class ControllerDebug {
 // output for mixxx.log with .noquote(), because in qt5 QDebug() is quoted by default.
 #define controllerDebug(stream)                                                           \
     {                                                                                     \
-        if (ControllerDebug::enabled()) {                                                 \
+        if (ControllerDebug::isEnabled()) {                                               \
             QDebug(QtDebugMsg).noquote() << ControllerDebug::kLogMessagePrefix << stream; \
         }                                                                                 \
     }

src/controllers/controllerpreset.h

@@ -55,14 +55,6 @@ class ControllerPreset {
         return m_scripts;
     }
 
-    void setModuleFileInfo(QFileInfo fileInfo) {
-        m_moduleFileInfo = std::move(fileInfo);
-    }
-
-    QFileInfo moduleFileInfo() const {
-        return m_moduleFileInfo;
-    }
-
     inline void setDirty(bool bDirty) {
         m_bDirty = bDirty;
     }
@@ -203,7 +195,6 @@ class ControllerPreset {
     QString m_mixxxVersion;
 
     QList<ScriptFileInfo> m_scripts;
-    QFileInfo m_moduleFileInfo;
 };
 
 typedef QSharedPointer<ControllerPreset> ControllerPresetPointer;

src/controllers/controllerpresetfilehandler.cpp

@@ -142,20 +142,6 @@ void ControllerPresetFileHandler::addScriptFilesToPreset(
         preset->addScriptFile(filename, functionPrefix, file);
         scriptFile = scriptFile.nextSiblingElement("file");
     }
-
-    QString moduleFileName = controller.firstChildElement("module").text();
-
-    if (moduleFileName.isEmpty()) {
-        return;
-    }
-
-    QFileInfo moduleFileInfo(preset->dirPath().absoluteFilePath(moduleFileName));
-    if (!moduleFileInfo.isFile()) {
-        qWarning() << "Controller Module is not a file:" << moduleFileInfo.absoluteFilePath();
-        return;
-    }
-
-    preset->setModuleFileInfo(moduleFileInfo);
 }
 
 bool ControllerPresetFileHandler::writeDocument(