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

Percussion panel - refinements round 3 #25810

Merged

Conversation

mathesoncalum
Copy link
Contributor

@mathesoncalum mathesoncalum commented Dec 12, 2024

This round of refinements:

  1. Pressing escape while the percussion panel is in edit mode will finish editing and keep the changes (60d9b72)
  2. Keyboard navigation fixes (mainly ea82362)
    2.1. The last active pad is focused when finishing editing
    2.2. Focus remains on the "destination pad" after a pad swap
    2.3. When adding a row, focus moves to the first pad in the new row
    2.4. Entering notes via write mode no longer switches focus to the notation (ab11c42 & 3dd2de2)
  3. Implemented a "sound title label" detailing the currently selected sound in the mixer (7497598)
  4. Small copy change from "Instrument names" to "Pad names" (36cc850)
  5. Implemented the "pad swap dialog" and associated preferences (71e9a30, 2545c9d, 7c43a48)
  6. Added notation/percussion translation context (a728361)

@mathesoncalum mathesoncalum force-pushed the percussions_refinements_3 branch from 15fd861 to 4b21bd2 Compare December 17, 2024 10:21
}
}

//! NOTE: Can't use radioButton.text because it won't wrap
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we should improve it and add the ability to specify the wrap mode

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed - I'll address this in a separate PR.

}

FlatButton {
id: useNewPercussionPanel
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really like the fact that we will have to support both the old and new percussion panels. We should get rid of the old panel as soon as possible

@mathesoncalum mathesoncalum force-pushed the percussions_refinements_3 branch from 4b21bd2 to 2338eba Compare December 17, 2024 15:39
@mathesoncalum mathesoncalum force-pushed the percussions_refinements_3 branch from 2338eba to a728361 Compare December 18, 2024 10:24
@shoogle
Copy link
Contributor

shoogle commented Dec 19, 2024

Looks good to me!

@avvvvve
Copy link

avvvvve commented Dec 20, 2024

Looking great so far! Some things I noticed:

  1. Crash: Open a score with a percussion instrument, enter edit mode in the percussion panel, move a pad, press undo, crash. I'm guessing it's because we're not registering the move as an undo step until the you "Finish editing", in which case all edits become a single "Edit percussion panel layout" undo step. We could solve this by making each individual move undoable, regardless of whether you're in edit layout mode or not.

  2. Pressing the left arrow when focusing within certain groups won't cycle back to the last focusable thing. i.e. focus on the Write/Preview button. Pressing the right, up, or down arrows over and over will swap between the two options, but pressing left when on "Write" does nothing. Same problem for 'Layout'/'Customize kit'

  3. (Checking against spreadsheet, I see that this might not have been worked on yet) Pressing up while focused in the middle of the top row moves focus left rather than back to the last row. Vice versa for the bottom row or a pad with empty slots beneath it: when pressing down, focus moves right when it should move down to the next row or back to the first one if you're already in the last row.

  4. When entering edit mode via keyboard nav, focus should probably move straight to the pads so you don't have to press tab again.

  5. When focused somewhere outside the percussion panel (with the percussion panel open), Shift + Tab to move focus backwards through the focus order is skipping everything in the percussion panel.

  6. If I enter some notes in write mode, and without exiting note input mode, enter "Edit layout" mode, then use "Esc" to exit "Edit layout" mode, a subsequent "Esc" should exit note input mode (currently it does nothing).

  7. When you're in edit mode, and click to anything in the score that isn't a percussion instrument, the "Select an unpitched percussion staff to see available sounds" state comes up but still appears to be in edit mode. Funnily enough it even lets you add rows! This fixes itself if you click a percussion instrument again, but it shouldn't happen.
    image

  8. The "Select an unpitched percussion staff to see available sounds" empty state should be vertically centered in the box if possible (right now it's top-aligned):
    image

  9. "Add row" button's focus outline is cut off on the edge

image
  1. I should be able to scroll the pad rows when hovering in the blank space to the left/right of the pads, but I can't.

  2. I think you already noted this one, but 'Reset layout' doesn't do anything sometimes. Try making and saving edits multiple times to reproduce.

  3. Preferences are functioning well, but when you get a chance please match up the vertical spacings between things on the Percussion tab to what is spec'd out in Figma.

@mathesoncalum
Copy link
Contributor Author

This is really excellent info @avvvvve. Quite a few points in there I wasn't aware of - thanks!

Point 1 - As noted in some private discussions, this crash seems quite difficult to repro (I haven’t been able to do it on my end yet). However, there is a related problem where the panel doesn’t always update visually on undo/redo or reset layout (point 11). I have a fix for this ready and I’ll be sticking it in my next refinements PR.



Point 5 - The navigation order of the panel itself is a known issue, but it should only be rectified after #22050. I haven’t actually been able to repro the “skipping entirely” bug myself, but it has been mentioned before so we’ll keep an eye on that.

I’ve noted down everything else you mentioned, and plan to address in a separate PR. For now, I think we should be OK to merge this PR as it doesn’t cause any regressions for the wider app, and none of the panel-specific regressions seem overly critical.

@mathesoncalum mathesoncalum merged commit 689fd84 into musescore:master Dec 20, 2024
11 checks passed
@mathesoncalum mathesoncalum deleted the percussions_refinements_3 branch December 20, 2024 10:28
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.

4 participants