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/pty.go b/pkg/gui/pty.go index 719f5c348..1cb8d9412 100644 --- a/pkg/gui/pty.go +++ b/pkg/gui/pty.go @@ -72,6 +72,12 @@ 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() + // 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..440bc0aab 100644 --- a/pkg/gui/tasks_adapter.go +++ b/pkg/gui/tasks_adapter.go @@ -18,6 +18,10 @@ 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() // 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 diff --git a/pkg/tasks/tasks.go b/pkg/tasks/tasks.go index dec232189..a2084ea36 100644 --- a/pkg/tasks/tasks.go +++ b/pkg/tasks/tasks.go @@ -78,6 +78,11 @@ type ViewBufferManager struct { taskKey string onNewKey func() + // 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() @@ -162,6 +167,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 @@ -379,6 +399,11 @@ func (self *ViewBufferManager) NewCmdTask(start func() (Cmd, io.Reader), prefix // whether to scroll) and sets the origin, both of which // are UI-thread-only, so run it there. _ = self.onUIThread(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