diff --git a/src/controllers/bulk/bulkcontroller.cpp b/src/controllers/bulk/bulkcontroller.cpp index 722d30cd04f5..97ec702462d0 100644 --- a/src/controllers/bulk/bulkcontroller.cpp +++ b/src/controllers/bulk/bulkcontroller.cpp @@ -97,14 +97,22 @@ QString BulkController::mappingExtension() { } void BulkController::setMapping(std::shared_ptr pMapping) { + m_pMutableMapping = pMapping; m_pMapping = downcastAndClone(pMapping.get()); } -std::shared_ptr BulkController::cloneMapping() { +QList BulkController::getMappingScriptFiles() { if (!m_pMapping) { - return nullptr; + return {}; } - return std::make_shared(*m_pMapping); + return m_pMapping->getScriptFiles(); +} + +QList> BulkController::getMappingSettings() { + if (!m_pMapping) { + return {}; + } + return m_pMapping->getSettings(); } bool BulkController::matchMapping(const MappingInfo& mapping) { @@ -202,7 +210,6 @@ int BulkController::open() { } #endif - setOpen(true); startEngine(); if (m_pReader != nullptr) { @@ -223,7 +230,8 @@ int BulkController::open() { // audio directly, like when scratching m_pReader->start(QThread::HighPriority); } - + applyMapping(); + setOpen(true); return 0; } diff --git a/src/controllers/bulk/bulkcontroller.h b/src/controllers/bulk/bulkcontroller.h index afca208fd1ef..019fc27c7cca 100644 --- a/src/controllers/bulk/bulkcontroller.h +++ b/src/controllers/bulk/bulkcontroller.h @@ -41,9 +41,11 @@ class BulkController : public Controller { QString mappingExtension() override; - virtual std::shared_ptr cloneMapping() override; void setMapping(std::shared_ptr pMapping) override; + QList getMappingScriptFiles() override; + QList> getMappingSettings() override; + bool isMappable() const override { if (!m_pMapping) { return false; @@ -56,11 +58,10 @@ class BulkController : public Controller { protected: void send(const QList& data, unsigned int length) override; - private slots: + private: int open() override; int close() override; - private: // For devices which only support a single report, reportID must be set to // 0x0. void sendBytes(const QByteArray& data) override; diff --git a/src/controllers/controller.cpp b/src/controllers/controller.cpp index f7bc4813a738..e40eb29a7f62 100644 --- a/src/controllers/controller.cpp +++ b/src/controllers/controller.cpp @@ -70,8 +70,7 @@ bool Controller::applyMapping() { return false; } - const std::shared_ptr pMapping = cloneMapping(); - QList scriptFiles = pMapping->getScriptFiles(); + QList scriptFiles = getMappingScriptFiles(); if (scriptFiles.isEmpty()) { qCWarning(m_logBase) << "No script functions available! Did the XML file(s) load " @@ -81,7 +80,7 @@ bool Controller::applyMapping() { m_pScriptEngineLegacy->setScriptFiles(scriptFiles); - m_pScriptEngineLegacy->setSettings(pMapping->getSettings()); + m_pScriptEngineLegacy->setSettings(getMappingSettings()); return m_pScriptEngineLegacy->initialize(); } diff --git a/src/controllers/controller.h b/src/controllers/controller.h index ec990779f521..c190fc4a007c 100644 --- a/src/controllers/controller.h +++ b/src/controllers/controller.h @@ -3,6 +3,7 @@ #include #include "controllers/controllermappinginfo.h" +#include "controllers/legacycontrollermapping.h" #include "util/duration.h" #include "util/runtimeloggingcategory.h" @@ -28,10 +29,15 @@ class Controller : public QObject { /// the controller (type.) virtual QString mappingExtension() = 0; - virtual std::shared_ptr 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 pMapping) = 0; + std::shared_ptr getMapping() { + // return the unused mutable copy of the mapping, that can be edited in the GUI thread + // and than adopted again via setMapping() + return m_pMutableMapping; + } + + virtual QList getMappingScriptFiles() = 0; + virtual QList> getMappingSettings() = 0; inline bool isOpen() const { return m_bIsOpen; @@ -69,8 +75,6 @@ class Controller : public QObject { // function that is assumed to exist. (Sub-classes may want to reimplement // this if they have an alternate way of handling such data.) virtual void receive(const QByteArray& data, mixxx::Duration timestamp); - - virtual bool applyMapping(); virtual void slotBeforeEngineShutdown(); // Puts the controller in and out of learning mode. @@ -78,6 +82,8 @@ class Controller : public QObject { void stopLearning(); protected: + virtual bool applyMapping(); + template requires(std::is_final_v == true) std::unique_ptr downcastAndClone(const LegacyControllerMapping* pMapping) { @@ -135,6 +141,7 @@ class Controller : public QObject { const RuntimeLoggingCategory m_logBase; const RuntimeLoggingCategory m_logInput; const RuntimeLoggingCategory m_logOutput; + std::shared_ptr m_pMutableMapping; private: // but used by ControllerManager diff --git a/src/controllers/controllermanager.cpp b/src/controllers/controllermanager.cpp index ecc3daa92566..37d579635ddd 100644 --- a/src/controllers/controllermanager.cpp +++ b/src/controllers/controllermanager.cpp @@ -11,6 +11,7 @@ #include "util/cmdlineargs.h" #include "util/compatibility/qmutex.h" #include "util/duration.h" +#include "util/thread_affinity.h" #include "util/time.h" #ifdef __PORTMIDI__ @@ -105,7 +106,7 @@ ControllerManager::ControllerManager(UserSettingsPointer pConfig) } m_pollTimer.setInterval(kPollInterval.toIntegerMillis()); - connect(&m_pollTimer, &QTimer::timeout, this, &ControllerManager::pollDevices); + connect(&m_pollTimer, &QTimer::timeout, this, &ControllerManager::slotPollDevices); m_pThread = new QThread; m_pThread->setObjectName("Controller"); @@ -300,7 +301,6 @@ void ControllerManager::slotSetUpDevices() { qWarning() << "There was a problem opening" << name; continue; } - pController->applyMapping(); } pollIfAnyControllersOpen(); @@ -337,7 +337,7 @@ void ControllerManager::stopPolling() { qDebug() << "Controller polling stopped."; } -void ControllerManager::pollDevices() { +void ControllerManager::slotPollDevices() { // Note: this function is called from a high priority thread which // may stall the GUI or may reduce the available CPU time for other // High Priority threads like caching reader or broadcasting more @@ -380,6 +380,7 @@ void ControllerManager::pollDevices() { } void ControllerManager::openController(Controller* pController) { + DEBUG_ASSERT_QOBJECT_THREAD_AFFINITY(this); if (!pController) { return; } @@ -392,8 +393,6 @@ void ControllerManager::openController(Controller* pController) { // If successfully opened the device, apply the mapping and save the // preference setting. if (result == 0) { - pController->applyMapping(); - // Update configuration to reflect controller is enabled. m_pConfig->setValue( ConfigKey("[Controller]", sanitizeDeviceName(pController->getName())), 1); @@ -401,6 +400,7 @@ void ControllerManager::openController(Controller* pController) { } void ControllerManager::closeController(Controller* pController) { + DEBUG_ASSERT_QOBJECT_THREAD_AFFINITY(this); if (!pController) { return; } @@ -419,9 +419,9 @@ void ControllerManager::slotApplyMapping(Controller* pController, return; } + closeController(pController); ConfigKey key(kSettingsGroup, sanitizeDeviceName(pController->getName())); if (!pMapping) { - closeController(pController); // Unset the controller mapping for this controller pController->setMapping(nullptr); m_pConfig->remove(key); @@ -438,14 +438,12 @@ void ControllerManager::slotApplyMapping(Controller* pController, // 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(std::move(pMapping)); if (bEnabled) { openController(pController); emit mappingApplied(pController->isMappable()); } else { - closeController(pController); emit mappingApplied(false); } } diff --git a/src/controllers/controllermanager.h b/src/controllers/controllermanager.h index cfa040d496a6..ded4d2998cd8 100644 --- a/src/controllers/controllermanager.h +++ b/src/controllers/controllermanager.h @@ -52,13 +52,9 @@ class ControllerManager : public QObject { void mappingApplied(bool applied); public slots: - void updateControllerList(); - void slotApplyMapping(Controller* pController, std::shared_ptr pMapping, bool bEnabled); - void openController(Controller* pController); - void closeController(Controller* pController); private slots: /// Perform initialization that should be delayed until the ControllerManager @@ -70,12 +66,16 @@ class ControllerManager : public QObject { void slotSetUpDevices(); void slotShutdown(); /// Calls poll() on all devices that have isPolling() true. - void pollDevices(); + void slotPollDevices(); + + private: + void updateControllerList(); void startPolling(); void stopPolling(); void pollIfAnyControllersOpen(); + void openController(Controller* pController); + void closeController(Controller* pController); - private: UserSettingsPointer m_pConfig; ControllerLearningEventFilter* m_pControllerLearningEventFilter; QTimer m_pollTimer; diff --git a/src/controllers/dlgprefcontroller.cpp b/src/controllers/dlgprefcontroller.cpp index 6dff258b2cfc..7a84a5cd039f 100644 --- a/src/controllers/dlgprefcontroller.cpp +++ b/src/controllers/dlgprefcontroller.cpp @@ -61,8 +61,8 @@ DlgPrefController::DlgPrefController( initTableView(m_ui.m_pInputMappingTableView); initTableView(m_ui.m_pOutputMappingTableView); - std::shared_ptr pMapping = m_pController->cloneMapping(); - slotShowMapping(pMapping); + std::shared_ptr pMapping = m_pController->getMapping(); + showMapping(pMapping); m_ui.labelDeviceName->setText(m_pController->getName()); QString category = m_pController->getCategory(); @@ -103,7 +103,11 @@ DlgPrefController::DlgPrefController( connect(this, &DlgPrefController::applyMapping, m_pControllerManager.get(), - &ControllerManager::slotApplyMapping); + &ControllerManager::slotApplyMapping, + Qt::BlockingQueuedConnection); + // Wait until the mapping has been cloned in the controller thread + // and we can continue to edit our copy + // Update GUI connect(m_pControllerManager.get(), &ControllerManager::mappingApplied, @@ -193,10 +197,10 @@ void DlgPrefController::showLearningWizard() { slotApply(); if (!m_pMapping) { - m_pMapping = std::shared_ptr(new LegacyMidiControllerMapping()); + m_pMapping = std::make_shared(); emit applyMapping(m_pController, m_pMapping, true); // shortcut for creating and assigning required I/O table models - slotShowMapping(m_pMapping); + showMapping(m_pMapping); } // Note that DlgControllerLearning is set to delete itself on close using @@ -685,7 +689,7 @@ void DlgPrefController::slotMappingSelected(int chosenIndex) { // the preset combobox. enumerateMappings(mappingFilePath); } - slotShowMapping(pMapping); + showMapping(pMapping); } bool DlgPrefController::saveMapping() { @@ -853,7 +857,7 @@ void DlgPrefController::initTableView(QTableView* pTable) { pTable->setAlternatingRowColors(true); } -void DlgPrefController::slotShowMapping(std::shared_ptr pMapping) { +void DlgPrefController::showMapping(std::shared_ptr pMapping) { m_ui.labelLoadedMapping->setText(mappingName(pMapping)); m_ui.labelLoadedMappingDescription->setText(mappingDescription(pMapping)); m_ui.labelLoadedMappingAuthor->setText(mappingAuthor(pMapping)); diff --git a/src/controllers/dlgprefcontroller.h b/src/controllers/dlgprefcontroller.h index 81f9af92d026..fe6ce7266370 100644 --- a/src/controllers/dlgprefcontroller.h +++ b/src/controllers/dlgprefcontroller.h @@ -55,9 +55,6 @@ class DlgPrefController : public DlgPreferencePage { private slots: /// Called when the user selects another mapping in the combobox void slotMappingSelected(int index); - /// Used to selected the current mapping in the combobox and display the - /// mapping information. - void slotShowMapping(std::shared_ptr mapping); void slotInputControlSearch(); void slotOutputControlSearch(); /// Called when the Controller Learning Wizard is closed. @@ -78,6 +75,9 @@ class DlgPrefController : public DlgPreferencePage { void midiInputMappingsLearned(const MidiInputMappings& mappings); private: + /// Used to selected the current mapping in the combobox and display the + /// mapping information. + void showMapping(std::shared_ptr mapping); QString mappingShortName(const std::shared_ptr pMapping) const; QString mappingName(const std::shared_ptr pMapping) const; QString mappingAuthor(const std::shared_ptr pMapping) const; diff --git a/src/controllers/dlgprefcontrollers.cpp b/src/controllers/dlgprefcontrollers.cpp index 7aefbe2accef..562b89f4f209 100644 --- a/src/controllers/dlgprefcontrollers.cpp +++ b/src/controllers/dlgprefcontrollers.cpp @@ -176,7 +176,7 @@ void DlgPrefControllers::setupControllerWidgets() { std::sort(controllerList.begin(), controllerList.end(), controllerCompare); for (auto* pController : std::as_const(controllerList)) { - DlgPrefController* pControllerDlg = new DlgPrefController( + auto pControllerDlg = make_parented( this, pController, m_pControllerManager, m_pConfig); connect(pControllerDlg, &DlgPrefController::mappingStarted, @@ -187,9 +187,9 @@ void DlgPrefControllers::setupControllerWidgets() { m_pDlgPreferences, &DlgPreferences::show); // Recreate the control picker menus when decks or samplers are added - m_pNumDecks->connectValueChanged(pControllerDlg, + m_pNumDecks->connectValueChanged(pControllerDlg.get(), &DlgPrefController::slotRecreateControlPickerMenu); - m_pNumSamplers->connectValueChanged(pControllerDlg, + m_pNumSamplers->connectValueChanged(pControllerDlg.get(), &DlgPrefController::slotRecreateControlPickerMenu); m_controllerPages.append(pControllerDlg); @@ -197,8 +197,8 @@ void DlgPrefControllers::setupControllerWidgets() { connect(pController, &Controller::openChanged, this, - [this, pControllerDlg](bool bOpen) { - slotHighlightDevice(pControllerDlg, bOpen); + [this, pDlg = pControllerDlg.get()](bool bOpen) { + slotHighlightDevice(pDlg, bOpen); }); QTreeWidgetItem* pControllerTreeItem = new QTreeWidgetItem( diff --git a/src/controllers/hid/hidcontroller.cpp b/src/controllers/hid/hidcontroller.cpp index edd43f5926d5..18b84e9b6246 100644 --- a/src/controllers/hid/hidcontroller.cpp +++ b/src/controllers/hid/hidcontroller.cpp @@ -29,14 +29,22 @@ QString HidController::mappingExtension() { } void HidController::setMapping(std::shared_ptr pMapping) { + m_pMutableMapping = pMapping; m_pMapping = downcastAndClone(pMapping.get()); } -std::shared_ptr HidController::cloneMapping() { +QList HidController::getMappingScriptFiles() { if (!m_pMapping) { - return nullptr; + return {}; } - return std::make_shared(*m_pMapping); + return m_pMapping->getScriptFiles(); +} + +QList> HidController::getMappingSettings() { + if (!m_pMapping) { + return {}; + } + return m_pMapping->getSettings(); } bool HidController::matchMapping(const MappingInfo& mapping) { @@ -101,8 +109,6 @@ int HidController::open() { return -1; } - setOpen(true); - m_pHidIoThread = std::make_unique(pHidDevice, m_deviceInfo); m_pHidIoThread->setObjectName(QStringLiteral("HidIoThread ") + getName()); @@ -132,6 +138,8 @@ int HidController::open() { qWarning() << "HidIoThread wasn't in expected OutputActive state"; } + applyMapping(); + setOpen(true); return 0; } diff --git a/src/controllers/hid/hidcontroller.h b/src/controllers/hid/hidcontroller.h index 0e31adb82054..411d02d5796e 100644 --- a/src/controllers/hid/hidcontroller.h +++ b/src/controllers/hid/hidcontroller.h @@ -17,9 +17,11 @@ class HidController final : public Controller { QString mappingExtension() override; - virtual std::shared_ptr cloneMapping() override; void setMapping(std::shared_ptr pMapping) override; + QList getMappingScriptFiles() override; + QList> getMappingSettings() override; + bool isMappable() const override { if (!m_pMapping) { return false; @@ -29,11 +31,10 @@ class HidController final : public Controller { bool matchMapping(const MappingInfo& mapping) override; - private slots: + private: int open() override; int close() override; - private: // For devices which only support a single report, reportID must be set to // 0x0. void sendBytes(const QByteArray& data) override; diff --git a/src/controllers/midi/hss1394controller.cpp b/src/controllers/midi/hss1394controller.cpp index 15e2c8e9b522..ad5b6e409659 100644 --- a/src/controllers/midi/hss1394controller.cpp +++ b/src/controllers/midi/hss1394controller.cpp @@ -129,8 +129,9 @@ int Hss1394Controller::open() { qWarning() << "Unable to set SCS.1d platter timer period."; } - setOpen(true); startEngine(); + applyMapping(); + setOpen(true); return 0; } diff --git a/src/controllers/midi/midicontroller.cpp b/src/controllers/midi/midicontroller.cpp index 780b012bbf11..b45546d4538d 100644 --- a/src/controllers/midi/midicontroller.cpp +++ b/src/controllers/midi/midicontroller.cpp @@ -56,14 +56,22 @@ QString MidiController::mappingExtension() { } void MidiController::setMapping(std::shared_ptr pMapping) { + m_pMutableMapping = pMapping; m_pMapping = downcastAndClone(pMapping.get()); } -std::shared_ptr MidiController::cloneMapping() { +QList MidiController::getMappingScriptFiles() { if (!m_pMapping) { - return nullptr; + return {}; } - return std::make_shared(*m_pMapping); + return m_pMapping->getScriptFiles(); +} + +QList> MidiController::getMappingSettings() { + if (!m_pMapping) { + return {}; + } + return m_pMapping->getSettings(); } int MidiController::close() { diff --git a/src/controllers/midi/midicontroller.h b/src/controllers/midi/midicontroller.h index c6334bda92c5..199d7695e518 100644 --- a/src/controllers/midi/midicontroller.h +++ b/src/controllers/midi/midicontroller.h @@ -41,7 +41,9 @@ class MidiController : public Controller { QString mappingExtension() override; void setMapping(std::shared_ptr pMapping) override; - virtual std::shared_ptr cloneMapping() override; + + QList getMappingScriptFiles() override; + QList> getMappingSettings() override; bool isMappable() const override { if (!m_pMapping) { @@ -73,6 +75,9 @@ class MidiController : public Controller { unsigned char control, const QJSValue& scriptCode); + bool applyMapping() override; + int close() override; + protected slots: virtual void receivedShortMessage( unsigned char status, @@ -81,12 +86,9 @@ class MidiController : public Controller { mixxx::Duration timestamp); // For receiving System Exclusive messages void receive(const QByteArray& data, mixxx::Duration timestamp) override; - int close() override; void slotBeforeEngineShutdown() override; private slots: - bool applyMapping() override; - void learnTemporaryInputMappings(const MidiInputMappings& mappings); void clearTemporaryInputMappings(); void commitTemporaryInputMappings(); diff --git a/src/controllers/midi/portmidicontroller.cpp b/src/controllers/midi/portmidicontroller.cpp index 0d0ef1f6a902..9a3223ec9932 100644 --- a/src/controllers/midi/portmidicontroller.cpp +++ b/src/controllers/midi/portmidicontroller.cpp @@ -78,9 +78,9 @@ int PortMidiController::open() { return -2; } } - - setOpen(true); startEngine(); + applyMapping(); + setOpen(true); return 0; } diff --git a/src/controllers/midi/portmidicontroller.h b/src/controllers/midi/portmidicontroller.h index 25012eb1ac99..d2480861ae5e 100644 --- a/src/controllers/midi/portmidicontroller.h +++ b/src/controllers/midi/portmidicontroller.h @@ -56,8 +56,6 @@ class PortMidiController : public MidiController { ~PortMidiController() override; private slots: - int open() override; - int close() override; bool poll() override; protected: @@ -66,6 +64,9 @@ class PortMidiController : public MidiController { unsigned char byte2) override; private: + int open() override; + int close() override; + // The sysex data must already contain the start byte 0xf0 and the end byte // 0xf7. void sendBytes(const QByteArray& data) override; diff --git a/src/test/controller_mapping_validation_test.h b/src/test/controller_mapping_validation_test.h index a58b4bef27a7..34872413f267 100644 --- a/src/test/controller_mapping_validation_test.h +++ b/src/test/controller_mapping_validation_test.h @@ -93,14 +93,23 @@ class FakeController : public Controller { } } - virtual std::shared_ptr cloneMapping() override { + QList getMappingScriptFiles() override { if (m_pMidiMapping) { - return std::make_shared(*m_pMidiMapping); + return m_pMidiMapping->getScriptFiles(); } else if (m_pHidMapping) { - return std::make_shared(*m_pHidMapping); + return m_pHidMapping->getScriptFiles(); } - return nullptr; - }; + return {}; + } + + QList> getMappingSettings() override { + if (m_pMidiMapping) { + return m_pMidiMapping->getSettings(); + } else if (m_pHidMapping) { + return m_pHidMapping->getSettings(); + } + return {}; + } bool isMappable() const override; @@ -120,7 +129,7 @@ class FakeController : public Controller { Q_UNUSED(data); } - private slots: + private: int open() override { return 0; } @@ -129,7 +138,6 @@ class FakeController : public Controller { return 0; } - private: bool m_bMidiMapping; bool m_bHidMapping; std::shared_ptr m_pMidiMapping;