Conversation
Fixes musescore#32742 The crash occurred when closing VST plugin windows (e.g., ARIA) on macOS. The issue was that IPlugView::removed() was being called during window closure, which tries to access NSView objects that macOS may already be deallocating as part of its own cleanup sequence. On macOS, the Cocoa framework handles NSView cleanup automatically during window destruction. Calling removed() at this point causes a crash because it attempts to interact with partially or fully deallocated NSView objects. On Windows and Linux, the underlying window handles (HWND/X11) remain valid throughout the close event, so calling removed() is safe and necessary for proper plugin cleanup. The fix skips the removed() call on macOS using #ifndef Q_OS_MAC, allowing the system to handle NSView cleanup naturally while maintaining proper cleanup on other platforms. This bug was introduced in commit d50a9bd (Dec 2022) when deinit() was added to closeEvent() to fix issue musescore#13867.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds extensive undoable score-structure editing APIs and EditPart helpers, expands DOM staff properties and Pid enums, extends Drumset mutators and cloning with ownership, tightens hairpin/playback snapping to playable dynamics, delegates many notation operations to EditPart, adds numerous unit tests, and guards VST deinit on macOS. Changes
Sequence Diagram(s)sequenceDiagram
participant Plugin as Plugin/QML
participant ScoreAPI as apiv1::Score
participant EditPart as EditPart (editing)
participant Undo as Undo System
participant DOM as DOM (Score/Part/Staff)
Plugin->>ScoreAPI: setPartVisible(part, true)
ScoreAPI->>EditPart: setPartVisible(score, part, true)
EditPart->>Undo: create undo action (Pid::VISIBLE)
Undo->>DOM: apply Staff::setVisible(true)
DOM-->>Undo: state updated
Undo-->>EditPart: undo registered
EditPart-->>ScoreAPI: return
ScoreAPI-->>Plugin: complete
sequenceDiagram
participant Plugin as Plugin/QML
participant ScoreAPI as apiv1::Score
participant Template as InstrumentTemplate
participant EditPart as EditPart (editing)
participant Undo as Undo System
participant DOM as DOM (Score/Part/Staff)
Plugin->>ScoreAPI: insertPart(instrumentId, index)
ScoreAPI->>Template: searchTemplate(instrumentId)
alt template found
ScoreAPI->>EditPart: insertPart(score, template, index)
EditPart->>DOM: create Part and Staves from template
EditPart->>Undo: register insert undo
Undo-->>DOM: part inserted (undoable)
EditPart-->>ScoreAPI: success
ScoreAPI-->>Plugin: complete
else not found
ScoreAPI-->>Plugin: warn / no-op
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/engraving/api/tests/score_tests.cpp (1)
184-213:⚠️ Potential issue | 🟠 MajorSkip this test when instrument templates are unavailable.
This happy-path case depends on the same
"violin"template lookup that later tests already guard withsearchTemplate(u"violin"). Without the same precondition here, the test validates environment setup rather thanScore::replaceInstrument().Proposed fix
TEST_F(Engraving_ApiScoreTests, replaceInstrumentApi) { + if (!searchTemplate(u"violin")) { + GTEST_SKIP() << "Instrument templates not loaded"; + } + // [GIVEN] A score with a part MasterScore* domScore = compat::ScoreAccess::createMasterScore(nullptr);
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4461574a-c56c-4e55-8420-bfb69664037b
📒 Files selected for processing (19)
src/engraving/api/tests/score_tests.cppsrc/engraving/api/v1/apitypes.hsrc/engraving/api/v1/elements.hsrc/engraving/api/v1/instrument.cppsrc/engraving/api/v1/instrument.hsrc/engraving/api/v1/score.cppsrc/engraving/api/v1/score.hsrc/engraving/dom/hairpin.cppsrc/engraving/dom/hairpin.hsrc/engraving/dom/property.cppsrc/engraving/dom/property.hsrc/engraving/dom/segment.cppsrc/engraving/dom/staff.cppsrc/engraving/editing/editpart.cppsrc/engraving/editing/editpart.hsrc/engraving/playback/playbackcontext.cppsrc/framework/vst/qml/Muse/Vst/vstview.cppsrc/framework/vst/widgets/vstviewdialog_qwidget.cppsrc/notation/internal/notationparts.cpp
| void Score::insertPart(const QString& instrumentId, int index) | ||
| { | ||
| const InstrumentTemplate* t = searchTemplate(instrumentId); | ||
| if (!t) { | ||
| LOGW("insertPart: <%s> not found", qPrintable(instrumentId)); | ||
| return; | ||
| } | ||
|
|
||
| mu::engraving::EditPart::insertPart(score(), t, static_cast<size_t>(index)); | ||
| } |
There was a problem hiding this comment.
Validate index before casting it to size_t.
Line 422 converts plugin input straight to size_t. A negative value from QML/JS wraps to a huge unsigned index, so insertPart(..., -1) will not fail cleanly and can hand EditPart::insertPart() a nonsensical position.
💡 Suggested guard
void Score::insertPart(const QString& instrumentId, int index)
{
+ const int partCount = static_cast<int>(score()->parts().size());
+ if (index < 0 || index > partCount) {
+ LOGW("insertPart: index %d out of range [0, %d]", index, partCount);
+ return;
+ }
+
const InstrumentTemplate* t = searchTemplate(instrumentId);
if (!t) {
LOGW("insertPart: <%s> not found", qPrintable(instrumentId));
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void Score::insertPart(const QString& instrumentId, int index) | |
| { | |
| const InstrumentTemplate* t = searchTemplate(instrumentId); | |
| if (!t) { | |
| LOGW("insertPart: <%s> not found", qPrintable(instrumentId)); | |
| return; | |
| } | |
| mu::engraving::EditPart::insertPart(score(), t, static_cast<size_t>(index)); | |
| } | |
| void Score::insertPart(const QString& instrumentId, int index) | |
| { | |
| const int partCount = static_cast<int>(score()->parts().size()); | |
| if (index < 0 || index > partCount) { | |
| LOGW("insertPart: index %d out of range [0, %d]", index, partCount); | |
| return; | |
| } | |
| const InstrumentTemplate* t = searchTemplate(instrumentId); | |
| if (!t) { | |
| LOGW("insertPart: <%s> not found", qPrintable(instrumentId)); | |
| return; | |
| } | |
| mu::engraving::EditPart::insertPart(score(), t, static_cast<size_t>(index)); | |
| } |
| { Pid::STAFF_CUTAWAY, P_TYPE::BOOL, PropertyGroup::APPEARANCE, false, "", QT_TRANSLATE_NOOP("engraving/propertyName", "cutaway") }, | ||
| { Pid::STAFF_HIDE_SYSTEM_BARLINE, P_TYPE::BOOL, PropertyGroup::APPEARANCE, false, "", QT_TRANSLATE_NOOP("engraving/propertyName", "hide system barline") }, | ||
| { Pid::STAFF_MERGE_MATCHING_RESTS, P_TYPE::INT, PropertyGroup::APPEARANCE, false, "", QT_TRANSLATE_NOOP("engraving/propertyName", "merge matching rests") }, | ||
| { Pid::STAFF_REFLECT_TRANSPOSITION, P_TYPE::BOOL, PropertyGroup::APPEARANCE, false, "", QT_TRANSLATE_NOOP("engraving/propertyName", "reflect transposition") }, | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify how mergeMatchingRests is typed in Staff class and how other AUTO_ON_OFF properties are handled
echo "=== Check mergeMatchingRests return type in Staff ==="
rg -n "mergeMatchingRests" --type cpp -C2
echo ""
echo "=== Check how other AUTO_ON_OFF properties handle get/set ==="
rg -n "HIDE_WHEN_EMPTY|SHOW_MEASURE_NUMBERS" --type cpp -C2 | head -80Repository: musescore/MuseScore
Length of output: 28316
🏁 Script executed:
sed -n '508,512p' src/engraving/dom/property.cppRepository: musescore/MuseScore
Length of output: 942
Use P_TYPE::AUTO_ON_OFF for STAFF_MERGE_MATCHING_RESTS to match the established pattern.
The property is declared with P_TYPE::INT, but the underlying type is AutoOnOff (stored as AutoOnOff m_mergeMatchingRests in the Staff class). Other AutoOnOff properties—HIDE_WHEN_EMPTY and SHOW_MEASURE_NUMBERS—use P_TYPE::AUTO_ON_OFF, which eliminates the need for explicit int casting and ensures consistent type handling across the codebase.
Suggested fix
{ Pid::STAFF_CUTAWAY, P_TYPE::BOOL, PropertyGroup::APPEARANCE, false, "", QT_TRANSLATE_NOOP("engraving/propertyName", "cutaway") },
{ Pid::STAFF_HIDE_SYSTEM_BARLINE, P_TYPE::BOOL, PropertyGroup::APPEARANCE, false, "", QT_TRANSLATE_NOOP("engraving/propertyName", "hide system barline") },
- { Pid::STAFF_MERGE_MATCHING_RESTS, P_TYPE::INT, PropertyGroup::APPEARANCE, false, "", QT_TRANSLATE_NOOP("engraving/propertyName", "merge matching rests") },
+ { Pid::STAFF_MERGE_MATCHING_RESTS, P_TYPE::AUTO_ON_OFF, PropertyGroup::APPEARANCE, false, "", QT_TRANSLATE_NOOP("engraving/propertyName", "merge matching rests") },
{ Pid::STAFF_REFLECT_TRANSPOSITION, P_TYPE::BOOL, PropertyGroup::APPEARANCE, false, "", QT_TRANSLATE_NOOP("engraving/propertyName", "reflect transposition") },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| { Pid::STAFF_CUTAWAY, P_TYPE::BOOL, PropertyGroup::APPEARANCE, false, "", QT_TRANSLATE_NOOP("engraving/propertyName", "cutaway") }, | |
| { Pid::STAFF_HIDE_SYSTEM_BARLINE, P_TYPE::BOOL, PropertyGroup::APPEARANCE, false, "", QT_TRANSLATE_NOOP("engraving/propertyName", "hide system barline") }, | |
| { Pid::STAFF_MERGE_MATCHING_RESTS, P_TYPE::INT, PropertyGroup::APPEARANCE, false, "", QT_TRANSLATE_NOOP("engraving/propertyName", "merge matching rests") }, | |
| { Pid::STAFF_REFLECT_TRANSPOSITION, P_TYPE::BOOL, PropertyGroup::APPEARANCE, false, "", QT_TRANSLATE_NOOP("engraving/propertyName", "reflect transposition") }, | |
| { Pid::STAFF_CUTAWAY, P_TYPE::BOOL, PropertyGroup::APPEARANCE, false, "", QT_TRANSLATE_NOOP("engraving/propertyName", "cutaway") }, | |
| { Pid::STAFF_HIDE_SYSTEM_BARLINE, P_TYPE::BOOL, PropertyGroup::APPEARANCE, false, "", QT_TRANSLATE_NOOP("engraving/propertyName", "hide system barline") }, | |
| { Pid::STAFF_MERGE_MATCHING_RESTS, P_TYPE::AUTO_ON_OFF, PropertyGroup::APPEARANCE, false, "", QT_TRANSLATE_NOOP("engraving/propertyName", "merge matching rests") }, | |
| { Pid::STAFF_REFLECT_TRANSPOSITION, P_TYPE::BOOL, PropertyGroup::APPEARANCE, false, "", QT_TRANSLATE_NOOP("engraving/propertyName", "reflect transposition") }, |
| case Pid::STAFF_REFLECT_TRANSPOSITION: | ||
| setReflectTranspositionInLinkedTab(v.toBool()); | ||
| break; |
There was a problem hiding this comment.
Copy the new reflect-transposition flag in the staff copy path.
Staff::clone() still goes through Staff::init(const Staff* s), and that method around Line 1375 never copies m_reflectTranspositionInLinkedTab. After this change, any clone-based path can silently reset Pid::STAFF_REFLECT_TRANSPOSITION back to its default.
Proposed fix
m_hideSystemBarLine = s->m_hideSystemBarLine;
m_mergeMatchingRests = s->m_mergeMatchingRests;
+ m_reflectTranspositionInLinkedTab = s->m_reflectTranspositionInLinkedTab;
m_color = s->m_color;| void EditPart::insertPart(Score* score, const InstrumentTemplate* templ, size_t index) | ||
| { | ||
| if (!score || !templ) { | ||
| return; | ||
| } | ||
|
|
||
| Part* part = new Part(score); | ||
| part->initFromInstrTemplate(templ); | ||
|
|
||
| for (staff_idx_t i = 0; i < templ->staffCount; ++i) { | ||
| Staff* staff = Factory::createStaff(part); | ||
| StaffType* stt = staff->staffType(Fraction(0, 1)); | ||
| staff->init(templ, stt, int(i)); | ||
| score->undoInsertStaff(staff, i); | ||
| } | ||
|
|
||
| score->undoInsertPart(part, index); | ||
| score->setUpTempoMapLater(); | ||
| score->masterScore()->rebuildMidiMapping(); |
There was a problem hiding this comment.
Synchronize key signatures after inserting the new staves.
The existing staff-insertion paths call adjustKeySigs() after undoInsertStaff(), but this helper never does. New parts inserted through this path—and replacePart(), which delegates to it—can therefore come in with the wrong key signatures for the current score.
💡 Suggested follow-up
void EditPart::insertPart(Score* score, const InstrumentTemplate* templ, size_t index)
{
if (!score || !templ) {
return;
}
@@
}
score->undoInsertPart(part, index);
+ if (part->nstaves() > 0) {
+ const staff_idx_t firstStaffIndex = part->staff(0)->idx();
+ score->adjustKeySigs(firstStaffIndex, firstStaffIndex + part->nstaves(), score->keyList());
+ }
score->setUpTempoMapLater();
score->masterScore()->rebuildMidiMapping();
}
RomanPudashkin
force-pushed
the
ports_470
branch
from
03252df to
5da3533
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/notation/internal/notationparts.cpp (1)
721-739:⚠️ Potential issue | 🟠 MajorUse
instrumentKey.tickto target the drumset change.This still matches every instrument change in the part that reuses the same
instrumentId. If the same percussion instrument id appears at multiple ticks, one edit here rewrites all of them instead of the selected change.Suggested fix
void NotationParts::replaceDrumset(const InstrumentKey& instrumentKey, const Drumset& newDrumset, bool undoable) { Part* part = partModifiable(instrumentKey.partId); if (!part) { return; @@ if (undoable) { startEdit(TranslatableString("undoableAction", "Edit drumset")); - - for (auto pair : part->instruments()) { - Instrument* instrument = pair.second; - if (instrument && instrument->drumset() && instrument->id() == instrumentKey.instrumentId) { - EditPart::replaceDrumset(score(), part, pair.first, newDrumset); - } - } + EditPart::replaceDrumset(score(), part, instrumentKey.tick, newDrumset); apply(); } else { - for (auto pair : part->instruments()) { - Instrument* instrument = pair.second; - if (instrument && instrument->drumset() && instrument->id() == instrumentKey.instrumentId) { - instrument->setDrumset(&newDrumset); - } + Instrument* instrument = part->instrument(instrumentKey.tick); + if (instrument && instrument->drumset()) { + instrument->setDrumset(&newDrumset); } }
♻️ Duplicate comments (8)
src/engraving/dom/staff.cpp (1)
1763-1765:⚠️ Potential issue | 🟠 Major
STAFF_REFLECT_TRANSPOSITIONis still lost during clone/init.After Line 1763 writes the new property,
Staff::init(const Staff* s)still doesn’t copym_reflectTranspositionInLinkedTab, so clone-based flows can reset it to default.Proposed fix
void Staff::init(const Staff* s) { @@ m_hideSystemBarLine = s->m_hideSystemBarLine; m_mergeMatchingRests = s->m_mergeMatchingRests; + m_reflectTranspositionInLinkedTab = s->m_reflectTranspositionInLinkedTab; m_color = s->m_color; @@ }src/engraving/dom/property.cpp (1)
510-510:⚠️ Potential issue | 🟡 MinorUse
P_TYPE::AUTO_ON_OFFforSTAFF_MERGE_MATCHING_RESTS.Line 510 currently declares
P_TYPE::INT, but staff get/set/default treat it asAutoOnOff. This mismatch can cause inconsistent property conversion paths.Proposed fix
- { Pid::STAFF_MERGE_MATCHING_RESTS, P_TYPE::INT, PropertyGroup::APPEARANCE, false, "", QT_TRANSLATE_NOOP("engraving/propertyName", "merge matching rests") }, + { Pid::STAFF_MERGE_MATCHING_RESTS, P_TYPE::AUTO_ON_OFF, PropertyGroup::APPEARANCE, false, "", QT_TRANSLATE_NOOP("engraving/propertyName", "merge matching rests") },src/engraving/api/v1/instrument.cpp (2)
116-162:⚠️ Potential issue | 🟠 MajorEnforce read-only contract in Drumset setters.
These mutators still write through even when the wrapper is non-owned/read-only. Add an
m_ownedguard soInstrument::drumset()cannot mutate live score state directly.Proposed fix
void Drumset::setName(int pitch, const QString& name) { + if (!m_owned) { + return; + } IF_ASSERT_FAILED(pitch >= 0 && pitch < DRUM_INSTRUMENTS) { return; } @@ void Drumset::setNoteHead(int pitch, int noteHead) { + if (!m_owned) { + return; + } @@ void Drumset::setLine(int pitch, int line) { + if (!m_owned) { + return; + } @@ void Drumset::setVoice(int pitch, int voice) { + if (!m_owned) { + return; + } @@ void Drumset::setStemDirection(int pitch, int stemDirection) { + if (!m_owned) { + return; + } @@ void Drumset::setShortcut(int pitch, const QString& shortcut) { + if (!m_owned) { + return; + }
238-246:⚠️ Potential issue | 🟠 MajorDo not parent
cloneDrumset()result toInstrument.At Line 245, returning the clone with parent
thisties lifetime to a transient wrapper. Return a parentless object for caller ownership and set QML ownership explicitly.In Qt 6 QML, when returning a QObject* from an invokable/API method, what is the recommended ownership pattern if the caller should own it, and how does QObject parentage affect deletion?Proposed fix
+#include <QQmlEngine> @@ Drumset* Instrument::cloneDrumset() { @@ - return new Drumset(new mu::engraving::Drumset(*ds), true, this); + Drumset* clone = new Drumset(new mu::engraving::Drumset(*ds), true, nullptr); + QQmlEngine::setObjectOwnership(clone, QQmlEngine::JavaScriptOwnership); + return clone; }src/engraving/api/v1/elements.h (1)
2343-2345:⚠️ Potential issue | 🟡 MinorFix the
hideSystemBarLinedescription.The current text says this property displays the left system barline, but the property name and behavior are the opposite.
Suggested fix
- /// Whether to display the system barline (leftmost barline). + /// Whether to hide the system barline (leftmost barline).src/engraving/editing/editpart.cpp (1)
711-729:⚠️ Potential issue | 🟠 MajorSynchronize key signatures for inserted parts.
This helper inserts new staves but never calls
adjustKeySigs(), unlike the existing staff-insertion paths.replacePart()delegates here, so inserted/replaced parts can start with stale key signatures.Suggested fix
void EditPart::insertPart(Score* score, const InstrumentTemplate* templ, size_t index) { if (!score || !templ) { return; } @@ } score->undoInsertPart(part, index); + if (part->nstaves() > 0) { + const staff_idx_t firstStaffIndex = part->staff(0)->idx(); + score->adjustKeySigs(firstStaffIndex, firstStaffIndex + part->nstaves(), score->keyList()); + } score->setUpTempoMapLater(); score->masterScore()->rebuildMidiMapping(); }src/engraving/api/v1/score.cpp (2)
288-305:⚠️ Potential issue | 🟠 MajorReject cross-part staff moves in
Score::moveStaves().
Score.moveStaves()is documented to only accept source staves from the destination part, but this wrapper still forwards mixed-part lists straight toEditPart::moveStaves(). That bypasses the guard kept insrc/notation/internal/notationparts.cpp.Suggested fix
void Score::moveStaves(QList<apiv1::Staff*> sourceStaves, apiv1::Staff* destinationStaff, int insertMode) { if (!destinationStaff) { LOGW("moveStaves: destinationStaff is null"); return; @@ for (apiv1::Staff* s : sourceStaves) { if (!s) { LOGW("moveStaves: null staff in list"); continue; } domStaves.push_back(s->staff()); } + + mu::engraving::Part* destinationPart = destinationStaff->staff()->part(); + for (mu::engraving::Staff* s : domStaves) { + if (s->part() != destinationPart) { + LOGW("moveStaves: all source staves must belong to the destination part"); + return; + } + } mu::engraving::EditPart::moveStaves(score(), domStaves, destinationStaff->staff(), insertMode == 1); }
403-412:⚠️ Potential issue | 🟠 MajorValidate
indexbefore casting tosize_t.Line 411 casts plugin input straight to
size_t. Negative JS/QML values wrap to huge positive indexes, soinsertPart(..., -1)will not fail cleanly.Suggested fix
void Score::insertPart(const QString& instrumentId, int index) { + const int partCount = static_cast<int>(score()->parts().size()); + if (index < 0 || index > partCount) { + LOGW("insertPart: index %d out of range [0, %d]", index, partCount); + return; + } + const InstrumentTemplate* t = searchTemplate(instrumentId); if (!t) { LOGW("insertPart: <%s> not found", qPrintable(instrumentId)); return; }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7417ddb3-640b-4ed4-a45a-ae6719100480
📒 Files selected for processing (17)
src/engraving/api/tests/score_tests.cppsrc/engraving/api/v1/apitypes.hsrc/engraving/api/v1/elements.hsrc/engraving/api/v1/instrument.cppsrc/engraving/api/v1/instrument.hsrc/engraving/api/v1/score.cppsrc/engraving/api/v1/score.hsrc/engraving/dom/hairpin.cppsrc/engraving/dom/hairpin.hsrc/engraving/dom/property.cppsrc/engraving/dom/property.hsrc/engraving/dom/segment.cppsrc/engraving/dom/staff.cppsrc/engraving/editing/editpart.cppsrc/engraving/editing/editpart.hsrc/engraving/playback/playbackcontext.cppsrc/notation/internal/notationparts.cpp
| // Add a test order to the global list for API lookup | ||
| ScoreOrder testOrder; | ||
| testOrder.id = u"test-api-order"; | ||
| testOrder.name = TranslatableString::untranslatable("Test API Order"); | ||
| instrumentOrders.push_back(testOrder); | ||
|
|
||
| apiv1::Score apiScore(domScore); | ||
|
|
||
| // [WHEN] We set the score order via API | ||
| apiScore.setScoreOrder("test-api-order"); | ||
|
|
||
| // [THEN] The score order should be changed | ||
| EXPECT_EQ(domScore->scoreOrder().id, u"test-api-order"); | ||
|
|
||
| // Clean up the added test order | ||
| instrumentOrders.pop_back(); |
There was a problem hiding this comment.
Protect global instrumentOrders from leaking state across tests.
Line 1510 mutates global state and Line 1521 manually rolls it back; if the test exits early, later tests can inherit polluted order data.
Proposed fix
- instrumentOrders.push_back(testOrder);
+ const size_t oldOrdersSize = instrumentOrders.size();
+ struct InstrumentOrdersGuard {
+ size_t oldSize;
+ ~InstrumentOrdersGuard() { instrumentOrders.resize(oldSize); }
+ } guard { oldOrdersSize };
+ instrumentOrders.push_back(testOrder);
@@
- // Clean up the added test order
- instrumentOrders.pop_back();
+ // cleanup handled by guard📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Add a test order to the global list for API lookup | |
| ScoreOrder testOrder; | |
| testOrder.id = u"test-api-order"; | |
| testOrder.name = TranslatableString::untranslatable("Test API Order"); | |
| instrumentOrders.push_back(testOrder); | |
| apiv1::Score apiScore(domScore); | |
| // [WHEN] We set the score order via API | |
| apiScore.setScoreOrder("test-api-order"); | |
| // [THEN] The score order should be changed | |
| EXPECT_EQ(domScore->scoreOrder().id, u"test-api-order"); | |
| // Clean up the added test order | |
| instrumentOrders.pop_back(); | |
| // Add a test order to the global list for API lookup | |
| ScoreOrder testOrder; | |
| testOrder.id = u"test-api-order"; | |
| testOrder.name = TranslatableString::untranslatable("Test API Order"); | |
| const size_t oldOrdersSize = instrumentOrders.size(); | |
| struct InstrumentOrdersGuard { | |
| size_t oldSize; | |
| ~InstrumentOrdersGuard() { instrumentOrders.resize(oldSize); } | |
| } guard { oldOrdersSize }; | |
| instrumentOrders.push_back(testOrder); | |
| apiv1::Score apiScore(domScore); | |
| // [WHEN] We set the score order via API | |
| apiScore.setScoreOrder("test-api-order"); | |
| // [THEN] The score order should be changed | |
| EXPECT_EQ(domScore->scoreOrder().id, u"test-api-order"); | |
| // cleanup handled by guard |
| enum class StaffTypes { | ||
| STANDARD = int(mu::engraving::StaffTypes::STANDARD), | ||
| PERC_1LINE = int(mu::engraving::StaffTypes::PERC_1LINE), | ||
| PERC_2LINE = int(mu::engraving::StaffTypes::PERC_2LINE), | ||
| PERC_3LINE = int(mu::engraving::StaffTypes::PERC_3LINE), | ||
| PERC_5LINE = int(mu::engraving::StaffTypes::PERC_5LINE), | ||
| TAB_6SIMPLE = int(mu::engraving::StaffTypes::TAB_6SIMPLE), | ||
| TAB_6COMMON = int(mu::engraving::StaffTypes::TAB_6COMMON), | ||
| TAB_6FULL = int(mu::engraving::StaffTypes::TAB_6FULL), | ||
| TAB_4SIMPLE = int(mu::engraving::StaffTypes::TAB_4SIMPLE), | ||
| TAB_4COMMON = int(mu::engraving::StaffTypes::TAB_4COMMON), | ||
| TAB_4FULL = int(mu::engraving::StaffTypes::TAB_4FULL), | ||
| TAB_5SIMPLE = int(mu::engraving::StaffTypes::TAB_5SIMPLE), | ||
| TAB_5COMMON = int(mu::engraving::StaffTypes::TAB_5COMMON), | ||
| TAB_5FULL = int(mu::engraving::StaffTypes::TAB_5FULL), | ||
| TAB_UKULELE = int(mu::engraving::StaffTypes::TAB_UKULELE), | ||
| TAB_BALALAJKA = int(mu::engraving::StaffTypes::TAB_BALALAJKA), | ||
| TAB_DULCIMER = int(mu::engraving::StaffTypes::TAB_DULCIMER), | ||
| TAB_ITALIAN = int(mu::engraving::StaffTypes::TAB_ITALIAN), | ||
| TAB_FRENCH = int(mu::engraving::StaffTypes::TAB_FRENCH), | ||
| TAB_7COMMON = int(mu::engraving::StaffTypes::TAB_7COMMON), | ||
| TAB_8COMMON = int(mu::engraving::StaffTypes::TAB_8COMMON), | ||
| TAB_9COMMON = int(mu::engraving::StaffTypes::TAB_9COMMON), | ||
| TAB_10COMMON = int(mu::engraving::StaffTypes::TAB_10COMMON), | ||
| TAB_7SIMPLE = int(mu::engraving::StaffTypes::TAB_7SIMPLE), | ||
| TAB_8SIMPLE = int(mu::engraving::StaffTypes::TAB_8SIMPLE), | ||
| TAB_9SIMPLE = int(mu::engraving::StaffTypes::TAB_9SIMPLE), | ||
| TAB_10SIMPLE = int(mu::engraving::StaffTypes::TAB_10SIMPLE), | ||
| PERC_DEFAULT = int(mu::engraving::StaffTypes::PERC_DEFAULT), | ||
| TAB_DEFAULT = int(mu::engraving::StaffTypes::TAB_DEFAULT), | ||
| }; |
There was a problem hiding this comment.
Freeze StaffTypes API numeric values to avoid future ABI drift.
Line 1441–Line 1469 currently mirror internal enum ordinals. Since this is a public API enum (@since 4.7), internal enum reordering can unintentionally change external numeric IDs. Use explicit stable API integers and optionally guard with static_assert mappings.
Proposed hardening
enum class StaffTypes {
- STANDARD = int(mu::engraving::StaffTypes::STANDARD),
- PERC_1LINE = int(mu::engraving::StaffTypes::PERC_1LINE),
- PERC_2LINE = int(mu::engraving::StaffTypes::PERC_2LINE),
+ STANDARD = 0,
+ PERC_1LINE = 1,
+ PERC_2LINE = 2,
...
- TAB_10SIMPLE = int(mu::engraving::StaffTypes::TAB_10SIMPLE),
- PERC_DEFAULT = int(mu::engraving::StaffTypes::PERC_DEFAULT),
- TAB_DEFAULT = int(mu::engraving::StaffTypes::TAB_DEFAULT),
+ TAB_10SIMPLE = 27,
+ PERC_DEFAULT = 4,
+ TAB_DEFAULT = 6,
};
+
+static_assert(int(StaffTypes::STANDARD) == int(mu::engraving::StaffTypes::STANDARD));
+static_assert(int(StaffTypes::PERC_DEFAULT) == int(mu::engraving::StaffTypes::PERC_DEFAULT));
+static_assert(int(StaffTypes::TAB_DEFAULT) == int(mu::engraving::StaffTypes::TAB_DEFAULT));| bool EditPart::setVoiceVisible(Score* score, Staff* staff, int voiceIndex, bool visible) | ||
| { | ||
| if (!score || !staff) { | ||
| return false; | ||
| } | ||
|
|
||
| if (!score->excerpt()) { | ||
| return false; | ||
| } | ||
|
|
||
| if (!visible && !staff->canDisableVoice()) { | ||
| return false; | ||
| } | ||
|
|
||
| score->excerpt()->setVoiceVisible(staff, voiceIndex, visible); | ||
| return true; |
There was a problem hiding this comment.
Validate voiceIndex before forwarding to Excerpt.
Line 707 forwards any int straight to Excerpt::setVoiceVisible(). Values outside the documented 0-3 range are not rejected here, and negative QML/JS inputs can wrap when they hit the downstream voice-index type.
Suggested fix
bool EditPart::setVoiceVisible(Score* score, Staff* staff, int voiceIndex, bool visible)
{
if (!score || !staff) {
return false;
}
+ if (voiceIndex < 0 || voiceIndex > 3) {
+ return false;
+ }
if (!score->excerpt()) {
return false;
}Expose a bunch of Part/Staff editing methods to Plugin API
Expose remaining Part/Staff editing methods to Plugin API
…ynamics Playback fixes for hairpin end dynamics
RomanPudashkin
force-pushed
the
ports_470
branch
from
5da3533 to
e71ef8a
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (9)
src/engraving/api/v1/instrument.cpp (2)
116-162:⚠️ Potential issue | 🟠 MajorStill block writes on non-cloned drumsets.
These setters still mutate the live score drumset when they are called on the wrapper returned by
Instrument::drumset(), so plugins can bypass the clone/replace workflow and its undo/rebuild path.Proposed fix
void Drumset::setName(int pitch, const QString& name) { + if (!m_owned) { + return; + } IF_ASSERT_FAILED(pitch >= 0 && pitch < DRUM_INSTRUMENTS) { return; } m_drumset->drum(pitch).name = name; }Apply the same guard to
setNoteHead(),setLine(),setVoice(),setStemDirection(), andsetShortcut().
238-245:⚠️ Potential issue | 🟠 MajorStill return the cloned drumset parentless.
This method documents caller ownership, but parenting the clone to
thisties its lifetime to the transientInstrumentwrapper instead. A QML caller can lose the clone as soon as that wrapper is destroyed.Proposed fix
+#include <QQmlEngine> + Drumset* Instrument::cloneDrumset() { const mu::engraving::Drumset* ds = instrument()->drumset(); if (!ds) { return nullptr; } - return new Drumset(new mu::engraving::Drumset(*ds), true, this); + Drumset* clone = new Drumset(new mu::engraving::Drumset(*ds), true, nullptr); + QQmlEngine::setObjectOwnership(clone, QQmlEngine::JavaScriptOwnership); + return clone; }In Qt/QML, when a QObject* is returned from a Q_INVOKABLE, how do QObject parent ownership and QQmlEngine::setObjectOwnership affect whether the caller can own and keep that object alive?src/engraving/dom/staff.cpp (1)
1657-1666:⚠️ Potential issue | 🟠 MajorStill copy
m_reflectTranspositionInLinkedTabin the staff clone path.These new get/set/default cases make the property public, but
Staff::init(const Staff* s)still omits that field. Any clone-based operation will silently reset it to the default.Proposed fix
m_hideSystemBarLine = s->m_hideSystemBarLine; m_mergeMatchingRests = s->m_mergeMatchingRests; + m_reflectTranspositionInLinkedTab = s->m_reflectTranspositionInLinkedTab; m_color = s->m_color;Also applies to: 1763-1765, 1805-1814
src/engraving/dom/property.cpp (1)
508-511:⚠️ Potential issue | 🟡 MinorUse
P_TYPE::AUTO_ON_OFFforSTAFF_MERGE_MATCHING_RESTS.The backing field is
AutoOnOff, but this metadata still registers it asINT. That leaks the wrong type through the generic property API and is why the new staff accessors have to cast manually.Proposed fix
- { Pid::STAFF_MERGE_MATCHING_RESTS, P_TYPE::INT, PropertyGroup::APPEARANCE, false, "", QT_TRANSLATE_NOOP("engraving/propertyName", "merge matching rests") }, + { Pid::STAFF_MERGE_MATCHING_RESTS, P_TYPE::AUTO_ON_OFF, PropertyGroup::APPEARANCE, false, "", QT_TRANSLATE_NOOP("engraving/propertyName", "merge matching rests") },Then update
Staff::getProperty(),Staff::setProperty(), andStaff::propertyDefault()to useAutoOnOffdirectly instead of manualintconversions.src/engraving/api/tests/score_tests.cpp (1)
1515-1530:⚠️ Potential issue | 🟡 MinorGuard
instrumentOrdersmutation with RAII to prevent cross-test pollution.
instrumentOrders.push_back()on Line 1519 and manualpop_back()on Line 1530 is fragile if the test exits early; state can leak into later tests.Proposed fix
- instrumentOrders.push_back(testOrder); + const size_t oldOrdersSize = instrumentOrders.size(); + struct InstrumentOrdersGuard { + size_t oldSize; + ~InstrumentOrdersGuard() { instrumentOrders.resize(oldSize); } + } guard { oldOrdersSize }; + instrumentOrders.push_back(testOrder); @@ - // Clean up the added test order - instrumentOrders.pop_back(); + // cleanup handled by guardsrc/engraving/editing/editpart.cpp (2)
693-709:⚠️ Potential issue | 🟠 MajorValidate
voiceIndexbounds before forwarding.The
voiceIndexparameter is forwarded directly toExcerpt::setVoiceVisible()without validation. Values outside the valid 0-3 range (especially negative values from QML/JS) could cause undefined behavior.Suggested fix
bool EditPart::setVoiceVisible(Score* score, Staff* staff, int voiceIndex, bool visible) { if (!score || !staff) { return false; } + if (voiceIndex < 0 || voiceIndex > 3) { + return false; + } if (!score->excerpt()) { return false;
711-730:⚠️ Potential issue | 🟠 MajorSynchronize key signatures after inserting new staves.
The
insertParthelper creates staves viaundoInsertStaff()but does not calladjustKeySigs()afterward. Contrast this withdoAppendStaff()at line 660 which correctly callsadjustKeySigs(). New parts inserted through this path may have incorrect key signatures.Suggested fix
score->undoInsertPart(part, index); + + if (part->nstaves() > 0) { + const staff_idx_t firstStaffIdx = part->staff(0)->idx(); + score->adjustKeySigs(firstStaffIdx, firstStaffIdx + part->nstaves(), score->keyList()); + } + score->setUpTempoMapLater(); score->masterScore()->rebuildMidiMapping(); }src/engraving/api/v1/score.cpp (2)
403-412:⚠️ Potential issue | 🟠 MajorValidate
indexbefore casting tosize_t.Line 411 casts
indexdirectly tosize_twithout bounds checking. A negative value from QML/JS wraps to a huge unsigned index, which could cause undefined behavior inEditPart::insertPart().Suggested fix
void Score::insertPart(const QString& instrumentId, int index) { + const int partCount = static_cast<int>(score()->parts().size()); + if (index < 0 || index > partCount) { + LOGW("insertPart: index %d out of range [0, %d]", index, partCount); + return; + } + const InstrumentTemplate* t = searchTemplate(instrumentId); if (!t) { LOGW("insertPart: <%s> not found", qPrintable(instrumentId)); return; } - mu::engraving::EditPart::insertPart(score(), t, static_cast<size_t>(index)); + mu::engraving::EditPart::insertPart(score(), t, static_cast<size_t>(index)); }
288-305:⚠️ Potential issue | 🟡 MinorAdd cross-part staff move validation to
moveStaves.The implementation allows moving staves across part boundaries without validation. Add a guard to reject moves where source staves belong to different parts than the destination staff. Validation should reject the operation early and log a warning if attempted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0add45f7-f2c8-4ffa-9744-08e5fa46f710
📒 Files selected for processing (17)
src/engraving/api/tests/score_tests.cppsrc/engraving/api/v1/apitypes.hsrc/engraving/api/v1/elements.hsrc/engraving/api/v1/instrument.cppsrc/engraving/api/v1/instrument.hsrc/engraving/api/v1/score.cppsrc/engraving/api/v1/score.hsrc/engraving/dom/hairpin.cppsrc/engraving/dom/hairpin.hsrc/engraving/dom/property.cppsrc/engraving/dom/property.hsrc/engraving/dom/segment.cppsrc/engraving/dom/staff.cppsrc/engraving/editing/editpart.cppsrc/engraving/editing/editpart.hsrc/engraving/playback/playbackcontext.cppsrc/notation/internal/notationparts.cpp
| /// Whether the staff is visible (combines part and staff visibility, read-only). | ||
| /// \since MuseScore 4.6 | ||
| Q_PROPERTY(bool show READ show) | ||
|
|
||
| /// Whether the staff itself is visible. | ||
| /// \since MuseScore 4.6 | ||
| API_PROPERTY_T(bool, visible, VISIBLE) | ||
| /// Whether this is a cutaway staff, which hides itself | ||
| /// mid-system when measures are empty. | ||
| /// \since MuseScore 4.6 | ||
| Q_PROPERTY(bool cutaway READ cutaway) | ||
| API_PROPERTY_T(bool, cutaway, STAFF_CUTAWAY) | ||
| /// Whether to display the system barline (leftmost barline). | ||
| /// \since MuseScore 4.6 | ||
| Q_PROPERTY(bool hideSystemBarLine READ hideSystemBarLine) | ||
| API_PROPERTY_T(bool, hideSystemBarLine, STAFF_HIDE_SYSTEM_BARLINE) | ||
| /// Whether to merge matching rests across voices. | ||
| /// One of PluginAPI::PluginAPI::AutoOnOff values. | ||
| /// If Auto, determined by the global style setting \p mergeMatchingRests . | ||
| /// \since MuseScore 4.6 | ||
| Q_PROPERTY(int mergeMatchingRests READ mergeMatchingRests) | ||
| /// Whether matching rests are to be merged. | ||
| API_PROPERTY(mergeMatchingRests, STAFF_MERGE_MATCHING_RESTS) | ||
| /// Whether to reflect transposition in the linked tablature staff. | ||
| /// \since MuseScore 4.6 | ||
| Q_PROPERTY(bool shouldMergeMatchingRests READ shouldMergeMatchingRests) | ||
| API_PROPERTY_T(bool, reflectTranspositionInLinkedTab, STAFF_REFLECT_TRANSPOSITION) |
There was a problem hiding this comment.
Breaking API change: shouldMergeMatchingRests property removed.
The shouldMergeMatchingRests read-only property has been removed from the Staff QML API. Based on the relevant code snippet, Staff::shouldMergeMatchingRests() still exists in the DOM layer (src/engraving/dom/staff.cpp), so QML scripts that previously accessed this property will now receive undefined at runtime.
Consider whether this removal is intentional. If so, document it as a breaking change in the release notes. If backward compatibility is needed, the computed read-only property could be retained alongside the new writable mergeMatchingRests setting.
| void Drumset::setVoice(int pitch, int voice) | ||
| { | ||
| IF_ASSERT_FAILED(pitch >= 0 && pitch < DRUM_INSTRUMENTS) { | ||
| return; | ||
| } | ||
| m_drumset->drum(pitch).voice = voice; |
There was a problem hiding this comment.
Validate voice before storing it.
Line 145 accepts any integer even though the API documents 0..3. Once that clone is applied, an out-of-range value can create invalid voice/track mappings downstream.
Proposed fix
void Drumset::setVoice(int pitch, int voice)
{
IF_ASSERT_FAILED(pitch >= 0 && pitch < DRUM_INSTRUMENTS) {
return;
}
+ IF_ASSERT_FAILED(voice >= 0 && voice < VOICES) {
+ return;
+ }
m_drumset->drum(pitch).voice = voice;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void Drumset::setVoice(int pitch, int voice) | |
| { | |
| IF_ASSERT_FAILED(pitch >= 0 && pitch < DRUM_INSTRUMENTS) { | |
| return; | |
| } | |
| m_drumset->drum(pitch).voice = voice; | |
| void Drumset::setVoice(int pitch, int voice) | |
| { | |
| IF_ASSERT_FAILED(pitch >= 0 && pitch < DRUM_INSTRUMENTS) { | |
| return; | |
| } | |
| IF_ASSERT_FAILED(voice >= 0 && voice < VOICES) { | |
| return; | |
| } | |
| m_drumset->drum(pitch).voice = voice; |
| /** APIDOC | ||
| * Sets the sharp/flat preference for a part's transposition. | ||
| * @method | ||
| * @param {Engraving.Part} part The Part object. | ||
| * @param {Number} sharpFlat 0 = NONE, 1 = SHARPS, 2 = FLATS. | ||
| * @since 4.7 | ||
| */ | ||
| Q_INVOKABLE void setPartSharpFlat(apiv1::Part* part, int sharpFlat); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider referencing the enum type in documentation.
The setPartSharpFlat documentation uses magic numbers (0 = NONE, 1 = SHARPS, 2 = FLATS). For consistency with other API methods that reference enum types, consider documenting the enum name (e.g., PreferSharpFlat) or adding an @see reference to the enum definition.
| void EditPart::doAppendStaff(Score* score, Staff* staff, Part* destinationPart, bool createRests) | ||
| { | ||
| if (!score || !staff || !destinationPart) { | ||
| return; | ||
| } | ||
|
|
||
| staff_idx_t staffLocalIndex = destinationPart->nstaves(); | ||
| KeyList keyList = *destinationPart->staff(staffLocalIndex - 1)->keyList(); | ||
|
|
||
| staff->setScore(score); | ||
| staff->setPart(destinationPart); | ||
|
|
||
| score->undoInsertStaff(staff, staffLocalIndex, createRests); | ||
|
|
||
| staff_idx_t staffGlobalIndex = staff->idx(); | ||
| score->adjustKeySigs(staffGlobalIndex, staffGlobalIndex + 1, keyList); | ||
|
|
||
| score->updateBracesAndBarlines(destinationPart, staffLocalIndex); | ||
|
|
||
| destinationPart->instrument()->setClefType(staffLocalIndex, staff->defaultClefType()); | ||
| } |
There was a problem hiding this comment.
Guard against empty part when appending staff.
Line 652 accesses destinationPart->staff(staffLocalIndex - 1) where staffLocalIndex = destinationPart->nstaves(). If the part has zero staves, this results in accessing staff(-1), causing undefined behavior.
Suggested fix
void EditPart::doAppendStaff(Score* score, Staff* staff, Part* destinationPart, bool createRests)
{
if (!score || !staff || !destinationPart) {
return;
}
+ if (destinationPart->nstaves() == 0) {
+ LOGW("doAppendStaff: cannot append to a part with no staves");
+ return;
+ }
staff_idx_t staffLocalIndex = destinationPart->nstaves();
KeyList keyList = *destinationPart->staff(staffLocalIndex - 1)->keyList();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void EditPart::doAppendStaff(Score* score, Staff* staff, Part* destinationPart, bool createRests) | |
| { | |
| if (!score || !staff || !destinationPart) { | |
| return; | |
| } | |
| staff_idx_t staffLocalIndex = destinationPart->nstaves(); | |
| KeyList keyList = *destinationPart->staff(staffLocalIndex - 1)->keyList(); | |
| staff->setScore(score); | |
| staff->setPart(destinationPart); | |
| score->undoInsertStaff(staff, staffLocalIndex, createRests); | |
| staff_idx_t staffGlobalIndex = staff->idx(); | |
| score->adjustKeySigs(staffGlobalIndex, staffGlobalIndex + 1, keyList); | |
| score->updateBracesAndBarlines(destinationPart, staffLocalIndex); | |
| destinationPart->instrument()->setClefType(staffLocalIndex, staff->defaultClefType()); | |
| } | |
| void EditPart::doAppendStaff(Score* score, Staff* staff, Part* destinationPart, bool createRests) | |
| { | |
| if (!score || !staff || !destinationPart) { | |
| return; | |
| } | |
| if (destinationPart->nstaves() == 0) { | |
| LOGW("doAppendStaff: cannot append to a part with no staves"); | |
| return; | |
| } | |
| staff_idx_t staffLocalIndex = destinationPart->nstaves(); | |
| KeyList keyList = *destinationPart->staff(staffLocalIndex - 1)->keyList(); | |
| staff->setScore(score); | |
| staff->setPart(destinationPart); | |
| score->undoInsertStaff(staff, staffLocalIndex, createRests); | |
| staff_idx_t staffGlobalIndex = staff->idx(); | |
| score->adjustKeySigs(staffGlobalIndex, staffGlobalIndex + 1, keyList); | |
| score->updateBracesAndBarlines(destinationPart, staffLocalIndex); | |
| destinationPart->instrument()->setClefType(staffLocalIndex, staff->defaultClefType()); | |
| } |
RomanPudashkin
force-pushed
the
ports_470
branch
from
e71ef8a to
f12eb94
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
src/engraving/api/tests/score_tests.cpp (1)
1515-1530: 🛠️ Refactor suggestion | 🟠 MajorProtect global
instrumentOrdersfrom leaking state across tests.Line 1519 mutates global state and Line 1530 manually rolls it back; if the test exits early, later tests can inherit polluted order data.
Proposed fix
- instrumentOrders.push_back(testOrder); + const size_t oldOrdersSize = instrumentOrders.size(); + struct InstrumentOrdersGuard { + size_t oldSize; + ~InstrumentOrdersGuard() { instrumentOrders.resize(oldSize); } + } guard { oldOrdersSize }; + instrumentOrders.push_back(testOrder); @@ - // Clean up the added test order - instrumentOrders.pop_back(); + // cleanup handled by guardsrc/engraving/editing/editpart.cpp (3)
693-709:⚠️ Potential issue | 🟡 MinorValidate
voiceIndexbefore forwarding toExcerpt.Line 707 forwards any
intstraight toExcerpt::setVoiceVisible(). Values outside the documented 0-3 range are not rejected here, and negative QML/JS inputs can cause unexpected behavior downstream.Proposed fix
bool EditPart::setVoiceVisible(Score* score, Staff* staff, int voiceIndex, bool visible) { if (!score || !staff) { return false; } + if (voiceIndex < 0 || voiceIndex > 3) { + return false; + } if (!score->excerpt()) { return false; }
711-730:⚠️ Potential issue | 🟠 MajorSynchronize key signatures after inserting the new staves.
The existing staff-insertion paths call
adjustKeySigs()afterundoInsertStaff(), but this helper never does. New parts inserted through this path—andreplacePart(), which delegates to it—can therefore come in with the wrong key signatures for the current score.Proposed fix
void EditPart::insertPart(Score* score, const InstrumentTemplate* templ, size_t index) { if (!score || !templ) { return; } @@ } score->undoInsertPart(part, index); + if (part->nstaves() > 0) { + const staff_idx_t firstStaffIndex = part->staff(0)->idx(); + score->adjustKeySigs(firstStaffIndex, firstStaffIndex + part->nstaves(), score->keyList()); + } score->setUpTempoMapLater(); score->masterScore()->rebuildMidiMapping(); }
645-665:⚠️ Potential issue | 🟠 MajorGuard against empty part when appending staff.
Line 652 accesses
destinationPart->staff(staffLocalIndex - 1)wherestaffLocalIndex = destinationPart->nstaves(). If the part has zero staves, this results in accessingstaff(-1), causing undefined behavior.Proposed fix
void EditPart::doAppendStaff(Score* score, Staff* staff, Part* destinationPart, bool createRests) { if (!score || !staff || !destinationPart) { return; } + if (destinationPart->nstaves() == 0) { + LOGW("doAppendStaff: cannot append to a part with no staves"); + return; + } staff_idx_t staffLocalIndex = destinationPart->nstaves(); KeyList keyList = *destinationPart->staff(staffLocalIndex - 1)->keyList();src/engraving/api/v1/score.cpp (1)
403-412:⚠️ Potential issue | 🟠 MajorValidate
indexbefore casting it tosize_t.Line 411 converts plugin input straight to
size_t. A negative value from QML/JS wraps to a huge unsigned index, soinsertPart(..., -1)will not fail cleanly and can handEditPart::insertPart()a nonsensical position.Proposed fix
void Score::insertPart(const QString& instrumentId, int index) { + const int partCount = static_cast<int>(score()->parts().size()); + if (index < 0 || index > partCount) { + LOGW("insertPart: index %d out of range [0, %d]", index, partCount); + return; + } + const InstrumentTemplate* t = searchTemplate(instrumentId); if (!t) { LOGW("insertPart: <%s> not found", qPrintable(instrumentId)); return; } mu::engraving::EditPart::insertPart(score(), t, static_cast<size_t>(index)); }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ab141cd3-b3fa-43af-a590-ac78d23518d6
📒 Files selected for processing (6)
src/engraving/api/tests/score_tests.cppsrc/engraving/api/v1/score.cppsrc/engraving/api/v1/score.hsrc/engraving/editing/editpart.cppsrc/engraving/editing/editpart.hsrc/notation/internal/notationparts.cpp
| void EditPart::setPartSharpFlat(Score* score, Part* part, PreferSharpFlat sharpFlat) | ||
| { | ||
| if (!score || !part) { | ||
| return; | ||
| } | ||
|
|
||
| Interval oldTransposition = part->staff(0)->transpose(Fraction(0, 1)); | ||
|
|
||
| part->undoChangeProperty(Pid::PREFER_SHARP_FLAT, int(sharpFlat)); | ||
| Transpose::transpositionChanged(score, part, oldTransposition); | ||
| } |
There was a problem hiding this comment.
Guard against part with no staves before accessing staff(0).
Line 390 calls part->staff(0) without checking that the part has at least one staff. If the part has no staves, this will cause undefined behavior.
Proposed fix
void EditPart::setPartSharpFlat(Score* score, Part* part, PreferSharpFlat sharpFlat)
{
if (!score || !part) {
return;
}
+ if (part->nstaves() == 0) {
+ return;
+ }
Interval oldTransposition = part->staff(0)->transpose(Fraction(0, 1));
part->undoChangeProperty(Pid::PREFER_SHARP_FLAT, int(sharpFlat));
Transpose::transpositionChanged(score, part, oldTransposition);
}
|
Tested on Win10, Mac13.7.8, LinuxUbuntu24.04.3 LTS. No regressions found. #32742 also looks good |
RomanPudashkin
force-pushed
the
ports_470
branch
from
f12eb94 to
d44fa34
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (6)
src/engraving/api/tests/score_tests.cpp (1)
1515-1530:⚠️ Potential issue | 🟡 MinorProtect global
instrumentOrdersfrom leaking state across tests.The test mutates global state at Line 1519 and manually rolls it back at Line 1530. If the test exits early (e.g., via
EXPECT_*failure), later tests can inherit polluted order data.Proposed fix using RAII guard
+ const size_t oldOrdersSize = instrumentOrders.size(); + struct InstrumentOrdersGuard { + size_t oldSize; + ~InstrumentOrdersGuard() { instrumentOrders.resize(oldSize); } + } guard { oldOrdersSize }; instrumentOrders.push_back(testOrder); @@ - // Clean up the added test order - instrumentOrders.pop_back(); + // cleanup handled by guardsrc/engraving/api/v1/score.cpp (1)
403-412:⚠️ Potential issue | 🟠 MajorValidate
indexbefore casting it tosize_t.Line 411 converts plugin input straight to
size_t. A negative value from QML/JS wraps to a huge unsigned index, soinsertPart(..., -1)will not fail cleanly and can handEditPart::insertPart()a nonsensical position.Suggested guard
void Score::insertPart(const QString& instrumentId, int index) { + const int partCount = static_cast<int>(score()->parts().size()); + if (index < 0 || index > partCount) { + LOGW("insertPart: index %d out of range [0, %d]", index, partCount); + return; + } + const InstrumentTemplate* t = searchTemplate(instrumentId); if (!t) { LOGW("insertPart: <%s> not found", qPrintable(instrumentId)); return; }src/engraving/editing/editpart.cpp (4)
384-394:⚠️ Potential issue | 🟠 MajorGuard against part with no staves before accessing
staff(0).Line 390 calls
part->staff(0)without checking that the part has at least one staff. If the part has no staves, this will cause undefined behavior.Proposed fix
void EditPart::setPartSharpFlat(Score* score, Part* part, PreferSharpFlat sharpFlat) { if (!score || !part) { return; } + if (part->nstaves() == 0) { + return; + } Interval oldTransposition = part->staff(0)->transpose(Fraction(0, 1)); part->undoChangeProperty(Pid::PREFER_SHARP_FLAT, int(sharpFlat)); Transpose::transpositionChanged(score, part, oldTransposition); }
645-665:⚠️ Potential issue | 🟠 MajorGuard against empty part when appending staff.
Line 652 accesses
destinationPart->staff(staffLocalIndex - 1)wherestaffLocalIndex = destinationPart->nstaves(). If the part has zero staves, this results in accessingstaff(-1), causing undefined behavior.Suggested fix
void EditPart::doAppendStaff(Score* score, Staff* staff, Part* destinationPart, bool createRests) { if (!score || !staff || !destinationPart) { return; } + if (destinationPart->nstaves() == 0) { + LOGW("doAppendStaff: cannot append to a part with no staves"); + return; + } staff_idx_t staffLocalIndex = destinationPart->nstaves(); KeyList keyList = *destinationPart->staff(staffLocalIndex - 1)->keyList();
693-709:⚠️ Potential issue | 🟠 MajorValidate
voiceIndexbefore forwarding toExcerpt.Line 707 forwards any
intstraight toExcerpt::setVoiceVisible(). Values outside the documented 0-3 range are not rejected here, and negative QML/JS inputs can cause unexpected behavior downstream.Suggested fix
bool EditPart::setVoiceVisible(Score* score, Staff* staff, int voiceIndex, bool visible) { if (!score || !staff) { return false; } + if (voiceIndex < 0 || voiceIndex > 3) { + return false; + } if (!score->excerpt()) { return false; }
711-730:⚠️ Potential issue | 🟠 MajorSynchronize key signatures after inserting the new staves.
The existing staff-insertion paths call
adjustKeySigs()afterundoInsertStaff(), but this helper never does. New parts inserted through this path—andreplacePart(), which delegates to it—can therefore come in with the wrong key signatures for the current score.Suggested follow-up
void EditPart::insertPart(Score* score, const InstrumentTemplate* templ, size_t index) { if (!score || !templ) { return; } @@ } score->undoInsertPart(part, index); + if (part->nstaves() > 0) { + const staff_idx_t firstStaffIndex = part->staff(0)->idx(); + score->adjustKeySigs(firstStaffIndex, firstStaffIndex + part->nstaves(), score->keyList()); + } score->setUpTempoMapLater(); score->masterScore()->rebuildMidiMapping(); }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c7199d9c-efd6-479a-9ad8-909bf9eb9413
📒 Files selected for processing (6)
src/engraving/api/tests/score_tests.cppsrc/engraving/api/v1/score.cppsrc/engraving/api/v1/score.hsrc/engraving/editing/editpart.cppsrc/engraving/editing/editpart.hsrc/notation/internal/notationparts.cpp
4 participants
Ports: #32185
Ports: #32636
Ports: #32745
Ports: #31678
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests