fix(plugin-store): keep scroll position across sheet body rebuilds - #3874
Draft
TheBinaryLoop wants to merge 1 commit into
Draft
fix(plugin-store): keep scroll position across sheet body rebuilds#3874TheBinaryLoop wants to merge 1 commit into
TheBinaryLoop wants to merge 1 commit into
Conversation
The settings sheet destroys and repopulates its whole body whenever an external option changes, and UPower's 30s battery poll is enough to trigger that on a laptop. The catalog grid and the plugin detail page were rebuilt from scratch each time, so both jumped back to the top roughly every 30 seconds. Bind both views to ScrollViewState that outlives the nodes instead of stashing the offset at the few call sites that happened to know a rebuild was coming; the detail page still starts at the top when a different plugin is opened. ScrollView also has to stop writing the clamped offset back into bound state on a pass that measures the content unconstrained: that pass reports no scrollable extent, so it zeroed the remembered offset before the real viewport height arrived. Fixes noctalia-dev#3872
2 tasks
Collaborator
|
Is this ready for review? |
Contributor
|
This works initially, but breaks after clicking into one plugin, then back. To reproduce:
|
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.
Summary
The plugin store's catalog grid and plugin detail page jumped back to the top roughly every 30 seconds. Two things combine to cause it:
SettingsWindow::onExternalOptionsChanged()runsrequestSceneRebuild(), which callsm_editorSheetPopup->rebuildBody(). That destroys and repopulates the entire sheet body, so any node-local state is gone.UPowerServiceis one of its callers, andupowerdpolls the battery every 30s — on a laptop the energy/rate values genuinely change, so the open sheet is rebuilt unprompted at that cadence.ScrollViewStateis the existing mechanism for surviving exactly this, but it did not work for sheet bodies:ScrollView::doLayoutwrote the clamped offset back into the bound state on every pass, and the first pass after a rebuild measures the content unconstrained (height() == 0, soviewportH == contentH). That pass reportsmaxScrollOffset == 0, so it zeroed the remembered offset before the real viewport height arrived on the following pass.So this PR:
ScrollViewStatemembers onPluginStoreContentthat outlive the rebuilt nodes.openDetailresets the detail state so opening a different plugin still starts at the top.stashScrollOffset/restoreScrollOffsetpair added in 32a3ad3. That approach only covered the three interaction sites that knew a rebuild was coming, so it could not help with rebuilds the store never initiated, and it did not cover the detail view at all.ScrollViewwrite to bound state only once there is a scrollable extent. This also repairs bound state for the other users of it —SettingsSheetPopupbindsm_scrollStatefor its scrollable body, so the plugin settings editor sheet had the same latent loss.VirtualGridViewnever hit thedoLayouttrap because it always hands itsScrollViewan explicit height, which is why the grid half was salvageable with a stash and the detail half was not.Motivation
Fixes the reported behavior: scrolling the store or a plugin's description, then having the view snap back to the top a few seconds later, repeatedly. On a laptop it is unavoidable — the battery poll alone is enough to trigger it.
Type of Change
Related Issue
Closes #3872
Testing
meson compile -C build-debug— cleanmeson test -C build-debug --print-errorlogs— 79/79 passclang-format --dry-run -Werroron the three changed files — cleanrun-clang-tidy -p build-debug -warnings-as-errors='*'on the two changed.cppfiles — cleanManual verification: the store sheet can only be opened by clicking and this machine has no pointer/key injection tool, so I drove it with a temporary env-gated harness (removed before commit) that auto-opened the store, fired
onExternalOptionsChanged()on a 30s repeat as a stand-in for the UPower tick, forced a scroll offset, and sampledscrollOffset()/maxScrollOffset().Note for anyone reproducing this way: with the display asleep no layout passes run, so the sampled offsets are pre-layout values and mean nothing. The numbers above are from a run with the display kept awake.
Manual Coverage
Screenshots / Videos
None — the change has no visual output of its own; the observable difference is the scroll offset table above.
Checklist
CONTRIBUTING.md.just formatwith clang-format v22+ installed, or this PR has no code changes.assets/translations/en.json, or this PR adds no new user-facing strings.Additional Notes
The
ScrollView::doLayoutchange is shared UI code, so it is worth a careful look. The behavioral delta is narrow: when a bound scroll view's content fits its viewport, the remembered offset is no longer overwritten with zero. Nothing renders differently in that state (there is nothing to scroll), and if the content grows again the offset comes back, which is the intent of binding state in the first place. Callers that want a genuine reset already do it explicitly — for examplesettings_window_scene.cppsetsm_contentScrollState.offset = 0.0fon section change, andopenDetailnow does the same.Left deliberately out of scope:
onExternalOptionsChangedis a blunt "rebuild everything" signal, so a battery tick rebuilds the whole settings scene every 30 seconds on a laptop. Narrowing it would touch every caller and deserves its own PR rather than riding along with a bug fix.