Only transpose when instrument transposition has changed#32951
Open
miiizen wants to merge 1 commit intomusescore:masterfrom
Open
Only transpose when instrument transposition has changed#32951miiizen wants to merge 1 commit intomusescore:masterfrom
miiizen wants to merge 1 commit intomusescore:masterfrom
Conversation
📝 WalkthroughWalkthroughThe 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/notation/internal/masternotationparts.cpp`:
- Around line 202-207: oldTranspose is being read after the part's instrument is
mutated so it may already reflect newInstrument.transpose(), causing the
equality check with newTranspose to incorrectly skip calling
engraving::Transpose::transpositionChanged(...). Fix by capturing
mu::engraving::Interval oldTranspose = part ? part->instrument()->transpose() :
mu::engraving::Interval(0,0) before you replace the part's instrument (before
the code that assigns or swaps in newInstrument), keep newTranspose =
newInstrument.transpose() as is, and then perform the oldTranspose !=
newTranspose check and call engraving::Transpose::transpositionChanged(score(),
part, Part::MAIN_INSTRUMENT_TICK, oldTranspose) afterward.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fc9f7437-df79-4d82-9e35-b09deaf5c277
📒 Files selected for processing (1)
src/notation/internal/masternotationparts.cpp
Comment on lines
+202
to
+207
| mu::engraving::Interval oldTranspose = part ? part->instrument()->transpose() : mu::engraving::Interval(0, 0); | ||
| mu::engraving::Interval newTranspose = newInstrument.transpose(); | ||
| // this also transposes all linked parts | ||
| engraving::Transpose::transpositionChanged(score(), part, Part::MAIN_INSTRUMENT_TICK, oldTranspose); | ||
| if (oldTranspose != newTranspose) { | ||
| engraving::Transpose::transpositionChanged(score(), part, Part::MAIN_INSTRUMENT_TICK, oldTranspose); | ||
| } |
There was a problem hiding this comment.
Capture oldTranspose before instrument replacement to avoid skipping required transposition.
oldTranspose on Line 202 is read after Line 196 mutates the part instrument, so it can already be the new value. In real transposition changes, this can make the Line 205 guard false and incorrectly skip transpositionChanged(...).
💡 Proposed fix
- NotationParts::replaceInstrument(instrumentKey, newInstrument, newStaffType);
-
- for (const INotationPartsPtr& parts : excerptsParts()) {
- parts->replaceInstrument(instrumentKey, newInstrument, newStaffType);
- }
-
- mu::engraving::Interval oldTranspose = part ? part->instrument()->transpose() : mu::engraving::Interval(0, 0);
- mu::engraving::Interval newTranspose = newInstrument.transpose();
+ mu::engraving::Interval oldTranspose = part ? part->instrument()->transpose() : mu::engraving::Interval(0, 0);
+ mu::engraving::Interval newTranspose = newInstrument.transpose();
+
+ NotationParts::replaceInstrument(instrumentKey, newInstrument, newStaffType);
+
+ for (const INotationPartsPtr& parts : excerptsParts()) {
+ parts->replaceInstrument(instrumentKey, newInstrument, newStaffType);
+ }
// this also transposes all linked parts
- if (oldTranspose != newTranspose) {
+ if (part && oldTranspose != newTranspose) {
engraving::Transpose::transpositionChanged(score(), part, Part::MAIN_INSTRUMENT_TICK, oldTranspose);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/notation/internal/masternotationparts.cpp` around lines 202 - 207,
oldTranspose is being read after the part's instrument is mutated so it may
already reflect newInstrument.transpose(), causing the equality check with
newTranspose to incorrectly skip calling
engraving::Transpose::transpositionChanged(...). Fix by capturing
mu::engraving::Interval oldTranspose = part ? part->instrument()->transpose() :
mu::engraving::Interval(0,0) before you replace the part's instrument (before
the code that assigns or swaps in newInstrument), keep newTranspose =
newInstrument.transpose() as is, and then perform the oldTranspose !=
newTranspose check and call engraving::Transpose::transpositionChanged(score(),
part, Part::MAIN_INSTRUMENT_TICK, oldTranspose) afterward.
miiizen
force-pushed
the
32393-enharmonicTransp
branch
from
6add974 to
170bd98
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/notation/internal/masternotationparts.cpp (1)
206-208:⚠️ Potential issue | 🔴 CriticalAdd a null guard for
partbefore callingtranspositionChanged.
transpositionChanged()dereferencespart; current guard allows a nullpartpath whenoldTranspose != newTranspose, which can crash.Proposed fix
- if (oldTranspose != newTranspose) { + if (part && oldTranspose != newTranspose) { engraving::Transpose::transpositionChanged(score(), part, Part::MAIN_INSTRUMENT_TICK, oldTranspose); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/notation/internal/masternotationparts.cpp` around lines 206 - 208, The call to engraving::Transpose::transpositionChanged(score(), part, Part::MAIN_INSTRUMENT_TICK, oldTranspose) can dereference a nullptr because we only check oldTranspose != newTranspose; add a null guard for the part pointer before invoking transpositionChanged (e.g., check that part is non-null with if (part) or bail out/return when part is null) so that transpositionChanged is only called when part is valid.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/notation/internal/masternotationparts.cpp`:
- Around line 206-208: The call to
engraving::Transpose::transpositionChanged(score(), part,
Part::MAIN_INSTRUMENT_TICK, oldTranspose) can dereference a nullptr because we
only check oldTranspose != newTranspose; add a null guard for the part pointer
before invoking transpositionChanged (e.g., check that part is non-null with if
(part) or bail out/return when part is null) so that transpositionChanged is
only called when part is valid.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ea70b09b-1aeb-4e03-b273-3a29816ad6ef
📒 Files selected for processing (1)
src/notation/internal/masternotationparts.cpp
Contributor
|
@miiizen Screencast.from.2026-04-09.11-43-24.webmAlso, could we refine this further to not reset spelling when changing to an octave transposing instrument with the same transposition pitch class? E.g. Clar. in Bb -> Bass Clarinet in Bb, Alto Sax -> Baritone Sax, etc. (This behaviour is already in place for octave transposing instruments in C, e.g. Flute -> Piccolo in C). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves: #32393