Support the new editing style for rehearsal signs.

This is similar to the changes in 56e5ba8 for tempo markers. - Missing switch cases are now compiler errors to flag this issue more easily when adding new enums. - Fixed a bug where using the menu item / keyboard shortcut to add a new item actually enabled the "remove" flag, since Qt forwards the "checked" argument to the slot. An explicit lambda is now used. - Clear the selection after editing an item. Bug: #23, #192, #220
powertab · Jan 11, 2021 · ec1006e · ec1006e
1 parent 8dd950b
commit ec1006e
Show file tree

Hide file tree

Showing 11 changed files with 167 additions and 54 deletions.
diff --git a/cmake/PTE_CompilerFlags.cmake b/cmake/PTE_CompilerFlags.cmake
@@ -15,7 +15,7 @@ function ( pte_add_compile_flags target )
     if ( PLATFORM_WIN )
         target_compile_definitions( ${target} PRIVATE -DNOMINMAX -D_SCL_SECURE_NO_WARNINGS )
     else ()
-        target_compile_options( ${target} PRIVATE -Wall -Wnon-virtual-dtor -Wextra )
+        target_compile_options( ${target} PRIVATE -Wall -Wnon-virtual-dtor -Wextra -Werror=switch )
 
         if ( PLATFORM_OSX )
             target_compile_options( ${target} PRIVATE -stdlib=libc++ )

diff --git a/source/actions/addrehearsalsign.cpp b/source/actions/addrehearsalsign.cpp
@@ -30,7 +30,8 @@ AddRehearsalSign::AddRehearsalSign(const ScoreLocation &location,
 
 void AddRehearsalSign::redo()
 {
-    myLocation.getBarline()->setRehearsalSign(RehearsalSign("A", myDescription));
+    myLocation.getBarline()->setRehearsalSign(
+        RehearsalSign("A", myDescription));
     ScoreUtils::adjustRehearsalSigns(myLocation.getScore());
 }
 
@@ -39,3 +40,24 @@ void AddRehearsalSign::undo()
     myLocation.getBarline()->clearRehearsalSign();
     ScoreUtils::adjustRehearsalSigns(myLocation.getScore());
 }
+
+EditRehearsalSign::EditRehearsalSign(const ScoreLocation &location,
+                                     const std::string &new_description)
+    : QUndoCommand(QObject::tr("Edit Rehearsal Sign")),
+      myLocation(location),
+      myNewDescription(new_description),
+      myOrigSign(location.getBarline()->getRehearsalSign())
+{
+}
+
+void EditRehearsalSign::redo()
+{
+    RehearsalSign sign = myOrigSign;
+    sign.setDescription(myNewDescription);
+    myLocation.getBarline()->setRehearsalSign(sign);
+}
+
+void EditRehearsalSign::undo()
+{
+    myLocation.getBarline()->setRehearsalSign(myOrigSign);
+}
diff --git a/source/actions/addrehearsalsign.h b/source/actions/addrehearsalsign.h
@@ -19,13 +19,15 @@
 #define ACTIONS_ADDREHEARSALSIGN_H
 
 #include <QUndoCommand>
+#include <score/rehearsalsign.h>
 #include <score/scorelocation.h>
 
+/// Adds a new rehearsal sign.
 class AddRehearsalSign : public QUndoCommand
 {
 public:
     AddRehearsalSign(const ScoreLocation &location,
-                     const std::string &description);
+                      const std::string &description);
 
     virtual void redo() override;
     virtual void undo() override;
@@ -35,4 +37,20 @@ class AddRehearsalSign : public QUndoCommand
     const std::string myDescription;
 };
 
+/// Edits an existing rehearsal sign.
+class EditRehearsalSign : public QUndoCommand
+{
+public:
+    EditRehearsalSign(const ScoreLocation &location,
+                      const std::string &new_description);
+
+    virtual void redo() override;
+    virtual void undo() override;
+
+private:
+    ScoreLocation myLocation;
+    const std::string myNewDescription;
+    RehearsalSign myOrigSign;
+};
+
 #endif
diff --git a/source/app/powertabeditor.cpp b/source/app/powertabeditor.cpp
@@ -779,6 +779,10 @@ void PowerTabEditor::removeSelectedItem()
             editTempoMarker(/* remove */ true);
             break;
 
+        case ScoreItem::RehearsalSign:
+            editRehearsalSign(/* remove */ true);
+            break;
+
         case ScoreItem::Staff:
         {
             auto location = getLocation();
@@ -1232,30 +1236,41 @@ void PowerTabEditor::editMultiBarRest()
     }
 }
 
-void PowerTabEditor::editRehearsalSign()
+void
+PowerTabEditor::editRehearsalSign(bool remove)
 {
     const ScoreLocation &location = getLocation();
     const Barline *barline = location.getBarline();
     Q_ASSERT(barline);
 
-    if (barline->hasRehearsalSign())
+    if (remove)
     {
+        Q_ASSERT(barline->hasRehearsalSign());
         myUndoManager->push(new RemoveRehearsalSign(location),
                             UndoManager::AFFECTS_ALL_SYSTEMS);
+        return;
     }
-    else
-    {
-        RehearsalSignDialog dialog(this);
 
-        if (dialog.exec() == QDialog::Accepted)
+    auto sign =
+        barline->hasRehearsalSign() ? &barline->getRehearsalSign() : nullptr;
+    RehearsalSignDialog dialog(this, sign ? sign->getDescription() : "");
+    if (dialog.exec() == QDialog::Accepted)
+    {
+        if (sign)
         {
-            myUndoManager->push(new AddRehearsalSign(location,
-                                                     dialog.getDescription()),
-                                UndoManager::AFFECTS_ALL_SYSTEMS);
+            myUndoManager->push(
+                new EditRehearsalSign(location, dialog.getDescription()),
+                location.getSystemIndex());
         }
         else
-            myRehearsalSignCommand->setChecked(false);
+        {
+            myUndoManager->push(
+                new AddRehearsalSign(location, dialog.getDescription()),
+                UndoManager::AFFECTS_ALL_SYSTEMS);
+        }
     }
+    else
+        myRehearsalSignCommand->setChecked(false);
 }
 
 void PowerTabEditor::editTempoMarker(bool remove)
@@ -1264,8 +1279,9 @@ void PowerTabEditor::editTempoMarker(bool remove)
     const TempoMarker *marker = ScoreUtils::findByPosition(
         location.getSystem().getTempoMarkers(), location.getPositionIndex());
 
-    if (remove && marker)
+    if (remove)
     {
+        Q_ASSERT(marker);
         myUndoManager->push(new RemoveTempoMarker(location),
                             location.getSystemIndex());
         return;
@@ -2419,14 +2435,14 @@ void PowerTabEditor::createCommands()
                     Qt::SHIFT + Qt::Key_R, this);
     myRehearsalSignCommand->setCheckable(true);
     connect(myRehearsalSignCommand, &QAction::triggered, this,
-            &PowerTabEditor::editRehearsalSign);
+            [=]() { editRehearsalSign(); });
 
     myTempoMarkerCommand = new Command(
         tr("Tempo Marker..."), "MusicSymbols.TempoMarker", Qt::Key_O, this,
         QStringLiteral(u":images/tempo.png"));
     myTempoMarkerCommand->setCheckable(true);
     connect(myTempoMarkerCommand, &QAction::triggered, this,
-            &PowerTabEditor::editTempoMarker);
+            [=]() { editTempoMarker(); });
 
     myAlterationOfPaceCommand =
         new Command(tr("Alteration of Pace..."),
@@ -3273,13 +3289,17 @@ void PowerTabEditor::setupNewTab()
                                 getCaret().moveToLocation(location);
                                 break;
                             default:
+                                // Do nothing.
                                 break;
                         }
                         break;
 
                     case ScoreItemAction::DoubleClicked:
                         switch (item)
                         {
+                            case ScoreItem::Staff:
+                                // Do nothing.
+                                break;
                             case ScoreItem::Barline:
                                 editBarline();
                                 break;
@@ -3296,9 +3316,16 @@ void PowerTabEditor::setupNewTab()
                             case ScoreItem::TempoMarker:
                                 editTempoMarker();
                                 break;
-                            default:
+                            case ScoreItem::RehearsalSign:
+                                editRehearsalSign();
                                 break;
                         }
+
+                        // Clear the selection after editing. We may have also
+                        // triggered a redraw anyways that cleared the graphics
+                        // scene selection.
+                        getScoreArea()->clearSelection();
+                        getCaret().setSelectedItem(ScoreItem::Staff);
                         break;
                 }
 
@@ -3369,6 +3396,23 @@ inline void updateNoteProperty(Command *command, const Note *note,
     command->setEnabled(note != nullptr);
     command->setChecked(note && note->hasProperty(property));
 }
+
+bool
+canDeleteItem(ScoreItem item)
+{
+    switch (item)
+    {
+        case ScoreItem::TempoMarker:
+        case ScoreItem::RehearsalSign:
+            return true;
+        case ScoreItem::Staff:
+        case ScoreItem::Barline:
+        case ScoreItem::TimeSignature:
+        case ScoreItem::KeySignature:
+        case ScoreItem::Clef:
+            return false;
+    }
+}
 }
 
 void PowerTabEditor::updateCommands()
@@ -3398,9 +3442,6 @@ void PowerTabEditor::updateCommands()
         ScoreUtils::findByPosition(staff.getDynamics(), position);
     const bool positions_selected = !location.getSelectedPositions().empty();
 
-    const bool item_selected =
-        getCaret().getSelectedItem() == ScoreItem::TempoMarker;
-
     myRemoveCurrentSystemCommand->setEnabled(score.getSystems().size() > 1);
     myRemoveCurrentStaffCommand->setEnabled(system.getStaves().size() > 1);
     myIncreaseLineSpacingCommand->setEnabled(score.getLineSpacing() <
@@ -3409,8 +3450,9 @@ void PowerTabEditor::updateCommands()
                                              Score::MIN_LINE_SPACING);
     myRemoveSpaceCommand->setEnabled(!pos && (position == 0 || !barline) &&
                                      !tempoMarker && !altEnding && !dynamic);
-    myRemoveItemCommand->setEnabled(pos || barline || positions_selected ||
-                                    item_selected);
+    myRemoveItemCommand->setEnabled(
+        pos || barline || positions_selected ||
+        canDeleteItem(getCaret().getSelectedItem()));
     myRemovePositionCommand->setEnabled(pos || barline || positions_selected);
 
     myChordNameCommand->setChecked(

diff --git a/source/app/powertabeditor.h b/source/app/powertabeditor.h
@@ -206,7 +206,7 @@ private slots:
     void editMultiBarRest();
 
     /// Adds or removes a rehearsal sign at the current barline.
-    void editRehearsalSign();
+    void editRehearsalSign(bool remove = false);
     /// Adds or removes a tempo marker at the current position.
     void editTempoMarker(bool remove = false);
     /// Adds or removes an accel/rit symbol at the current position.

diff --git a/source/dialogs/rehearsalsigndialog.cpp b/source/dialogs/rehearsalsigndialog.cpp
@@ -21,9 +21,9 @@
 #include <QCompleter>
 #include <QMessageBox>
 
-RehearsalSignDialog::RehearsalSignDialog(QWidget *parent)
-    : QDialog(parent),
-      ui(new Ui::RehearsalSignDialog)
+RehearsalSignDialog::RehearsalSignDialog(QWidget *parent,
+                                         const std::string &description)
+    : QDialog(parent), ui(new Ui::RehearsalSignDialog)
 {
     ui->setupUi(this);
 
@@ -32,6 +32,8 @@ RehearsalSignDialog::RehearsalSignDialog(QWidget *parent)
 
     populateDescriptionChoices();
     ui->descriptionComboBox->clearEditText();
+    ui->descriptionComboBox->setCurrentText(
+        QString::fromStdString(description));
 }
 
 RehearsalSignDialog::~RehearsalSignDialog()
@@ -60,9 +62,7 @@ void RehearsalSignDialog::populateDescriptionChoices()
 
 void RehearsalSignDialog::accept()
 {
-    std::string description = ui->descriptionComboBox->currentText().toStdString();
-
-    if (description.empty())
+    if (getDescription().empty())
     {
         QMessageBox(QMessageBox::Warning, tr("Rehearsal Sign"),
                     tr("The Rehearsal Sign description cannot be empty.")).exec();

diff --git a/source/dialogs/rehearsalsigndialog.h b/source/dialogs/rehearsalsigndialog.h
@@ -29,7 +29,7 @@ class RehearsalSignDialog : public QDialog
     Q_OBJECT
 
 public:
-    RehearsalSignDialog(QWidget *parent);
+    RehearsalSignDialog(QWidget *parent, const std::string &description);
     ~RehearsalSignDialog();
 
     /// Returns the description of the rehearsal sign that was entered by

diff --git a/source/painters/scoreclickevent.h b/source/painters/scoreclickevent.h
@@ -30,7 +30,8 @@ enum class ScoreItem
     KeySignature,
     TimeSignature,
     Clef,
-    TempoMarker
+    TempoMarker,
+    RehearsalSign
 };
 
 enum class ScoreItemAction