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

Added buttons for PianoKeyboard to switch between Notated Pitch (effective pitch) and Playback Pitch. #22991

Closed
wants to merge 2 commits into from

Conversation

NathanPrazeres
Copy link
Contributor

@NathanPrazeres NathanPrazeres commented May 27, 2024

Resolves: #15594

Title describes well enough. We just 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.

  • 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

Co-authored-by: David Faia Nunes <[email protected]>
Copy link
Contributor

@cbjeukendrup cbjeukendrup left a comment

Choose a reason for hiding this comment

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

That's looking much better already! I have a few more cleanup suggestions, but nothing fundamental!

src/notation/inotationconfiguration.h Outdated Show resolved Hide resolved
src/notation/internal/notationconfiguration.h Outdated Show resolved Hide resolved
src/notation/view/pianokeyboard/pianokeyboardcontroller.h Outdated Show resolved Hide resolved
src/notation/view/pianokeyboard/pianokeyboardcontroller.h Outdated Show resolved Hide resolved
UiAction action;
action.title = title;
action.code = SET_NOTATED_PITCH_CODE;
action.checkable = Checkable::Yes;
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps, instead of "checkable", we should use "selectable". That is not a property of UiAction but of MenuItem.

So: item->setSelectable(true) to mark the item as selectable, and then bool selected = …; item->setSelected(selected) to set the state.

@shoogle
Copy link
Contributor

shoogle commented Jul 7, 2024

Here I have selected C4 in the score and it is correctly highlighted as Bb3 in the piano keyboard.

image

However, if I press Bb3 in the piano keyboard it gives me a Bb3 (actually A#3) in the score instead of C4.

I would expect the "Use playback pitch" option to work for entering notes as well as for highlighting exisiting ones.

@051056
Copy link

051056 commented Aug 24, 2024

Hello,
Please, let me know when this fonction (Added buttons for PianoKeyboard to switch between Notated Pitch)will be avalaible.
Thanks

@rpatters1
Copy link
Contributor

It is unclear to me what remains to do with this pull request? Is it just the final code review item?

@rpatters1
Copy link
Contributor

rpatters1 commented Sep 17, 2024

@cbjeukendrup I am trying to revive this PR. I have rebased my branch of it (with effort) on the current master branch.

It all seems to be working so far as it went, except for the problem noted in the comment above that when "Use playback pitch" is selected, it still enters the notated pitch into the score. It looks like NotationMidiInput::addNoteToScore may need to be modifed as well. I could use some direction on the most judicious change.

EDIT: I figured it out.

@rpatters1
Copy link
Contributor

This pull request is now completed as #24784

@cbjeukendrup
Copy link
Contributor

@NathanPrazeres Thanks for your work; although it has not been merged directly, it has been included in a new PR that expands on it, and will be finished via that PR!

@NathanPrazeres
Copy link
Contributor Author

Sorry for leaving this unfinished. I really haven't had time as of late. I'll glad it will be useful anyways. Thanks :)

rpatters1 added a commit to rpatters1/MuseScore that referenced this pull request Sep 19, 2024
@shoogle
Copy link
Contributor

shoogle commented Sep 19, 2024

@NathanPrazeres, no worries! Thanks for your contribution!

rpatters1 added a commit to rpatters1/MuseScore that referenced this pull request Sep 27, 2024
rpatters1 added a commit to rpatters1/MuseScore that referenced this pull request Oct 2, 2024
cbjeukendrup pushed a commit to cbjeukendrup/MuseScore that referenced this pull request Oct 5, 2024
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.

Feature request: Toggle for behaviour of on-screen/MIDI keyboard regarding transposing instruments
5 participants