refactor: move unique_ptr::get() to improve usability

This ensures that if a connection is a DisplayConnection then it is also added to the other connections. This improves memory safety as well as usability because it makes it impossible to accidentally set a connection as DisplayConnection that is not tracked by the WBaseWidget.
mixxxdj · Nov 9, 2024 · afb744c · afb744c
1 parent 78a7cc4
commit afb744c
Show file tree

Hide file tree

Showing 3 changed files with 71 additions and 55 deletions.
diff --git a/src/skin/legacy/legacyskinparser.cpp b/src/skin/legacy/legacyskinparser.cpp
@@ -1,5 +1,7 @@
 #include "skin/legacy/legacyskinparser.h"
 
+#include <qnamespace.h>
+
 #include <QDir>
 #include <QGridLayout>
 #include <QLabel>
@@ -25,6 +27,7 @@
 #include "skin/legacy/launchimage.h"
 #include "skin/legacy/skincontext.h"
 #include "track/track.h"
+#include "util/assert.h"
 #include "util/cmdlineargs.h"
 #include "util/timer.h"
 #include "util/valuetransformer.h"
@@ -96,7 +99,25 @@ using mixxx::skin::SkinManifest;
 /// of QString instead of every widget keeping its own copy.
 QSet<QString> LegacySkinParser::s_sharedGroupStrings;
 
-static bool sDebug = false;
+namespace {
+bool sDebug = false;
+
+WBaseWidget::ConnectionSide sideFromMouseButton(Qt::MouseButton btn) {
+    using enum WBaseWidget::ConnectionSide;
+    switch (btn) {
+    case Qt::NoButton:
+        return None;
+    case Qt::LeftButton:
+        return Left;
+    case Qt::RightButton:
+        return Right;
+    default:
+        DEBUG_ASSERT(false);
+        return None;
+    }
+}
+
+} // namespace
 
 ControlObject* LegacySkinParser::controlFromConfigKey(
         const ConfigKey& key, bool bPersist, bool* pCreated) {
@@ -2439,31 +2460,21 @@ void LegacySkinParser::setupConnections(const QDomNode& node, WBaseWidget* pWidg
                             static_cast<ControlParameterWidgetConnection::
                                             EmitOption>(emitOption));
 
-            switch (state) {
-            case Qt::NoButton:
-                if (directionOption & ControlParameterWidgetConnection::DIR_TO_WIDGET) {
-                    pLastLeftOrNoButtonConnection = pConnection.get();
-                }
-                pWidget->addConnection(std::move(pConnection));
-                break;
-            case Qt::LeftButton:
-                if (directionOption & ControlParameterWidgetConnection::DIR_TO_WIDGET) {
-                    pLastLeftOrNoButtonConnection = pConnection.get();
-                }
-                pWidget->addLeftConnection(std::move(pConnection));
-                break;
-            case Qt::RightButton:
-                pWidget->addRightConnection(std::move(pConnection));
-                break;
-            default:
-                // can't happen. Nothing else is returned by parseButtonState();
-                DEBUG_ASSERT(false);
-                break;
-            }
-
             // We only add info for controls that this widget affects, not
             // controls that only affect the widget.
-            if (directionOption & ControlParameterWidgetConnection::DIR_FROM_WIDGET) {
+            WBaseWidget::ConnectionSide side = sideFromMouseButton(state);
+            if (!(directionOption & ControlParameterWidgetConnection::DIR_FROM_WIDGET)) {
+                pWidget->addConnection(std::move(pConnection), side);
+            } else {
+                // Legacy behavior: The last left-button or no-button connection with
+                // connectValueToWidget is the display connection. If no left-button or
+                // no-button connection exists, use the last right-button connection as the
+                // display connection.
+                if (side == WBaseWidget::ConnectionSide::None ||
+                        side == WBaseWidget::ConnectionSide::Left) {
+                    pWidget->setDisplayConnection(std::move(pConnection),
+                            WBaseWidget::ConnectionSide::Left);
+                }
                 m_pControllerManager->getControllerLearningEventFilter()
                         ->addWidgetClickInfo(pWidget->toQWidget(), state, control,
                                 static_cast<ControlParameterWidgetConnection::EmitOption>(emitOption));
@@ -2543,14 +2554,6 @@ void LegacySkinParser::setupConnections(const QDomNode& node, WBaseWidget* pWidg
             }
         }
     }
-
-    // Legacy behavior: The last left-button or no-button connection with
-    // connectValueToWidget is the display connection. If no left-button or
-    // no-button connection exists, use the last right-button connection as the
-    // display connection.
-    if (pLastLeftOrNoButtonConnection != nullptr) {
-        pWidget->setDisplayConnection(pLastLeftOrNoButtonConnection);
-    }
 }
 
 void LegacySkinParser::addShortcutToToolTip(WBaseWidget* pWidget,

diff --git a/src/widget/wbasewidget.cpp b/src/widget/wbasewidget.cpp
@@ -17,22 +17,27 @@ void WBaseWidget::Init() {
     }
 }
 
-void WBaseWidget::setDisplayConnection(ControlParameterWidgetConnection* pConnection) {
-    //qDebug() << "WBaseWidget::setDisplayConnection()" << pConnection->toDebugString();
-    m_pDisplayConnection = pConnection;
-}
-
-void WBaseWidget::addConnection(std::unique_ptr<ControlParameterWidgetConnection> pConnection) {
-    m_connections.push_back(std::move(pConnection));
-}
-
-void WBaseWidget::addLeftConnection(std::unique_ptr<ControlParameterWidgetConnection> pConnection) {
-    m_leftConnections.push_back(std::move(pConnection));
-}
-
-void WBaseWidget::addRightConnection(
-        std::unique_ptr<ControlParameterWidgetConnection> pConnection) {
-    m_rightConnections.push_back(std::move(pConnection));
+void WBaseWidget::addConnection(
+        std::unique_ptr<ControlParameterWidgetConnection> pConnection,
+        ConnectionSide side) {
+    switch (side) {
+    case ConnectionSide::None:
+        m_connections.push_back(std::move(pConnection));
+        break;
+    case ConnectionSide::Left:
+        m_leftConnections.push_back(std::move(pConnection));
+        break;
+    case ConnectionSide::Right:
+        m_rightConnections.push_back(std::move(pConnection));
+        break;
+    }
+}
+
+void WBaseWidget::setDisplayConnection(
+        std::unique_ptr<ControlParameterWidgetConnection> pConnection,
+        ConnectionSide side) {
+    m_pDisplayConnection = pConnection.get();
+    addConnection(std::move(pConnection), side);
 }
 
 void WBaseWidget::addPropertyConnection(

diff --git a/src/widget/wbasewidget.h b/src/widget/wbasewidget.h
@@ -3,6 +3,7 @@
 #include <QString>
 #include <QWidget>
 #include <memory>
+#include <utility>
 #include <vector>
 
 class ControlWidgetPropertyConnection;
@@ -13,6 +14,12 @@ class WBaseWidget {
     explicit WBaseWidget(QWidget* pWidget);
     virtual ~WBaseWidget();
 
+    enum class ConnectionSide {
+        None,
+        Left,
+        Right
+    };
+
     virtual void Init();
 
     QWidget* toQWidget() {
@@ -38,16 +45,17 @@ class WBaseWidget {
         return m_baseTooltip;
     }
 
-    void addLeftConnection(std::unique_ptr<ControlParameterWidgetConnection> pConnection);
-    void addRightConnection(std::unique_ptr<ControlParameterWidgetConnection> pConnection);
-    void addConnection(std::unique_ptr<ControlParameterWidgetConnection> pConnection);
+    void addConnection(
+            std::unique_ptr<ControlParameterWidgetConnection> pConnection,
+            ConnectionSide side);
 
     void addPropertyConnection(std::unique_ptr<ControlWidgetPropertyConnection> pConnection);
 
-    // Set a ControlWidgetConnection to be the display connection for the
-    // widget. The connection should also be added via an addConnection method
-    // or it will not be deleted or receive updates.
-    void setDisplayConnection(ControlParameterWidgetConnection* pConnection);
+    // Add a ControlWidgetConnection to be the display connection for the
+    // widget.
+    void setDisplayConnection(
+            std::unique_ptr<ControlParameterWidgetConnection> pConnection,
+            ConnectionSide side);
 
     double getControlParameter() const;
     double getControlParameterLeft() const;