Parse dynamics when finished editing

[CMakeScore]

Split from #25252
With this PR, dynamics you type with Ctrl+Shift+Alphabets will be interpreted and reflected in the playback.

• 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)

[avvvvve]

@CMakeScore You noted that you can use Ctrl + Shift + [character] to enter the proper dynamic character, which works, but it looks like you've also included some handling for automatically changing regular text to dynamic characters after editing (if that's all you've typed). This is an awesome addition, but there are a couple bugs and some room for improvement if you're willing to take it on.

Screen.Recording.2024-10-22.at.4.44.31.PM.2.mov

Bug: Replacing selected text (0:00–0:17 in video)

If I double click into an existing dynamic to select the text, the first character I type does not appear.

1. Ctrl + D to add a dynamic
2. Type f. It appears in the text box as the expression text style (not ideal, but not a dealbreaker)
3. Exit editing. The f automatically changes to the dynamic f character, and it's a functional forte in playback (awesome)
4. Double click to edit the text (selecting it) and type f again. Nothing appears until you type a second character (though if you look in the bottom bar, it shows that the character is there)

This appears to be a preeexisting issue (at least in MSS 4.4), but since we didn't really previously provide much utility to typing in dynamics, I think we should aim to fix it in this PR.

Request: Format known dynamics while typing (0:18–end of video)

Ideally, anytime you begin a "word" in a dynamic text field with a character that typically starts a dynamic (p, m, f, s, or r), it should show up as a dynamic character while you're still editing the field. If you start a word with any other character, or you type a space and add expression text, it should look like expression text. You know you want a dynamic, so why require modifier keys to get the right characters?

We should essentially recognize if the user is typing any of the existing dynamics in palettes until the string stops matching (i.e. another letter or a space is typed). Ideally, we would also retain the ability to prepend expression text by typing before a dynamic (and append after).

Example 1:

1. Type 'f'. The 'f' shows as the dynamic character.
   
2. Type space. The 'f' remains as-is.
   
3. Type 'm'. The 'f' remains as-is, and 'm' renders as expression text because there's already a "word" in the field containing dynamic characters.
   
4. Type 'aestoso' to complete the expression text.
   

Example 2:

1. Type 'm'. The 'm' shows as the dynamic character.
   
2. Type 'a'. The 'm' changes to the expression text style, as does the 'a', because it no longer matches an existing dynamic.
   
3. Type 'estoso' and a space to complete the expression text.
   
4. Type 'f'. The 'f' should render as the dynamic character because it's the start of a new word.
   

Would much appreciate the bug fix to get this merged, and if you want to take on the request too, even better. If not, we can just turn that part into a feature request. Thanks!

[cbjeukendrup]

Before working on that request, it might make sense to do #25229 first. Otherwise, we're creating even more complexity right now, that will all need to be considered during the refactor task.
On the other hand, it is not very clear yet when the refactor will happen. So @CMakeScore if you happen to find out relatively easily how to implement the request, feel free to push it and we'll check it out. But it's not advisable to spend a lot of time on it right now, if it's not easy.

[avvvvve]

Thanks for the context @cbjeukendrup. I'd then recommend fixing that bug (if not too difficult) and merging this, since the Add dynamic shortcut isn't very useful without at least the basic parsing of dynamics that's already been implemented. The request section can be saved for later implementation.

[CMakeScore]

@avvvvve I’ve identified the cause of the bug you mentioned and committed a fix.
Additionally, I updated the code to check each portion of text for matches with dynamic symbols, bringing it closer to the requested feature. Now, for example, if you type "maestoso f" or "f maestoso," the "f" will automatically be replaced with the forte symbol once editing is complete.
Please take a look!

[avvvvve]

@CMakeScore Thanks for making quick work of this! Really nice solution you've come up with and I think people will be really excited to use this.

I'll create another feature request to render dynamic characters as they're typed so we at least have it recorded.

@cbjeukendrup Ready for code review
@zacjansheski Would be great to get your eyes on this too; I haven't tested on Windows yet (essentially, type dynamics directly in the text box and make sure they are registered as the correct dynamic element, optionally adding expression text before or after)

[zacjansheski]

Dynamic characters are not synced when parts exist

Untitled.mp4

[CMakeScore]

Fixed a problem with synchronization with the part.
@zacjansheski Could you please test it again?

Edit:
I also noticed that I had made a mistake with the order of the menu in the note input bar added in a previous PR (#25252), so I added a commit to correct it.
As mentioned in #25252 (comment), "Dynamic" should come before "Expression text."

[mathesoncalum]

Thanks for doing those tweaks - almost there!

This isn't quite what I had in mind regarding these changes. I think what you had before was mostly fine but instead we should do something like this:

1. Change parseDynamicText so that it returns a std::pair<DynamicType, String>

2. Instead of directly calling setXmlText inside parseDynamicText, do this (where dynamicInfo is a pair):

dynamicInfo.first = DynamicType(i);
dynamicInfo.second = String::fromStdString(utf8Tag);
return dynamicInfo;

3. Finally, inside setDynamicType:

setXmlText(dynamicInfo.second);
setDynamicType(dynamicInfo.first);

Let me know how that sounds - in the meantime I'll investigate the crash you mentioned here.

[CMakeScore]

@mathesoncalum Thank you for your feedback!
It seems I have misunderstood your earlier review. I’ve now updated the code based on your suggestions.
Please let me know if there’s anything else that needs adjustment!

[mathesoncalum]

Sorry for the delay @CMakeScore - thanks very much for doing those tweaks! In the process of looking into this I discovered a crash in master - I think it would be wise to resolve this first before finishing up this PR.

I'll drop another message here once that's done - thanks again!

[mathesoncalum]

Thanks so much for your patience @CMakeScore - been a busy few weeks but I created a fix for the crash the other day, so we should be safe to continue.

Here's what I'm thinking, hopefully it's viable:

1. In TextBase::endEdit, simply:

Dynamic* d = toDynamic(this);
undoChangeProperty(Pid::DYNAMIC_TYPE, xmlText());

2. For this to work we'll need to tweak Dynamic::setProperty so that it dispatches to the correct "set" method (instead of just modifying m_dynamicType):

case Pid::DYNAMIC_TYPE:
if (v.type() == P_TYPE::DYNAMIC_TYPE) {
    setDynamicType(v.value<DynamicType>());
    break;
}
setDynamicType(v.value<String>());
break;
...

3. Make parseDynamicText private.

This seemed reliable in the (brief) testing I did - but do let me know if you notice any problems with this approach.

[mathesoncalum]

Looking good now @CMakeScore - now if you could just squash this into 1 or 2 commits that would be perfect.

Cheers!

[CMakeScore]

OK, I've squashed the commits now. Thank you for all your support so far!

[zacjansheski]

Undo after changing dynamic does not update playback

video1410913511.mp4

[CMakeScore]

Thank you for testing! I’ve pushed a new commit to address the issue you mentioned. Could you please test it again?
Let me know if there's anything else that needs to be fixed.

Oops, I'll look into the failed utests later.

[CMakeScore]

Hmm, it seems that for some reason textWasEdited is false during utest.
For now, I have reverted the reference file to its state before my changes to avoid any errors. However, it might be necessary to adjust the test script to ensure this is tested correctly. (I'm not sure what needs to be modified or how to proceed, though...)

[zacjansheski]

Tested on MacOS 14, Windows 11, Ubuntu 22.04.3. Approved

[mathesoncalum]

Thanks for looking into that @CMakeScore! The change itself seems fine to me, but indeed it looks like we'll need to tweak the unit tests (what you had before in the reference file looked correct to me).

I'm pretty sure all we need to do is call Score::startCmd immediately after Dynamic::startEdit, and Score::endCmd immediately before Dynamic::endEdit for those three tests. This will ensure the undo stack updates its current index as it would if the text was edited by hand (and we get the correct value for textWasEdited).

In dynamicAddTextBefore it looks like this:

...
dynamic->startEdit(ed);
score->startCmd(TranslatableString::untranslatable("Edit dynamic text (test)"));
score->undo(new InsertText(dynamic->cursor(), String(u"poco ")), &ed);
score->endCmd();
dynamic->endEdit(ed);
...

Once that's done just squash everything again and we should be golden (you can squash my commit into yours, I don't mind).

Additional tweaks required

[CMakeScore]

I've been busy this past week so I apologize for the delay in addressing this.
@mathesoncalum Thanks for the detailed feedback and guidance—it was really helpful!