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

Change "Respell pitches" to "Recalculate pitches" and add the "Change enharmonic spelling" commands to the Tools menu #26130

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

Conversation

Jojo-Schmitz
Copy link
Contributor

@Jojo-Schmitz Jojo-Schmitz commented Jan 17, 2025

See discussion in https://discord.com/channels/818804595450445834/818852942882275379/1328981777443721368 ff.

"Respell" is a) confusing (see above, also gets confused for "Change enharmonic spelling", something quite different) and b) doesn't describe what really happens, "Recalculate" does a much better job here, IMHO.
Or, as done now, "Optimize enharmonic spellings".

Brings up another question: why does that particular string and quite a few others exist several times (well, twice), but in different contexts, ("undoableAction" vs. "action")? Seems new in master/4.5, as far as I can tell.

@MarcSabatella
Copy link
Contributor

I support this request.

The command isn’t actually all that useful in practice (as I recall, it supposedly applies a heuristic that had been published in some paper, to determine the optimum spelling for all accidentals within the selection, but few people find the results convincing).

But people definitely seem to confuse it with the “change enharmonic spelling” command and wonder why it doesn’t simply change the spelling of a single selected note. To make matters worse, the “change enharmonic spelling” command is one of the very few commands not exposed in the menus anywhere - you have to know the shortcut “J”. So I am concerned that regardless if/how “respell notes” gets renamed, it won’t solve the real problem, which is that people can’t figure out how to actually change enharmonic spelling.

So I suggest also adding the change enharmonic spelling to the Tools menu. Possibly both variants (there is a version that changes the spelling in the current transposition mode only), but the main version for sure.

@Jojo-Schmitz Jojo-Schmitz changed the title Change "Respell" to "Recalculate Change "Respell pitches" to "Recalculate pitches" and add the "Change enharmonic spelling" commands to the Tools menu Jan 17, 2025
Jojo-Schmitz added a commit to Jojo-Schmitz/MuseScore that referenced this pull request Jan 19, 2025
… enharmonic spelling" commands to the Tools menu

Backport of musescore#26130
Jojo-Schmitz added a commit to Jojo-Schmitz/MuseScore that referenced this pull request Jan 19, 2025
… enharmonic spelling" commands to the Tools menu

Backport of musescore#26130
@bkunda
Copy link

bkunda commented Jan 20, 2025

I agree with you @MarcSabatella that this PR doesn't solve the real problems at hand, which are:

  • The functionality behind the current "respell pitches" command is extremely opaque
  • "Change enharmonic spelling" is what people actually need to use most of the time, and yet it is completely undiscoverable

I do not think that changing "respell pitches" to "recalculate pitches" solves anything. IMO, it actually makes an already bewildering function even harder to understand.

Moreover, even if this new label is technically correct, it still won't help users understand the functionality. For this reason, I am not in favour of making this change ("correctness" is in and of itself not a reason to change something; above all else, UI copy has to be easy to understand).

It would be better to tackle the real problems here. "Change enharmonic spelling" needs to be added to the Tools menu, and it would make a lot of sense if it was actually called "respell pitches", because it does exactly what you'd expect "respell pitches" it to do. We'd then resolve some of the current confusion behind the current "respell pitches" appearing to not do anything.

As for the current "respell pitches" functionality, I'd like to see some of the documentation about how it's supposed to work – only I haven't yet succeeded in finding any. @MarcSabatella, are you able to point me in the right direction?

From my own testing, I failed to get it to do anything on a range selection of 4 bars containing accidentals, but I did get it to make a few minor tweaks to a 2-page score that contained lots of accidentals. The results didn't make terribly much sense to me though.

Unless there's an extremely compelling reason to continue supporting this functionality – and unless satisfactory documentation for it exists to explain it – I'd sooner see it removed.

@MarcSabatella
Copy link
Contributor

MarcSabatella commented Jan 20, 2025

I did a search, and I believe this is the paper that provides the algorithm (the title and the author are given in comments within the MuseScore source code):

https://ofai.at/papers/oefai-tr-2001-12.pdf

There was also once a PR that purported to fix a flaw in our implementation algorithm: #5257

Also see this discussion: https://musescore.org/en/node/293080

@MarcSabatella
Copy link
Contributor

I’m not crazy about taking the name of an existing poorly understood command and suddenly changing it to do something completely different.

I have no attachment to keeping the existing respell pitches command as I’ve never heard of anyone saying they have been using it successfully, but if we’re really looking for a better name that, I’d suggest maybe “calculate optimum pitch spelling”.

@Jojo-Schmitz
Copy link
Contributor Author

Jojo-Schmitz commented Jan 20, 2025

There was also once a PR that purported to fix a flaw in our implementation algorithm: #5257

I've added that too, as the 2nd commit (would love to credit the author for it, but can't)

It would be better to tackle the real problems here. "Change enharmonic spelling" needs to be added to the Tools menu,

That's what this PR does, it the 3rd commit

Jojo-Schmitz added a commit to Jojo-Schmitz/MuseScore that referenced this pull request Jan 20, 2025
… enharmonic spelling" commands to the Tools menu

Backport of musescore#26130
@shoogle
Copy link
Contributor

shoogle commented Jan 20, 2025

How about this? (Just a suggestion; not a decision.)

Option 1:

  • Rename "Respell pitches" to "Optimize enharmonic spelling".
  • Make "Change enharmonic spelling" available in the Tools menu.

Option 2:

  • Rename "Respell pitches" to "Optimize pitch spelling".
  • Rename "Change enharmonic spelling" to "Change pitch spelling" and make it available in the Tools menu.

I agree with @MarcSabatella that it's probably not a good idea to rename "Change enharmonic spelling" to "Respell pitches" because that will cause confusion with the existing command. Also, I think "Change" is a better description of what it does than "Respell". To me "Respell" sounds like it will reset (i.e. optimize) the spellings rather than simply change them to something else.

@Jojo-Schmitz
Copy link
Contributor Author

Jojo-Schmitz commented Jan 20, 2025

Option 1 is less work, so I'll go for that ;-)

Brings up another question: why does that particular string and quite a few others exist several times (well, twice), but in different contexts, ("undoableAction" vs. "action")? Seems new in master/4.5, as far as I can tell.

What's the deal here?

@Jojo-Schmitz Jojo-Schmitz force-pushed the translations branch 4 times, most recently from d6dba93 to 693affd Compare January 20, 2025 15:05
@Jojo-Schmitz
Copy link
Contributor Author

Jojo-Schmitz commented Jan 20, 2025

What's up with uncrustify?

terminate called after throwing an instance of 'std::out_of_range'
  what():  vector::_M_range_check: __n (which is 18446744073709551615) >= this->size() (which is 1)
...
xargs: /home/runner/work/MuseScore/MuseScore/buildscripts/ci/checkcodestyle/_deps/tools/linux/uncrustify_073: terminated by signal 6
...
CMake Error at buildscripts/ci/checkcodestyle/_deps/checkcodestyle.cmake:19 (message):
  Scan failed, please check log for details

@cbjeukendrup
Copy link
Contributor

So uncrustify is crashing... @igorkorsukov do you have any idea what's going on there?


Brings up another question: why does that particular string and quite a few others exist several times (well, twice), but in different contexts, ("undoableAction" vs. "action")? Seems new in master/4.5, as far as I can tell.

It was intentional to keep them separated; action is for actions in the menu, undoableAction is what gets recorded in the UndoStack and is displayed in the Edit menu and History panel.

@Jojo-Schmitz
Copy link
Contributor Author

Brings up another question: why does that particular string and quite a few others exist several times (well, twice), but in different contexts, ("undoableAction" vs. "action")? Seems new in master/4.5, as far as I can tell.

It was intentional to keep them separated; action is for actions in the menu, undoableAction is what gets recorded in the UndoStack and is displayed in the Edit menu and History panel.

But can't those reuse the strings?

@cbjeukendrup
Copy link
Contributor

Technically yes, but I'm afraid that will become messy: some action strings are used only for the menus, others also for the history panel, some history panels strings come from yet another place, others just from undoableAction...
So in this case I've chosen for being very consistent, at the expense of a bit of duplication, but I think we can live with that, as Transifex autofills translations for matching strings anyway.

@MarcSabatella
Copy link
Contributor

There is one other thing to consider here, and @shoogle you might have some insight.

Elsehwere I made the observation that the existing "respell pitches" is analogous to "regroup rhythms" in what it does: both algorithms are designed to enforce an internal model of what is "correct". They analyze your music and make changes only for the notes that are "incorrect" according to that model.

For regroup rhythms, my understanding is that the algorithm used is intended to be exactly the same as what is used to notate rhythms on MIDI import, and indeed reuses at least some of the same code.

I don't think this is true at all for respell pitches, though. As far as I know, the pitch spelling algorithm for MIDI import is entirely based on the key and the circle-of-fifths relationship of notes to the key, no other context considered. Whereas the respell pitches algorithm is all about surrounding context - especially horizontal.

Long term, I think the best solution is to consider a new algorithm that can be applied to both MIDI import and the current "respell pitches" command (regardless of what we call it), and name the latter command in a way that makes sense. Short term, we either use that name for the current "respell pitches" command, or ditch that command until it is more usable.

Plus, of course, add "change enharmonic spelling" to the menu. I personally think the name of that command is fine as is. The verb "change" is appropriately direct, and the phrase "enharmonic spelling" is precise. "Change pitch spelling" is somehow a little less obvious what it actually means. Something about B vs H? G vs Sol? I think the word "enharmonic" really helps with the clarity.

Jojo-Schmitz added a commit to Jojo-Schmitz/MuseScore that referenced this pull request Jan 20, 2025
…he "Change enharmonic spelling" commands to the Tools menu

Backport of musescore#26130
@Jojo-Schmitz
Copy link
Contributor Author

Jojo-Schmitz commented Jan 20, 2025

This is how the Tools menu currently (i.e. with this PR) looks:
image

@Jojo-Schmitz
Copy link
Contributor Author

Codestyle check failure is unrelated, see #26172

@Jojo-Schmitz
Copy link
Contributor Author

Jojo-Schmitz commented Jan 20, 2025

Now it neeeds #26173 to fix the codestyle issue

just a very minor nitpick, esp. when considering that this string is for accessibilty, i.e. only for screen readers. where capitalization wouldn't make a difference anyway.
Came up in https://discord.com/channels/818804595450445834/818852942882275379/1329909513159180288 ff.
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