Skip to content

not updating dead notes for transposition#32856

Open
alexpavlov96 wants to merge 1 commit intomusescore:masterfrom
alexpavlov96:not_updating_deadnotes
Open

not updating dead notes for transposition#32856
alexpavlov96 wants to merge 1 commit intomusescore:masterfrom
alexpavlov96:not_updating_deadnotes

Conversation

@alexpavlov96
Copy link
Copy Markdown
Contributor

@alexpavlov96 alexpavlov96 commented Mar 30, 2026

Summary by CodeRabbit

  • Bug Fixes
    • Improved handling of dead (inactive) notes so they are skipped during transposition, accidental-state updates, and chord fretting operations.
    • Prevented accidental removal or pitch-change actions for inactive notes, avoiding unintended modifications.
    • Ensured inactive notes no longer participate in string/fret conflict resolution or fretting reassignment, improving stability.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 30, 2026

📝 Walkthrough

Walkthrough

Adds early-exit guards and skip conditions for notes where note->deadNote() is true across accidental updates, transposition, and fretting/string-assignment logic, so those operations return or skip when a note is dead.

Changes

Cohort / File(s) Summary
Note handling
src/engraving/dom/note.cpp
Added early returns when deadNote() is true in updateAccidental(AccidentalState*), transposeDiatonic(...), and transpose(Interval ...), including undo/removal calls and rel-line adjustments where applicable.
String/fret assignment
src/engraving/dom/stringdata.cpp
Excluded dead notes from fretChords(), sortChordNotesUseSameString(), and sortChordNotes() flows: skip processing after clearing fretConflict, avoid per-note fret recalculation/reset for dead notes, and guard clearing of string/fret by !note->deadNote().
Transpose utility
src/engraving/editing/transpose.cpp
Added note->deadNote() early-exit in Transpose::transposeNote() to return true without invoking transposeDiatonic/transpose on dead notes.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Description check ⚠️ Warning The pull request description is empty and does not follow the required template structure with motivation, issue reference, or checklist items. Add a complete description following the template: include the issue number (Resolves: #32856), a brief explanation of the changes and motivation, and fill out all required checklist items.
Docstring Coverage ⚠️ Warning Docstring coverage is 57.14% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main objective of the changeset: preventing updates to dead notes during transposition operations across multiple files.

✏️ 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.

@alexpavlov96 alexpavlov96 force-pushed the not_updating_deadnotes branch from 3f60481 to 9a292d9 Compare March 30, 2026 14:38
@alexpavlov96 alexpavlov96 force-pushed the not_updating_deadnotes branch from 9a292d9 to 076e9ce Compare March 30, 2026 14:44
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 5930033e-0485-4d65-b36a-dbcd9191b191

📥 Commits

Reviewing files that changed from the base of the PR and between 3f60481 and 076e9ce.

📒 Files selected for processing (3)
  • src/engraving/dom/note.cpp
  • src/engraving/dom/stringdata.cpp
  • src/engraving/editing/transpose.cpp

Comment on lines +2143 to +2145
if (m_accidental) {
score()->undoRemoveElement(m_accidental);
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Don't delete persisted accidental state in updateAccidental().

Line 2144 removes the Accidental element from the model, not just from layout. The ElementType::ACCIDENTAL branch in Note::remove() clears m_centOffset, and playingTuning() derives tuning from that field, so a dead-note refresh can silently discard microtonal tuning plus any user/courtesy accidental metadata. If dead notes should suppress accidentals visually, this needs to happen without removing the stored accidental state.

Comment on lines +2147 to +2149
as->setForceRestateAccidental(absLine, false);
updateRelLine(absLine, true);
return;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Don't consume forceRestateAccidental on a skipped note.

Line 2147 clears a one-shot flag in the shared AccidentalState even though this branch is explicitly skipping the note. If an earlier note or ornament set forceRestateAccidental for this pitch line, a dead note in between will swallow that request and the next live note will miss its courtesy/restated accidental.

Suggested fix
-        as->setForceRestateAccidental(absLine, false);
         updateRelLine(absLine, true);
         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.

Suggested change
as->setForceRestateAccidental(absLine, false);
updateRelLine(absLine, true);
return;
updateRelLine(absLine, true);
return;

Comment on lines +249 to +251
if (note->deadNote()) {
continue;
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

The dead-note guard happens after the first write.

Line 248 already clears fretConflict before this continue, and Lines 305-311 can set it again afterwards. So dead notes are still being rewritten in fretChords(). If they are meant to stay untouched, the guard needs to move before the first mutation and be mirrored in the final conflict pass as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant