diff --git a/pkg/gocui/gui.go b/pkg/gocui/gui.go index 4aaa0a984..af741732a 100644 --- a/pkg/gocui/gui.go +++ b/pkg/gocui/gui.go @@ -1246,7 +1246,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 d61a60eac..fa3729dd8 100644 --- a/pkg/gocui/view.go +++ b/pkg/gocui/view.go @@ -58,10 +58,17 @@ 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, the diff-line readers, …) 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 @@ -100,6 +107,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 @@ -868,6 +886,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.wy onwards, so any cached wrapping // below that stays valid. @@ -1151,6 +1177,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. @@ -1215,6 +1250,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 @@ -1228,6 +1266,73 @@ 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() + + v.offscreen = &viewBuffer{ei: newEscapeInterpreter(v.outMode)} +} + +// 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() @@ -1502,6 +1607,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 bfc4fe99f..3a49e4b0d 100644 --- a/pkg/gocui/view_test.go +++ b/pkg/gocui/view_test.go @@ -197,14 +197,12 @@ func TestDiffLineMetadata(t *testing.T) { } // When a re-render produces fewer view lines than the previous one, -// refreshViewLinesIfNeeded overwrites viewLines in place without truncating, so -// the tail keeps the previous render's entries (deliberately, so the view keeps -// showing old content until the new content catches up). A reader must not map -// a view line in that stale tail to a buffer line. With wrapping, the stale -// entry's buffer index can still be in range of the new (shorter, less-wrapped) -// buffer, so the in-range guard alone lets it through and maps a view line that -// no longer exists onto the wrong buffer line. See diff-line-metadata-notes.md -// §8. +// refreshViewLinesIfNeeded must truncate viewLines to the new content. If it +// didn't (it used to overwrite in place and keep the tail), a reader could map a +// view line that no longer exists onto the wrong buffer line — and with wrapping +// the stale entry's buffer index can still be in range of the new, shorter, +// less-wrapped buffer, so an in-range guard alone wouldn't catch it. See +// diff-line-metadata-notes.md §8. func TestBufferLineForViewLineStaleTail(t *testing.T) { v := NewView("name", 0, 0, 10, 10, OutputNormal) // InnerWidth is 9 v.Wrap = true @@ -214,26 +212,100 @@ func TestBufferLineForViewLineStaleTail(t *testing.T) { v.writeString(strings.Repeat("a", 27) + "\n" + strings.Repeat("b", 27)) assert.Equal(t, 6, v.ViewLinesHeight()) - // Re-render with shorter content the flicker-avoidance way: rewind (which - // keeps the old view lines) and overwrite from the top with three short, - // unwrapped lines. There are now only 3 real view lines, but the previous - // render's view lines 3..5 linger in the tail. + // Re-render with shorter content (rewind, then overwrite from the top with + // three short, unwrapped lines). There are now only 3 view lines. v.Reset() v.writeString("aaa\nbbb\nccc") + assert.Equal(t, 3, v.ViewLinesHeight()) // A real view line maps to its buffer line as usual. bufferLine, ok := v.BufferLineForViewLine(1) assert.True(t, ok) assert.Equal(t, 1, bufferLine) - // View line 4 is in the stale tail: it no longer exists in the current - // buffer, so the mapping must fail. (On the buggy code it instead maps to - // buffer line 1, the stale entry's lingering index.) + // View line 4 no longer exists in the current buffer, so the mapping must + // fail rather than land on a stale entry from the previous render. _, ok = v.BufferLineForViewLine(4) - /* EXPECTED: assert.False(t, ok) - ACTUAL: */ +} + +// 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, + // and the view-line→buffer-line mapping stays consistent with it. + assert.Equal(t, []string{"a", "b", "c"}, v.ViewBufferLines()) + bufferLine, ok := v.BufferLineForViewLine(1) assert.True(t, ok) + assert.Equal(t, 1, bufferLine) + + // 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()) +} + +// 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) { diff --git a/pkg/gui/pty.go b/pkg/gui/pty.go index b3ca1b7f4..df0fa2cbd 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() // Read any requested scroll-restore now so we can size the initial read to it // in afterLayout; the task itself clears the request and applies the scroll at diff --git a/pkg/gui/tasks_adapter.go b/pkg/gui/tasks_adapter.go index 699715dca..0aa136b36 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 @@ -150,12 +155,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() { @@ -167,6 +170,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() @@ -181,6 +189,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 4f40adb0b..20c0c9600 100644 --- a/pkg/tasks/tasks.go +++ b/pkg/tasks/tasks.go @@ -113,6 +113,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, @@ -167,6 +174,8 @@ func NewViewBufferManager( refreshView func(), onEndOfInput func(), onNewKey func(), + beginRender func(), + swapInRender func(), newGocuiTask func() gocui.Task, onUIThread func(f func() error) error, ) *ViewBufferManager { @@ -177,6 +186,8 @@ func NewViewBufferManager( refreshView: refreshView, onEndOfInput: onEndOfInput, onNewKey: onNewKey, + beginRender: beginRender, + swapInRender: swapInRender, newGocuiTask: newGocuiTask, onUIThread: onUIThread, } @@ -456,7 +467,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)) } @@ -465,14 +479,15 @@ 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. Apply the - // saved scroll first (if any) so that onEndOfInput clamps it back + // We're at EOF before reaching InitialRefreshAfter (the content was + // shorter than a screenful), so swap in whatever we read now. Apply + // the saved scroll first (if any) so that onEndOfInput clamps it back // into range when the new content turned out shorter than expected. // 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(func() error { + self.swapInRender() applyInitialScroll() self.onEndOfInput() return nil @@ -509,11 +524,12 @@ 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. Apply the saved scroll first (if any) - // so the first paint already lands at it. 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. Apply the saved + // scroll first (if any) so the first paint already lands at it. + // Continue reading and refresh again at the end to make sure the + // scrollbar has the right size. + self.swapInRender() applyInitialScroll() refreshViewIfStale() } diff --git a/pkg/tasks/tasks_test.go b/pkg/tasks/tasks_test.go index c5266e5ed..94242a3c1 100644 --- a/pkg/tasks/tasks_test.go +++ b/pkg/tasks/tasks_test.go @@ -25,6 +25,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 { @@ -38,6 +40,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) error { return f() }, @@ -67,6 +71,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 { @@ -92,6 +98,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 { @@ -105,6 +113,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) error { return f() }, @@ -134,10 +144,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 { @@ -240,6 +252,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) error { return f() },