fixup! feat: improve screen rendering framework

src/controllers/controller.h

@@ -141,7 +141,8 @@ class Controller : public QObject {
 
   public:
     // This must be reimplemented by sub-classes desiring to send raw bytes to a
-    // controller. Return true in case of successful completion, false in case of failure.
+    // controller. Return true in case of successful completion, false in case
+    // of partial completion or failure.
     virtual bool sendBytes(const QByteArray& data) = 0;
 
   private: // but used by ControllerManager

src/controllers/scripting/legacy/controllerscriptenginelegacy.cpp

@@ -114,11 +114,11 @@ bool ControllerScriptEngineLegacy::callShutdownFunction() {
         return callFunctionOnObjects(m_scriptFunctionPrefixes, "shutdown");
 #ifdef MIXXX_USE_QML
     } else {
-        QHashIterator<QString, mixxx::qml::QmlMixxxController*> i(m_rootItems);
+        QHashIterator<QString, mixxx::qml::QmlMixxxControllerScreen*> i(m_rootItems);
         bool success = true;
         while (i.hasNext()) {
             i.next();
-            auto& screen = i.value();
+            const auto& screen = i.value();
             const QString& screenIdentifier = i.key();
 
             if (!screen->getShutdown().isCallable()) {
@@ -148,9 +148,8 @@ bool ControllerScriptEngineLegacy::callShutdownFunction() {
 }
 bool ControllerScriptEngineLegacy::callInitFunction() {
     // m_pController is nullptr in tests.
-    const auto controllerName = m_pController ? m_pController->getName() : QString{};
     const auto args = QJSValueList{
-            controllerName,
+            m_pController ? m_pController->getName() : QString{},
             m_logger().isDebugEnabled(),
     };
 
@@ -160,10 +159,10 @@ bool ControllerScriptEngineLegacy::callInitFunction() {
         return callFunctionOnObjects(m_scriptFunctionPrefixes, "init", args, true);
 #ifdef MIXXX_USE_QML
     } else {
-        QHashIterator<QString, mixxx::qml::QmlMixxxController*> i(m_rootItems);
+        QHashIterator<QString, mixxx::qml::QmlMixxxControllerScreen*> i(m_rootItems);
         while (i.hasNext()) {
             i.next();
-            auto& screen = i.value();
+            const auto& screen = i.value();
             const QString& screenIdentifier = i.key();
 
             if (!screen->getInit().isCallable()) {
@@ -481,7 +480,7 @@ bool ControllerScriptEngineLegacy::bindSceneToScreen(
     // evaluating it.
     watchFilePath(qmlFile.file.absoluteFilePath());
 
-    auto pScene = loadQMLFile(qmlFile, pScreen);
+    auto* pScene = loadQMLFile(qmlFile, pScreen);
     if (!pScene) {
         VERIFY_OR_DEBUG_ASSERT(!pScreen->isValid() ||
                 !pScreen->isRunning() || pScreen->stop()) {
@@ -520,7 +519,7 @@ void ControllerScriptEngineLegacy::handleScreenFrame(
         return;
     };
 
-    auto screen = m_rootItems.value(screenInfo.identifier);
+    auto* pScreen = m_rootItems.value(screenInfo.identifier);
 
     if (CmdlineArgs::Instance().getControllerPreviewScreens()) {
         QImage screenDebug(frame);
@@ -550,12 +549,12 @@ void ControllerScriptEngineLegacy::handleScreenFrame(
     // compilers that don't support it (e.g. older than Xcode 14.3/macOS 13)
     QByteArray input(reinterpret_cast<const char*>(frame.constBits()), frame.sizeInBytes());
 
-    if (!screen->getTransform().isCallable() && screenInfo.rawData) {
+    if (!pScreen->getTransform().isCallable() && screenInfo.rawData) {
         m_renderingScreens[screenInfo.identifier]->requestSendingFrameData(m_pController, input);
         return;
     }
 
-    if (!screen->getTransform().isCallable()) {
+    if (!pScreen->getTransform().isCallable()) {
         qCWarning(m_logger)
                 << "Could not find a valid transform function but the screen "
                    "doesn't accept raw data. Aborting screen rendering.";
@@ -569,7 +568,7 @@ void ControllerScriptEngineLegacy::handleScreenFrame(
     }
     // During the frame transformation, any QML errors are considered fatal.
     setErrorsAreFatal(true);
-    auto result = screen->getTransform().call(
+    auto result = pScreen->getTransform().call(
             QJSValueList{m_pJSEngine->toScriptValue(input),
                     m_pJSEngine->toScriptValue(timestamp)});
     if (result.isError()) {
@@ -743,7 +742,7 @@ bool ControllerScriptEngineLegacy::evaluateScriptFile(const QFileInfo& scriptFil
 }
 
 #ifdef MIXXX_USE_QML
-mixxx::qml::QmlMixxxController* ControllerScriptEngineLegacy::loadQMLFile(
+mixxx::qml::QmlMixxxControllerScreen* ControllerScriptEngineLegacy::loadQMLFile(
         const LegacyControllerMapping::ScriptFileInfo& qmlScript,
         std::shared_ptr<ControllerRenderingEngine> pScreen) {
     VERIFY_OR_DEBUG_ASSERT(m_pJSEngine ||
@@ -804,8 +803,8 @@ mixxx::qml::QmlMixxxController* ControllerScriptEngineLegacy::loadQMLFile(
         return nullptr;
     }
 
-    mixxx::qml::QmlMixxxController* rootItem =
-            qobject_cast<mixxx::qml::QmlMixxxController*>(pRootObject);
+    mixxx::qml::QmlMixxxControllerScreen* rootItem =
+            qobject_cast<mixxx::qml::QmlMixxxControllerScreen*>(pRootObject);
     if (!rootItem) {
         qWarning("run: Not a QQuickItem");
         delete pRootObject;

src/controllers/scripting/legacy/controllerscriptenginelegacy.h

@@ -16,7 +16,7 @@ class QQuickItem;
 class ControllerRenderingEngine;
 namespace mixxx {
 namespace qml {
-class QmlMixxxController;
+class QmlMixxxControllerScreen;
 }
 } // namespace mixxx
 #endif
@@ -90,7 +90,7 @@ class ControllerScriptEngineLegacy : public ControllerScriptEngineBase {
     void extractTransformFunction(const QMetaObject* metaObject, const QString& screenIdentifier);
 
     // The returned QmlMixxxController will be owned and managed by the pScreen
-    mixxx::qml::QmlMixxxController* loadQMLFile(
+    mixxx::qml::QmlMixxxControllerScreen* loadQMLFile(
             const LegacyControllerMapping::ScriptFileInfo& qmlScript,
             std::shared_ptr<ControllerRenderingEngine> pScreen);
 
@@ -120,8 +120,9 @@ class ControllerScriptEngineLegacy : public ControllerScriptEngineBase {
     QHash<QString, std::shared_ptr<ControllerRenderingEngine>> m_renderingScreens;
     // Contains all the scenes loaded for this mapping. Key is the scene
     // identifier (LegacyControllerMapping::ScreenInfo::identifier), value in
-    // the QML root item
-    QHash<QString, mixxx::qml::QmlMixxxController*> m_rootItems;
+    // the QML root item. Note that the pointer is owned by the QML scene which
+    // will free them on shutdown (ControllerScriptEngineLegacy::shutdown)
+    QHash<QString, mixxx::qml::QmlMixxxControllerScreen*> m_rootItems;
     QList<LegacyControllerMapping::QMLModuleInfo> m_modules;
     QList<LegacyControllerMapping::ScreenInfo> m_infoScreens;
     QString m_resourcePath{QStringLiteral(".")};

src/controllers/scripting/legacy/controllerscriptinterfacelegacy.cpp

@@ -524,7 +524,9 @@ int ControllerScriptInterfaceLegacy::beginTimer(
     m_timers[timerId] = info;
     if (timerId == 0) {
         m_pScriptEngineLegacy->logOrThrowError(QStringLiteral("Script timer could not be created"));
-    } else if (oneShot && CmdlineArgs::Instance().getControllerDebug()) {
+    } else if (oneShot &&
+            // FIXME workaround log spam (Github issue to be created)
+            CmdlineArgs::Instance().getControllerDebug()) {
         qCDebug(m_logger) << "Starting one-shot timer:" << timerId;
     } else {
         qCDebug(m_logger) << "Starting timer:" << timerId;

src/qml/qmlmixxxcontroller.cpp

@@ -8,27 +8,27 @@
 namespace mixxx {
 namespace qml {
 
-QmlMixxxController::QmlMixxxController(QQuickItem* parent)
+QmlMixxxControllerScreen::QmlMixxxControllerScreen(QQuickItem* parent)
         : QQuickItem(parent) {
 }
 
-void QmlMixxxController::setTransform(const QJSValue& value) {
+void QmlMixxxControllerScreen::setTransform(const QJSValue& value) {
     if (!value.isCallable()) {
         return;
     }
     m_transformFunc = value;
     emit transformChanged();
 }
 
-void QmlMixxxController::setInit(const QJSValue& value) {
+void QmlMixxxControllerScreen::setInit(const QJSValue& value) {
     if (!value.isCallable()) {
         return;
     }
     m_initFunc = value;
     emit initChanged();
 }
 
-void QmlMixxxController::setShutdown(const QJSValue& value) {
+void QmlMixxxControllerScreen::setShutdown(const QJSValue& value) {
     if (!value.isCallable()) {
         return;
     }

src/qml/qmlmixxxcontroller.h

@@ -7,7 +7,7 @@
 namespace mixxx {
 namespace qml {
 
-class QmlMixxxController : public QQuickItem {
+class QmlMixxxControllerScreen : public QQuickItem {
     Q_OBJECT
     QML_NAMED_ELEMENT(Controller)
     Q_PROPERTY(QJSValue init READ getInit WRITE setInit
@@ -18,7 +18,7 @@ class QmlMixxxController : public QQuickItem {
                     NOTIFY transformChanged REQUIRED);
 
   public:
-    explicit QmlMixxxController(QQuickItem* parent = nullptr);
+    explicit QmlMixxxControllerScreen(QQuickItem* parent = nullptr);
 
     void setInit(const QJSValue& value);
     void setShutdown(const QJSValue& value);

src/test/controllerscriptenginelegacy_test.cpp

@@ -56,6 +56,12 @@ class ControllerScriptEngineLegacyTest : public ControllerScriptEngineLegacy, pu
 
     void TearDown() override {
         mixxx::Time::setTestMode(false);
+#ifdef MIXXX_USE_QML
+        for (auto& item : m_rootItems) {
+            delete item;
+        }
+        m_rootItems.clear();
+#endif
     }
 
     bool evaluateScriptFile(const QFileInfo& scriptFile) {
@@ -86,7 +92,7 @@ class ControllerScriptEngineLegacyTest : public ControllerScriptEngineLegacy, pu
         return m_renderingScreens;
     }
 
-    QHash<QString, mixxx::qml::QmlMixxxController*>& rootItems() {
+    QHash<QString, mixxx::qml::QmlMixxxControllerScreen*>& rootItems() {
         return m_rootItems;
     }
 
@@ -686,7 +692,7 @@ TEST_F(ControllerScriptEngineLegacyTest, screenWontSentRawDataIfNotConfigured) {
             "accept raw data. Aborting screen rendering.");
 
     renderingScreens().insert(dummyScreen.identifier, pDummyRender);
-    rootItems().insert(dummyScreen.identifier, new mixxx::qml::QmlMixxxController());
+    rootItems().insert(dummyScreen.identifier, new mixxx::qml::QmlMixxxControllerScreen());
 
     testHandleScreen(
             dummyScreen,
@@ -717,7 +723,7 @@ TEST_F(ControllerScriptEngineLegacyTest, screenWillSentRawDataIfConfigured) {
     EXPECT_CALL(*pDummyRender, requestSendingFrameData(_, QByteArray()));
 
     renderingScreens().insert(dummyScreen.identifier, pDummyRender);
-    rootItems().insert(dummyScreen.identifier, new mixxx::qml::QmlMixxxController());
+    rootItems().insert(dummyScreen.identifier, new mixxx::qml::QmlMixxxControllerScreen());
 
     testHandleScreen(
             dummyScreen,