Keep background refreshes from scrolling an open select menu - #7413
Open
melonamin wants to merge 1 commit into
Open
Keep background refreshes from scrolling an open select menu#7413melonamin wants to merge 1 commit into
melonamin wants to merge 1 commit into
Conversation
Desktop-entry rescans (e.g. mise touching ~/.local/share on every shell activation), provider exits, guard batches, and menu source reloads all called rebuildDisplay() on the open menu. In dmenu mode the rows come solely from dmenuOptions, so the rebuild only cleared and re-appended an identical model — resetting the ListView scroll to the cursor row at the top while the user was reading. Skip the rebuild when a dmenu is active.
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Prevents background refreshes from rebuilding an actively open dmenu (avoiding scroll position resets) and adds a regression test to enforce the new guard.
Changes:
- Gate
root.rebuildDisplay()behind!root.dmenuActiveduring background refresh paths. - Simplify provider reload logic by removing the inner
if (!root.dmenuActive)block (now implied by the new outer condition). - Add a test asserting background rebuild sites include the
!root.dmenuActiveguard.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| test/shell.d/menu-test.sh | Adds assertions to ensure background refresh rebuilds are guarded to avoid touching an open dmenu. |
| shell/plugins/menu/Menu.qml | Adds !root.dmenuActive guards to prevent rebuildDisplay during background refreshes while dmenu is active. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+589
to
+592
| assertEqual( | ||
| backgroundRebuilds.length, | ||
| 4, | ||
| 'menu rebuilds the open display from app, provider, guard, and source refreshes' |
Comment on lines
+258
to
262
| if (root.opened && !root.dmenuActive) { | ||
| root.rebuildDisplay() | ||
| if (!root.dmenuActive) { | ||
| if (root.filterText.trim()) root.loadProvidersForSearch() | ||
| else root.loadProviderForMenu(root.activeMenu) | ||
| } | ||
| if (root.filterText.trim()) root.loadProvidersForSearch() | ||
| else root.loadProviderForMenu(root.activeMenu) | ||
| } |
| root.items = merged.items | ||
| root.itemOrder = merged.itemOrder | ||
| if (root.opened) root.rebuildDisplay() | ||
| if (root.opened && !root.dmenuActive) root.rebuildDisplay() |
| root.items = merged.items | ||
| root.itemOrder = merged.itemOrder | ||
| if (root.opened) root.rebuildDisplay() | ||
| if (root.opened && !root.dmenuActive) root.rebuildDisplay() |
| root.checkedResults = nextChecked | ||
| root.disabledResults = nextDisabled | ||
| if (root.opened) root.rebuildDisplay() | ||
| if (root.opened && !root.dmenuActive) root.rebuildDisplay() |
3 tasks
This file contains hidden or 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
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.
Leave the Shortcuts menu (Super+K) open for a few seconds and it snaps back to the top while you're reading. Anything that wakes the desktop-entry watcher triggers it — mise stamps
~/.local/shareon every shell activation, so on a machine with mise the menu resets every few seconds.The chain:
DesktopEntriesrescans →AppLibraryfiresappsChanged→ the menu plugin'smergeAppRows()ends withrebuildDisplay()on the open menu (armed once the apps provider has loaded during the shell's lifetime). In dmenu mode the rows come solely fromdmenuOptions, so the rebuild clears and re-appends an identical model, resetting the ListView's scroll;revealCursor()then scrolls back to the cursor, which wheel scrolling never moved off row 0. Provider exits, guard batches, and menu source reloads hit the same path.Fix: skip the display rebuild in the four async completion paths when a dmenu is active. Menu mode is unchanged — those rebuilds still run there, where rows genuinely depend on the refreshed data.
Testing:
test/shell.d/menu-test.shnow asserts all fourroot.opened-guardedrebuildDisplay()sites carry!root.dmenuActive;./test/shellpasses..desktopfile to force a rescan. Stock 4.0.0 and pre-fix HEAD snap back to the top (after-trigger frame pixel-identical to the opening frame); with this change the scrolled position holds (pixel-identical to the scrolled frame).