From aebf495dce7fa1da4fd30e41ad032b23fb50377a Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sat, 8 Aug 2026 16:01:43 +0200 Subject: [PATCH] Scroll the selection into view by default MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- pkg/gui/context/list_context_trait.go | 2 +- pkg/gui/controllers/helpers/refresh_helper.go | 24 +++++++++++++++---- pkg/gui/controllers/list_controller.go | 14 +++++++---- .../controllers/local_commits_controller.go | 4 ++-- pkg/gui/gui_common.go | 6 ++++- pkg/gui/types/common.go | 7 +++++- pkg/gui/types/context.go | 10 +++++--- pkg/gui/view_helpers.go | 6 ++--- 8 files changed, 53 insertions(+), 20 deletions(-) diff --git a/pkg/gui/context/list_context_trait.go b/pkg/gui/context/list_context_trait.go index 597fc99df..9ab24cf9b 100644 --- a/pkg/gui/context/list_context_trait.go +++ b/pkg/gui/context/list_context_trait.go @@ -89,7 +89,7 @@ func formatListFooter(selectedLineIdx int, length int) string { } func (self *ListContextTrait) HandleFocus(opts types.OnFocusOpts) { - self.FocusLine(opts.ScrollSelectionIntoView) + self.FocusLine(!opts.KeepScrollPosition) self.GetViewTrait().SetHighlight(self.list.Len() > 0) diff --git a/pkg/gui/controllers/helpers/refresh_helper.go b/pkg/gui/controllers/helpers/refresh_helper.go index 6672fab26..81eeec4d1 100644 --- a/pkg/gui/controllers/helpers/refresh_helper.go +++ b/pkg/gui/controllers/helpers/refresh_helper.go @@ -111,6 +111,14 @@ type refreshEnv struct { // persist its refreshed stat cache. backgroundRoutine bool + // Whether the views this refresh updates must keep the scroll position they + // have. Focusing a list scrolls its selection into view, which is what a + // user action should do — but a refresh that no user action is behind must + // leave the viewport wherever the user last scrolled it to. That's the case + // for the unattended background routines, and for the refreshes that merely + // reload state (see RefreshOptions.DontBlockRepoSwitch). + keepScrollPosition bool + // the repo generation captured when the refresh started generation int @@ -220,8 +228,9 @@ func (self *RefreshHelper) performRefresh(options types.RefreshOptions, calledFr // against the repo it started in, and the generation guard drops its // writes. env := refreshEnv{ - background: options.Background || options.DontBlockRepoSwitch, - backgroundRoutine: options.Background, + background: options.Background || options.DontBlockRepoSwitch, + backgroundRoutine: options.Background, + keepScrollPosition: options.Background || options.DontBlockRepoSwitch, } if !self.captureOnUIThread(calledFromWorker, env.background, func() { env.generation = self.c.State().GetRepoGeneration() @@ -1595,7 +1604,11 @@ func (self *RefreshHelper) refreshView(context types.Context, env refreshEnv) { // the filtered list model is up to date for rendering. self.searchHelper.ReApplyFilter(context) - self.c.PostRefreshUpdate(context) + if env.keepScrollPosition { + self.c.PostRefreshUpdateKeepingScrollPosition(context) + } else { + self.c.PostRefreshUpdate(context) + } self.c.AfterLayout(func() error { // Re-applying the search must be done after re-rendering the view though, @@ -1771,7 +1784,10 @@ func (self *RefreshHelper) setGithubPullRequests(baseInfo *githubRemoteInfo, bra // the branches and remotes as they are on the UI thread, after their // own refreshes' bounces have applied. self.rebuildPullRequestsMap() - self.c.PostRefreshUpdate(self.c.Contexts().Branches) + // This lands whenever the network call happens to return, and only + // changes how the branches are rendered, not which one is selected, so + // it has no business moving the viewport. + self.c.PostRefreshUpdateKeepingScrollPosition(self.c.Contexts().Branches) }) } diff --git a/pkg/gui/controllers/list_controller.go b/pkg/gui/controllers/list_controller.go index 8136c6aa9..c073e5141 100644 --- a/pkg/gui/controllers/list_controller.go +++ b/pkg/gui/controllers/list_controller.go @@ -136,7 +136,7 @@ func (self *ListController) handleLineChangeAux(f func(int), change int) error { self.context.SetNeedRerenderVisibleLines() } - self.context.HandleFocus(types.OnFocusOpts{ScrollSelectionIntoView: true}) + self.context.HandleFocus(types.OnFocusOpts{}) } else { // If the selection did not change (because, for example, we are at the top of the list and // press up), we still want to ensure that the selection is visible. This is useful after @@ -205,9 +205,10 @@ func (self *ListController) handlePageChange(delta int) error { // must tell it explicitly to rerender. self.context.SetNeedRerenderVisibleLines() - // Since we are maintaining the scroll position ourselves above, there's no point in passing - // ScrollSelectionIntoView=true here. - self.context.HandleFocus(types.OnFocusOpts{}) + // This function scrolls the view itself, keeping the selection at the edge of + // the viewport rather than in its middle, so the scroll position is ours to + // maintain, not the focus mechanism's. + self.context.HandleFocus(types.OnFocusOpts{KeepScrollPosition: true}) return nil } @@ -280,7 +281,10 @@ func (self *ListController) selectRangeThroughViewIndex(viewIndex int) { newSelectedLineIdx := self.context.ViewIndexToModelIndex(viewIndex) list.ExpandNonStickyRange(newSelectedLineIdx - list.GetSelectedLineIdx()) - self.context.HandleFocus(types.OnFocusOpts{}) + // The pointer can be outside the viewport, in which case so is the end of + // the range; the drag autoscroller takes care of following it, one line at a + // time, for as long as the pointer stays there. + self.context.HandleFocus(types.OnFocusOpts{KeepScrollPosition: true}) } func (self *ListController) handleDragAutoscroll(viewIndex int) bool { diff --git a/pkg/gui/controllers/local_commits_controller.go b/pkg/gui/controllers/local_commits_controller.go index 642263d12..0a02e7398 100644 --- a/pkg/gui/controllers/local_commits_controller.go +++ b/pkg/gui/controllers/local_commits_controller.go @@ -1171,7 +1171,7 @@ func (self *LocalCommitsController) move( return err } self.context().MoveSelection(offset) - self.context().HandleFocus(types.OnFocusOpts{ScrollSelectionIntoView: true}) + self.context().HandleFocus(types.OnFocusOpts{}) // Block input until the refresh has landed: a quick second press must // read the moved todo from the refreshed model, not grab whatever the @@ -1204,7 +1204,7 @@ func (self *LocalCommitsController) move( Then: func() error { if err == nil { self.context().MoveSelection(offset) - self.context().HandleFocus(types.OnFocusOpts{ScrollSelectionIntoView: true}) + self.context().HandleFocus(types.OnFocusOpts{}) } if onComplete != nil { return onComplete() diff --git a/pkg/gui/gui_common.go b/pkg/gui/gui_common.go index e7b14ba04..d92284ea5 100644 --- a/pkg/gui/gui_common.go +++ b/pkg/gui/gui_common.go @@ -39,7 +39,11 @@ func (self *guiCommon) RefreshFromWorker(opts types.RefreshOptions) { } func (self *guiCommon) PostRefreshUpdate(context types.Context) { - self.gui.postRefreshUpdate(context) + self.gui.postRefreshUpdate(context, false) +} + +func (self *guiCommon) PostRefreshUpdateKeepingScrollPosition(context types.Context) { + self.gui.postRefreshUpdate(context, true) } func (self *guiCommon) RunSubprocessAndRefresh(cmdObj *oscommands.CmdObj) error { diff --git a/pkg/gui/types/common.go b/pkg/gui/types/common.go index 75cb53018..ff73b91f6 100644 --- a/pkg/gui/types/common.go +++ b/pkg/gui/types/common.go @@ -48,8 +48,13 @@ type IGuiCommon interface { RefreshFromWorker(RefreshOptions) // we call this when we've changed something in the view model but not the actual model, // e.g. expanding or collapsing a folder in a file view. Calling 'Refresh' in this - // case would be overkill, although refresh will internally call 'PostRefreshUpdate' + // case would be overkill, although refresh will internally call 'PostRefreshUpdate'. + // It re-focuses the context's selection, which scrolls it into view. PostRefreshUpdate(Context) + // Like PostRefreshUpdate, but leaves the view scrolled where it is. For + // refreshes that no user action is behind: those must not move the viewport + // away from wherever the user last put it. + PostRefreshUpdateKeepingScrollPosition(Context) // renders string to a view without resetting its origin SetViewContent(view *gocui.View, content string) diff --git a/pkg/gui/types/context.go b/pkg/gui/types/context.go index 416b39b95..35662d86c 100644 --- a/pkg/gui/types/context.go +++ b/pkg/gui/types/context.go @@ -227,9 +227,13 @@ type IViewTrait interface { } type OnFocusOpts struct { - ClickedWindowName string - ClickedViewLineIdx int - ScrollSelectionIntoView bool + ClickedWindowName string + ClickedViewLineIdx int + + // Focusing a list context scrolls its selection into view. Set this to leave + // the view's scroll position alone instead; only for callers that maintain + // it themselves, e.g. by keeping the selection at the edge of the viewport. + KeepScrollPosition bool } type OnFocusLostOpts struct { diff --git a/pkg/gui/view_helpers.go b/pkg/gui/view_helpers.go index d139984fa..e9ad48aab 100644 --- a/pkg/gui/view_helpers.go +++ b/pkg/gui/view_helpers.go @@ -132,7 +132,7 @@ func (gui *Gui) renderContentOnly() { // postRefreshUpdate is to be called on a context after the state that it depends on has been refreshed // if the context's view is set to another context we do nothing. // if the context's view is the current view we trigger a focus; re-selecting the current item. -func (gui *Gui) postRefreshUpdate(c types.Context) { +func (gui *Gui) postRefreshUpdate(c types.Context, keepScrollPosition bool) { t := time.Now() defer func() { gui.Log.Infof("postRefreshUpdate for %s took %s", c.GetKey(), time.Since(t)) @@ -141,14 +141,14 @@ func (gui *Gui) postRefreshUpdate(c types.Context) { c.HandleRender() if gui.currentViewName() == c.GetViewName() { - c.HandleFocus(types.OnFocusOpts{}) + c.HandleFocus(types.OnFocusOpts{KeepScrollPosition: keepScrollPosition}) } else { // The FocusLine call is included in the HandleFocus method which we // call for focused views above; but we need to call it here for // non-focused views to ensure that an inactive selection is painted // correctly, and that integration tests see the up to date selection // state. - c.FocusLine(false) + c.FocusLine(!keepScrollPosition) currentCtx := gui.State.ContextMgr.Current() if currentCtx.GetKey() == context.NORMAL_MAIN_CONTEXT_KEY || currentCtx.GetKey() == context.NORMAL_SECONDARY_CONTEXT_KEY {