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 LegacyControllerMappingVisitor #3635

Merged
merged 1 commit into from
Feb 15, 2021
Merged

remove LegacyControllerMappingVisitor #3635

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
24 changes: 8 additions & 16 deletions src/controllers/controllermappingtablemodel.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,19 +4,19 @@

ControllerMappingTableModel::ControllerMappingTableModel(QObject* pParent)
: QAbstractTableModel(pParent),
m_pMidiMapping(nullptr),
m_pHidMapping(nullptr) {
m_pMidiMapping(nullptr) {
}

ControllerMappingTableModel::~ControllerMappingTableModel() {

}

void ControllerMappingTableModel::setMapping(LegacyControllerMappingPointer pMapping) {
m_pMapping = pMapping;
if (m_pMapping) {
// This immediately calls one of the two visit() methods below.
m_pMapping->accept(this);
m_pMidiMapping = dynamic_cast<LegacyMidiControllerMapping*>(pMapping.data());
// Only legacy MIDI mappings are supported
// TODO: prevent calling this code for unsupported mapping types?
if (!m_pMidiMapping) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this become VERIFY_OR_DEBUG_ASSERT? Otherwise please add a short comment that silently ignoring any other derived type here and resetting the member to nullptr when the dynamic cast fails is intended.

If the setMapping() is called twice first with the expected type (Midi) and then with another type (Hid) the internal pointer would be reset and the first invocation would get lost.

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 tried that initially but was asserting even though everything was working fine. A check could be added to the calling code to avoid that, but I'm trying to keep the changes as minimal as possible for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's fine. Just write a short comment about what you know. This knowledge might be helpful in the future.

The code looks suspicious to me without knowing the context or history. Usually you take a different route if a dynamic_cast fails. Here you (re-)set the member unconditionally before aborting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I added a comment. I may implement that debug assertion later after cleaning up more code. We'll see. First I need to clear the road before I can see the paths ahead.

return;
}

// Notify the child class a mapping was loaded.
Expand All @@ -25,16 +25,8 @@ void ControllerMappingTableModel::setMapping(LegacyControllerMappingPointer pMap

void ControllerMappingTableModel::cancel() {
// Apply mutates the mapping so to revert to the time just before the last
// apply, simply call setMapping again.
setMapping(m_pMapping);
}

void ControllerMappingTableModel::visit(LegacyMidiControllerMapping* pMidiMapping) {
m_pMidiMapping = pMidiMapping;
}

void ControllerMappingTableModel::visit(LegacyHidControllerMapping* pHidMapping) {
m_pHidMapping = pHidMapping;
// apply, simply reload the mapping.
onMappingLoaded();
}

bool ControllerMappingTableModel::setHeaderData(int section,
Expand Down
7 changes: 1 addition & 6 deletions src/controllers/controllermappingtablemodel.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,16 +12,13 @@
#include "controllers/legacycontrollermapping.h"
#include "controllers/midi/legacymidicontrollermapping.h"

class ControllerMappingTableModel : public QAbstractTableModel,
public LegacyControllerMappingVisitor {
class ControllerMappingTableModel : public QAbstractTableModel {
Q_OBJECT
public:
ControllerMappingTableModel(QObject* pParent);
~ControllerMappingTableModel() override;

void setMapping(LegacyControllerMappingPointer pMapping);
void visit(LegacyHidControllerMapping* pHidMapping) override;
void visit(LegacyMidiControllerMapping* pMidiMapping) override;

// Revert changes made since the last apply.
virtual void cancel();
Expand All @@ -42,7 +39,5 @@ class ControllerMappingTableModel : public QAbstractTableModel,
virtual void onMappingLoaded() = 0;

QVector<QHash<int, QVariant> > m_headerInfo;
LegacyControllerMappingPointer m_pMapping;
LegacyMidiControllerMapping* m_pMidiMapping;
LegacyHidControllerMapping* m_pHidMapping;
};
8 changes: 0 additions & 8 deletions src/controllers/controllermappingvisitor.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,6 @@
class LegacyMidiControllerMapping;
class LegacyHidControllerMapping;

class LegacyControllerMappingVisitor {
public:
virtual ~LegacyControllerMappingVisitor() {
}
virtual void visit(LegacyMidiControllerMapping* mapping) = 0;
virtual void visit(LegacyHidControllerMapping* mapping) = 0;
};

class ConstLegacyControllerMappingVisitor {
public:
virtual ~ConstLegacyControllerMappingVisitor() {
Expand Down
6 changes: 0 additions & 6 deletions src/controllers/hid/legacyhidcontrollermapping.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,6 @@ bool LegacyHidControllerMapping::saveMapping(const QString& fileName) const {
return handler.save(*this, fileName);
}

void LegacyHidControllerMapping::accept(LegacyControllerMappingVisitor* visitor) {
if (visitor) {
visitor->visit(this);
}
}

void LegacyHidControllerMapping::accept(ConstLegacyControllerMappingVisitor* visitor) const {
if (visitor) {
visitor->visit(this);
Expand Down
1 change: 0 additions & 1 deletion src/controllers/hid/legacyhidcontrollermapping.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ class LegacyHidControllerMapping : public LegacyControllerMapping {

bool saveMapping(const QString& fileName) const override;

void accept(LegacyControllerMappingVisitor* visitor) override;
void accept(ConstLegacyControllerMappingVisitor* visitor) const override;
bool isMappable() const override;
};
1 change: 0 additions & 1 deletion src/controllers/legacycontrollermapping.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,6 @@ class LegacyControllerMapping {

virtual bool saveMapping(const QString& filename) const = 0;

virtual void accept(LegacyControllerMappingVisitor* visitor) = 0;
virtual void accept(ConstLegacyControllerMappingVisitor* visitor) const = 0;
virtual bool isMappable() const = 0;

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(LegacyControllerMappingVisitor* visitor) {
if (visitor) {
visitor->visit(this);
}
}

void LegacyMidiControllerMapping::accept(ConstLegacyControllerMappingVisitor* visitor) const {
if (visitor) {
visitor->visit(this);
Expand Down
1 change: 0 additions & 1 deletion src/controllers/midi/legacymidicontrollermapping.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ class LegacyMidiControllerMapping : public LegacyControllerMapping {

bool saveMapping(const QString& fileName) const override;

virtual void accept(LegacyControllerMappingVisitor* visitor) override;
virtual void accept(ConstLegacyControllerMappingVisitor* visitor) const override;
virtual bool isMappable() const override;

Expand Down