diff --git a/pkg/gui/controllers/helpers/patch_building_helper.go b/pkg/gui/controllers/helpers/patch_building_helper.go index 25335e2cc..0d5bc4119 100644 --- a/pkg/gui/controllers/helpers/patch_building_helper.go +++ b/pkg/gui/controllers/helpers/patch_building_helper.go @@ -61,15 +61,34 @@ func EscapeFromPatchExplorer(c *HelperCommon, context types.IPatchExplorerContex view := snapshot.MainView.GetView() - // Ask the upcoming re-render to restore the scroll position. Pushing the side - // panel re-renders its content into the main view via a cmd/pty task. Until - // that content is ready, the main view keeps showing the placeholder that - // CopyContent left in it (the view we're leaving) at its current scroll; the - // task then scrolls to the saved position as part of the first paint that - // shows the real content. Doing it this way (rather than setting the origin up - // front) avoids both a jump to the top and a misplaced placeholder frame. - if manager := c.GetViewBufferManagerForView(view); manager != nil { + restore := func() { + view.FocusPoint(0, snapshot.SelectedLineIdx, false) + view.Highlight = true + view.HighlightInactive = false + } + + // Ask the upcoming re-render to restore the scroll position and selection. + // Pushing the side panel re-renders its content into the main view via a + // cmd/pty task. Until that content is ready, the main view keeps showing the + // placeholder that CopyContent left in it (the view we're leaving) at its + // current scroll; the task then scrolls to the saved position as part of the + // first paint that shows the real content (rather than setting the origin up + // front, which would jump to the top or show a misplaced placeholder frame). + // The selection needs the content loaded down to the selected line, so it + // rides the same task and fires at the end of its initial read. Threading it + // through the task (rather than a ReadToEnd issued after the pushes) avoids a + // race: ReadToEnd fires synchronously when the freshly-created task's read + // channel isn't live yet, which would run FocusPoint before the content is + // loaded and silently drop the selection. + manager := c.GetViewBufferManagerForView(view) + if manager != nil { manager.ScrollToOriginYForNextTask(snapshot.OriginY) + manager.ThenForNextTask(func() { + c.OnUIThread(func() error { + restore() + return nil + }) + }) } // Land on the side panel first (this re-renders the original content into the @@ -77,31 +96,10 @@ func EscapeFromPatchExplorer(c *HelperCommon, context types.IPatchExplorerContex c.Context().Push(snapshot.SidePanel, types.OnFocusOpts{}) c.Context().Push(snapshot.MainView, types.OnFocusOpts{}) - restore := func() { - view.FocusPoint(0, snapshot.SelectedLineIdx, false) - view.Highlight = true - view.HighlightInactive = false + // Without a buffer manager there is no re-render task to ride, so restore now. + if manager == nil { + restore() } - - // The scroll position is handled by the re-render above, but the selection - // still needs the content loaded down to the selected line, which happens - // asynchronously. Wait until the diff has been fully read before restoring - // it. We do this on the next UI tick, by which point the re-render task is - // live and ReadToEnd can hook into it. - c.OnUIThread(func() error { - manager := c.GetViewBufferManagerForView(view) - if manager == nil { - restore() - return nil - } - manager.ReadToEnd(func() { - c.OnUIThread(func() error { - restore() - return nil - }) - }) - return nil - }) } // kills the custom patch and returns us back to the commit files panel if needed diff --git a/pkg/gui/pty.go b/pkg/gui/pty.go index f6b471eb7..b3ca1b7f4 100644 --- a/pkg/gui/pty.go +++ b/pkg/gui/pty.go @@ -155,6 +155,10 @@ func (gui *Gui) newPtyTask(view *gocui.View, cmd *exec.Cmd, prefix string) error } linesToRead := gui.linesToReadFromCmdTask(view, targetOriginY) + // As in newCmdTask: let the task run any requested after-load callback at + // the end of its initial read (e.g. restoring a focused main view's + // selection on escape). The task clears the request when it starts. + linesToRead.Then = manager.GetThenForNextTask() 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 38543cd37..699715dca 100644 --- a/pkg/gui/tasks_adapter.go +++ b/pkg/gui/tasks_adapter.go @@ -52,6 +52,11 @@ func (gui *Gui) newCmdTask(view *gocui.View, cmd *exec.Cmd, prefix string) error } linesToRead := gui.linesToReadFromCmdTask(view, targetOriginY) + // If a caller asked us to run something once this re-render has loaded (e.g. + // restoring a focused main view's selection on escape), let the task own it, + // firing at the end of its initial read. The task clears the request when it + // starts. + linesToRead.Then = manager.GetThenForNextTask() 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 083519a8f..4f40adb0b 100644 --- a/pkg/tasks/tasks.go +++ b/pkg/tasks/tasks.go @@ -90,6 +90,19 @@ type ViewBufferManager struct { // Cleared once the next task has started. scrollToOriginYForNextTask *int + // When non-nil, run once after the next cmd/pty task has read enough of its + // content (the end of its initial read). Used to restore state that depends + // on the re-rendered content being loaded — e.g. the selection in a focused + // main view we're returning to on escape — by riding the task's own + // lifecycle rather than a separate post-hoc ReadToEnd. A ReadToEnd issued + // right after triggering the re-render can fire synchronously, because the + // freshly-created task's read channel isn't live yet (it is created inside + // the task goroutine, after the previous task has been stopped); that would + // run the restore before any content is loaded and silently drop it. + // + // Cleared once the next task has started. + thenForNextTask 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. @@ -198,6 +211,22 @@ func (self *ViewBufferManager) GetScrollToOriginYForNextTask() *int { return self.scrollToOriginYForNextTask } +// ThenForNextTask makes the next cmd/pty task run the given function once it has +// read enough of its content (the end of its initial read). Call this right +// before triggering a re-render whose loaded content the function depends on. +// See the field doc for why this is preferable to a separate ReadToEnd. It is +// cleared once the next task starts. +func (self *ViewBufferManager) ThenForNextTask(then func()) { + self.thenForNextTask = then +} + +// GetThenForNextTask returns the function requested by a preceding +// ThenForNextTask call, or nil if none. It does not clear it; the task clears it +// when it starts. +func (self *ViewBufferManager) GetThenForNextTask() func() { + return self.thenForNextTask +} + // IsLoading reports whether a command task is currently reading content into the // view, meaning the content is still growing. func (self *ViewBufferManager) IsLoading() bool { @@ -610,6 +639,7 @@ func (self *ViewBufferManager) NewTask(f func(TaskOpts) error, key string) error // saved position as part of its first paint (see scrollToOriginYForNextTask). resetOrigin := self.GetTaskKey() != key && self.onNewKey != nil && self.scrollToOriginYForNextTask == nil self.scrollToOriginYForNextTask = nil + self.thenForNextTask = nil self.taskKey = key self.taskIDMutex.Unlock()