Implement partial & repeat ties #25745

miiizen · 2024-12-05T14:35:44Z

Resolves: #25426

This PR introduces partial ties to MuseScore, enabling ties to be connected across repeats. When adding a tie before a repeat item (repeat barlines, markers, jumps, and voltas) if any note on the other side of the repeat destination can be tied, a partial tie will be added.
When more than one endpoint is available, clicking the outgoing tie will open a menu of possible endpoints. These can be toggled on and off.

Screen.Recording.2024-12-05.at.14.33.29.mov

Playback implementation to follow.

mike-spa · 2024-12-19T09:03:15Z

src/engraving/compat/midi/compatmidirender.cpp

@@ -1080,7 +1080,7 @@ std::set<size_t> CompatMidiRender::getNotesIndexesToRender(Chord* chord)
    }

    auto noteShouldBeRendered = [](Note* n) {
-        while (n->tieBack() && n != n->tieBack()->startNote()) {
+        while (n->tieBack() && !n->incomingPartialTie() && n != n->tieBack()->startNote()) {


I've seen this check repeated several times: every time you are looking for a tie-back-but-not-incoming-partial. Perhaps this is worth adding a tieBackNonPartial() method?

mike-spa · 2024-12-19T09:04:50Z

src/engraving/dom/edit.cpp

+static Tie* createAndAddTie(Note* startNote, Note* endNote)
+{
+    Score* score = startNote->score();
+    const bool createPartialTie = !endNote;


I actually find it more readable without this additional bool that's just the negation of another

mike-spa · 2024-12-19T09:10:20Z

src/engraving/dom/excerpt.cpp

        Tie* tie = toTie(originalNote->tieFor()->linkedClone());
        tie->setScore(score);
        newNote->setTieFor(tie);
        tie->setStartNote(newNote);
        tie->setTrack(newNote->track());
        tieMap.add(originalNote->tieFor(), tie);
    }
-    if (originalNote->tieBack()) {
+    if (originalNote->tieBack() && originalNote->tieBack()->type() == ElementType::TIE) {


Same as above (kind of). Maybe we could modify tieBack and tieFor to return only "real" ties, and not LV and/or partial ties?

I considered this, but it seemed to me that there were far more scenarios where we want tieFor and tieBack to return LVs & partial ties than when we didn't.

mike-spa · 2024-12-19T09:29:19Z

src/engraving/dom/measure.cpp

+
+    Segment* seg = first(SegmentType::ChordRest);
+    while (seg) {
+        for (track_idx_t track = startTrack; track < ntracks; track++) {


Since you're checking all the available tracks, I think you can more compactly do
for (EngravingItem* item : seg->elist()) {}

mike-spa · 2024-12-19T09:59:02Z

src/engraving/dom/note.cpp

        tieFor()->endNote()->setTieBack(tieFor());
    }
 }

+bool Note::followingJumpItem()


From the name of this function I expected it to return an EngravingItem. You could either rename it into hasFollowingJumpItem, or it seems like it could also be useful to return the items themselves rather than bools?

mike-spa · 2024-12-19T10:12:50Z

src/engraving/dom/note.cpp

+    return false;
+}
+
+String Note::precedingJumpItemName()


I think this function really should not be here. At a first glance, I thought this was the symmetric of followingJumpItem but it looks like it has a completely different use and the fact that it returns a String left me quite confused.

From what I understand, the string returned by this function has a UI-related purpose which is exclusively referred to partial ties, so it definitely should be in the PartialTie class (perhaps as a static method)

mike-spa · 2024-12-19T10:28:05Z

src/engraving/dom/tie.h

+    void toggleEndPoint(const String& id);
+
+    void addTie(TieEndPoint* endPoint);
+    void removeTie(TieEndPoint* endPoint);


The fact that there is a public add method but also an addTie is a bit odd. Someone reading the public interface of your class wouldn't know why they are different and which one they should use. Could there be a more descriptive name?

mike-spa · 2024-12-19T10:30:50Z

src/engraving/dom/tie.cpp

+{
+    Note* note = endPoint->note();
+    Score* score = note ? note->score() : nullptr;
+    Tie* _startTie = startTie();


mike-spa · 2024-12-19T11:04:25Z

src/engraving/dom/tie.cpp

+        if (std::find_if((*tieEndPoints()).begin(), (*tieEndPoints()).end(), findEndTie) != (*tieEndPoints()).end()) {
+            continue;
+        }
+        score()->undoRemoveElement(tie);


A method of the Tie class should not be removing other ties from the Score, it could be a bit dangerous. Also, from a function named collectPossibleEndPoints I'd probably expect something which returns me a list of TieEndPoint*, not something which actively modifies the current tie and even the score. Here I would try to do a combination of (not necessarily all of them)

Giving this method a more descriptive name

Moving out the code which removes other ties into a separate function

Given that this method performs an undoable action on the score, it looks more like the kind of methods we usually put in edit.cpp, that's also an option

Yes, this code to remove ties is absolutely in the wrong place - it doesn't even cover all the situations I was hoping.
I think we need to remove stale endpoints when the repeat structure is recalculated - either inside or just after RepeatList::unwind.

mike-spa · 2024-12-19T11:09:15Z

src/engraving/dom/tie.h

@@ -25,6 +25,64 @@
 #include "slurtie.h"

 namespace mu::engraving {
+class TieEndPointList;
+class TieEndPoint


Side question: would TieJumpPoint be perhaps a better name for this class? I know what you mean by TieEndPoint, but it feels to me like it doesn't communicate the main purpose. Also because we often call "end point" the graphical end points too.

mike-spa · 2024-12-19T11:18:51Z

src/engraving/dom/note.cpp

+    case ElementType::PARTIAL_TIE: {
+        PartialTie* pt = toPartialTie(e);
+        pt->setTick(tick());
+        if (pt->partialSpannerDirection() == PartialSpannerDirection::OUTGOING) {


pt->itOutGoing() ?

Add separate style option for hanging ties

cbjeukendrup

Here are some first (somewhat superficial) review comments. I haven't been able yet to take a deeper look at some parts of the code, so I might add some more comments later; if you prefer, you can of course wait until then before fixing these first comments.

cbjeukendrup · 2025-01-01T21:15:58Z

src/engraving/dom/barline.cpp

@@ -103,6 +103,8 @@ static void undoChangeBarLineType(BarLine* bl, BarLineType barType, bool allStav
    // createMMRest will then set for the mmrest directly
    Measure* m2 = m->isMMRest() ? m->mmRestLast() : m;

+    BarLineType prevBarType = bl->barLineType();


Is this used?

cbjeukendrup · 2025-01-01T21:34:27Z

src/engraving/dom/measure.h

@@ -207,6 +206,7 @@ class Measure final : public MeasureBase
    Segment* firstActive() const { return m_segments.firstActive(); }

    Segment* last() const { return m_segments.last(); }
+    Segment* last(SegmentType t) const { return m_segments.last(t); }


General remark: I'm sometimes a bit worried about the fact that we keep adding more and more auxiliary methods and overloads to central classes like EngravingItem, Score, Measure, Segment and Chord(Rest). On the one hand, reducing code duplication is good. On the other hand, having huge classes that are used in many places but contain many methods that are used only a few times, is not ideal, for example for compilation time and understandability.

For now I don't suggest to change anything in this regard, but maybe when everyone is back we should have a discussion at some point with the team about what we want to do with this dilemma.

cbjeukendrup · 2025-01-01T22:05:17Z

src/framework/uicomponents/qml/Muse/UiComponents/internal/StyledMenu.qml

@@ -32,6 +32,9 @@ MenuView {

    property alias model: view.model

+    property NavigationSection notationViewNavigationSection: null


The UiComponents module should not contain the word "notation", because it is intended to be as general as possible. Something like navigationSectionOverride would be okay though.

cbjeukendrup · 2025-01-01T22:09:07Z

src/framework/uicomponents/qml/Muse/UiComponents/internal/StyledMenu.qml

+    onNotationViewNavigationSectionChanged: function() {
+        menuNavPanel.section = root.notationViewNavigationSection
+    }
+    onNavigationOrderStartChanged: function() {
+        menuNavPanel.order = root.navigationOrderStart
+    }


This isn't very idiomatic QML: instead of setting the property imperatively to a fixed value, a binding (inside the NavigationPanel would be better:

section: root.navigationSectionOverride ?? content.navigationSection order: root.navigationOrderStart

(which would remove the need for the addition of the id: menuNavPanel line)

cbjeukendrup · 2025-01-01T22:16:43Z

src/notation/qml/MuseScore/NotationScene/internal/PartialTiePopup.qml

+    property int navigationOrderStart: 0
+    property int navigationOrderEnd: partialTieNavPanel.order
+
+    property QtObject model: partialTiePopupModel


It might be good to make this and similar properties in other popups readonly and alias.

notationViewNavigationSection and navigationOrderStart are set in ElementPopupLoader.qml, so can't be readonly.

Sorry, I was unclear; I was referring to the model properties specifically here.

cbjeukendrup · 2025-01-01T22:20:41Z

src/notation/view/widgets/editstyle.ui

+                  </widget>
+                 </item>
+                 <item>
+                  <widget class="QDoubleSpinBox" name="minHangingTieLength">


It would be good to fix the keyboard navigation order too, to prevent a situation like #25530; see that issue for info

cbjeukendrup · 2025-01-01T22:31:49Z

src/notation/qml/MuseScore/NotationScene/internal/PartialTiePopup.qml

+            onItemsChanged: function() {
+                tieMenuLoader.items = partialTiePopupModel.items
+                tieMenuLoader.show(Qt.point(0, 0))
+            }


This could also be done in a more idiomatic way:

the tieMenuLoader.items = partialTiePopupModel.items assignment is not necessary, because inside tieMenuLoader you create a "binding" by writing items: partialTiePopupModel.items, which means that tieMenuLoader.items is automatically updated when partialTiePopupModel.items is updated.

as a consequence, for the .show(…) call, it is better to write
onItemsChanged: function() { tieMenuLoader.show(Qt.point(0, 0)) }
inside tieMenuLoader.

cbjeukendrup · 2025-01-01T22:34:01Z

src/notation/qml/MuseScore/NotationScene/internal/ElementPopupLoader.qml

+        var component = prv.componentByType(elementType)
+
+        var popup = loader.loadPopup(component, elementRect)
+        if (!popup.model.canOpen) {


It would be more ideal to make this decision earlier, in C++, to avoid needlessly creating and destroying popup windows.

cbjeukendrup · 2025-01-01T22:39:21Z

src/notation/qml/MuseScore/NotationScene/internal/PartialTiePopup.qml

+    showArrow: false
+
+    onOpened: {
+        tieMenuLoader.show(Qt.point(0, 0))


It is a little bit of an ugly hack to have a StyledPopupView with nothing in it, and then launch a ContextMenuLoader from it. It would be nicer if we can avoid the StyledPopupView. That would require a bit of not-entirely-trivial refactoring though in, ElementPopupLoader. It might be better to save that for a next PR. Also, we should check if this hack does not cause graphical weirdnesses, like an extra border somewhere, or double drop shadow in a corner; if it does, that might increase the priority of the follow-up refactoring PR a bit.

Yeah, I agree it's messy and not ideal. I'd like to address it outside of this PR though.

cbjeukendrup · 2025-01-01T22:44:31Z

src/notation/qml/MuseScore/NotationScene/internal/PartialTiePopup.qml

+    contentWidth: content.width
+    contentHeight: content.childrenRect.height
+
+    ColumnLayout {


Perhaps this should not be a ColumnLayout but a simple Item, or perhaps even make the ContextMenuLoader itself a direct child of root, if that works (putting the model and NavigationPanel inside the ContextMenuLoader).

miiizen · 2025-01-06T10:11:34Z

@cbjeukendrup I've made the changes requested - I'd like to make any other changes in future PRs so we can get this merged asap!

cbjeukendrup

A few more comments; feel free to merge this PR and address them later.

It would be great if you could provide an explanation of the structure of the PR: which information is stored how and where; how is the 'tree of ownership' (e.g. a Note owns a PartialTie), how are the crosslinks within that tree (e.g. a partial tie might have a reference to its counterpart(s)); etc..
Of course I should be able to extract that information from the code itself, but due to the size of the code, that is quite difficult in this case, both mentally and practically. So a small explanation to help me interpret what I'm seeing would increase efficiency a lot, to avoid constantly having to scroll up and down and use Ctrl+F all the time.

That said, I've not been able to look at every line of this PR; even if I'd do a local-only review, i.e. without trying to get a full understanding of the structure of the whole PR, it would take me some hours. So I've scanned it and commented on a few things that caught my eye. But again, feel free to address them in later PRs indeed.

cbjeukendrup · 2025-01-06T15:09:53Z

src/engraving/dom/tie.cpp

+    TranslatableString undoCmd = addPartialTie ? TranslatableString("engraving", "Replace full tie with partial tie") : TranslatableString(
+        "engraving", "Replace partial tie with full tie");


(This line got split in an unfortunate way)

cbjeukendrup · 2025-01-06T15:15:36Z

src/engraving/dom/tie.h

+class TieJumpPointList;
+class TieJumpPoint


Could you explain how these classes are used? Who "owns" them, the Note or the Tie? And who is responsible for creating and deleteing them?

It might even be good to add some documentation comments to the code itself, because for an uninformed reader it is not immediately clear what's going on.

Also, these classes (and especially their implementation) are big enough to give them their own file.

cbjeukendrup · 2025-01-06T15:38:29Z

src/engraving/dom/tie.cpp

+    const TranslatableString tieTo("engraving", "Tie to ");
+    const String title = tieTo.str + precedingJumpItemName() + u" " + muse::mtrc("engraving", "(m. %1)").arg(measureNo);


There are a few issues here:

the tieTo string won't be detected by lupdate, because you need to write explicitly TranslatableString(…, …)

translatable strings ending with a space are not allowed. Normally the Linux build would complain about them, but now it doesn't, because the string is not detected. The reasons are

translators can easily overlook the trailing whitespace, or fail to understand its importance

concatenating strings becomes problematic for languages with different word order

tieTo.str only means "Tie to ", i.e. it does not return a translated version

without more info, translators can't know what "m. %1" is supposed to mean.

Something like the following should work:

//: An explanation of the meaning of this string can go here, in this special `//:` comment on the line above //: For example, explain what %1 and %2 will be replaced with return muse::mtrc("engraving", "Tie to %1 (m. %2)") .arg(precedingJumpItemName(), measureNo);

And, translators will need to know that the strings from precedingJumpItemName will be inserted into this string: in many languages, after a preposition, a different form of the word is used. To make sure that those strings won't be mixed up with others, you could add a disambiguation, which is the third argument of mtrc.

//: Used at %1 in the string "Tie to %1 (m. %2)" return muse::mtrc("engraving", "start of score", "partial tie target"); //: Used at %1 in the string "Tie to %1 (m. %2)" return muse::mtrc("engraving", "coda", "partial tie target");

cbjeukendrup · 2025-01-06T15:50:58Z

src/engraving/dom/tie.h

+    bool active() const { return m_active; }
+    void setActive(bool v) { m_active = v; }
+    void undoSetActive(bool v);
+    void setJumpPointList(TieJumpPointList* jumpPointList) { m_jumpPointList = jumpPointList; }


I'm not the biggest fan of the pattern where a list item needs to know in which list it is being stored. Perhaps we can find a more elegant solution at some point.

Yes neither am I! As you can see below this is solely down to needing to be able to find the start tie from an end tie. I'd like to find a different way of finding this, as it creates a trail of pointers which need to be kept updated (recipe for disaster)

cbjeukendrup · 2025-01-06T15:57:40Z

src/engraving/dom/tie.cpp

+    MasterScore* master = masterScore();
+    const Note* note = toNote(parentItem());
+    const Chord* chord = note->chord();
+    const Measure* measure = chord->measure();
+    const MeasureBase* masterMeasureBase = master->measure(measure->index());
+    const Measure* masterMeasure = masterMeasureBase && masterMeasureBase->isMeasure() ? toMeasure(masterMeasureBase) : nullptr;


Looks like these could be moved after the two early-returns

miiizen · 2025-01-07T16:23:31Z

@cbjeukendrup hope this clears some things up. This is just information on the relationship between elements - who owns what, and why certain pointers are being stored.

PartialTie

PartialTie and PartialTieSegment are children of Tie and TieSegment. They are added like normal ties to a Note, filling m_tieFor and m_tieBack. They behave in exactly the same way as regular ties, only without an end note.

Tie

Ties can act either as outgoing start ties, or incoming end ties.

tieJumpPoints()

This returns the TieJumpPointList belonging to the Tie's parent note. This is only used when the tie is acting as a start tie

setJumpPoint() & jumpPoint()

This sets m_jumpPoint which is a TieJumpPoint. This is only used when the tie is acting as an end tie. The tie needs to know about its TieJumpPoint to find its start tie

TieJumpPoint

note()

This returns m_note which is a pointer to a Note. This is the note the end tie belongs to.

endTie()

This returns tieBack() of the note.

jumpPointList()

This returns a pointer to the JumpPointList that this object belongs to. This information is needed so a jump point can find its start tie. Storing a pointer to a Tie would be more complex, as this Tie can change (eg. a Tie could be swapped for a PartialTie by unchecking "Tie to next note"). Each end point would then have to be updated.
This is needed for repitching and removing the start tie if all available endpoints have been removed (Tie::updateStartTieOnRemoval)

TieJumpPointList

Each note has a TieJumpPointList. This list needs to be managed by the Note as it lives longer than a Tie (eg. a Tie could be swapped for a PartialTie by unchecking "Tie to next note").
The TieJumpPointList stores a vector of TieJumpPoints and a pointer to the start Note.

m_jumpPoints

TieJumpPoints are created in Tie::updatePossibleJumpPoints and added to the TieJumpPointList where they are managed. They are deleted in ~TieJumpPointLists, or in TieJumpPointList::clear.

startTie()

This returns the start tie. This is the tieFor of the Note that this list belongs to. It is set in Note::setTieFor. However, I will change this in the refinements PR.

undoAddTieToScore(TieJumpPoint) & undoRemoveTieFromScore(TieJumpPoint)

These methods create the correct type of tie and add them to the score.

So in short - ownership & management of Ties and PartialTies is unchanged (through the note).
Ownership & management of TieJumpPointList is done by Note.
Ownership and management of TieJumpPoint is done by TieJumpPointList

miiizen requested review from bkunda and oktophonie December 5, 2024 14:36

miiizen force-pushed the partial-ties branch from 34d3c6c to 05d09b7 Compare December 5, 2024 14:45

oktophonie mentioned this pull request Dec 6, 2024

[TIED NOTES] One note is missing in repeats right after adding/deleting tie (tie between repeat barline) #25748

Open

4 tasks

oktophonie approved these changes Dec 13, 2024

View reviewed changes

miiizen mentioned this pull request Dec 18, 2024

Implement partial slurs #25883

Merged

mike-spa reviewed Dec 19, 2024

View reviewed changes

miiizen added 13 commits December 30, 2024 07:55

Remove unneeded endEdit

e2cad03

Implement partial ties

0f7cd87

Add tests

c792f5d

Review fixes

7b3b55b

Fix inconsistencies with ties before segnos

a2290a7

Enforce min hanging tie distance at end of systems

d161a7d

Add separate style option for hanging ties

Fix horizontal spacing for partial ties

4f46472

Note pad interaction fix

53f4b40

Disallow ties between 2 different voltas

ad0f6c3

Fix partial tie focus issues

87ce59a

Cover more tie clean up cases

672a005

Review fixes

cc454db

Remove partial ties on repeat structure change

ea6001f

miiizen force-pushed the partial-ties branch from 3c25b0a to ea6001f Compare December 30, 2024 07:56

miiizen requested a review from cbjeukendrup December 30, 2024 07:58

cbjeukendrup reviewed Jan 1, 2025

View reviewed changes

miiizen mentioned this pull request Jan 3, 2025

Partial lyric lines #25980

Open

oktophonie added the vtests This PR produces approved changes to vtest results label Jan 6, 2025

Review fixes

8ef59ad

cbjeukendrup reviewed Jan 6, 2025

View reviewed changes

mike-spa merged commit fb40c79 into musescore:master Jan 7, 2025
10 of 11 checks passed

Eism mentioned this pull request Jan 9, 2025

Menus from top menu bar don't close after first click #26026

Closed

4 tasks

		@@ -32,6 +32,9 @@ MenuView {

		property alias model: view.model

		property NavigationSection notationViewNavigationSection: null

		TranslatableString undoCmd = addPartialTie ? TranslatableString("engraving", "Replace full tie with partial tie") : TranslatableString(
		"engraving", "Replace partial tie with full tie");

		const TranslatableString tieTo("engraving", "Tie to ");
		const String title = tieTo.str + precedingJumpItemName() + u" " + muse::mtrc("engraving", "(m. %1)").arg(measureNo);

Implement partial & repeat ties #25745

Implement partial & repeat ties #25745

Conversation

miiizen commented Dec 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbjeukendrup left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miiizen commented Jan 6, 2025

cbjeukendrup left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbjeukendrup Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

miiizen commented Jan 7, 2025 • edited Loading

PartialTie

Tie

tieJumpPoints()

setJumpPoint() & jumpPoint()

TieJumpPoint

note()

endTie()

jumpPointList()

TieJumpPointList

m_jumpPoints

startTie()

undoAddTieToScore(TieJumpPoint*) & undoRemoveTieFromScore(TieJumpPoint*)

cbjeukendrup Jan 6, 2025 •

edited

Loading

miiizen commented Jan 7, 2025 •

edited

Loading

undoAddTieToScore(TieJumpPoint) & undoRemoveTieFromScore(TieJumpPoint)