diff --git a/pkg/gui/controllers/helpers/staging_helper.go b/pkg/gui/controllers/helpers/staging_helper.go index 2d0b78761..ac0894969 100644 --- a/pkg/gui/controllers/helpers/staging_helper.go +++ b/pkg/gui/controllers/helpers/staging_helper.go @@ -285,18 +285,21 @@ func (self *StagingHelper) restoreDiffLinePositionOnRerender(view *gocui.View, c } return view.OffscreenLineCount() >= primaryBufferLine+view.InnerHeight() }, - Apply: func() { - // The off-screen render has just been swapped in, so the displayed buffer - // now holds it. Land on the nearest candidate it still contains: the - // primary one if the incremental scan found it, otherwise scan the now- - // complete content in priority order (the common path for buffer-parse, - // which only becomes well-formed once the whole diff has loaded, at which - // point the swap happens at end of input). + Apply: func(swapIn func()) { + // Find the candidate to land on, then swap the off-screen content in and + // place it. The find runs against the *off-screen* buffer, before the swap, + // so the (possibly whole-diff) scan happens while the previous content is + // still displayed — otherwise the new content would be drawn at the old + // scroll for the duration of the scan. Land on the nearest candidate the + // content still contains: the primary one if the incremental scan found it, + // otherwise scan the now-complete content in priority order (the common path + // for buffer-parse, which only becomes well-formed once the whole diff has + // loaded, so its swap is at end of input and the off-screen buffer is whole). matched, bufferLine := -1, primaryBufferLine if bufferLine != -1 { matched = 0 } else { - resolved := self.resolveDiffLines(view.DiffLineContents()) + resolved := self.resolveDiffLines(view.OffscreenDiffLineContents()) for i, candidate := range candidates { if line := findResolvedDiffLine(resolved, candidate.identity, 0); line != -1 { matched, bufferLine = i, line @@ -304,6 +307,9 @@ func (self *StagingHelper) restoreDiffLinePositionOnRerender(view *gocui.View, c } } } + + swapIn() + // If no candidate is there (the content changed and they're all gone), // leave the scroll and selection as they are rather than acting on a line // that no longer means what it did. diff --git a/pkg/tasks/tasks.go b/pkg/tasks/tasks.go index a4e0f4334..d31f31f3b 100644 --- a/pkg/tasks/tasks.go +++ b/pkg/tasks/tasks.go @@ -171,20 +171,22 @@ type LinesToRead struct { // re-renders content the user was already looking at (returning to a focused main // view on escape). The render task reads the new content into an off-screen // buffer; RenderRestore decides, as that buffer fills, when the task has read far -// enough to show the saved position (FirstPaintReady), and then — once the -// off-screen buffer has been swapped in — scrolls there and restores the -// selection (Apply). It is a predicate rather than a fixed scroll position so the -// target can be a row matching a patch identity, located by scanning the loading -// content, rather than a line number that the changed content may have moved. +// enough to show the saved position (FirstPaintReady), and then locates the target +// and reveals it (Apply). It is a predicate rather than a fixed scroll position so +// the target can be a row matching a patch identity, located by scanning the +// loading content, rather than a line number that the changed content may have moved. type RenderRestore struct { // FirstPaintReady reports whether the task has now read enough of the new // (off-screen) content to first-paint at the saved position. Evaluated after // each line is read. FirstPaintReady func() bool - // Apply runs once, just after the off-screen render is swapped in at the first - // paint, to scroll to the saved position and restore the selection. - Apply func() + // Apply runs once at the first paint. It locates the target in the not-yet- + // displayed off-screen content, then calls swapIn to promote that content to + // the display and settles the scroll/selection onto the target. Doing the + // (potentially whole-diff) scan before swapIn keeps the previous content shown + // while it runs, so the new content is never briefly drawn at the old scroll. + Apply func(swapIn func()) } func (self *ViewBufferManager) GetTaskKey() string { @@ -464,11 +466,17 @@ func (self *ViewBufferManager) NewCmdTask(start func() (Cmd, io.Reader), prefix return } painted = true - self.swapInRender() if restore != nil { - restore.Apply() - } else if linesToRead.ResetOrigin && self.resetOrigin != nil { - self.resetOrigin() + // Apply locates the target in the off-screen content, swaps it in, + // and settles the scroll in one step — so the scan it may run first + // happens while the previous content is still displayed, never the + // new content at the old scroll. + restore.Apply(self.swapInRender) + } else { + self.swapInRender() + if linesToRead.ResetOrigin && self.resetOrigin != nil { + self.resetOrigin() + } } } diff --git a/pkg/tasks/tasks_test.go b/pkg/tasks/tasks_test.go index 17daf7f35..8dd0c7f7c 100644 --- a/pkg/tasks/tasks_test.go +++ b/pkg/tasks/tasks_test.go @@ -174,9 +174,10 @@ func TestNewCmdTask(t *testing.T) { // thing we want to do with the output is count the number of lines. // When a RenderRestore is set, the first paint is driven by its FirstPaintReady // predicate rather than the InitialRefreshAfter line count, and Apply runs exactly -// once, right after the off-screen render is swapped in. This is the read-loop -// half of the escape restore: scroll to and select the saved position as the new -// content first appears. See RenderRestore. +// once. Apply controls when the off-screen render is swapped in (it calls swapIn +// after locating the target), so its scan runs while the previous content is still +// displayed rather than revealing the new content at the old scroll. This is the +// read-loop half of the escape restore. See RenderRestore. func TestNewCmdTaskRestore(t *testing.T) { writer := bytes.NewBuffer(nil) linesWritten := func() int { return strings.Count(writer.String(), "\n") } @@ -184,7 +185,8 @@ func TestNewCmdTaskRestore(t *testing.T) { swapped := false applyCount := 0 applyAtLines := -1 - applyAfterSwap := true + swappedBeforeApply := false + swappedByApply := false task := gocui.NewFakeTask() manager := NewViewBufferManager( @@ -204,12 +206,15 @@ func TestNewCmdTaskRestore(t *testing.T) { restore := &RenderRestore{ // Ready once five lines have loaded — well before InitialRefreshAfter (30). FirstPaintReady: func() bool { return linesWritten() >= 5 }, - Apply: func() { + Apply: func(swapIn func()) { applyCount++ applyAtLines = linesWritten() - if !swapped { - applyAfterSwap = false + // The render must not be swapped in until Apply asks for it. + if swapped { + swappedBeforeApply = true } + swapIn() + swappedByApply = swapped }, } @@ -230,7 +235,8 @@ func TestNewCmdTaskRestore(t *testing.T) { wg.Wait() assert.Equal(t, 1, applyCount, "Apply should run exactly once") - assert.True(t, applyAfterSwap, "Apply should run after the off-screen render is swapped in") + assert.False(t, swappedBeforeApply, "the off-screen render should not be swapped in before Apply runs") + assert.True(t, swappedByApply, "Apply should swap the off-screen render in via swapIn") // The first paint was driven by FirstPaintReady (>=5 lines), not by // InitialRefreshAfter (30). assert.GreaterOrEqual(t, applyAtLines, 5)