Merge pull request #25441 from cbjeukendrup/instrumentspanel/replace_…

…instrument_crash Fix crash when replacing instrument via popup in instruments panel
musescore · Nov 20, 2024 · 1dce053 · 1dce053
2 parents fd97bf1 + 6102fc3
commit 1dce053
Show file tree

Hide file tree

Showing 7 changed files with 78 additions and 82 deletions.
diff --git a/src/instrumentsscene/qml/MuseScore/InstrumentsScene/internal/InstrumentSettingsPopup.qml b/src/instrumentsscene/qml/MuseScore/InstrumentsScene/internal/InstrumentSettingsPopup.qml
@@ -30,6 +30,9 @@ StyledPopupView {
 
     property bool needActiveFirstItem: false
 
+    signal replaceInstrumentRequested()
+    signal resetAllFormattingRequested()
+
     contentHeight: contentColumn.childrenRect.height
 
     onOpened: {
@@ -113,9 +116,8 @@ StyledPopupView {
             visible: settingsModel.isMainScore
 
             onClicked: {
+                root.replaceInstrumentRequested()
                 root.close()
-
-                settingsModel.replaceInstrument()
             }
         }
 
@@ -130,9 +132,8 @@ StyledPopupView {
             visible: !settingsModel.isMainScore
 
             onClicked: {
+                root.resetAllFormattingRequested()
                 root.close()
-
-                settingsModel.resetAllFormatting()
             }
         }
     }

diff --git a/src/instrumentsscene/qml/MuseScore/InstrumentsScene/internal/InstrumentsTreeItemDelegate.qml b/src/instrumentsscene/qml/MuseScore/InstrumentsScene/internal/InstrumentsTreeItemDelegate.qml
@@ -132,10 +132,10 @@ FocusableControl {
     Loader {
         id: popupLoader
 
-        property alias openedPopup: popupLoader.item
-        property bool isPopupOpened: Boolean(openedPopup) && openedPopup.isOpened
+        readonly property StyledPopupView openedPopup: popupLoader.item as StyledPopupView
+        readonly property bool isPopupOpened: Boolean(openedPopup) && openedPopup.isOpened
 
-        function openPopup(comp, btn, item) {
+        function openPopup(comp: Component, btn: Item, item) {
             popupLoader.sourceComponent = comp
             if (!openedPopup) {
                 return
@@ -152,10 +152,7 @@ FocusableControl {
 
             openedPopup.closed.connect(function() {
                 root.popupClosed()
-
-                Qt.callLater(function() {
-                    popupLoader.sourceComponent = null
-                })
+                sourceComponent = null
             })
 
             openedPopup.open()
@@ -173,6 +170,22 @@ FocusableControl {
 
         InstrumentSettingsPopup {
             anchorItem: popupAnchorItem
+
+            onReplaceInstrumentRequested: {
+                // The popup would close when the dialog to select the new
+                // instrument is shown; when it closes, it is unloaded, i.e.
+                // deleted, which means that it is deleted while a signal
+                // handler inside it is being executed. This causes a crash.
+                // To prevent that, let the popup close itself, and perform the
+                // actual operation "later", i.e. not (directly or indirectly)
+                // inside the signal handler in the popup.
+                Qt.callLater(model.itemRole.replaceInstrument)
+            }
+
+            onResetAllFormattingRequested: {
+                // Same as above
+                Qt.callLater(model.itemRole.resetAllFormatting)
+            }
         }
     }
 

diff --git a/src/instrumentsscene/view/instrumentsettingsmodel.cpp b/src/instrumentsscene/view/instrumentsettingsmodel.cpp
@@ -21,9 +21,6 @@
  */
 #include "instrumentsettingsmodel.h"
 
-#include "log.h"
-#include "translation.h"
-
 using namespace muse;
 using namespace mu::instrumentsscene;
 using namespace mu::notation;
@@ -59,50 +56,6 @@ void InstrumentSettingsModel::load(const QVariant& instrument)
     emit dataChanged();
 }
 
-void InstrumentSettingsModel::replaceInstrument()
-{
-    if (!masterNotationParts()) {
-        return;
-    }
-
-    RetVal<Instrument> selectedInstrument = selectInstrumentsScenario()->selectInstrument(m_instrumentKey);
-    if (!selectedInstrument.ret) {
-        LOGE() << selectedInstrument.ret.toString();
-        return;
-    }
-
-    const Instrument& newInstrument = selectedInstrument.val;
-    masterNotationParts()->replaceInstrument(m_instrumentKey, newInstrument);
-
-    m_instrumentKey.instrumentId = newInstrument.id();
-    m_instrumentName = newInstrument.nameAsPlainText();
-    m_instrumentAbbreviature = newInstrument.abbreviatureAsPlainText();
-
-    emit dataChanged();
-}
-
-void InstrumentSettingsModel::resetAllFormatting()
-{
-    if (!masterNotationParts() || !notationParts()) {
-        return;
-    }
-
-    std::string title = muse::trc("instruments", "Are you sure you want to reset all formatting?");
-    std::string body = muse::trc("instruments", "This action can not be undone");
-
-    IInteractive::Button button = interactive()->question(title, body, {
-        IInteractive::Button::No,
-        IInteractive::Button::Yes
-    }).standardButton();
-
-    if (button == IInteractive::Button::No) {
-        return;
-    }
-
-    const Part* masterPart = masterNotationParts()->part(m_instrumentKey.partId);
-    notationParts()->replacePart(m_instrumentKey.partId, masterPart->clone());
-}
-
 QString InstrumentSettingsModel::instrumentName() const
 {
     return m_instrumentName;
@@ -154,9 +107,3 @@ INotationPartsPtr InstrumentSettingsModel::notationParts() const
     INotationPtr notation = currentNotation();
     return notation ? notation->parts() : nullptr;
 }
-
-INotationPartsPtr InstrumentSettingsModel::masterNotationParts() const
-{
-    INotationPtr notation = currentMasterNotation();
-    return notation ? notation->parts() : nullptr;
-}
diff --git a/src/instrumentsscene/view/instrumentsettingsmodel.h b/src/instrumentsscene/view/instrumentsettingsmodel.h
@@ -28,8 +28,6 @@
 
 #include "modularity/ioc.h"
 #include "context/iglobalcontext.h"
-#include "notation/iselectinstrumentscenario.h"
-#include "global/iinteractive.h"
 
 #include "notation/notationtypes.h"
 
@@ -38,9 +36,7 @@ class InstrumentSettingsModel : public QObject, public muse::async::Asyncable
 {
     Q_OBJECT
 
-    INJECT(notation::ISelectInstrumentsScenario, selectInstrumentsScenario)
     INJECT(context::IGlobalContext, context)
-    INJECT(muse::IInteractive, interactive)
 
     Q_PROPERTY(QString instrumentName READ instrumentName WRITE setInstrumentName NOTIFY dataChanged)
     Q_PROPERTY(QString abbreviature READ abbreviature WRITE setAbbreviature NOTIFY dataChanged)
@@ -51,8 +47,6 @@ class InstrumentSettingsModel : public QObject, public muse::async::Asyncable
     explicit InstrumentSettingsModel(QObject* parent = nullptr);
 
     Q_INVOKABLE void load(const QVariant& instrument);
-    Q_INVOKABLE void replaceInstrument();
-    Q_INVOKABLE void resetAllFormatting();
 
     QString instrumentName() const;
     QString abbreviature() const;
@@ -71,7 +65,6 @@ public slots:
     notation::INotationPtr currentNotation() const;
     notation::INotationPtr currentMasterNotation() const;
     notation::INotationPartsPtr notationParts() const;
-    notation::INotationPartsPtr masterNotationParts() const;
 
     notation::InstrumentKey m_instrumentKey;
     QString m_instrumentName;

diff --git a/src/instrumentsscene/view/instrumentspaneltreemodel.cpp b/src/instrumentsscene/view/instrumentspaneltreemodel.cpp
@@ -324,7 +324,7 @@ void InstrumentsPanelTreeModel::load()
     beginResetModel();
     deleteItems();
 
-    m_rootItem = new RootTreeItem(m_masterNotation, m_notation);
+    m_rootItem = new RootTreeItem(m_masterNotation, m_notation, this);
 
     async::NotifyList<const Part*> masterParts = m_masterNotation->parts()->partList();
     sortParts(masterParts);

diff --git a/src/instrumentsscene/view/parttreeitem.cpp b/src/instrumentsscene/view/parttreeitem.cpp
@@ -30,7 +30,7 @@ using namespace muse;
 using ItemType = InstrumentsTreeItemType::ItemType;
 
 PartTreeItem::PartTreeItem(IMasterNotationPtr masterNotation, INotationPtr notation, QObject* parent)
-    : AbstractInstrumentsPanelTreeItem(ItemType::PART, masterNotation, notation, parent)
+    : AbstractInstrumentsPanelTreeItem(ItemType::PART, masterNotation, notation, parent), Injectable(iocCtxForQmlObject(this))
 {
     listenVisibilityChanged();
 }
@@ -133,11 +133,6 @@ size_t PartTreeItem::resolveNewPartIndex(const ID& partId) const
     return parts.size();
 }
 
-QString PartTreeItem::instrumentId() const
-{
-    return m_instrumentId;
-}
-
 MoveParams PartTreeItem::buildMoveParams(int sourceRow, int count, AbstractInstrumentsPanelTreeItem* destinationParent,
                                          int destinationRow) const
 {
@@ -192,3 +187,43 @@ void PartTreeItem::removeChildren(int row, int count, bool deleteChild)
 
     AbstractInstrumentsPanelTreeItem::removeChildren(row, count, deleteChild);
 }
+
+QString PartTreeItem::instrumentId() const
+{
+    return m_instrumentId;
+}
+
+void PartTreeItem::replaceInstrument()
+{
+    InstrumentKey instrumentKey;
+    instrumentKey.partId = id();
+    instrumentKey.instrumentId = m_instrumentId;
+    instrumentKey.tick = Part::MAIN_INSTRUMENT_TICK;
+
+    RetVal<Instrument> selectedInstrument = selectInstrumentsScenario()->selectInstrument(instrumentKey);
+    if (!selectedInstrument.ret) {
+        LOGE() << selectedInstrument.ret.toString();
+        return;
+    }
+
+    const Instrument& newInstrument = selectedInstrument.val;
+    masterNotation()->parts()->replaceInstrument(instrumentKey, newInstrument);
+}
+
+void PartTreeItem::resetAllFormatting()
+{
+    std::string title = muse::trc("instruments", "Are you sure you want to reset all formatting?");
+    std::string body = muse::trc("instruments", "This action can not be undone");
+
+    IInteractive::Button button = interactive()->question(title, body, {
+        IInteractive::Button::No,
+        IInteractive::Button::Yes
+    }).standardButton();
+
+    if (button != IInteractive::Button::Yes) {
+        return;
+    }
+
+    const Part* masterPart = masterNotation()->parts()->part(id());
+    notation()->parts()->replacePart(id(), masterPart->clone());
+}
diff --git a/src/instrumentsscene/view/parttreeitem.h b/src/instrumentsscene/view/parttreeitem.h
@@ -24,22 +24,25 @@
 
 #include "abstractinstrumentspaneltreeitem.h"
 
-#include "notation/inotationparts.h"
+#include "modularity/ioc.h"
+#include "iinteractive.h"
+#include "notation/iselectinstrumentscenario.h"
 
 namespace mu::instrumentsscene {
-class PartTreeItem : public AbstractInstrumentsPanelTreeItem
+class PartTreeItem : public AbstractInstrumentsPanelTreeItem, public muse::Injectable
 {
     Q_OBJECT
 
+    muse::Inject<notation::ISelectInstrumentsScenario> selectInstrumentsScenario { this };
+    muse::Inject<muse::IInteractive> interactive { this };
+
 public:
     PartTreeItem(notation::IMasterNotationPtr masterNotation, notation::INotationPtr notation, QObject* parent);
 
     void init(const notation::Part* masterPart);
 
     bool isSelectable() const override;
 
-    Q_INVOKABLE QString instrumentId() const;
-
     MoveParams buildMoveParams(int sourceRow, int count, AbstractInstrumentsPanelTreeItem* destinationParent,
                                int destinationRow) const override;
 
@@ -48,6 +51,10 @@ class PartTreeItem : public AbstractInstrumentsPanelTreeItem
 
     void removeChildren(int row, int count, bool deleteChild) override;
 
+    Q_INVOKABLE QString instrumentId() const;
+    Q_INVOKABLE void replaceInstrument();
+    Q_INVOKABLE void resetAllFormatting();
+
 private:
     void listenVisibilityChanged();
     void createAndAddPart(const muse::ID& masterPartId);