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

Fix #9514: only apply text to the first element in range selections #9532

Conversation

martin-danhier
Copy link
Contributor

Resolves: #9514

When a text is added to a range selection, the text will be added to the first element instead of each one.
This works for any subtype of TextBase so it includes staff text, system text, expression, dynamics...

Before:

before.mp4

After:

after.mp4
  • I signed CLA
  • I made sure the code in the PR follows the coding rules
  • I made sure the code compiles on my machine
  • I made sure there are no unnecessary changes in the code
  • I made sure the title of the PR reflects the core meaning of the issue you are solving
  • I made sure the commit message(s) contain a description and answer the question "Why do those changes fix that particular issue?" or "Why are those changes really necessary as improvements?"
  • I made sure the commit message title starts with "fix #424242:" if there is a related issue
  • I created the test (mtest, vtest, script test) to verify the changes I made

@MarcSabatella
Copy link
Contributor

Make sure you don't break the case of selections across multiple staves - you need to be sure to add to the first note of each.
It's an incredibly useful way of adding dynamics especially.

@martin-danhier
Copy link
Contributor Author

@MarcSabatella Good point, I added it back. Now, it will add the text to the first element of each selected staff.

Jojo-Schmitz added a commit to Jojo-Schmitz/MuseScore that referenced this pull request Oct 25, 2021
Jojo-Schmitz added a commit to Jojo-Schmitz/MuseScore that referenced this pull request Oct 26, 2021
@DmitryArefiev
Copy link
Contributor

@martin-danhier Great job! It looks much better then it was. Thanks!

But one question.. Is it hard to add dynamic this way (to each first note)?
dynamics

@Jojo-Schmitz
Copy link
Contributor

Jojo-Schmitz commented Oct 26, 2021

That's why the fix is called:
Fix #9514: only apply text to the first element in range selections

First element is not first note.

@martin-danhier
Copy link
Contributor Author

@DmitryArefiev That's right, I considered the first element, which is always at the start of the selection.

Though, I could change that to the first note if this is how we want it to behave. But then, I would need more details, for example what happens if a selected staff doesn't have any note on it? Does this behavior only apply to dynamics, or all types of text ?

If we choose not to do that, I'll change the comment as @Jojo-Schmitz suggested.

By the way, how do you create GIFs like that ? ^^

@MarcSabatella
Copy link
Contributor

Nice as it might be in this particular case to have dynamics be smart enough to apply the first note rather than first element (technically, first chordrest), I think that's going too far. My expectation is that it is my responsibility to make the selection match my needs, and if that means Ctrl+click to add "staggered" dynamics like this, so be it. Because there are any number of other markings I might want to apply to the first note or rest (eg, a custom staff text I've created and added to my palette).

Although I guess it wouldn't hurt to add the additional smarts for dynamics to skip rests. Really, it's a very nice idea, just somehow seems possibly a little too smart if such a thing is possible!

@DmitryArefiev
Copy link
Contributor

DmitryArefiev commented Oct 27, 2021

@martin-danhier Yeah, let's leave it as it is now (add dynamic/text to the first element of range selection).
I will create a separate PR later for a smart dynamic logic when using range selection and multiple staves selection.

By the way, how do you create GIFs like that ? ^^

Oh, it's recordit app (https://recordit.co/)

If we choose not to do that, I'll change the comment as @Jojo-Schmitz suggested.

Good. After that the PR can be merged.

Thanks!

Jojo-Schmitz added a commit to Jojo-Schmitz/MuseScore that referenced this pull request Oct 27, 2021
@RomanPudashkin RomanPudashkin merged commit d10a96a into musescore:master Oct 27, 2021
Jojo-Schmitz added a commit to Jojo-Schmitz/MuseScore that referenced this pull request Oct 27, 2021
Jojo-Schmitz added a commit to Jojo-Schmitz/MuseScore that referenced this pull request Oct 27, 2021
Jojo-Schmitz added a commit to Jojo-Schmitz/MuseScore that referenced this pull request Oct 27, 2021
@martin-danhier martin-danhier deleted the 9514-multi-selection-add-only-one-text branch October 27, 2021 18:52
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Oct 27, 2021
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Oct 27, 2021
@DmitryArefiev DmitryArefiev removed their assignment Oct 30, 2021
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Nov 23, 2021
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Nov 24, 2021
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Nov 28, 2021
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Nov 30, 2021
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Dec 7, 2021
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.

[MU3 Issue] MU adds system/staff Text to each note when using a range selection (whole bar selection)
5 participants