Switch PianoKeyboard between Notated Pitch (effective pitch) and Playback Pitch (completed).

[rpatters1]

Resolves: #15594

Supercedes: #22991, which should be closed.

Added buttons in the PianoKeyboardPanelContextMenuModel class that communicate with the PianoKeyboardController class to change a boolean variable. The aim is to address a limitation in the current implementation of MuseScore's keyboard view, which struggles with differently-tuned instruments. When users click on a key, the corresponding note used to be placed on the score as notated pitch, while the equivalent standard pitch was highlighted on the keyboard UI, leading to potential confusion.

This PR also adds a change to Score::noteVal so that the correct pitch is entered into the score, based on the menu option selected.

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

[rpatters1]

This PR should be ready for review now. I moved the pianoKeyboardUseNotatedPitch getter and setter from INotationConfiguration to IEngravingConfiguration to allow accessing the property inside the Score class. Everything seems to be working., though I am not currently set up to test configuration persistence across executions of MuseScore

EDIT: I tested without the -F option and it indeed preserves the setting across executions of MuseScore.

[cbjeukendrup]

I'm not sure if we should do this check here. It might be preferable to move the setting back to notationConfiguration and retrieve its value outside the engraving module (since I'd rather consider it UI logic than business logic) and pass it as a bool parameter to Score::addMidiPitch and Score::noteVal.

[rpatters1]

I don't disagree with what you say at all, but I have trouble seeing any philosophical difference between the Display Concert Pitch setting and this new one as far as to whether they are business logic or UI. If we are going to move the new one, we should move the old one as well, and that may get into a rats' nest of refactoring. (You would know better than I whether it is a lot.)

Or maybe the noteVal function should not be part of Score. But that seems like gets into even more refactoring.

[cbjeukendrup]

I would say that the difference is the scope of these settings. Sid::concertPitch is a score setting, and affects also how the score is typeset, i.e. really the content; pianoKeyboardUseNotatedPitch is a UI setting, independent of the score, affecting only the behaviour of the UI.

There are absolutely a lot more opportunities for refactoring in the Score class and things around note input... but let's save that for different PRs!

[rpatters1]

Hmm. I hope you will get to know me well enough that I sometimes when write out a counter argument, I then begin arguing against my own argument in my head and eventually convince myself that I was wrong. I think I get how Display Concert Pitch could be construed as business logic (perhaps because it controls engraving output) while there is no possibility of the input being so construed. So I will investigate your suggestion.

[cbjeukendrup]

See also the recent discussion in #15594; the scope of the issue has been extended a little bit. Personally I prefer the two "use notated pitch" / "use sounding pitch" options over a single checkbox though, because with these options you really know what you are choosing from. I'll add that as a comment at #15594.

[rpatters1]

The boolean (potentially) needs to be passed from three locations:

• NotationMidiInput::doRealtimeAdvance
• NotationMidiInput::addNoteToScore
• NotationMidiInput::makeNote

I am passing the boolean in from the latter 2 but not the 1st (doRealtimeAdvance). The only one I have figured out how to test is addNoteToScore. Do you think they all 3 should pass in the boolean?

[rpatters1]

I agree that the UI should be in Preferences. I'll tackle that on a different commit, though it can be on this PR if you wish. I would like to nail down the PR as it was.

[cbjeukendrup]

Hm, adding a bool parameter useNotatedPitch to Score::realtimeAdvance would also look a bit weird. One way to solve that would be to get rid of that method and move its content of that method to NotationMidiInput. Perhaps that's not a bad idea anyway.

Re. how to test: when you right-click the note input button in the toolbar, you get a menu where you can choose "realtime" note input mode. I've never studied how this works, but maybe the MuseScore 3 (!) handbook is useful: https://musescore.org/en/handbook/3/note-input-modes#realtime-auto

Making the preferences changes in separate commits to this PR seems good to me.

[rpatters1]

Given that Score is business logic, I provided a name for the boolean that makes no reference to the piano keyboard: allowTransposition. Does adding that parameter to Score::realtimeAdvance seem weird? Given how it works, it doesn't seem weird to me, but if you want me to I'll look at moving. It gets called exactly once by NotationMidiInput::doRealtimeAdvance.

Perhaps allowTransposition describes more precisely what the business logic is doing than useNotatedPitch would. Basically, if Display Concert Pitch is unchecked, the business logic transposes the note(s). This new parameter says only do that when it is allowed.

[rpatters1]

I have removed the menu items from the piano keyboard and added them to the playback settings menu, as shown.

I went ahead and changed the text to simple Anglo Saxon words instead of the fancy French words that we were using before. This is consistent with prior art. (I'm assuming menu item text is not copyrighted. If so, we can change it.)

[rpatters1]

Assuming the checks pass, it looks ready for review to me. (I've reviewed the cumulative file changes and they now all look as I intended them to.) The names have all been changed to match the new menu items and location of the menu.

[rpatters1]

excellent suggestions both. update is posted

[cbjeukendrup]

Apart from that last thing, looks good to me!

[rpatters1]

they say naming is the hardest thing in programming. fixed it.

[Jojo-Schmitz]

they say naming is the hardest thing in programming. fixed it.

There are only two hard things in Computer Science: cache invalidation and naming ... things.

-- Phil Karlton

There are 2 hard problems in computer science: cache invalidation, naming things, and off-by-1 errors.

-- Leon Bambrick

There are only two hard problems in distributed systems: 2. Exactly-once delivery 1. Guaranteed order of messages 2. Exactly-once delivery

-- Mathias Verraes

there's two hard problems in computer science: we only have one joke and it's not funny.

-- Phillip Scott Bowden

There are so many variations on the “there are only two hard problems in computer programming...” joke that I’m starting to suspect that programming isn’t actually very easy.

-- Nat Pryce

[zacjansheski]

Tested on MacOS 14, Windows 11, Ubuntu 22.04.3. Approved
#15594 FIXED

[avvvvve]

Functionally this looks good, and the naming and placement seem sensible to me! I've asked for @bkunda's signoff on the latter and I have one suggestion for a different icon we could use (the tuning fork that's used for "Concert pitch"), but we can save that for a part 2.

[cbjeukendrup]

@rpatters1 I'll merge this PR right now. After some discussion today, we decided that we might want to make some further tweaks how and where this option is presented; see #25033. The exact design is still in progress, but we'll ping you when it's ready, in case you would like to implement that part as well. Thanks!

[rpatters1]

I'm up for doing the implementation. If we are using a new icon, I'd prefer someone else design it. (Graphic design is not where my talents lie.😅)