Scroll the selection into view by default again - #5928
Merged
Conversation
We are about to make list panels scroll their selection into view automatically. Page up and down are one of the few places that manage the scroll position themselves, keeping the selection at the edge of the viewport rather than in its middle, and nothing covers that today. Asserting on it needs an exact scroll position assertion; only OriginYAtLeast existed so far. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The one behaviour that made scrolling the selection into view opt-in in the first place — a background refresh must not yank the view back to a selection the user scrolled away from — has never been covered by a test. It's about to become the one case that the automatic scrolling has to suppress, so cover it first. Getting there needs two things from the test harness: mouse wheel events, which are the only way to scroll a list panel without moving the selection, and a way to trigger a background refresh. The periodic routine that issues it is turned off in tests, and turning it on would mean waiting for its timer and hoping it fires while we're looking, so drive the refresh directly instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This is the other place that manages its own scroll position: while a drag extends the selection to a line below the viewport, the view stays put, and the drag autoscroller scrolls it one line at a time for as long as the pointer stays there. Making the scroll automatic would centre the selection instead, i.e. jump the view rather than scroll it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Since scrolling the selection into view became opt-in, five places have had to be fixed by hand after the fact, none of them with a test. Cover them now: making the scrolling automatic has to keep all five working, and once it does, the hand-added scroll calls can go. Two of them assert that the selection is visible rather than on an exact scroll position, because the panel they look at changes height along the way (filtering mode switches to half screen), or because what matters is only that the commit we jumped to can be seen. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Ever since scrolling the selection into view became opt-in, we have been fixing the same class of regression by hand, five times so far: a controller moves the selection somewhere new, doesn't say that it wants the view to follow, and the selection ends up off screen. The decision needs facts from two places — whether the selection went somewhere new is known to the list, whether the scroll position is the caller's to manage is known to the caller — and asking every caller for both is what keeps going wrong. The callers that get it wrong are usually not even the ones that moved the selection: they are pass-throughs like postRefreshUpdate, which can't know what a refresh did to the selection. So default to scrolling, and let the two callers that maintain the scroll position themselves say so. The one case where scrolling is always wrong is a refresh that no user action is behind: a background poll, or a reload of state on window focus, after a subprocess, or after a repo switch. Those must leave the viewport wherever the user last scrolled it to — that is what made the scrolling opt-in in the first place. Both are already marked in RefreshOptions, so the refresh can decide it once, centrally, instead of each caller judging it. A user action that ends in a foreground refresh does now yank the view back to the selection if the user had scrolled away from it. That's a behaviour change, and there may be actions where it turns out to be unwelcome; those we can fix individually, and it beats the ones that don't scroll today. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every one of these did by hand what focusing the list now does on its own: five hand-added scroll requests, and four origin resets that paired a "select the first item" with a "and show the top of the list". The scroll that the commits refresh performed when it found the selected commit at a new index goes too. It is now unconditional for a foreground refresh, and deliberately absent for a background one: when an agent commits in another window, we would rather see the new commits arrive than have the view yank itself back to the commit we had selected. The one origin reset that stays is the one in ReApplyFilter, which runs as part of a refresh and so can't rely on the refresh scrolling. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
1 participant
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.
In v0.58 (see #5134) we changed the refresh behavior to no longer scroll a selection into view by default if it had been scrolled out of view using the mouse wheel; the primary reason for that was to avoid a background fetch or files refresh to yank the selection back into view while you are looking at something else, which was pretty annoying. However, that meant we had to fix lots of cases where the selection didn't become visible after a normal, foreground user action, and add code to manually scroll it into view again in those cases. This is error-prone and easy to forget for new features, and to this day we were still missing some.
So turn it around: by default, the selection is scrolled into view again, and the few cases where we don't want it (background routines and the focus-in refresh) opt out.
This does mean that some user actions now scroll into view that didn't before, and there might be cases where this is unwanted (I can't think of one, but who knows). If we come across one we can easily fix it by opting out; this is probably still much better than not scrolling into view where it's wanted.