Skip to content

Don't remove outgoing ties when incrasing chord duration#32982

Open
MaxiSchwindler wants to merge 2 commits intomusescore:masterfrom
MaxiSchwindler:31608_preserve_tied_chord_duration
Open

Don't remove outgoing ties when incrasing chord duration#32982
MaxiSchwindler wants to merge 2 commits intomusescore:masterfrom
MaxiSchwindler:31608_preserve_tied_chord_duration

Conversation

@MaxiSchwindler
Copy link
Copy Markdown
Contributor

@MaxiSchwindler MaxiSchwindler commented Apr 11, 2026

Resolves: #31608

Ensures that the total length of a tied chord doesn't decrease when increasing the duration of a chord contained in the tie.

Additionally, adds a tieNotesTogether utility method, since the more-or-less same lines were repeated a few times in the codebase. I only replaced existing code with this utility method where I was reasonably sure that its 1:1 the same; the only potential difference in some places is that note->setTieFor and note->setTieBack are actually called, which I assume to be what should happen anyways.

Note: I made the change while on 4.7 branch, and cherry-picked to master; there were no conflicts, so this should run fine; but I haven't managed to build master yet

  • 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

Release Notes

  • Refactor
    • Improved internal tie handling mechanism for note operations, enhancing consistency and reliability during chord restructuring and note manipulation.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 11, 2026

📝 Walkthrough

Walkthrough

A refactoring that introduces a new Score::tieNotesTogether() helper function to consolidate repeated tie creation logic, applied across multiple functions in noteentry, cmd, and edit files. Additionally, changeCRlen() is enhanced to preserve ties when chord duration is modified without requiring rest insertion.

Changes

Cohort / File(s) Summary
Tie Helper Definition
src/engraving/dom/score.h, src/engraving/dom/score.cpp
Added new public method tieNotesTogether(Note* n1, Note* n2) that centralizes tie creation, initialization, and undo registration logic.
Tie Helper Adoption - Repitching & Combining
src/engraving/dom/noteentry.cpp, src/engraving/editing/cmd.cpp
Replaced inline Tie creation and setup with tieNotesTogether() calls in repitchReplaceNote(), createCRSequence(), and cmdImplode(); added tie-preservation logic in changeCRlen() to detect and reapply ties when chord extension reduces total duration.
Tie Helper Adoption - Input & Regrouping
src/engraving/editing/edit.cpp
Replaced manual tie construction in addChord(), addTiedMidiPitch(), and regroupNotesAndRests() with tieNotesTogether() calls.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 45.45% 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 describes the main fix: preventing removal of outgoing ties when increasing chord duration, which directly addresses issue #31608.
Description check ✅ Passed The PR description includes issue reference, clear motivation, and completes most checklist items. However, the unit test checkbox is unchecked, and the author notes they haven't built master yet.
Linked Issues check ✅ Passed The PR directly addresses issue #31608 by adding logic to preserve outgoing ties when chord duration increases, preventing loss of ties during duration changes.
Out of Scope Changes check ✅ Passed The PR introduces the tieNotesTogether utility method and refactors existing code to use it, which is within scope as it improves code maintainability for the tie-handling fix.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
⚔️ Resolve merge conflicts
  • Resolve merge conflict in branch 31608_preserve_tied_chord_duration

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

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: 4ba9b0e6-00a5-4f85-85ef-260011cde5e5

📥 Commits

Reviewing files that changed from the base of the PR and between 6f59207 and d7c8c61.

📒 Files selected for processing (5)
  • src/engraving/dom/noteentry.cpp
  • src/engraving/dom/score.cpp
  • src/engraving/dom/score.h
  • src/engraving/editing/cmd.cpp
  • src/engraving/editing/edit.cpp

Comment thread src/engraving/dom/score.cpp
Comment on lines +1861 to +1873
Fraction endTickAfter = cr->isChord() ? toChord(cr)->endTickIncludingTied() : cr->endTick();
while (endTickAfter < endTickBefore) { // making a chord *longer* shouldn't _shorten_ its total duration
if (!oc){ // everything up to oc should already be tied together
oc = toChord(cr1);
}

for (Note* n: oc->notes()){
if (Note* nn = searchTieNote(n)){
tieNotesTogether(n, nn);
}
}
oc = oc->next();
endTickAfter = cr->isChord() ? toChord(cr)->endTickIncludingTied() : cr->endTick();
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

Only recreate ties for notes that were originally tied forward.

Line 1867 currently retries every note in oc if searchTieNote() finds a match. On mixed chords, that can introduce brand-new ties on notes that were previously untied. Please capture the original outgoing-tie membership before mutating the chain and filter this loop to that subset only.

Comment on lines +1897 to 1898
Tie* tie = tieNotesTogether(nl1[j], nl2[j]);
ties.push_back(tie);
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 | 🔴 Critical

Avoid double-registering ties in undo stack.

tieNotesTogether(...) already performs undoAddElement(tie), but these ties are still re-added in the later loop at Line 1966–1969. This can create duplicate undo entries / invalid state in regrouping.

🔧 Proposed fix
@@
-                                    Tie* tie = tieNotesTogether(nl1[j], nl2[j]);
-                                    ties.push_back(tie);
+                                    ties.push_back(tieNotesTogether(nl1[j], nl2[j]));
@@
-                        if (tieBack[i]) {
-                            Tie* tie = Factory::createTie(this->dummy());
-                            tie->setStartNote(tieBack[i]);
-                            tie->setEndNote(n);
-                            tie->setTick(tie->startNote()->tick());
-                            tie->setTick2(tie->endNote()->tick());
-                            tie->setTrack(track);
-                            n->setTieBack(tie);
-                            tieBack[i]->setTieFor(tie);
-                            ties.push_back(tie);
-                        }
-                        if (tieFor[i]) {
-                            Tie* tie = Factory::createTie(this->dummy());
-                            tie->setStartNote(nn);
-                            tie->setEndNote(tieFor[i]);
-                            tie->setTick(tie->startNote()->tick());
-                            tie->setTick2(tie->endNote()->tick());
-                            tie->setTrack(track);
-                            nn->setTieFor(tie);
-                            tieFor[i]->setTieBack(tie);
-                            ties.push_back(tie);
-                        }
+                        if (tieBack[i]) {
+                            ties.push_back(tieNotesTogether(tieBack[i], n));
+                        }
+                        if (tieFor[i]) {
+                            ties.push_back(tieNotesTogether(nn, tieFor[i]));
+                        }
@@
-                    if (!ties.empty()) {         // at least one tie was created
-                        for (Tie* tie : ties) {
-                            undoAddElement(tie);
-                        }
-                        connectTies();
-                    }
+                    if (!ties.empty()) {         // at least one tie was created
+                        connectTies();
+                    }

@mathesoncalum mathesoncalum requested a review from miiizen April 13, 2026 09:00
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.

Changing two notes to dotted equivalent removes tie to next note

3 participants