diff --git a/pkg/gui/main_panels.go b/pkg/gui/main_panels.go index 8feeaccf6..5613525d0 100644 --- a/pkg/gui/main_panels.go +++ b/pkg/gui/main_panels.go @@ -4,6 +4,7 @@ import ( "github.com/jesseduffield/lazygit/pkg/gocui" "github.com/jesseduffield/lazygit/pkg/gui/context" "github.com/jesseduffield/lazygit/pkg/gui/types" + "github.com/jesseduffield/lazygit/pkg/tasks" ) func (gui *Gui) runTaskForView(view *gocui.View, task types.UpdateTask) error { @@ -108,9 +109,14 @@ func (gui *Gui) allMainContextPairs() []types.MainContextPair { } func (gui *Gui) refreshMainViews(opts types.RefreshMainOpts) { + panes := mainPanesFor(opts) + + // Before the render is triggered, so that the pane the focus moves into can be + // told where to put its selection as it renders. + gui.followFocusIntoShownPane(opts.Pair, panes) + gui.moveMainContextPairToTop(opts.Pair) - panes := mainPanesFor(opts) gui.handOverMainSection(opts.Pair, panes) if opts.Main != nil { @@ -185,6 +191,70 @@ func mainPanesFor(opts types.RefreshMainOpts) types.MainPanes { } } +// followFocusIntoShownPane moves the focus out of a main pane that the render about to +// happen leaves nothing in, and into the one it does. +// +// Each side of a file's diff has a pane of its own, and a pane is only shown while its +// side has something in it. So anything that empties the side the focus is on takes +// that pane away with it: staging the last unstaged change, committing what was +// staged, or either of those happening outside lazygit and arriving with a refresh. +// Left where it was, the focus would be on a pane that isn't there, and the next +// keypress would act on nothing. +// +// The pane moved into gets its selection once the render has finished and there is +// something to put one on, and shows none until then, so that the selection it was +// left with the last time it was used doesn't appear for a frame. A pane that has +// already been told where to put its selection — by the action that caused all this — +// keeps what it was told. +func (gui *Gui) followFocusIntoShownPane(pair types.MainContextPair, panes types.MainPanes) { + // The focused main view's two panes only: the staging and patch-building views + // arrange theirs for themselves, and the merge-conflicts view has just the one. + if pair.Main.GetKey() != context.NORMAL_MAIN_CONTEXT_KEY { + return + } + + current := gui.State.ContextMgr.CurrentStatic().GetKey() + if current != pair.Main.GetKey() && current != pair.Secondary.GetKey() { + return + } + shown := onlyShownPane(pair, panes) + if shown == nil || shown.GetKey() == current { + return + } + + target := gui.mainContextForView(shown.GetView()) + target.SetHasSelectableContent(false) + gui.State.ContextMgr.UpdateSelectionHighlights() + if manager := gui.getManager(target.GetView()); !manager.HasRestoreForNextTask() { + manager.SetRestoreForNextTask(&tasks.RenderRestore{ + // The whole render is read before it is shown: where the selection goes + // is decided from what is there, and a change line further down would + // otherwise be missed. + FirstPaintReady: func() bool { return false }, + Apply: func(swapIn func()) bool { + swapIn() + gui.helpers.DiffLine.EstablishSelection(target, -1) + return false + }, + }) + } + gui.State.ContextMgr.Push(target, types.OnFocusOpts{}) +} + +// onlyShownPane returns the main pane a render leaves showing on its own, or nil when +// it leaves both showing. +func onlyShownPane(pair types.MainContextPair, panes types.MainPanes) types.Context { + switch panes { + case types.MainPaneOnly: + return pair.Main + case types.SecondaryPaneOnly: + return pair.Secondary + case types.BothMainPanes: + return nil + } + return nil +} + // clampDiffSelectionToContent brings the focused main view's selection back onto the // content when the render that just finished left the diff with fewer lines than the // selection was on — a diff renderer that renders the same diff more compactly, a diff --git a/pkg/integration/tests/main_view/focus_follows_a_pane_emptied_from_outside.go b/pkg/integration/tests/main_view/focus_follows_a_pane_emptied_from_outside.go new file mode 100644 index 000000000..708e0d2f0 --- /dev/null +++ b/pkg/integration/tests/main_view/focus_follows_a_pane_emptied_from_outside.go @@ -0,0 +1,52 @@ +package main_view + +import ( + "github.com/jesseduffield/lazygit/pkg/config" + . "github.com/jesseduffield/lazygit/pkg/integration/components" +) + +var FocusFollowsAPaneEmptiedFromOutside = NewIntegrationTest(NewIntegrationTestArgs{ + Description: "Committing the staged changes outside lazygit takes the staged pane away, so the focus follows into the one that is left", + ExtraCmdArgs: []string{}, + Skip: false, + SetupConfig: func(cfg *config.AppConfig) { + cfg.GetUserConfig().Gui.UseHunkModeInStagingView = false + }, + SetupRepo: func(shell *Shell) { + shell.CreateFileAndAdd("file1", "one\ntwo\nthree\nfour\nfive\n") + shell.Commit("one") + + // One change on each side, so the diff is split. + shell.UpdateFileAndAdd("file1", "one\nSTAGED\ntwo\nthree\nfour\nfive\n") + shell.UpdateFile("file1", "one\nSTAGED\ntwo\nthree\nUNSTAGED\nfour\nfive\n") + }, + Run: func(t *TestDriver, keys config.KeybindingConfig) { + t.Views().Files(). + IsFocused(). + Press(keys.Universal.FocusMainView) + + t.Views().Main(). + IsFocused(). + Press(keys.Universal.TogglePanel) + + t.Views().Secondary(). + IsFocused(). + Title(Equals("Staged changes")). + SelectedLines( + Contains("+STAGED"), + ) + + // Nothing lazygit did empties the staged side here; the refresh simply finds + // it empty, and the pane the focus was in is gone by the time it lands. + t.Shell().Commit("two") + t.GlobalPress(keys.Universal.Refresh) + + t.Views().Secondary().IsInvisible() + t.Views().Main(). + IsFocused(). + Title(Equals("Unstaged changes")). + SelectedLines( + Contains("+UNSTAGED"), + ) + }, +}) diff --git a/pkg/integration/tests/main_view/focus_follows_into_a_pane_taking_over.go b/pkg/integration/tests/main_view/focus_follows_into_a_pane_taking_over.go new file mode 100644 index 000000000..0f2a9af0d --- /dev/null +++ b/pkg/integration/tests/main_view/focus_follows_into_a_pane_taking_over.go @@ -0,0 +1,74 @@ +package main_view + +import ( + "fmt" + "strings" + + "github.com/jesseduffield/lazygit/pkg/config" + . "github.com/jesseduffield/lazygit/pkg/integration/components" +) + +var FocusFollowsIntoAPaneTakingOver = NewIntegrationTest(NewIntegrationTestArgs{ + Description: "The pane the focus follows into as it takes the section over gets its selection from the top of the diff it is given", + ExtraCmdArgs: []string{}, + Skip: false, + Width: 120, + Height: 30, + SetupConfig: func(cfg *config.AppConfig) { + cfg.GetUserConfig().Gui.UseHunkModeInStagingView = false + }, + SetupRepo: func(shell *Shell) { + lines := make([]string, 40) + for i := range lines { + lines[i] = fmt.Sprintf("line%02d", i+1) + } + shell.CreateFileAndAdd("file1", strings.Join(lines, "\n")+"\n") + shell.Commit("one") + + // Everything staged and nothing unstaged, so the staged side has the section to + // itself, with more changes in it than fit. + for i := range lines { + lines[i] = strings.ToUpper(lines[i]) + } + shell.UpdateFileAndAdd("file1", strings.Join(lines, "\n")+"\n") + }, + Run: func(t *TestDriver, keys config.KeybindingConfig) { + t.Views().Files(). + IsFocused(). + Lines( + Contains("M file1").IsSelected(), + ) + + // Read a way into the diff before focusing it, so that the selection starts out + // somewhere other than the first change. + t.Views().Secondary(). + IsVisible(). + ScrollWheelDown(). + ScrollWheelDown(). + ScrollWheelDown(). + ScrollWheelDown(). + OriginY(8) + + t.Views().Files().Press(keys.Universal.FocusMainView) + t.Views().Secondary(). + IsFocused(). + SelectedLines( + Contains("-line04"), + ) + + // The index is reset outside lazygit, so the staged side empties and the + // unstaged side has everything: the pane the focus is in goes away and the other + // one takes the section over with a diff that is new to it. + t.Shell().RunCommand([]string{"git", "reset"}) + t.GlobalPress(keys.Universal.Refresh) + + t.Views().Secondary().IsInvisible() + t.Views().Main(). + IsFocused(). + Title(Equals("Unstaged changes")). + OriginY(0). + SelectedLines( + Contains("-line01"), + ) + }, +}) diff --git a/pkg/integration/tests/test_list.go b/pkg/integration/tests/test_list.go index cc6572f2e..fb58456b9 100644 --- a/pkg/integration/tests/test_list.go +++ b/pkg/integration/tests/test_list.go @@ -375,6 +375,8 @@ var tests = []*components.IntegrationTest{ main_view.EditSelectedDiffLine, main_view.EscapeDismissesSelection, main_view.FileNavigationScrollsToTheTop, + main_view.FocusFollowsAPaneEmptiedFromOutside, + main_view.FocusFollowsIntoAPaneTakingOver, main_view.HideSelectionWhenChangesVanish, main_view.KeepAWrappedLineCoveredAcrossARerender, main_view.KeepBothHalvesOfAChangeSelected, diff --git a/pkg/tasks/tasks.go b/pkg/tasks/tasks.go index cbabc868f..2282906b4 100644 --- a/pkg/tasks/tasks.go +++ b/pkg/tasks/tasks.go @@ -207,6 +207,16 @@ func (self *ViewBufferManager) SetRestoreForNextTask(restore *RenderRestore) { self.restoreForNextTask = restore } +// HasRestoreForNextTask reports whether the next command task already has a position +// waiting to be put back, for a caller that would otherwise install one of its own +// over it. +func (self *ViewBufferManager) HasRestoreForNextTask() bool { + self.taskIDMutex.Lock() + defer self.taskIDMutex.Unlock() + + return self.restoreForNextTask != nil +} + func (self *ViewBufferManager) getRestoreForNextTask() *RenderRestore { self.taskIDMutex.Lock() defer self.taskIDMutex.Unlock() @@ -543,22 +553,22 @@ func (self *ViewBufferManager) NewCmdTask(start func() (Cmd, io.Reader), prefix return } painted = true - if restore != nil { - // The restore does the swap itself, so that it can find where the - // user was in the new content before it is revealed. - placed := restore.Apply(self.swapInRender) - self.clearRestore(restore) - if placed { - // The view is where the user left it, which is exactly what the - // scroll reset would undo. - self.newContentPending.Store(false) - return - } - } - self.swapInRender() + // Content the view hasn't seen is shown from the top, and this is where + // the view goes there — before the restore below, which decides where to + // put the view from where it is. The position the paint settles on is the + // restore's to move from, so it has to be the one the new content is + // about to be revealed at. if self.newContentPending.Swap(false) { self.resetOrigin() } + if restore != nil { + // The restore does the swap itself, so that it can find where the + // user was in the new content before it is revealed. + restore.Apply(self.swapInRender) + self.clearRestore(restore) + return + } + self.swapInRender() } // Set LAZYGIT_SLOW_RENDER= to sleep that long after each diff --git a/pkg/tasks/tasks_test.go b/pkg/tasks/tasks_test.go index 31c9a09d3..187b9aed1 100644 --- a/pkg/tasks/tasks_test.go +++ b/pkg/tasks/tasks_test.go @@ -388,8 +388,8 @@ func TestLoadingIndicatorOnlyTakesOverForNewContent(t *testing.T) { // A pending restore takes the first paint over: it says when enough of the new // content has arrived to show the position it remembers, and does the swap itself so // that it can look for that position while the previous content is still displayed. -// Having put the view where the user left it, it also keeps the scroll reset that new -// content would otherwise get. +// The scroll reset that new content is owed happens before it runs, so that where it +// puts the view is where the view stays. func TestNewCmdTaskRestore(t *testing.T) { writer := bytes.NewBuffer(nil) linesWritten := func() int { return strings.Count(writer.String(), "\n") } @@ -400,6 +400,7 @@ func TestNewCmdTaskRestore(t *testing.T) { applyAtLines := -1 swappedBeforeApply := false swappedByApply := false + resetsBeforeApply := -1 manager := NewViewBufferManager( utils.NewDummyLog(), @@ -422,6 +423,7 @@ func TestNewCmdTaskRestore(t *testing.T) { applyCount++ applyAtLines = linesWritten() swappedBeforeApply = swappedBeforeApply || swapped + resetsBeforeApply = getResetOriginCallCount() swapIn() swappedByApply = swapped return true @@ -443,7 +445,8 @@ func TestNewCmdTaskRestore(t *testing.T) { // fill the view. assert.GreaterOrEqual(t, applyAtLines, 5) assert.Less(t, applyAtLines, 30) - assert.Equal(t, 0, getResetOriginCallCount(), "a restore that placed the view leaves the scroll alone") + assert.Equal(t, 1, resetsBeforeApply, "new content should be put at the top before the restore places it") + assert.Equal(t, 1, getResetOriginCallCount(), "and not reset again afterwards, over the restore") } // A restore that never finds what it is looking for keeps the task reading to the