diff --git a/pkg/gui/controllers/helpers/diff_line_selection.go b/pkg/gui/controllers/helpers/diff_line_selection.go index 4d6b26b51..ff9a9d2c9 100644 --- a/pkg/gui/controllers/helpers/diff_line_selection.go +++ b/pkg/gui/controllers/helpers/diff_line_selection.go @@ -65,8 +65,9 @@ func (self *DiffLineHelper) EstablishSelection(mainContext *context.MainContext, // PlaceNavigationTarget moves the pane's selection to the row a jump found, bringing // it on screen if it isn't already. With no selection to move — a pane that isn't -// focused, or one showing a diff with nothing selectable in it — the row goes to the -// top of the view instead, that being all a jump can do there. +// focused, or one showing a diff with nothing selectable in it — all a jump can do is +// scroll the row to the top of the view, or as close to the top as the end of the diff +// allows. // // alignTop asks for the target to become the view's top line, so that everything that // begins there is on screen. It only applies to a target the view has to scroll to: a @@ -84,7 +85,7 @@ func (self *DiffLineHelper) PlaceNavigationTarget( ) { view := pane.GetView() if !view.Highlight { - view.SetOrigin(0, target) + view.SetOrigin(0, originYWithTargetAtTop(pane, target)) return } if alignTop { @@ -106,16 +107,21 @@ func (self *DiffLineHelper) PlaceNavigationTarget( } // scrollTargetToTop scrolls the given row of the diff to the top of the view, leaving -// the view where it is when that row is on screen already. The last screenful of the -// diff is as far as it goes, so that the view doesn't scroll past the end of what it is -// showing. +// the view where it is when that row is on screen already. func (self *DiffLineHelper) scrollTargetToTop(pane types.DiffPaneContext, target int) { - view := pane.GetView() originY, height := pane.GetViewTrait().ViewPortYBounds() if target >= originY && target < originY+height { return } - view.SetOriginY(min(target, max(0, view.ViewLinesHeight()-height))) + pane.GetView().SetOriginY(originYWithTargetAtTop(pane, target)) +} + +// originYWithTargetAtTop returns the origin that puts the given row of the diff at the +// top of the view. The last screenful of the diff is as far as it goes, so that the +// view doesn't scroll past the end of what it is showing. +func originYWithTargetAtTop(pane types.DiffPaneContext, target int) int { + _, height := pane.GetViewTrait().ViewPortYBounds() + return min(target, max(0, pane.GetView().ViewLinesHeight()-height)) } // CollapseNonStickyRange drops a range that only grows while shift is held back to a diff --git a/pkg/gui/view_helpers.go b/pkg/gui/view_helpers.go index 9d70b6e4b..ff1cb1073 100644 --- a/pkg/gui/view_helpers.go +++ b/pkg/gui/view_helpers.go @@ -18,12 +18,14 @@ func (gui *Gui) resetViewOrigin(v *gocui.View) { // Returns the number of lines that we should read initially from a cmd task so // that the scrollbar has the correct size, along with the number of lines after -// which the view is filled and we can do a first refresh. +// which the view is filled and we can do a first refresh. Both count from the top +// of the view. The task adds the lines above the view if it shows its content +// where the view is (see LinesToRead.LinesAboveView). func (gui *Gui) linesToReadFromCmdTask(v *gocui.View) tasks.LinesToRead { height := v.InnerHeight() - oy := v.OriginY() + linesAboveView := v.OriginY() - linesForFirstRefresh := height + oy + 10 + linesForFirstRefresh := height + 10 // A search counts the matches in everything the view holds, so a re-render of a // view that is being searched is read all the way to the end (as opening the @@ -34,6 +36,7 @@ func (gui *Gui) linesToReadFromCmdTask(v *gocui.View) tasks.LinesToRead { return tasks.LinesToRead{ Total: -1, InitialRefreshAfter: linesForFirstRefresh, + LinesAboveView: linesAboveView, } } @@ -44,11 +47,14 @@ func (gui *Gui) linesToReadFromCmdTask(v *gocui.View) tasks.LinesToRead { linesToReadForAccurateScrollbar := min( // However, cap it at some arbitrary max limit, so that we don't get // performance problems for huge monitors or tiny font sizes - height*(height-1)/minScrollbarHeight+oy, 5000) + height*(height-1)/minScrollbarHeight, 5000) return tasks.LinesToRead{ - Total: linesToReadForAccurateScrollbar, + // A view no more than a few lines tall needs fewer lines for its scrollbar + // than it takes to fill it, and the task must read at least those. + Total: max(linesToReadForAccurateScrollbar, linesForFirstRefresh), InitialRefreshAfter: linesForFirstRefresh, + LinesAboveView: linesAboveView, } } diff --git a/pkg/integration/tests/main_view/click_a_file_in_a_diff_stat_that_comes_first.go b/pkg/integration/tests/main_view/click_a_file_in_a_diff_stat_that_comes_first.go index 2d459e21d..1f1f71fa1 100644 --- a/pkg/integration/tests/main_view/click_a_file_in_a_diff_stat_that_comes_first.go +++ b/pkg/integration/tests/main_view/click_a_file_in_a_diff_stat_that_comes_first.go @@ -1,6 +1,8 @@ package main_view import ( + "strings" + "github.com/jesseduffield/lazygit/pkg/config" . "github.com/jesseduffield/lazygit/pkg/integration/components" ) @@ -23,7 +25,9 @@ var ClickAFileInADiffStatThatComesFirst = NewIntegrationTest(NewIntegrationTestA SetupRepo: func(shell *Shell) { shell.CreateFileAndAdd("aaa.txt", "one\n") shell.Commit("one") - shell.CreateFileAndAdd("zzz.txt", "one\n") + // Longer than the view, so that the diff can be scrolled to put aaa.txt at the + // top. + shell.CreateFileAndAdd("zzz.txt", strings.Repeat("line\n", 40)) shell.Commit("two") }, Run: func(t *TestDriver, keys config.KeybindingConfig) { diff --git a/pkg/integration/tests/main_view/jump_to_the_last_file_of_a_long_diff.go b/pkg/integration/tests/main_view/jump_to_the_last_file_of_a_long_diff.go new file mode 100644 index 000000000..e2d3c0745 --- /dev/null +++ b/pkg/integration/tests/main_view/jump_to_the_last_file_of_a_long_diff.go @@ -0,0 +1,50 @@ +package main_view + +import ( + "fmt" + "strings" + + "github.com/jesseduffield/lazygit/pkg/config" + . "github.com/jesseduffield/lazygit/pkg/integration/components" +) + +var JumpToTheLastFileOfALongDiff = NewIntegrationTest(NewIntegrationTestArgs{ + Description: "Jumping to the last file of a long diff from the commits panel doesn't scroll past the end of the diff", + ExtraCmdArgs: []string{}, + Skip: false, + Width: 100, + Height: 20, + SetupConfig: func(config *config.AppConfig) { + config.GetUserConfig().Gui.ScrollPastBottom = false + }, + SetupRepo: func(shell *Shell) { + // Longer than the 5000 lines that a render reads at most before it waits for + // the user to scroll further. + lines := make([]string, 6000) + for i := range lines { + lines[i] = fmt.Sprintf("line%04d", i+1) + } + shell.CreateFileAndAdd("long.txt", strings.Join(lines, "\n")+"\n") + shell.CreateFileAndAdd("short.txt", "one\n") + shell.Commit("first commit") + }, + Run: func(t *TestDriver, keys config.KeybindingConfig) { + t.Views().Commits(). + Focus(). + SelectedLine(Contains("first commit")). + Press(keys.Universal.JumpToFile) + + t.ExpectPopup().Menu(). + Title(Equals("Jump to file in diff")). + Select(Equals("short.txt")). + Confirm() + + t.Views().Commits(). + IsFocused() + + // The view shows the last screenful of the diff: the end of long.txt's diff, + // and all of short.txt's below it. + t.Views().Main(). + TopVisibleLine(Equals("+line5994")) + }, +}) diff --git a/pkg/integration/tests/main_view/show_another_long_diff_after_scrolling_far_down.go b/pkg/integration/tests/main_view/show_another_long_diff_after_scrolling_far_down.go new file mode 100644 index 000000000..2f0580663 --- /dev/null +++ b/pkg/integration/tests/main_view/show_another_long_diff_after_scrolling_far_down.go @@ -0,0 +1,52 @@ +package main_view + +import ( + "fmt" + "strings" + + "github.com/jesseduffield/lazygit/pkg/config" + . "github.com/jesseduffield/lazygit/pkg/integration/components" +) + +var ShowAnotherLongDiffAfterScrollingFarDown = NewIntegrationTest(NewIntegrationTestArgs{ + Description: "Selecting another commit shows its diff when the main view is scrolled far down a long diff", + ExtraCmdArgs: []string{}, + Skip: false, + Width: 100, + Height: 20, + SetupConfig: func(config *config.AppConfig) {}, + SetupRepo: func(shell *Shell) { + // Longer than the 5000 lines that a render reads at most before it waits for + // the user to scroll further. + lines := make([]string, 6000) + for i := range lines { + lines[i] = fmt.Sprintf("line%04d", i+1) + } + long := strings.Join(lines, "\n") + "\n" + + shell.CreateFileAndAdd("first.txt", long) + shell.Commit("first commit") + shell.CreateFileAndAdd("second.txt", long) + shell.Commit("second commit") + }, + Run: func(t *TestDriver, keys config.KeybindingConfig) { + t.Views().Commits(). + Focus(). + SelectedLine(Contains("second commit")). + Press(keys.Universal.FocusMainView) + + t.Views().Main(). + IsFocused(). + Press(keys.Universal.GotoBottom). + SelectedLines(Contains("+line6000")). + Press(keys.Universal.Return) + + t.Views().Commits(). + IsFocused(). + SelectNextItem(). + SelectedLine(Contains("first commit")) + + t.Views().Main(). + Content(Contains("diff --git a/first.txt b/first.txt")) + }, +}) diff --git a/pkg/integration/tests/test_list.go b/pkg/integration/tests/test_list.go index aea6c4f13..76b8b3357 100644 --- a/pkg/integration/tests/test_list.go +++ b/pkg/integration/tests/test_list.go @@ -414,6 +414,7 @@ var tests = []*components.IntegrationTest{ main_view.JumpToAFileOfTheDiff, main_view.JumpToAFileOnlyOverADiff, main_view.JumpToAnotherFileScrollsToTheLeftEdge, + main_view.JumpToTheLastFileOfALongDiff, main_view.KeepAWrappedLineCoveredAcrossARerender, main_view.KeepAnEmptiedPaneEmpty, main_view.KeepBothHalvesOfAChangeSelected, @@ -493,6 +494,7 @@ var tests = []*components.IntegrationTest{ main_view.SelectionCommandTooltipsFollowTheDiff, main_view.SelectionCommandsOnlyWhereTheyApply, main_view.SelectionOverTheCustomPatch, + main_view.ShowAnotherLongDiffAfterScrollingFarDown, main_view.ShowTheTabSwitchedToLast, main_view.StageDeletedFile, main_view.StageDiffLines, diff --git a/pkg/tasks/tasks.go b/pkg/tasks/tasks.go index 510fffa49..37f586222 100644 --- a/pkg/tasks/tasks.go +++ b/pkg/tasks/tasks.go @@ -167,12 +167,37 @@ type LinesToRead struct { // Number of lines after which we have read enough to fill the view, and can // do an initial refresh. Only set for the initial read request; -1 for // subsequent requests. + // + // Unless Total is -1, it must not be more than Total. The task shows nothing + // of what it reads until it has read this many lines or has reached the end + // of its input, so a task that stops reading before then leaves the view + // showing the previous render. InitialRefreshAfter int + // The number of lines above the view when the task was asked for. Only set + // for the initial read request, whose Total and InitialRefreshAfter don't + // count these lines. A task that shows its content where the view is reads + // them as well, so that it fills the view there. A task that shows content + // the view isn't showing yet shows it from the top and doesn't read them. + LinesAboveView int + // Function to call after reading the lines is done Then func() } +// includingLinesAboveView returns the initial read request for a task that shows its +// content where the view is rather than from the top. Such a task reads the lines +// above the view as well. +func (self LinesToRead) includingLinesAboveView() LinesToRead { + if self.Total != -1 { + self.Total += self.LinesAboveView + } + if self.InitialRefreshAfter != -1 { + self.InitialRefreshAfter += self.LinesAboveView + } + return self +} + // RenderRestore puts a view back where it was when it re-renders content the user // is already looking at, laid out differently — a different context size, whitespace // ignored, another diff renderer — instead of showing the new render from the top. @@ -447,6 +472,14 @@ func (self *ViewBufferManager) NewCmdTask(start func() (Cmd, io.Reader), prefix // remembered just before the re-render that led here was triggered. restore := self.getRestoreForNextTask() + // Content the view isn't showing yet is shown from the top (see + // newContentPending), and content the view is showing is shown where the + // view is. + initialRequest := linesToRead + if !self.newContentPending.Load() { + initialRequest = linesToRead.includingLinesAboveView() + } + if self.throttle.Load() { self.Log.Info("throttling task") time.Sleep(THROTTLE_TIME) @@ -799,7 +832,7 @@ func (self *ViewBufferManager) NewCmdTask(start func() (Cmd, io.Reader), prefix close(lineWrittenChan) }) - self.readRequests.enqueue(linesToRead) + self.readRequests.enqueue(initialRequest) <-done diff --git a/pkg/tasks/tasks_test.go b/pkg/tasks/tasks_test.go index a5b195a9c..12b1ce52d 100644 --- a/pkg/tasks/tasks_test.go +++ b/pkg/tasks/tasks_test.go @@ -60,7 +60,7 @@ func TestNewCmdTaskInstantStop(t *testing.T) { return ExecCmd{Cmd: cmd}, reader } - fn := manager.NewCmdTask(start, func() string { return "prefix\n" }, LinesToRead{20, -1, nil}, onDone) + fn := manager.NewCmdTask(start, func() string { return "prefix\n" }, LinesToRead{Total: 20, InitialRefreshAfter: -1}, onDone) _ = fn(TaskOpts{Stop: stop, InitialContentLoaded: func() { task.Done() }}) @@ -131,7 +131,7 @@ func TestNewCmdTask(t *testing.T) { return ExecCmd{Cmd: cmd}, reader } - fn := manager.NewCmdTask(start, func() string { return "prefix\n" }, LinesToRead{20, -1, nil}, onDone) + fn := manager.NewCmdTask(start, func() string { return "prefix\n" }, LinesToRead{Total: 20, InitialRefreshAfter: -1}, onDone) wg := sync.WaitGroup{} wg.Go(func() { time.Sleep(100 * time.Millisecond) @@ -196,7 +196,7 @@ func TestNewCmdTaskStoppedWhileProducingItsPrefix(t *testing.T) { } onDone, getOnDoneCallCount := getCounter() - fn := manager.NewCmdTask(start, prefix, LinesToRead{20, -1, nil}, onDone) + fn := manager.NewCmdTask(start, prefix, LinesToRead{Total: 20, InitialRefreshAfter: -1}, onDone) _ = fn(TaskOpts{Stop: stop, InitialContentLoaded: func() { task.Done() }}) assert.False(t, started) @@ -277,7 +277,7 @@ func TestNewCmdTaskQueuedReadAtEndOfInput(t *testing.T) { // The initial request asks for far more lines than the reader has, so the // task reaches EOF while that request is still the one being served. - fn := manager.NewCmdTask(start, nil, LinesToRead{100, -1, nil}, func() {}) + fn := manager.NewCmdTask(start, nil, LinesToRead{Total: 100, InitialRefreshAfter: -1}, func() {}) thenCalled := false wg := sync.WaitGroup{} @@ -326,7 +326,7 @@ func TestResetOriginSurvivesTaskReplacement(t *testing.T) { } // The first-paint point is far beyond what any of these readers yield, so // only reaching EOF paints. - _ = manager.NewTask(manager.NewCmdTask(start, nil, LinesToRead{100, 50, nil}, onDone), key) + _ = manager.NewTask(manager.NewCmdTask(start, nil, LinesToRead{Total: 100, InitialRefreshAfter: 50}, onDone), key) } runTaskToCompletion := func(key string) { done := make(chan struct{}) @@ -381,7 +381,7 @@ func TestLoadingIndicatorOnlyTakesOverForNewContent(t *testing.T) { // not actually starting this because it's not necessary return ExecCmd{Cmd: exec.Command("blah")}, reader } - _ = manager.NewTask(manager.NewCmdTask(start, nil, LinesToRead{100, 50, nil}, onDone), key) + _ = manager.NewTask(manager.NewCmdTask(start, nil, LinesToRead{Total: 100, InitialRefreshAfter: 50}, onDone), key) } // Starts a task whose command produces nothing at all, so that it is still // waiting for its first line when the loading indicator falls due. Returns @@ -467,7 +467,7 @@ func TestNewCmdTaskRestore(t *testing.T) { // not actually starting this because it's not necessary return ExecCmd{Cmd: exec.Command("blah")}, &BlankLineReader{totalLinesToYield: 50} } - _ = manager.NewTask(manager.NewCmdTask(start, nil, LinesToRead{100, 30, nil}, func() { close(done) }), "cmd") + _ = manager.NewTask(manager.NewCmdTask(start, nil, LinesToRead{Total: 100, InitialRefreshAfter: 30}, func() { close(done) }), "cmd") <-done assert.Equal(t, 1, applyCount, "Apply should run exactly once") @@ -520,7 +520,7 @@ func TestNewCmdTaskRestoreThatFindsNothing(t *testing.T) { // not actually starting this because it's not necessary return ExecCmd{Cmd: exec.Command("blah")}, &BlankLineReader{totalLinesToYield: 50} } - _ = manager.NewTask(manager.NewCmdTask(start, nil, LinesToRead{100, 30, nil}, func() { close(done) }), "cmd") + _ = manager.NewTask(manager.NewCmdTask(start, nil, LinesToRead{Total: 100, InitialRefreshAfter: 30}, func() { close(done) }), "cmd") <-done assert.Equal(t, 1, applyCount, "Apply should still run, to swap the render in") @@ -562,7 +562,7 @@ func TestRestoreSurvivesTaskReplacement(t *testing.T) { // not actually starting this because it's not necessary return ExecCmd{Cmd: exec.Command("blah")}, reader } - _ = manager.NewTask(manager.NewCmdTask(start, nil, LinesToRead{100, 50, nil}, onDone), "cmd") + _ = manager.NewTask(manager.NewCmdTask(start, nil, LinesToRead{Total: 100, InitialRefreshAfter: 50}, onDone), "cmd") } // The task the restore was installed for stalls before it can paint. @@ -609,7 +609,7 @@ func TestKeepScrollPositionForNextTask(t *testing.T) { // not actually starting this because it's not necessary return ExecCmd{Cmd: exec.Command("blah")}, reader } - _ = manager.NewTask(manager.NewCmdTask(start, nil, LinesToRead{100, 50, nil}, onDone), key) + _ = manager.NewTask(manager.NewCmdTask(start, nil, LinesToRead{Total: 100, InitialRefreshAfter: 50}, onDone), key) } runTaskToCompletion := func(key string) { done := make(chan struct{}) @@ -670,7 +670,7 @@ func TestForgetRenderedContent(t *testing.T) { } done := make(chan struct{}) _ = manager.NewTask( - manager.NewCmdTask(start, nil, LinesToRead{100, 50, nil}, func() { close(done) }), key) + manager.NewCmdTask(start, nil, LinesToRead{Total: 100, InitialRefreshAfter: 50}, func() { close(done) }), key) <-done } @@ -687,6 +687,62 @@ func TestForgetRenderedContent(t *testing.T) { assert.Equal(t, 2, getResetOriginCallCount(), "an emptied view is shown its content afresh") } +// A render of the content the view is showing keeps the view where it is, so it reads +// the lines above the view as well, both before it first paints and in all. A render +// of content the view isn't showing yet is shown from the top, so the lines above the +// view are no use to it. Reading them would only hold up the first paint of a diff +// selected while the view is scrolled far down another one. +func TestOnlyARenderInPlaceReadsTheLinesAboveTheView(t *testing.T) { + writer := bytes.NewBuffer(nil) + linesWritten := func() int { return strings.Count(writer.String(), "\n") } + linesWrittenAtFirstPaint := -1 + + manager := NewViewBufferManager( + utils.NewDummyLog(), + writer, + func() {}, // beforeStart + func() {}, // refreshView + func() {}, // onEndOfInput + func() {}, // resetOrigin + writer.Reset, + func() { linesWrittenAtFirstPaint = linesWritten() }, + func() gocui.Task { return gocui.NewFakeTask() }, + // no UI thread in the test; run the view mutations inline + func(f func()) error { f(); return nil }, + ) + defer manager.Close() + + // Renders the given command's output, which is longer than any of the reading + // asked for here, and returns once the task has read the lines it was asked for. + render := func(key string) { + start := func() (Cmd, io.Reader) { + // not actually starting this because it's not necessary + return ExecCmd{Cmd: exec.Command("blah")}, &BlankLineReader{totalLinesToYield: 1000} + } + served := make(chan struct{}) + linesToRead := LinesToRead{ + Total: 20, + InitialRefreshAfter: 15, + LinesAboveView: 100, + Then: func() { close(served) }, + } + _ = manager.NewTask(manager.NewCmdTask(start, nil, linesToRead, nil), key) + <-served + } + + render("cmd1") + assert.Equal(t, 15, linesWrittenAtFirstPaint) + assert.Equal(t, 20, linesWritten()) + + render("cmd1") + assert.Equal(t, 115, linesWrittenAtFirstPaint) + assert.Equal(t, 120, linesWritten()) + + render("cmd2") + assert.Equal(t, 15, linesWrittenAtFirstPaint) + assert.Equal(t, 20, linesWritten()) +} + func TestNewCmdTaskRefresh(t *testing.T) { type scenario struct { name string @@ -699,37 +755,37 @@ func TestNewCmdTaskRefresh(t *testing.T) { { "total < initialRefreshAfter", 150, - LinesToRead{100, 120, nil}, + LinesToRead{Total: 100, InitialRefreshAfter: 120}, []int{100}, }, { "total == initialRefreshAfter", 150, - LinesToRead{100, 100, nil}, + LinesToRead{Total: 100, InitialRefreshAfter: 100}, []int{100}, }, { "total > initialRefreshAfter", 150, - LinesToRead{100, 50, nil}, + LinesToRead{Total: 100, InitialRefreshAfter: 50}, []int{50, 100}, }, { "initialRefreshAfter == -1", 150, - LinesToRead{100, -1, nil}, + LinesToRead{Total: 100, InitialRefreshAfter: -1}, []int{100}, }, { "totalTaskLines < initialRefreshAfter", 25, - LinesToRead{100, 50, nil}, + LinesToRead{Total: 100, InitialRefreshAfter: 50}, []int{25}, }, { "totalTaskLines between total and initialRefreshAfter", 75, - LinesToRead{100, 50, nil}, + LinesToRead{Total: 100, InitialRefreshAfter: 50}, []int{50, 75}, }, } @@ -934,7 +990,7 @@ func TestMessageEndsTheLoadingOfTheTaskItReplaces(t *testing.T) { reservation := manager.ReserveTask() manager.StartLoading() - _ = manager.NewReservedTask(reservation, manager.NewCmdTask(start, nil, LinesToRead{100, 50, nil}, nil), "cmd") + _ = manager.NewReservedTask(reservation, manager.NewCmdTask(start, nil, LinesToRead{Total: 100, InitialRefreshAfter: 50}, nil), "cmd") <-stalled.blocked assert.True(t, manager.IsLoading()) @@ -973,7 +1029,7 @@ func TestEarlierTaskEndingLeavesALaterTaskLoading(t *testing.T) { reservation := manager.ReserveTask() manager.StartLoading() _ = manager.NewReservedTask(reservation, - manager.NewCmdTask(start, nil, LinesToRead{100, 50, nil}, func() { close(earlierDone) }), "earlier") + manager.NewCmdTask(start, nil, LinesToRead{Total: 100, InitialRefreshAfter: 50}, func() { close(earlierDone) }), "earlier") <-stalled.blocked // The later task is asked for, but not created yet, as for a render whose task