Fix Issue #26867: Proper Export Menu Keyboard Navagation#32990
Fix Issue #26867: Proper Export Menu Keyboard Navagation#32990sshadid-code wants to merge 1 commit intomusescore:masterfrom
Conversation
Implemented navigation handling for ExportScoresListView, allowing up and down navigation through the list and focus management without unexpected behavior of looping after 20 items without manual scrolling
📝 WalkthroughWalkthroughEnhanced keyboard navigation in the export dialog's parts list by adding event handlers to the checkbox navigation. The implementation computes target indices when navigating with Up/Down keys, clamps bounds to prevent cycling, repositions the list view for visibility, and defers focus updates to the appropriate item. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d81967cb-cce1-473e-b3a7-1db674586929
📒 Files selected for processing (1)
src/project/qml/MuseScore/Project/internal/Export/ExportScoresListView.qml
| navigation.onNavigationEvent: function(event) { | ||
| let targetIndex = delegateItem.index | ||
|
|
||
| switch (event.type) { | ||
| case NavigationEvent.Up: | ||
| if (delegateItem.index === 0) { | ||
| event.accepted = true | ||
| return | ||
| } | ||
| targetIndex = delegateItem.index - 1 | ||
| break | ||
|
|
||
| case NavigationEvent.Down: | ||
| if (delegateItem.index === listView.count - 1) { | ||
| event.accepted = true | ||
| return | ||
| } | ||
| targetIndex = delegateItem.index + 1 | ||
| break | ||
|
|
||
| default: | ||
| return | ||
| } | ||
|
|
||
| listView.positionViewAtIndex(targetIndex, ListView.Contain) | ||
|
|
||
| Qt.callLater(function() { | ||
| let item = listView.itemAtIndex(targetIndex) | ||
| if (item) { | ||
| item.requestActiveFocus() | ||
| } | ||
| }) | ||
|
|
||
| event.accepted = true | ||
| } | ||
|
|
||
| Connections { | ||
| target: checkBox.navigation | ||
|
|
||
| function onActiveChanged() { | ||
| if (target.active) { | ||
| listView.positionViewAtIndex(index, ListView.Contain) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Please add a UI/QML regression test for this keyboard path.
Given this now owns custom boundary and focus behavior, add automated coverage for: (1) repeated Down to last item without wrap, (2) repeated Up to first item without wrap, and (3) deep-list traversal beyond initial viewport.
Based on learnings: In the MuseScore codebase, vtests are intended for src/engraving only; for .qml UI changes, use UI/QML-appropriate checks instead of vtests.
| } | ||
| }) | ||
|
|
||
| event.accepted = true |
There was a problem hiding this comment.
Lines 115-128 should not be needed. When the event is not accepted, the navigation system will move the focus to the checkbox above/below (what lines 121-126 do). This will trigger onActiveChanged and inside it the list view will be scrolled (i.e. what line 119 does will be done by line 136). Then targetIndex won't be needed either. P.S. Nope, I am wrong. Upon further testing I see that my suggestion does not work as I expected. Moving down works well but when you start going up, at some point it skips items and goes directly to the first item (the main score). I just noticed that the same buggy behavior exists with the Shortcuts list in Preferences that I gave you as an example. :) I'll investigate this when I can. Disregard my suggestion. P.S. ListView acts weirdly with regard to the instantiation of delegates outside of the visible area.
Your code does work by first scrolling the list view and then navigating. Do we need lines 121-128? If the list view gets scrolled (line 119) and the event is not accepted, then the navigation system should correctly move the focus. Also positionViewAtIndex ends up called twice (once in onNavigationEvent and again in onActiveChanged) but at least everything works. The second call won't do anything and should be fast. onActiveChanged can be removed to avoid the double call, but it does help with the other navigation keys not handled by the code such as PgUp, PgDn, Home and End.
There was a problem hiding this comment.
Your code does work by first scrolling the list view and then navigating. Do we need lines 121-128? If the list view gets scrolled (line 119) and the event is not accepted, then the navigation system should correctly move the focus. Also positionViewAtIndex ends up called twice (once in onNavigationEvent and again in onActiveChanged) but at least everything works. The second call won't do anything and should be fast. onActiveChanged can be removed to avoid the double call, but it does help with the other navigation keys not handled by the code such as PgUp, PgDn, Home and End.
From what I can tell navigation.onActiveChanged (which I updated to no longer use Connection) could be removed if either
A. the alternate navigation options such as left arrow, right arrow, PgUp, and PgDn could be disabled
B. the alternate navigation options could be included in the navigation.onNavigationEvent code
I've attempted to take approach B but have been unable to find what events these alternate navigation options are tied to. This may very well be due to my lack of understanding of the MuseScore codebase and/or my lack of familiarity with QML. Without knowing how to implement option A or B, navigation.onActiveChanged remains required.
There was a problem hiding this comment.
That's fine. I concluded my comment saying onActiveChanged should stay because it helps with the navigation with PgUp, PgDn, Home and End.
| } | ||
|
|
||
| Connections { | ||
| target: checkBox.navigation |
There was a problem hiding this comment.
Since this code is inside the checkbox, there is no need for Connections. Just like with the previous handler for navigation.onNavigationEvent above, here we can simply write
navigation.onActiveChanged: {
. . .
}
Then target.active will become navigation.active. You can take a look at the other navigation.onActiveChanged instances in the code.
I also suggest changing index to delegateItem.Index. index works but is more work for Qt to find it since it will first look for it in the checkbox and then up in the parents. Qualifying it with the ID of the object it belongs to eliminates this (and makes it clearer).
|
Nice work, @sshadid-code ! I've left a couple of suggestions for simplification. :) |
Thank @krasko78. I'll aim to implement and test these changes by the end of the day (potentially by tomorrow depending on my schedule). If there are any other changes you think I should implement for the PR to be accepted please let me know. |
|
I edited my first comment because I discovered I was wrong. PR will have to be reviewed by Eism so you may wait for his feedback. |
3 participants
Implemented navigation handling for ExportScoresListView, allowing up and down navigation through the list and focus management without unexpected behavior of looping after 20 items without manual scrolling
Resolves: #26867
Explicitly set navigation.column to 0 to ensure proper handling of the left and right arrow keys. The addition of Connections handles automatically scrolling the export menu while navigating with the arrow keys. This alone completely handles using arrow key navigation down the list of export options. Added navigation.onNavigationEvent to disallow navigating down from the final option and up from the first option. Otherwise such inputs would cause improper navigation behavior. This code also handles a bug where attempting to navigate back up the list would skip to the first option after 11 up inputs past the currently displayed top of the list.
No unit test was made in place of using extensive manual testing. This was due to time constraints and the small number of inputs possible in the export menu's part selection.
Summary by CodeRabbit