Skip to content

fix(plugin-store): keep scroll position across sheet body rebuilds - #3874

Draft
TheBinaryLoop wants to merge 1 commit into
noctalia-dev:mainfrom
TheBinaryLoop:fix/plugin-store-scroll-reset
Draft

fix(plugin-store): keep scroll position across sheet body rebuilds#3874
TheBinaryLoop wants to merge 1 commit into
noctalia-dev:mainfrom
TheBinaryLoop:fix/plugin-store-scroll-reset

Conversation

@TheBinaryLoop

Copy link
Copy Markdown
Contributor

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:

  1. SettingsWindow::onExternalOptionsChanged() runs requestSceneRebuild(), which calls m_editorSheetPopup->rebuildBody(). That destroys and repopulates the entire sheet body, so any node-local state is gone. UPowerService is one of its callers, and upowerd polls the battery every 30s — on a laptop the energy/rate values genuinely change, so the open sheet is rebuilt unprompted at that cadence.
  2. ScrollViewState is the existing mechanism for surviving exactly this, but it did not work for sheet bodies: ScrollView::doLayout wrote the clamped offset back into the bound state on every pass, and the first pass after a rebuild measures the content unconstrained (height() == 0, so viewportH == contentH). That pass reports maxScrollOffset == 0, so it zeroed the remembered offset before the real viewport height arrived on the following pass.

So this PR:

  • Binds the catalog grid and the detail page to ScrollViewState members on PluginStoreContent that outlive the rebuilt nodes. openDetail resets the detail state so opening a different plugin still starts at the top.
  • Replaces the stashScrollOffset/restoreScrollOffset pair 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.
  • Makes ScrollView write to bound state only once there is a scrollable extent. This also repairs bound state for the other users of it — SettingsSheetPopup binds m_scrollState for its scrollable body, so the plugin settings editor sheet had the same latent loss.

VirtualGridView never hit the doLayout trap because it always hands its ScrollView an 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

  • Bug fix
  • New feature
  • Breaking change
  • Refactoring
  • Build / packaging

Related Issue

Closes #3872

Testing

  • meson compile -C build-debug — clean
  • meson test -C build-debug --print-errorlogs — 79/79 pass
  • clang-format --dry-run -Werror on the three changed files — clean
  • run-clang-tidy -p build-debug -warnings-as-errors='*' on the two changed .cpp files — clean

Manual 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 sampled scrollOffset()/maxScrollOffset().

grid detail
before 400 → 0 on every tick 300 → 0
after 400 → 400 → 400 300 → 300

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

  • Tested on Niri
  • Tested on Hyprland
  • Tested on Sway
  • Tested on another compositor:
  • Tested with different bar positions and density settings
  • Tested at different interface scaling values
  • Tested with multiple monitors

Screenshots / Videos

None — the change has no visual output of its own; the observable difference is the scroll offset table above.

Checklist

  • This PR is ready for review, or it is marked as Draft.
  • I read and followed the relevant guidance in CONTRIBUTING.md.
  • I ran just format with clang-format v22+ installed, or this PR has no code changes.
  • I ran the relevant build or test commands, or explained why they were not run.
  • I self-reviewed the changes.
  • I checked for new warnings or errors.
  • I will update end-user documentation after merge, or this PR does not change user-facing configuration or behavior.
  • I added or updated assets/translations/en.json, or this PR adds no new user-facing strings.
  • I did not edit non-English translation files unless this PR is explicitly for translation tooling, an import/export sync, or a maintainer-requested locale change.
  • I used the existing canonical names for config keys, IPC names, paths, and identifiers.

Additional Notes

The ScrollView::doLayout change 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 example settings_window_scene.cpp sets m_contentScrollState.offset = 0.0f on section change, and openDetail now does the same.

Left deliberately out of scope: onExternalOptionsChanged is 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.

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
@ItsLemmy

Copy link
Copy Markdown
Collaborator

Is this ready for review?

@jinliu

jinliu commented Aug 15, 2026

Copy link
Copy Markdown
Contributor

This works initially, but breaks after clicking into one plugin, then back.

To reproduce:

  1. Open plugin store.
  2. Click on a plugin.
  3. Click "back".
  4. Scroll down.
  5. Wait for 30s

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.

[BUG] Plugin Store periodically resets scroll position

3 participants