Make sure scrollbars have the right size initially

We refresh the view after reading just enough to fill it, so that we see the
initial content as quickly as possible, but then we continue reading enough
lines so that we can tell how long the scrollbar needs to be, and then we
refresh again. This can result in slight flicker of the scrollbar when it is
first drawn with a bigger size and then jumps to a smaller size; however, that's
a good tradeoff for a solution that provides both good speed and accuracy.

5 files changed, 167 insertions, 16 deletions

diff --git a/pkg/gui/pty.go b/pkg/gui/pty.go
index 4a87c98da..fe53697c8 100644
--- a/pkg/gui/pty.go
+++ b/pkg/gui/pty.go
@@ -55,9 +55,6 @@ func (gui *Gui) newPtyTask(view *gocui.View, cmd *exec.Cmd, prefix string) error
 
 	cmd.Env = append(cmd.Env, "GIT_PAGER="+pager)
 
-	_, height := view.Size()
-	_, oy := view.Origin()
-
 	manager := gui.getManager(view)
 
 	var ptmx *os.File
@@ -82,7 +79,8 @@ func (gui *Gui) newPtyTask(view *gocui.View, cmd *exec.Cmd, prefix string) error
 		gui.Mutexes.PtyMutex.Unlock()
 	}
 
-	if err := manager.NewTask(manager.NewCmdTask(start, prefix, height+oy+10, onClose), cmdStr); err != nil {
+	linesToRead := gui.linesToReadFromCmdTask(view)
+	if err := manager.NewTask(manager.NewCmdTask(start, prefix, linesToRead, onClose), cmdStr); err != nil {
 		return err
 	}
 
diff --git a/pkg/gui/tasks_adapter.go b/pkg/gui/tasks_adapter.go
index 33aa09eb9..69b64d7b1 100644
--- a/pkg/gui/tasks_adapter.go
+++ b/pkg/gui/tasks_adapter.go
@@ -16,9 +16,6 @@ func (gui *Gui) newCmdTask(view *gocui.View, cmd *exec.Cmd, prefix string) error
 		cmdStr,
 	).Debug("RunCommand")
 
-	_, height := view.Size()
-	_, oy := view.Origin()
-
 	manager := gui.getManager(view)
 
 	start := func() (*exec.Cmd, io.Reader) {
@@ -35,7 +32,8 @@ func (gui *Gui) newCmdTask(view *gocui.View, cmd *exec.Cmd, prefix string) error
 		return cmd, r
 	}
 
-	if err := manager.NewTask(manager.NewCmdTask(start, prefix, height+oy+10, nil), cmdStr); err != nil {
+	linesToRead := gui.linesToReadFromCmdTask(view)
+	if err := manager.NewTask(manager.NewCmdTask(start, prefix, linesToRead, nil), cmdStr); err != nil {
 		gui.c.Log.Error(err)
 	}
 
diff --git a/pkg/gui/view_helpers.go b/pkg/gui/view_helpers.go
index c51942b03..8f2055245 100644
--- a/pkg/gui/view_helpers.go
+++ b/pkg/gui/view_helpers.go
@@ -6,6 +6,7 @@ import (
 	"github.com/jesseduffield/gocui"
 	"github.com/jesseduffield/lazygit/pkg/gui/keybindings"
 	"github.com/jesseduffield/lazygit/pkg/gui/types"
+	"github.com/jesseduffield/lazygit/pkg/tasks"
 	"github.com/jesseduffield/lazygit/pkg/utils"
 	"github.com/spkg/bom"
 )
@@ -15,6 +16,33 @@ func (gui *Gui) resetOrigin(v *gocui.View) error {
 	return v.SetOrigin(0, 0)
 }
 
+// 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.
+func (gui *Gui) linesToReadFromCmdTask(v *gocui.View) tasks.LinesToRead {
+	_, height := v.Size()
+	_, oy := v.Origin()
+
+	linesForFirstRefresh := height + oy + 10
+
+	// We want to read as many lines initially as necessary to let the
+	// scrollbar go to its minimum height, so that the scrollbar thumb doesn't
+	// change size as you scroll down.
+	minScrollbarHeight := 2
+	linesToReadForAccurateScrollbar := height*(height-1)/minScrollbarHeight + oy
+
+	// However, cap it at some arbitrary max limit, so that we don't get
+	// performance problems for huge monitors or tiny font sizes
+	if linesToReadForAccurateScrollbar > 5000 {
+		linesToReadForAccurateScrollbar = 5000
+	}
+
+	return tasks.LinesToRead{
+		Total:               linesToReadForAccurateScrollbar,
+		InitialRefreshAfter: linesForFirstRefresh,
+	}
+}
+
 func (gui *Gui) cleanString(s string) string {
 	output := string(bom.Clean([]byte(s)))
 	return utils.NormalizeLinefeeds(output)
diff --git a/pkg/tasks/tasks.go b/pkg/tasks/tasks.go
index 448857fca..896af91ab 100644
--- a/pkg/tasks/tasks.go
+++ b/pkg/tasks/tasks.go
@@ -39,7 +39,7 @@ type ViewBufferManager struct {
 	taskIDMutex  deadlock.Mutex
 	Log          *logrus.Entry
 	newTaskID    int
-	readLines    chan int
+	readLines    chan LinesToRead
 	taskKey      string
 	onNewKey     func()
 
@@ -55,6 +55,16 @@ type ViewBufferManager struct {
 	throttle bool
 }
 
+type LinesToRead struct {
+	// Total number of lines to read
+	Total int
+
+	// 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.
+	InitialRefreshAfter int
+}
+
 func (m *ViewBufferManager) GetTaskKey() string {
 	return m.taskKey
 }
@@ -73,19 +83,19 @@ func NewViewBufferManager(
 		beforeStart:  beforeStart,
 		refreshView:  refreshView,
 		onEndOfInput: onEndOfInput,
-		readLines:    make(chan int, 1024),
+		readLines:    make(chan LinesToRead, 1024),
 		onNewKey:     onNewKey,
 	}
 }
 
 func (self *ViewBufferManager) ReadLines(n int) {
 	go utils.Safe(func() {
-		self.readLines <- n
+		self.readLines <- LinesToRead{Total: n, InitialRefreshAfter: -1}
 	})
 }
 
 // note: onDone may be called twice
-func (self *ViewBufferManager) NewCmdTask(start func() (*exec.Cmd, io.Reader), prefix string, linesToRead int, onDone func()) func(chan struct{}) error {
+func (self *ViewBufferManager) NewCmdTask(start func() (*exec.Cmd, io.Reader), prefix string, linesToRead LinesToRead, onDone func()) func(chan struct{}) error {
 	return func(stop chan struct{}) error {
 		var once sync.Once
 		var onDoneWrapper func()
@@ -130,7 +140,7 @@ func (self *ViewBufferManager) NewCmdTask(start func() (*exec.Cmd, io.Reader), p
 		loadingMutex := deadlock.Mutex{}
 
 		// not sure if it's the right move to redefine this or not
-		self.readLines = make(chan int, 1024)
+		self.readLines = make(chan LinesToRead, 1024)
 
 		done := make(chan struct{})
 
@@ -163,7 +173,7 @@ func (self *ViewBufferManager) NewCmdTask(start func() (*exec.Cmd, io.Reader), p
 				case <-stop:
 					break outer
 				case linesToRead := <-self.readLines:
-					for i := 0; i < linesToRead; i++ {
+					for i := 0; i < linesToRead.Total; i++ {
 						select {
 						case <-stop:
 							break outer
@@ -188,6 +198,13 @@ func (self *ViewBufferManager) NewCmdTask(start func() (*exec.Cmd, io.Reader), p
 							break outer
 						}
 						_, _ = self.writer.Write(append(scanner.Bytes(), '\n'))
+
+						if i+1 == linesToRead.InitialRefreshAfter {
+							// We have read enough lines to fill the view, so do a first refresh
+							// here to show what we have. Continue reading and refresh again at
+							// the end to make sure the scrollbar has the right size.
+							self.refreshView()
+						}
 					}
 					self.refreshView()
 				}
diff --git a/pkg/tasks/tasks_test.go b/pkg/tasks/tasks_test.go
index 9bd552162..f12da405f 100644
--- a/pkg/tasks/tasks_test.go
+++ b/pkg/tasks/tasks_test.go
@@ -4,6 +4,8 @@ import (
 	"bytes"
 	"io"
 	"os/exec"
+	"reflect"
+	"strings"
 	"sync"
 	"testing"
 	"time"
@@ -45,7 +47,7 @@ func TestNewCmdTaskInstantStop(t *testing.T) {
 		return cmd, reader
 	}
 
-	fn := manager.NewCmdTask(start, "prefix\n", 20, onDone)
+	fn := manager.NewCmdTask(start, "prefix\n", LinesToRead{20, -1}, onDone)
 
 	_ = fn(stop)
 
@@ -99,7 +101,7 @@ func TestNewCmdTask(t *testing.T) {
 		return cmd, reader
 	}
 
-	fn := manager.NewCmdTask(start, "prefix\n", 20, onDone)
+	fn := manager.NewCmdTask(start, "prefix\n", LinesToRead{20, -1}, onDone)
 	wg := sync.WaitGroup{}
 	wg.Add(1)
 	go func() {
@@ -134,3 +136,111 @@ func TestNewCmdTask(t *testing.T) {
 		t.Errorf("expected writer to receive the following content: \n%s\n. But instead it received: %s", expectedContent, actualContent)
 	}
 }
+
+// A dummy reader that simply yields as many blank lines as requested. The only
+// thing we want to do with the output is count the number of lines.
+type BlankLineReader struct {
+	totalLinesToYield int
+	linesYielded      int
+}
+
+func (d *BlankLineReader) Read(p []byte) (n int, err error) {
+	if d.totalLinesToYield == d.linesYielded {
+		return 0, io.EOF
+	}
+
+	d.linesYielded += 1
+	p[0] = '\n'
+	return 1, nil
+}
+
+func TestNewCmdTaskRefresh(t *testing.T) {
+	type scenario struct {
+		name                        string
+		totalTaskLines              int
+		linesToRead                 LinesToRead
+		expectedLineCountsOnRefresh []int
+	}
+
+	scenarios := []scenario{
+		{
+			"total < initialRefreshAfter",
+			150,
+			LinesToRead{100, 120},
+			[]int{100, 100},
+		},
+		{
+			"total == initialRefreshAfter",
+			150,
+			LinesToRead{100, 100},
+			[]int{100, 100, 100},
+		},
+		{
+			"total > initialRefreshAfter",
+			150,
+			LinesToRead{100, 50},
+			[]int{50, 100, 100},
+		},
+		{
+			"initialRefreshAfter == -1",
+			150,
+			LinesToRead{100, -1},
+			[]int{100, 100},
+		},
+		{
+			"totalTaskLines < initialRefreshAfter",
+			25,
+			LinesToRead{100, 50},
+			[]int{25},
+		},
+		{
+			"totalTaskLines between total and initialRefreshAfter",
+			75,
+			LinesToRead{100, 50},
+			[]int{50, 75},
+		},
+	}
+
+	for _, s := range scenarios {
+		writer := bytes.NewBuffer(nil)
+		lineCountsOnRefresh := []int{}
+		refreshView := func() {
+			lineCountsOnRefresh = append(lineCountsOnRefresh, strings.Count(writer.String(), "\n"))
+		}
+
+		manager := NewViewBufferManager(
+			utils.NewDummyLog(),
+			writer,
+			func() {},
+			refreshView,
+			func() {},
+			func() {},
+		)
+
+		stop := make(chan struct{})
+		reader := BlankLineReader{totalLinesToYield: s.totalTaskLines}
+		start := func() (*exec.Cmd, io.Reader) {
+			// not actually starting this because it's not necessary
+			cmd := secureexec.Command("blah blah")
+
+			return cmd, &reader
+		}
+
+		fn := manager.NewCmdTask(start, "", s.linesToRead, func() {})
+		wg := sync.WaitGroup{}
+		wg.Add(1)
+		go func() {
+			time.Sleep(100 * time.Millisecond)
+			close(stop)
+			wg.Done()
+		}()
+		_ = fn(stop)
+
+		wg.Wait()
+
+		if !reflect.DeepEqual(lineCountsOnRefresh, s.expectedLineCountsOnRefresh) {
+			t.Errorf("%s: expected line counts on refresh: %v, got %v",
+				s.name, s.expectedLineCountsOnRefresh, lineCountsOnRefresh)
+		}
+	}
+}