From b3a1880a61280b8919a0946c40484e40dbec3ac0 Mon Sep 17 00:00:00 2001 From: Ted Felix Date: Wed, 12 Jun 2024 15:42:37 -0400 Subject: [PATCH 01/17] Cleanup --- src/gui/studio/MidiProgramsEditor.cpp | 47 +++++++---------- src/gui/studio/MidiProgramsEditor.h | 76 +++++++++++++-------------- 2 files changed, 58 insertions(+), 65 deletions(-) diff --git a/src/gui/studio/MidiProgramsEditor.cpp b/src/gui/studio/MidiProgramsEditor.cpp index c53158206..af9e02f33 100644 --- a/src/gui/studio/MidiProgramsEditor.cpp +++ b/src/gui/studio/MidiProgramsEditor.cpp @@ -65,10 +65,9 @@ MidiProgramsEditor::MidiProgramsEditor(BankEditorDialog* bankEditor, tr("Bank and Program details"), // title parent, true), // showKeyMapButtons - m_device(nullptr), m_bankList(bankEditor->getBankList()), - m_programList(bankEditor->getProgramList()), - m_oldBank(false, 0, 0) + //m_oldBank(false, 0, 0), + m_programList(bankEditor->getProgramList()) { QWidget *additionalWidget = makeAdditionalWidget(m_topFrame); if (additionalWidget) { @@ -134,12 +133,6 @@ MidiProgramsEditor::getBankSubset(const MidiBank &bank) return program; } -MidiBank* -MidiProgramsEditor::getCurrentBank() -{ - return m_currentBank; -} - void MidiProgramsEditor::modifyCurrentPrograms(const MidiBank &oldBank, const MidiBank &newBank) @@ -292,14 +285,14 @@ MidiProgramsEditor::slotNewPercussion() RG_DEBUG << "MidiProgramsEditor::slotNewPercussion: calling setChecked(" << !percussion << ")"; newBank = new MidiBank(m_percussion->isChecked(), m_msb->value(), - m_lsb->value(), getCurrentBank()->getName()); + m_lsb->value(), m_currentBank->getName()); } else { newBank = new MidiBank(true, m_msb->value(), m_lsb->value()); } - modifyCurrentPrograms(*getCurrentBank(), *newBank); - *getCurrentBank() = *newBank; + modifyCurrentPrograms(*m_currentBank, *newBank); + *m_currentBank = *newBank; m_bankEditor->slotApply(); // Hack to force the percussion icons to switch state if needed. @@ -323,19 +316,19 @@ MidiProgramsEditor::slotNewMSB(int value) int msb; try { - msb = ensureUniqueMSB(value, value > getCurrentBank()->getMSB()); + msb = ensureUniqueMSB(value, value > m_currentBank->getMSB()); } catch (bool) { - msb = getCurrentBank()->getMSB(); + msb = m_currentBank->getMSB(); } MidiBank newBank(m_percussion->isChecked(), msb, - m_lsb->value(), getCurrentBank()->getName()); + m_lsb->value(), m_currentBank->getName()); - modifyCurrentPrograms(*getCurrentBank(), newBank); + modifyCurrentPrograms(*m_currentBank, newBank); m_msb->setValue(msb); - *getCurrentBank() = newBank; + *m_currentBank = newBank; m_msb->blockSignals(false); @@ -352,19 +345,19 @@ MidiProgramsEditor::slotNewLSB(int value) int lsb; try { - lsb = ensureUniqueLSB(value, value > getCurrentBank()->getLSB()); + lsb = ensureUniqueLSB(value, value > m_currentBank->getLSB()); } catch (bool) { - lsb = getCurrentBank()->getLSB(); + lsb = m_currentBank->getLSB(); } MidiBank newBank(m_percussion->isChecked(), m_msb->value(), - lsb, getCurrentBank()->getName()); + lsb, m_currentBank->getName()); - modifyCurrentPrograms(*getCurrentBank(), newBank); + modifyCurrentPrograms(*m_currentBank, newBank); m_lsb->setValue(lsb); - *getCurrentBank() = newBank; + *m_currentBank = newBank; m_lsb->blockSignals(false); @@ -402,7 +395,7 @@ MidiProgramsEditor::slotNameChanged(const QString& programName) //RG_DEBUG << "slotNameChanged(" << programName << ") : id = " << id; - MidiBank *currBank = getCurrentBank(); + MidiBank *currBank = m_currentBank; if (!currBank) { RG_WARNING << "slotNameChanged(): WARNING: currBank is nullptr."; @@ -422,14 +415,14 @@ MidiProgramsEditor::slotNameChanged(const QString& programName) return; // Create a new MidiProgram and add it to m_programList. - MidiProgram newProgram(*getCurrentBank(), id); + MidiProgram newProgram(*m_currentBank, id); m_programList.push_back(newProgram); // Sort by program number. std::sort(m_programList.begin(), m_programList.end(), ProgramCmp()); // Now, get the MidiProgram from the m_programList. - program = getProgram(*getCurrentBank(), id); + program = getProgram(*m_currentBank, id); } else { // If we've found a program and the label is now empty, @@ -483,7 +476,7 @@ MidiProgramsEditor::slotKeyMapButtonPressed() const unsigned id = button->property("index").toUInt(); - MidiProgram *program = getProgram(*getCurrentBank(), id); + MidiProgram *program = getProgram(*m_currentBank, id); if (!program) return; @@ -544,7 +537,7 @@ MidiProgramsEditor::slotKeyMapMenuItemSelected(int i) if (kml.empty()) return ; - MidiProgram *program = getProgram(*getCurrentBank(), m_currentMenuProgram); + MidiProgram *program = getProgram(*m_currentBank, m_currentMenuProgram); if (!program) return ; diff --git a/src/gui/studio/MidiProgramsEditor.h b/src/gui/studio/MidiProgramsEditor.h index db8501bd0..2eade659e 100644 --- a/src/gui/studio/MidiProgramsEditor.h +++ b/src/gui/studio/MidiProgramsEditor.h @@ -19,22 +19,20 @@ #ifndef RG_MIDIPROGRAMSEDITOR_H #define RG_MIDIPROGRAMSEDITOR_H -#include "base/MidiProgram.h" +#include "base/MidiProgram.h" // BankList, ProgramList #include "NameSetEditor.h" - class QWidget; class QString; class QSpinBox; class QTreeWidgetItem; class QCheckBox; -class BankList; namespace Rosegarden { -class MidiProgram; + class MidiDevice; class BankEditorDialog; @@ -47,65 +45,67 @@ class MidiProgramsEditor : public NameSetEditor QWidget *parent); void clearAll(); - void populate(QTreeWidgetItem*); + void populate(QTreeWidgetItem *); void reset(); public slots: - // Check that any new MSB/LSB combination is unique for this device - // + /// Check that any new MSB/LSB combination is unique for this device. + /** + * ??? This is causing some usability concerns. It can be tricky + * to assign MSBs and LSBs when the UI keeps refusing to take + * the value you are trying to assign. Recommend making the + * MSB/LSB fields read-only. Clicking on either brings up a + * dialog that allows changing both to whatever. Dismissing + * the dialog triggers the dupe check. + */ void slotNewMSB(int value); void slotNewLSB(int value); - void slotNewPercussion(); // gets value from checkbox + void slotNewPercussion(); void slotNameChanged(const QString &) override; void slotKeyMapButtonPressed() override; void slotKeyMapMenuItemSelected(QAction *); void slotKeyMapMenuItemSelected(int); -protected: +private: - MidiBank* getCurrentBank(); + void setBankName(const QString &s); - int ensureUniqueMSB(int msb, bool ascending); - int ensureUniqueLSB(int lsb, bool ascending); + QWidget *makeAdditionalWidget(QWidget *parent); + + // ??? There's usually a way to avoid this. Like using the + // proper signals. Ones that don't fire in response to + // API calls. User interaction only. + void blockAllSignals(bool block); + // Widgets + QCheckBox *m_percussion; + QSpinBox *m_msb; + QSpinBox *m_lsb; + + MidiDevice *m_device{nullptr}; + + BankList &m_bankList; // Does the banklist contain this combination already? // Disregard percussion bool, we care only about msb / lsb // in these situations. - // bool banklistContains(const MidiBank &); + int ensureUniqueMSB(int msb, bool ascending); + int ensureUniqueLSB(int lsb, bool ascending); - ProgramList getBankSubset(const MidiBank &); + MidiBank m_oldBank{false, 0, 0}; + MidiBank *m_currentBank{nullptr}; + ProgramList &m_programList; + // Get a program (pointer into program list) for modification + MidiProgram *getProgram(const MidiBank &bank, int programNo); + ProgramList getBankSubset(const MidiBank &); /// Set the currently loaded programs to new MSB and LSB void modifyCurrentPrograms(const MidiBank &oldBank, const MidiBank &newBank); - // Get a program (pointer into program list) for modification - // - MidiProgram* getProgram(const MidiBank &bank, int programNo); - - void setBankName(const QString& s); - - virtual QWidget *makeAdditionalWidget(QWidget *parent); - - void blockAllSignals(bool block); - - //--------------- Data members --------------------------------- - QCheckBox *m_percussion; - QSpinBox *m_msb; - QSpinBox *m_lsb; - - MidiDevice *m_device; - - MidiBank *m_currentBank; - BankList &m_bankList; - ProgramList &m_programList; - - MidiBank m_oldBank; - - unsigned int m_currentMenuProgram; + unsigned int m_currentMenuProgram; }; From 84bd7d853d8d0e2ac03808bec1a0d7182d8f7199 Mon Sep 17 00:00:00 2001 From: Ted Felix Date: Wed, 12 Jun 2024 16:21:52 -0400 Subject: [PATCH 02/17] Cleanup --- src/gui/studio/MidiProgramsEditor.cpp | 15 +++---- src/gui/studio/MidiProgramsEditor.h | 63 ++++++++++++++++++++++----- 2 files changed, 59 insertions(+), 19 deletions(-) diff --git a/src/gui/studio/MidiProgramsEditor.cpp b/src/gui/studio/MidiProgramsEditor.cpp index af9e02f33..5a09e6056 100644 --- a/src/gui/studio/MidiProgramsEditor.cpp +++ b/src/gui/studio/MidiProgramsEditor.cpp @@ -66,18 +66,17 @@ MidiProgramsEditor::MidiProgramsEditor(BankEditorDialog* bankEditor, parent, true), // showKeyMapButtons m_bankList(bankEditor->getBankList()), - //m_oldBank(false, 0, 0), m_programList(bankEditor->getProgramList()) { - QWidget *additionalWidget = makeAdditionalWidget(m_topFrame); - if (additionalWidget) { - m_topLayout->addWidget(additionalWidget, 0, 0, 3, 3); - } + QWidget *frame = initWidgets(m_topFrame); + m_topLayout->addWidget(frame, 0, 0, 3, 3); } -QWidget * -MidiProgramsEditor::makeAdditionalWidget(QWidget *parent) +QFrame * +MidiProgramsEditor::initWidgets(QWidget *parent) { + // ??? Inline this into the ctor like every other dialog. + QFrame *frame = new QFrame(parent); m_percussion = new QCheckBox(frame); @@ -185,7 +184,7 @@ MidiProgramsEditor::populate(QTreeWidgetItem* item) setEnabled(true); - setBankName(item->text(0)); + setTitle(item->text(0)); RG_DEBUG << "MidiProgramsEditor::populate : bankItem->getBank = " << bankItem->getBank(); diff --git a/src/gui/studio/MidiProgramsEditor.h b/src/gui/studio/MidiProgramsEditor.h index 2eade659e..2dad7e86d 100644 --- a/src/gui/studio/MidiProgramsEditor.h +++ b/src/gui/studio/MidiProgramsEditor.h @@ -40,15 +40,30 @@ class BankEditorDialog; class MidiProgramsEditor : public NameSetEditor { Q_OBJECT + public: + MidiProgramsEditor(BankEditorDialog *bankEditor, QWidget *parent); + /// Switch to the cleared and disabled state. + /** + * Called at the end of populateDeviceEditors() if no valid + * bank or program is selected in the tree. + */ void clearAll(); + + /// Show the programs for the selected bank. void populate(QTreeWidgetItem *); + + /// Limited undo in response to the Reset button in the lower left. + /** + * ??? But the caller calls populate right after this. Why do we + * need this? + */ void reset(); -public slots: +private slots: /// Check that any new MSB/LSB combination is unique for this device. /** @@ -70,41 +85,67 @@ public slots: private: + /// Set the name (title) that appears at the top of the editor. + /** + * ??? Remove this wrapper. + */ void setBankName(const QString &s); - QWidget *makeAdditionalWidget(QWidget *parent); - - // ??? There's usually a way to avoid this. Like using the - // proper signals. Ones that don't fire in response to - // API calls. User interaction only. - void blockAllSignals(bool block); + MidiDevice *m_device{nullptr}; // Widgets + QCheckBox *m_percussion; QSpinBox *m_msb; QSpinBox *m_lsb; + QFrame *initWidgets(QWidget *parent); - MidiDevice *m_device{nullptr}; + // ??? There's usually a way to avoid this. Like using the + // proper signals. Ones that don't fire in response to + // API calls. User interaction only. + void blockAllSignals(bool block); + + // Banks + /// The BankList we are editing. + /** + * We use this to check for dupes and make changes to the banks. + */ BankList &m_bankList; - // Does the banklist contain this combination already? + // Does m_bankList contain this combination already? // Disregard percussion bool, we care only about msb / lsb // in these situations. bool banklistContains(const MidiBank &); int ensureUniqueMSB(int msb, bool ascending); int ensureUniqueLSB(int lsb, bool ascending); - MidiBank m_oldBank{false, 0, 0}; + /// The bank we are editing right now. MidiBank *m_currentBank{nullptr}; + // Set by populate to the current bank. Used by reset() to restore + // the previous bank. + MidiBank m_oldBank{false, 0, 0}; + // Programs + + /// Programs for the bank being edited. ProgramList &m_programList; - // Get a program (pointer into program list) for modification + // Get a program (pointer into program list) for modification. MidiProgram *getProgram(const MidiBank &bank, int programNo); ProgramList getBankSubset(const MidiBank &); /// Set the currently loaded programs to new MSB and LSB void modifyCurrentPrograms(const MidiBank &oldBank, const MidiBank &newBank); + /// For assigning keymaps to programs. + /** + * Holds the index of the keymap button that was pressed. Set by + * slotKeyMapButtonPressed(). + * + * Used by slotKeyMapMenuItemSelected() to make sure the keymap + * selection ends up associated with the right program. + * + * ??? rename: m_keymapButtonIndex? + */ unsigned int m_currentMenuProgram; }; From 34eaeea04de7c2d32a186aed21794cd0f58450f1 Mon Sep 17 00:00:00 2001 From: Ted Felix Date: Wed, 12 Jun 2024 22:41:15 -0400 Subject: [PATCH 03/17] Cleanup --- src/gui/studio/MidiProgramsEditor.cpp | 101 ++++++++++++-------------- 1 file changed, 47 insertions(+), 54 deletions(-) diff --git a/src/gui/studio/MidiProgramsEditor.cpp b/src/gui/studio/MidiProgramsEditor.cpp index 5a09e6056..658cd13a7 100644 --- a/src/gui/studio/MidiProgramsEditor.cpp +++ b/src/gui/studio/MidiProgramsEditor.cpp @@ -19,6 +19,7 @@ #define RG_MODULE_STRING "[MidiProgramsEditor]" #include "MidiProgramsEditor.h" + #include "MidiBankTreeWidgetItem.h" #include "NameSetEditor.h" #include "BankEditorDialog.h" @@ -33,34 +34,28 @@ #include #include -#include #include +#include #include -#include -#include -#include #include #include #include #include -#include #include #include -#include #include -#include -#include #include -#include -#include +#include // std::sort +#include namespace Rosegarden { -MidiProgramsEditor::MidiProgramsEditor(BankEditorDialog* bankEditor, - QWidget* parent) : + +MidiProgramsEditor::MidiProgramsEditor(BankEditorDialog *bankEditor, + QWidget *parent) : NameSetEditor(bankEditor, tr("Bank and Program details"), // title parent, @@ -68,7 +63,7 @@ MidiProgramsEditor::MidiProgramsEditor(BankEditorDialog* bankEditor, m_bankList(bankEditor->getBankList()), m_programList(bankEditor->getProgramList()) { - QWidget *frame = initWidgets(m_topFrame); + QFrame *frame = initWidgets(m_topFrame); m_topLayout->addWidget(frame, 0, 0, 3, 3); } @@ -78,70 +73,72 @@ MidiProgramsEditor::initWidgets(QWidget *parent) // ??? Inline this into the ctor like every other dialog. QFrame *frame = new QFrame(parent); - - m_percussion = new QCheckBox(frame); - m_msb = new QSpinBox(frame); - m_lsb = new QSpinBox(frame); - frame->setContentsMargins(0, 0, 0, 0); + QGridLayout *gridLayout = new QGridLayout(frame); gridLayout->setSpacing(0); + // Percussion gridLayout->addWidget(new QLabel(tr("Percussion"), frame), 0, 0, Qt::AlignLeft); - gridLayout->addWidget(m_percussion, 0, 1, Qt::AlignLeft); + m_percussion = new QCheckBox(frame); connect(m_percussion, &QAbstractButton::clicked, this, &MidiProgramsEditor::slotNewPercussion); + gridLayout->addWidget(m_percussion, 0, 1, Qt::AlignLeft); + // MSB Value gridLayout->addWidget(new QLabel(tr("MSB Value"), frame), 1, 0, Qt::AlignLeft); + m_msb = new QSpinBox(frame); + m_msb->setToolTip(tr("Selects a MSB controller Bank number (MSB/LSB pairs are always unique for any Device)")); m_msb->setMinimum(0); m_msb->setMaximum(127); + connect(m_msb, + //QOverload::of(&QSpinBox::valueChanged), // Qt5.7+ + static_cast(&QSpinBox::valueChanged), + this, &MidiProgramsEditor::slotNewMSB); gridLayout->addWidget(m_msb, 1, 1, Qt::AlignLeft); - m_msb->setToolTip(tr("Selects a MSB controller Bank number (MSB/LSB pairs are always unique for any Device)")); - - m_lsb->setToolTip(tr("Selects a LSB controller Bank number (MSB/LSB pairs are always unique for any Device)")); - - connect(m_msb, SIGNAL(valueChanged(int)), - this, SLOT(slotNewMSB(int))); - + // LSB Value gridLayout->addWidget(new QLabel(tr("LSB Value"), frame), 2, 0, Qt::AlignLeft); + m_lsb = new QSpinBox(frame); + m_lsb->setToolTip(tr("Selects a LSB controller Bank number (MSB/LSB pairs are always unique for any Device)")); m_lsb->setMinimum(0); m_lsb->setMaximum(127); + connect(m_lsb, + //QOverload::of(&QSpinBox::valueChanged), // Qt5.7+ + static_cast(&QSpinBox::valueChanged), + this, &MidiProgramsEditor::slotNewLSB); gridLayout->addWidget(m_lsb, 2, 1, Qt::AlignLeft); - connect(m_lsb, SIGNAL(valueChanged(int)), - this, SLOT(slotNewLSB(int))); - return frame; } ProgramList MidiProgramsEditor::getBankSubset(const MidiBank &bank) { - ProgramList program; - ProgramList::iterator it; + ProgramList programList; - for (it = m_programList.begin(); it != m_programList.end(); ++it) { - if (it->getBank().partialCompare(bank)) - program.push_back(*it); + // For each program, copy the ones for the requested bank to programList. + for (const MidiProgram &program : m_programList) { + if (program.getBank().partialCompare(bank)) + programList.push_back(program); } - return program; + return programList; } void -MidiProgramsEditor::modifyCurrentPrograms(const MidiBank &oldBank, - const MidiBank &newBank) +MidiProgramsEditor::modifyCurrentPrograms( + const MidiBank &oldBank, const MidiBank &newBank) { - ProgramList::iterator it; - - for (it = m_programList.begin(); it != m_programList.end(); ++it) { - if (it->getBank().partialCompare(oldBank)) { - *it = MidiProgram(newBank, it->getProgram(), it->getName()); - } + // For each program in m_programList... + for (MidiProgram &program : m_programList) { + // If this one is in the old bank, update it to the new. + if (program.getBank().partialCompare(oldBank)) + program = MidiProgram( + newBank, program.getProgram(), program.getName()); } } @@ -310,8 +307,6 @@ MidiProgramsEditor::slotNewMSB(int value) { RG_DEBUG << "MidiProgramsEditor::slotNewMSB(" << value << ")\n"; - m_msb->blockSignals(true); - int msb; try { @@ -322,14 +317,14 @@ MidiProgramsEditor::slotNewMSB(int value) MidiBank newBank(m_percussion->isChecked(), msb, - m_lsb->value(), m_currentBank->getName()); + m_lsb->value(), + m_currentBank->getName()); modifyCurrentPrograms(*m_currentBank, newBank); m_msb->setValue(msb); - *m_currentBank = newBank; - m_msb->blockSignals(false); + *m_currentBank = newBank; m_bankEditor->slotApply(); } @@ -339,8 +334,6 @@ MidiProgramsEditor::slotNewLSB(int value) { RG_DEBUG << "MidiProgramsEditor::slotNewLSB(" << value << ")\n"; - m_lsb->blockSignals(true); - int lsb; try { @@ -351,14 +344,14 @@ MidiProgramsEditor::slotNewLSB(int value) MidiBank newBank(m_percussion->isChecked(), m_msb->value(), - lsb, m_currentBank->getName()); + lsb, + m_currentBank->getName()); modifyCurrentPrograms(*m_currentBank, newBank); m_lsb->setValue(lsb); - *m_currentBank = newBank; - m_lsb->blockSignals(false); + *m_currentBank = newBank; m_bankEditor->slotApply(); } @@ -462,7 +455,7 @@ MidiProgramsEditor::slotKeyMapButtonPressed() QToolButton *button = dynamic_cast(sender()); if (!button) { - RG_WARNING << "slotKeyMapButtonPressed() : WARNING: Sender is not a QPushButton."; + RG_WARNING << "slotKeyMapButtonPressed() : WARNING: Sender is not a QToolButton."; return; } From d80bd55430f73a9ac2cf8f1e73ae6a8e15d3c443 Mon Sep 17 00:00:00 2001 From: Ted Felix Date: Wed, 12 Jun 2024 23:02:51 -0400 Subject: [PATCH 04/17] Cleanup --- src/gui/studio/MidiProgramsEditor.cpp | 58 +++++++++++++-------------- src/gui/studio/NameSetEditor.cpp | 2 + 2 files changed, 29 insertions(+), 31 deletions(-) diff --git a/src/gui/studio/MidiProgramsEditor.cpp b/src/gui/studio/MidiProgramsEditor.cpp index 658cd13a7..2ef414b66 100644 --- a/src/gui/studio/MidiProgramsEditor.cpp +++ b/src/gui/studio/MidiProgramsEditor.cpp @@ -15,8 +15,8 @@ COPYING included with this distribution for more information. */ - #define RG_MODULE_STRING "[MidiProgramsEditor]" +#define RG_NO_DEBUG_PRINT #include "MidiProgramsEditor.h" @@ -145,32 +145,35 @@ MidiProgramsEditor::modifyCurrentPrograms( void MidiProgramsEditor::clearAll() { - blockAllSignals(true); - - for (size_t i = 0; i < m_names.size(); ++i) - m_names[i]->clear(); - setTitle(tr("Bank and Program details")); m_percussion->setChecked(false); m_msb->setValue(0); m_lsb->setValue(0); + m_currentBank = nullptr; + m_librarian->clear(); m_librarianEmail->clear(); - m_currentBank = nullptr; - setEnabled(false); + // Need this because NameSetEditor connects to each name + // field's textChanged signal instead of textEdited. + // ??? Fix NameSetEditor! + blockAllSignals(true); + for (size_t i = 0; i < m_names.size(); ++i) + m_names[i]->clear(); blockAllSignals(false); + + setEnabled(false); } void MidiProgramsEditor::populate(QTreeWidgetItem* item) { - RG_DEBUG << "MidiProgramsEditor::populate\n"; + RG_DEBUG << "populate()"; MidiBankTreeWidgetItem* bankItem = dynamic_cast(item); if (!bankItem) { - RG_DEBUG << "MidiProgramsEditor::populate : not a bank item - returning\n"; + RG_DEBUG << "MidiProgramsEditor::populate : not a bank item - returning"; return ; } @@ -183,13 +186,10 @@ MidiProgramsEditor::populate(QTreeWidgetItem* item) setTitle(item->text(0)); - RG_DEBUG << "MidiProgramsEditor::populate : bankItem->getBank = " - << bankItem->getBank(); + RG_DEBUG << "populate() : bankItem->getBank = " << bankItem->getBank(); m_currentBank = &(m_bankList[bankItem->getBank()]); // m_device->getBankByIndex(bankItem->getBank()); - blockAllSignals(true); - // set the bank values m_percussion->setChecked(m_currentBank->isPercussion()); m_msb->setValue(m_currentBank->getMSB()); @@ -212,6 +212,10 @@ MidiProgramsEditor::populate(QTreeWidgetItem* item) bool haveKeyMappings = m_device->getKeyMappings().size() > 0; + // Need this because NameSetEditor connects to each name + // field's textChanged signal instead of textEdited. + // ??? Fix NameSetEditor! + blockAllSignals(true); for (unsigned int i = 0; i < (unsigned int)m_names.size(); i++) { m_names[i]->clear(); @@ -246,17 +250,12 @@ MidiProgramsEditor::populate(QTreeWidgetItem* item) // show start of label m_names[i]->setCursorPosition(0); } - blockAllSignals(false); } void MidiProgramsEditor::reset() { - m_percussion->blockSignals(true); - m_msb->blockSignals(true); - m_lsb->blockSignals(true); - m_percussion->setChecked(m_oldBank.isPercussion()); m_msb->setValue(m_oldBank.getMSB()); m_lsb->setValue(m_oldBank.getLSB()); @@ -265,20 +264,17 @@ MidiProgramsEditor::reset() modifyCurrentPrograms(*m_currentBank, m_oldBank); *m_currentBank = m_oldBank; } - - m_percussion->blockSignals(false); - m_msb->blockSignals(false); - m_lsb->blockSignals(false); } void MidiProgramsEditor::slotNewPercussion() { - RG_DEBUG << "MidiProgramsEditor::slotNewPercussion"; + RG_DEBUG << "slotNewPercussion()"; + bool percussion = false; // Doesn't matter MidiBank *newBank; if (banklistContains(MidiBank(percussion, m_msb->value(), m_lsb->value()))) { - RG_DEBUG << "MidiProgramsEditor::slotNewPercussion: calling setChecked(" << !percussion << ")"; + RG_DEBUG << "slotNewPercussion(): calling setChecked(" << !percussion << ")"; newBank = new MidiBank(m_percussion->isChecked(), m_msb->value(), m_lsb->value(), m_currentBank->getName()); @@ -305,7 +301,7 @@ MidiProgramsEditor::slotNewPercussion() void MidiProgramsEditor::slotNewMSB(int value) { - RG_DEBUG << "MidiProgramsEditor::slotNewMSB(" << value << ")\n"; + RG_DEBUG << "slotNewMSB(" << value << ")"; int msb; @@ -332,7 +328,7 @@ MidiProgramsEditor::slotNewMSB(int value) void MidiProgramsEditor::slotNewLSB(int value) { - RG_DEBUG << "MidiProgramsEditor::slotNewLSB(" << value << ")\n"; + RG_DEBUG << "slotNewLSB(" << value << ")"; int lsb; @@ -633,7 +629,7 @@ MidiProgramsEditor::getProgram(const MidiBank &bank, int programNo) if (it->getBank().partialCompare(bank) && it->getProgram() == programNo) { - //Only show hits to avoid overflow of console. + // Only show hits to avoid overflow of console. RG_DEBUG << "it->getBank() " << "== bank"; return &(*it); } @@ -650,6 +646,8 @@ MidiProgramsEditor::setBankName(const QString& s) void MidiProgramsEditor::blockAllSignals(bool block) { + // Blocks all LineEdit signals. + QList allChildren = findChildren((QRegularExpression)"[0-9]+"); QList::iterator it; @@ -657,9 +655,7 @@ void MidiProgramsEditor::blockAllSignals(bool block) for (it = allChildren.begin(); it != allChildren.end(); ++it) { (*it)->blockSignals(block); } - - m_msb->blockSignals(block); - m_lsb->blockSignals(block); } + } diff --git a/src/gui/studio/NameSetEditor.cpp b/src/gui/studio/NameSetEditor.cpp index fa6346829..8b8e446c9 100644 --- a/src/gui/studio/NameSetEditor.cpp +++ b/src/gui/studio/NameSetEditor.cpp @@ -158,6 +158,8 @@ NameSetEditor::NameSetEditor(BankEditorDialog *bankEditor, lineEdit->setCompleter(new QCompleter(m_completions)); m_names.push_back(lineEdit); + // ??? This should be textEdited. Then we don't need to + // block signals all the time. connect(m_names[index], &QLineEdit::textChanged, this, &NameSetEditor::slotNameChanged); From 7cc8c62c2f565a2cc7afdeba4f2d157e27ec3960 Mon Sep 17 00:00:00 2001 From: Ted Felix Date: Thu, 13 Jun 2024 11:42:35 -0400 Subject: [PATCH 05/17] Cleanup --- src/gui/studio/MidiProgramsEditor.cpp | 113 +++++++++++++++----------- src/gui/studio/MidiProgramsEditor.h | 7 +- 2 files changed, 68 insertions(+), 52 deletions(-) diff --git a/src/gui/studio/MidiProgramsEditor.cpp b/src/gui/studio/MidiProgramsEditor.cpp index 2ef414b66..32c506d47 100644 --- a/src/gui/studio/MidiProgramsEditor.cpp +++ b/src/gui/studio/MidiProgramsEditor.cpp @@ -167,88 +167,105 @@ MidiProgramsEditor::clearAll() } void -MidiProgramsEditor::populate(QTreeWidgetItem* item) +MidiProgramsEditor::populate(QTreeWidgetItem *item) { RG_DEBUG << "populate()"; - MidiBankTreeWidgetItem* bankItem = dynamic_cast(item); + MidiBankTreeWidgetItem *bankItem = + dynamic_cast(item); if (!bankItem) { - RG_DEBUG << "MidiProgramsEditor::populate : not a bank item - returning"; - return ; + RG_DEBUG << "populate(): not a bank item - returning"; + return; } + RG_DEBUG << "populate() : bankItem->getBank = " << bankItem->getBank(); + + //m_currentBank = m_device->getBankByIndex(bankItem->getBank()); + m_currentBank = &(m_bankList[bankItem->getBank()]); + m_oldBank = *m_currentBank; + DeviceId deviceId = bankItem->getDeviceId(); m_device = m_bankEditor->getMidiDevice(deviceId); if (!m_device) - return ; + return; setEnabled(true); - setTitle(item->text(0)); + setTitle(bankItem->text(0)); - RG_DEBUG << "populate() : bankItem->getBank = " << bankItem->getBank(); - - m_currentBank = &(m_bankList[bankItem->getBank()]); // m_device->getBankByIndex(bankItem->getBank()); - - // set the bank values + // Percussion m_percussion->setChecked(m_currentBank->isPercussion()); + + // MSB Value m_msb->setValue(m_currentBank->getMSB()); - m_lsb->setValue(m_currentBank->getLSB()); - m_oldBank = *m_currentBank; + // LSB Value + m_lsb->setValue(m_currentBank->getLSB()); - // Librarian details - // + // Provided By m_librarian->setText(strtoqstr(m_device->getLibrarianName())); m_librarianEmail->setText(strtoqstr(m_device->getLibrarianEmail())); - ProgramList programSubset = getBankSubset(*m_currentBank); - ProgramList::iterator it; + // Program List - QPixmap noKeyPixmap, keyPixmap; + ProgramList programSubset = getBankSubset(*m_currentBank); - noKeyPixmap = IconLoader::loadPixmap("key-white"); - keyPixmap = IconLoader::loadPixmap("key-green"); + // Use a white icon to indicate there is no keymap for this program. + static const QIcon noKeymapIcon(IconLoader::loadPixmap("key-white")); + // Use a green icon to indicate there *is* a keymap for this program. + static const QIcon keymapIcon(IconLoader::loadPixmap("key-green")); - bool haveKeyMappings = m_device->getKeyMappings().size() > 0; + const bool haveKeyMappings = (m_device->getKeyMappings().size() > 0); // Need this because NameSetEditor connects to each name // field's textChanged signal instead of textEdited. // ??? Fix NameSetEditor! blockAllSignals(true); - for (unsigned int i = 0; i < (unsigned int)m_names.size(); i++) { - m_names[i]->clear(); - getKeyMapButton(i)->setEnabled(haveKeyMappings); - getKeyMapButton(i)->setIcon(QIcon(noKeyPixmap)); - // QToolTip::remove - // ( getKeyMapButton(i) ); - getKeyMapButton(i)->setToolTip(QString("")); //@@@ Usefull ? - getKeyMapButton(i)->setMaximumHeight( 12 ); - - for (it = programSubset.begin(); it != programSubset.end(); ++it) { - if (it->getProgram() == i) { - - // zero in on "Harpsichord" vs. "Coupled Harpsichord to cut down - // on noise (0-based) -// if (i == 6) std::cout << "it->getName(): " << it->getName() << std::endl; - QString programName = strtoqstr(it->getName()); - m_completions << programName; - m_names[i]->setText(programName); - - if (m_device->getKeyMappingForProgram(*it)) { - getKeyMapButton(i)->setIcon(QIcon(keyPixmap)); - getKeyMapButton(i)->setToolTip - (tr("Key Mapping: %1") - .arg(strtoqstr(m_device->getKeyMappingForProgram(*it)->getName()))); - } + // For each name in the program list... + // programIndex is also the program change number. + for (size_t programIndex = 0; programIndex < m_names.size(); ++programIndex) { + + // ??? Restructure this as: + // - find program in programSubset + // - if not found, clear and continue. + // - Set everything up. - break; + // Assume not found and clear everything. + + m_names[programIndex]->clear(); + + QToolButton *keyMapButton = getKeyMapButton(programIndex); + + keyMapButton->setEnabled(haveKeyMappings); + keyMapButton->setIcon(noKeymapIcon); + keyMapButton->setToolTip(""); + keyMapButton->setMaximumHeight(12); + + // Find the program in programSubset... + for (ProgramList::const_iterator midiProgramIter = programSubset.begin(); + midiProgramIter != programSubset.end(); + ++midiProgramIter) { + // Not it? Try the next. + if (midiProgramIter->getProgram() != programIndex) + continue; + + QString programName = strtoqstr(midiProgramIter->getName()); + m_completions << programName; + m_names[programIndex]->setText(programName); + + if (m_device->getKeyMappingForProgram(*midiProgramIter)) { + keyMapButton->setIcon(QIcon(keymapIcon)); + keyMapButton->setToolTip + (tr("Key Mapping: %1") + .arg(strtoqstr(m_device->getKeyMappingForProgram(*midiProgramIter)->getName()))); } + + break; } // show start of label - m_names[i]->setCursorPosition(0); + m_names[programIndex]->setCursorPosition(0); } blockAllSignals(false); } diff --git a/src/gui/studio/MidiProgramsEditor.h b/src/gui/studio/MidiProgramsEditor.h index 2dad7e86d..1a357a281 100644 --- a/src/gui/studio/MidiProgramsEditor.h +++ b/src/gui/studio/MidiProgramsEditor.h @@ -69,10 +69,9 @@ private slots: /** * ??? This is causing some usability concerns. It can be tricky * to assign MSBs and LSBs when the UI keeps refusing to take - * the value you are trying to assign. Recommend making the - * MSB/LSB fields read-only. Clicking on either brings up a - * dialog that allows changing both to whatever. Dismissing - * the dialog triggers the dupe check. + * the value you are trying to assign. Recommend reducing the + * MSB/LSB fields to a single MSB:LSB edit box. Perform + * the dupe check when the user is done editing. Tab or Enter. */ void slotNewMSB(int value); void slotNewLSB(int value); From 7d2d5938c68175e29bf712ce4cfaad9eb9d42251 Mon Sep 17 00:00:00 2001 From: Ted Felix Date: Thu, 13 Jun 2024 13:12:58 -0400 Subject: [PATCH 06/17] Cleanup --- src/gui/studio/MidiProgramsEditor.cpp | 91 +++++++++++++-------------- src/gui/studio/MidiProgramsEditor.h | 9 ++- 2 files changed, 49 insertions(+), 51 deletions(-) diff --git a/src/gui/studio/MidiProgramsEditor.cpp b/src/gui/studio/MidiProgramsEditor.cpp index 32c506d47..8280fe973 100644 --- a/src/gui/studio/MidiProgramsEditor.cpp +++ b/src/gui/studio/MidiProgramsEditor.cpp @@ -83,7 +83,7 @@ MidiProgramsEditor::initWidgets(QWidget *parent) 0, 0, Qt::AlignLeft); m_percussion = new QCheckBox(frame); connect(m_percussion, &QAbstractButton::clicked, - this, &MidiProgramsEditor::slotNewPercussion); + this, &MidiProgramsEditor::slotPercussionClicked); gridLayout->addWidget(m_percussion, 0, 1, Qt::AlignLeft); // MSB Value @@ -208,6 +208,7 @@ MidiProgramsEditor::populate(QTreeWidgetItem *item) // Program List + // Get the programs for the current bank. ProgramList programSubset = getBankSubset(*m_currentBank); // Use a white icon to indicate there is no keymap for this program. @@ -226,46 +227,51 @@ MidiProgramsEditor::populate(QTreeWidgetItem *item) // programIndex is also the program change number. for (size_t programIndex = 0; programIndex < m_names.size(); ++programIndex) { + QToolButton *keyMapButton = getKeyMapButton(programIndex); + // ??? Seems like something NameSetEditor should take care of? + keyMapButton->setMaximumHeight(12); + keyMapButton->setEnabled(haveKeyMappings); + // ??? Restructure this as: // - find program in programSubset // - if not found, clear and continue. // - Set everything up. // Assume not found and clear everything. - m_names[programIndex]->clear(); - - QToolButton *keyMapButton = getKeyMapButton(programIndex); - - keyMapButton->setEnabled(haveKeyMappings); keyMapButton->setIcon(noKeymapIcon); keyMapButton->setToolTip(""); - keyMapButton->setMaximumHeight(12); // Find the program in programSubset... - for (ProgramList::const_iterator midiProgramIter = programSubset.begin(); - midiProgramIter != programSubset.end(); - ++midiProgramIter) { + for (const MidiProgram &midiProgram : programSubset) { // Not it? Try the next. - if (midiProgramIter->getProgram() != programIndex) + if (midiProgram.getProgram() != programIndex) continue; - QString programName = strtoqstr(midiProgramIter->getName()); + // Found it. + + QString programName = strtoqstr(midiProgram.getName()); m_completions << programName; m_names[programIndex]->setText(programName); - - if (m_device->getKeyMappingForProgram(*midiProgramIter)) { - keyMapButton->setIcon(QIcon(keymapIcon)); - keyMapButton->setToolTip - (tr("Key Mapping: %1") - .arg(strtoqstr(m_device->getKeyMappingForProgram(*midiProgramIter)->getName()))); + // show start of label + m_names[programIndex]->setCursorPosition(0); + + const MidiKeyMapping *midiKeyMapping = + m_device->getKeyMappingForProgram(midiProgram); + if (midiKeyMapping) { + // Indicate that this program has a keymap. + keyMapButton->setIcon(keymapIcon); + // Put the name in the tool tip. + keyMapButton->setToolTip(tr("Key Mapping: %1").arg( + strtoqstr(midiKeyMapping->getName()))); + } else { // No key mapping. + // Indicate that this program has no keymap. + keyMapButton->setIcon(noKeymapIcon); + keyMapButton->setToolTip(""); } break; } - - // show start of label - m_names[programIndex]->setCursorPosition(0); } blockAllSignals(false); } @@ -273,10 +279,14 @@ MidiProgramsEditor::populate(QTreeWidgetItem *item) void MidiProgramsEditor::reset() { + // Go back to m_oldBank's MSB/LSB and percussion setting. + m_percussion->setChecked(m_oldBank.isPercussion()); m_msb->setValue(m_oldBank.getMSB()); m_lsb->setValue(m_oldBank.getLSB()); + // Make sure all the programs in m_programList are set back to the m_oldBank + // MSB/LSB. if (m_currentBank) { modifyCurrentPrograms(*m_currentBank, m_oldBank); *m_currentBank = m_oldBank; @@ -284,35 +294,24 @@ MidiProgramsEditor::reset() } void -MidiProgramsEditor::slotNewPercussion() +MidiProgramsEditor::slotPercussionClicked() { - RG_DEBUG << "slotNewPercussion()"; + RG_DEBUG << "slotPercussionClicked()"; - bool percussion = false; // Doesn't matter - MidiBank *newBank; - if (banklistContains(MidiBank(percussion, m_msb->value(), m_lsb->value()))) { - RG_DEBUG << "slotNewPercussion(): calling setChecked(" << !percussion << ")"; - newBank = new MidiBank(m_percussion->isChecked(), - m_msb->value(), - m_lsb->value(), m_currentBank->getName()); - } else { - newBank = new MidiBank(true, - m_msb->value(), - m_lsb->value()); - } - modifyCurrentPrograms(*m_currentBank, *newBank); - *m_currentBank = *newBank; - m_bankEditor->slotApply(); + MidiBank newBank( + m_percussion->isChecked(), + m_msb->value(), + m_lsb->value(), + m_currentBank->getName()); - // Hack to force the percussion icons to switch state if needed. - // code stole from populate. - if (m_device) { - bool haveKeyMappings = m_device->getKeyMappings().size() > 0; + // Make sure the programs in m_programList have the new percussion setting. + modifyCurrentPrograms(*m_currentBank, newBank); - for (unsigned int i = 0; i < (unsigned int)m_names.size(); i++) { - getKeyMapButton(i)->setEnabled(haveKeyMappings); - } - } + // Update the current bank. + *m_currentBank = newBank; + + // Refresh the tree so that it shows "Percussion" or "Bank" as appropriate. + m_bankEditor->slotApply(); } void diff --git a/src/gui/studio/MidiProgramsEditor.h b/src/gui/studio/MidiProgramsEditor.h index 1a357a281..58075908e 100644 --- a/src/gui/studio/MidiProgramsEditor.h +++ b/src/gui/studio/MidiProgramsEditor.h @@ -58,8 +58,8 @@ class MidiProgramsEditor : public NameSetEditor /// Limited undo in response to the Reset button in the lower left. /** - * ??? But the caller calls populate right after this. Why do we - * need this? + * This appears to be a very limited undo for just the MSB/LSB, + * and the percussion setting. */ void reset(); @@ -75,7 +75,7 @@ private slots: */ void slotNewMSB(int value); void slotNewLSB(int value); - void slotNewPercussion(); + void slotPercussionClicked(); void slotNameChanged(const QString &) override; void slotKeyMapButtonPressed() override; @@ -120,8 +120,7 @@ private slots: /// The bank we are editing right now. MidiBank *m_currentBank{nullptr}; - // Set by populate to the current bank. Used by reset() to restore - // the previous bank. + /// Used by reset() to restore the original MSB/LSB/Percussion. MidiBank m_oldBank{false, 0, 0}; // Programs From 4d9006001661533914cbee9277e89b19a0863988 Mon Sep 17 00:00:00 2001 From: Ted Felix Date: Fri, 14 Jun 2024 01:31:33 -0400 Subject: [PATCH 07/17] Cleanup --- src/base/MidiProgram.h | 19 ++++++ src/gui/studio/MidiProgramsEditor.cpp | 91 ++++++++++++++------------- src/gui/studio/MidiProgramsEditor.h | 5 ++ 3 files changed, 73 insertions(+), 42 deletions(-) diff --git a/src/base/MidiProgram.h b/src/base/MidiProgram.h index e959bdead..630a65dca 100644 --- a/src/base/MidiProgram.h +++ b/src/base/MidiProgram.h @@ -63,6 +63,17 @@ class MidiBank * a partial comparison such as this is frequently needed. */ bool partialCompare(const MidiBank &rhs) const; + /// Less + /** + * Note that this only looks at msb and lsb. Those are the only + * things that really count when sorting. + */ + bool operator<(const MidiBank &rhs) const + { + if (m_msb == rhs.m_msb) + return (m_lsb < rhs.m_lsb); + return (m_msb < rhs.m_msb); + } private: bool m_percussion; @@ -95,6 +106,14 @@ class MidiProgram // Only compares m_bank, m_program, and m_name. Does not compare // m_keyMapping. bool partialCompareWithName(const MidiProgram &rhs) const; + bool operator<(const MidiProgram &rhs) + { + // ??? But bank is more important than program. Why + // is this comparing program first? This makes no sense. + if (m_program == rhs.m_program) + return (m_bank < rhs.m_bank); + return (m_program < rhs.m_program); + } private: MidiBank m_bank; diff --git a/src/gui/studio/MidiProgramsEditor.cpp b/src/gui/studio/MidiProgramsEditor.cpp index 8280fe973..b79c496c3 100644 --- a/src/gui/studio/MidiProgramsEditor.cpp +++ b/src/gui/studio/MidiProgramsEditor.cpp @@ -319,6 +319,8 @@ MidiProgramsEditor::slotNewMSB(int value) { RG_DEBUG << "slotNewMSB(" << value << ")"; + // ??? Not sure we should clean this up since we are getting rid of it. + int msb; try { @@ -338,6 +340,7 @@ MidiProgramsEditor::slotNewMSB(int value) *m_currentBank = newBank; + // Refresh the tree so that it shows the new MSB. m_bankEditor->slotApply(); } @@ -346,6 +349,8 @@ MidiProgramsEditor::slotNewLSB(int value) { RG_DEBUG << "slotNewLSB(" << value << ")"; + // ??? Not sure we should clean this up since we are getting rid of it. + int lsb; try { @@ -365,81 +370,78 @@ MidiProgramsEditor::slotNewLSB(int value) *m_currentBank = newBank; + // Refresh the tree so that it shows the new MSB. m_bankEditor->slotApply(); } -struct ProgramCmp -{ - bool operator()(const Rosegarden::MidiProgram &p1, - const Rosegarden::MidiProgram &p2) const - { - if (p1.getProgram() == p2.getProgram()) { - const Rosegarden::MidiBank &b1(p1.getBank()); - const Rosegarden::MidiBank &b2(p2.getBank()); - if (b1.getMSB() == b2.getMSB()) - if (b1.getLSB() == b2.getLSB()) - return ((b1.isPercussion() ? 1 : 0) < (b2.isPercussion() ? 1 : 0)); - else return (b1.getLSB() < b2.getLSB()); - else return (b1.getMSB() < b2.getMSB()); - } else return (p1.getProgram() < p2.getProgram()); - } -}; - void -MidiProgramsEditor::slotNameChanged(const QString& programName) +MidiProgramsEditor::slotNameChanged(const QString &programName) { - const LineEdit *lineEdit = dynamic_cast(sender()); + //RG_DEBUG << "slotNameChanged(" << programName << ")"; - if (!lineEdit) { - RG_WARNING << "slotNameChanged(): WARNING: Signal sender is not a LineEdit."; + // This is called for every single change to the edit box. E.g. + // If the user types "hi", this slot is called twice. Once with + // "h" and again with "hi". + + if (!m_currentBank) { + RG_WARNING << "slotNameChanged(): WARNING: m_currentBank is nullptr."; return; } - const unsigned id = lineEdit->property("index").toUInt(); - - //RG_DEBUG << "slotNameChanged(" << programName << ") : id = " << id; - - MidiBank *currBank = m_currentBank; + const LineEdit *lineEdit = dynamic_cast(sender()); - if (!currBank) { - RG_WARNING << "slotNameChanged(): WARNING: currBank is nullptr."; + if (!lineEdit) { + RG_WARNING << "slotNameChanged(): WARNING: Signal sender is not a LineEdit."; return; } - //RG_DEBUG << "slotNameChanged(): currBank: " << currBank; + // Zero-based program number. This is the same as the MIDI program change. + const unsigned programNumber = lineEdit->property("index").toUInt(); - //RG_DEBUG << "slotNameChanged(): current bank name: " << currBank->getName(); + //RG_DEBUG << "slotNameChanged(" << programName << ") : id = " << programNumber; - MidiProgram *program = getProgram(*currBank, id); + MidiProgram *program = getProgram(*m_currentBank, programNumber); - // If the MidiProgram doesn't exist + // If the MidiProgram doesn't exist in m_programList, add it. if (!program) { // Do nothing if program name is empty if (programName.isEmpty()) return; // Create a new MidiProgram and add it to m_programList. - MidiProgram newProgram(*m_currentBank, id); + MidiProgram newProgram(*m_currentBank, programNumber); m_programList.push_back(newProgram); - // Sort by program number. - std::sort(m_programList.begin(), m_programList.end(), ProgramCmp()); + // Sort m_programList. + // ??? Why do we need this sorted? Is it a Device requirement? + // If it is a requirement, is there another std::sort() someplace? + // Confusing. And MidiProgram's op< is backwards. It is + // comparing program and then bank. But bank is more important. + // I have a feeling sorting and the op< are unnecessary. Need to + // do some digging. + // ??? Recommend seeing if we can remove this and remove the op<() in + // MidiBank and MidiProgram. + //std::sort(m_programList.begin(), m_programList.end(), ProgramCmp()); + std::sort(m_programList.begin(), m_programList.end()); - // Now, get the MidiProgram from the m_programList. - program = getProgram(*m_currentBank, id); + // Get the MidiProgram from the m_programList. + program = getProgram(*m_currentBank, programNumber); - } else { - // If we've found a program and the label is now empty, - // remove it from the program list. + } else { // The MidiProgram already exists in m_programList. + + // If the label is now empty... if (programName.isEmpty()) { + // See if the program is in the program list. for (ProgramList::iterator it = m_programList.begin(); it != m_programList.end(); ++it) { - if (static_cast(it->getProgram()) == id) { + // Found it? Remove it. + if (static_cast(it->getProgram()) == programNumber) { m_programList.erase(it); + // ??? Why? m_bankEditor->slotApply(); - //RG_DEBUG << "slotNameChanged(): deleting empty program (" << id << ")"; + //RG_DEBUG << "slotNameChanged(): deleting empty program (" << programNumber << ")"; return; } @@ -457,6 +459,7 @@ MidiProgramsEditor::slotNameChanged(const QString& programName) // If the name has actually changed if (qstrtostr(programName) != program->getName()) { program->setName(qstrtostr(programName)); + // ??? Why? m_bankEditor->slotApply(); } } @@ -583,6 +586,8 @@ MidiProgramsEditor::slotKeyMapMenuItemSelected(int i) int MidiProgramsEditor::ensureUniqueMSB(int msb, bool ascending) { + // ??? Not sure we should clean this up since we are getting rid of it. + bool percussion = false; // Doesn't matter int newMSB = msb; while (banklistContains(MidiBank(percussion, @@ -603,6 +608,8 @@ MidiProgramsEditor::ensureUniqueMSB(int msb, bool ascending) int MidiProgramsEditor::ensureUniqueLSB(int lsb, bool ascending) { + // ??? Not sure we should clean this up since we are getting rid of it. + bool percussion = false; // Doesn't matter int newLSB = lsb; while (banklistContains(MidiBank(percussion, diff --git a/src/gui/studio/MidiProgramsEditor.h b/src/gui/studio/MidiProgramsEditor.h index 58075908e..af25dfd7e 100644 --- a/src/gui/studio/MidiProgramsEditor.h +++ b/src/gui/studio/MidiProgramsEditor.h @@ -77,6 +77,7 @@ private slots: void slotNewLSB(int value); void slotPercussionClicked(); + /// One of the program names was changed by the user. void slotNameChanged(const QString &) override; void slotKeyMapButtonPressed() override; void slotKeyMapMenuItemSelected(QAction *); @@ -126,6 +127,10 @@ private slots: // Programs /// Programs for the bank being edited. + /** + * ??? No, I think this is all of the programs for the device. + * Otherwise there would be no need for getBankSubset(). + */ ProgramList &m_programList; // Get a program (pointer into program list) for modification. MidiProgram *getProgram(const MidiBank &bank, int programNo); From 0480049a96fea582d4c002b7d1aa4f841c6d972c Mon Sep 17 00:00:00 2001 From: Ted Felix Date: Wed, 19 Jun 2024 11:52:50 -0400 Subject: [PATCH 08/17] Fix program name deletion Deleting a program name by clearing the name field would typically delete the program from another bank instead of the current bank. Now it works correctly. --- src/gui/studio/MidiProgramsEditor.cpp | 60 ++++++++++++++++----------- src/gui/studio/MidiProgramsEditor.h | 2 + 2 files changed, 38 insertions(+), 24 deletions(-) diff --git a/src/gui/studio/MidiProgramsEditor.cpp b/src/gui/studio/MidiProgramsEditor.cpp index b79c496c3..a44c20041 100644 --- a/src/gui/studio/MidiProgramsEditor.cpp +++ b/src/gui/studio/MidiProgramsEditor.cpp @@ -383,6 +383,9 @@ MidiProgramsEditor::slotNameChanged(const QString &programName) // If the user types "hi", this slot is called twice. Once with // "h" and again with "hi". + // ??? Can we be more efficient? E.g. only make the change when + // the cursor leaves the edit box, or "Ok" is clicked? + if (!m_currentBank) { RG_WARNING << "slotNameChanged(): WARNING: m_currentBank is nullptr."; return; @@ -400,11 +403,13 @@ MidiProgramsEditor::slotNameChanged(const QString &programName) //RG_DEBUG << "slotNameChanged(" << programName << ") : id = " << programNumber; - MidiProgram *program = getProgram(*m_currentBank, programNumber); + // Get the MidiProgram that needs to be changed from m_programList. + ProgramList::iterator programIter = + getProgramIter(*m_currentBank, programNumber); // If the MidiProgram doesn't exist in m_programList, add it. - if (!program) { - // Do nothing if program name is empty + if (programIter == m_programList.end()) { + // If the program name is empty, do nothing. if (programName.isEmpty()) return; @@ -424,41 +429,32 @@ MidiProgramsEditor::slotNameChanged(const QString &programName) //std::sort(m_programList.begin(), m_programList.end(), ProgramCmp()); std::sort(m_programList.begin(), m_programList.end()); - // Get the MidiProgram from the m_programList. - program = getProgram(*m_currentBank, programNumber); + // Get the new MidiProgram from m_programList. + programIter = getProgramIter(*m_currentBank, programNumber); } else { // The MidiProgram already exists in m_programList. // If the label is now empty... if (programName.isEmpty()) { - // See if the program is in the program list. - for (ProgramList::iterator it = m_programList.begin(); - it != m_programList.end(); - ++it) { - // Found it? Remove it. - if (static_cast(it->getProgram()) == programNumber) { - m_programList.erase(it); - // ??? Why? - m_bankEditor->slotApply(); - - //RG_DEBUG << "slotNameChanged(): deleting empty program (" << programNumber << ")"; - - return; - } - } + //RG_DEBUG << "slotNameChanged(): deleting empty program (" << programNumber << ")"; + m_programList.erase(programIter); + // ??? Why? + m_bankEditor->slotApply(); + + return; } } - if (!program) { - RG_WARNING << "slotNameChanged(): WARNING: program is nullptr."; + if (programIter == m_programList.end()) { + RG_WARNING << "slotNameChanged(): WARNING: programIter is end()."; return; } //RG_DEBUG << "slotNameChanged(): program: " << program; // If the name has actually changed - if (qstrtostr(programName) != program->getName()) { - program->setName(qstrtostr(programName)); + if (qstrtostr(programName) != programIter->getName()) { + programIter->setName(qstrtostr(programName)); // ??? Why? m_bankEditor->slotApply(); } @@ -661,6 +657,22 @@ MidiProgramsEditor::getProgram(const MidiBank &bank, int programNo) return nullptr; } +ProgramList::iterator +MidiProgramsEditor::getProgramIter(const MidiBank &bank, int programNo) +{ + // For each program in m_programList... + for (ProgramList::iterator programIter = m_programList.begin(); + programIter != m_programList.end(); + ++programIter) { + // Match? + if (programIter->getBank().partialCompare(bank) && + programIter->getProgram() == programNo) + return programIter; + } + + return m_programList.end(); +} + void MidiProgramsEditor::setBankName(const QString& s) { diff --git a/src/gui/studio/MidiProgramsEditor.h b/src/gui/studio/MidiProgramsEditor.h index af25dfd7e..684e6886c 100644 --- a/src/gui/studio/MidiProgramsEditor.h +++ b/src/gui/studio/MidiProgramsEditor.h @@ -134,6 +134,8 @@ private slots: ProgramList &m_programList; // Get a program (pointer into program list) for modification. MidiProgram *getProgram(const MidiBank &bank, int programNo); + /// Get an iterator which is more flexible. + ProgramList::iterator getProgramIter(const MidiBank &bank, int programNo); ProgramList getBankSubset(const MidiBank &); /// Set the currently loaded programs to new MSB and LSB void modifyCurrentPrograms(const MidiBank &oldBank, From f2609be1f01d43dac729d42dd98bf99ebb25b9f9 Mon Sep 17 00:00:00 2001 From: Ted Felix Date: Wed, 19 Jun 2024 15:09:46 -0400 Subject: [PATCH 09/17] MPE: Cleanup --- src/gui/studio/MidiProgramsEditor.cpp | 78 +++++++++++++++------------ src/gui/studio/MidiProgramsEditor.h | 8 ++- 2 files changed, 47 insertions(+), 39 deletions(-) diff --git a/src/gui/studio/MidiProgramsEditor.cpp b/src/gui/studio/MidiProgramsEditor.cpp index a44c20041..3a8b0c420 100644 --- a/src/gui/studio/MidiProgramsEditor.cpp +++ b/src/gui/studio/MidiProgramsEditor.cpp @@ -463,19 +463,14 @@ MidiProgramsEditor::slotNameChanged(const QString &programName) void MidiProgramsEditor::slotKeyMapButtonPressed() { - QToolButton *button = dynamic_cast(sender()); - - if (!button) { - RG_WARNING << "slotKeyMapButtonPressed() : WARNING: Sender is not a QToolButton."; - return; - } - if (!m_device) return; - const KeyMappingList &kml = m_device->getKeyMappings(); - if (kml.empty()) + QToolButton *button = dynamic_cast(sender()); + if (!button) { + RG_WARNING << "slotKeyMapButtonPressed(): WARNING: Sender is not a QPushButton."; return; + } const unsigned id = button->property("index").toUInt(); @@ -483,24 +478,35 @@ MidiProgramsEditor::slotKeyMapButtonPressed() if (!program) return; - m_currentMenuProgram = id; + const KeyMappingList &keyMappingList = m_device->getKeyMappings(); + if (keyMappingList.empty()) + return; + + // Save the program number we are editing for slotKeyMapMenuItemSelected(). + m_keyMapProgramNumber = id; // Create a new popup menu. QMenu *menu = new QMenu(button); const MidiKeyMapping *currentMapping = - m_device->getKeyMappingForProgram(*program); + m_device->getKeyMappingForProgram(*program); + // Keep track of the current key map selection for + // popup menu positioning. int currentKeyMap = 0; - QAction *a = menu->addAction(tr("")); - a->setObjectName("0"); + // Add the initial "". + QAction *action = menu->addAction(tr("")); + action->setObjectName("0"); - for (size_t i = 0; i < kml.size(); ++i) { - a = menu->addAction(strtoqstr(kml[i].getName())); - a->setObjectName(QString("%1").arg(i+1)); + // For each key mapping... + for (size_t i = 0; i < keyMappingList.size(); ++i) { + // Add the mapping to the menu. + action = menu->addAction(strtoqstr(keyMappingList[i].getName())); + action->setObjectName(QString("%1").arg(i+1)); - if (currentMapping && (kml[i] == *currentMapping)) + // If the current keymap for this program is found, keep track of it. + if (currentMapping && (keyMappingList[i] == *currentMapping)) currentKeyMap = static_cast(i + 1); } @@ -509,29 +515,32 @@ MidiProgramsEditor::slotKeyMapButtonPressed() // Compute the position for the pop-up menu. - // QMenu::popup() can do this for us, but it doesn't place the - // cursor over top of the current selection. + // Make sure the menu will be positioned such that the mouse pointer + // is over the currently selected item. - // Get the QRect for the current entry. QRect actionRect = menu->actionGeometry(menu->actions().value(currentKeyMap)); - - QPoint pos = QCursor::pos(); - pos.rx() -= 10; - pos.ry() -= actionRect.top() + actionRect.height() / 2; + QPoint menuPos = QCursor::pos(); + // Adjust position so that the mouse will end up on top of + // the current selection. + menuPos.rx() -= 10; + menuPos.ry() -= actionRect.top() + actionRect.height() / 2; // Display the menu. - menu->popup(pos); + menu->popup(menuPos); + + // slotKeyMapMenuItemSelected() is the next step in this process. } void -MidiProgramsEditor::slotKeyMapMenuItemSelected(QAction *a) +MidiProgramsEditor::slotKeyMapMenuItemSelected(QAction *action) { - slotKeyMapMenuItemSelected(a->objectName().toInt()); + // Extract the program number from the object name. + slotKeyMapMenuItemSelected(action->objectName().toInt()); } void -MidiProgramsEditor::slotKeyMapMenuItemSelected(int i) +MidiProgramsEditor::slotKeyMapMenuItemSelected(int programNumber) { if (!m_device) return ; @@ -540,18 +549,19 @@ MidiProgramsEditor::slotKeyMapMenuItemSelected(int i) if (kml.empty()) return ; - MidiProgram *program = getProgram(*m_currentBank, m_currentMenuProgram); + MidiProgram *program = getProgram(*m_currentBank, m_keyMapProgramNumber); if (!program) return ; std::string newMapping; - if (i == 0) { // no key mapping + if (programNumber == 0) { // no key mapping newMapping = ""; } else { - --i; - if (i < (int)kml.size()) { - newMapping = kml[i].getName(); + // Convert from 1-based program number to 0-based. + --programNumber; + if (programNumber < (int)kml.size()) { + newMapping = kml[programNumber].getName(); } } @@ -560,7 +570,7 @@ MidiProgramsEditor::slotKeyMapMenuItemSelected(int i) QIcon icon; bool haveKeyMappings = (m_device->getKeyMappings().size() > 0); //@@@ JAS restored from before port/ - QToolButton *btn = getKeyMapButton(m_currentMenuProgram); + QToolButton *btn = getKeyMapButton(m_keyMapProgramNumber); if (newMapping.empty()) { icon = IconLoader::load( "key-white" ); diff --git a/src/gui/studio/MidiProgramsEditor.h b/src/gui/studio/MidiProgramsEditor.h index 684e6886c..b9186e3a0 100644 --- a/src/gui/studio/MidiProgramsEditor.h +++ b/src/gui/studio/MidiProgramsEditor.h @@ -80,8 +80,8 @@ private slots: /// One of the program names was changed by the user. void slotNameChanged(const QString &) override; void slotKeyMapButtonPressed() override; - void slotKeyMapMenuItemSelected(QAction *); - void slotKeyMapMenuItemSelected(int); + void slotKeyMapMenuItemSelected(QAction *action); + void slotKeyMapMenuItemSelected(int programNumber); private: @@ -148,10 +148,8 @@ private slots: * * Used by slotKeyMapMenuItemSelected() to make sure the keymap * selection ends up associated with the right program. - * - * ??? rename: m_keymapButtonIndex? */ - unsigned int m_currentMenuProgram; + unsigned int m_keyMapProgramNumber; }; From a260079a038aab74f6f152d0f83a3e747bdc03b5 Mon Sep 17 00:00:00 2001 From: Ted Felix Date: Wed, 19 Jun 2024 15:43:27 -0400 Subject: [PATCH 10/17] MPE: Cleanup --- src/gui/studio/MidiProgramsEditor.cpp | 98 ++++++++++++++++----------- 1 file changed, 59 insertions(+), 39 deletions(-) diff --git a/src/gui/studio/MidiProgramsEditor.cpp b/src/gui/studio/MidiProgramsEditor.cpp index 3a8b0c420..9ff4ef2eb 100644 --- a/src/gui/studio/MidiProgramsEditor.cpp +++ b/src/gui/studio/MidiProgramsEditor.cpp @@ -54,6 +54,25 @@ namespace Rosegarden { +// Load once for performance. +static const QIcon &getNoKeyMapIcon() +{ + // Use a white icon to indicate there is no keymap for this program. + static const QIcon noKeyMapIcon(IconLoader::loadPixmap("key-white")); + + return noKeyMapIcon; +} + +// Load once for performance. +static const QIcon &getKeyMapIcon() +{ + // Use a green icon to indicate there *is* a keymap for this program. + static const QIcon keyMapIcon(IconLoader::loadPixmap("key-green")); + + return keyMapIcon; +} + + MidiProgramsEditor::MidiProgramsEditor(BankEditorDialog *bankEditor, QWidget *parent) : NameSetEditor(bankEditor, @@ -211,11 +230,6 @@ MidiProgramsEditor::populate(QTreeWidgetItem *item) // Get the programs for the current bank. ProgramList programSubset = getBankSubset(*m_currentBank); - // Use a white icon to indicate there is no keymap for this program. - static const QIcon noKeymapIcon(IconLoader::loadPixmap("key-white")); - // Use a green icon to indicate there *is* a keymap for this program. - static const QIcon keymapIcon(IconLoader::loadPixmap("key-green")); - const bool haveKeyMappings = (m_device->getKeyMappings().size() > 0); // Need this because NameSetEditor connects to each name @@ -239,7 +253,7 @@ MidiProgramsEditor::populate(QTreeWidgetItem *item) // Assume not found and clear everything. m_names[programIndex]->clear(); - keyMapButton->setIcon(noKeymapIcon); + keyMapButton->setIcon(getNoKeyMapIcon()); keyMapButton->setToolTip(""); // Find the program in programSubset... @@ -260,13 +274,13 @@ MidiProgramsEditor::populate(QTreeWidgetItem *item) m_device->getKeyMappingForProgram(midiProgram); if (midiKeyMapping) { // Indicate that this program has a keymap. - keyMapButton->setIcon(keymapIcon); + keyMapButton->setIcon(getKeyMapIcon()); // Put the name in the tool tip. keyMapButton->setToolTip(tr("Key Mapping: %1").arg( strtoqstr(midiKeyMapping->getName()))); } else { // No key mapping. // Indicate that this program has no keymap. - keyMapButton->setIcon(noKeymapIcon); + keyMapButton->setIcon(getNoKeyMapIcon()); keyMapButton->setToolTip(""); } @@ -485,7 +499,8 @@ MidiProgramsEditor::slotKeyMapButtonPressed() // Save the program number we are editing for slotKeyMapMenuItemSelected(). m_keyMapProgramNumber = id; - // Create a new popup menu. + // Create a pop-up menu filled with the available key mappings. + QMenu *menu = new QMenu(button); const MidiKeyMapping *currentMapping = @@ -535,58 +550,60 @@ MidiProgramsEditor::slotKeyMapButtonPressed() void MidiProgramsEditor::slotKeyMapMenuItemSelected(QAction *action) { - // Extract the program number from the object name. + // Extract the key map number from the object name. slotKeyMapMenuItemSelected(action->objectName().toInt()); } void -MidiProgramsEditor::slotKeyMapMenuItemSelected(int programNumber) +MidiProgramsEditor::slotKeyMapMenuItemSelected(int keyMapNumber) { + // The user has selected an item from the menu presented + // by slotKeyMapButtonPressed(). + if (!m_device) - return ; + return; - const KeyMappingList &kml = m_device->getKeyMappings(); - if (kml.empty()) - return ; + const KeyMappingList &keyMappingList = m_device->getKeyMappings(); + if (keyMappingList.empty()) + return; MidiProgram *program = getProgram(*m_currentBank, m_keyMapProgramNumber); if (!program) - return ; + return; std::string newMapping; - if (programNumber == 0) { // no key mapping + // Convert from 1-based key map number to 0-based. + // Simplifies keyMappingList[] vector access. + --keyMapNumber; + + // No key mapping? + if (keyMapNumber <= -1) { newMapping = ""; } else { - // Convert from 1-based program number to 0-based. - --programNumber; - if (programNumber < (int)kml.size()) { - newMapping = kml[programNumber].getName(); - } + if (keyMapNumber < static_cast(keyMappingList.size())) + newMapping = keyMappingList[keyMapNumber].getName(); } + // Set the key mapping. + // ??? Only the name is used? Then we need to disallow key mappings + // with empty names. The UI currently allows this. m_device->setKeyMappingForProgram(*program, newMapping); -// QString pixmapDir = KGlobal::dirs()->findResource("appdata", "pixmaps/"); - QIcon icon; - bool haveKeyMappings = (m_device->getKeyMappings().size() > 0); //@@@ JAS restored from before port/ - QToolButton *btn = getKeyMapButton(m_keyMapProgramNumber); + // Update the key mapping icon. - if (newMapping.empty()) { - icon = IconLoader::load( "key-white" ); - if( ! icon.isNull() ) { - btn->setIcon( icon ); - } - // QToolTip::remove(btn); - btn->setToolTip(QString("")); //@@@ Usefull ? + bool haveKeyMappings = (m_device->getKeyMappings().size() > 0); + QToolButton *keyMapButton = getKeyMapButton(m_keyMapProgramNumber); + + // selected? + if (keyMapNumber == -1) { + keyMapButton->setIcon(getNoKeyMapIcon()); + keyMapButton->setToolTip(""); } else { - icon = IconLoader::load( "key-green" ); - if( ! icon.isNull() ){ - btn->setIcon( icon ); - } - btn->setToolTip(tr("Key Mapping: %1").arg(strtoqstr(newMapping))); + keyMapButton->setIcon(getKeyMapIcon()); + keyMapButton->setToolTip(tr("Key Mapping: %1").arg(strtoqstr(newMapping))); } - btn->setEnabled(haveKeyMappings); + keyMapButton->setEnabled(haveKeyMappings); } int @@ -652,6 +669,9 @@ MidiProgramsEditor::banklistContains(const MidiBank &bank) MidiProgram* MidiProgramsEditor::getProgram(const MidiBank &bank, int programNo) { + // ??? Consider using getProgramIter() instead of this. Would + // that be better? Can we get rid of this? + ProgramList::iterator it = m_programList.begin(); for (; it != m_programList.end(); ++it) { From fc0e801ae67bb56ce07153a7dd48179c7298b004 Mon Sep 17 00:00:00 2001 From: Ted Felix Date: Wed, 19 Jun 2024 23:47:11 -0400 Subject: [PATCH 11/17] MPE: Cleanup --- src/gui/studio/MidiProgramsEditor.cpp | 43 ++++++++++++--------------- src/gui/studio/MidiProgramsEditor.h | 22 ++------------ 2 files changed, 22 insertions(+), 43 deletions(-) diff --git a/src/gui/studio/MidiProgramsEditor.cpp b/src/gui/studio/MidiProgramsEditor.cpp index 9ff4ef2eb..de799ae74 100644 --- a/src/gui/studio/MidiProgramsEditor.cpp +++ b/src/gui/studio/MidiProgramsEditor.cpp @@ -654,33 +654,28 @@ bool MidiProgramsEditor::banklistContains(const MidiBank &bank) { // For each bank - for (BankList::iterator it = m_bankList.begin(); - it != m_bankList.end(); - ++it) + for (const MidiBank ¤tBank : m_bankList) { // Just compare the MSB/LSB. - if (it->getMSB() == bank.getMSB() && it->getLSB() == bank.getLSB()) + // ??? MidiBank has a partialCompare() but it checks percussion as + // well. Might need that in the next version of this. + if (currentBank.getMSB() == bank.getMSB() && + currentBank.getLSB() == bank.getLSB()) return true; } return false; } -MidiProgram* +MidiProgram * MidiProgramsEditor::getProgram(const MidiBank &bank, int programNo) { - // ??? Consider using getProgramIter() instead of this. Would - // that be better? Can we get rid of this? - - ProgramList::iterator it = m_programList.begin(); - - for (; it != m_programList.end(); ++it) { - if (it->getBank().partialCompare(bank) && - it->getProgram() == programNo) { - - // Only show hits to avoid overflow of console. - RG_DEBUG << "it->getBank() " << "== bank"; - return &(*it); + for (MidiProgram &midiProgram : m_programList) { + // Match? + if (midiProgram.getBank().getMSB() == bank.getMSB() && + midiProgram.getBank().getLSB() == bank.getLSB() && + midiProgram.getProgram() == programNo) { + return &midiProgram; } } @@ -695,7 +690,8 @@ MidiProgramsEditor::getProgramIter(const MidiBank &bank, int programNo) programIter != m_programList.end(); ++programIter) { // Match? - if (programIter->getBank().partialCompare(bank) && + if (programIter->getBank().getMSB() == bank.getMSB() && + programIter->getBank().getLSB() == bank.getLSB() && programIter->getProgram() == programNo) return programIter; } @@ -704,15 +700,14 @@ MidiProgramsEditor::getProgramIter(const MidiBank &bank, int programNo) } void -MidiProgramsEditor::setBankName(const QString& s) -{ - setTitle(s); -} - -void MidiProgramsEditor::blockAllSignals(bool block) +MidiProgramsEditor::blockAllSignals(bool block) { // Blocks all LineEdit signals. + // ??? Get rid of this routine. Make sure the LineEdit controls + // do not send notifications on programmatic changes. Then + // this should no longer be necessary. + QList allChildren = findChildren((QRegularExpression)"[0-9]+"); QList::iterator it; diff --git a/src/gui/studio/MidiProgramsEditor.h b/src/gui/studio/MidiProgramsEditor.h index b9186e3a0..8271b94d9 100644 --- a/src/gui/studio/MidiProgramsEditor.h +++ b/src/gui/studio/MidiProgramsEditor.h @@ -66,13 +66,6 @@ class MidiProgramsEditor : public NameSetEditor private slots: /// Check that any new MSB/LSB combination is unique for this device. - /** - * ??? This is causing some usability concerns. It can be tricky - * to assign MSBs and LSBs when the UI keeps refusing to take - * the value you are trying to assign. Recommend reducing the - * MSB/LSB fields to a single MSB:LSB edit box. Perform - * the dupe check when the user is done editing. Tab or Enter. - */ void slotNewMSB(int value); void slotNewLSB(int value); void slotPercussionClicked(); @@ -85,12 +78,6 @@ private slots: private: - /// Set the name (title) that appears at the top of the editor. - /** - * ??? Remove this wrapper. - */ - void setBankName(const QString &s); - MidiDevice *m_device{nullptr}; // Widgets @@ -126,16 +113,13 @@ private slots: // Programs - /// Programs for the bank being edited. - /** - * ??? No, I think this is all of the programs for the device. - * Otherwise there would be no need for getBankSubset(). - */ + /// Programs for the device being edited. ProgramList &m_programList; - // Get a program (pointer into program list) for modification. + /// Get a pointer into the program list. MidiProgram *getProgram(const MidiBank &bank, int programNo); /// Get an iterator which is more flexible. ProgramList::iterator getProgramIter(const MidiBank &bank, int programNo); + /// Get a ProgramList containing only the programs in a bank. ProgramList getBankSubset(const MidiBank &); /// Set the currently loaded programs to new MSB and LSB void modifyCurrentPrograms(const MidiBank &oldBank, From 5401309c1f4e2afdb937f4361ebbc0ca7b1d5891 Mon Sep 17 00:00:00 2001 From: Ted Felix Date: Thu, 20 Jun 2024 00:08:00 -0400 Subject: [PATCH 12/17] Remove unnecessary signal blocking --- src/gui/studio/MidiKeyMappingEditor.cpp | 21 ++---------------- src/gui/studio/MidiKeyMappingEditor.h | 1 - src/gui/studio/MidiProgramsEditor.cpp | 29 ------------------------- src/gui/studio/MidiProgramsEditor.h | 5 ----- src/gui/studio/NameSetEditor.cpp | 4 +--- 5 files changed, 3 insertions(+), 57 deletions(-) diff --git a/src/gui/studio/MidiKeyMappingEditor.cpp b/src/gui/studio/MidiKeyMappingEditor.cpp index 53dbb433b..3ca92f59f 100644 --- a/src/gui/studio/MidiKeyMappingEditor.cpp +++ b/src/gui/studio/MidiKeyMappingEditor.cpp @@ -16,6 +16,7 @@ */ #define RG_MODULE_STRING "[MidiKeyMappingEditor]" +#define RG_NO_DEBUG_PRINT #include "MidiKeyMappingEditor.h" #include "NameSetEditor.h" @@ -69,8 +70,6 @@ MidiKeyMappingEditor::makeAdditionalWidget(QWidget */* parent */) void MidiKeyMappingEditor::clearAll() { - blockAllSignals(true); - for (size_t i = 0; i < m_names.size(); ++i) m_names[i]->clear(); @@ -79,9 +78,6 @@ MidiKeyMappingEditor::clearAll() m_librarian->clear(); m_librarianEmail->clear(); setEnabled(false); - - blockAllSignals(false); - } void @@ -126,8 +122,6 @@ MidiKeyMappingEditor::reset() m_mapping = *m; - blockAllSignals(true); - // Librarian details // m_librarian->setText(strtoqstr(m_device->getLibrarianName())); @@ -151,8 +145,6 @@ MidiKeyMappingEditor::reset() } } } - - blockAllSignals(false); } void @@ -177,17 +169,8 @@ MidiKeyMappingEditor::slotNameChanged(const QString &name) void MidiKeyMappingEditor::slotKeyMapButtonPressed() -{} - -void MidiKeyMappingEditor::blockAllSignals(bool block) { - QList allChildren = - findChildren((QRegularExpression)"[0-9]+"); - QList::iterator it; - - for (it = allChildren.begin(); it != allChildren.end(); ++it) { - (*it)->blockSignals(block); - } } + } diff --git a/src/gui/studio/MidiKeyMappingEditor.h b/src/gui/studio/MidiKeyMappingEditor.h index 734396557..9a595d314 100644 --- a/src/gui/studio/MidiKeyMappingEditor.h +++ b/src/gui/studio/MidiKeyMappingEditor.h @@ -54,7 +54,6 @@ public slots: protected: virtual QWidget *makeAdditionalWidget(QWidget *parent); - void blockAllSignals(bool block); //--------------- Data members --------------------------------- diff --git a/src/gui/studio/MidiProgramsEditor.cpp b/src/gui/studio/MidiProgramsEditor.cpp index de799ae74..bc1499746 100644 --- a/src/gui/studio/MidiProgramsEditor.cpp +++ b/src/gui/studio/MidiProgramsEditor.cpp @@ -174,13 +174,8 @@ MidiProgramsEditor::clearAll() m_librarian->clear(); m_librarianEmail->clear(); - // Need this because NameSetEditor connects to each name - // field's textChanged signal instead of textEdited. - // ??? Fix NameSetEditor! - blockAllSignals(true); for (size_t i = 0; i < m_names.size(); ++i) m_names[i]->clear(); - blockAllSignals(false); setEnabled(false); } @@ -232,11 +227,6 @@ MidiProgramsEditor::populate(QTreeWidgetItem *item) const bool haveKeyMappings = (m_device->getKeyMappings().size() > 0); - // Need this because NameSetEditor connects to each name - // field's textChanged signal instead of textEdited. - // ??? Fix NameSetEditor! - blockAllSignals(true); - // For each name in the program list... // programIndex is also the program change number. for (size_t programIndex = 0; programIndex < m_names.size(); ++programIndex) { @@ -287,7 +277,6 @@ MidiProgramsEditor::populate(QTreeWidgetItem *item) break; } } - blockAllSignals(false); } void @@ -699,23 +688,5 @@ MidiProgramsEditor::getProgramIter(const MidiBank &bank, int programNo) return m_programList.end(); } -void -MidiProgramsEditor::blockAllSignals(bool block) -{ - // Blocks all LineEdit signals. - - // ??? Get rid of this routine. Make sure the LineEdit controls - // do not send notifications on programmatic changes. Then - // this should no longer be necessary. - - QList allChildren = - findChildren((QRegularExpression)"[0-9]+"); - QList::iterator it; - - for (it = allChildren.begin(); it != allChildren.end(); ++it) { - (*it)->blockSignals(block); - } -} - } diff --git a/src/gui/studio/MidiProgramsEditor.h b/src/gui/studio/MidiProgramsEditor.h index 8271b94d9..dd671b574 100644 --- a/src/gui/studio/MidiProgramsEditor.h +++ b/src/gui/studio/MidiProgramsEditor.h @@ -87,11 +87,6 @@ private slots: QSpinBox *m_lsb; QFrame *initWidgets(QWidget *parent); - // ??? There's usually a way to avoid this. Like using the - // proper signals. Ones that don't fire in response to - // API calls. User interaction only. - void blockAllSignals(bool block); - // Banks /// The BankList we are editing. diff --git a/src/gui/studio/NameSetEditor.cpp b/src/gui/studio/NameSetEditor.cpp index 8b8e446c9..edf53a29e 100644 --- a/src/gui/studio/NameSetEditor.cpp +++ b/src/gui/studio/NameSetEditor.cpp @@ -158,10 +158,8 @@ NameSetEditor::NameSetEditor(BankEditorDialog *bankEditor, lineEdit->setCompleter(new QCompleter(m_completions)); m_names.push_back(lineEdit); - // ??? This should be textEdited. Then we don't need to - // block signals all the time. connect(m_names[index], - &QLineEdit::textChanged, + &QLineEdit::textEdited, this, &NameSetEditor::slotNameChanged); rowLayout->addWidget(lineEdit, 1); From 1c1976919718d1abf5be4b2625be934dd6d5aa57 Mon Sep 17 00:00:00 2001 From: Ted Felix Date: Thu, 20 Jun 2024 00:30:38 -0400 Subject: [PATCH 13/17] MPE: Cleanup --- src/gui/studio/MidiProgramsEditor.cpp | 33 ++++++++------------------- src/gui/studio/MidiProgramsEditor.h | 2 -- 2 files changed, 9 insertions(+), 26 deletions(-) diff --git a/src/gui/studio/MidiProgramsEditor.cpp b/src/gui/studio/MidiProgramsEditor.cpp index bc1499746..bb561ad54 100644 --- a/src/gui/studio/MidiProgramsEditor.cpp +++ b/src/gui/studio/MidiProgramsEditor.cpp @@ -82,16 +82,7 @@ MidiProgramsEditor::MidiProgramsEditor(BankEditorDialog *bankEditor, m_bankList(bankEditor->getBankList()), m_programList(bankEditor->getProgramList()) { - QFrame *frame = initWidgets(m_topFrame); - m_topLayout->addWidget(frame, 0, 0, 3, 3); -} - -QFrame * -MidiProgramsEditor::initWidgets(QWidget *parent) -{ - // ??? Inline this into the ctor like every other dialog. - - QFrame *frame = new QFrame(parent); + QFrame *frame = new QFrame(m_topFrame); frame->setContentsMargins(0, 0, 0, 0); QGridLayout *gridLayout = new QGridLayout(frame); @@ -131,7 +122,7 @@ MidiProgramsEditor::initWidgets(QWidget *parent) this, &MidiProgramsEditor::slotNewLSB); gridLayout->addWidget(m_lsb, 2, 1, Qt::AlignLeft); - return frame; + m_topLayout->addWidget(frame, 0, 0, 3, 3); } ProgramList @@ -514,8 +505,8 @@ MidiProgramsEditor::slotKeyMapButtonPressed() currentKeyMap = static_cast(i + 1); } - connect(menu, SIGNAL(triggered(QAction *)), - this, SLOT(slotKeyMapMenuItemSelected(QAction *))); + connect(menu, &QMenu::triggered, + this, &MidiProgramsEditor::slotKeyMapMenuItemSelected); // Compute the position for the pop-up menu. @@ -538,13 +529,6 @@ MidiProgramsEditor::slotKeyMapButtonPressed() void MidiProgramsEditor::slotKeyMapMenuItemSelected(QAction *action) -{ - // Extract the key map number from the object name. - slotKeyMapMenuItemSelected(action->objectName().toInt()); -} - -void -MidiProgramsEditor::slotKeyMapMenuItemSelected(int keyMapNumber) { // The user has selected an item from the menu presented // by slotKeyMapButtonPressed(). @@ -560,11 +544,12 @@ MidiProgramsEditor::slotKeyMapMenuItemSelected(int keyMapNumber) if (!program) return; - std::string newMapping; - - // Convert from 1-based key map number to 0-based. + // Extract the key map number from the object name. + // Subtract one to convert from 1-based key map number to 0-based. // Simplifies keyMappingList[] vector access. - --keyMapNumber; + const int keyMapNumber = action->objectName().toInt() - 1; + + std::string newMapping; // No key mapping? if (keyMapNumber <= -1) { diff --git a/src/gui/studio/MidiProgramsEditor.h b/src/gui/studio/MidiProgramsEditor.h index dd671b574..029f3ffeb 100644 --- a/src/gui/studio/MidiProgramsEditor.h +++ b/src/gui/studio/MidiProgramsEditor.h @@ -74,7 +74,6 @@ private slots: void slotNameChanged(const QString &) override; void slotKeyMapButtonPressed() override; void slotKeyMapMenuItemSelected(QAction *action); - void slotKeyMapMenuItemSelected(int programNumber); private: @@ -85,7 +84,6 @@ private slots: QCheckBox *m_percussion; QSpinBox *m_msb; QSpinBox *m_lsb; - QFrame *initWidgets(QWidget *parent); // Banks From 74c3d940fe59be00c850bb5d1c473a87304fe29b Mon Sep 17 00:00:00 2001 From: Ted Felix Date: Thu, 20 Jun 2024 09:46:22 -0400 Subject: [PATCH 14/17] MPE: Cleanup --- src/gui/studio/MidiKeyMappingEditor.cpp | 1 - src/gui/studio/MidiProgramsEditor.cpp | 81 +++++++++++++------------ src/gui/studio/NameSetEditor.cpp | 2 - src/gui/studio/NameSetEditor.h | 3 +- 4 files changed, 43 insertions(+), 44 deletions(-) diff --git a/src/gui/studio/MidiKeyMappingEditor.cpp b/src/gui/studio/MidiKeyMappingEditor.cpp index 3ca92f59f..ba21884c8 100644 --- a/src/gui/studio/MidiKeyMappingEditor.cpp +++ b/src/gui/studio/MidiKeyMappingEditor.cpp @@ -139,7 +139,6 @@ MidiKeyMappingEditor::reset() if ( (int)i == index) { QString name = strtoqstr(it->second); - m_completions << name; m_names[i]->setText(name); m_names[i]->setCursorPosition(0); } diff --git a/src/gui/studio/MidiProgramsEditor.cpp b/src/gui/studio/MidiProgramsEditor.cpp index bb561ad54..414129ac4 100644 --- a/src/gui/studio/MidiProgramsEditor.cpp +++ b/src/gui/studio/MidiProgramsEditor.cpp @@ -218,54 +218,58 @@ MidiProgramsEditor::populate(QTreeWidgetItem *item) const bool haveKeyMappings = (m_device->getKeyMappings().size() > 0); - // For each name in the program list... + // For each name field (LineEdit) on the UI... // programIndex is also the program change number. for (size_t programIndex = 0; programIndex < m_names.size(); ++programIndex) { QToolButton *keyMapButton = getKeyMapButton(programIndex); - // ??? Seems like something NameSetEditor should take care of? keyMapButton->setMaximumHeight(12); keyMapButton->setEnabled(haveKeyMappings); - // ??? Restructure this as: - // - find program in programSubset - // - if not found, clear and continue. - // - Set everything up. + bool found = false; + MidiProgram foundProgram; - // Assume not found and clear everything. - m_names[programIndex]->clear(); - keyMapButton->setIcon(getNoKeyMapIcon()); - keyMapButton->setToolTip(""); - - // Find the program in programSubset... + // Find the program in programSubset. for (const MidiProgram &midiProgram : programSubset) { - // Not it? Try the next. - if (midiProgram.getProgram() != programIndex) - continue; - - // Found it. - - QString programName = strtoqstr(midiProgram.getName()); - m_completions << programName; - m_names[programIndex]->setText(programName); - // show start of label - m_names[programIndex]->setCursorPosition(0); - - const MidiKeyMapping *midiKeyMapping = - m_device->getKeyMappingForProgram(midiProgram); - if (midiKeyMapping) { - // Indicate that this program has a keymap. - keyMapButton->setIcon(getKeyMapIcon()); - // Put the name in the tool tip. - keyMapButton->setToolTip(tr("Key Mapping: %1").arg( - strtoqstr(midiKeyMapping->getName()))); - } else { // No key mapping. - // Indicate that this program has no keymap. - keyMapButton->setIcon(getNoKeyMapIcon()); - keyMapButton->setToolTip(""); + // Found? We're done. + if (midiProgram.getProgram() == programIndex) { + found = true; + foundProgram = midiProgram; + break; } + } + + // If not found, clear and continue. + if (!found) { + m_names[programIndex]->clear(); + keyMapButton->setIcon(getNoKeyMapIcon()); + keyMapButton->setToolTip(""); + continue; + } - break; + // Found it. + + // Name + + QString programName = strtoqstr(foundProgram.getName()); + m_names[programIndex]->setText(programName); + // Show start of label. + m_names[programIndex]->setCursorPosition(0); + + // Icon and ToolTip + + const MidiKeyMapping *midiKeyMapping = + m_device->getKeyMappingForProgram(foundProgram); + if (midiKeyMapping) { + // Indicate that this program has a keymap. + keyMapButton->setIcon(getKeyMapIcon()); + // Put the name in the tool tip. + keyMapButton->setToolTip(tr("Key Mapping: %1").arg( + strtoqstr(midiKeyMapping->getName()))); + } else { // No key mapping. + // Indicate that this program has no keymap. + keyMapButton->setIcon(getNoKeyMapIcon()); + keyMapButton->setToolTip(""); } } } @@ -420,7 +424,6 @@ MidiProgramsEditor::slotNameChanged(const QString &programName) // do some digging. // ??? Recommend seeing if we can remove this and remove the op<() in // MidiBank and MidiProgram. - //std::sort(m_programList.begin(), m_programList.end(), ProgramCmp()); std::sort(m_programList.begin(), m_programList.end()); // Get the new MidiProgram from m_programList. @@ -631,8 +634,6 @@ MidiProgramsEditor::banklistContains(const MidiBank &bank) for (const MidiBank ¤tBank : m_bankList) { // Just compare the MSB/LSB. - // ??? MidiBank has a partialCompare() but it checks percussion as - // well. Might need that in the next version of this. if (currentBank.getMSB() == bank.getMSB() && currentBank.getLSB() == bank.getLSB()) return true; diff --git a/src/gui/studio/NameSetEditor.cpp b/src/gui/studio/NameSetEditor.cpp index edf53a29e..6585a1b19 100644 --- a/src/gui/studio/NameSetEditor.cpp +++ b/src/gui/studio/NameSetEditor.cpp @@ -51,7 +51,6 @@ NameSetEditor::NameSetEditor(BankEditorDialog *bankEditor, m_librarian(nullptr), m_librarianEmail(nullptr), m_names(), - m_completions(), m_numberingBaseButton(nullptr), m_numberingBase(1), m_labels(), @@ -155,7 +154,6 @@ NameSetEditor::NameSetEditor(BankEditorDialog *bankEditor, // sure why. lineEdit->setObjectName(QString("Line Edit %1").arg(index)); lineEdit->setProperty("index", index); - lineEdit->setCompleter(new QCompleter(m_completions)); m_names.push_back(lineEdit); connect(m_names[index], diff --git a/src/gui/studio/NameSetEditor.h b/src/gui/studio/NameSetEditor.h index 3c508cffc..caa4253c6 100644 --- a/src/gui/studio/NameSetEditor.h +++ b/src/gui/studio/NameSetEditor.h @@ -85,6 +85,8 @@ public slots: QWidget *parent, bool showKeyMapButtons = false); + // ??? Key mappings are only used by one deriver (MidiProgramsEditor). + // Push this down to there. QToolButton *getKeyMapButton(int n) { return m_keyMapButtons[n]; } const QToolButton *getKeyMapButton(int n) const { return m_keyMapButtons[n]; } @@ -111,7 +113,6 @@ public slots: QLabel *m_librarianEmail; std::vector m_names; - QStringList m_completions; private: QPushButton *m_numberingBaseButton; From 665f8460bf2f40194b978d35a95773f99b490df9 Mon Sep 17 00:00:00 2001 From: Ted Felix Date: Thu, 20 Jun 2024 14:25:58 -0400 Subject: [PATCH 15/17] Cleanup --- src/base/Instrument.cpp | 2 +- src/base/MidiDevice.cpp | 8 +-- src/base/MidiProgram.cpp | 4 +- src/base/MidiProgram.h | 62 ++++++++++++++----- .../MIDIInstrumentParameterPanel.cpp | 3 +- src/gui/studio/BankEditorDialog.cpp | 10 +-- src/gui/studio/MidiProgramsEditor.cpp | 20 +++--- 7 files changed, 70 insertions(+), 39 deletions(-) diff --git a/src/base/Instrument.cpp b/src/base/Instrument.cpp index b58f4a2ad..0c6ff98ca 100644 --- a/src/base/Instrument.cpp +++ b/src/base/Instrument.cpp @@ -222,7 +222,7 @@ Instrument::isProgramValid() const oBankIter != validBanks.end(); ++oBankIter) { - if (oBankIter->partialCompare(m_program.getBank())) { + if (oBankIter->compareKey(m_program.getBank())) { bankValid = true; break; } diff --git a/src/base/MidiDevice.cpp b/src/base/MidiDevice.cpp index 93dc7157d..95152b0b9 100644 --- a/src/base/MidiDevice.cpp +++ b/src/base/MidiDevice.cpp @@ -463,7 +463,7 @@ MidiDevice::getPrograms(const MidiBank &bank) const for (ProgramList::const_iterator it = m_programList.begin(); it != m_programList.end(); ++it) { - if (it->getBank().partialCompare(bank)) programs.push_back(*it); + if (it->getBank().compareKey(bank)) programs.push_back(*it); } return programs; @@ -497,7 +497,7 @@ MidiDevice::getBankName(const MidiBank &bank) const { for (BankList::const_iterator it = m_bankList.begin(); it != m_bankList.end(); ++it) { - if ((*it).partialCompare(bank)) return it->getName(); + if ((*it).compareKey(bank)) return it->getName(); } return ""; } @@ -606,7 +606,7 @@ MidiDevice::toXmlString() const // for (pt = m_programList.begin(); pt != m_programList.end(); ++pt) { - if (pt->getBank().partialCompare(*it)) + if (pt->getBank().compareKey(*it)) { midiDevice << " getProgram() << "\" " @@ -740,7 +740,7 @@ MidiDevice::mergeBankList(const BankList &bankList) { for (oIt = m_bankList.begin(); oIt != m_bankList.end(); ++oIt) { - if ((*it).partialCompare(*oIt)) + if ((*it).compareKey(*oIt)) { clash = true; break; diff --git a/src/base/MidiProgram.cpp b/src/base/MidiProgram.cpp index acc47da8f..72fabd4f7 100644 --- a/src/base/MidiProgram.cpp +++ b/src/base/MidiProgram.cpp @@ -42,7 +42,7 @@ MidiBank::operator==(const MidiBank &rhs) const } bool -MidiBank::partialCompare(const MidiBank &rhs) const +MidiBank::compareKey(const MidiBank &rhs) const { return (m_percussion == rhs.m_percussion && m_msb == rhs.m_msb && @@ -95,7 +95,7 @@ MidiProgram::MidiProgram(const MidiBank &bank, MidiByte program, const std::stri bool MidiProgram::partialCompare(const MidiProgram &rhs) const { - return m_bank.partialCompare(rhs.m_bank) && m_program == rhs.m_program; + return m_bank.compareKey(rhs.m_bank) && m_program == rhs.m_program; } bool diff --git a/src/base/MidiProgram.h b/src/base/MidiProgram.h index 630a65dca..178931379 100644 --- a/src/base/MidiProgram.h +++ b/src/base/MidiProgram.h @@ -53,7 +53,14 @@ class MidiBank /// A full comparison of all fields. /** - * This probably isn't what you want. See partialCompare(). + * MIDIInstrumentParameterPanel::updateBankComboBox() uses this to + * detect changes that might require a repopulation of the bank combobox. + * + * See MidiBank::compareKey(). + * + * ??? It's difficult to agree on what constitutes a proper comparison + * for this class. I think a complete compare is probably best + * for op==(). */ bool operator==(const MidiBank &rhs) const; bool operator!=(const MidiBank &rhs) const { return !operator==(rhs); } @@ -61,24 +68,35 @@ class MidiBank /** * Since MidiProgram stores a partial MidiBank object (without name), * a partial comparison such as this is frequently needed. + * + * ??? Would compareKey() be better? + * ??? Should this really compare percussion? I think that's wrong + * right now. */ - bool partialCompare(const MidiBank &rhs) const; - /// Less - /** - * Note that this only looks at msb and lsb. Those are the only - * things that really count when sorting. - */ - bool operator<(const MidiBank &rhs) const + bool compareKey(const MidiBank &rhs) const; + + // Only compares (percussion), msb, and lsb. + // Most useful for sorting and searching. + // This is specifically NOT operator<() because it does not compare + // all fields. + bool lessKey(const MidiBank &rhs) const { - if (m_msb == rhs.m_msb) - return (m_lsb < rhs.m_lsb); - return (m_msb < rhs.m_msb); + // ??? We'll need percussion soon. + //if (m_percussion == rhs.m_percussion) { + if (m_msb == rhs.m_msb) + return (m_lsb < rhs.m_lsb); + return (m_msb < rhs.m_msb); + //} + //return (m_percussion < rhs.m_percussion); } private: - bool m_percussion; + // Key fields. + bool m_percussion; // Not a key field yet. But soon. MidiByte m_msb; MidiByte m_lsb; + + // Data fields. std::string m_name; }; @@ -106,22 +124,32 @@ class MidiProgram // Only compares m_bank, m_program, and m_name. Does not compare // m_keyMapping. bool partialCompareWithName(const MidiProgram &rhs) const; + + // This only compares bank and program. + // Most useful for sorting and searching. + // Currently this is only used by MidiProgramsEditor for sorting. + // ??? rename: lessKey() bool operator<(const MidiProgram &rhs) { - // ??? But bank is more important than program. Why - // is this comparing program first? This makes no sense. - if (m_program == rhs.m_program) - return (m_bank < rhs.m_bank); - return (m_program < rhs.m_program); + if (m_bank.compareKey(rhs.m_bank)) + return (m_program < rhs.m_program); + return m_bank.lessKey(rhs.m_bank); } private: + // Key fields. MidiBank m_bank; MidiByte m_program; + + // Data fields. std::string m_name; std::string m_keyMapping; }; +// ??? std::vector? This means all throughout rg we have to do linear +// searches. Wouldn't a std::set<> indexed by MSB:LSB:Percussion:PC +// make a *lot* more sense in the long run? Should reduce CPU usage +// and complexity significantly. typedef std::vector ProgramList; inline bool diff --git a/src/gui/editors/parameters/MIDIInstrumentParameterPanel.cpp b/src/gui/editors/parameters/MIDIInstrumentParameterPanel.cpp index c5a90a825..045e88780 100644 --- a/src/gui/editors/parameters/MIDIInstrumentParameterPanel.cpp +++ b/src/gui/editors/parameters/MIDIInstrumentParameterPanel.cpp @@ -566,7 +566,8 @@ MIDIInstrumentParameterPanel::updateBankComboBox() // Find the selected bank in the MIDI Device's bank list. for (unsigned int i = 0; i < banks.size(); ++i) { - if (getSelectedInstrument()->getProgram().getBank().partialCompare(banks[i])) { + if (getSelectedInstrument()->getProgram().getBank().compareKey( + banks[i])) { currentBank = i; break; } diff --git a/src/gui/studio/BankEditorDialog.cpp b/src/gui/studio/BankEditorDialog.cpp index 4e2a2022c..22d4bc1ac 100644 --- a/src/gui/studio/BankEditorDialog.cpp +++ b/src/gui/studio/BankEditorDialog.cpp @@ -1172,10 +1172,10 @@ BankEditorDialog::slotDelete() it != m_programList.end(); ++it) { // If this program isn't in the bank that is being deleted, - // add it to the new program list. We use partialCompare() + // add it to the new program list. We use compareKey() // because the MidiBank objects in the program list do not // have their name fields filled in. - if (!it->getBank().partialCompare(bank)) + if (!it->getBank().compareKey(bank)) newProgramList.push_back(*it); } @@ -1691,7 +1691,7 @@ BankEditorDialog::slotEditPaste() // Remove programs that will be overwritten // for (it = m_programList.begin(); it != m_programList.end(); ++it) { - if (!(it->getBank().partialCompare(m_lastBank))) + if (!(it->getBank().compareKey(m_lastBank))) tempProg.push_back(*it); } m_programList = tempProg; @@ -1704,7 +1704,7 @@ BankEditorDialog::slotEditPaste() // Add the new programs // for (it = tempProg.begin(); it != tempProg.end(); ++it) { - if (it->getBank().partialCompare(sourceBank)) { + if (it->getBank().compareKey(sourceBank)) { // Insert with new MSB and LSB // MidiProgram copyProgram(m_lastBank, @@ -1895,7 +1895,7 @@ bool BankEditorDialog::tracksUsingBank(const MidiBank& bank, const MidiProgram& program = instrument->getProgram(); const MidiBank& ibank = program.getBank(); - if (bank.partialCompare(ibank)) { + if (bank.compareKey(ibank)) { // Found a Track using this bank trackPositions.push_back(track->getPosition()); } diff --git a/src/gui/studio/MidiProgramsEditor.cpp b/src/gui/studio/MidiProgramsEditor.cpp index 414129ac4..70aadf930 100644 --- a/src/gui/studio/MidiProgramsEditor.cpp +++ b/src/gui/studio/MidiProgramsEditor.cpp @@ -132,7 +132,7 @@ MidiProgramsEditor::getBankSubset(const MidiBank &bank) // For each program, copy the ones for the requested bank to programList. for (const MidiProgram &program : m_programList) { - if (program.getBank().partialCompare(bank)) + if (program.getBank().compareKey(bank)) programList.push_back(program); } @@ -146,7 +146,7 @@ MidiProgramsEditor::modifyCurrentPrograms( // For each program in m_programList... for (MidiProgram &program : m_programList) { // If this one is in the old bank, update it to the new. - if (program.getBank().partialCompare(oldBank)) + if (program.getBank().compareKey(oldBank)) program = MidiProgram( newBank, program.getProgram(), program.getName()); } @@ -416,14 +416,16 @@ MidiProgramsEditor::slotNameChanged(const QString &programName) m_programList.push_back(newProgram); // Sort m_programList. - // ??? Why do we need this sorted? Is it a Device requirement? - // If it is a requirement, is there another std::sort() someplace? - // Confusing. And MidiProgram's op< is backwards. It is + // We need to sort this for the MIPP. It just needs the PCs in + // order. That's how it displays them. + // If we do not sort this, the .rg file is also mixed up. But + // that's not a serious issue since no one looks at that. + // ??? MidiProgram's op< is backwards. It is // comparing program and then bank. But bank is more important. - // I have a feeling sorting and the op< are unnecessary. Need to - // do some digging. - // ??? Recommend seeing if we can remove this and remove the op<() in - // MidiBank and MidiProgram. + // ??? Is there a better place to sort this? What if we take + // the first steps toward moving to std::set? Make it a + // std::set, but continue using it like a vector. That + // way it is always sorted. std::sort(m_programList.begin(), m_programList.end()); // Get the new MidiProgram from m_programList. From b03890529a1f85a92404379dcff9c3574bad8280 Mon Sep 17 00:00:00 2001 From: Ted Felix Date: Thu, 20 Jun 2024 15:54:26 -0400 Subject: [PATCH 16/17] Cleanup --- src/base/MidiProgram.h | 12 +++--------- src/gui/studio/MidiProgramsEditor.cpp | 6 +++--- 2 files changed, 6 insertions(+), 12 deletions(-) diff --git a/src/base/MidiProgram.h b/src/base/MidiProgram.h index 178931379..75c620083 100644 --- a/src/base/MidiProgram.h +++ b/src/base/MidiProgram.h @@ -57,21 +57,16 @@ class MidiBank * detect changes that might require a repopulation of the bank combobox. * * See MidiBank::compareKey(). - * - * ??? It's difficult to agree on what constitutes a proper comparison - * for this class. I think a complete compare is probably best - * for op==(). */ bool operator==(const MidiBank &rhs) const; bool operator!=(const MidiBank &rhs) const { return !operator==(rhs); } - /// Compare all fields except name. + /// Compare MSB:LSB:Percussion. /** * Since MidiProgram stores a partial MidiBank object (without name), * a partial comparison such as this is frequently needed. * - * ??? Would compareKey() be better? * ??? Should this really compare percussion? I think that's wrong - * right now. + * right now. See if we can cause bugs with it. Then fix it. */ bool compareKey(const MidiBank &rhs) const; @@ -128,8 +123,7 @@ class MidiProgram // This only compares bank and program. // Most useful for sorting and searching. // Currently this is only used by MidiProgramsEditor for sorting. - // ??? rename: lessKey() - bool operator<(const MidiProgram &rhs) + bool lessKey(const MidiProgram &rhs) const { if (m_bank.compareKey(rhs.m_bank)) return (m_program < rhs.m_program); diff --git a/src/gui/studio/MidiProgramsEditor.cpp b/src/gui/studio/MidiProgramsEditor.cpp index 70aadf930..b7566ceb5 100644 --- a/src/gui/studio/MidiProgramsEditor.cpp +++ b/src/gui/studio/MidiProgramsEditor.cpp @@ -420,13 +420,13 @@ MidiProgramsEditor::slotNameChanged(const QString &programName) // order. That's how it displays them. // If we do not sort this, the .rg file is also mixed up. But // that's not a serious issue since no one looks at that. - // ??? MidiProgram's op< is backwards. It is - // comparing program and then bank. But bank is more important. // ??? Is there a better place to sort this? What if we take // the first steps toward moving to std::set? Make it a // std::set, but continue using it like a vector. That // way it is always sorted. - std::sort(m_programList.begin(), m_programList.end()); + std::sort(m_programList.begin(), + m_programList.end(), + [](const MidiProgram &lhs, const MidiProgram &rhs){ return lhs.lessKey(rhs); }); // Get the new MidiProgram from m_programList. programIter = getProgramIter(*m_currentBank, programNumber); From 257a68c5e2d395e2c21236c22957f9f6ee1a6a3a Mon Sep 17 00:00:00 2001 From: Ted Felix Date: Thu, 20 Jun 2024 22:49:36 -0400 Subject: [PATCH 17/17] Comments --- src/gui/studio/MidiProgramsEditor.cpp | 27 +++++++++++++++++++++++---- src/gui/studio/MidiProgramsEditor.h | 1 - 2 files changed, 23 insertions(+), 5 deletions(-) diff --git a/src/gui/studio/MidiProgramsEditor.cpp b/src/gui/studio/MidiProgramsEditor.cpp index b7566ceb5..6491f2e0e 100644 --- a/src/gui/studio/MidiProgramsEditor.cpp +++ b/src/gui/studio/MidiProgramsEditor.cpp @@ -382,7 +382,8 @@ MidiProgramsEditor::slotNameChanged(const QString &programName) // "h" and again with "hi". // ??? Can we be more efficient? E.g. only make the change when - // the cursor leaves the edit box, or "Ok" is clicked? + // the cursor leaves the edit box, or "Ok" is clicked? That + // would help with the command history spam issue. if (!m_currentBank) { RG_WARNING << "slotNameChanged(): WARNING: m_currentBank is nullptr."; @@ -437,7 +438,20 @@ MidiProgramsEditor::slotNameChanged(const QString &programName) if (programName.isEmpty()) { //RG_DEBUG << "slotNameChanged(): deleting empty program (" << programNumber << ")"; m_programList.erase(programIter); - // ??? Why? + // Call the parent's slotApply() to make the change to the + // document. + // ??? Command History SPAM. + // ??? This creates a command and adds it to the history. + // Are we seeing a ton of things appear in the undo history? + // Yes! This spams the undo history until it is useless. + // And there's no way to undo from within the editor. So + // adding to the command history seems pointless. + // ??? We should only call this on switching banks or + // closing the BankEditorDialog. We need to review + // BankEditorDialog to make sure this is the best solution. + // ??? Other parts of the editor call this for an update. But + // program name changes do not appear on the tree. So an + // update of any kind has no value. m_bankEditor->slotApply(); return; @@ -454,7 +468,12 @@ MidiProgramsEditor::slotNameChanged(const QString &programName) // If the name has actually changed if (qstrtostr(programName) != programIter->getName()) { programIter->setName(qstrtostr(programName)); - // ??? Why? + // Call the parent's slotApply() to make the change to the + // document. + // ??? Command History SPAM. See comments above. + // ??? We should only call this on switching banks or + // closing the BankEditorDialog. We need to review + // BankEditorDialog to make sure this is the best solution. m_bankEditor->slotApply(); } } @@ -566,7 +585,7 @@ MidiProgramsEditor::slotKeyMapMenuItemSelected(QAction *action) // Set the key mapping. // ??? Only the name is used? Then we need to disallow key mappings - // with empty names. The UI currently allows this. + // with empty names. BankEditorDialog currently allows this. m_device->setKeyMappingForProgram(*program, newMapping); // Update the key mapping icon. diff --git a/src/gui/studio/MidiProgramsEditor.h b/src/gui/studio/MidiProgramsEditor.h index 029f3ffeb..d0d1fe852 100644 --- a/src/gui/studio/MidiProgramsEditor.h +++ b/src/gui/studio/MidiProgramsEditor.h @@ -1,4 +1,3 @@ - /* -*- c-basic-offset: 4 indent-tabs-mode: nil -*- vi:set ts=8 sts=4 sw=4: */ /*