fixup! feat: improve screen rendering framework

acolombier · Oct 20, 2024 · 9b2aafa · 9b2aafa
1 parent 99fed94
commit 9b2aafa
Show file tree

Hide file tree

Showing 4 changed files with 21 additions and 32 deletions.
diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -2801,8 +2801,6 @@ if(QML)
   # and :/mixxx.org/imports/Mixxx/Controls are placed into beginning of the binary
   qt_finalize_target(mixxx)
 
-  # Required for src/qml/qmlmixxxcontrollerscreen.h, used in controllerscriptenginelegacy_test.cpp
-  target_link_libraries(mixxx-test PRIVATE Qt6::Quick)
 
   install(
     DIRECTORY

diff --git a/src/controllers/scripting/legacy/controllerscriptenginelegacy.cpp b/src/controllers/scripting/legacy/controllerscriptenginelegacy.cpp
@@ -1,5 +1,7 @@
 #include "controllers/scripting/legacy/controllerscriptenginelegacy.h"
 
+#include <memory>
+
 #ifdef MIXXX_USE_QML
 #include <QDirIterator>
 #include <QQmlEngine>
@@ -114,13 +116,8 @@ bool ControllerScriptEngineLegacy::callShutdownFunction() {
         return callFunctionOnObjects(m_scriptFunctionPrefixes, "shutdown");
 #ifdef MIXXX_USE_QML
     } else {
-        QHashIterator<QString, mixxx::qml::QmlMixxxControllerScreen*> i(m_rootItems);
         bool success = true;
-        while (i.hasNext()) {
-            i.next();
-            const auto& screen = i.value();
-            const QString& screenIdentifier = i.key();
-
+        for (const auto& [screenIdentifier, screen] : m_rootItems) {
             if (!screen->getShutdown().isCallable()) {
                 qCDebug(m_logger) << "QML Scene for screen" << screenIdentifier
                                   << "has no valid shutdown method.";
@@ -159,12 +156,7 @@ bool ControllerScriptEngineLegacy::callInitFunction() {
         return callFunctionOnObjects(m_scriptFunctionPrefixes, "init", args, true);
 #ifdef MIXXX_USE_QML
     } else {
-        QHashIterator<QString, mixxx::qml::QmlMixxxControllerScreen*> i(m_rootItems);
-        while (i.hasNext()) {
-            i.next();
-            const auto& screen = i.value();
-            const QString& screenIdentifier = i.key();
-
+        for (const auto& [screenIdentifier, screen] : m_rootItems) {
             if (!screen->getInit().isCallable()) {
                 qCDebug(m_logger) << "QML Scene for screen" << screenIdentifier
                                   << "has no valid init method.";
@@ -480,7 +472,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()) {
@@ -494,7 +486,7 @@ bool ControllerScriptEngineLegacy::bindSceneToScreen(
             this,
             &ControllerScriptEngineLegacy::handleScreenFrame);
     m_renderingScreens.insert(screenIdentifier, pScreen);
-    m_rootItems.insert(screenIdentifier, pScene);
+    m_rootItems.emplace(screenIdentifier, std::move(pScene));
     // In case a rendering issue occurs, we need to shutdown the controller
     // since its only purpose is to render screens. This might not be the case
     // in the future controller modules
@@ -519,7 +511,7 @@ void ControllerScriptEngineLegacy::handleScreenFrame(
         return;
     };
 
-    auto* pScreen = m_rootItems.value(screenInfo.identifier);
+    auto& pScreen = m_rootItems.at(screenInfo.identifier);
 
     if (CmdlineArgs::Instance().getControllerPreviewScreens()) {
         QImage screenDebug(frame);
@@ -742,7 +734,7 @@ bool ControllerScriptEngineLegacy::evaluateScriptFile(const QFileInfo& scriptFil
 }
 
 #ifdef MIXXX_USE_QML
-mixxx::qml::QmlMixxxControllerScreen* ControllerScriptEngineLegacy::loadQMLFile(
+std::unique_ptr<mixxx::qml::QmlMixxxControllerScreen> ControllerScriptEngineLegacy::loadQMLFile(
         const LegacyControllerMapping::ScriptFileInfo& qmlScript,
         std::shared_ptr<ControllerRenderingEngine> pScreen) {
     VERIFY_OR_DEBUG_ASSERT(m_pJSEngine ||
@@ -806,7 +798,7 @@ mixxx::qml::QmlMixxxControllerScreen* ControllerScriptEngineLegacy::loadQMLFile(
     mixxx::qml::QmlMixxxControllerScreen* rootItem =
             qobject_cast<mixxx::qml::QmlMixxxControllerScreen*>(pRootObject);
     if (!rootItem) {
-        qWarning("run: Not a QQuickItem");
+        qWarning("run: Not a MixxxControllerScreen");
         delete pRootObject;
         return nullptr;
     }
@@ -820,7 +812,7 @@ mixxx::qml::QmlMixxxControllerScreen* ControllerScriptEngineLegacy::loadQMLFile(
         rootItem->setHeight(pScreen->quickWindow()->height());
     }
 
-    return rootItem;
+    return std::unique_ptr<mixxx::qml::QmlMixxxControllerScreen>(rootItem);
 }
 #endif
 

diff --git a/src/controllers/scripting/legacy/controllerscriptenginelegacy.h b/src/controllers/scripting/legacy/controllerscriptenginelegacy.h
@@ -4,8 +4,10 @@
 #include <QJSEngine>
 #include <QJSValue>
 #include <QMessageBox>
+#include <memory>
 #ifdef MIXXX_USE_QML
 #include <QMetaMethod>
+#include <unordered_map>
 #endif
 
 #include "controllers/legacycontrollermapping.h"
@@ -17,7 +19,7 @@ class ControllerRenderingEngine;
 namespace mixxx {
 namespace qml {
 class QmlMixxxControllerScreen;
-}
+} // namespace qml
 } // namespace mixxx
 #endif
 
@@ -89,8 +91,7 @@ class ControllerScriptEngineLegacy : public ControllerScriptEngineBase {
             std::shared_ptr<ControllerRenderingEngine> pScreen);
     void extractTransformFunction(const QMetaObject* metaObject, const QString& screenIdentifier);
 
-    // The returned QmlMixxxController will be owned and managed by the pScreen
-    mixxx::qml::QmlMixxxControllerScreen* loadQMLFile(
+    std::unique_ptr<mixxx::qml::QmlMixxxControllerScreen> loadQMLFile(
             const LegacyControllerMapping::ScriptFileInfo& qmlScript,
             std::shared_ptr<ControllerRenderingEngine> pScreen);
 
@@ -120,9 +121,8 @@ 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. 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;
+    // the QML root item.
+    std::unordered_map<QString, std::unique_ptr<mixxx::qml::QmlMixxxControllerScreen>> m_rootItems;
     QList<LegacyControllerMapping::QMLModuleInfo> m_modules;
     QList<LegacyControllerMapping::ScreenInfo> m_infoScreens;
     QString m_resourcePath{QStringLiteral(".")};

diff --git a/src/test/controllerscriptenginelegacy_test.cpp b/src/test/controllerscriptenginelegacy_test.cpp
@@ -21,7 +21,9 @@
 #include "controllers/softtakeover.h"
 #include "helpers/log_test.h"
 #include "preferences/usersettings.h"
+#ifdef MIXXX_USE_QML
 #include "qml/qmlmixxxcontrollerscreen.h"
+#endif
 #include "test/mixxxtest.h"
 #include "util/color/colorpalette.h"
 #include "util/time.h"
@@ -57,9 +59,6 @@ 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
     }
@@ -92,7 +91,7 @@ class ControllerScriptEngineLegacyTest : public ControllerScriptEngineLegacy, pu
         return m_renderingScreens;
     }
 
-    QHash<QString, mixxx::qml::QmlMixxxControllerScreen*>& rootItems() {
+    std::unordered_map<QString, std::unique_ptr<mixxx::qml::QmlMixxxControllerScreen>>& rootItems() {
         return m_rootItems;
     }
 
@@ -692,7 +691,7 @@ TEST_F(ControllerScriptEngineLegacyTest, screenWontSentRawDataIfNotConfigured) {
             "accept raw data. Aborting screen rendering.");
 
     renderingScreens().insert(dummyScreen.identifier, pDummyRender);
-    rootItems().insert(dummyScreen.identifier, new mixxx::qml::QmlMixxxControllerScreen());
+    rootItems().emplace(dummyScreen.identifier, std::make_unique<mixxx::qml::QmlMixxxControllerScreen>());
 
     testHandleScreen(
             dummyScreen,
@@ -723,7 +722,7 @@ TEST_F(ControllerScriptEngineLegacyTest, screenWillSentRawDataIfConfigured) {
     EXPECT_CALL(*pDummyRender, requestSendingFrameData(_, QByteArray()));
 
     renderingScreens().insert(dummyScreen.identifier, pDummyRender);
-    rootItems().insert(dummyScreen.identifier, new mixxx::qml::QmlMixxxControllerScreen());
+    rootItems().emplace(dummyScreen.identifier, std::make_unique<mixxx::qml::QmlMixxxControllerScreen>());
 
     testHandleScreen(
             dummyScreen,