diff --git a/pkg/gui/pty.go b/pkg/gui/pty.go index 2ee234997..97a94aa58 100644 --- a/pkg/gui/pty.go +++ b/pkg/gui/pty.go @@ -158,10 +158,14 @@ func (gui *Gui) newPtyTask(view *gocui.View, cmd *exec.Cmd, prefix string) error // focused main view on escape), let the task re-establish the scroll // position and selection as it first paints, reading to end of input so a // deep target line is found and the scrollbar ends up accurate. - if restore := manager.GetRestoreForNextTask(); restore != nil { + restore := manager.GetRestoreForNextTask() + if restore != nil { linesToRead.Restore = restore linesToRead.Total = -1 } + // New content scrolls back to the top at the first paint; same content keeps + // its scroll, and a restore places the scroll itself. See LinesToRead.ResetOrigin. + linesToRead.ResetOrigin = restore == nil && cmdStr != manager.GetTaskKey() return manager.NewTask(manager.NewCmdTask(start, prefix, linesToRead, onClose), cmdStr) }) diff --git a/pkg/gui/tasks_adapter.go b/pkg/gui/tasks_adapter.go index ed3ca3387..0640b2482 100644 --- a/pkg/gui/tasks_adapter.go +++ b/pkg/gui/tasks_adapter.go @@ -56,10 +56,15 @@ func (gui *Gui) newCmdTask(view *gocui.View, cmd *exec.Cmd, prefix string) error // on escape), let the task re-establish the scroll position and selection as // it first paints. It also reads to end of input so a deep target line is // found and the scrollbar ends up accurate. See RenderRestore. - if restore := manager.GetRestoreForNextTask(); restore != nil { + restore := manager.GetRestoreForNextTask() + if restore != nil { linesToRead.Restore = restore linesToRead.Total = -1 } + // New content (a different command than the view last showed) scrolls back to + // the top, at the first paint; same content keeps its scroll, and a restore + // places the scroll itself. See LinesToRead.ResetOrigin. + linesToRead.ResetOrigin = restore == nil && cmdStr != manager.GetTaskKey() if err := manager.NewTask(manager.NewCmdTask(start, prefix, linesToRead, onClose), cmdStr); err != nil { gui.c.Log.Error(err) } diff --git a/pkg/tasks/tasks.go b/pkg/tasks/tasks.go index e1807de25..a4e0f4334 100644 --- a/pkg/tasks/tasks.go +++ b/pkg/tasks/tasks.go @@ -72,7 +72,14 @@ type ViewBufferManager struct { // 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 LinesToRead.ResetOrigin. + resetOrigin func() // When non-nil, the next cmd/pty task re-establishes the view's scroll // position and selection once it has re-rendered the content (returning to a @@ -147,6 +154,15 @@ type LinesToRead struct { // RenderRestore. Restore *RenderRestore + // When true, the view's scroll position is reset to the top at the first paint + // (the swap), because this render's content differs from what the view last + // showed. Set for a new command key with no Restore; left false when + // re-rendering the same content (keep the scroll) or when a Restore will place + // the scroll itself. Doing it at the first paint rather than at task start + // keeps the previous content, still displayed off-screen until the swap, from + // visibly jumping to the top first. + ResetOrigin bool + // Function to call after reading the lines is done Then func() } @@ -181,7 +197,7 @@ func NewViewBufferManager( beforeStart func(), refreshView func(), onEndOfInput func(), - onNewKey func(), + resetOrigin func(), beginRender func(), swapInRender func(), newGocuiTask func() gocui.Task, @@ -193,7 +209,7 @@ func NewViewBufferManager( beforeStart: beforeStart, refreshView: refreshView, onEndOfInput: onEndOfInput, - onNewKey: onNewKey, + resetOrigin: resetOrigin, beginRender: beginRender, swapInRender: swapInRender, newGocuiTask: newGocuiTask, @@ -385,6 +401,13 @@ func (self *ViewBufferManager) NewCmdTask(start func() (Cmd, io.Reader), prefix loadingMutex.Lock() if !loaded { self.beforeStart() + // beforeStart cleared the previous content to show "loading...". If + // this is new content, reset the scroll to the top now so the message + // is visible (beforeStart doesn't touch the origin); for a same-content + // re-render we keep the scroll, as the first paint will too. + if linesToRead.ResetOrigin && self.resetOrigin != nil { + self.resetOrigin() + } _, _ = self.writer.Write([]byte("loading...")) self.refreshView() } @@ -426,12 +449,14 @@ func (self *ViewBufferManager) NewCmdTask(start func() (Cmd, io.Reader), prefix linesRead := 0 // The first paint swaps the off-screen render in to reveal the new - // content. When a restore is pending (returning to a focused main view on - // escape, see RenderRestore), it scrolls to the saved position and - // restores the selection in the same step, so the real content first - // appears already at the right place rather than at the top. firstPaint - // happens once, either when we've read far enough (below) or at end of - // input for content shorter than that. + // content, and settles the scroll position in the same step — so the new + // content first appears already where it belongs. A restore (returning to + // a focused main view on escape, see RenderRestore) scrolls to its saved + // position and restores the selection; otherwise, if this is new content + // (ResetOrigin), the scroll resets to the top. Either way it happens at + // the swap, not before, so the previous content — displayed until the swap + // — doesn't visibly jump first. firstPaint happens once, either when we've + // read far enough (below) or at end of input for content shorter than that. restore := linesToRead.Restore painted := false firstPaint := func() { @@ -442,6 +467,8 @@ func (self *ViewBufferManager) NewCmdTask(start func() (Cmd, io.Reader), prefix self.swapInRender() if restore != nil { restore.Apply() + } else if linesToRead.ResetOrigin && self.resetOrigin != nil { + self.resetOrigin() } } @@ -694,31 +721,18 @@ func (self *ViewBufferManager) NewTask(f func(TaskOpts) error, key string) error return } - // Reset the origin to the top when the command changed, unless a restore is - // pending: a restore re-establishes the scroll position itself (and we keep - // the placeholder showing at its current scroll until it does), so resetting - // to the top would just flicker. The restore isn't cleared here: it must - // outlive a task being stopped and replaced before it could paint, and it - // validates itself against the content it lands in (see restoreForNextTask), - // so a stale one can't apply to the wrong place — it's cleared once a task - // has applied it. - resetOrigin := self.GetTaskKey() != key && self.onNewKey != nil && self.restoreForNextTask == nil + // Note we don't reset the origin here even when the command key changed: + // that's deferred to the task's first paint (see resetOrigin / the task's + // ResetOrigin), so the previous content — left displayed off-screen until + // the swap — doesn't visibly jump to the top before the new content appears. + // The restore isn't cleared here either: it must outlive a task being stopped + // and replaced before it could paint, and it validates itself against the + // content it lands in (see restoreForNextTask), so a stale one can't apply to + // the wrong place — it's cleared once a task has applied it. 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(func() error { - self.onNewKey() - return nil - }) - } - self.waitingMutex.Lock() // Re-check staleness after acquiring waitingMutex: a newer task