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

Hovering piano keyboard shows shadow note #23612

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

Conversation

22justinl
Copy link
Contributor

@22justinl 22justinl commented Jul 14, 2024

Resolves: #13273

Hovering the keys in the virtual keyboard moves the shadow note.
For tablature and drum staves the shadow note vanishes.

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

@22justinl 22justinl force-pushed the pianokeyboard-shadownote branch from c6816d7 to 3026c63 Compare July 14, 2024 14:46
@22justinl 22justinl marked this pull request as ready for review July 14, 2024 14:46
@22justinl 22justinl force-pushed the pianokeyboard-shadownote branch 2 times, most recently from 39bcf51 to 407a308 Compare July 14, 2024 15:30
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.

Thanks, this looks quite good! I have a few comments, that should form a story together, if they appear in the correct order 😄

src/notation/internal/notationinteraction.cpp Outdated Show resolved Hide resolved
src/notation/view/abstractnotationpaintview.cpp Outdated Show resolved Hide resolved
src/notation/internal/notationinteraction.h Outdated Show resolved Hide resolved
src/notation/internal/notationinteraction.cpp Outdated Show resolved Hide resolved
src/notation/internal/notationinteraction.cpp Outdated Show resolved Hide resolved
src/notation/internal/notationinteraction.cpp Show resolved Hide resolved
@22justinl 22justinl force-pushed the pianokeyboard-shadownote branch 2 times, most recently from 7a7dbe2 to 30a414e Compare August 15, 2024 16:20
@22justinl 22justinl force-pushed the pianokeyboard-shadownote branch from 30a414e to 7dc4c05 Compare August 15, 2024 16:23
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.

Almost there! Some last cleanup remarks:

src/notation/internal/notationinteraction.cpp Outdated Show resolved Hide resolved
src/notation/internal/notationinteraction.cpp Outdated Show resolved Hide resolved
src/notation/view/pianokeyboard/pianokeyboardview.cpp Outdated Show resolved Hide resolved
@22justinl 22justinl force-pushed the pianokeyboard-shadownote branch from 7dc4c05 to db27dc0 Compare August 16, 2024 16:05
@22justinl
Copy link
Contributor Author

22justinl commented Aug 16, 2024

Would it be fine to rename showShadowNote(const PointF& pos) to showShadowNoteForPosition() since there are now two functions to show shadow notes? Also, should I add a hoverLeaveEvent for NotationViewInputController that hides the shadow note (like with the hoverLeaveEvent in the PianoKeyboardView)

@cbjeukendrup
Copy link
Contributor

Yes, both sound like good ideas!

@22justinl 22justinl force-pushed the pianokeyboard-shadownote branch 2 times, most recently from 30c7cc1 to 3d139b3 Compare August 16, 2024 17:31
@22justinl 22justinl force-pushed the pianokeyboard-shadownote branch from 3d139b3 to 47e681b Compare August 17, 2024 04:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants