Implement repeat courtesies #26264

miiizen · 2025-01-28T22:21:34Z

This PR implements a selection of style options to create courtesy time signatures, key signatures and clefs before and after repeats and jumps.

"Place all changes before the barline" - If a clef, key or time signature applies both to a repeat/jump's destination and continuation, it is moved to the end of the previous measure
Clefs can be placed on either side of a repeat barline

Changes can be placed between repeat barlines, or after them

If a clef, key or time signature is different at a repeat or jump desitnation, courtesies can be placed before the repeat or jump. Cancellation courtesies can optionally be placed at the continuation from the repeat or jump. Two styles of parentheses are provided

Screen.Recording.2025-01-29.at.15.50.30.mov

mike-spa

Absolutely heroic work

mike-spa · 2025-02-07T09:15:04Z

src/engraving/dom/clef.cpp

@@ -287,6 +287,7 @@ PropertyValue Clef::getProperty(Pid propertyId) const
    case Pid::SMALL:         return isSmall();
    case Pid::CLEF_TO_BARLINE_POS: return m_clefToBarlinePosition;
    case Pid::IS_HEADER: return m_isHeader;
+    case Pid::IS_COURTESY: return _isCourtesy;


m_isCourtesy, for consistency

mike-spa · 2025-02-07T09:17:43Z

src/engraving/dom/keysig.cpp

@@ -212,6 +212,8 @@ PropertyValue KeySig::getProperty(Pid propertyId) const
        return int(showCourtesy());
    case Pid::KEYSIG_MODE:
        return int(mode());
+    case Pid::IS_COURTESY:
+        return _isCourtesy;


m_isCourtesy

mike-spa · 2025-02-07T09:31:44Z

src/engraving/dom/clef.h

@@ -89,6 +89,8 @@ class Clef final : public EngravingItem
    OBJECT_ALLOCATOR(engraving, Clef)
    DECLARE_CLASSOF(ElementType::CLEF)

+    M_PROPERTY2(bool, isCourtesy, setIsCourtesy, false)


Yeah I've used this too in the past, but lately I've always avoided this macro, and taken the boredome to write member/getter/setter. It's boilerplate, but you'll thank yourself for the boilerplate the first time you'll need to place a breakpoint to see who's getting/setting this variable.

mike-spa · 2025-02-07T09:46:09Z

src/engraving/dom/measurebase.h

+    void setHasCourtesyTimeSig(bool v) const { setFlag(ElementFlag::TIMESIG, v); }
+
+    bool hasCourtesyClef() const { return flag(ElementFlag::CLEF); }
+    void setHasCourtesyClef(bool v) const { setFlag(ElementFlag::CLEF, v); }


Perhaps the flags could be better named as COURTESY_TIMESIG, COURTESY_CLEF?

mike-spa · 2025-02-07T09:53:24Z

src/engraving/dom/measure.cpp

+                if (seg->segmentType()
+                    & (SegmentType::EndBarLine | SegmentType::TimeSigAnnounce | SegmentType::KeySigAnnounce
+                       | SegmentType::TimeSigRepeatAnnounce | SegmentType::KeySigRepeatAnnounce | SegmentType::TimeSigStartRepeatAnnounce
+                       | SegmentType::KeySigStartRepeatAnnounce)) {


Pretty sure you can use some of the aliases you've created among SegmentType to make this statement a bit shorter/more readable?

mike-spa · 2025-02-07T09:55:09Z

src/engraving/dom/segment.cpp

+        if (m_segmentType == SegmentType::KeySigAnnounce
+            || m_segmentType == SegmentType::KeySigRepeatAnnounce || m_segmentType == SegmentType::KeySigStartRepeatAnnounce) {
+            toKeySig(el)->setIsCourtesy(true);
+        }


Here and all of the ones above I think you can use the aliases (and also take advantage of the bitwise | operations which are faster)

mike-spa · 2025-02-07T10:00:00Z

src/engraving/dom/segment.cpp

+        || segmentType() == SegmentType::KeySigRepeatAnnounce
+        || segmentType() == SegmentType::ClefStartRepeatAnnounce
+        || segmentType() == SegmentType::TimeSigStartRepeatAnnounce
+        || segmentType() == SegmentType::KeySigStartRepeatAnnounce) {


Wait... why doesn't this function simply return segmentType() != SegmentType::ChordRest? 😅

mike-spa · 2025-02-07T10:30:24Z

src/engraving/dom/segment.cpp

@@ -2532,11 +2564,15 @@ void Segment::createShape(staff_idx_t staffIdx)
        }
    }

+    rendering::score::SegmentLayout::placeParentheses(this, staffIdx);


This is a tempting thing to do, but it's a mistake that I've made myself before and rolling it back later was ugly. We should never call layout functions from here. This function should have the one and only task of adding the (already laid out) elements to the segment shape, because that's what we expect it to do when we call it. If we let this function actively modify things by doing layout calls we open ouselves to side effects and properly shoot ourselves in the foot (ask me how I know...).

You're calling it here because obvs you need to know the shape of things in order to place the parenthesis correctly. But you can know that shape right after you've laid out the courtesy elements. So perhaps a good place to call this may be instead MeasureLayout::addRepeatContinuationCourtesies?

mike-spa · 2025-02-07T10:40:57Z

src/engraving/rw/read410/measureread.cpp

-            // if time sig not at beginning of measure => courtesy time sig
+
+            // PRE 4.5: if time sig not at beginning of measure => courtesy time sig
+            // 4.5+ just tag it


YES. This insane thing of inferring that something is a courtesy because it's not at the start of the measure gave me nightmares. Let's purge it wherever we can

mike-spa · 2025-02-07T10:53:40Z

src/engraving/rendering/score/segmentlayout.cpp

@@ -231,3 +233,84 @@ void SegmentLayout::layoutChordsStem(const Segment& segment, track_idx_t startTr
        }
    }
 }
+
+void SegmentLayout::placeParentheses(const Segment* segment, staff_idx_t staffIdx)


Also, just for organization, it's a bit weird to have this in SegmentLayout, given that this is stricly related to all the rest of the courtesy layout code. So I'd rather put this in MeasureLayout together with all the rest. Or, if you want or think it could make sense, even create a new layout class like CourtesyLayout and put everything in there that relates to courtesies

Small refactor of MeasureLayout::addSystemTrailer

Fix creating parens on single segment

Fix QML warnings Remove show at final bar setting Fix show courtesies before barline Fix tests

mike-spa

Couple of cleanup comments, but you can do that in the next PR :)

mike-spa · 2025-02-07T15:53:34Z

src/engraving/dom/timesig.h

@@ -52,6 +52,8 @@ class TimeSig final : public EngravingItem
    OBJECT_ALLOCATOR(engraving, TimeSig)
    DECLARE_CLASSOF(ElementType::TIMESIG)

+    M_PROPERTY2(bool, isCourtesy, setIsCourtesy, false)


Same as before about avoiding this macro. And m_isCourtesy

mike-spa · 2025-02-07T15:54:59Z

src/engraving/rendering/score/segmentlayout.cpp


 #include "tlayout.h"
 #include "chordlayout.h"
+#include "horizontalspacing.h"


These can be now removed

miiizen force-pushed the repeat-courtesies branch 2 times, most recently from 2bacb0c to e656d9e Compare February 6, 2025 10:32

oktophonie added the engraving label Feb 6, 2025

miiizen force-pushed the repeat-courtesies branch 2 times, most recently from 4fd33e2 to beaed54 Compare February 6, 2025 17:40

mike-spa reviewed Feb 7, 2025

View reviewed changes

miiizen and others added 21 commits February 7, 2025 14:26

Add styles

eee4523

Add style dialogue page

a7e618a

Implement allow changes between end-start repeats

8fc7a75

Implement clef position style setting

e59a98a

Small refactor of MeasureLayout::createEndBarLines

385efe4

Small refactor of MeasureLayout::addSystemTrailer

Implement courtesies before repeat

9132954

Implement all courtesy repeat options

fe70156

Add parentheses for courtesy items

699981c

Fix creating parens on single segment

Fix layout

9acb085

Implement changes before barline

b31d4e5

Bugfixes

d7829ff

Unify clef and key/time signature positioning code

d0c8988

Bugfixes

ca723dc

Create small parenthesis option

10606fe

Fix build

9228d1c

Fix QML warnings Remove show at final bar setting Fix show courtesies before barline Fix tests

Add tests

e4d6ef4

Fix clef parens offset issue

19237e4

Disable individual courtesy option after section break if applicable

75bbbc8

Bufixes

31653c5

Fix placement of parens on single segment

ec3f4fc

Review changes

cbd93b6

miiizen force-pushed the repeat-courtesies branch from beaed54 to cbd93b6 Compare February 7, 2025 14:29

oktophonie approved these changes Feb 7, 2025

View reviewed changes

oktophonie added the vtests This PR produces approved changes to vtest results label Feb 7, 2025

mike-spa approved these changes Feb 7, 2025

View reviewed changes

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement repeat courtesies #26264

Implement repeat courtesies #26264

miiizen commented Jan 28, 2025 •

edited

Loading

mike-spa left a comment

mike-spa Feb 7, 2025

mike-spa Feb 7, 2025

mike-spa Feb 7, 2025

mike-spa Feb 7, 2025

mike-spa Feb 7, 2025

mike-spa Feb 7, 2025

mike-spa Feb 7, 2025

mike-spa Feb 7, 2025

mike-spa Feb 7, 2025

mike-spa Feb 7, 2025

mike-spa left a comment

mike-spa Feb 7, 2025

mike-spa Feb 7, 2025

Implement repeat courtesies #26264

Implement repeat courtesies #26264

Conversation

miiizen commented Jan 28, 2025 • edited Loading

mike-spa 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

mike-spa left a 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 28, 2025 •

edited

Loading