diff --git a/pkg/tasks/tasks.go b/pkg/tasks/tasks.go index a2084ea36..0fa04eb3e 100644 --- a/pkg/tasks/tasks.go +++ b/pkg/tasks/tasks.go @@ -78,6 +78,13 @@ type ViewBufferManager struct { taskKey string onNewKey func() + // Whether the content the running task is rendering differs from what the + // view is currently showing (i.e. the command key changed). The loading + // indicator only takes the view over when it is set: there is no point + // clearing content we are about to render identically. Cleared once the + // task has rendered enough for the view to be showing the new content. + 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. @@ -300,7 +307,14 @@ 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). + if !loaded && self.newContentPending.Load() { self.beforeStart() _, _ = self.writer.Write([]byte("loading...")) self.refreshView() @@ -399,6 +413,8 @@ 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) + // Whatever there was to show is on screen now. + self.newContentPending.Store(false) // 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 @@ -434,6 +450,7 @@ func (self *ViewBufferManager) NewCmdTask(start func() (Cmd, io.Reader), prefix // 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. + self.newContentPending.Store(false) refreshViewIfStale() } } @@ -554,7 +571,11 @@ func (self *ViewBufferManager) NewTask(f func(TaskOpts) error, key string) error // Read taskKey directly: we already hold the mutex that guards it, and // GetTaskKey would take it again. - resetOrigin := self.taskKey != key && self.onNewKey != nil + newContent := self.taskKey != key + if newContent { + self.newContentPending.Store(true) + } + resetOrigin := newContent && self.onNewKey != nil self.taskKey = key self.taskIDMutex.Unlock() diff --git a/pkg/tasks/tasks_test.go b/pkg/tasks/tasks_test.go index d9ec7e4d4..ad4469043 100644 --- a/pkg/tasks/tasks_test.go +++ b/pkg/tasks/tasks_test.go @@ -7,6 +7,7 @@ import ( "reflect" "strings" "sync" + "sync/atomic" "testing" "time" @@ -251,6 +252,80 @@ func TestNewCmdTaskQueuedReadAtEndOfInput(t *testing.T) { assert.True(t, thenCalled) } +// A writer that records whether the loading indicator was ever written to it. +type LoadingIndicatorSpy struct { + sawLoadingIndicator atomic.Bool +} + +func (self *LoadingIndicatorSpy) Write(p []byte) (n int, err error) { + if bytes.Contains(p, []byte("loading...")) { + self.sawLoadingIndicator.Store(true) + } + return len(p), nil +} + +// A render that takes long enough to produce its first line takes the view over +// to say "loading...", which means clearing 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 would otherwise clear the view and render the +// same thing straight back, a visible flicker for nothing. +func TestLoadingIndicatorOnlyTakesOverForNewContent(t *testing.T) { + writer := &LoadingIndicatorSpy{} + + manager := NewViewBufferManager( + utils.NewDummyLog(), + writer, + 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.False(t, writer.sawLoadingIndicator.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.False(t, writer.sawLoadingIndicator.Load()) + + // Different content, though, is worth taking the view over for. + newContent := startStalledTask("cmd2") + defer close(newContent.unblock) + assert.Eventually(t, + writer.sawLoadingIndicator.Load, + 2*time.Second, 10*time.Millisecond) +} + func TestNewCmdTaskRefresh(t *testing.T) { type scenario struct { name string