remove ConstLegacyControllerMappingVisitor

This class increased coupling and made the code very confusing.

src/controllers/bulk/bulkcontroller.cpp

@@ -98,16 +98,10 @@ QString BulkController::mappingExtension() {
     return BULK_MAPPING_EXTENSION;
 }
 
-void BulkController::visit(const LegacyMidiControllerMapping* mapping) {
-    Q_UNUSED(mapping);
-    // TODO(XXX): throw a hissy fit.
-    qWarning() << "ERROR: Attempting to load a LegacyMidiControllerMapping to an HidController!";
-}
-
-void BulkController::visit(const LegacyHidControllerMapping* mapping) {
-    m_mapping = *mapping;
-    // Emit mappingLoaded with a clone of the mapping.
-    emit mappingLoaded(getMapping());
+void BulkController::setMapping(LegacyControllerMapping* pMapping) {
+    auto pHidMapping = dynamic_cast<LegacyHidControllerMapping*>(pMapping);
+    DEBUG_ASSERT(pHidMapping);
+    m_mapping = *pHidMapping;
 }
 
 bool BulkController::matchMapping(const MappingInfo& mapping) {

src/controllers/bulk/bulkcontroller.h

@@ -50,8 +50,7 @@ class BulkController : public Controller {
         return LegacyControllerMappingPointer(pClone);
     }
 
-    void visit(const LegacyMidiControllerMapping* mapping) override;
-    void visit(const LegacyHidControllerMapping* mapping) override;
+    void setMapping(LegacyControllerMapping* pMapping) override;
 
     bool isMappable() const override {
         return m_mapping.isMappable();

src/controllers/controller.h

@@ -4,7 +4,6 @@
 #include <QTimerEvent>
 
 #include "controllers/controllermappinginfo.h"
-#include "controllers/controllermappingvisitor.h"
 #include "controllers/legacycontrollermapping.h"
 #include "controllers/legacycontrollermappingfilehandler.h"
 #include "controllers/scripting/legacy/controllerscriptenginelegacy.h"
@@ -15,7 +14,7 @@ class ControllerJSProxy;
 /// 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.
-class Controller : public QObject, ConstLegacyControllerMappingVisitor {
+class Controller : public QObject {
     Q_OBJECT
   public:
     explicit Controller();
@@ -31,11 +30,7 @@ class Controller : public QObject, ConstLegacyControllerMappingVisitor {
     /// the controller (type.)
     virtual QString mappingExtension() = 0;
 
-    void setMapping(const LegacyControllerMapping& mapping) {
-        // We don't know the specific type of the mapping so we need to ask
-        // the mapping to call our visitor methods with its type.
-        mapping.accept(this);
-    }
+    virtual void setMapping(LegacyControllerMapping* pMapping) = 0;
 
     // Returns a clone of the Controller's loaded mapping.
     virtual LegacyControllerMappingPointer getMapping() const = 0;

src/controllers/controllermanager.cpp

@@ -269,7 +269,8 @@ void ControllerManager::slotSetUpDevices() {
             continue;
         }
 
-        pController->setMapping(*pMapping);
+        // This runs on the main thread but LegacyControllerMapping is not thread safe, so clone it.
+        pController->setMapping(pMapping->clone());
 
         // If we are in safe mode, skip opening controllers.
         if (CmdlineArgs::Instance().getSafeMode()) {
@@ -415,12 +416,14 @@ void ControllerManager::slotApplyMapping(Controller* pController,
         qWarning() << "Mapping is dirty, changes might be lost on restart!";
     }
 
-    pController->setMapping(*pMapping);
 
     // Save the file path/name in the config so it can be auto-loaded at
     // startup next time
     m_pConfig->set(key, pMapping->filePath());
 
+    // This runs on the main thread but LegacyControllerMapping is not thread safe, so clone it.
+    pController->setMapping(pMapping->clone());
+
     if (bEnabled) {
         openController(pController);
     } else {

src/controllers/controllermappingtablemodel.h

@@ -7,7 +7,6 @@
 #include <QVariant>
 #include <QVector>
 
-#include "controllers/controllermappingvisitor.h"
 #include "controllers/hid/legacyhidcontrollermapping.h"
 #include "controllers/legacycontrollermapping.h"
 #include "controllers/midi/legacymidicontrollermapping.h"

src/controllers/hid/hidcontroller.cpp

@@ -38,16 +38,10 @@ QString HidController::mappingExtension() {
     return HID_MAPPING_EXTENSION;
 }
 
-void HidController::visit(const LegacyMidiControllerMapping* mapping) {
-    Q_UNUSED(mapping);
-    // TODO(XXX): throw a hissy fit.
-    qWarning() << "ERROR: Attempting to load a LegacyMidiControllerMapping to an HidController!";
-}
-
-void HidController::visit(const LegacyHidControllerMapping* mapping) {
-    m_mapping = *mapping;
-    // Emit mappingLoaded with a clone of the mapping.
-    emit mappingLoaded(getMapping());
+void HidController::setMapping(LegacyControllerMapping* pMapping) {
+    auto pHidMapping = dynamic_cast<LegacyHidControllerMapping*>(pMapping);
+    DEBUG_ASSERT(pHidMapping);
+    m_mapping = *pHidMapping;
 }
 
 bool HidController::matchMapping(const MappingInfo& mapping) {

src/controllers/hid/hidcontroller.h

@@ -22,8 +22,7 @@ class HidController final : public Controller {
                 new LegacyHidControllerMapping(m_mapping));
     }
 
-    void visit(const LegacyMidiControllerMapping* mapping) override;
-    void visit(const LegacyHidControllerMapping* mapping) override;
+    void setMapping(LegacyControllerMapping* pMapping) override;
 
     bool isMappable() const override {
         return m_mapping.isMappable();

src/controllers/hid/legacyhidcontrollermapping.cpp

@@ -5,7 +5,6 @@
 
 #include "controllers/hid/legacyhidcontrollermapping.h"
 
-#include "controllers/controllermappingvisitor.h"
 #include "controllers/defs_controllers.h"
 #include "controllers/hid/legacyhidcontrollermappingfilehandler.h"
 
@@ -14,12 +13,6 @@ bool LegacyHidControllerMapping::saveMapping(const QString& fileName) const {
     return handler.save(*this, fileName);
 }
 
-void LegacyHidControllerMapping::accept(ConstLegacyControllerMappingVisitor* visitor) const {
-    if (visitor) {
-        visitor->visit(this);
-    }
-}
-
 bool LegacyHidControllerMapping::isMappable() const {
     return false;
 }

src/controllers/hid/legacyhidcontrollermapping.h

@@ -1,6 +1,5 @@
 #pragma once
 
-#include "controllers/controllermappingvisitor.h"
 #include "controllers/hid/legacyhidcontrollermappingfilehandler.h"
 #include "controllers/legacycontrollermapping.h"
 
@@ -13,8 +12,11 @@ class LegacyHidControllerMapping : public LegacyControllerMapping {
     ~LegacyHidControllerMapping() override {
     }
 
+    LegacyControllerMapping* clone() const override {
+        return new LegacyHidControllerMapping(*this);
+    }
+
     bool saveMapping(const QString& fileName) const override;
 
-    void accept(ConstLegacyControllerMappingVisitor* visitor) const override;
     bool isMappable() const override;
 };

src/controllers/hid/legacyhidcontrollermappingfilehandler.cpp

@@ -1,5 +1,7 @@
 #include "controllers/hid/legacyhidcontrollermappingfilehandler.h"
 
+#include "controllers/hid/legacyhidcontrollermapping.h"
+
 bool LegacyHidControllerMappingFileHandler::save(const LegacyHidControllerMapping& mapping,
         const QString& fileName) const {
     QDomDocument doc = buildRootWithScripts(mapping);

src/controllers/hid/legacyhidcontrollermappingfilehandler.h

@@ -1,8 +1,9 @@
 #pragma once
 
-#include "controllers/hid/legacyhidcontrollermapping.h"
 #include "controllers/legacycontrollermappingfilehandler.h"
 
+class LegacyHidControllerMapping;
+
 class LegacyHidControllerMappingFileHandler : public LegacyControllerMappingFileHandler {
   public:
     LegacyHidControllerMappingFileHandler(){};

src/controllers/legacycontrollermapping.h

@@ -9,9 +9,6 @@
 
 #include "defs_urls.h"
 
-class LegacyControllerMappingVisitor;
-class ConstLegacyControllerMappingVisitor;
-
 /// This class represents a controller mapping, containing the data elements that
 /// make it up.
 class LegacyControllerMapping {
@@ -21,6 +18,8 @@ class LegacyControllerMapping {
     }
     virtual ~LegacyControllerMapping() = default;
 
+    virtual LegacyControllerMapping* clone() const = 0;
+
     struct ScriptFileInfo {
         ScriptFileInfo()
                 : builtin(false) {
@@ -172,7 +171,6 @@ class LegacyControllerMapping {
 
     virtual bool saveMapping(const QString& filename) const = 0;
 
-    virtual void accept(ConstLegacyControllerMappingVisitor* visitor) const = 0;
     virtual bool isMappable() const = 0;
 
     // Optional list of controller device match details

src/controllers/midi/legacymidicontrollermapping.cpp

@@ -8,12 +8,6 @@ bool LegacyMidiControllerMapping::saveMapping(const QString& fileName) const {
     return handler.save(*this, fileName);
 }
 
-void LegacyMidiControllerMapping::accept(ConstLegacyControllerMappingVisitor* visitor) const {
-    if (visitor) {
-        visitor->visit(this);
-    }
-}
-
 bool LegacyMidiControllerMapping::isMappable() const {
     return true;
 }

src/controllers/midi/legacymidicontrollermapping.h

@@ -2,7 +2,6 @@
 
 #include <QMultiHash>
 
-#include "controllers/controllermappingvisitor.h"
 #include "controllers/legacycontrollermapping.h"
 #include "controllers/midi/midimessage.h"
 
@@ -13,9 +12,12 @@ class LegacyMidiControllerMapping : public LegacyControllerMapping {
     LegacyMidiControllerMapping(){};
     virtual ~LegacyMidiControllerMapping(){};
 
+    virtual LegacyControllerMapping* clone() const override {
+        return new LegacyMidiControllerMapping(*this);
+    }
+
     bool saveMapping(const QString& fileName) const override;
 
-    virtual void accept(ConstLegacyControllerMappingVisitor* visitor) const override;
     virtual bool isMappable() const override;
 
     // Input mappings

src/controllers/midi/midicontroller.cpp

@@ -30,22 +30,17 @@ QString MidiController::mappingExtension() {
     return MIDI_MAPPING_EXTENSION;
 }
 
-void MidiController::visit(const LegacyMidiControllerMapping* mapping) {
-    m_mapping = *mapping;
-    emit mappingLoaded(getMapping());
+void MidiController::setMapping(LegacyControllerMapping* pMapping) {
+    auto pMidiMapping = dynamic_cast<LegacyMidiControllerMapping*>(pMapping);
+    DEBUG_ASSERT(pMidiMapping);
+    m_mapping = *pMidiMapping;
 }
 
 int MidiController::close() {
     destroyOutputHandlers();
     return 0;
 }
 
-void MidiController::visit(const LegacyHidControllerMapping* mapping) {
-    Q_UNUSED(mapping);
-    qWarning() << "ERROR: Attempting to load an LegacyHidControllerMapping to a MidiController!";
-    // TODO(XXX): throw a hissy fit.
-}
-
 bool MidiController::matchMapping(const MappingInfo& mapping) {
     // Product info mapping not implemented for MIDI devices yet
     Q_UNUSED(mapping);

src/controllers/midi/midicontroller.h

@@ -26,13 +26,10 @@ class MidiController : public Controller {
     QString mappingExtension() override;
 
     LegacyControllerMappingPointer getMapping() const override {
-        LegacyMidiControllerMapping* pClone = new LegacyMidiControllerMapping();
-        *pClone = m_mapping;
-        return LegacyControllerMappingPointer(pClone);
+        return LegacyControllerMappingPointer(m_mapping.clone());
     }
 
-    void visit(const LegacyMidiControllerMapping* mapping) override;
-    void visit(const LegacyHidControllerMapping* mapping) override;
+    void setMapping(LegacyControllerMapping* mapping) override;
 
     bool isMappable() const override {
         return m_mapping.isMappable();

src/test/controller_mapping_validation_test.cpp

@@ -81,7 +81,7 @@ bool LegacyControllerMappingValidationTest::testLoadMapping(const MappingInfo& m
 
     FakeController controller;
     controller.setDeviceName("Test Controller");
-    controller.setMapping(*pMapping);
+    controller.setMapping(pMapping.data());
     bool result = controller.applyMapping();
     controller.stopEngine();
     return result;

src/test/controller_mapping_validation_test.h

@@ -37,18 +37,23 @@ class FakeController : public Controller {
 
     LegacyControllerMappingPointer getMapping() const override;
 
-    void visit(const LegacyMidiControllerMapping* mapping) override {
-        m_bMidiMapping = true;
-        m_bHidMapping = false;
-        m_midiMapping = *mapping;
-        m_hidMapping = LegacyHidControllerMapping();
-    }
-
-    void visit(const LegacyHidControllerMapping* mapping) override {
-        m_bMidiMapping = false;
-        m_bHidMapping = true;
-        m_midiMapping = LegacyMidiControllerMapping();
-        m_hidMapping = *mapping;
+    void setMapping(LegacyControllerMapping* pMapping) override {
+        auto pMidiMapping = dynamic_cast<LegacyMidiControllerMapping*>(pMapping);
+        if (pMidiMapping) {
+            m_bMidiMapping = true;
+            m_bHidMapping = false;
+            m_midiMapping = *pMidiMapping;
+            m_hidMapping = LegacyHidControllerMapping();
+            return;
+        }
+
+        auto pHidMapping = dynamic_cast<LegacyHidControllerMapping*>(pMapping);
+        if (pHidMapping) {
+            m_bMidiMapping = false;
+            m_bHidMapping = true;
+            m_midiMapping = LegacyMidiControllerMapping();
+            m_hidMapping = *pHidMapping;
+        }
     }
 
     bool isMappable() const override;