Refactor passing of LegacyControllerMapping

src/controllers/bulk/bulkcontroller.cpp

@@ -97,14 +97,14 @@ QString BulkController::mappingExtension() {
 }
 
 void BulkController::setMapping(std::shared_ptr<LegacyControllerMapping> pMapping) {
-    m_pMapping = downcastAndTakeOwnership<LegacyHidControllerMapping>(std::move(pMapping));
+    m_pMapping = downcastAndClone<LegacyHidControllerMapping>(pMapping.get());
 }
 
 std::shared_ptr<LegacyControllerMapping> BulkController::cloneMapping() {
     if (!m_pMapping) {
         return nullptr;
     }
-    return m_pMapping->clone();
+    return std::make_shared<LegacyHidControllerMapping>(*m_pMapping);
 }
 
 bool BulkController::matchMapping(const MappingInfo& mapping) {

src/controllers/bulk/bulkcontroller.h

@@ -84,5 +84,5 @@ class BulkController : public Controller {
 
     QString m_sUID;
     BulkReader* m_pReader;
-    std::shared_ptr<LegacyHidControllerMapping> m_pMapping;
+    std::unique_ptr<LegacyHidControllerMapping> m_pMapping;
 };

src/controllers/controller.h

@@ -79,24 +79,20 @@ class Controller : public QObject {
 
   protected:
     template<typename SpecificMappingType>
-    std::shared_ptr<SpecificMappingType> downcastAndTakeOwnership(
-            std::shared_ptr<LegacyControllerMapping>&& pMapping) {
+        requires(std::is_final_v<SpecificMappingType> == true)
+    std::unique_ptr<SpecificMappingType> downcastAndClone(const LegacyControllerMapping* pMapping) {
         // When unsetting a mapping (select 'No mapping') we receive a nullptr
-        if (pMapping == nullptr) {
+        if (!pMapping) {
             return nullptr;
         }
-        // Controller cannot take ownership if pMapping is referenced elsewhere because
-        // the controller polling thread needs exclusive accesses to the non-thread safe
-        // LegacyControllerMapping.
-        // Trying to cast a std::shared_ptr to a std::unique_ptr is not worth the trouble.
-        VERIFY_OR_DEBUG_ASSERT(pMapping.use_count() == 1) {
-            return nullptr;
-        }
-        auto pDowncastedMapping = std::dynamic_pointer_cast<SpecificMappingType>(pMapping);
-        VERIFY_OR_DEBUG_ASSERT(pDowncastedMapping) {
+        auto* pSpecifiedMapping = dynamic_cast<const SpecificMappingType*>(pMapping);
+        VERIFY_OR_DEBUG_ASSERT(pSpecifiedMapping) {
             return nullptr;
         }
-        return pDowncastedMapping;
+        // Controller cannot take ownership if pMapping is referenced elsewhere because
+        // the controller polling thread needs exclusive accesses to the non-thread safe
+        // LegacyControllerMapping. So we do a deep copy here.
+        return std::make_unique<SpecificMappingType>(*pSpecifiedMapping);
     }
 
     // The length parameter is here for backwards compatibility for when scripts

src/controllers/controllermanager.cpp

@@ -285,7 +285,7 @@ void ControllerManager::slotSetUpDevices() {
         pMapping->loadSettings(m_pConfig, pController->getName());
 
         // This runs on the main thread but LegacyControllerMapping is not thread safe, so clone it.
-        pController->setMapping(pMapping->clone());
+        pController->setMapping(std::move(pMapping));
 
         // If we are in safe mode, skip opening controllers.
         if (CmdlineArgs::Instance().getSafeMode()) {
@@ -439,7 +439,7 @@ void ControllerManager::slotApplyMapping(Controller* pController,
     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());
+    pController->setMapping(std::move(pMapping));
 
     if (bEnabled) {
         openController(pController);

src/controllers/hid/hidcontroller.cpp

@@ -29,14 +29,14 @@ QString HidController::mappingExtension() {
 }
 
 void HidController::setMapping(std::shared_ptr<LegacyControllerMapping> pMapping) {
-    m_pMapping = downcastAndTakeOwnership<LegacyHidControllerMapping>(std::move(pMapping));
+    m_pMapping = downcastAndClone<LegacyHidControllerMapping>(pMapping.get());
 }
 
 std::shared_ptr<LegacyControllerMapping> HidController::cloneMapping() {
     if (!m_pMapping) {
         return nullptr;
     }
-    return m_pMapping->clone();
+    return std::make_shared<LegacyHidControllerMapping>(*m_pMapping);
 }
 
 bool HidController::matchMapping(const MappingInfo& mapping) {

src/controllers/hid/hidcontroller.h

@@ -41,7 +41,7 @@ class HidController final : public Controller {
     const mixxx::hid::DeviceInfo m_deviceInfo;
 
     std::unique_ptr<HidIoThread> m_pHidIoThread;
-    std::shared_ptr<LegacyHidControllerMapping> m_pMapping;
+    std::unique_ptr<LegacyHidControllerMapping> m_pMapping;
 
     friend class HidControllerJSProxy;
 };

src/controllers/hid/legacyhidcontrollermapping.h

@@ -4,17 +4,13 @@
 
 /// This class represents a HID or Bulk controller mapping, containing the data
 /// elements that make it up.
-class LegacyHidControllerMapping : public LegacyControllerMapping {
+class LegacyHidControllerMapping final : public LegacyControllerMapping {
   public:
     LegacyHidControllerMapping() {
     }
     ~LegacyHidControllerMapping() override {
     }
 
-    std::shared_ptr<LegacyControllerMapping> clone() const override {
-        return std::make_shared<LegacyHidControllerMapping>(*this);
-    }
-
     bool saveMapping(const QString& fileName) const override;
 
     bool isMappable() const override;

src/controllers/legacycontrollermapping.h

@@ -13,7 +13,7 @@
 /// This class represents a controller mapping, containing the data elements that
 /// make it up.
 class LegacyControllerMapping {
-  public:
+  protected:
     LegacyControllerMapping()
             : m_bDirty(false),
               m_deviceDirection(DeviceDirection::Bidirectionnal) {
@@ -38,10 +38,12 @@ class LegacyControllerMapping {
               m_scripts(other.m_scripts),
               m_deviceDirection(other.m_deviceDirection) {
     }
+    LegacyControllerMapping& operator=(const LegacyControllerMapping&) = delete;
+    LegacyControllerMapping(LegacyControllerMapping&&) = delete;
+    LegacyControllerMapping& operator=(LegacyControllerMapping&&) = delete;
     virtual ~LegacyControllerMapping() = default;
 
-    virtual std::shared_ptr<LegacyControllerMapping> clone() const = 0;
-
+  public:
     struct ScriptFileInfo {
         ScriptFileInfo()
                 : builtin(false) {

src/controllers/midi/legacymidicontrollermapping.h

@@ -7,15 +7,11 @@
 
 /// Represents a MIDI controller mapping, containing the data elements that make
 /// it up.
-class LegacyMidiControllerMapping : public LegacyControllerMapping {
+class LegacyMidiControllerMapping final : public LegacyControllerMapping {
   public:
     LegacyMidiControllerMapping(){};
     virtual ~LegacyMidiControllerMapping(){};
 
-    std::shared_ptr<LegacyControllerMapping> clone() const override {
-        return std::make_shared<LegacyMidiControllerMapping>(*this);
-    }
-
     bool saveMapping(const QString& fileName) const override;
 
     virtual bool isMappable() const override;

src/controllers/midi/midicontroller.cpp

@@ -20,14 +20,15 @@ const QString kMakeInputHandlerError = QStringLiteral(
         "Please pass a function and make sure that your code contains no syntax errors.");
 
 MidiInputHandleJSProxy::MidiInputHandleJSProxy(
-        const std::shared_ptr<LegacyMidiControllerMapping> mapping,
+        MidiController* pMidiController,
         const MidiInputMapping& inputMapping)
-        : m_mapping(mapping), m_inputMapping(inputMapping) {
+        : m_pMidiController(pMidiController),
+          m_inputMapping(inputMapping) {
 }
 
 bool MidiInputHandleJSProxy::disconnect() {
     // We want to remove only this mapping when disconnecting
-    return m_mapping->removeInputMapping(m_inputMapping.key.key, m_inputMapping);
+    return m_pMidiController->removeInputMapping(m_inputMapping.key.key, m_inputMapping);
 }
 
 MidiController::MidiController(const QString& deviceName)
@@ -55,14 +56,14 @@ QString MidiController::mappingExtension() {
 }
 
 void MidiController::setMapping(std::shared_ptr<LegacyControllerMapping> pMapping) {
-    m_pMapping = downcastAndTakeOwnership<LegacyMidiControllerMapping>(std::move(pMapping));
+    m_pMapping = downcastAndClone<LegacyMidiControllerMapping>(pMapping.get());
 }
 
 std::shared_ptr<LegacyControllerMapping> MidiController::cloneMapping() {
     if (!m_pMapping) {
         return nullptr;
     }
-    return m_pMapping->clone();
+    return std::make_shared<LegacyMidiControllerMapping>(*m_pMapping);
 }
 
 int MidiController::close() {
@@ -655,5 +656,13 @@ QJSValue MidiController::makeInputHandler(unsigned char status,
             std::make_shared<QJSValue>(scriptCode));
 
     m_pMapping->addInputMapping(inputMapping.key.key, inputMapping);
-    return pJsEngine->newQObject(new MidiInputHandleJSProxy(m_pMapping, inputMapping));
+    // The returned object can be used for disconnecting like this:
+    // var connection = midi.makeInputHandler();
+    // connection.disconnect();
+    return pJsEngine->newQObject(new MidiInputHandleJSProxy(this, inputMapping));
+}
+
+bool MidiController::removeInputMapping(
+        uint16_t key, const MidiInputMapping& mapping) {
+    return m_pMapping->removeInputMapping(key, mapping);
 }

src/controllers/midi/midicontroller.h

@@ -9,17 +9,18 @@
 #include "controllers/softtakeover.h"
 
 class MidiOutputHandler;
+class MidiController;
 
 class MidiInputHandleJSProxy final : public QObject {
     Q_OBJECT
   public:
     MidiInputHandleJSProxy(
-            const std::shared_ptr<LegacyMidiControllerMapping> mapping,
+            MidiController* pMidiController,
             const MidiInputMapping& inputMapping);
     Q_INVOKABLE bool disconnect();
 
   protected:
-    std::shared_ptr<LegacyMidiControllerMapping> m_mapping;
+    MidiController* m_pMidiController;
     MidiInputMapping m_inputMapping;
 };
 
@@ -50,6 +51,7 @@ class MidiController : public Controller {
     }
 
     bool matchMapping(const MappingInfo& mapping) override;
+    bool removeInputMapping(uint16_t key, const MidiInputMapping& mapping);
 
   signals:
     void messageReceived(unsigned char status, unsigned char control, unsigned char value);
@@ -108,7 +110,7 @@ class MidiController : public Controller {
 
     QHash<uint16_t, MidiInputMapping> m_temporaryInputMappings;
     QList<MidiOutputHandler*> m_outputs;
-    std::shared_ptr<LegacyMidiControllerMapping> m_pMapping;
+    std::unique_ptr<LegacyMidiControllerMapping> m_pMapping;
     SoftTakeoverCtrl m_st;
     QList<QPair<MidiInputMapping, unsigned char>> m_fourteen_bit_queued_mappings;
 

src/test/controller_mapping_settings_test.cpp

@@ -396,10 +396,6 @@ class LegacyDummyMapping : public LegacyControllerMapping {
     LegacyDummyMapping() {
     }
 
-    std::shared_ptr<LegacyControllerMapping> clone() const override {
-        return std::make_shared<LegacyDummyMapping>(*this);
-    }
-
     bool saveMapping(const QString&) const override {
         return false;
     }

src/test/controller_mapping_validation_test.h

@@ -95,9 +95,9 @@ class FakeController : public Controller {
 
     virtual std::shared_ptr<LegacyControllerMapping> cloneMapping() override {
         if (m_pMidiMapping) {
-            return m_pMidiMapping->clone();
+            return std::make_shared<LegacyMidiControllerMapping>(*m_pMidiMapping);
         } else if (m_pHidMapping) {
-            return m_pHidMapping->clone();
+            return std::make_shared<LegacyHidControllerMapping>(*m_pHidMapping);
         }
         return nullptr;
     };