Merge pull request #2497 from stefanhaller/fix-initial-scroll-bar-size

5 files changed, 183 insertions, 20 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..ad227fc65 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{})
 
@@ -157,13 +167,25 @@ func (self *ViewBufferManager) NewCmdTask(start func() (*exec.Cmd, io.Reader), p
 		})
 
 		go utils.Safe(func() {
+			isViewStale := true
+			writeToView := func(content []byte) {
+				_, _ = self.writer.Write(content)
+				isViewStale = true
+			}
+			refreshViewIfStale := func() {
+				if isViewStale {
+					self.refreshView()
+					isViewStale = false
+				}
+			}
+
 		outer:
 			for {
 				select {
 				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
@@ -175,7 +197,7 @@ func (self *ViewBufferManager) NewCmdTask(start func() (*exec.Cmd, io.Reader), p
 						if !loaded {
 							self.beforeStart()
 							if prefix != "" {
-								_, _ = self.writer.Write([]byte(prefix))
+								writeToView([]byte(prefix))
 							}
 							loaded = true
 						}
@@ -187,13 +209,20 @@ func (self *ViewBufferManager) NewCmdTask(start func() (*exec.Cmd, io.Reader), p
 							self.onEndOfInput()
 							break outer
 						}
-						_, _ = self.writer.Write(append(scanner.Bytes(), '\n'))
+						writeToView(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.
+							refreshViewIfStale()
+						}
 					}
-					self.refreshView()
+					refreshViewIfStale()
 				}
 			}
 
-			self.refreshView()
+			refreshViewIfStale()
 
 			if err := cmd.Wait(); err != nil {
 				// it's fine if we've killed this program ourselves
diff --git a/pkg/tasks/tasks_test.go b/pkg/tasks/tasks_test.go
index 9bd552162..ef13f6bf6 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},
+		},
+		{
+			"total == initialRefreshAfter",
+			150,
+			LinesToRead{100, 100},
+			[]int{100},
+		},
+		{
+			"total > initialRefreshAfter",
+			150,
+			LinesToRead{100, 50},
+			[]int{50, 100},
+		},
+		{
+			"initialRefreshAfter == -1",
+			150,
+			LinesToRead{100, -1},
+			[]int{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)
+		}
+	}
+}