From 4adca84d68105f703f8c4755229c6e46914a99ca Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Fri, 10 Mar 2023 13:35:19 +0100 Subject: 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. --- pkg/gui/pty.go | 6 +-- pkg/gui/tasks_adapter.go | 6 +-- pkg/gui/view_helpers.go | 28 ++++++++++++ pkg/tasks/tasks.go | 29 +++++++++--- pkg/tasks/tasks_test.go | 114 ++++++++++++++++++++++++++++++++++++++++++++++- 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) + } + } +} -- cgit v1.2.3 From 0af4e5a843f21bb2bc1caa6a24acf839df9c991a Mon Sep 17 00:00:00 2001 From: Jesse Duffield Date: Sun, 2 Apr 2023 15:44:05 +1000 Subject: prevent unnecessary re-renders of view --- pkg/tasks/tasks.go | 22 +++++++++++++++++----- pkg/tasks/tasks_test.go | 8 ++++---- 2 files changed, 21 insertions(+), 9 deletions(-) diff --git a/pkg/tasks/tasks.go b/pkg/tasks/tasks.go index 896af91ab..ad227fc65 100644 --- a/pkg/tasks/tasks.go +++ b/pkg/tasks/tasks.go @@ -167,6 +167,18 @@ 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 { @@ -185,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 } @@ -197,20 +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. - self.refreshView() + 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 f12da405f..ef13f6bf6 100644 --- a/pkg/tasks/tasks_test.go +++ b/pkg/tasks/tasks_test.go @@ -167,25 +167,25 @@ func TestNewCmdTaskRefresh(t *testing.T) { "total < initialRefreshAfter", 150, LinesToRead{100, 120}, - []int{100, 100}, + []int{100}, }, { "total == initialRefreshAfter", 150, LinesToRead{100, 100}, - []int{100, 100, 100}, + []int{100}, }, { "total > initialRefreshAfter", 150, LinesToRead{100, 50}, - []int{50, 100, 100}, + []int{50, 100}, }, { "initialRefreshAfter == -1", 150, LinesToRead{100, -1}, - []int{100, 100}, + []int{100}, }, { "totalTaskLines < initialRefreshAfter", -- cgit v1.2.3