Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove ConstLegacyControllerMappingVisitor #3636

Merged
merged 3 commits into from
Feb 17, 2021
Merged

remove ConstLegacyControllerMappingVisitor #3636

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
80f141c
remove ConstLegacyControllerMappingVisitor
Be-ing Feb 16, 2021
fd670fe
use std::shared_ptr for LegacyControllerMapping
Be-ing Feb 17, 2021
4efdeb5
Controller: create templated downcastAndTakeOwnership method
Be-ing Feb 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 4 additions & 10 deletions src/controllers/bulk/bulkcontroller.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Be-ing marked this conversation as resolved.
Show resolved Hide resolved
m_mapping = *pHidMapping;
}

bool BulkController::matchMapping(const MappingInfo& mapping) {
Expand Down
3 changes: 1 addition & 2 deletions src/controllers/bulk/bulkcontroller.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
9 changes: 2 additions & 7 deletions src/controllers/controller.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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();
Expand All @@ -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;
Expand Down
7 changes: 5 additions & 2 deletions src/controllers/controllermanager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()) {
Expand Down Expand Up @@ -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 {
Expand Down
1 change: 0 additions & 1 deletion src/controllers/controllermappingtablemodel.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
12 changes: 0 additions & 12 deletions src/controllers/controllermappingvisitor.h
View file
Open in desktop

This file was deleted.

14 changes: 4 additions & 10 deletions src/controllers/hid/hidcontroller.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
3 changes: 1 addition & 2 deletions src/controllers/hid/hidcontroller.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
7 changes: 0 additions & 7 deletions src/controllers/hid/legacyhidcontrollermapping.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@

#include "controllers/hid/legacyhidcontrollermapping.h"

#include "controllers/controllermappingvisitor.h"
#include "controllers/defs_controllers.h"
#include "controllers/hid/legacyhidcontrollermappingfilehandler.h"

Expand All @@ -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;
}
6 changes: 4 additions & 2 deletions src/controllers/hid/legacyhidcontrollermapping.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
#pragma once

#include "controllers/controllermappingvisitor.h"
#include "controllers/hid/legacyhidcontrollermappingfilehandler.h"
#include "controllers/legacycontrollermapping.h"

Expand All @@ -13,8 +12,11 @@ class LegacyHidControllerMapping : public LegacyControllerMapping {
~LegacyHidControllerMapping() override {
}

LegacyControllerMapping* clone() const override {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consistently use LegacyControllerMappingPointer everywhere, not plain pointers. Your code already has memory leaks.

Change the typedef from QSharedPointer to std::shared_pointer and then you could use dynamic_pointer_cast instead of dynamic_cast.

http://www.cplusplus.com/reference/memory/dynamic_pointer_cast/

Alternatively use qSharedPointerDynamicCast but I would prefer the standard C++ route instead of dealing with custom Qt re-inventions if not strictly necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll get rid of the QSharedPointer and use std. Might make the diff quite big though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the diff is too large then keep using QSharedPointer. Doesn't really matter.

Copy link
Contributor Author

@Be-ing Be-ing Feb 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Come to think of it, std::unique_ptr might be a better choice here to help enforce that the cloned object is not shared between threads.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh, there's no equivalent of dynamic_pointer_cast for unique_ptr...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a comment: // Downcast and take ownership on success

It's a shame that you need to write so much code and a comment to get this right. C++ 😩

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙈

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, a shared pointer is needed. DlgPrefController, ControllerInputMappingTableModel, and ControllerOutputMappingTableModel all share a pointer in the main thread.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

void HidController::setMapping(std::unique_ptr<LegacyControllerMapping> pMapping) {
    m_pMapping = std::unique_ptr<LegacyHidControllerMapping>(
        dynamic_cast<LegacyHidControllerMapping*>(pMapping.get()));
    if (m_pMapping) {
        pMapping.release();
    }
}

This seems to double free or something? Forget it, I'll use shared pointers and DEBUG_ASSERT that use_count is 1. 🙈

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly oncy. It is freed later by m_pMapping if the dynamic_cast succeeded or immediately when pMapping goes out of scope. release() doesn't free.

return new LegacyHidControllerMapping(*this);
}

bool saveMapping(const QString& fileName) const override;

void accept(ConstLegacyControllerMappingVisitor* visitor) const override;
bool isMappable() const override;
};
2 changes: 2 additions & 0 deletions src/controllers/hid/legacyhidcontrollermappingfilehandler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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);
Expand Down
3 changes: 2 additions & 1 deletion src/controllers/hid/legacyhidcontrollermappingfilehandler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
#pragma once

#include "controllers/hid/legacyhidcontrollermapping.h"
#include "controllers/legacycontrollermappingfilehandler.h"

class LegacyHidControllerMapping;

class LegacyHidControllerMappingFileHandler : public LegacyControllerMappingFileHandler {
public:
LegacyHidControllerMappingFileHandler(){};
Expand Down
6 changes: 2 additions & 4 deletions src/controllers/legacycontrollermapping.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -21,6 +18,8 @@ class LegacyControllerMapping {
}
virtual ~LegacyControllerMapping() = default;

virtual LegacyControllerMapping* clone() const = 0;

struct ScriptFileInfo {
ScriptFileInfo()
: builtin(false) {
Expand Down Expand Up @@ -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
Expand Down
6 changes: 0 additions & 6 deletions src/controllers/midi/legacymidicontrollermapping.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
6 changes: 4 additions & 2 deletions src/controllers/midi/legacymidicontrollermapping.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@

#include <QMultiHash>

#include "controllers/controllermappingvisitor.h"
#include "controllers/legacycontrollermapping.h"
#include "controllers/midi/midimessage.h"

Expand All @@ -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
Expand Down
13 changes: 4 additions & 9 deletions src/controllers/midi/midicontroller.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
7 changes: 2 additions & 5 deletions src/controllers/midi/midicontroller.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
2 changes: 1 addition & 1 deletion src/test/controller_mapping_validation_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
29 changes: 17 additions & 12 deletions src/test/controller_mapping_validation_test.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Loading