diff --git a/pkg/gocui/gui.go b/pkg/gocui/gui.go index 9f40ff17d..b4cf107de 100644 --- a/pkg/gocui/gui.go +++ b/pkg/gocui/gui.go @@ -1276,7 +1276,7 @@ func calcScrollbarRune( func calcRealScrollbarStartEnd(v *View) (bool, int, int) { height := v.InnerHeight() - fullHeight := v.ViewLinesHeight() - v.scrollMargin() + fullHeight := v.scrollbarContentHeight() - v.scrollMargin() if v.CanScrollPastBottom { fullHeight += height diff --git a/pkg/gocui/view.go b/pkg/gocui/view.go index 4d9998529..fe01e50ca 100644 --- a/pkg/gocui/view.go +++ b/pkg/gocui/view.go @@ -58,11 +58,18 @@ type View struct { outMode OutputMode // buf bundles the view's cell buffer and the cursor / escape-parser state - // used to write into it (see the viewBuffer type). Bundling these makes it - // possible to build a second, off-screen buffer during a re-render and swap - // it in atomically once ready, so no reader ever sees a half-written buffer. + // used to write into it (see the viewBuffer type). It is the buffer every + // reader sees. buf *viewBuffer + // While non-nil, writes go here instead of buf, so an async re-render can + // build its new content without disturbing what readers (draw, clicks, + // scrolling, …) see. The task swaps it into buf once it has read enough to + // paint (SwapInOffscreenRender), so the displayed content jumps straight + // from the previous render to the new one with no half-written frame in + // between. nil during normal (non-async) writes. + offscreen *viewBuffer + // The y position of the first line of a range selection. // This is not relative to the view's origin: it is relative to the first line // of the view's content, so you can scroll the view and this value will remain @@ -101,6 +108,17 @@ type View struct { // true and viewLines to nil viewLines []viewLine + // While a re-render is loading new content (see offscreen), the displayed + // buffer is only partially filled once we've swapped the off-screen render + // in: the task keeps appending lines after the first paint, up to the count + // needed for an accurate scrollbar. Sizing the scrollbar from that partial + // view-line count would make the thumb shrink and snap back as the rest + // streams in. So while a load is in progress we hold the scrollbar's height + // at this value — the height the view had when the load began — and let it + // grow only if the new content turns out taller. Zero means no load is in + // progress and the scrollbar tracks the content directly. + scrollbarHeightFloor int + // writeMutex protects locks the write process writeMutex sync.Mutex @@ -866,6 +884,14 @@ func (v *View) Write(p []byte) (n int, err error) { } func (v *View) write(p []byte) { + // An async re-render builds into the off-screen buffer (see View.offscreen) + // until it swaps in; until then the displayed buffer, and so everything + // readers see, is left untouched. + if v.offscreen != nil { + v.offscreen.write(v, p) + return + } + v.tainted = true // write only ever touches lines from v.buf.wy onwards, so any cached wrapping // below that stays valid. @@ -1144,6 +1170,15 @@ func (v *View) clear() { v.rewind() v.buf.lines = nil v.clearViewLines() + // Abandon any in-progress off-screen render: a synchronous SetContent/Clear + // is taking over the displayed buffer, so writes must go there, not into a + // stale off-screen buffer left by a stopped task. + v.offscreen = nil + // Likewise release any held scrollbar height: the new content is defined + // synchronously (e.g. a string render superseding a still-loading diff), so + // there's no async growth left to smooth over and the scrollbar should track + // the new content directly. + v.scrollbarHeightFloor = 0 } // Clear empties the view's internal buffer. @@ -1208,6 +1243,9 @@ func (v *View) Reset() { v.rewind() v.buf.lines = nil + // As in clear(): abandon any in-progress off-screen render so writes after a + // reset go to the displayed buffer. + v.offscreen = nil } // This is for when we've done a restart for the sake of avoiding a flicker and @@ -1221,6 +1259,79 @@ func (v *View) FlushStaleCells() { v.clearViewLines() } +// BeginOffscreenRender starts building a re-render into an off-screen buffer. +// Until SwapInOffscreenRender promotes it, writes go to that buffer and the +// displayed buffer — what every reader sees — is left as it was. This is how an +// async re-render avoids exposing a half-written buffer: it accumulates +// off-screen and swaps in once it has read enough to paint. +func (v *View) BeginOffscreenRender() { + v.writeMutex.Lock() + defer v.writeMutex.Unlock() + + ei := newEscapeInterpreter(v.outMode) + // The screen width content is wrapped at is render configuration set by + // SetContentWidth, not per-buffer state, so the off-screen buffer's parser + // needs it too — otherwise it counts no soft wraps and cursor-positioning + // escapes land on the wrong rows. + ei.screenColMax = v.buf.ei.screenColMax + v.offscreen = &viewBuffer{ei: ei} +} + +// SwapInOffscreenRender promotes the off-screen buffer (see BeginOffscreenRender) +// to the displayed buffer in one step, so the view jumps straight from the +// previous render to the new one with no half-written frame. Writes after this +// append to the now-displayed buffer directly. It is a no-op if no off-screen +// render is in progress, so it is safe to call more than once (e.g. again at EOF +// after an earlier paint already swapped). +func (v *View) SwapInOffscreenRender() { + v.writeMutex.Lock() + defer v.writeMutex.Unlock() + + if v.offscreen == nil { + return + } + v.buf = v.offscreen + v.offscreen = nil + v.tainted = true + v.clearHover() +} + +// FreezeScrollbarHeight records the view's current content height so the +// scrollbar keeps that size while a re-render loads, instead of shrinking and +// snapping back as the partially-loaded content streams in past the first paint +// (see scrollbarHeightFloor). Call it when a load begins, while the view still +// shows the previous render; UnfreezeScrollbarHeight clears it when the load +// ends. +func (v *View) FreezeScrollbarHeight() { + v.writeMutex.Lock() + defer v.writeMutex.Unlock() + + v.refreshViewLinesIfNeeded() + v.scrollbarHeightFloor = len(v.viewLines) +} + +// UnfreezeScrollbarHeight clears the height held by FreezeScrollbarHeight, so +// the scrollbar tracks the view's content directly again. Call it when a load +// ends. +func (v *View) UnfreezeScrollbarHeight() { + v.writeMutex.Lock() + defer v.writeMutex.Unlock() + + v.scrollbarHeightFloor = 0 +} + +// scrollbarContentHeight is the view-line height the scrollbar is sized from. +// While a re-render is loading it is held at the height the view had when the +// load began (see FreezeScrollbarHeight), so the thumb doesn't shrink and jump +// as partially-loaded content streams in. +func (v *View) scrollbarContentHeight() int { + v.writeMutex.Lock() + defer v.writeMutex.Unlock() + + v.refreshViewLinesIfNeeded() + return max(len(v.viewLines), v.scrollbarHeightFloor) +} + func (v *View) rewind() { v.buf.ei.reset() v.buf.ei.resetScreenCursor() @@ -1495,6 +1606,13 @@ func (v *View) refreshViewLinesIfNeeded() { } v.firstDirtyLine = len(lines) + // Truncate any entries left over from a previous, longer render. An async + // re-render builds its content off-screen and swaps it in whole (see + // View.offscreen), so the buffer this rebuilds from is always a complete + // render — there is no half-loaded shorter buffer whose tail we'd need to + // keep showing to avoid a flicker, and a leftover tail would just be stale + // lines mapping to the wrong buffer rows. + v.viewLines = v.viewLines[:lineIdx] v.tainted = false } diff --git a/pkg/gocui/view_test.go b/pkg/gocui/view_test.go index 61dc26b52..294f02b32 100644 --- a/pkg/gocui/view_test.go +++ b/pkg/gocui/view_test.go @@ -159,6 +159,102 @@ func TestAutoRenderingHyperlinks(t *testing.T) { assert.Equal(t, "https://example.com", v.buf.lines[0].cells[0].hyperlink) } +// An async re-render builds into an off-screen buffer and swaps it in once it +// has enough to paint, so readers keep seeing the previous render — coherent and +// consistent — until the new content appears in one step. See View.offscreen. +func TestOffscreenRender(t *testing.T) { + v := NewView("name", 0, 0, 80, 10, OutputNormal) + + v.writeString("a\nb\nc") + assert.Equal(t, []string{"a", "b", "c"}, v.ViewBufferLines()) + + // Render new, longer content off-screen. + v.BeginOffscreenRender() + v.writeString("w\nx\ny\nz") + + // The displayed buffer is untouched: readers still see the previous render. + assert.Equal(t, []string{"a", "b", "c"}, v.ViewBufferLines()) + + // Swapping in reveals the new content in one step. + v.SwapInOffscreenRender() + assert.Equal(t, []string{"w", "x", "y", "z"}, v.ViewBufferLines()) + + // A further write now appends to the displayed buffer directly. + v.writeString("\nmore") + assert.Equal(t, []string{"w", "x", "y", "z", "more"}, v.ViewBufferLines()) +} + +// When a render produces fewer view lines than the previous one, +// refreshViewLinesIfNeeded must truncate viewLines to the new content rather +// than leaving the previous render's entries in the tail: with the off-screen +// render there is no half-loaded buffer whose tail we'd want to keep showing, +// and a leftover tail is just stale lines describing content that is gone. +func TestViewLinesTruncatedByShorterRender(t *testing.T) { + v := NewView("name", 0, 0, 10, 10, OutputNormal) // InnerWidth is 9 + v.Wrap = true + + // Two lines of 27 characters each wrap into 3 view lines apiece. + v.writeString(strings.Repeat("a", 27) + "\n" + strings.Repeat("b", 27)) + assert.Equal(t, 6, v.ViewLinesHeight()) + + // Re-render with three short, unwrapped lines: only 3 view lines remain. + v.BeginOffscreenRender() + v.writeString("aaa\nbbb\nccc") + v.SwapInOffscreenRender() + assert.Equal(t, 3, v.ViewLinesHeight()) + assert.Equal(t, []string{"aaa", "bbb", "ccc"}, v.ViewBufferLines()) +} + +// While an async re-render loads, it swaps in only a partially-filled buffer at +// its first paint and keeps appending lines afterwards. The scrollbar must keep +// using the pre-load height until the load ends, so the thumb doesn't shrink and +// snap back as the rest streams in. See View.scrollbarHeightFloor. +func TestScrollbarHeightHeldWhileLoading(t *testing.T) { + v := NewView("name", 0, 0, 80, 12, OutputNormal) + + // Initial render: 100 lines, scrolled well down. + v.writeString(strings.Repeat("x\n", 100)) + v.SetOrigin(0, 80) + assert.Equal(t, 100, v.scrollbarContentHeight()) + + // A re-render begins while the previous render is still shown: hold the + // scrollbar height at the current value. + v.FreezeScrollbarHeight() + + // The off-screen render swaps in only a screenful at its first paint. + v.BeginOffscreenRender() + v.writeString(strings.Repeat("y\n", 30)) + v.SwapInOffscreenRender() + + // The displayed buffer is now short, but the scrollbar height stays held, so + // the thumb keeps its position instead of jumping. + assert.Equal(t, 30, v.ViewLinesHeight()) + assert.Equal(t, 100, v.scrollbarContentHeight()) + + // The rest of the content streams in. + v.writeString(strings.Repeat("y\n", 70)) + assert.Equal(t, 100, v.scrollbarContentHeight()) + + // Once the load ends, the scrollbar tracks the real content directly again. + v.UnfreezeScrollbarHeight() + assert.Equal(t, 100, v.scrollbarContentHeight()) +} + +// If a synchronous render (e.g. a string render) supersedes a still-loading diff +// before it reaches its end, the held scrollbar height must be released, so the +// scrollbar reflects the new content rather than the abandoned load's height. +func TestScrollbarHeightReleasedWhenContentReplaced(t *testing.T) { + v := NewView("name", 0, 0, 80, 12, OutputNormal) + + v.writeString(strings.Repeat("x\n", 100)) + v.FreezeScrollbarHeight() + assert.Equal(t, 100, v.scrollbarContentHeight()) + + // A synchronous render replaces the content before the (notional) load ends. + v.SetContent("just a few\nshort lines\nhere") + assert.Equal(t, 3, v.scrollbarContentHeight()) +} + func TestContainsColoredText(t *testing.T) { hexColor := func(text string, hexStr string) []cell { cells := make([]cell, len(text)) @@ -282,6 +378,31 @@ func TestWriteCursorPositionEscapeAcrossWrites(t *testing.T) { }, got) } +func TestWriteCursorPositionEscapeInOffscreenRender(t *testing.T) { + // Soft-wrap counting has to work in an off-screen render too: the content + // width the parser counts wraps against is set by SetContentWidth before the + // render starts, so the off-screen buffer's parser has to pick it up. If it + // doesn't, no wraps are counted and the CUP below is evaluated against a + // stale row, overshooting into an extra blank line. + v := NewView("name", 0, 0, 30, 30, OutputNormal) + v.SetContentWidth(5) + + v.BeginOffscreenRender() + // Seven characters soft-wrap once on a 5-column screen, putting ConPTY on + // row 2; CUP to row 3 should then skip no rows at all. + v.writeString("aaaaaaa\x1b[3;1Hb\n") + v.SwapInOffscreenRender() + + got := make([][]string, 0, len(v.buf.lines)) + for _, l := range v.buf.lines { + got = append(got, cellsToStrings(l.cells)) + } + assert.Equal(t, [][]string{ + {"a", "a", "a", "a", "a", "a", "a"}, + {"b"}, + }, got) +} + func TestWriteCursorForwardEscape(t *testing.T) { // ConPTY compresses runs of default-colored spaces into ECH (\x1b[NX, // "clear N cells, cursor stationary") + CUF (\x1b[NC, "cursor forward diff --git a/pkg/gui/pty.go b/pkg/gui/pty.go index 1cb8d9412..fb7ba352e 100644 --- a/pkg/gui/pty.go +++ b/pkg/gui/pty.go @@ -77,6 +77,10 @@ func (gui *Gui) newPtyTask(view *gocui.View, cmd *exec.Cmd, prefix string) error // without this the next layout pass would clamp the scroll position to the // not-yet-loaded content. gui.getManager(view).StartLoading() + // Hold the scrollbar at its current height while the re-render loads, so the + // thumb doesn't shrink and snap back when the first partial paint swaps in + // (see the matching call in newCmdTask). + view.FreezeScrollbarHeight() // Run the pty after layout so that it gets the correct size gui.afterLayout(func() error { diff --git a/pkg/gui/tasks_adapter.go b/pkg/gui/tasks_adapter.go index 440bc0aab..f2f567a75 100644 --- a/pkg/gui/tasks_adapter.go +++ b/pkg/gui/tasks_adapter.go @@ -22,6 +22,11 @@ func (gui *Gui) newCmdTask(view *gocui.View, cmd *exec.Cmd, prefix string) error // and before the next layout pass) so the layout doesn't clamp the scroll // position to the not-yet-loaded content. manager.StartLoading() + // Hold the scrollbar at the height the view has now (the previous render), + // while it still shows that render: once the re-render swaps in its first + // partial paint the displayed buffer is briefly short, and we don't want the + // thumb to shrink and snap back as the rest loads. + view.FreezeScrollbarHeight() // Snapshot the view width here, on the UI thread, so the task goroutine // doesn't read the view's live dimensions while it streams output. It's @@ -137,12 +142,10 @@ func (gui *Gui) getManager(view *gocui.View) *tasks.ViewBufferManager { gui.Log, view, func() { - // we could clear here, but that actually has the effect of causing a flicker - // where the view may contain no content momentarily as the gui refreshes. - // Instead, we're rewinding the write pointer so that we will just start - // overwriting the existing content from the top down. Once we've reached - // the end of the content do display, we call view.FlushStaleCells() to - // clear out the remaining content from the previous render. + // Called before showing the "loading..." indicator: clear the + // displayed buffer so only "loading..." is shown. The actual content + // is rendered off-screen (beginRender below) and swapped in, so it + // never overwrites the displayed buffer incrementally. view.Reset() }, func() { @@ -154,6 +157,11 @@ func (gui *Gui) getManager(view *gocui.View) *tasks.ViewBufferManager { gui.renderContentOnly() }, func() { + // The content is fully loaded now, so let the scrollbar track it + // directly again (it was held at the previous render's height while + // loading, see FreezeScrollbarHeight). + view.UnfreezeScrollbarHeight() + // Need to check if the content of the view is well past the origin. linesHeight := view.ViewLinesHeight() _, originY := view.Origin() @@ -168,6 +176,8 @@ func (gui *Gui) getManager(view *gocui.View) *tasks.ViewBufferManager { func() { view.SetOrigin(0, 0) }, + view.BeginOffscreenRender, + view.SwapInOffscreenRender, func() gocui.Task { // A background task: rendering content into a view is display // work, not lazygit driving a git operation, so it must not diff --git a/pkg/tasks/tasks.go b/pkg/tasks/tasks.go index 0fa04eb3e..918713a05 100644 --- a/pkg/tasks/tasks.go +++ b/pkg/tasks/tasks.go @@ -95,6 +95,13 @@ type ViewBufferManager struct { refreshView func() onEndOfInput func() + // beginRender starts an off-screen render: the new content is built without + // disturbing what's displayed. swapInRender then promotes it to the display + // in one step. Together they keep the view showing the previous render until + // the new one has read enough to paint, instead of revealing it line by line. + beginRender func() + swapInRender func() + // see docs/dev/Busy.md // A gocui task is not the same thing as the tasks defined in this file. // A gocui task simply represents the fact that lazygit is busy doing something, @@ -146,6 +153,8 @@ func NewViewBufferManager( refreshView func(), onEndOfInput func(), onNewKey func(), + beginRender func(), + swapInRender func(), newGocuiTask func() gocui.Task, onUIThread func(f func()) error, ) *ViewBufferManager { @@ -156,6 +165,8 @@ func NewViewBufferManager( refreshView: refreshView, onEndOfInput: onEndOfInput, onNewKey: onNewKey, + beginRender: beginRender, + swapInRender: swapInRender, newGocuiTask: newGocuiTask, onUIThread: onUIThread, } @@ -340,8 +351,9 @@ func (self *ViewBufferManager) NewCmdTask(start func() (Cmd, io.Reader), prefix // closed the selects below could still service a ready data channel // instead of bailing. Check stop explicitly first to give it priority: // a task that's been stopped (it's being replaced by a newer one) must - // not touch the view here — beforeStart clears it and the prefix gets - // written, clobbering what the incoming task is about to render. + // not touch the view here — it would start an off-screen render and + // write the prefix into it, clobbering what the incoming task is about + // to render. stopped := func() bool { select { case <-opts.Stop: @@ -398,7 +410,10 @@ func (self *ViewBufferManager) NewCmdTask(start func() (Cmd, io.Reader), prefix loadingMutex.Lock() if !loaded { - self.beforeStart() + // Build the new content off-screen, leaving the previous render + // displayed until we swap in below; this is what keeps an async + // re-render from showing a half-loaded buffer. + self.beginRender() if prefix != "" { writeToView([]byte(prefix)) } @@ -407,12 +422,16 @@ func (self *ViewBufferManager) NewCmdTask(start func() (Cmd, io.Reader), prefix loadingMutex.Unlock() if !ok { - // if we're here then there's nothing left to scan from the source - // so we're at the EOF and can flush the stale content. + // We're at EOF before reaching InitialRefreshAfter (the content was + // shorter than a screenful), so swap in whatever we read now, and + // flush the stale content. // onEndOfInput reads the view's dimensions (to decide // whether to scroll) and sets the origin, both of which // are UI-thread-only, so run it there. - _ = self.onUIThread(self.onEndOfInput) + _ = self.onUIThread(func() { + self.swapInRender() + self.onEndOfInput() + }) // Whatever there was to show is on screen now. self.newContentPending.Store(false) // The content is fully loaded now, so it's safe again for the @@ -447,9 +466,11 @@ func (self *ViewBufferManager) NewCmdTask(start func() (Cmd, io.Reader), prefix } if linesRead == 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. + // We have read enough lines to fill the view, so swap the off-screen + // content in and do a first refresh to show it. Continue reading and + // refresh again at the end to make sure the scrollbar has the right + // size. + self.swapInRender() self.newContentPending.Store(false) refreshViewIfStale() } diff --git a/pkg/tasks/tasks_test.go b/pkg/tasks/tasks_test.go index ad4469043..7d10836d8 100644 --- a/pkg/tasks/tasks_test.go +++ b/pkg/tasks/tasks_test.go @@ -27,6 +27,8 @@ func TestNewCmdTaskInstantStop(t *testing.T) { refreshView, getRefreshViewCallCount := getCounter() onEndOfInput, getOnEndOfInputCallCount := getCounter() onNewKey, getOnNewKeyCallCount := getCounter() + beginRender, getBeginRenderCallCount := getCounter() + swapInRender, getSwapInRenderCallCount := getCounter() onDone, getOnDoneCallCount := getCounter() task := gocui.NewFakeTask() newTask := func() gocui.Task { @@ -40,6 +42,8 @@ func TestNewCmdTaskInstantStop(t *testing.T) { refreshView, onEndOfInput, onNewKey, + beginRender, + swapInRender, newTask, // no UI thread in the test; run the view mutations inline func(f func()) error { f(); return nil }, @@ -69,6 +73,8 @@ func TestNewCmdTaskInstantStop(t *testing.T) { {1, getRefreshViewCallCount(), "refreshView"}, {0, getOnEndOfInputCallCount(), "onEndOfInput"}, {0, getOnNewKeyCallCount(), "onNewKey"}, + {0, getBeginRenderCallCount(), "beginRender"}, + {0, getSwapInRenderCallCount(), "swapInRender"}, {1, getOnDoneCallCount(), "onDone"}, } for _, expectation := range callCountExpectations { @@ -94,6 +100,8 @@ func TestNewCmdTask(t *testing.T) { refreshView, getRefreshViewCallCount := getCounter() onEndOfInput, getOnEndOfInputCallCount := getCounter() onNewKey, getOnNewKeyCallCount := getCounter() + beginRender, getBeginRenderCallCount := getCounter() + swapInRender, getSwapInRenderCallCount := getCounter() onDone, getOnDoneCallCount := getCounter() task := gocui.NewFakeTask() newTask := func() gocui.Task { @@ -107,6 +115,8 @@ func TestNewCmdTask(t *testing.T) { refreshView, onEndOfInput, onNewKey, + beginRender, + swapInRender, newTask, // no UI thread in the test; run the view mutations inline func(f func()) error { f(); return nil }, @@ -136,10 +146,12 @@ func TestNewCmdTask(t *testing.T) { actual int name string }{ - {1, getBeforeStartCallCount(), "beforeStart"}, + {0, getBeforeStartCallCount(), "beforeStart"}, {1, getRefreshViewCallCount(), "refreshView"}, {1, getOnEndOfInputCallCount(), "onEndOfInput"}, {0, getOnNewKeyCallCount(), "onNewKey"}, + {1, getBeginRenderCallCount(), "beginRender"}, + {1, getSwapInRenderCallCount(), "swapInRender"}, {1, getOnDoneCallCount(), "onDone"}, } for _, expectation := range callCountExpectations { @@ -213,6 +225,8 @@ func TestNewCmdTaskQueuedReadAtEndOfInput(t *testing.T) { func() {}, func() {}, func() {}, + func() {}, + func() {}, func() gocui.Task { return task }, // no UI thread in the test; run the view mutations inline func(f func()) error { f(); return nil }, @@ -279,6 +293,8 @@ func TestLoadingIndicatorOnlyTakesOverForNewContent(t *testing.T) { func() {}, func() {}, func() {}, + func() {}, + func() {}, func() gocui.Task { return gocui.NewFakeTask() }, // no UI thread in the test; run the view mutations inline func(f func()) error { f(); return nil }, @@ -392,6 +408,8 @@ func TestNewCmdTaskRefresh(t *testing.T) { refreshView, func() {}, func() {}, + func() {}, + func() {}, newTask, // no UI thread in the test; run the view mutations inline func(f func()) error { f(); return nil },