Create defaults for pedal begin, continue and end text #19666

miiizen · 2023-10-10T10:27:54Z

Resolves: #18603

Create defaults for pedal begin, continue and end text
The end text default varies based on the visibility of the line. If the line is visible the default is no text. If the line is invisible the default is the rosette symbol
The begin text varies based on the start hook. If there is a start hook the default is no text. If there is no start hook the default is the 'Ped.' symbol
Import MS1, 2, 3, 4, & 4.1 pedals properly
Import MEI and musicXML scores properly
Prevent "Reset shapes & positions" from resetting text line's text.

The end text default varies based on the end hook. If there is no end hook, the default is the rosette symbol. If there is an end hook, the default is no text at all.

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)

cbjeukendrup · 2023-10-11T13:35:14Z

src/engraving/rw/read206/read206.cpp

@@ -2045,6 +2045,12 @@ static void readVolta206(XmlReader& e, ReadContext& ctx, Volta* volta)

 static void readPedal(XmlReader& e, ReadContext& ctx, Pedal* pedal)
 {
+    if (pedal->score()->mscVersion() < 420) {


This if seems a bit tautological given that it's in read206

cbjeukendrup · 2023-10-11T14:08:45Z

src/engraving/rw/read206/read206.cpp

+        pedal->setBeginText(String());
+        pedal->setContinueText(String());
+        pedal->setEndText(String());


Shouldn't we set the propertyFlags to UNSTYLED if we reach the bottom of this method without having encountered a tag for the these properties?

No, I don't think so? if there's no tag present, doesn't that mean the property should follow the default? If I do set them to unstyled, then the start and end text is set based on hook/line visibility but the continue text is always read as a blank string which is incorrect.

If no tag is present, the property should indeed follow the default; so in principle, it should be STYLED indeed. But in this case, the default has changed; so if no tag is present in an old score, that means that it should follow the old default, namely an empty string. It looks indeed like your code is properly doing that.

But if you set them to an empty string at the start of the method, and don't set them to UNSTYLED, then you basically get in an inconsistent state, because the property value (which is an empty string) does not equal the style value (which is now some text), while having its flags set to STYLED implies that it should be equal. That's what I'm wondering about.

Ah sorry, I understand now thanks! Yes that makes sense, they should be set to UNSTYLED

cbjeukendrup · 2023-10-11T14:13:20Z

src/engraving/rw/read400/tread.cpp

+    if (p->score()->mscVersion() < 420) {
+        p->setBeginText(String());
+        p->setContinueText(String());
+        p->setEndText(String());
+    }


Same two questions here

src/engraving/rw/read114/read114.cpp

src/inspector/models/notation/lines/pedalsettingsmodel.h

cbjeukendrup · 2023-10-11T14:29:56Z

src/engraving/rw/read410/tread.cpp

+        } else if (tag == "lineVisible") {
+            if (TRead::readProperty(p, tag, e, ctx, Pid::LINE_VISIBLE)) {


It looks like this should either be

Suggested change

} else if (tag == "lineVisible") {

if (TRead::readProperty(p, tag, e, ctx, Pid::LINE_VISIBLE)) {

} else if (TRead::readProperty(p, tag, e, ctx, Pid::LINE_VISIBLE)) {

or

Suggested change

} else if (tag == "lineVisible") {

if (TRead::readProperty(p, tag, e, ctx, Pid::LINE_VISIBLE)) {

} else if (tag == "lineVisible") {

TRead::readProperty(p, e, ctx, Pid::LINE_VISIBLE));

cbjeukendrup · 2023-10-11T14:30:41Z

src/engraving/rw/read410/tread.cpp

    while (e.readNextStartElement()) {
        const AsciiStringView tag(e.name());
        if (readStyledProperty(p, tag, e, ctx)) {
+        } else if (tag == "lineVisible") {
+            if (TRead::readProperty(p, tag, e, ctx, Pid::LINE_VISIBLE)) {
+                p->setPropertyFlags(Pid::LINE_VISIBLE, PropertyFlags::UNSTYLED);


This is already done inside TRead::readProperty afaics

RomanPudashkin · 2023-10-20T16:16:53Z

src/inspector/models/notation/lines/pedalsettingsmodel.h

@@ -50,6 +50,7 @@ class PedalSettingsModel : public TextLineSettingsModel
    void setLineType(int newType);

    PropertyItem* m_lineType = nullptr;
+    bool rosetteHookSelected = false;


m_rosetteHookSelected

RomanPudashkin · 2023-10-20T16:17:11Z

src/inspector/models/notation/lines/pedalsettingsmodel.cpp

@@ -59,7 +59,8 @@ PropertyItem* PedalSettingsModel::lineType() const

 bool PedalSettingsModel::isChangingLineVisibilityAllowed() const
 {
-    return isStarSymbolVisible();
+//    return isStarSymbolVisible();


Please remove this comment

cbjeukendrup · 2023-10-20T16:40:07Z

src/inspector/models/notation/lines/pedalsettingsmodel.cpp

@@ -91,15 +92,18 @@ void PedalSettingsModel::loadProperties()
    m_lineType->setIsEnabled(true);

    if (isStarSymbolVisible()) {
+        rosetteHookSelected = true;
+        emit isChangingLineVisibilityAllowedChanged();


Shouldn't this be emitted in all cases (not only when isStarSymbolVisible())?

cbjeukendrup

I realised there was one more potential issue so I pushed a fix for that. Now it looks good to me!

cbjeukendrup · 2023-10-29T14:08:36Z

src/engraving/rw/read114/read114.cpp

+    pedal->setBeginText(String());
+    pedal->setContinueText(String());
+    pedal->setEndText(String());


I realised there's still one issue: if symbol != pedal->propertyDefault(Pid::BEGIN_TEXT).value<String>() is false a few lines below, then the begin text will not be set there, so it will have been set to String() here at the top of the function.
So, instead we should only set it to String() if the tag was absent.

cbjeukendrup · 2023-10-29T16:43:29Z

It turns out that 114 also has beginText etc beside beginSymbol (I didn't know that yet). So here is yet another version: miiizen/MuseScore@18603-pedal-text...cbjeukendrup:MuseScore:pedal_text_reading
@miiizen What do you think about that one?

miiizen · 2023-11-06T09:42:55Z

@cbjeukendrup looks good to me!

cbjeukendrup · 2023-11-09T00:46:17Z

@miiizen In that case I'll push that commit to this PR and merge it!

cbjeukendrup · 2023-11-09T10:26:44Z

VTests are still not happy apparently... I can hopefully take a look tonight, unless you beat me to it :)

miiizen · 2023-11-09T10:38:43Z

@cbjeukendrup these are desired changes showing the defaults working. All looks good to me!

cbjeukendrup · 2023-11-09T10:51:05Z

Ah, of course, since those are new VTests and thus 420 scores. I see, thanks!

miiizen requested a review from oktophonie October 10, 2023 10:28

cbjeukendrup reviewed Oct 11, 2023

View reviewed changes

miiizen force-pushed the 18603-pedal-text branch 2 times, most recently from 1c6a0b3 to 995eb3d Compare October 19, 2023 09:49

oktophonie approved these changes Oct 20, 2023

View reviewed changes

oktophonie added the vtests This PR produces approved changes to vtest results label Oct 20, 2023

oktophonie requested a review from cbjeukendrup October 20, 2023 13:35

RomanPudashkin reviewed Oct 20, 2023

View reviewed changes

cbjeukendrup reviewed Oct 20, 2023

View reviewed changes

miiizen force-pushed the 18603-pedal-text branch from 059d237 to e085f29 Compare October 23, 2023 14:14

cbjeukendrup force-pushed the 18603-pedal-text branch from e085f29 to 07ef1b0 Compare October 29, 2023 14:43

cbjeukendrup approved these changes Oct 29, 2023

View reviewed changes

miiizen and others added 6 commits November 9, 2023 01:56

Add default strings for pedal text

deae275

Exclude textline text from reset

ef475b4

Fix musicxml import

087737d

Set correct defaults for continue text

7b36c8f

Code review fixes

6c4d902

Another attempt to perfect the compat reading code for pedal text

f351a10

cbjeukendrup force-pushed the 18603-pedal-text branch from e29a279 to f351a10 Compare November 9, 2023 00:56

cbjeukendrup merged commit c2b234c into musescore:master Nov 9, 2023
10 of 11 checks passed

MarcSabatella mentioned this pull request Dec 22, 2023

Pedal text not rendering at all when I insert a marking with said text #20646

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create defaults for pedal begin, continue and end text #19666

Create defaults for pedal begin, continue and end text #19666

miiizen commented Oct 10, 2023 •

edited

Loading

cbjeukendrup Oct 11, 2023

cbjeukendrup Oct 11, 2023

miiizen Oct 17, 2023

cbjeukendrup Oct 17, 2023

miiizen Oct 18, 2023

cbjeukendrup Oct 11, 2023

cbjeukendrup Oct 11, 2023

cbjeukendrup Oct 11, 2023

RomanPudashkin Oct 20, 2023

RomanPudashkin Oct 20, 2023

cbjeukendrup Oct 20, 2023

cbjeukendrup left a comment

cbjeukendrup Oct 29, 2023 •

edited

Loading

cbjeukendrup commented Oct 29, 2023

miiizen commented Nov 6, 2023

cbjeukendrup commented Nov 9, 2023

cbjeukendrup commented Nov 9, 2023

miiizen commented Nov 9, 2023

cbjeukendrup commented Nov 9, 2023

		} else if (tag == "lineVisible") {
		if (TRead::readProperty(p, tag, e, ctx, Pid::LINE_VISIBLE)) {

	} else if (tag == "lineVisible") {
	if (TRead::readProperty(p, tag, e, ctx, Pid::LINE_VISIBLE)) {
	} else if (TRead::readProperty(p, tag, e, ctx, Pid::LINE_VISIBLE)) {

Create defaults for pedal begin, continue and end text #19666

Create defaults for pedal begin, continue and end text #19666

Conversation

miiizen commented Oct 10, 2023 • edited Loading

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

cbjeukendrup Oct 29, 2023 • edited Loading

Choose a reason for hiding this comment

cbjeukendrup commented Oct 29, 2023

miiizen commented Nov 6, 2023

cbjeukendrup commented Nov 9, 2023

cbjeukendrup commented Nov 9, 2023

miiizen commented Nov 9, 2023

cbjeukendrup commented Nov 9, 2023

miiizen commented Oct 10, 2023 •

edited

Loading

cbjeukendrup Oct 29, 2023 •

edited

Loading