diff --git a/src/controllers/bulk/bulkcontroller.cpp b/src/controllers/bulk/bulkcontroller.cpp index 6a2d1212b65f..722d30cd04f5 100644 --- a/src/controllers/bulk/bulkcontroller.cpp +++ b/src/controllers/bulk/bulkcontroller.cpp @@ -97,14 +97,14 @@ QString BulkController::mappingExtension() { } void BulkController::setMapping(std::shared_ptr pMapping) { - m_pMapping = downcastAndTakeOwnership(std::move(pMapping)); + m_pMapping = downcastAndClone(pMapping.get()); } std::shared_ptr BulkController::cloneMapping() { if (!m_pMapping) { return nullptr; } - return m_pMapping->clone(); + return std::make_shared(*m_pMapping); } bool BulkController::matchMapping(const MappingInfo& mapping) { diff --git a/src/controllers/bulk/bulkcontroller.h b/src/controllers/bulk/bulkcontroller.h index f7376c8ccf99..afca208fd1ef 100644 --- a/src/controllers/bulk/bulkcontroller.h +++ b/src/controllers/bulk/bulkcontroller.h @@ -84,5 +84,5 @@ class BulkController : public Controller { QString m_sUID; BulkReader* m_pReader; - std::shared_ptr m_pMapping; + std::unique_ptr m_pMapping; }; diff --git a/src/controllers/controller.h b/src/controllers/controller.h index 9d8ac370a1e3..ec990779f521 100644 --- a/src/controllers/controller.h +++ b/src/controllers/controller.h @@ -79,24 +79,20 @@ class Controller : public QObject { protected: template - std::shared_ptr downcastAndTakeOwnership( - std::shared_ptr&& pMapping) { + requires(std::is_final_v == true) + std::unique_ptr 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(pMapping); - VERIFY_OR_DEBUG_ASSERT(pDowncastedMapping) { + auto* pSpecifiedMapping = dynamic_cast(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(*pSpecifiedMapping); } // The length parameter is here for backwards compatibility for when scripts diff --git a/src/controllers/controllermanager.cpp b/src/controllers/controllermanager.cpp index e5cba1c27c43..ecc3daa92566 100644 --- a/src/controllers/controllermanager.cpp +++ b/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); diff --git a/src/controllers/hid/hidcontroller.cpp b/src/controllers/hid/hidcontroller.cpp index e97a94045d39..edd43f5926d5 100644 --- a/src/controllers/hid/hidcontroller.cpp +++ b/src/controllers/hid/hidcontroller.cpp @@ -29,14 +29,14 @@ QString HidController::mappingExtension() { } void HidController::setMapping(std::shared_ptr pMapping) { - m_pMapping = downcastAndTakeOwnership(std::move(pMapping)); + m_pMapping = downcastAndClone(pMapping.get()); } std::shared_ptr HidController::cloneMapping() { if (!m_pMapping) { return nullptr; } - return m_pMapping->clone(); + return std::make_shared(*m_pMapping); } bool HidController::matchMapping(const MappingInfo& mapping) { diff --git a/src/controllers/hid/hidcontroller.h b/src/controllers/hid/hidcontroller.h index 4da9df976e6c..0e31adb82054 100644 --- a/src/controllers/hid/hidcontroller.h +++ b/src/controllers/hid/hidcontroller.h @@ -41,7 +41,7 @@ class HidController final : public Controller { const mixxx::hid::DeviceInfo m_deviceInfo; std::unique_ptr m_pHidIoThread; - std::shared_ptr m_pMapping; + std::unique_ptr m_pMapping; friend class HidControllerJSProxy; }; diff --git a/src/controllers/hid/legacyhidcontrollermapping.h b/src/controllers/hid/legacyhidcontrollermapping.h index 97574aed1c06..cb976f556728 100644 --- a/src/controllers/hid/legacyhidcontrollermapping.h +++ b/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 clone() const override { - return std::make_shared(*this); - } - bool saveMapping(const QString& fileName) const override; bool isMappable() const override; diff --git a/src/controllers/legacycontrollermapping.h b/src/controllers/legacycontrollermapping.h index 0e556c9a11e7..a8abea26d91d 100644 --- a/src/controllers/legacycontrollermapping.h +++ b/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 clone() const = 0; - + public: struct ScriptFileInfo { ScriptFileInfo() : builtin(false) { diff --git a/src/controllers/midi/legacymidicontrollermapping.h b/src/controllers/midi/legacymidicontrollermapping.h index 574333c8b146..0f08fdacb4b6 100644 --- a/src/controllers/midi/legacymidicontrollermapping.h +++ b/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 clone() const override { - return std::make_shared(*this); - } - bool saveMapping(const QString& fileName) const override; virtual bool isMappable() const override; diff --git a/src/controllers/midi/midicontroller.cpp b/src/controllers/midi/midicontroller.cpp index c875d315b9be..45dc2d0f290b 100644 --- a/src/controllers/midi/midicontroller.cpp +++ b/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 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 pMapping) { - m_pMapping = downcastAndTakeOwnership(std::move(pMapping)); + m_pMapping = downcastAndClone(pMapping.get()); } std::shared_ptr MidiController::cloneMapping() { if (!m_pMapping) { return nullptr; } - return m_pMapping->clone(); + return std::make_shared(*m_pMapping); } int MidiController::close() { @@ -655,5 +656,13 @@ QJSValue MidiController::makeInputHandler(unsigned char status, std::make_shared(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); } diff --git a/src/controllers/midi/midicontroller.h b/src/controllers/midi/midicontroller.h index 8822c40099b7..c6334bda92c5 100644 --- a/src/controllers/midi/midicontroller.h +++ b/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 mapping, + MidiController* pMidiController, const MidiInputMapping& inputMapping); Q_INVOKABLE bool disconnect(); protected: - std::shared_ptr 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 m_temporaryInputMappings; QList m_outputs; - std::shared_ptr m_pMapping; + std::unique_ptr m_pMapping; SoftTakeoverCtrl m_st; QList> m_fourteen_bit_queued_mappings; diff --git a/src/test/controller_mapping_settings_test.cpp b/src/test/controller_mapping_settings_test.cpp index 9689f5a6c279..70fe5261765e 100644 --- a/src/test/controller_mapping_settings_test.cpp +++ b/src/test/controller_mapping_settings_test.cpp @@ -396,10 +396,6 @@ class LegacyDummyMapping : public LegacyControllerMapping { LegacyDummyMapping() { } - std::shared_ptr clone() const override { - return std::make_shared(*this); - } - bool saveMapping(const QString&) const override { return false; } diff --git a/src/test/controller_mapping_validation_test.h b/src/test/controller_mapping_validation_test.h index da868da44cb1..a58b4bef27a7 100644 --- a/src/test/controller_mapping_validation_test.h +++ b/src/test/controller_mapping_validation_test.h @@ -95,9 +95,9 @@ class FakeController : public Controller { virtual std::shared_ptr cloneMapping() override { if (m_pMidiMapping) { - return m_pMidiMapping->clone(); + return std::make_shared(*m_pMidiMapping); } else if (m_pHidMapping) { - return m_pHidMapping->clone(); + return std::make_shared(*m_pHidMapping); } return nullptr; }; diff --git a/src/test/midicontrollertest.cpp b/src/test/midicontrollertest.cpp index 92770fa80a24..07580f9eaa92 100644 --- a/src/test/midicontrollertest.cpp +++ b/src/test/midicontrollertest.cpp @@ -60,8 +60,8 @@ class MidiControllerTest : public MixxxTest { return m_pController->m_pScriptEngineLegacy->jsEngine()->evaluate(code).isError(); } - std::shared_ptr getControllerMapping() { - return m_pController->m_pMapping; + int getInputMappingCount() { + return m_pController->m_pMapping->getInputMappings().count(); } void shutdownController() { @@ -91,7 +91,7 @@ TEST_F(MidiControllerTest, ReceiveMessage_PushButtonCO_PushOnOff) { control), MidiOptions(), key)); - m_pController->setMapping(m_pMapping->clone()); + m_pController->setMapping(m_pMapping); // Receive an on/off, sets the control on/off with each press. receivedShortMessage(MidiOpCode::NoteOn, channel, control, 0x7F); @@ -120,7 +120,7 @@ TEST_F(MidiControllerTest, ReceiveMessage_PushButtonCO_PushOnOn) { control), MidiOptions(), key)); - m_pController->setMapping(m_pMapping->clone()); + m_pController->setMapping(m_pMapping); // Receive an on/off, sets the control on/off with each press. receivedShortMessage(MidiOpCode::NoteOn, channel, control, 0x7F); @@ -157,7 +157,7 @@ TEST_F(MidiControllerTest, ReceiveMessage_PushButtonCO_ToggleOnOff_ButtonMidiOpt control), options, key)); - m_pController->setMapping(m_pMapping->clone()); + m_pController->setMapping(m_pMapping); // NOTE(rryan): This behavior is broken! @@ -194,7 +194,7 @@ TEST_F(MidiControllerTest, ReceiveMessage_PushButtonCO_ToggleOnOff_SwitchMidiOpt control), options, key)); - m_pController->setMapping(m_pMapping->clone()); + m_pController->setMapping(m_pMapping); // NOTE(rryan): This behavior is broken! @@ -242,7 +242,7 @@ TEST_F(MidiControllerTest, ReceiveMessage_PushButtonCO_PushCC) { control), MidiOptions(), key)); - m_pController->setMapping(m_pMapping->clone()); + m_pController->setMapping(m_pMapping); // Receive an on/off, sets the control on/off with each press. receivedShortMessage(MidiOpCode::ControlChange, channel, control, 0x7F); @@ -277,7 +277,7 @@ TEST_F(MidiControllerTest, ReceiveMessage_ToggleCO_PushOnOff) { control), MidiOptions(), key)); - m_pController->setMapping(m_pMapping->clone()); + m_pController->setMapping(m_pMapping); // Receive an on/off, toggles the control. receivedShortMessage(MidiOpCode::NoteOn, channel, control, 0x7F); @@ -307,7 +307,7 @@ TEST_F(MidiControllerTest, ReceiveMessage_ToggleCO_PushOnOn) { control), MidiOptions(), key)); - m_pController->setMapping(m_pMapping->clone()); + m_pController->setMapping(m_pMapping); // Receive an on/off, toggles the control. receivedShortMessage(MidiOpCode::NoteOn, channel, control, 0x7F); @@ -345,7 +345,7 @@ TEST_F(MidiControllerTest, ReceiveMessage_ToggleCO_ToggleOnOff_ButtonMidiOption) control), options, key)); - m_pController->setMapping(m_pMapping->clone()); + m_pController->setMapping(m_pMapping); // NOTE(rryan): If the intended behavior of the button MIDI option is to // make a toggle MIDI button act like a push button then this isn't @@ -385,7 +385,7 @@ TEST_F(MidiControllerTest, ReceiveMessage_ToggleCO_ToggleOnOff_SwitchMidiOption) control), options, key)); - m_pController->setMapping(m_pMapping->clone()); + m_pController->setMapping(m_pMapping); // NOTE(rryan): If the intended behavior of switch MIDI option is to make a // toggle MIDI button act like a toggle button then this isn't working. The @@ -434,7 +434,7 @@ TEST_F(MidiControllerTest, ReceiveMessage_ToggleCO_PushCC) { control), MidiOptions(), key)); - m_pController->setMapping(m_pMapping->clone()); + m_pController->setMapping(m_pMapping); // Receive an on/off, toggles the control. receivedShortMessage(MidiOpCode::ControlChange, channel, control, 0x7F); @@ -466,7 +466,7 @@ TEST_F(MidiControllerTest, ReceiveMessage_PotMeterCO_7BitCC) { control), MidiOptions(), key)); - m_pController->setMapping(m_pMapping->clone()); + m_pController->setMapping(m_pMapping); // Receive a 0, MIDI parameter should map to the min value. receivedShortMessage(MidiOpCode::ControlChange, channel, control, 0x00); @@ -512,7 +512,7 @@ TEST_F(MidiControllerTest, ReceiveMessage_PotMeterCO_14BitCC) { msb_control), msb, key)); - m_pController->setMapping(m_pMapping->clone()); + m_pController->setMapping(m_pMapping); // If kMinValue or kMaxValue are such that the middle value is 0 then the // set(0) commands below allow us to hide failures. @@ -592,7 +592,7 @@ TEST_F(MidiControllerTest, ReceiveMessage_PotMeterCO_14BitPitchBend) { 0xFF), MidiOptions(), key)); - m_pController->setMapping(m_pMapping->clone()); + m_pController->setMapping(m_pMapping); // Receive a 0x0000, MIDI parameter should map to the min value. receivedShortMessage(MidiOpCode::PitchBendChange, channel, 0x00, 0x00); @@ -618,13 +618,13 @@ TEST_F(MidiControllerTest, JSInputHandler_BindHandler) { constexpr double kMinValue = -1234.5; constexpr double kMaxValue = 678.9; ControlPotmeter potmeter(ConfigKey("[Channel1]", "test_pot"), kMinValue, kMaxValue); - m_pController->setMapping(m_pMapping->clone()); - EXPECT_EQ(getControllerMapping()->getInputMappings().count(), 0); + m_pController->setMapping(m_pMapping); + EXPECT_EQ(getInputMappingCount(), 0); evaluateAndAssert( "midi.makeInputHandler(0x90, 0x43, (channel, control, value, status) => {" "engine.setParameter('[Channel1]', 'test_pot', value);" "})"); - EXPECT_EQ(getControllerMapping()->getInputMappings().count(), 1); + EXPECT_EQ(getInputMappingCount(), 1); receivedShortMessage(0x90, 0x43, 0x00); EXPECT_DOUBLE_EQ(potmeter.get(), kMinValue); receivedShortMessage(0x90, 0x43, 0x7F); @@ -632,29 +632,29 @@ TEST_F(MidiControllerTest, JSInputHandler_BindHandler) { } TEST_F(MidiControllerTest, JSInputHandler_ControllerShutdownSlot) { - m_pController->setMapping(m_pMapping->clone()); - EXPECT_EQ(getControllerMapping()->getInputMappings().count(), 0); + m_pController->setMapping(m_pMapping); + EXPECT_EQ(getInputMappingCount(), 0); evaluateAndAssert( - "midi.makeInputHandler(0x90, 0x00, (channel, control, value, status) => {})"); - EXPECT_EQ(getControllerMapping()->getInputMappings().count(), 1); + "midi.makeInputHandler(0x90, 0x43, (channel, control, value, status) => {})"); + EXPECT_EQ(getInputMappingCount(), 1); shutdownController(); - EXPECT_EQ(getControllerMapping()->getInputMappings().count(), 0); + EXPECT_EQ(getInputMappingCount(), 0); } TEST_F(MidiControllerTest, JSInputHandler_ErrorWhenControlIsTooLarge) { - m_pController->setMapping(m_pMapping->clone()); - EXPECT_EQ(getControllerMapping()->getInputMappings().count(), 0); + m_pController->setMapping(m_pMapping); + EXPECT_EQ(getInputMappingCount(), 0); bool isError = evaluateAndAssert( "midi.makeInputHandler(0x90, 0x80, (channel, control, value, status) => {})"); ASSERT_TRUE(isError); - EXPECT_EQ(getControllerMapping()->getInputMappings().count(), 0); + EXPECT_EQ(getInputMappingCount(), 0); } TEST_F(MidiControllerTest, JSInputHandler_ErrorWhenStatusIsTooSmall) { - m_pController->setMapping(m_pMapping->clone()); - EXPECT_EQ(getControllerMapping()->getInputMappings().count(), 0); + m_pController->setMapping(m_pMapping); + EXPECT_EQ(getInputMappingCount(), 0); bool isError = evaluateAndAssert( "midi.makeInputHandler(0x7F, 0x00, (channel, control, value, status) => {})"); ASSERT_TRUE(isError); - EXPECT_EQ(getControllerMapping()->getInputMappings().count(), 0); + EXPECT_EQ(getInputMappingCount(), 0); }