Fix instrument change clef not reacting to concert pitch toggle

[CubikingChill]

Resolves: #32614

• I signed the CLA
• 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
• If changes are extensive, there is a sequence of easily reviewable commits
• The code in the PR follows the coding rules
• There are no unnecessary changes
• The code compiles and runs on my machine, preferably after each commit individually
• I created a unit test or vtest to verify the changes I made (if applicable)

Summary by CodeRabbit

• Bug Fixes
  • Improved clef handling during instrument changes to properly track and update both concert and transposing clef information, ensuring more accurate clef transitions when instruments change.

[CubikingChill]

Please do not merge at the moment. I am still working on the functionality of the code.

[shoogle]

Code comments in English please.

If these are just notes to help you understand things and remember what they do, that's fine. Just make sure you remove them all (or rewrite them in English) before you mark the PR as "ready for review".

But if you're using AI, we shouldn't be able to tell. The code should look how it would look if you had written it all yourself; the AI should just be used to arrive at the end result sooner.

[CubikingChill]

Your guess is correct. I am using some level of AI to skip reading all existing code. But I can assure you that once this PR goes out of draft. It will be clean.

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR modifies instrument change clef handling to fix concert pitch synchronization issues. The changes introduce a parameter-multiplexing approach: InstrumentChange::setupInstrument() now packs both concert and transposing clef enum values into an integer, which Score::undoChangeClef() decodes to apply both clefs independently during instrument changes.

Changes

Cohort / File(s)	Summary
Score Signature & Implementation src/engraving/dom/score.h, src/engraving/editing/edit.cpp	Changed undoChangeClef method's forInstrumentChange parameter from bool to int to support packed clef data. Implementation now decodes the integer into two ClefType values and applies them independently for instrument changes, bypassing staff group filtering and using setTransposingClef/setConcertClef instead of setClefType.
Instrument Change Caller src/engraving/dom/instrchange.cpp	Modified setupInstrument() to retrieve both concert and transposing clefs per staff, detect changes on either side, and pass packed clef data (offset + concert clef + transposing clef shifted) to undoChangeClef instead of a single clef type.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the primary fix: addressing instrument change clef unresponsiveness to concert pitch toggle.
Description check	✅ Passed	The PR description references issue #32614, fills most required checklist items, and demonstrates understanding of the fix. However, code compilation/run and testing verification remain unchecked.
Linked Issues check	✅ Passed	Code changes implement the proposed technical solution from #32614: upgrading forInstrumentChange parameter from bool to int to pack both concert and transposing clef data, addressing root cause of incorrect clef inheritance during instrument changes.
Out of Scope Changes check	✅ Passed	All changes directly support the linked issue objective: InstrumentChange now retrieves and packs both clef types, Score::undoChangeClef parameter signature updated, and edit.cpp implements unpacking logic for correct clef initialization.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

[coderabbitai]

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

src/engraving/editing/edit.cpp (2)

6214-6253: ⚠️ Potential issue | 🔴 Critical

Fix the IC staff-group guard before it hits ClefType::INVALID.

Line 6251 still evaluates ClefInfo::staffGroup(ct) before the !isIC check. In the packed instrument-change path, ct is intentionally ClefType::INVALID, so this can trip invalid-enum lookup/asserts in the exact flow this PR adds. Also, once reordered, the IC path still needs a staff-group compatibility check based on cp/tp rather than skipping it entirely.

Suggested fix

-        if (ClefInfo::staffGroup(ct) != staffGroup && !isIC) {
-            continue;
-        }
+        if (isIC) {
+            if (cp == ClefType::INVALID || tp == ClefType::INVALID
+                || ClefInfo::staffGroup(cp) != staffGroup
+                || ClefInfo::staffGroup(tp) != staffGroup) {
+                continue;
+            }
+        } else if (ClefInfo::staffGroup(ct) != staffGroup) {
+            continue;
+        }

---

6176-6326: 🧹 Nitpick | 🔵 Trivial

Please lock this packed IC path down with a regression.

This function now owns the special concert/transposing-clef initialization that fixes #32614. A test that toggles Concert Pitch after multiple instrument changes, then deletes an earlier IC, would make future regressions here much easier to catch.

If you want, I can draft the regression scenario for the existing engraving test framework.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: f24ce917-5a94-4e5f-aad2-22f844cfb0c9

📥 Commits

Reviewing files that changed from the base of the PR and between c3842f5 and 142bdd6.

📒 Files selected for processing (3)

• src/engraving/dom/instrchange.cpp
• src/engraving/dom/score.h
• src/engraving/editing/edit.cpp

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Compare full clef types instead of glyph IDs.

Line 105 and Line 106 compare only ClefInfo::symId(...). Different clef types can share the same symbol, so required IC clef updates may be skipped.

Proposed fix

-        if (ClefInfo::symId(oldClefTypeList.concertClef) != ClefInfo::symId(newClefTypeList.concertClef)
-            || ClefInfo::symId(oldClefTypeList.transposingClef) != ClefInfo::symId(newClefTypeList.transposingClef)) {
+        if (oldClefTypeList.concertClef != newClefTypeList.concertClef
+            || oldClefTypeList.transposingClef != newClefTypeList.transposingClef) {

📝 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.

Suggested change

	if (ClefInfo::symId(oldClefTypeList.concertClef) != ClefInfo::symId(newClefTypeList.concertClef)
	|| ClefInfo::symId(oldClefTypeList.transposingClef) != ClefInfo::symId(newClefTypeList.transposingClef)) {
	if (oldClefTypeList.concertClef != newClefTypeList.concertClef
	|| oldClefTypeList.transposingClef != newClefTypeList.transposingClef) {

[coderabbitai]

🛠️ Refactor suggestion | 🟠 Major

Extract IC clef packing into shared constants/helper.

Line 109 uses inline 1000 and 0xFF. Please move this packing contract to a shared helper (or shared constants) used by both encode/decode sites to prevent divergence.

Refactor direction

-            int cp = static_cast<int>(newClefTypeList.concertClef);
-            int tp = static_cast<int>(newClefTypeList.transposingClef);
-            int packedData = 1000 + (cp & 0xFF) + ((tp & 0xFF) << 8);
+            constexpr int kIcClefPackOffset = 1000;
+            constexpr int kIcClefMask = 0xFF;
+            const int cp = static_cast<int>(newClefTypeList.concertClef);
+            const int tp = static_cast<int>(newClefTypeList.transposingClef);
+            const int packedData = kIcClefPackOffset + (cp & kIcClefMask) + ((tp & kIcClefMask) << 8);

📝 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.

Suggested change

	int cp = static_cast<int>(newClefTypeList.concertClef);
	int tp = static_cast<int>(newClefTypeList.transposingClef);
	int packedData = 1000 + (cp & 0xFF) + ((tp & 0xFF) << 8);
	constexpr int kIcClefPackOffset = 1000;
	constexpr int kIcClefMask = 0xFF;
	const int cp = static_cast<int>(newClefTypeList.concertClef);
	const int tp = static_cast<int>(newClefTypeList.transposingClef);
	const int packedData = kIcClefPackOffset + (cp & kIcClefMask) + ((tp & kIcClefMask) << 8);

[coderabbitai]

🛠️ Refactor suggestion | 🟠 Major

Type change from bool to int with false default is semantically confusing.

While false implicitly converts to 0 in C++, using a boolean literal as the default for an int parameter obscures the intent. Additionally, the parameter name forInstrumentChange no longer accurately describes its content when it carries packed clef data.

Consider:

1. Change the default to 0 for clarity.
2. Rename the parameter (e.g., instrumentChangeData or clefPackedData) to reflect its dual purpose.
3. Add a brief doc comment explaining the packing scheme (1000 + concertClef + (transposingClef << 8)), or define named constants for the magic offset.

Proposed minimal fix for default value

-    void undoChangeClef(Staff* ostaff, EngravingItem*, ClefType st, int forInstrumentChange = false, Clef* clefToRelink = nullptr);
+    void undoChangeClef(Staff* ostaff, EngravingItem*, ClefType st, int forInstrumentChange = 0, Clef* clefToRelink = nullptr);

For a more robust API, consider introducing a small struct or enum to encapsulate the instrument-change clef data instead of relying on magic-number packing in a public interface.

📝 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.

Suggested change

	void undoChangeClef(Staff* ostaff, EngravingItem*, ClefType st, int forInstrumentChange = false, Clef* clefToRelink = nullptr);
	void undoChangeClef(Staff* ostaff, EngravingItem*, ClefType st, int forInstrumentChange = 0, Clef* clefToRelink = nullptr);

[mike-spa]

Closing because the code was not compiled before contributing, and because clefs, key sigs and time sigs management are a very fragile part of our code base that has been in long need of a refactor and where it's not safe to accept contributions.

See also #32755 (comment)