Fix stale instrument-change keys after editing or deleting a preceding key in linked scores by 0xMashiro · Pull Request #34848 · musescore/MuseScore

0xMashiro · 2026-09-08T08:43:45Z

Resolves: #34847

Changing or deleting an explicit key before an instrument change can leave the later instrument-change key stale in the other linked score. For the supplied flute-to-B-flat-clarinet examples, editing the opening key updates the part's later key to three sharps while the full score keeps two; deleting the preceding key restores two sharps in the part while the full score keeps three.

Dispatch subsequent instrument-change key updates on each linked staff's owning score in EditKeySig::undoChangeKeySig. Independently, follow linked staves after key deletion and update each owning score. The change preserves existing global/local key policy and the helper's stopping rule at the next explicit key.

Validation

Native suite: Engraving_InstrumentChangeTests.*:Engraving_KeySigTests.* — 12 tests pass (Ubuntu, Qt 6.10.2, GCC 14, Debug/ASan).
The two regressions in keysig_tests.cpp share a two-measure, one-staff MSCX and create the linked part with TestUtils::createPart. They separately change and delete the preceding key, entering from both the full score and part, and check the key map and key-signature element in both scores before/after the edit, undo, and redo.
The same final tests fail on the unpatched PR baseline a64e0d0c8725a89af2b4b9f8e058a75be20c01d3 with stale instrument-change keys. Both production corrections are retained; an ownership assertion now documents the receiver/staff invariant.
Both changed production translation units compile independently. Official Uncrustify formatting and git diff --check pass.
Earlier desktop recordings and production MSCZ save/reopen checks remain supplementary evidence. The maintained regressions focus on in-memory propagation and no longer implement archive writing or toggle global test mode.

Before and after recordings

These recordings use the original desktop examples; the regression tests now use the smaller fixture described above.

Change a preceding key — Before:

change-before.mp4

Change a preceding key — After:

change-after.mp4

Delete a preceding key — Before:

delete-before.mp4

Delete a preceding key — After:

delete-after.mp4

Related work and review scope

Related reports #18590 item 5 (same-score concert-pitch transposition) and #23934 (global keys across different instruments) are outside the claimed fix. Historical PRs #23053, #23975 and #26315 are closed and unmerged; no active matching fix was identified in the bounded search.

AI was used to improve efficiency during development.

Contributor checklist

I signed the CLA: signed.
The title of the PR describes the problem it addresses.
Each commit's message describes its purpose and effects, and references the issue it resolves.
The code in the PR follows the coding rules.
I understand all aspects of the code I'm contributing and I'm able to explain it if requested.
The code compiles and runs on my machine.
I listed related prior attempts and described how this change differs from them; search limitations are recorded above.
There are no unnecessary changes.
I created unit tests to verify the changes.

Dispatch later instrument-change key updates on each linked staff owner when changing an explicit key. Traverse linked staves independently after key deletion so the other score does not retain a stale later key. Add separate edit and delete regressions covering master and excerpt entry points, DOM/key-map agreement, undo, redo, integrity checks, and MSCZ save/reopen. All 12 native instrument-change/key tests pass. Both new tests fail on the unmodified upstream baseline; equivalent desktop before/after recordings confirm both paths. Refs: musescore#34847

coderabbitai · 2026-09-08T09:03:37Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 9241f648-10c7-4363-bd7b-98876844c5cd

📥 Commits

Reviewing files that changed from the base of the PR and between b16fb4f and 2b81b17.

📒 Files selected for processing (3)

src/engraving/editing/edit.cpp
src/engraving/tests/keysig_data/linked-instrument-change.mscx
src/engraving/tests/keysig_tests.cpp

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.

📝 Walkthrough

Walkthrough

The change routes instrument-change transposition updates through the score that owns each linked staff. Key-signature deletion now updates every linked staff score. An assertion verifies the staff belongs to the receiving score. New fixture data and tests cover preceding key changes and deletions across master scores and excerpts, including undo and redo.

Priority: ➖ Normal

Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to 2b81b

Linked full scores and parts now keep instrument-change keys synchronized after preceding key edits or deletions, with regression coverage for both entry points and undo/redo. No current merge-blocking risk remains.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 15.38% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 13 functions across 4 files. (1 skipped: …	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The changes address issue `#34847`. They update instrument-change keys through each linked staff's owning score for both edit and deletion paths, cover operations initiated from the full score and linke…
Out of Scope Changes check	✅ Passed	The production changes, fixture, and regression tests are limited to linked-score instrument-change key synchronization. The description explicitly excludes unrelated transposition issues, and no unre…
Description check	✅ Passed	The description identifies issue `#34847`, explains the cause and fix, documents validation, related work, and review scope, and completes all required checklist items.
Title check	✅ Passed	The title clearly and concisely describes the main fix: preventing stale instrument-change keys after editing or deleting a preceding key in linked scores.

Full details: Docstring Coverage

Explanation

Docstring coverage is 15.38% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 13 functions across 4 files. (1 skipped: 1 unsupported.)

Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

Warning

Linked repositories: Public OSS repositories can only analyze public repositories installed in this organization. No linked repositories were analyzed; skipped musescore/muse_framework.git.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

ajuncosa

The fix itself seems reasonable. However, the tests need some re-working or at least some explanation:

the bug is about in-memory propagation across linked scores. Is it really necessary to do a MscWriter/MscSaver round-trip for this specific case? If you do want to do it for some reason, maybe it'd be cleaner to just use the ScoreComp::saveCompareScore()helper? Test mode already inlines excerpts into the master's XML.
I don't think the .mscz test files are minimal examples for the specific tests (e.g. unused measures and parts). We could probably even use a single .mscx for both tests.
nitpick: the fix is in in editkeysig.cpp and the keysig delete path, so keysig_tests.cpp might be a better place to put them?
see inline comments

ajuncosa · 2026-09-08T17:30:18Z

+        {
+            // Write production MSCZ excerpts separately, without test-mode inline excerpts.
+            const bool testMode = MScore::testMode;
+            DEFER { MScore::testMode = testMode;


why toggle the the global MScore::testMode inside a test?

The toggle was intended to exercise separately stored MSCZ excerpts, but that is unnecessary for this in-memory regression. I removed the custom archive round-trip and the test-mode toggle; the earlier save/reopen check remains supplementary evidence.

ajuncosa · 2026-09-08T17:56:03Z

+#include "global/defer.h"
+#include "global/io/buffer.h"
+#include "global/io/file.h"
+#include "engraving/rw/mscsaver.h"
+#include "engraving/dom/mscore.h"
+#include "engraving/dom/excerpt.h"
+#include "engraving/dom/keysig.h"
+#include "engraving/editing/editkeysig.h"
+#include "engraving/editing/transaction/transaction.h"


please review our coding guidelines for sorting/grouping recommendations and ensure only needed things are included

Removed the archive-related includes and moved the regressions to keysig_tests.cpp. The remaining includes are grouped and sorted according to the guidelines.

ajuncosa · 2026-09-08T18:15:36Z

+            KeySigEvent key;
+            key.setConcertKey(Key::G);
+            score->startCmd(TranslatableString::untranslatable("Change preceding key"));
+            EditKeySig::undoChangeKeySig(score->transactionManager()->currentOrDummyTransaction(), owner, owner->staff(0), Fraction(0,


EditKeySig::undoChangeKeySig isn't really covering the same scope as owner->cmdDeleteSelection() (the change case vs. the delete case). Is this intended? Can you explain what we're really testing in these two tests?

Yes: the change test targets propagation in EditKeySig::undoChangeKeySig, following the existing keysig test. Deletion needs the Score::deleteItem path reached by cmdDeleteSelection; removing the element alone would miss the update being tested. I split the boolean-driven helper into two explicit tests. Both now edit the same mid-measure key, run from the master and part, and check both scores after the operation, undo, and redo.

Move the change/delete cases to the key-signature suite and share a minimal two-measure MSCX using TestUtils::createPart. Keep each command path explicit and check both scores through undo and redo. Remove custom archive writing and global test-mode changes; assert the staff/score ownership invariant. The same final regressions fail on the unpatched baseline while its 10 existing tests pass. All 12 key-signature/instrument-change tests pass with the fix. Both production translation units compile independently, and the changed code passes official formatting and diff checks. Refs: musescore#34847

0xMashiro · 2026-09-08T21:10:37Z

Updated in 2b81b17. The regressions now live in keysig_tests.cpp and share one two-measure MSCX, with the part created by TestUtils::createPart. The change and delete paths are explicit; both check the master and part through undo/redo. I removed the custom MSCZ round-trip and added the ownership assertion.

With the same final tests, the unpatched baseline passes the 10 existing tests and fails both new regressions on stale keys. The fix passes all 12 tests. Standalone compilation of both production translation units and the formatting checks also pass.

The existing recordings remain supplementary evidence using the original desktop examples. The PR description now distinguishes those from the smaller regression fixture.

@ajuncosa, could you take another look when you have time?

0xMashiro mentioned this pull request Sep 8, 2026

Instrument-change keys become stale in linked scores after changing or deleting a preceding key #34847

Open

4 tasks

0xMashiro marked this pull request as ready for review September 8, 2026 08:53

avvvvve requested a review from miiizen September 8, 2026 14:04

miiizen requested review from ajuncosa and removed request for miiizen September 8, 2026 14:17

ajuncosa requested changes Sep 8, 2026

View reviewed changes

ajuncosa self-requested a review September 9, 2026 15:38

Navigation Menu

Uh oh!

Fix stale instrument-change keys after editing or deleting a preceding key in linked scores - #34848

Fix stale instrument-change keys after editing or deleting a preceding key in linked scores#34848
0xMashiro wants to merge 2 commits into
musescore:mainfrom
0xMashiro:fix/linked-instrument-change-keys

0xMashiro commented Sep 8, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Sep 8, 2026 •

edited

Loading

Walkthrough

❌ Failed checks (1 warning)

Uh oh!

ajuncosa left a comment

Uh oh!

Uh oh!

ajuncosa Sep 8, 2026

Uh oh!

0xMashiro Sep 8, 2026

Uh oh!

Uh oh!

Uh oh!

ajuncosa Sep 8, 2026

Uh oh!

0xMashiro Sep 8, 2026

Uh oh!

ajuncosa Sep 8, 2026

Uh oh!

0xMashiro Sep 8, 2026

Uh oh!

0xMashiro commented Sep 8, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

0xMashiro commented Sep 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Validation

Before and after recordings

Related work and review scope

Contributor checklist

Uh oh!

coderabbitai Bot commented Sep 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

❌ Failed checks (1 warning)

Uh oh!

ajuncosa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ajuncosa Sep 8, 2026

Choose a reason for hiding this comment

Uh oh!

0xMashiro Sep 8, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ajuncosa Sep 8, 2026

Choose a reason for hiding this comment

Uh oh!

0xMashiro Sep 8, 2026

Choose a reason for hiding this comment

Uh oh!

ajuncosa Sep 8, 2026

Choose a reason for hiding this comment

Uh oh!

0xMashiro Sep 8, 2026

Choose a reason for hiding this comment

Uh oh!

0xMashiro commented Sep 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

0xMashiro commented Sep 8, 2026 •

edited

Loading

coderabbitai Bot commented Sep 8, 2026 •

edited

Loading

0xMashiro commented Sep 8, 2026 •

edited

Loading