diff --git a/pkg/gui/gui_driver.go b/pkg/gui/gui_driver.go index af261b453..5db3d9d92 100644 --- a/pkg/gui/gui_driver.go +++ b/pkg/gui/gui_driver.go @@ -266,12 +266,12 @@ func (self *GuiDriver) TopViewInWindow(windowName string) *gocui.View { } func (self *GuiDriver) SetCaption(caption string) { - self.gui.setCaption(caption) + self.OnUIThreadAndWait(func() { self.gui.setCaption(caption) }) self.waitTillIdle() } func (self *GuiDriver) SetCaptionPrefix(prefix string) { - self.gui.setCaptionPrefix(prefix) + self.OnUIThreadAndWait(func() { self.gui.setCaptionPrefix(prefix) }) self.waitTillIdle() } diff --git a/pkg/gui/main_panels.go b/pkg/gui/main_panels.go index 5a723125b..abab6fcc8 100644 --- a/pkg/gui/main_panels.go +++ b/pkg/gui/main_panels.go @@ -355,9 +355,12 @@ func (gui *Gui) clampDiffSelectionToContent(view *gocui.View) { // claiming the render it was showing: whatever it is given next is content the user // hasn't seen there, and is shown from the top like any other. // -// A position waiting to be put back goes too: this pane is getting no render for it -// to ride, and whoever is waiting for the view to be back where it belongs has to -// hear that it never will be. +// The pane is emptied right away, and it is also given an empty render. The render +// takes its place among the view's tasks, so that a render asked for before it can't +// fill the pane again, whether that render's task is still to be created or is still +// reading. Like any render that isn't a re-render of what the pane was showing, it +// drops a position waiting to be put back: whoever is waiting for the view to be back +// where it belongs has to hear that it never will be. func (gui *Gui) clearMainView(mainContext types.Context) { view := mainContext.GetView() view.Clear() @@ -369,7 +372,9 @@ func (gui *Gui) clearMainView(mainContext types.Context) { gui.State.ContextMgr.UpdateSelectionHighlights() if manager := gui.getViewBufferManagerForView(view); manager != nil { manager.ForgetRenderedContent() - manager.DropRestoreForNextTask() + if err := gui.newStringTask(view, ""); err != nil { + gui.c.Log.Error(err) + } } } diff --git a/pkg/gui/main_view_render.go b/pkg/gui/main_view_render.go index 5c615208d..4b661bf85 100644 --- a/pkg/gui/main_view_render.go +++ b/pkg/gui/main_view_render.go @@ -56,11 +56,16 @@ func (gui *Gui) newRenderTask(view *gocui.View, cmd *exec.Cmd, prefix string) er // it before anything else can touch the command's arguments. cmdStr := strings.Join(cmd.Args, " ") + manager := gui.getManager(view) + // The task takes its place among the view's tasks now, although it is only + // created after the layout (see the matching call in newCmdTask). + reservation := manager.ReserveTask() + // 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() + manager.StartLoading() // Hold the scrollbar at its current height while the re-render loads, so the // thumb doesn't shrink and snap back when the first partial paint swaps in // (see the matching call in newCmdTask). @@ -68,6 +73,10 @@ func (gui *Gui) newRenderTask(view *gocui.View, cmd *exec.Cmd, prefix string) er // Run the render after layout so that it gets the correct size gui.afterLayout(func() error { + if manager.IsSuperseded(reservation) { + return nil + } + // The layout may have changed the size of the view, so only now is the // width to render at known, and with it the renderer command. width := gui.renderWidth(view) @@ -113,7 +122,7 @@ func (gui *Gui) newRenderTask(view *gocui.View, cmd *exec.Cmd, prefix string) er if rendersThroughAPipe() { run = gui.pipedRender } - return gui.newTaskForRender(spec, prefix, cmdStr, run) + return gui.newTaskForRender(reservation, spec, prefix, cmdStr, run) }) return nil @@ -144,16 +153,17 @@ type ( type runRender func(spec renderSpec) (startRender, onCloseRender) // newTaskForRender creates the task that reads the render's output into its -// view, running the command the given way. key names what is rendered, so that +// view, running the command the given way. The task takes the place that the +// reservation holds among the view's tasks. key names what is rendered, so that // a re-render of the same content can be told from a render of other content. -func (gui *Gui) newTaskForRender(spec renderSpec, prefix string, key string, run runRender) error { +func (gui *Gui) newTaskForRender(reservation tasks.TaskReservation, spec renderSpec, prefix string, key string, run runRender) error { setColumnsEnvVar(spec.cmd, spec.width) start, onClose := run(spec) manager := gui.getManager(spec.view) linesToRead := gui.linesToReadFromCmdTask(spec.view) - return manager.NewTask(manager.NewCmdTask(start, prefix, linesToRead, onClose), key) + return manager.NewReservedTask(reservation, manager.NewCmdTask(start, prefix, linesToRead, onClose), key) } // renderWithoutPtyEnvVar makes a render take the piped path on a platform that diff --git a/pkg/gui/tasks_adapter.go b/pkg/gui/tasks_adapter.go index 644b642a9..4c37a18ee 100644 --- a/pkg/gui/tasks_adapter.go +++ b/pkg/gui/tasks_adapter.go @@ -18,10 +18,16 @@ func (gui *Gui) newCmdTask(view *gocui.View, cmd *exec.Cmd, prefix string) error cmdStr, ).Debug("RunCommand") + manager := gui.getManager(view) + // The task is only created after the layout (see below), but it has to take + // its place among the view's tasks now. Otherwise a task asked for after this + // one, but created before the layout, would be replaced by it. + reservation := manager.ReserveTask() + // 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. - gui.getManager(view).StartLoading() + manager.StartLoading() // Hold the scrollbar at the height the view has now (the previous render), // while it still shows that render: once the re-render swaps in its first // partial paint the displayed buffer is briefly short, and we don't want the @@ -34,8 +40,12 @@ func (gui *Gui) newCmdTask(view *gocui.View, cmd *exec.Cmd, prefix string) error // keeps the task goroutine from reading the view's live dimensions while it // streams output. gui.afterLayout(func() error { + if manager.IsSuperseded(reservation) { + return nil + } + spec := renderSpec{view: view, cmd: cmd, width: gui.renderWidth(view)} - return gui.newTaskForRender(spec, prefix, cmdStr, gui.plainRender) + return gui.newTaskForRender(reservation, spec, prefix, cmdStr, gui.plainRender) }) return nil diff --git a/pkg/integration/tests/main_view/keep_an_emptied_pane_empty.go b/pkg/integration/tests/main_view/keep_an_emptied_pane_empty.go new file mode 100644 index 000000000..a0b12509f --- /dev/null +++ b/pkg/integration/tests/main_view/keep_an_emptied_pane_empty.go @@ -0,0 +1,43 @@ +package main_view + +import ( + "github.com/jesseduffield/lazygit/pkg/config" + . "github.com/jesseduffield/lazygit/pkg/integration/components" +) + +// Both selection changes are handled before the next layout. Selecting file_a asks +// for its staged changes in the lower pane, whose task is only created after the +// layout. Selecting file_b again leaves that pane with nothing to show, and empties it +// right away. The pane is hidden then, but when it is shown again, it shows what it +// holds until its next render replaces it. +var KeepAnEmptiedPaneEmpty = NewIntegrationTest(NewIntegrationTestArgs{ + Description: "Selecting a file and moving back off it in rapid succession leaves the pane that the file filled empty", + ExtraCmdArgs: []string{}, + Skip: false, + SetupConfig: func(config *config.AppConfig) {}, + SetupRepo: func(shell *Shell) { + shell.CreateFileAndAdd("file_a", "one\n") + shell.CreateFileAndAdd("file_b", "one\n") + shell.Commit("one") + + shell.UpdateFileAndAdd("file_a", "STAGED\n") + shell.UpdateFile("file_a", "UNSTAGED\n") + shell.UpdateFile("file_b", "two\n") + }, + Run: func(t *TestDriver, keys config.KeybindingConfig) { + t.Views().Files(). + IsFocused(). + NavigateToLine(Contains("file_b")) + + t.Views().Secondary(). + IsInvisible() + + t.Views().Files(). + PressRapidly(keys.Universal.PrevItem, keys.Universal.NextItem). + SelectedLine(Contains("file_b")) + + t.Views().Secondary(). + IsInvisible(). + Content(Equals("")) + }, +}) diff --git a/pkg/integration/tests/main_view/show_the_tab_switched_to_last.go b/pkg/integration/tests/main_view/show_the_tab_switched_to_last.go new file mode 100644 index 000000000..962185f6e --- /dev/null +++ b/pkg/integration/tests/main_view/show_the_tab_switched_to_last.go @@ -0,0 +1,39 @@ +package main_view + +import ( + "github.com/jesseduffield/lazygit/pkg/config" + . "github.com/jesseduffield/lazygit/pkg/integration/components" +) + +// Both tab switches are handled before the next layout. Switching to the Files tab +// asks for the file's diff, whose task is only created after the layout; switching +// back to the Worktrees tab asks for the worktree's details, whose task is created +// right away. The main view has to show what was asked for last. +var ShowTheTabSwitchedToLast = NewIntegrationTest(NewIntegrationTestArgs{ + Description: "Switching tabs twice in rapid succession leaves the main view showing the second tab's content", + ExtraCmdArgs: []string{}, + Skip: false, + SetupConfig: func(config *config.AppConfig) {}, + SetupRepo: func(shell *Shell) { + shell.CreateFileAndAdd("file1", "one\n") + shell.Commit("one") + shell.UpdateFile("file1", "ONE\n") + }, + Run: func(t *TestDriver, keys config.KeybindingConfig) { + t.Views().Worktrees(). + Focus(). + Lines( + Contains("(main worktree)").IsSelected(), + ) + + t.Views().Main(). + Content(Contains("Path:")) + + t.Views().Worktrees(). + PressRapidly(keys.Universal.PrevTab, keys.Universal.NextTab). + IsFocused() + + t.Views().Main(). + Content(Contains("Path:")) + }, +}) diff --git a/pkg/integration/tests/test_list.go b/pkg/integration/tests/test_list.go index 264a4bc95..7a5c728ba 100644 --- a/pkg/integration/tests/test_list.go +++ b/pkg/integration/tests/test_list.go @@ -410,6 +410,7 @@ var tests = []*components.IntegrationTest{ main_view.JumpToAFileOfTheDiff, main_view.JumpToAFileOnlyOverADiff, main_view.KeepAWrappedLineCoveredAcrossARerender, + main_view.KeepAnEmptiedPaneEmpty, main_view.KeepBothHalvesOfAChangeSelected, main_view.KeepPositionByTheVisibleEndOfASelection, main_view.KeepPositionInBothPanesWhenChangingContextSize, @@ -480,6 +481,7 @@ var tests = []*components.IntegrationTest{ main_view.SelectionCommandTooltipsFollowTheDiff, main_view.SelectionCommandsOnlyWhereTheyApply, main_view.SelectionOverTheCustomPatch, + main_view.ShowTheTabSwitchedToLast, main_view.StageDeletedFile, main_view.StageDiffLines, main_view.StageDiffLinesOfAPathWithASpace, diff --git a/pkg/tasks/tasks.go b/pkg/tasks/tasks.go index c1656b2de..c2b8177d0 100644 --- a/pkg/tasks/tasks.go +++ b/pkg/tasks/tasks.go @@ -64,9 +64,10 @@ type ViewBufferManager struct { waitingMutex deadlock.Mutex // Guards newTaskID and taskKey, which identify the most recently requested - // task. Both are written on the goroutine NewTask spawns, and taskKey is - // read from the UI thread (GetTaskKey), so neither may be touched without - // holding this. + // task. newTaskID is written wherever a task is asked for (ReserveTask) and + // read on the goroutine NewReservedTask spawns. taskKey is written on that + // goroutine and read from the UI thread (GetTaskKey). So neither may be + // touched without holding this. taskIDMutex deadlock.Mutex Log *logrus.Entry newTaskID int @@ -116,10 +117,12 @@ type ViewBufferManager struct { // Guarded by taskIDMutex, like the task key. keepScrollForNextTask 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. - loading atomic.Bool + // The command task whose output the view is loading: the one that was asked + // for last when StartLoading was called. It is cleared when that task has + // read all of its input. The view counts as loading only while this task is + // still the one asked for last; a task asked for after it takes the view + // over, whatever it shows. Guarded by taskIDMutex, like the task key. + loadingTaskID int // beforeStart is the function that is called before starting a new task beforeStart func() @@ -343,19 +346,36 @@ func (self *ViewBufferManager) ReadLines(totalLines int) { self.readRequests.enqueue(LinesToRead{Total: totalLines, InitialRefreshAfter: -1}) } -// IsLoading reports whether a command task is currently reading content into the -// view, meaning the content is still growing. +// IsLoading reports whether the task asked for last is a command task that is +// still reading content into the view, meaning the content is still growing. func (self *ViewBufferManager) IsLoading() bool { - return self.loading.Load() + self.taskIDMutex.Lock() + defer self.taskIDMutex.Unlock() + + return self.loadingTaskID != 0 && self.loadingTaskID == self.newTaskID } -// 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. +// StartLoading marks the view as loading the output of the task asked for last, +// which must be a command task. Call it right when that task is asked for, before +// its goroutine runs and before the next layout pass, so that the layout doesn't +// clamp the scroll position to the not-yet-loaded content. The view stops loading +// when the task reaches the end of its input, or when another task is asked for. func (self *ViewBufferManager) StartLoading() { - self.loading.Store(true) + self.taskIDMutex.Lock() + defer self.taskIDMutex.Unlock() + + self.loadingTaskID = self.newTaskID +} + +// finishLoading records that the given task has read all of its input. If the +// view is loading another task's output, that task is still loading. +func (self *ViewBufferManager) finishLoading(taskID int) { + self.taskIDMutex.Lock() + defer self.taskIDMutex.Unlock() + + if self.loadingTaskID == taskID { + self.loadingTaskID = 0 + } } func (self *ViewBufferManager) ReadToEnd(then func()) { @@ -700,10 +720,8 @@ func (self *ViewBufferManager) NewCmdTask(start func() (Cmd, io.Reader), prefix 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) + // layout to clamp the scroll position to it. + self.finishLoading(opts.taskID) callThen() break outer } @@ -815,9 +833,48 @@ type TaskOpts struct { // We use this to keep track of when a user's action is complete (i.e. all views // have been refreshed to display the results of their action) InitialContentLoaded func() + + // The task's place in the order of the view's tasks (see ReserveTask). + taskID int } +// A TaskReservation holds a task's place in the order of the view's tasks, from +// when the task is asked for until it is created. See ReserveTask. +type TaskReservation struct { + taskID int +} + +// ReserveTask gives a task its place in the order of the view's tasks, for a task +// that is only created later, with NewReservedTask. The view shows the task that +// was asked for last. If another task is asked for after the reservation, that +// task replaces the reserved one, even when it is created first. +func (self *ViewBufferManager) ReserveTask() TaskReservation { + self.taskIDMutex.Lock() + defer self.taskIDMutex.Unlock() + + self.newTaskID++ + return TaskReservation{taskID: self.newTaskID} +} + +// IsSuperseded reports whether another task has been asked for since the +// reservation was made. The reserved task would then stop as soon as it was +// created, so there is no point in creating it. +func (self *ViewBufferManager) IsSuperseded(reservation TaskReservation) bool { + self.taskIDMutex.Lock() + defer self.taskIDMutex.Unlock() + + return reservation.taskID < self.newTaskID +} + +// NewTask creates a task that takes its place in the order of the view's tasks +// right away. func (self *ViewBufferManager) NewTask(f func(TaskOpts) error, key string) error { + return self.NewReservedTask(self.ReserveTask(), f, key) +} + +// NewReservedTask creates the task that the reservation was made for. It doesn't +// run if another task has been asked for since the reservation was made. +func (self *ViewBufferManager) NewReservedTask(reservation TaskReservation, f func(TaskOpts) error, key string) error { gocuiTask := self.newGocuiTask() var completeTaskOnce sync.Once @@ -828,15 +885,7 @@ func (self *ViewBufferManager) NewTask(f func(TaskOpts) error, key string) error }) } - // Assign the taskID synchronously so it reflects NewTask call order - // rather than the order in which the spawned goroutines happen to be - // scheduled. Otherwise two NewTask calls in quick succession can have - // their goroutines race, with the later-called task ending up with the - // lower taskID and losing the staleness check below. - self.taskIDMutex.Lock() - self.newTaskID++ - taskID := self.newTaskID - self.taskIDMutex.Unlock() + taskID := reservation.taskID go utils.Safe(func() { defer completeGocuiTask() @@ -906,7 +955,7 @@ func (self *ViewBufferManager) NewTask(f func(TaskOpts) error, key string) error self.waitingMutex.Unlock() - if err := f(TaskOpts{Stop: stop, InitialContentLoaded: completeGocuiTask}); err != nil { + if err := f(TaskOpts{Stop: stop, InitialContentLoaded: completeGocuiTask, taskID: taskID}); err != nil { self.Log.Error(err) // might need an onError callback } diff --git a/pkg/tasks/tasks_test.go b/pkg/tasks/tasks_test.go index 7383070e7..f1bd5920d 100644 --- a/pkg/tasks/tasks_test.go +++ b/pkg/tasks/tasks_test.go @@ -813,3 +813,142 @@ func TestReadToEndHoldsItsTaskUntilThenReturns(t *testing.T) { assert.Equal(t, gocui.TaskStatusBusy, statusDuringThen) assert.Equal(t, gocui.TaskStatusDone, task.Status()) } + +// doneSignallingTask lets a test wait for the tasks of a view to finish, including +// those that never get to run. +type doneSignallingTask struct { + *gocui.FakeTask + done func() +} + +func (self *doneSignallingTask) Done() { + self.FakeTask.Done() + self.done() +} + +// A render whose task is only created after the layout takes its place among the +// view's tasks when it is asked for. A message asked for after it, before the +// layout, is created first, and the render's task mustn't replace it. +func TestReservedTaskDoesntReplaceATaskAskedForLater(t *testing.T) { + var tasksDone sync.WaitGroup + manager := NewViewBufferManager( + utils.NewDummyLog(), + bytes.NewBuffer(nil), + func() {}, + func() {}, + func() {}, + func() {}, + func() {}, + func() {}, + func() gocui.Task { + tasksDone.Add(1) + return &doneSignallingTask{FakeTask: gocui.NewFakeTask(), done: tasksDone.Done} + }, + // no UI thread in the test; run the view mutations inline + func(f func()) error { f(); return nil }, + ) + + var mutex sync.Mutex + var tasksRun []string + task := func(name string) func(TaskOpts) error { + return func(TaskOpts) error { + mutex.Lock() + defer mutex.Unlock() + tasksRun = append(tasksRun, name) + return nil + } + } + + reservation := manager.ReserveTask() + assert.False(t, manager.IsSuperseded(reservation)) + + _ = manager.NewTask(task("message"), "message") + assert.True(t, manager.IsSuperseded(reservation)) + + _ = manager.NewReservedTask(reservation, task("render"), "render") + tasksDone.Wait() + + assert.Equal(t, []string{"message"}, tasksRun) +} + +// A message that replaces a command task while the task is still reading its +// output leaves nothing loading content into the view. +func TestMessageEndsTheLoadingOfTheTaskItReplaces(t *testing.T) { + manager := NewViewBufferManager( + utils.NewDummyLog(), + bytes.NewBuffer(nil), + func() {}, + func() {}, + 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 }, + ) + + stalled := BlockingLineReader{ + linesToYield: 3, + blocked: make(chan struct{}), + unblock: make(chan struct{}), + } + defer close(stalled.unblock) + start := func() (Cmd, io.Reader) { + // not actually starting this because it's not necessary + return ExecCmd{Cmd: exec.Command("blah")}, &stalled + } + + reservation := manager.ReserveTask() + manager.StartLoading() + _ = manager.NewReservedTask(reservation, manager.NewCmdTask(start, "", LinesToRead{100, 50, nil}, nil), "cmd") + <-stalled.blocked + assert.True(t, manager.IsLoading()) + + _ = manager.NewTask(func(TaskOpts) error { return nil }, "message") + assert.False(t, manager.IsLoading()) +} + +// The view is loading the content of the command task asked for last. A task +// asked for before it reaching the end of its input doesn't end that. +func TestEarlierTaskEndingLeavesALaterTaskLoading(t *testing.T) { + manager := NewViewBufferManager( + utils.NewDummyLog(), + bytes.NewBuffer(nil), + func() {}, + func() {}, + 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 }, + ) + + stalled := BlockingLineReader{ + linesToYield: 3, + blocked: make(chan struct{}), + unblock: make(chan struct{}), + } + start := func() (Cmd, io.Reader) { + // not actually starting this because it's not necessary + return ExecCmd{Cmd: exec.Command("blah")}, &stalled + } + + earlierDone := make(chan struct{}) + reservation := manager.ReserveTask() + manager.StartLoading() + _ = manager.NewReservedTask(reservation, + manager.NewCmdTask(start, "", LinesToRead{100, 50, nil}, func() { close(earlierDone) }), "earlier") + <-stalled.blocked + + // The later task is asked for, but not created yet, as for a render whose task + // is created after the layout. + manager.ReserveTask() + manager.StartLoading() + + close(stalled.unblock) + <-earlierDone + assert.True(t, manager.IsLoading()) +}