New percussion mouse input interaction & popup #26000
Open
+830
−51
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mathesoncalum
force-pushed
the
percussion_input_popup
branch
5 times, most recently
from
da5ed71 to
e9cfe53
Compare
mathesoncalum
force-pushed
the
percussion_input_popup
branch
from
e9cfe53 to
ae30d4d
Compare
mathesoncalum
changed the title
mathesoncalum
force-pushed
the
percussion_input_popup
branch
from
ae30d4d to
54069cb
Compare
mathesoncalum
force-pushed
the
percussion_input_popup
branch
from
54069cb to
c015a98
Compare
| nval.pitch = m_is.drumNote(); | ||
| if (nval.pitch < 0 || !ds->isValid(nval.pitch) || ds->line(nval.pitch) != line) { | ||
| // Drum note from input state is not valid - fall back to the first valid pitch for this line... | ||
| m_is.setDrumNote(-1); |
There was a problem hiding this comment.
I don't like that we change the input state here... Ideally, noteValForPosition should be const, and not change the state implicitly
| } | ||
| if (drumNotePitch < 0) { | ||
| shadowNote.setVisible(false); | ||
| m_shadowNoteChanged.notify(); |
There was a problem hiding this comment.
We should not notify here, because this function is also used to draw the input by duration preview. The notification should be moved to NotationInteraction::showShadowNote
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR implements the new percussion mouse input interaction and associated popup.
Part 1 -
ShadowNotePopupgroundwork (eb6c141 & 21cb8ed)Two of the main goals in this PR were to:
ElementPopuplogic as possible.With this in mind, the idea with this commit was to provide a generalised shadow-note popup which will determine its “content” based on the context (
ShadowNotePopupModel::currentPopupType). This should help with future flexibility, for example if we wish to implement other shadow-note-based popups.A couple of optimisations were made to our existing
ElementPopuplogic. Firstly, we no longer attempt to show a popup if it is already showing - this is essential forShadowNotePopupsas we should try to update the visibility on everyhoverMoveEvent. Second, the option to close a specific element popup is also included here - we need this because we don't want to close all element popups when shift is released, only the shadow-note popup....
Part 2 - Percussion shadow-note popup (c015a98)
The content of this commit is fairly self-explanatory. It implements the “percussion note content” UI and outlines the circumstances where
ShadowNotePopupshould load this content.This commit also contains logic for evaluating which percussion note should be inputted on click (see
Score::noteValForPosition)