diff --git a/pkg/commands/oscommands/pty_windows.go b/pkg/commands/oscommands/pty_windows.go index e645ae627..ff707c519 100644 --- a/pkg/commands/oscommands/pty_windows.go +++ b/pkg/commands/oscommands/pty_windows.go @@ -204,7 +204,8 @@ func (p *winPty) Close() error { // slave closes on child exit, but ConPTY keeps the pipe alive until we call // ClosePseudoConsole explicitly. Without doing that on child exit, the // scanner in pkg/tasks.NewCmdTask would block forever on the next read and -// the post-content view never gets cleared (FlushStaleCells never fires). +// the render would never reach its end of input, so the new content would +// never be swapped in. func startWaiter(proc *os.Process, p *winPty) func() error { done := make(chan struct{}) var waitErr error diff --git a/pkg/gocui/gui.go b/pkg/gocui/gui.go index 2f22afe01..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 @@ -1497,7 +1497,7 @@ func (g *Gui) drawSubtitle(v *View, fgColor, bgColor Attribute) error { // drawListFooter draws the footer of a list view, showing something like '1 of 10' func (g *Gui) drawListFooter(v *View, fgColor, bgColor Attribute) error { - if len(v.lines) == 0 { + if len(v.buf.lines) == 0 { return nil } @@ -1747,13 +1747,13 @@ func (g *Gui) onKey(ev *GocuiEvent) error { if newY < 0 { newY = 0 newCy = -v.oy - } else if newY >= len(v.lines) { - newY = len(v.lines) - 1 + } else if newY >= len(v.buf.lines) { + newY = len(v.buf.lines) - 1 newCy = newY - v.oy } visibleLineWidth := 0 - for _, c := range v.lines[newY].cells { + for _, c := range v.buf.lines[newY].cells { visibleLineWidth += c.width } if visibleLineWidth < newX { @@ -1763,10 +1763,8 @@ func (g *Gui) onKey(ev *GocuiEvent) error { } if ev.Key.KeyName() == MouseLeft && (ev.Key.Mod()&ModMotion) == 0 && !v.Editable && g.openHyperlink != nil { - if newY >= 0 && newY <= len(v.viewLines)-1 && newX >= 0 && newX <= len(v.viewLines[newY].line)-1 { - if link := v.viewLines[newY].line[newX].hyperlink; link != "" { - return g.openHyperlink(link, v.name) - } + if link := v.hyperlinkAt(newX, newY); link != "" { + return g.openHyperlink(link, v.name) } } diff --git a/pkg/gocui/view.go b/pkg/gocui/view.go index 787d11475..dba71ab52 100644 --- a/pkg/gocui/view.go +++ b/pkg/gocui/view.go @@ -25,17 +25,51 @@ const ( RIGHT = 8 // view is overlapping at right edge ) +// viewBuffer holds a view's content as cells, together with the cursor and +// escape-sequence decoder state used to turn incoming bytes into those cells. +// A view normally has a single buffer (the one it displays), but bundling this +// state lets a re-render build a second, off-screen buffer and swap it in +// atomically once the new content is ready, so no reader ever sees a +// half-written buffer. +type viewBuffer struct { + // the view's content: one []cell per unwrapped line + lines []lineType + + // write cursor into lines + wx, wy int + + // decodes ESC sequences as bytes are written + ei *escapeInterpreter + + // If the last character written was a newline, we don't write it but instead + // set pendingNewline to true. If more text is written, we write the newline + // then. This avoids an extra blank line at the end of the view. + pendingNewline bool +} + // A View is a window. It maintains its own internal buffer and cursor // position. type View struct { name string - x0, y0, x1, y1 int // left top right bottom - ox, oy int // view offsets - cx, cy int // cursor position - rx, ry int // Read() offsets - wx, wy int // Write() offsets - lines []lineType // All the data + x0, y0, x1, y1 int // left top right bottom + ox, oy int // view offsets + cx, cy int // cursor position + rx, ry int // Read() offsets outMode OutputMode + + // buf bundles the view's cell buffer and the cursor / escape-parser state + // 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 @@ -74,17 +108,20 @@ type View struct { // true and viewLines to nil viewLines []viewLine - // If the last character written was a newline, we don't write it but - // instead set pendingNewline to true. If more text is written, we write the - // newline then. This is to avoid having an extra blank at the end of the view. - pendingNewline bool + // 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 - // ei is used to decode ESC sequences on Write - ei *escapeInterpreter - // Visible specifies whether the view is visible. Visible bool @@ -402,7 +439,7 @@ func (v *View) FocusPoint(cx int, cy int, scrollIntoView bool) { if scrollIntoView { height := v.InnerHeight() - v.oy = calculateNewOrigin(cy, v.oy, lineCount, height) + v.SetOriginY(calculateNewOrigin(cy, v.oy, lineCount, height)) } v.cx = cx @@ -461,7 +498,7 @@ type SearchPosition struct { } type viewLine struct { - linesX, linesY int // coordinates relative to v.lines + linesX, linesY int // coordinates relative to v.buf.lines line []cell // Colors used to extend the bg past this wrapped segment's content. @@ -470,7 +507,7 @@ type viewLine struct { trailingFillAttributes *trailingFillAttributes } -// lineType is one of v.lines: the cells of a source line, plus optional +// lineType is one of v.buf.lines: the cells of a source line, plus optional // trailingFillAttributes recording the colors used to extend the bg // past the line's content when the writer emitted '\x1b[K'. type lineType struct { @@ -536,7 +573,7 @@ func NewView(name string, x0, y0, x1, y1 int, mode OutputMode) *View { Editor: DefaultEditor, tainted: true, outMode: mode, - ei: newEscapeInterpreter(mode), + buf: &viewBuffer{ei: newEscapeInterpreter(mode)}, searcher: &searcher{}, TextArea: &TextArea{}, rangeSelectStartY: -1, @@ -547,7 +584,7 @@ func NewView(name string, x0, y0, x1, y1 int, mode OutputMode) *View { v.SelFgColor, v.SelBgColor = ColorDefault, ColorDefault v.InactiveViewSelBgColor = ColorDefault v.TitleColor, v.FrameColor = ColorDefault, ColorDefault - v.ei.screenColMax = v.InnerWidth() + v.buf.ei.screenColMax = v.InnerWidth() return v } @@ -558,7 +595,7 @@ func NewView(name string, x0, y0, x1, y1 int, mode OutputMode) *View { // content can consult this snapshot instead of reading the view's live // dimensions (which the UI thread mutates during layout). func (v *View) SetContentWidth(width int) { - v.ei.screenColMax = width + v.buf.ei.screenColMax = width } // Dimensions returns the dimensions of the View @@ -707,15 +744,8 @@ func (v *View) CursorY() int { // implement Horizontal and Vertical scrolling with just incrementing // or decrementing ox and oy. func (v *View) SetOrigin(x, y int) { - if x < 0 { - x = 0 - } - if y < 0 { - y = 0 - } - - v.ox = x - v.oy = y + v.SetOriginX(x) + v.SetOriginY(y) } func (v *View) SetOriginX(x int) { @@ -755,16 +785,16 @@ func (v *View) SetWritePos(x, y int) { y = 0 } - v.wx = x - v.wy = y + v.buf.wx = x + v.buf.wy = y // Changing the write position makes a pending newline obsolete - v.pendingNewline = false + v.buf.pendingNewline = false } // WritePos returns the current write position of the view's internal buffer. func (v *View) WritePos() (x, y int) { - return v.wx, v.wy + return v.buf.wx, v.buf.wy } // SetReadPos sets the read position of the view's internal buffer. @@ -788,56 +818,56 @@ func (v *View) ReadPos() (x, y int) { } // makeWriteable creates empty cells if required to make position (x, y) writeable. -func (v *View) makeWriteable(x, y int) { +func (b *viewBuffer) makeWriteable(x, y int) { // TODO: make this more efficient // line `y` must be index-able (that's why `<=`) - for len(v.lines) <= y { - if cap(v.lines) > len(v.lines) { - newLen := cap(v.lines) + for len(b.lines) <= y { + if cap(b.lines) > len(b.lines) { + newLen := cap(b.lines) if newLen > y { newLen = y + 1 } - v.lines = v.lines[:newLen] + b.lines = b.lines[:newLen] } else { - v.lines = append(v.lines, lineType{}) + b.lines = append(b.lines, lineType{}) } } // cell `x` need not be index-able (that's why `<`) // append should be used by `lines[y]` user if he wants to write beyond `x` - for len(v.lines[y].cells) < x { - if cap(v.lines[y].cells) > len(v.lines[y].cells) { - newLen := cap(v.lines[y].cells) + for len(b.lines[y].cells) < x { + if cap(b.lines[y].cells) > len(b.lines[y].cells) { + newLen := cap(b.lines[y].cells) if newLen > x { newLen = x } - v.lines[y].cells = v.lines[y].cells[:newLen] + b.lines[y].cells = b.lines[y].cells[:newLen] } else { - v.lines[y].cells = append(v.lines[y].cells, cell{}) + b.lines[y].cells = append(b.lines[y].cells, cell{}) } } } -// writeCells copies []cell to (v.wx, v.wy), and advances v.wx accordingly. +// writeCells copies []cell to (b.wx, b.wy), and advances b.wx accordingly. // !!! caller MUST ensure that specified location (x, y) is writeable by calling makeWriteable -func (v *View) writeCells(cells []cell) { +func (b *viewBuffer) writeCells(cells []cell) { var newLen int // use maximum len available - line := v.lines[v.wy].cells[:cap(v.lines[v.wy].cells)] - maxCopy := len(line) - v.wx + line := b.lines[b.wy].cells[:cap(b.lines[b.wy].cells)] + maxCopy := len(line) - b.wx if maxCopy < len(cells) { - copy(line[v.wx:], cells[:maxCopy]) + copy(line[b.wx:], cells[:maxCopy]) line = append(line, cells[maxCopy:]...) newLen = len(line) } else { // maxCopy >= len(cells) - copy(line[v.wx:], cells) - newLen = v.wx + len(cells) - if newLen < len(v.lines[v.wy].cells) { - newLen = len(v.lines[v.wy].cells) + copy(line[b.wx:], cells) + newLen = b.wx + len(cells) + if newLen < len(b.lines[b.wy].cells) { + newLen = len(b.lines[b.wy].cells) } } - v.lines[v.wy].cells = line[:newLen] - v.wx += len(cells) + b.lines[b.wy].cells = line[:newLen] + b.wx += len(cells) } // Write appends a byte slice into the view's internal buffer. Because @@ -854,36 +884,54 @@ 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 + // write only ever touches lines from v.buf.wy onwards, so any cached wrapping // below that stays valid. - v.firstDirtyLine = min(v.firstDirtyLine, v.wy) + v.firstDirtyLine = min(v.firstDirtyLine, v.buf.wy) v.clearHover() + v.buf.write(v, p) + + v.updateSearchPositions() +} + +// write parses p into cells and appends them to the buffer at its write cursor. +// It only touches the buffer; the View wrapper above handles display-side +// effects (tainting, hover, search). v supplies render config (Editable, colors, +// width, tab width, hyperlink auto-rendering). +func (b *viewBuffer) write(v *View, p []byte) { // Fill with empty cells, if writing outside current view buffer - v.makeWriteable(v.wx, v.wy) + b.makeWriteable(b.wx, b.wy) finishLine := func() { - v.autoRenderHyperlinksInCurrentLine() + b.autoRenderHyperlinksInCurrentLine(v) } advanceToNextLine := func() { - v.wx = 0 - v.wy++ - if v.wy >= len(v.lines) { - v.lines = append(v.lines, lineType{}) + b.wx = 0 + b.wy++ + if b.wy >= len(b.lines) { + b.lines = append(b.lines, lineType{}) } } - if v.pendingNewline { + if b.pendingNewline { advanceToNextLine() - v.ei.notifyRowAdvance() - v.pendingNewline = false + b.ei.notifyRowAdvance() + b.pendingNewline = false } until := len(p) if !v.Editable && until > 0 && p[until-1] == '\n' { - v.pendingNewline = true + b.pendingNewline = true until-- } @@ -899,26 +947,26 @@ func (v *View) write(p []byte) { case characterEquals(chr, '\n') || isCRLF(chr): finishLine() advanceToNextLine() - v.ei.notifyRowAdvance() + b.ei.notifyRowAdvance() case characterEquals(chr, '\r'): finishLine() - v.wx = 0 - v.ei.notifyColumnReset() + b.wx = 0 + b.ei.notifyColumnReset() default: - truncateLine, cells := v.parseInput(chr, width, v.wx, v.wy) - if cd, ok := v.ei.instruction.(cursorDown); ok { - v.ei.instructionRead() + truncateLine, cells := b.parseInput(v, chr, width, b.wx, b.wy) + if cd, ok := b.ei.instruction.(cursorDown); ok { + b.ei.instructionRead() for range cd.n { - v.autoRenderHyperlinksInCurrentLine() + b.autoRenderHyperlinksInCurrentLine(v) advanceToNextLine() } } if cells == nil { continue } - v.writeCells(cells) + b.writeCells(cells) if truncateLine { - v.lines[v.wy].cells = v.lines[v.wy].cells[:v.wx] + b.lines[b.wy].cells = b.lines[b.wy].cells[:b.wx] } // Soft-wrap tracking. truncateLine is true exactly when the // cells are from \x1b[K filling to end of line — ConPTY @@ -929,18 +977,16 @@ func (v *View) write(p []byte) { for _, c := range cells { totalWidth += c.width } - v.ei.notifyCellsWritten(totalWidth) + b.ei.notifyCellsWritten(totalWidth) } } } - if v.pendingNewline { + if b.pendingNewline { finishLine() } else { - v.autoRenderHyperlinksInCurrentLine() + b.autoRenderHyperlinksInCurrentLine(v) } - - v.updateSearchPositions() } // exported functions use the mutex. Non-exported functions are for internal use @@ -983,12 +1029,12 @@ var lineEndCharacters = map[string]bool{ ")": true, } -func (v *View) autoRenderHyperlinksInCurrentLine() { +func (b *viewBuffer) autoRenderHyperlinksInCurrentLine(v *View) { if !v.AutoRenderHyperLinks { return } - line := v.lines[v.wy].cells + line := b.lines[b.wy].cells start := 0 for { linkStart := findLinkStart(line[start:]) @@ -1005,7 +1051,7 @@ func (v *View) autoRenderHyperlinksInCurrentLine() { link.WriteString(line[linkEnd].chr) } for i := linkStart; i < linkEnd; i++ { - v.lines[v.wy].cells[i].hyperlink = link.String() + b.lines[b.wy].cells[i].hyperlink = link.String() } start = linkEnd } @@ -1014,13 +1060,13 @@ func (v *View) autoRenderHyperlinksInCurrentLine() { // parseInput parses char by char the input written to the View. It returns nil // while processing ESC sequences. Otherwise, it returns a cell slice that // contains the processed data. -func (v *View) parseInput(ch []byte, width int, x int, _ int) (bool, []cell) { +func (b *viewBuffer) parseInput(v *View, ch []byte, width int, x int, _ int) (bool, []cell) { cells := []cell{} truncateLine := false - isEscape, err := v.ei.parseOne(ch) + isEscape, err := b.ei.parseOne(ch) if err != nil { - for _, chr := range v.ei.characters() { + for _, chr := range b.ei.characters() { c := cell{ fgColor: v.FgColor, bgColor: v.BgColor, @@ -1029,28 +1075,28 @@ func (v *View) parseInput(ch []byte, width int, x int, _ int) (bool, []cell) { } cells = append(cells, c) } - v.ei.reset() + b.ei.reset() } else { repeatCount := 1 - if _, ok := v.ei.instruction.(eraseInLineFromCursor); ok { + if _, ok := b.ei.instruction.(eraseInLineFromCursor); ok { // Discard any old content past the cursor and record the // fill colors so draw() paints the trailing area with them. // This extends the bg to the right edge in both the // content-fits and content-wraps cases — for the latter, // the metadata is what reaches every wrapped segment past // the last word. - v.ei.instructionRead() + b.ei.instructionRead() truncateLine = true - v.lines[v.wy].trailingFillAttributes = &trailingFillAttributes{ - fg: v.ei.curFgColor, - bg: v.ei.curBgColor, + b.lines[b.wy].trailingFillAttributes = &trailingFillAttributes{ + fg: b.ei.curFgColor, + bg: b.ei.curBgColor, } return truncateLine, []cell{} - } else if cf, ok := v.ei.instruction.(cursorForward); ok { + } else if cf, ok := b.ei.instruction.(cursorForward); ok { // emit `n` space cells under the parser-tracked SGR — used // to materialize ConPTY's compressed runs of spaces (which // it emits as ECH+CUF instead of literal whitespace). - v.ei.instructionRead() + b.ei.instructionRead() repeatCount = cf.n ch = []byte{' '} width = 1 @@ -1068,9 +1114,9 @@ func (v *View) parseInput(ch []byte, width int, x int, _ int) (bool, []cell) { repeatCount = tabWidth - (x % tabWidth) } c := cell{ - fgColor: v.ei.curFgColor, - bgColor: v.ei.curBgColor, - hyperlink: v.ei.hyperlink.String(), + fgColor: b.ei.curFgColor, + bgColor: b.ei.curBgColor, + hyperlink: b.ei.hyperlink.String(), chr: string(ch), width: width, } @@ -1098,9 +1144,9 @@ func (v *View) Read(p []byte) (n int, err error) { } v.readBuffer = nil } - for v.ry < len(v.lines) { - for v.rx < len(v.lines[v.ry].cells) { - s := v.lines[v.ry].cells[v.rx].chr + for v.ry < len(v.buf.lines) { + for v.rx < len(v.buf.lines[v.ry].cells) { + s := v.buf.lines[v.ry].cells[v.rx].chr count := len(s) copy(p[offset:], s) v.rx++ @@ -1122,8 +1168,17 @@ func (v *View) Read(p []byte) (n int, err error) { // only use this if the calling function has a lock on writeMutex func (v *View) clear() { v.rewind() - v.lines = nil + 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. @@ -1164,10 +1219,10 @@ func (v *View) CopyContent(from *View) { // This is a shallow clone -- the per-row cell data is immutable once written // and stays shared, so the cost is proportional to the number of rows, not // their contents. - v.lines = slices.Clone(from.lines) + v.buf.lines = slices.Clone(from.buf.lines) v.viewLines = slices.Clone(from.viewLines) - v.ox = from.ox - v.oy = from.oy + v.SetOriginX(from.ox) + v.SetOriginY(from.oy) v.cx = from.cx v.cy = from.cy } @@ -1187,23 +1242,88 @@ func (v *View) Reset() { defer v.writeMutex.Unlock() v.rewind() - v.lines = nil + 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 -// we've reached the end of the new content to display: we need to clear the remaining -// content from the previous round. We do this by setting v.viewLines to nil so that -// we just render the new content from v.lines directly -func (v *View) FlushStaleCells() { +// 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.clearViewLines() + 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.ei.reset() - v.ei.resetScreenCursor() + v.buf.ei.reset() + v.buf.ei.resetScreenCursor() v.SetReadPos(0, 0) v.SetWritePos(0, 0) @@ -1275,14 +1395,14 @@ func (v *View) updateSearchPositions() { for _, result := range v.searcher.modelSearchResults { // This code only works when v.Wrap is false. - if result.Y >= len(v.lines) { + if result.Y >= len(v.buf.lines) { break } // If a view line exists for this line index: - if v.lines[result.Y].cells != nil { + if v.buf.lines[result.Y].cells != nil { // search this view line for the search string - positions := searchPositionsForLine(v.lines[result.Y].cells, result.Y) + positions := searchPositionsForLine(v.buf.lines[result.Y].cells, result.Y) if len(positions) > 0 { // If we found any occurrences, add them v.searcher.searchPositions = append(v.searcher.searchPositions, positions...) @@ -1335,14 +1455,14 @@ func (v *View) draw(isWindowFocused bool) { if maxX == 0 { return } - v.ox = 0 + v.SetOriginX(0) } v.refreshViewLinesIfNeeded() visibleViewLinesHeight := v.viewLineLengthIgnoringTrailingBlankLines() if v.Autoscroll && visibleViewLinesHeight > maxY { - v.oy = visibleViewLinesHeight - maxY + v.SetOriginY(visibleViewLinesHeight - maxY) } if len(v.viewLines) == 0 { @@ -1429,7 +1549,7 @@ func (v *View) refreshViewLinesIfNeeded() { } lineIdx := 0 - lines := v.lines + lines := v.buf.lines for i := range lines { line := &lines[i] @@ -1475,6 +1595,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 } @@ -1553,8 +1680,8 @@ func (v *View) BufferLines() []string { v.writeMutex.Lock() defer v.writeMutex.Unlock() - lines := make([]string, len(v.lines)) - for i, l := range v.lines { + lines := make([]string, len(v.buf.lines)) + for i, l := range v.buf.lines { lines[i] = l.cells.String() } return lines @@ -1566,7 +1693,7 @@ func (v *View) Buffer() string { v.writeMutex.Lock() defer v.writeMutex.Unlock() - return linesToString(v.lines) + return linesToString(v.buf.lines) } // ViewBufferLines returns the lines in the view's internal @@ -1586,7 +1713,7 @@ func (v *View) ViewBufferLines() []string { // LinesHeight is the count of view lines (i.e. lines excluding wrapping) func (v *View) LinesHeight() int { - return len(v.lines) + return len(v.buf.lines) } // ViewLinesHeight is the count of view lines (i.e. lines including wrapping) @@ -1617,11 +1744,11 @@ func (v *View) Line(y int) (string, bool) { return "", false } - if y < 0 || y >= len(v.lines) { + if y < 0 || y >= len(v.buf.lines) { return "", false } - return v.lines[y].cells.String(), true + return v.buf.lines[y].cells.String(), true } // Word returns a string with the word of the view's internal buffer @@ -1632,11 +1759,11 @@ func (v *View) Word(x, y int) (string, bool) { return "", false } - if x < 0 || y < 0 || y >= len(v.lines) || x >= len(v.lines[y].cells) { + if x < 0 || y < 0 || y >= len(v.buf.lines) || x >= len(v.buf.lines[y].cells) { return "", false } - str := v.lines[y].cells.String() + str := v.buf.lines[y].cells.String() nl := strings.LastIndexFunc(str[:x], indexFunc) if nl == -1 { @@ -1662,12 +1789,12 @@ func indexFunc(r rune) bool { // SetHighlight toggles highlighting of separate lines, for custom lists // or multiple selection in views. func (v *View) SetHighlight(y int, on bool) { - if y < 0 || y >= len(v.lines) { + if y < 0 || y >= len(v.buf.lines) { return } - cells := make([]cell, 0, len(v.lines[y].cells)) - for _, c := range v.lines[y].cells { + cells := make([]cell, 0, len(v.buf.lines[y].cells)) + for _, c := range v.buf.lines[y].cells { if on { c.bgColor = v.SelBgColor c.fgColor = v.SelFgColor @@ -1679,7 +1806,7 @@ func (v *View) SetHighlight(y int, on bool) { } v.tainted = true v.firstDirtyLine = min(v.firstDirtyLine, y) - v.lines[y].cells = cells + v.buf.lines[y].cells = cells v.clearHover() } @@ -1791,7 +1918,7 @@ func (v *View) SelectedLine() string { v.writeMutex.Lock() defer v.writeMutex.Unlock() - if len(v.lines) == 0 { + if len(v.buf.lines) == 0 { return "" } @@ -1803,7 +1930,7 @@ func (v *View) SelectedLines() []string { v.writeMutex.Lock() defer v.writeMutex.Unlock() - if len(v.lines) == 0 { + if len(v.buf.lines) == 0 { return nil } @@ -1818,7 +1945,7 @@ func (v *View) SelectedLines() []string { } func (v *View) lineContentAtIdx(idx int) string { - return v.lines[idx].cells.String() + return v.buf.lines[idx].cells.String() } func (v *View) SelectedPoint() (int, int) { @@ -1891,8 +2018,8 @@ func (v *View) ClearTextArea() { func (v *View) overwriteLines(y int, content string) { // break by newline, then for each line, write it, then add that erase command - v.wx = 0 - v.wy = y + v.buf.wx = 0 + v.buf.wy = y v.clearViewLines() lines := strings.ReplaceAll(content, "\n", "\x1b[K\n") @@ -1904,7 +2031,7 @@ func (v *View) overwriteLines(y int, content string) { v.writeString(lines) } -// only call this function if you don't care where v.wx and v.wy end up +// only call this function if you don't care where v.buf.wx and v.buf.wy end up func (v *View) OverwriteLines(y int, content string) { v.writeMutex.Lock() defer v.writeMutex.Unlock() @@ -1912,7 +2039,7 @@ func (v *View) OverwriteLines(y int, content string) { v.overwriteLines(y, content) } -// only call this function if you don't care where v.wx and v.wy end up +// only call this function if you don't care where v.buf.wx and v.buf.wy end up func (v *View) OverwriteLinesAndClearEverythingElse(lineCount int, y int, content string) { v.writeMutex.Lock() defer v.writeMutex.Unlock() @@ -1922,19 +2049,19 @@ func (v *View) OverwriteLinesAndClearEverythingElse(lineCount int, y int, conten v.overwriteLines(y, content) for i := range y { - v.lines[i] = lineType{} + v.buf.lines[i] = lineType{} } - for i := v.wy + 1; i < len(v.lines); i += 1 { - v.lines[i] = lineType{} + for i := v.buf.wy + 1; i < len(v.buf.lines); i += 1 { + v.buf.lines[i] = lineType{} } } func (v *View) setContentLineCount(lineCount int) { if lineCount > 0 { - v.makeWriteable(0, lineCount-1) + v.buf.makeWriteable(0, lineCount-1) } - v.lines = v.lines[:lineCount] + v.buf.lines = v.buf.lines[:lineCount] } // If the current search result is no longer visible after a scroll up, select the last search @@ -1989,7 +2116,7 @@ func (v *View) ScrollUp(amount int) { } if amount != 0 { - v.oy -= amount + v.SetOriginY(v.oy - amount) v.cy += amount v.clearHover() @@ -2001,7 +2128,7 @@ func (v *View) ScrollUp(amount int) { func (v *View) ScrollDown(amount int) { adjustedAmount := v.adjustDownwardScrollAmount(amount) if adjustedAmount > 0 { - v.oy += adjustedAmount + v.SetOriginY(v.oy + adjustedAmount) v.cy -= adjustedAmount v.clearHover() @@ -2015,7 +2142,7 @@ func (v *View) ScrollLeft(amount int) { newOx = 0 } if newOx != v.ox { - v.ox = newOx + v.SetOriginX(newOx) v.clearHover() } @@ -2023,7 +2150,7 @@ func (v *View) ScrollLeft(amount int) { // not applying any limits to this func (v *View) ScrollRight(amount int) { - v.ox += amount + v.SetOriginX(v.ox + amount) v.clearHover() } @@ -2068,7 +2195,7 @@ func (v *View) scrollMargin() int { // Returns true if the view contains a line containing the given text with the given // foreground color func (v *View) ContainsColoredText(fgColor string, text string) bool { - for _, line := range v.lines { + for _, line := range v.buf.lines { if containsColoredTextInLine(fgColor, text, line.cells) { return true } @@ -2105,6 +2232,9 @@ func (v *View) onMouseMove(x int, y int) { return } + v.writeMutex.Lock() + defer v.writeMutex.Unlock() + // newCx and newCy are relative to the view port, i.e. to the visible area of the view newCx := x - v.x0 - 1 newCy := y - v.y0 - 1 @@ -2123,6 +2253,19 @@ func (v *View) onMouseMove(x int, y int) { } } +// hyperlinkAt returns the hyperlink at the given position of the view's +// content, or an empty string if there is none. +func (v *View) hyperlinkAt(x, y int) string { + v.writeMutex.Lock() + defer v.writeMutex.Unlock() + + if y < 0 || y >= len(v.viewLines) || x < 0 || x >= len(v.viewLines[y].line) { + return "" + } + + return v.viewLines[y].line[x].hyperlink +} + func (v *View) findHyperlinkAt(x, y int) *SearchPosition { linkStr := v.viewLines[y].line[x].hyperlink if linkStr == "" { diff --git a/pkg/gocui/view_test.go b/pkg/gocui/view_test.go index 121f656d8..294f02b32 100644 --- a/pkg/gocui/view_test.go +++ b/pkg/gocui/view_test.go @@ -101,13 +101,13 @@ func TestWriteString(t *testing.T) { for _, test := range tests { v := NewView("name", 0, 0, 10, 10, OutputNormal) for _, l := range test.existingLines { - v.lines = append(v.lines, lineType{cells: stringToCells(l)}) + v.buf.lines = append(v.buf.lines, lineType{cells: stringToCells(l)}) } for _, s := range test.stringsToWrite { v.writeString(s) } var resultingLines [][]string - for _, l := range v.lines { + for _, l := range v.buf.lines { resultingLines = append(resultingLines, cellsToStrings(l.cells)) } assert.Equal(t, test.expectedLines, resultingLines) @@ -144,19 +144,115 @@ func TestAutoRenderingHyperlinks(t *testing.T) { v.writeString("htt") // No hyperlinks are generated for incomplete URLs - assert.Equal(t, "", v.lines[0].cells[0].hyperlink) + assert.Equal(t, "", v.buf.lines[0].cells[0].hyperlink) // Writing more characters to the same line makes the link complete (even // though we didn't see a newline yet) v.writeString("ps://example.com") - assert.Equal(t, "https://example.com", v.lines[0].cells[0].hyperlink) + assert.Equal(t, "https://example.com", v.buf.lines[0].cells[0].hyperlink) v.Clear() // Valid but incomplete URL v.writeString("https://exa") - assert.Equal(t, "https://exa", v.lines[0].cells[0].hyperlink) + assert.Equal(t, "https://exa", v.buf.lines[0].cells[0].hyperlink) // Writing more characters to the same fixes the link v.writeString("mple.com") - assert.Equal(t, "https://example.com", v.lines[0].cells[0].hyperlink) + 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) { @@ -233,7 +329,7 @@ func TestContainsColoredText(t *testing.T) { for j, cells := range test.lines { lines[j] = lineType{cells: cells} } - v := &View{lines: lines} + v := &View{buf: &viewBuffer{lines: lines}} assert.Equal(t, test.expected, v.ContainsColoredText(test.fgColorStr, test.text), "Test %d failed", i) } } @@ -248,8 +344,8 @@ func TestWriteCursorPositionEscape(t *testing.T) { // "a", then "skip to row 3" (i.e. one blank row), then "b". v.writeString("a\r\n\x1b[3;1Hb\r\n") - got := make([][]string, 0, len(v.lines)) - for _, l := range v.lines { + got := make([][]string, 0, len(v.buf.lines)) + for _, l := range v.buf.lines { got = append(got, cellsToStrings(l.cells)) } @@ -269,8 +365,8 @@ func TestWriteCursorPositionEscapeAcrossWrites(t *testing.T) { // ConPTY is on row 3 here; CUP to row 5 should skip exactly one row. v.writeString("c\x1b[5;1Hd\n") - got := make([][]string, 0, len(v.lines)) - for _, l := range v.lines { + got := make([][]string, 0, len(v.buf.lines)) + for _, l := range v.buf.lines { got = append(got, cellsToStrings(l.cells)) } assert.Equal(t, [][]string{ @@ -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 @@ -292,8 +413,8 @@ func TestWriteCursorForwardEscape(t *testing.T) { // "a" + ECH 5 + CUF 5 + "b" — visually "a b". v.writeString("a\x1b[5X\x1b[5Cb\n") - got := make([][]string, 0, len(v.lines)) - for _, l := range v.lines { + got := make([][]string, 0, len(v.buf.lines)) + for _, l := range v.buf.lines { got = append(got, cellsToStrings(l.cells)) } @@ -312,8 +433,8 @@ func TestWriteCursorPositionEscapeWithSoftWraps(t *testing.T) { v.writeString("abcdefghij\n") v.writeString("\x1b[4;1Hxyz\n") - got := make([][]string, 0, len(v.lines)) - for _, l := range v.lines { + got := make([][]string, 0, len(v.buf.lines)) + for _, l := range v.buf.lines { got = append(got, cellsToStrings(l.cells)) } assert.Equal(t, [][]string{ diff --git a/pkg/gui/layout.go b/pkg/gui/layout.go index bcdc0edfc..67e695f2b 100644 --- a/pkg/gui/layout.go +++ b/pkg/gui/layout.go @@ -88,7 +88,13 @@ func (gui *Gui) layout(g *gocui.Gui) error { if !view.CanScrollPastBottom { maxOriginY -= newHeight - 1 } - if oldOriginY := view.OriginY(); oldOriginY > maxOriginY { + // Don't scroll up while the view's content is still being loaded: its + // height only reflects what has been read so far, so clamping to it now + // would yank the view to the top even though more content is on the way + // (e.g. when re-rendering a diff the user was scrolled into). + manager := gui.getViewBufferManagerForView(view) + stillLoading := manager != nil && manager.IsLoading() + if oldOriginY := view.OriginY(); oldOriginY > maxOriginY && !stillLoading { view.ScrollUp(oldOriginY - maxOriginY) // the view might not have scrolled actually (if it was at the limit // already), so we need to check if it did diff --git a/pkg/gui/main_panels.go b/pkg/gui/main_panels.go index 03b7469d2..a0efcb14c 100644 --- a/pkg/gui/main_panels.go +++ b/pkg/gui/main_panels.go @@ -107,16 +107,6 @@ func (gui *Gui) allMainContextPairs() []types.MainContextPair { } func (gui *Gui) refreshMainViews(opts types.RefreshMainOpts) { - // need to reset scroll positions of all other main views - for _, pair := range gui.allMainContextPairs() { - if pair.Main != opts.Pair.Main { - pair.Main.GetView().SetOrigin(0, 0) - } - if pair.Secondary != nil && pair.Secondary != opts.Pair.Secondary { - pair.Secondary.GetView().SetOrigin(0, 0) - } - } - gui.moveMainContextPairToTop(opts.Pair) if opts.Main != nil { @@ -129,6 +119,20 @@ func (gui *Gui) refreshMainViews(opts types.RefreshMainOpts) { opts.Pair.Secondary.GetView().Clear() } + // Reset the scroll positions of all the other main views. We do this after + // moving this pair to the top (which copies the previously-shown view's + // content into the now-visible one to avoid a blank frame): resetting first + // would zero that source view's scroll before it gets copied, forcing the + // placeholder to the top instead of leaving it where the screen already was. + for _, pair := range gui.allMainContextPairs() { + if pair.Main != opts.Pair.Main { + pair.Main.GetView().SetOrigin(0, 0) + } + if pair.Secondary != nil && pair.Secondary != opts.Pair.Secondary { + pair.Secondary.GetView().SetOrigin(0, 0) + } + } + gui.splitMainPanel(opts.Secondary != nil) } diff --git a/pkg/gui/pty.go b/pkg/gui/pty.go index 719f5c348..fb7ba352e 100644 --- a/pkg/gui/pty.go +++ b/pkg/gui/pty.go @@ -72,6 +72,16 @@ func (gui *Gui) newPtyTask(view *gocui.View, cmd *exec.Cmd, prefix string) error cmd.Args = withPtyGitConfig(cmd.Args, runtime.GOOS) + // Mark the view as loading synchronously now, before the layout pass: the + // actual task is created in afterLayout (below), which runs after layout, so + // 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 { // Need to get the width and the pager command again because the layout might have diff --git a/pkg/gui/tasks_adapter.go b/pkg/gui/tasks_adapter.go index ccc0b308c..5e5295639 100644 --- a/pkg/gui/tasks_adapter.go +++ b/pkg/gui/tasks_adapter.go @@ -18,6 +18,15 @@ func (gui *Gui) newCmdTask(view *gocui.View, cmd *exec.Cmd, prefix string) error ).Debug("RunCommand") manager := gui.getManager(view) + // Mark the view as loading synchronously (before the task's goroutine runs + // 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 @@ -133,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() { @@ -150,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() @@ -158,12 +170,12 @@ func (gui *Gui) getManager(view *gocui.View) *tasks.ViewBufferManager { view.SetOrigin(0, newOriginY) } - - view.FlushStaleCells() }, 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 df7791aaf..17cfabb5f 100644 --- a/pkg/tasks/tasks.go +++ b/pkg/tasks/tasks.go @@ -4,7 +4,9 @@ import ( "bufio" "fmt" "io" + "os" "os/exec" + "strconv" "sync" "sync/atomic" "time" @@ -61,22 +63,59 @@ type ViewBufferManager struct { writer io.Writer waitingMutex deadlock.Mutex - taskIDMutex deadlock.Mutex - Log *logrus.Entry - newTaskID int + // Guards newTaskID and taskKey, which identify the most recently requested + // task. Both are written on the goroutine NewTask spawns, and taskKey is + // read from the UI thread (GetTaskKey), so neither may be touched without + // holding this. + taskIDMutex deadlock.Mutex + Log *logrus.Entry + newTaskID int // The channel by which the currently-running task is told to read more // lines (e.g. as the user scrolls). Held in an atomic because it's swapped // out as tasks come and go while ReadLines/ReadToEnd read it from the UI // thread; nil when no task is running. readLines atomic.Pointer[chan LinesToRead] taskKey string - onNewKey func() + + // Resets the view's scroll position to the top. A render whose content is + // different from what the view last showed (a different command key) calls + // this — but at its *first paint*, not when the task starts: the off-screen + // render leaves the previous content displayed until the swap, so resetting + // the origin up front would scroll that still-displayed content to the top + // before the new content replaces it. See newContentPending. + resetOrigin func() + + // Whether the content the running task is rendering differs from what the + // view is currently showing (i.e. the command key changed). Two things key + // off it: the loading indicator only takes the view over when it is set, + // since there is no point clearing content we are about to render + // identically; and the first paint that reveals the content resets the + // scroll to the top and clears it. + // + // It deliberately outlives the task that set it: a task can be stopped and + // replaced before it ever paints — a background refresh landing just after + // the user clicked a different item, say — and the replacement, which + // renders the same content and so sets nothing of its own, still has to do + // what that task was owed. + newContentPending atomic.Bool + + // Whether a command task is currently reading content into the view. While + // this is true the content is still growing, so callers (e.g. the layout) + // must not clamp the view's scroll position to the amount loaded so far. + loading atomic.Bool // beforeStart is the function that is called before starting a new task beforeStart func() 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, @@ -115,6 +154,9 @@ type LinesToRead struct { } func (self *ViewBufferManager) GetTaskKey() string { + self.taskIDMutex.Lock() + defer self.taskIDMutex.Unlock() + return self.taskKey } @@ -124,7 +166,9 @@ func NewViewBufferManager( beforeStart func(), refreshView func(), onEndOfInput func(), - onNewKey func(), + resetOrigin func(), + beginRender func(), + swapInRender func(), newGocuiTask func() gocui.Task, onUIThread func(f func()) error, ) *ViewBufferManager { @@ -134,7 +178,9 @@ func NewViewBufferManager( beforeStart: beforeStart, refreshView: refreshView, onEndOfInput: onEndOfInput, - onNewKey: onNewKey, + resetOrigin: resetOrigin, + beginRender: beginRender, + swapInRender: swapInRender, newGocuiTask: newGocuiTask, onUIThread: onUIThread, } @@ -153,6 +199,21 @@ func (self *ViewBufferManager) ReadLines(totalLines int) { } } +// IsLoading reports whether a command task is currently reading content into the +// view, meaning the content is still growing. +func (self *ViewBufferManager) IsLoading() bool { + return self.loading.Load() +} + +// StartLoading marks the view as loading content. It must be called +// synchronously when a command/pty task is started, before the task's goroutine +// runs, so that a layout pass happening in between doesn't clamp the scroll +// position to the not-yet-loaded content. It is cleared when the task reaches +// the end of its input. +func (self *ViewBufferManager) StartLoading() { + self.loading.Store(true) +} + func (self *ViewBufferManager) ReadToEnd(then func()) { if ch := self.readLines.Load(); ch != nil { readLines := *ch @@ -271,8 +332,21 @@ func (self *ViewBufferManager) NewCmdTask(start func() (Cmd, io.Reader), prefix return case <-ticker.C: loadingMutex.Lock() - if !loaded { + // Only take the view over to say "loading..." when the content coming + // is different from what's on screen. A re-render of the same content + // leaves the view showing exactly what it should already, so clearing + // it for the message and then rendering the same thing back is a + // visible flicker for nothing — and a slow re-render of unchanged + // content is common (a background refresh over a repo with submodules + // that have uncommitted changes, say). The pending flag isn't consumed + // here; the first paint still owes the scroll reset. + if !loaded && self.newContentPending.Load() { self.beforeStart() + // beforeStart cleared the previous content to show "loading...", so + // put the view back at the top for it (beforeStart doesn't touch the + // origin). The origin is view state the UI thread reads while laying + // out, so write it there. + _ = self.onUIThread(self.resetOrigin) _, _ = self.writer.Write([]byte("loading...")) self.refreshView() } @@ -297,8 +371,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: @@ -313,6 +388,36 @@ func (self *ViewBufferManager) NewCmdTask(start func() (Cmd, io.Reader), prefix // this to work out how many more lines, if any, we still need to read. linesRead := 0 + // The first paint swaps the off-screen render in to reveal the new + // content, and settles the scroll position in the same step — so the new + // content first appears already where it belongs, and no draw can land + // between the two and show it at the previous render's scroll. It happens + // once, either when we've read far enough (below) or at end of input for + // content shorter than that. Callers run it on the UI thread: it writes + // the view's origin. + painted := false + firstPaint := func() { + if painted { + return + } + painted = true + self.swapInRender() + if self.newContentPending.Swap(false) { + self.resetOrigin() + } + } + + // Set LAZYGIT_SLOW_RENDER= to sleep that long after each + // line is written to the view, stretching async loads out so the frames + // of a re-render become visible. Useful for debugging scroll/flicker + // behaviour; has no effect when the variable is unset. + var slowRenderPerLine time.Duration + if v := os.Getenv("LAZYGIT_SLOW_RENDER"); v != "" { + if ms, err := strconv.Atoi(v); err == nil { + slowRenderPerLine = time.Duration(ms) * time.Millisecond + } + } + outer: for { if stopped() { @@ -344,7 +449,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)) } @@ -353,23 +461,68 @@ 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. - // 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) + // lineChan is closed. At a genuine end of input we swap in what we + // read and finalize. But lineChan is also closed when this task has + // been stopped to make way for a newer one: stopping closes + // opts.Stop, and the scanner goroutine then closes lineChan, so the + // select above can land here instead of on the opts.Stop case. A + // stopped task is being replaced and must leave the view to the + // incoming task — swapping in its half-read buffer, clamping the + // origin, or clearing `loading` would all corrupt what that task is + // about to render. So bail out here, the same as the explicit stop + // case above. + select { + case <-opts.Stop: + callThen() + break outer + default: + } + // Genuine end of input: do the first paint now if it hasn't happened + // yet (the content was shorter than a screenful, so we never reached + // the point below), 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 — as is + // firstPaint, which also writes the origin. + _ = self.onUIThread(func() { + firstPaint() + self.onEndOfInput() + }) + // The content is fully loaded now, so it's safe again for the + // layout to clamp the scroll position to it. We deliberately + // don't clear this when stopped (rather than EOF'd), because that + // means a newer task is taking over and is still loading. + self.loading.Store(false) callThen() + // Any read requests that were queued while we were reading are + // now trivially satisfied, since we've read everything. Fire + // their callbacks instead of dropping them when we break out of + // the loop below (and nil out readLines). + drain: + for { + select { + case queued := <-readLines: + if queued.Then != nil { + queued.Then() + } + default: + break drain + } + } break outer } writeToView(append(line, '\n')) lineWrittenChan <- struct{}{} linesRead++ + if slowRenderPerLine > 0 { + time.Sleep(slowRenderPerLine) + } + 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 do the first paint + // and refresh to show it. Continue reading and refresh again at the + // end to make sure the scrollbar has the right size. + _ = self.onUIThread(firstPaint) refreshViewIfStale() } } @@ -488,20 +641,19 @@ func (self *ViewBufferManager) NewTask(f func(TaskOpts) error, key string) error return } - resetOrigin := self.GetTaskKey() != key && self.onNewKey != nil + // Note we don't reset the origin here even when the command key changed: + // that's deferred to the first paint that reveals the new content (see + // newContentPending), so the previous content — left displayed until the + // swap — doesn't visibly jump to the top before the new content appears. + // Read taskKey directly: we already hold the mutex that guards it, and + // GetTaskKey would take it again. + if self.taskKey != key && self.resetOrigin != nil { + self.newContentPending.Store(true) + } self.taskKey = key self.taskIDMutex.Unlock() - if resetOrigin { - // onNewKey resets the view's scroll origin, which is view state the - // UI thread reads while laying out and drawing, so do it there. This - // must happen after releasing taskIDMutex: it blocks until the UI - // thread runs it, and a NewTask call on the UI thread takes - // taskIDMutex, so holding it here would deadlock. - _ = self.onUIThread(self.onNewKey) - } - self.waitingMutex.Lock() // Re-check staleness after acquiring waitingMutex: a newer task diff --git a/pkg/tasks/tasks_test.go b/pkg/tasks/tasks_test.go index dec476ba4..b15b48ee4 100644 --- a/pkg/tasks/tasks_test.go +++ b/pkg/tasks/tasks_test.go @@ -7,11 +7,13 @@ import ( "reflect" "strings" "sync" + "sync/atomic" "testing" "time" "github.com/jesseduffield/lazygit/pkg/gocui" "github.com/jesseduffield/lazygit/pkg/utils" + "github.com/stretchr/testify/assert" ) func getCounter() (func(), func() int) { @@ -24,7 +26,9 @@ func TestNewCmdTaskInstantStop(t *testing.T) { beforeStart, getBeforeStartCallCount := getCounter() refreshView, getRefreshViewCallCount := getCounter() onEndOfInput, getOnEndOfInputCallCount := getCounter() - onNewKey, getOnNewKeyCallCount := getCounter() + resetOrigin, getResetOriginCallCount := getCounter() + beginRender, getBeginRenderCallCount := getCounter() + swapInRender, getSwapInRenderCallCount := getCounter() onDone, getOnDoneCallCount := getCounter() task := gocui.NewFakeTask() newTask := func() gocui.Task { @@ -37,7 +41,9 @@ func TestNewCmdTaskInstantStop(t *testing.T) { beforeStart, refreshView, onEndOfInput, - onNewKey, + resetOrigin, + beginRender, + swapInRender, newTask, // no UI thread in the test; run the view mutations inline func(f func()) error { f(); return nil }, @@ -66,7 +72,9 @@ func TestNewCmdTaskInstantStop(t *testing.T) { {0, getBeforeStartCallCount(), "beforeStart"}, {1, getRefreshViewCallCount(), "refreshView"}, {0, getOnEndOfInputCallCount(), "onEndOfInput"}, - {0, getOnNewKeyCallCount(), "onNewKey"}, + {0, getResetOriginCallCount(), "resetOrigin"}, + {0, getBeginRenderCallCount(), "beginRender"}, + {0, getSwapInRenderCallCount(), "swapInRender"}, {1, getOnDoneCallCount(), "onDone"}, } for _, expectation := range callCountExpectations { @@ -91,7 +99,9 @@ func TestNewCmdTask(t *testing.T) { beforeStart, getBeforeStartCallCount := getCounter() refreshView, getRefreshViewCallCount := getCounter() onEndOfInput, getOnEndOfInputCallCount := getCounter() - onNewKey, getOnNewKeyCallCount := getCounter() + resetOrigin, getResetOriginCallCount := getCounter() + beginRender, getBeginRenderCallCount := getCounter() + swapInRender, getSwapInRenderCallCount := getCounter() onDone, getOnDoneCallCount := getCounter() task := gocui.NewFakeTask() newTask := func() gocui.Task { @@ -104,7 +114,9 @@ func TestNewCmdTask(t *testing.T) { beforeStart, refreshView, onEndOfInput, - onNewKey, + resetOrigin, + beginRender, + swapInRender, newTask, // no UI thread in the test; run the view mutations inline func(f func()) error { f(); return nil }, @@ -134,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"}, + {0, getResetOriginCallCount(), "resetOrigin"}, + {1, getBeginRenderCallCount(), "beginRender"}, + {1, getSwapInRenderCallCount(), "swapInRender"}, {1, getOnDoneCallCount(), "onDone"}, } for _, expectation := range callCountExpectations { @@ -174,6 +188,206 @@ func (d *BlankLineReader) Read(p []byte) (n int, err error) { return 1, nil } +// A dummy reader that yields the given number of blank lines and then blocks +// until unblock is closed, at which point it reports EOF. This lets a test hold +// a task in its "still loading" state for as long as it needs to. +type BlockingLineReader struct { + linesToYield int + linesYielded int + reachedEnd bool + blocked chan struct{} + unblock chan struct{} +} + +func (d *BlockingLineReader) Read(p []byte) (n int, err error) { + if d.linesYielded == d.linesToYield { + if !d.reachedEnd { + d.reachedEnd = true + close(d.blocked) + } + <-d.unblock + return 0, io.EOF + } + + d.linesYielded++ + p[0] = '\n' + return 1, nil +} + +func TestNewCmdTaskQueuedReadAtEndOfInput(t *testing.T) { + writer := bytes.NewBuffer(nil) + task := gocui.NewFakeTask() + + manager := NewViewBufferManager( + utils.NewDummyLog(), + writer, + func() {}, + 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 }, + ) + + reader := BlockingLineReader{ + linesToYield: 5, + blocked: make(chan struct{}), + unblock: make(chan struct{}), + } + start := func() (Cmd, io.Reader) { + // not actually starting this because it's not necessary + return ExecCmd{Cmd: exec.Command("blah")}, &reader + } + + // The initial request asks for far more lines than the reader has, so the + // task reaches EOF while that request is still the one being served. + fn := manager.NewCmdTask(start, "", LinesToRead{100, -1, nil}, func() {}) + + thenCalled := false + wg := sync.WaitGroup{} + wg.Go(func() { + _ = fn(TaskOpts{Stop: make(chan struct{}), InitialContentLoaded: func() { task.Done() }}) + }) + + <-reader.blocked + manager.ReadToEnd(func() { thenCalled = true }) + // ReadToEnd queues its request from a goroutine; wait for it to land so that + // it is definitely outstanding by the time we let the task reach EOF. + for len(*manager.readLines.Load()) == 0 { + time.Sleep(time.Millisecond) + } + close(reader.unblock) + + wg.Wait() + + assert.True(t, thenCalled) +} + +// A task rendering content the view wasn't already showing resets the scroll +// position to the top, at its first paint. If it is stopped and replaced before +// it ever paints — a background refresh landing just after the user clicked a +// different item, say — the replacement renders the same content and so decides +// on no reset of its own; it has to perform the one the stopped task was owed, +// or the view keeps the scroll position of the content it showed before. +func TestResetOriginSurvivesTaskReplacement(t *testing.T) { + resetOrigin, getResetOriginCallCount := getCounter() + + manager := NewViewBufferManager( + utils.NewDummyLog(), + bytes.NewBuffer(nil), + func() {}, + func() {}, + func() {}, + resetOrigin, + 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 }, + ) + + startTask := func(key string, reader io.Reader, onDone func()) { + start := func() (Cmd, io.Reader) { + // not actually starting this because it's not necessary + return ExecCmd{Cmd: exec.Command("blah")}, reader + } + // The first-paint point is far beyond what any of these readers yield, so + // only reaching EOF paints. + _ = manager.NewTask(manager.NewCmdTask(start, "", LinesToRead{100, 50, nil}, onDone), key) + } + runTaskToCompletion := func(key string) { + done := make(chan struct{}) + startTask(key, &BlankLineReader{totalLinesToYield: 3}, func() { close(done) }) + <-done + } + + // A render of content the view wasn't showing resets the scroll position. + runTaskToCompletion("cmd1") + assert.Equal(t, 1, getResetOriginCallCount()) + + // Different content again, but this task stalls before it can paint. + stalled := BlockingLineReader{ + linesToYield: 3, + blocked: make(chan struct{}), + unblock: make(chan struct{}), + } + defer close(stalled.unblock) + startTask("cmd2", &stalled, nil) + <-stalled.blocked + + // The replacement shows the same content as the stalled task, so it has no + // reset of its own to do — but it must still do that task's. + runTaskToCompletion("cmd2") + assert.Equal(t, 2, getResetOriginCallCount()) +} + +// A render that takes long enough to start takes the view over to say +// "loading...", which means blanking whatever it was showing. That is only worth +// doing when the content coming is different from what's on screen: re-rendering +// the same content (a background refresh, say) would otherwise blank the view and +// paint the same thing back, a visible flicker for nothing. +func TestLoadingIndicatorOnlyTakesOverForNewContent(t *testing.T) { + var beforeStartCount atomic.Int32 + + manager := NewViewBufferManager( + utils.NewDummyLog(), + io.Discard, + func() { beforeStartCount.Add(1) }, + 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 }, + ) + + startTask := func(key string, reader io.Reader, onDone func()) { + start := func() (Cmd, io.Reader) { + // not actually starting this because it's not necessary + return ExecCmd{Cmd: exec.Command("blah")}, reader + } + _ = manager.NewTask(manager.NewCmdTask(start, "", LinesToRead{100, 50, nil}, onDone), key) + } + // Starts a task whose command produces nothing at all, so that it is still + // waiting for its first line when the loading indicator falls due. Returns + // the reader so the caller can let it finish. + startStalledTask := func(key string) *BlockingLineReader { + reader := &BlockingLineReader{ + blocked: make(chan struct{}), + unblock: make(chan struct{}), + } + startTask(key, reader, nil) + <-reader.blocked + return reader + } + + // Get some content on screen first: the indicator is only due when a render + // is slow, and this one isn't. + done := make(chan struct{}) + startTask("cmd1", &BlankLineReader{totalLinesToYield: 3}, func() { close(done) }) + <-done + assert.EqualValues(t, 0, beforeStartCount.Load()) + + // A slow re-render of that same content must leave the view alone however + // long it takes. The indicator is due 200ms in, so give it well past that. + sameContent := startStalledTask("cmd1") + defer close(sameContent.unblock) + time.Sleep(500 * time.Millisecond) + assert.EqualValues(t, 0, beforeStartCount.Load()) + + // Different content, though, is worth taking the view over for. + newContent := startStalledTask("cmd2") + defer close(newContent.unblock) + assert.Eventually(t, + func() bool { return beforeStartCount.Load() == 1 }, + 2*time.Second, 10*time.Millisecond) +} + func TestNewCmdTaskRefresh(t *testing.T) { type scenario struct { name string @@ -240,6 +454,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 },