Merge pull request #3636 from Be-ing/remove_constlegacycontrollermapp…

…ingvisitor

remove ConstLegacyControllerMappingVisitor

src/controllers/bulk/bulkcontroller.cpp

@@ -98,16 +98,15 @@ 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::setMapping(std::shared_ptr<LegacyControllerMapping> pMapping) {
+    m_pMapping = downcastAndTakeOwnership<LegacyHidControllerMapping>(std::move(pMapping));
 }
 
-void BulkController::visit(const LegacyHidControllerMapping* mapping) {
-    m_mapping = *mapping;
-    // Emit mappingLoaded with a clone of the mapping.
-    emit mappingLoaded(getMapping());
+std::shared_ptr<LegacyControllerMapping> BulkController::cloneMapping() {
+    if (!m_pMapping) {
+        return nullptr;
+    }
+    return m_pMapping->clone();
 }
 
 bool BulkController::matchMapping(const MappingInfo& mapping) {

src/controllers/bulk/bulkcontroller.h

@@ -44,17 +44,14 @@ class BulkController : public Controller {
 
     QString mappingExtension() override;
 
-    LegacyControllerMappingPointer getMapping() const override {
-        LegacyHidControllerMapping* pClone = new LegacyHidControllerMapping();
-        *pClone = m_mapping;
-        return LegacyControllerMappingPointer(pClone);
-    }
-
-    void visit(const LegacyMidiControllerMapping* mapping) override;
-    void visit(const LegacyHidControllerMapping* mapping) override;
+    virtual std::shared_ptr<LegacyControllerMapping> cloneMapping() override;
+    void setMapping(std::shared_ptr<LegacyControllerMapping> pMapping) override;
 
     bool isMappable() const override {
-        return m_mapping.isMappable();
+        if (!m_pMapping) {
+            return false;
+        }
+        return m_pMapping->isMappable();
     }
 
     bool matchMapping(const MappingInfo& mapping) override;
@@ -71,12 +68,6 @@ class BulkController : public Controller {
     // 0x0.
     void sendBytes(const QByteArray& data) override;
 
-    // Returns a pointer to the currently loaded controller mapping. For internal
-    // use only.
-    LegacyControllerMapping* mapping() override {
-        return &m_mapping;
-    }
-
     bool matchProductInfo(const ProductInfo& product);
 
     libusb_context* m_context;
@@ -93,5 +84,5 @@ class BulkController : public Controller {
 
     QString m_sUID;
     BulkReader* m_pReader;
-    LegacyHidControllerMapping m_mapping;
+    std::shared_ptr<LegacyHidControllerMapping> m_pMapping;
 };

src/controllers/controller.cpp

@@ -49,7 +49,7 @@ void Controller::stopEngine() {
 bool Controller::applyMapping() {
     qDebug() << "Applying controller mapping...";
 
-    const LegacyControllerMapping* pMapping = mapping();
+    const std::shared_ptr<LegacyControllerMapping> pMapping = cloneMapping();
 
     // Load the script code into the engine
     if (!m_pScriptEngineLegacy) {

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,14 +30,10 @@ 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);
-    }
-
-    // Returns a clone of the Controller's loaded mapping.
-    virtual LegacyControllerMappingPointer getMapping() const = 0;
+    virtual std::shared_ptr<LegacyControllerMapping> cloneMapping() = 0;
+    /// WARNING: LegacyControllerMapping is not thread safe!
+    /// Clone the mapping before passing to setMapping for use in the controller polling thread.
+    virtual void setMapping(std::shared_ptr<LegacyControllerMapping> pMapping) = 0;
 
     inline bool isOpen() const {
         return m_bIsOpen;
@@ -63,10 +58,6 @@ class Controller : public QObject, ConstLegacyControllerMappingVisitor {
     virtual bool matchMapping(const MappingInfo& mapping) = 0;
 
   signals:
-    // Emitted when a new mapping is loaded. pMapping is a /clone/ of the loaded
-    // mapping, not a pointer to the mapping itself.
-    void mappingLoaded(LegacyControllerMappingPointer pMapping);
-
     /// Emitted when the controller is opened or closed.
     void openChanged(bool bOpen);
 
@@ -88,6 +79,23 @@ class Controller : public QObject, ConstLegacyControllerMappingVisitor {
     void stopLearning();
 
   protected:
+    template<typename SpecificMappingType>
+    std::shared_ptr<SpecificMappingType> downcastAndTakeOwnership(
+            std::shared_ptr<LegacyControllerMapping>&& pMapping) {
+        // 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) {
+            return nullptr;
+        }
+        return pDowncastedMapping;
+    }
+
     // The length parameter is here for backwards compatibility for when scripts
     // were required to specify it.
     virtual void send(const QList<int>& data, unsigned int length = 0);
@@ -142,9 +150,6 @@ class Controller : public QObject, ConstLegacyControllerMappingVisitor {
     }
 
   private:
-    // Returns a pointer to the currently loaded controller mapping. For internal
-    // use only.
-    virtual LegacyControllerMapping* mapping() = 0;
     ControllerScriptEngineLegacy* m_pScriptEngineLegacy;
 
     // Verbose and unique device name suitable for display.

src/controllers/controllermanager.cpp

@@ -87,7 +87,8 @@ ControllerManager::ControllerManager(UserSettingsPointer pConfig)
           m_pControllerLearningEventFilter(new ControllerLearningEventFilter()),
           m_pollTimer(this),
           m_skipPoll(false) {
-    qRegisterMetaType<LegacyControllerMappingPointer>("LegacyControllerMappingPointer");
+    qRegisterMetaType<std::shared_ptr<LegacyControllerMapping>>(
+            "std::shared_ptr<LegacyControllerMapping>");
 
     // Create controller mapping paths in the user's home directory.
     QString userMappings = userMappingsPath(m_pConfig);
@@ -262,14 +263,16 @@ void ControllerManager::slotSetUpDevices() {
             continue;
         }
 
-        LegacyControllerMappingPointer pMapping = LegacyControllerMappingFileHandler::loadMapping(
-                mappingFile, resourceMappingsPath(m_pConfig));
+        std::shared_ptr<LegacyControllerMapping> pMapping =
+                LegacyControllerMappingFileHandler::loadMapping(
+                        mappingFile, resourceMappingsPath(m_pConfig));
 
         if (!pMapping) {
             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()) {
@@ -396,7 +399,7 @@ void ControllerManager::closeController(Controller* pController) {
 }
 
 void ControllerManager::slotApplyMapping(Controller* pController,
-        LegacyControllerMappingPointer pMapping,
+        std::shared_ptr<LegacyControllerMapping> pMapping,
         bool bEnabled) {
     VERIFY_OR_DEBUG_ASSERT(pController) {
         qWarning() << "slotApplyMapping got invalid controller!";
@@ -415,12 +418,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/controllermanager.h

@@ -51,7 +51,7 @@ class ControllerManager : public QObject {
     void updateControllerList();
 
     void slotApplyMapping(Controller* pController,
-            LegacyControllerMappingPointer pMapping,
+            std::shared_ptr<LegacyControllerMapping> pMapping,
             bool bEnabled);
     void openController(Controller* pController);
     void closeController(Controller* pController);

src/controllers/controllermappingtablemodel.cpp

@@ -11,8 +11,8 @@ ControllerMappingTableModel::~ControllerMappingTableModel() {
 
 }
 
-void ControllerMappingTableModel::setMapping(LegacyControllerMappingPointer pMapping) {
-    m_pMidiMapping = dynamic_cast<LegacyMidiControllerMapping*>(pMapping.data());
+void ControllerMappingTableModel::setMapping(std::shared_ptr<LegacyControllerMapping> pMapping) {
+    m_pMidiMapping = std::dynamic_pointer_cast<LegacyMidiControllerMapping>(pMapping);
     // Only legacy MIDI mappings are supported
     // TODO: prevent calling this code for unsupported mapping types?
     if (!m_pMidiMapping) {

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"
@@ -18,7 +17,7 @@ class ControllerMappingTableModel : public QAbstractTableModel {
     ControllerMappingTableModel(QObject* pParent);
     ~ControllerMappingTableModel() override;
 
-    void setMapping(LegacyControllerMappingPointer pMapping);
+    void setMapping(std::shared_ptr<LegacyControllerMapping> pMapping);
 
     // Revert changes made since the last apply.
     virtual void cancel();
@@ -39,5 +38,5 @@ class ControllerMappingTableModel : public QAbstractTableModel {
     virtual void onMappingLoaded() = 0;
 
     QVector<QHash<int, QVariant> > m_headerInfo;
-    LegacyMidiControllerMapping* m_pMidiMapping;
+    std::shared_ptr<LegacyMidiControllerMapping> m_pMidiMapping;
 };

src/controllers/dlgprefcontroller.cpp

@@ -46,13 +46,7 @@ DlgPrefController::DlgPrefController(
     initTableView(m_ui.m_pInputMappingTableView);
     initTableView(m_ui.m_pOutputMappingTableView);
 
-    connect(m_pController, &Controller::mappingLoaded, this, &DlgPrefController::slotShowMapping);
-    // TODO(rryan): Eh, this really isn't thread safe but it's the way it's been
-    // since 1.11.0. We shouldn't be calling Controller methods because it lives
-    // in a different thread. Booleans (like isOpen()) are fine but a complex
-    // object like a mapping involves QHash's and other data structures that
-    // really don't like concurrent access.
-    LegacyControllerMappingPointer pMapping = m_pController->getMapping();
+    std::shared_ptr<LegacyControllerMapping> pMapping = m_pController->cloneMapping();
     slotShowMapping(pMapping);
 
     m_ui.labelDeviceName->setText(m_pController->getName());
@@ -206,7 +200,7 @@ void DlgPrefController::midiInputMappingsLearned(
 }
 
 QString DlgPrefController::mappingShortName(
-        const LegacyControllerMappingPointer pMapping) const {
+        const std::shared_ptr<LegacyControllerMapping> pMapping) const {
     QString mappingName = tr("None");
     if (pMapping) {
         QString name = pMapping->name();
@@ -224,7 +218,7 @@ QString DlgPrefController::mappingShortName(
 }
 
 QString DlgPrefController::mappingName(
-        const LegacyControllerMappingPointer pMapping) const {
+        const std::shared_ptr<LegacyControllerMapping> pMapping) const {
     if (pMapping) {
         QString name = pMapping->name();
         if (name.length() > 0) {
@@ -235,7 +229,7 @@ QString DlgPrefController::mappingName(
 }
 
 QString DlgPrefController::mappingDescription(
-        const LegacyControllerMappingPointer pMapping) const {
+        const std::shared_ptr<LegacyControllerMapping> pMapping) const {
     if (pMapping) {
         QString description = pMapping->description();
         if (description.length() > 0) {
@@ -246,7 +240,7 @@ QString DlgPrefController::mappingDescription(
 }
 
 QString DlgPrefController::mappingAuthor(
-        const LegacyControllerMappingPointer pMapping) const {
+        const std::shared_ptr<LegacyControllerMapping> pMapping) const {
     if (pMapping) {
         QString author = pMapping->author();
         if (author.length() > 0) {
@@ -257,7 +251,7 @@ QString DlgPrefController::mappingAuthor(
 }
 
 QString DlgPrefController::mappingSupportLinks(
-        const LegacyControllerMappingPointer pMapping) const {
+        const std::shared_ptr<LegacyControllerMapping> pMapping) const {
     if (!pMapping) {
         return QString();
     }
@@ -299,7 +293,7 @@ QString DlgPrefController::mappingSupportLinks(
 }
 
 QString DlgPrefController::mappingFileLinks(
-        const LegacyControllerMappingPointer pMapping) const {
+        const std::shared_ptr<LegacyControllerMapping> pMapping) const {
     if (!pMapping) {
         return QString();
     }
@@ -526,8 +520,9 @@ void DlgPrefController::slotMappingSelected(int chosenIndex) {
         }
     }
 
-    LegacyControllerMappingPointer pMapping = LegacyControllerMappingFileHandler::loadMapping(
-            mappingPath, QDir(resourceMappingsPath(m_pConfig)));
+    std::shared_ptr<LegacyControllerMapping> pMapping =
+            LegacyControllerMappingFileHandler::loadMapping(
+                    mappingPath, QDir(resourceMappingsPath(m_pConfig)));
 
     if (pMapping) {
         DEBUG_ASSERT(!pMapping->isDirty());
@@ -679,21 +674,21 @@ void DlgPrefController::initTableView(QTableView* pTable) {
     pTable->setAlternatingRowColors(true);
 }
 
-void DlgPrefController::slotShowMapping(LegacyControllerMappingPointer mapping) {
-    m_ui.labelLoadedMapping->setText(mappingName(mapping));
-    m_ui.labelLoadedMappingDescription->setText(mappingDescription(mapping));
-    m_ui.labelLoadedMappingAuthor->setText(mappingAuthor(mapping));
-    m_ui.labelLoadedMappingSupportLinks->setText(mappingSupportLinks(mapping));
-    m_ui.labelLoadedMappingScriptFileLinks->setText(mappingFileLinks(mapping));
+void DlgPrefController::slotShowMapping(std::shared_ptr<LegacyControllerMapping> pMapping) {
+    m_ui.labelLoadedMapping->setText(mappingName(pMapping));
+    m_ui.labelLoadedMappingDescription->setText(mappingDescription(pMapping));
+    m_ui.labelLoadedMappingAuthor->setText(mappingAuthor(pMapping));
+    m_ui.labelLoadedMappingSupportLinks->setText(mappingSupportLinks(pMapping));
+    m_ui.labelLoadedMappingScriptFileLinks->setText(mappingFileLinks(pMapping));
 
     // We mutate this mapping so keep a reference to it while we are using it.
     // TODO(rryan): Clone it? Technically a waste since nothing else uses this
     // copy but if someone did they might not expect it to change.
-    m_pMapping = mapping;
+    m_pMapping = pMapping;
 
     ControllerInputMappingTableModel* pInputModel =
             new ControllerInputMappingTableModel(this);
-    pInputModel->setMapping(mapping);
+    pInputModel->setMapping(pMapping);
 
     QSortFilterProxyModel* pInputProxyModel = new QSortFilterProxyModel(this);
     pInputProxyModel->setSortRole(Qt::UserRole);
@@ -717,7 +712,7 @@ void DlgPrefController::slotShowMapping(LegacyControllerMappingPointer mapping)
 
     ControllerOutputMappingTableModel* pOutputModel =
             new ControllerOutputMappingTableModel(this);
-    pOutputModel->setMapping(mapping);
+    pOutputModel->setMapping(pMapping);
 
     QSortFilterProxyModel* pOutputProxyModel = new QSortFilterProxyModel(this);
     pOutputProxyModel->setSortRole(Qt::UserRole);