Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Parse dynamics when finished editing #25396

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

CMakeScore
Copy link
Contributor

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
Copy link

avvvvve commented Nov 4, 2024

@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.
    image
  2. Type space. The 'f' remains as-is.
    image
  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.
    image
  4. Type 'aestoso' to complete the expression text.
    image

Example 2:

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

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
Copy link
Contributor

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
Copy link

avvvvve commented Nov 5, 2024

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 CMakeScore force-pushed the parse-dynamics branch 2 times, most recently from 21e8a53 to f9c396e Compare November 7, 2024 14:52
@CMakeScore
Copy link
Contributor Author

@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!

@CMakeScore CMakeScore force-pushed the parse-dynamics branch 3 times, most recently from 710f891 to d2043bf Compare November 7, 2024 15:32
@avvvvve
Copy link

avvvvve commented Nov 7, 2024

@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
Copy link
Contributor

zacjansheski commented Nov 8, 2024

Dynamic characters are not synced when parts exist

Untitled.mp4

@CMakeScore CMakeScore force-pushed the parse-dynamics branch 2 times, most recently from a5b3ef8 to c70a897 Compare November 11, 2024 03:57
deleteSelectedText() updates the font family and must be executed before setting the new font family.
@CMakeScore
Copy link
Contributor Author

CMakeScore commented Nov 11, 2024

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."

src/engraving/dom/dynamic.cpp Outdated Show resolved Hide resolved
src/engraving/dom/dynamic.cpp Outdated Show resolved Hide resolved
for (size_t i = 0; i < n; ++i) {
if (TConv::toXml(DynamicType(i)).ascii() == matchStr || DYN_LIST[i].text == matchStr) {
utf8Tag.replace(match.position(0), match.length(0), DYN_LIST[i].text);
setXmlText(String::fromStdString(utf8Tag));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
setXmlText(String::fromStdString(utf8Tag));

In principle this function should have a single responsibility - parsing and returning dynamics - so I don't think we should also be mutating the object with setXmlText here.

It might be easier said than done, but moving this call up to setDynamicText would be more cohesive. Once that's done we can make parseDynamicText a const function, and we can perhaps even make it private after the suggested changes to textedit.cpp.

Comment on lines +148 to +149
const DynamicType dt = d->parseDynamicText(xmlText());
undoChangeProperty(Pid::DYNAMIC_TYPE, dt);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason we can't do something like this?

Suggested change
const DynamicType dt = d->parseDynamicText(xmlText());
undoChangeProperty(Pid::DYNAMIC_TYPE, dt);
d->setDynamicType(xmlText());

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding is that I need to call undoChangeProperty to set the dynamic type consistently across all parts. Otherwise, for example, if you add or edit the dynamics in a part score, the playback will not reflect those changes.

However, while calling undoChangeProperty from within Dynamic::setDynamicType seems fine at first glance, the program crashes when I copy and paste the dynamic. This led me to believe that something is wrong, so I refrained from doing so.

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.

5 participants