Scroll the selection into view by default again

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)
 

pkg/gui/controllers/helpers/fixup_helper.go

@@ -141,7 +141,6 @@ func (self *FixupHelper) HandleFindBaseCommitForFixupPress() error {
 			}
 
 			self.c.Contexts().LocalCommits.SetSelection(index)
-			self.c.Contexts().LocalCommits.FocusLine(true)
 			self.c.Context().Push(self.c.Contexts().LocalCommits, types.OnFocusOpts{})
 			return nil
 		},

pkg/gui/controllers/helpers/mode_helper.go

@@ -270,11 +270,6 @@ func (self *ModeHelper) changeFiltering(setFilter func(), selectCommit func()) e
 
 				selectCommit()
 				self.c.PostRefreshUpdate(self.c.Contexts().LocalCommits)
-				// The list we just selected in has nothing to do with the one
-				// that was showing, so wherever it was scrolled to says nothing
-				// about where the selection now is. PostRefreshUpdate leaves the
-				// scroll position alone, so ask for it separately.
-				self.c.Contexts().LocalCommits.FocusLine(true)
 				return nil
 			},
 		})

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()
@@ -842,33 +851,21 @@ func (self *RefreshHelper) refreshCommitsWithLimit(captured capturedCommitState,
 			self.c.Model().CheckedOutBranch = ""
 		}
 
-		scrollSelectionIntoView := false
 		switch commitSelection {
 		case types.SelectHeadCommit:
 			if headCommitIdx := models.HeadCommitIdx(commits); headCommitIdx >= 0 {
 				self.c.Contexts().LocalCommits.SetSelection(headCommitIdx)
-				scrollSelectionIntoView = true
 			}
 		case types.KeepCommitSelectionByHash:
 			if selectionRange != nil {
-				selectedIdx, rangeStartIdx, didMove, found := findLocalCommitSelectionRange(commits, selectionRange)
+				selectedIdx, rangeStartIdx, found := findLocalCommitSelectionRange(commits, selectionRange)
 				if found {
 					self.c.Contexts().LocalCommits.SetSelectionRangeAndMode(selectedIdx, rangeStartIdx, selectionRange.mode)
-					scrollSelectionIntoView = didMove
 				}
 			}
 		case types.KeepCommitSelectionIndex:
 			// The caller set the selection index deliberately; leave it untouched.
 		}
-
-		if scrollSelectionIntoView {
-			// Enqueued from within this bounce so it runs after refreshView's
-			// render below (which was enqueued first), matching the previous
-			// ordering where FocusLine ran after the view was re-rendered.
-			self.onUIThreadUnlessRepoChanged(env, func() {
-				self.c.Contexts().LocalCommits.FocusLine(true)
-			})
-		}
 	})
 
 	self.refreshView(self.c.Contexts().LocalCommits, env)
@@ -880,8 +877,6 @@ type localCommitSelectionRange struct {
 	selectedIsTODO   bool
 	rangeStartHash   string
 	rangeStartIsTODO bool
-	selectedIdx      int
-	rangeStartIdx    int
 	mode             traits.RangeSelectMode
 }
 
@@ -900,26 +895,23 @@ func captureLocalCommitSelectionRange(
 		selectedIsTODO:   commits[selectedIdx].IsTODO(),
 		rangeStartHash:   commits[rangeStartIdx].Hash(),
 		rangeStartIsTODO: commits[rangeStartIdx].IsTODO(),
-		selectedIdx:      selectedIdx,
-		rangeStartIdx:    rangeStartIdx,
 		mode:             mode,
 	}
 }
 
 func findLocalCommitSelectionRange(
 	commits []*models.Commit,
 	selectionRange *localCommitSelectionRange,
-) (int, int, bool, bool) {
+) (int, int, bool) {
 	selectedIdx, foundSelected := findCommitByHashPreferringTODOStatus(
 		commits, selectionRange.selectedHash, selectionRange.selectedIsTODO)
 	rangeStartIdx, foundRangeStart := findCommitByHashPreferringTODOStatus(
 		commits, selectionRange.rangeStartHash, selectionRange.rangeStartIsTODO)
 	if !foundSelected || !foundRangeStart {
-		return 0, 0, false, false
+		return 0, 0, false
 	}
 
-	didMove := selectedIdx != selectionRange.selectedIdx || rangeStartIdx != selectionRange.rangeStartIdx
-	return selectedIdx, rangeStartIdx, didMove, true
+	return selectedIdx, rangeStartIdx, true
 }
 
 // findCommitByHashPreferringTODOStatus finds the commit with the given hash.
@@ -1170,10 +1162,8 @@ func (self *RefreshHelper) refreshBranches(captured capturedBranchState, refresh
 				}
 			}
 		case types.SelectCheckedOutBranch:
-			// The checked-out branch is always at the top of the list. Setting
-			// the selection doesn't scroll the view, so also reset the origin.
+			// The checked-out branch is always at the top of the list.
 			self.c.Contexts().Branches.SetSelectedLineIdx(0)
-			self.c.Contexts().Branches.GetView().SetOriginY(0)
 		}
 
 		// Need to re-render the commits view because the visualization of local
@@ -1464,11 +1454,9 @@ func (self *RefreshHelper) refreshReflogCommits(captured capturedReflogState, en
 		self.c.Model().ReflogCommits = reflogCommits
 		self.c.Model().FilteredReflogCommits = filteredReflogCommits
 		// Setting the selection here, in the same bounce that writes the list,
-		// keeps it on the UI thread and atomic with the list update. Setting the
-		// selection doesn't scroll the view, so also reset the origin.
+		// keeps it on the UI thread and atomic with the list update.
 		if selectTopEntry {
 			self.c.Contexts().ReflogCommits.SetSelectedLineIdx(0)
-			self.c.Contexts().ReflogCommits.GetView().SetOriginY(0)
 		}
 	})
 
@@ -1595,7 +1583,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 +1763,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)
 	})
 }
 

pkg/gui/controllers/helpers/refresh_helper_test.go

@@ -28,8 +28,6 @@ func TestCaptureLocalCommitSelectionRange(t *testing.T) {
 			expected: &localCommitSelectionRange{
 				selectedHash:   "b",
 				rangeStartHash: "a",
-				selectedIdx:    1,
-				rangeStartIdx:  0,
 				mode:           traits.RangeSelectModeSticky,
 			},
 		},
@@ -74,15 +72,12 @@ func TestFindLocalCommitSelectionRange(t *testing.T) {
 	type expectation struct {
 		selectedIdx   int
 		rangeStartIdx int
-		moved         bool
 		found         bool
 	}
 
 	selectionRange := localCommitSelectionRange{
 		selectedHash:   "b",
 		rangeStartHash: "c",
-		selectedIdx:    1,
-		rangeStartIdx:  2,
 		mode:           traits.RangeSelectModeSticky,
 	}
 
@@ -97,7 +92,6 @@ func TestFindLocalCommitSelectionRange(t *testing.T) {
 			expected: expectation{
 				selectedIdx:   2,
 				rangeStartIdx: 3,
-				moved:         true,
 				found:         true,
 			},
 		},
@@ -126,7 +120,6 @@ func TestFindLocalCommitSelectionRange(t *testing.T) {
 			expected: expectation{
 				selectedIdx:   2,
 				rangeStartIdx: 3,
-				moved:         true,
 				found:         true,
 			},
 		},
@@ -139,19 +132,17 @@ func TestFindLocalCommitSelectionRange(t *testing.T) {
 			expected: expectation{
 				selectedIdx:   0,
 				rangeStartIdx: 1,
-				moved:         true,
 				found:         true,
 			},
 		},
 	}
 
 	for _, testCase := range testCases {
 		t.Run(testCase.name, func(t *testing.T) {
-			selectedIdx, rangeStartIdx, moved, found := findLocalCommitSelectionRange(testCase.commits, &selectionRange)
+			selectedIdx, rangeStartIdx, found := findLocalCommitSelectionRange(testCase.commits, &selectionRange)
 			actual := expectation{
 				selectedIdx:   selectedIdx,
 				rangeStartIdx: rangeStartIdx,
-				moved:         moved,
 				found:         found,
 			}
 

pkg/gui/controllers/helpers/search_helper.go

@@ -225,7 +225,6 @@ func (self *SearchHelper) OnPromptContentChanged(searchString string) {
 	switch context := state.Context.(type) {
 	case types.IFilterableContext:
 		context.SetSelection(0)
-		context.GetView().SetOriginY(0)
 		context.SetFilter(searchString, self.c.UserConfig().Gui.UseFuzzySearch())
 		self.c.PostRefreshUpdate(context)
 	case types.ISearchableContext:
@@ -241,6 +240,9 @@ func (self *SearchHelper) ReApplyFilter(context types.Context) {
 		state := self.searchState()
 		if context == state.Context && self.c.Context().Current().GetKey() == self.c.Contexts().Search.GetKey() {
 			filterableContext.SetSelection(0)
+			// This runs as part of a refresh, and a refresh that no user action
+			// is behind keeps the scroll position, which would leave the view
+			// scrolled somewhere the filtered list no longer has anything at.
 			filterableContext.GetView().SetOriginY(0)
 		}
 		filterableContext.ReApplyFilter(self.c.UserConfig().Gui.UseFuzzySearch())

pkg/gui/controllers/helpers/sub_commits_helper.go

@@ -66,7 +66,6 @@ func (self *SubCommitsHelper) ViewSubCommits(opts ViewSubCommitsOpts) error {
 	subCommitsContext.GetView().TitlePrefix = opts.Context.GetView().TitlePrefix
 
 	self.c.PostRefreshUpdate(self.c.Contexts().SubCommits)
-	subCommitsContext.FocusLine(true)
 
 	self.c.Context().Push(self.c.Contexts().SubCommits, types.OnFocusOpts{})
 	return nil

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 {

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()

pkg/gui/controllers/stash_controller.go

@@ -251,7 +251,6 @@ func (self *StashController) handleRenameStashEntry(stashEntry *models.StashEntr
 				return err
 			}
 			self.context().SetSelection(0) // Select the renamed stash
-			self.context().FocusLine(true)
 			// Renaming re-creates the stash at the top, shifting the other
 			// entries' indices; block input so that a quick next action sees
 			// the refreshed list rather than the stale indices.

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 {

pkg/gui/gui_driver.go

@@ -69,6 +69,10 @@ func (self *GuiDriver) MouseMove(x, y int) {
 	self.replayMouseEvent(x, y, tcell.ButtonPrimary)
 }
 
+func (self *GuiDriver) ScrollWheelDown(x, y int) {
+	self.replayMouseEvent(x, y, tcell.WheelDown)
+}
+
 func (self *GuiDriver) MouseRelease(x, y int) {
 	self.replayMouseEvent(x, y, tcell.ButtonNone)
 }
@@ -128,6 +132,16 @@ func (self *GuiDriver) FocusInAndClick(x, y int) {
 	self.waitTillIdle()
 }
 
+// RefreshInBackground performs the refresh that the background routines perform
+// on a timer (see BackgroundRoutineMgr). Tests drive it directly rather than
+// turning those routines on, so that they neither wait for a timer nor depend on
+// one firing at a particular moment.
+func (self *GuiDriver) RefreshInBackground() {
+	self.gui.c.RefreshFromWorker(types.RefreshOptions{Background: true})
+
+	self.waitTillIdle()
+}
+
 func (self *GuiDriver) PretendMergeOrRebaseStartedInLazygit() {
 	self.gui.onUIThread(func() error {
 		self.gui.State.SetMergeOrRebaseStartedInLazygit(true)

pkg/gui/menu_panel.go

@@ -72,8 +72,6 @@ func (gui *Gui) createMenu(opts types.CreateMenuOptions) error {
 	gui.State.Contexts.Menu.SetOnCancel(opts.OnCancel)
 	gui.State.Contexts.Menu.SetSelection(0)
 
-	gui.Views.Menu.SetOriginY(0)
-
 	gui.Views.Menu.Title = opts.Title
 	gui.Views.Menu.FgColor = theme.GocuiDefaultTextColor
 

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)