From a5ee61c117a7b1e895cced1c131eb52d8c1e5236 Mon Sep 17 00:00:00 2001 From: Jesse Duffield Date: Wed, 19 Jul 2023 21:16:27 +1000 Subject: Properly fix accordion issue The true issue was that we were focusing the line in the view before it gets resized in the layout function. This meant if the view was squashed in accordion mode, the view wouldn't know how to set the cursor/origin to focus the line. Now we've got a queue of 'after layout' functions i.e. functions to call at the end of the layout function, right before views are drawn. The only caveat is that we can't have an infinite buffer so we're arbitrarily capping it at 1000 and dropping functions if we exceed that limit. But that really should never happen. --- pkg/gui/context/list_context_trait.go | 9 ++++++++- pkg/gui/gui.go | 5 ++++- pkg/gui/gui_common.go | 9 +++++++++ pkg/gui/layout.go | 18 +++++++++++++++++- pkg/gui/types/common.go | 4 ++++ 5 files changed, 42 insertions(+), 3 deletions(-) diff --git a/pkg/gui/context/list_context_trait.go b/pkg/gui/context/list_context_trait.go index e993719d5..de88d3d3b 100644 --- a/pkg/gui/context/list_context_trait.go +++ b/pkg/gui/context/list_context_trait.go @@ -31,7 +31,14 @@ func (self *ListContextTrait) GetList() types.IList { } func (self *ListContextTrait) FocusLine() { - self.GetViewTrait().FocusPoint(self.list.GetSelectedLineIdx()) + // Doing this at the end of the layout function because we need the view to be + // resized before we focus the line, otherwise if we're in accordion mode + // the view could be squashed and won't how to adjust the cursor/origin + self.c.AfterLayout(func() error { + self.GetViewTrait().FocusPoint(self.list.GetSelectedLineIdx()) + return nil + }) + self.setFooter() if self.refreshViewportOnChange { diff --git a/pkg/gui/gui.go b/pkg/gui/gui.go index 1057a85bf..8e8f6e2bd 100644 --- a/pkg/gui/gui.go +++ b/pkg/gui/gui.go @@ -132,6 +132,8 @@ type Gui struct { helpers *helpers.Helpers integrationTest integrationTypes.IntegrationTest + + afterLayoutFuncs chan func() error } type StateAccessor struct { @@ -458,7 +460,8 @@ func NewGui( PopupMutex: &deadlock.Mutex{}, PtyMutex: &deadlock.Mutex{}, }, - InitialDir: initialDir, + InitialDir: initialDir, + afterLayoutFuncs: make(chan func() error, 1000), } gui.WatchFilesForChanges() diff --git a/pkg/gui/gui_common.go b/pkg/gui/gui_common.go index c0d7bd460..ee0e51b26 100644 --- a/pkg/gui/gui_common.go +++ b/pkg/gui/gui_common.go @@ -168,3 +168,12 @@ func (self *guiCommon) IsAnyModeActive() bool { func (self *guiCommon) GetInitialKeybindingsWithCustomCommands() ([]*types.Binding, []*gocui.ViewMouseBinding) { return self.gui.GetInitialKeybindingsWithCustomCommands() } + +func (self *guiCommon) AfterLayout(f func() error) { + select { + case self.gui.afterLayoutFuncs <- f: + default: + // hopefully this never happens + self.gui.c.Log.Error("afterLayoutFuncs channel is full, skipping function") + } +} diff --git a/pkg/gui/layout.go b/pkg/gui/layout.go index 14f79cb5a..ec192527b 100644 --- a/pkg/gui/layout.go +++ b/pkg/gui/layout.go @@ -149,7 +149,23 @@ func (gui *Gui) layout(g *gocui.Gui) error { // if you run `lazygit --logs` // this will let you see these branches as prettified json // gui.c.Log.Info(utils.AsJson(gui.State.Model.Branches[0:4])) - return gui.helpers.Confirmation.ResizeCurrentPopupPanel() + if err := gui.helpers.Confirmation.ResizeCurrentPopupPanel(); err != nil { + return err + } + +outer: + for { + select { + case f := <-gui.afterLayoutFuncs: + if err := f(); err != nil { + return err + } + default: + break outer + } + } + + return nil } func (gui *Gui) prepareView(viewName string) (*gocui.View, error) { diff --git a/pkg/gui/types/common.go b/pkg/gui/types/common.go index 4e5ef627f..02dee3b15 100644 --- a/pkg/gui/types/common.go +++ b/pkg/gui/types/common.go @@ -80,6 +80,10 @@ type IGuiCommon interface { // Runs a function in a goroutine. Use this whenever you want to run a goroutine and keep track of the fact // that lazygit is still busy. See docs/dev/Busy.md OnWorker(f func(gocui.Task)) + // Function to call at the end of our 'layout' function which renders views + // For example, you may want a view's line to be focused only after that view is + // resized, if in accordion mode. + AfterLayout(f func() error) // returns the gocui Gui struct. There is a good chance you don't actually want to use // this struct and instead want to use another method above -- cgit v1.2.3 From 866e0a618b2a5959f5284369a49ec23c972af806 Mon Sep 17 00:00:00 2001 From: Jesse Duffield Date: Wed, 19 Jul 2023 22:02:43 +1000 Subject: Add integration test for accordion mode --- go.mod | 2 +- go.sum | 4 +- pkg/gui/gui.go | 24 ++++++++- pkg/integration/clients/tui.go | 5 +- pkg/integration/components/test.go | 26 +++++++++ pkg/integration/components/view_driver.go | 24 +++++++++ pkg/integration/tests/test_list.go | 1 + pkg/integration/tests/ui/accordion.go | 61 ++++++++++++++++++++++ pkg/integration/types/types.go | 3 ++ vendor/github.com/jesseduffield/gocui/gui.go | 29 +++++++--- .../github.com/jesseduffield/gocui/tcell_driver.go | 4 +- vendor/modules.txt | 2 +- 12 files changed, 168 insertions(+), 17 deletions(-) create mode 100644 pkg/integration/tests/ui/accordion.go diff --git a/go.mod b/go.mod index ae3751f30..d46597ee5 100644 --- a/go.mod +++ b/go.mod @@ -18,7 +18,7 @@ require ( github.com/integrii/flaggy v1.4.0 github.com/jesseduffield/generics v0.0.0-20220320043834-727e535cbe68 github.com/jesseduffield/go-git/v5 v5.1.2-0.20221018185014-fdd53fef665d - github.com/jesseduffield/gocui v0.3.1-0.20230719103719-ea5c8b64cf35 + github.com/jesseduffield/gocui v0.3.1-0.20230719120401-398f4965241f github.com/jesseduffield/kill v0.0.0-20220618033138-bfbe04675d10 github.com/jesseduffield/lazycore v0.0.0-20221012050358-03d2e40243c5 github.com/jesseduffield/minimal/gitignore v0.3.3-0.20211018110810-9cde264e6b1e diff --git a/go.sum b/go.sum index d24f7fbd2..41f5f3b47 100644 --- a/go.sum +++ b/go.sum @@ -72,8 +72,8 @@ github.com/jesseduffield/generics v0.0.0-20220320043834-727e535cbe68 h1:EQP2Tv8T github.com/jesseduffield/generics v0.0.0-20220320043834-727e535cbe68/go.mod h1:+LLj9/WUPAP8LqCchs7P+7X0R98HiFujVFANdNaxhGk= github.com/jesseduffield/go-git/v5 v5.1.2-0.20221018185014-fdd53fef665d h1:bO+OmbreIv91rCe8NmscRwhFSqkDJtzWCPV4Y+SQuXE= github.com/jesseduffield/go-git/v5 v5.1.2-0.20221018185014-fdd53fef665d/go.mod h1:nGNEErzf+NRznT+N2SWqmHnDnF9aLgANB1CUNEan09o= -github.com/jesseduffield/gocui v0.3.1-0.20230719103719-ea5c8b64cf35 h1:ZWI/Tkg89iTpZDuxWYsR/f6xu+U827N8L1YnSvqug88= -github.com/jesseduffield/gocui v0.3.1-0.20230719103719-ea5c8b64cf35/go.mod h1:dJ/BEUt3OWtaRg/PmuJWendRqREhre9JQ1SLvqrVJ8s= +github.com/jesseduffield/gocui v0.3.1-0.20230719120401-398f4965241f h1:w/pxI34XepTAx4HwxUu8ipimbVRgSTS+7ahmgFQwH80= +github.com/jesseduffield/gocui v0.3.1-0.20230719120401-398f4965241f/go.mod h1:dJ/BEUt3OWtaRg/PmuJWendRqREhre9JQ1SLvqrVJ8s= github.com/jesseduffield/kill v0.0.0-20220618033138-bfbe04675d10 h1:jmpr7KpX2+2GRiE91zTgfq49QvgiqB0nbmlwZ8UnOx0= github.com/jesseduffield/kill v0.0.0-20220618033138-bfbe04675d10/go.mod h1:aA97kHeNA+sj2Hbki0pvLslmE4CbDyhBeSSTUUnOuVo= github.com/jesseduffield/lazycore v0.0.0-20221012050358-03d2e40243c5 h1:CDuQmfOjAtb1Gms6a1p5L2P8RhbLUq5t8aL7PiQd2uY= diff --git a/pkg/gui/gui.go b/pkg/gui/gui.go index 8e8f6e2bd..25ec769a5 100644 --- a/pkg/gui/gui.go +++ b/pkg/gui/gui.go @@ -522,9 +522,29 @@ var RuneReplacements = map[rune]string{ } func (gui *Gui) initGocui(headless bool, test integrationTypes.IntegrationTest) (*gocui.Gui, error) { - playRecording := test != nil && os.Getenv(components.SANDBOX_ENV_VAR) != "true" + runInSandbox := os.Getenv(components.SANDBOX_ENV_VAR) == "true" + playRecording := test != nil && !runInSandbox + + width, height := 0, 0 + if test != nil { + if test.RequiresHeadless() { + if runInSandbox { + panic("Test requires headless, can't run in sandbox") + } + headless = true + } + width, height = test.HeadlessDimensions() + } - g, err := gocui.NewGui(gocui.OutputTrue, OverlappingEdges, playRecording, headless, RuneReplacements) + g, err := gocui.NewGui(gocui.NewGuiOpts{ + OutputMode: gocui.OutputTrue, + SupportOverlaps: OverlappingEdges, + PlayRecording: playRecording, + Headless: headless, + RuneReplacements: RuneReplacements, + Width: width, + Height: height, + }) if err != nil { return nil, err } diff --git a/pkg/integration/clients/tui.go b/pkg/integration/clients/tui.go index b931e8954..7d82a6630 100644 --- a/pkg/integration/clients/tui.go +++ b/pkg/integration/clients/tui.go @@ -28,7 +28,10 @@ func RunTUI() { app := newApp(testDir) app.loadTests() - g, err := gocui.NewGui(gocui.OutputTrue, false, false, false, gui.RuneReplacements) + g, err := gocui.NewGui(gocui.NewGuiOpts{ + OutputMode: gocui.OutputTrue, + RuneReplacements: gui.RuneReplacements, + }) if err != nil { log.Panicln(err) } diff --git a/pkg/integration/components/test.go b/pkg/integration/components/test.go index 464779e38..d17e3f7c5 100644 --- a/pkg/integration/components/test.go +++ b/pkg/integration/components/test.go @@ -19,6 +19,11 @@ import ( // to get the test's name via it's file's path. const unitTestDescription = "test test" +const ( + defaultWidth = 100 + defaultHeight = 100 +) + type IntegrationTest struct { name string description string @@ -32,6 +37,8 @@ type IntegrationTest struct { keys config.KeybindingConfig, ) gitVersion GitVersionRestriction + width int + height int } var _ integrationTypes.IntegrationTest = &IntegrationTest{} @@ -52,6 +59,11 @@ type NewIntegrationTestArgs struct { Skip bool // to run a test only on certain git versions GitVersion GitVersionRestriction + // width and height when running in headless mode, for testing + // the UI in different sizes. + // If these are set, the test must be run in headless mode + Width int + Height int } type GitVersionRestriction struct { @@ -120,6 +132,8 @@ func NewIntegrationTest(args NewIntegrationTestArgs) *IntegrationTest { setupConfig: args.SetupConfig, run: args.Run, gitVersion: args.GitVersion, + width: args.Width, + height: args.Height, } } @@ -172,6 +186,18 @@ func (self *IntegrationTest) Run(gui integrationTypes.GuiDriver) { } } +func (self *IntegrationTest) HeadlessDimensions() (int, int) { + if self.width == 0 && self.height == 0 { + return defaultWidth, defaultHeight + } + + return self.width, self.height +} + +func (self *IntegrationTest) RequiresHeadless() bool { + return self.width != 0 && self.height != 0 +} + func testNameFromCurrentFilePath() string { path := utils.FilePath(3) return TestNameFromFilePath(path) diff --git a/pkg/integration/components/view_driver.go b/pkg/integration/components/view_driver.go index 2c4a23572..d1d1571c7 100644 --- a/pkg/integration/components/view_driver.go +++ b/pkg/integration/components/view_driver.go @@ -82,6 +82,20 @@ func (self *ViewDriver) TopLines(matchers ...*TextMatcher) *ViewDriver { return self.assertLines(0, matchers...) } +// Asserts on the visible lines of the view. +// Note, this assumes that the view's viewport is filled with lines +func (self *ViewDriver) VisibleLines(matchers ...*TextMatcher) *ViewDriver { + self.validateMatchersPassed(matchers) + self.validateVisibleLineCount(matchers) + + // Get the origin of the view and offset that. + // Note that we don't do any retrying here so if we want to bring back retry logic + // we'll need to update this. + originY := self.getView().OriginY() + + return self.assertLines(originY, matchers...) +} + // asserts that somewhere in the view there are consequetive lines matching the given matchers. func (self *ViewDriver) ContainsLines(matchers ...*TextMatcher) *ViewDriver { self.validateMatchersPassed(matchers) @@ -212,6 +226,16 @@ func (self *ViewDriver) validateEnoughLines(matchers []*TextMatcher) { }) } +// assumes the view's viewport is filled with lines +func (self *ViewDriver) validateVisibleLineCount(matchers []*TextMatcher) { + view := self.getView() + + self.t.assertWithRetries(func() (bool, string) { + count := view.InnerHeight() + 1 + return count == len(matchers), fmt.Sprintf("unexpected number of visible lines in view '%s'. Expected exactly %d, got %d", view.Name(), len(matchers), count) + }) +} + func (self *ViewDriver) assertLines(offset int, matchers ...*TextMatcher) *ViewDriver { view := self.getView() diff --git a/pkg/integration/tests/test_list.go b/pkg/integration/tests/test_list.go index af0ac1c5b..caaf2039e 100644 --- a/pkg/integration/tests/test_list.go +++ b/pkg/integration/tests/test_list.go @@ -209,6 +209,7 @@ var tests = []*components.IntegrationTest{ tag.CrudAnnotated, tag.CrudLightweight, tag.Reset, + ui.Accordion, ui.DoublePopup, ui.SwitchTabFromMenu, undo.UndoCheckoutAndDrop, diff --git a/pkg/integration/tests/ui/accordion.go b/pkg/integration/tests/ui/accordion.go new file mode 100644 index 000000000..abfe27dbb --- /dev/null +++ b/pkg/integration/tests/ui/accordion.go @@ -0,0 +1,61 @@ +package ui + +import ( + "github.com/jesseduffield/lazygit/pkg/config" + . "github.com/jesseduffield/lazygit/pkg/integration/components" +) + +// When in acccordion mode, Lazygit looks like this: +// +// ╶─Status─────────────────────────╴┌─Patch──────────────────────────────────────────────────────────┐ +// ╶─Files - Submodules──────0 of 0─╴│commit 6e56dd04b70e548976f7f2928c4d9c359574e2bc ▲ +// ╶─Local branches - Remotes1 of 1─╴│Author: CI █ +// ┌─Commits - Reflog───────────────┐│Date: Wed Jul 19 22:00:03 2023 +1000 │ +// │7fe02805 CI commit 12 ▲│ ▼ +// │6e56dd04 CI commit 11 █└────────────────────────────────────────────────────────────────┘ +// │a35c687d CI commit 10 ▼┌─Command log────────────────────────────────────────────────────┐ +// └───────────────────────10 of 20─┘│Random tip: To filter commits by path, press '' │ +// ╶─Stash───────────────────0 of 0─╴└────────────────────────────────────────────────────────────────┘ +// /: Scroll, : Cancel, q: Quit, ?: Keybindings, 1-Donate Ask Question unversioned + +var Accordion = NewIntegrationTest(NewIntegrationTestArgs{ + Description: "Verify accordion mode kicks in when the screen height is too small", + ExtraCmdArgs: []string{}, + Width: 100, + Height: 10, + Skip: false, + SetupConfig: func(config *config.AppConfig) {}, + SetupRepo: func(shell *Shell) { + shell.CreateNCommits(20) + }, + Run: func(t *TestDriver, keys config.KeybindingConfig) { + t.Views().Commits(). + Focus(). + VisibleLines( + Contains("commit 20").IsSelected(), + Contains("commit 19"), + Contains("commit 18"), + ). + // go past commit 11, then come back, so that it ends up in the centre of the viewport + NavigateToLine(Contains("commit 11")). + NavigateToLine(Contains("commit 10")). + NavigateToLine(Contains("commit 11")). + VisibleLines( + Contains("commit 12"), + Contains("commit 11").IsSelected(), + Contains("commit 10"), + ) + + t.Views().Files(). + Focus() + + // ensure we retain the same viewport upon re-focus + t.Views().Commits(). + Focus(). + VisibleLines( + Contains("commit 12"), + Contains("commit 11").IsSelected(), + Contains("commit 10"), + ) + }, +}) diff --git a/pkg/integration/types/types.go b/pkg/integration/types/types.go index a26ac67af..266304bbf 100644 --- a/pkg/integration/types/types.go +++ b/pkg/integration/types/types.go @@ -13,6 +13,9 @@ import ( type IntegrationTest interface { Run(GuiDriver) SetupConfig(config *config.AppConfig) + RequiresHeadless() bool + // width and height when running headless + HeadlessDimensions() (int, int) } // this is the interface through which our integration tests interact with the lazygit gui diff --git a/vendor/github.com/jesseduffield/gocui/gui.go b/vendor/github.com/jesseduffield/gocui/gui.go index 47590f959..d07287685 100644 --- a/vendor/github.com/jesseduffield/gocui/gui.go +++ b/vendor/github.com/jesseduffield/gocui/gui.go @@ -177,13 +177,26 @@ type Gui struct { taskManager *TaskManager } +type NewGuiOpts struct { + OutputMode OutputMode + SupportOverlaps bool + PlayRecording bool + Headless bool + // only applicable when Headless is true + Width int + // only applicable when Headless is true + Height int + + RuneReplacements map[rune]string +} + // NewGui returns a new Gui object with a given output mode. -func NewGui(mode OutputMode, supportOverlaps bool, playRecording bool, headless bool, runeReplacements map[rune]string) (*Gui, error) { +func NewGui(opts NewGuiOpts) (*Gui, error) { g := &Gui{} var err error - if headless { - err = g.tcellInitSimulation() + if opts.Headless { + err = g.tcellInitSimulation(opts.Width, opts.Height) } else { err = g.tcellInit(runeReplacements) } @@ -191,7 +204,7 @@ func NewGui(mode OutputMode, supportOverlaps bool, playRecording bool, headless return nil, err } - if headless || runtime.GOOS == "windows" { + if opts.Headless || runtime.GOOS == "windows" { g.maxX, g.maxY = g.screen.Size() } else { // TODO: find out if we actually need this bespoke logic for linux @@ -201,7 +214,7 @@ func NewGui(mode OutputMode, supportOverlaps bool, playRecording bool, headless } } - g.outputMode = mode + g.outputMode = opts.OutputMode g.stop = make(chan struct{}) @@ -209,7 +222,7 @@ func NewGui(mode OutputMode, supportOverlaps bool, playRecording bool, headless g.userEvents = make(chan userEvent, 20) g.taskManager = newTaskManager() - if playRecording { + if opts.PlayRecording { g.ReplayedEvents = replayedEvents{ Keys: make(chan *TcellKeyEventWrapper), Resizes: make(chan *TcellResizeEventWrapper), @@ -221,14 +234,14 @@ func NewGui(mode OutputMode, supportOverlaps bool, playRecording bool, headless // SupportOverlaps is true when we allow for view edges to overlap with other // view edges - g.SupportOverlaps = supportOverlaps + g.SupportOverlaps = opts.SupportOverlaps // default keys for when searching strings in a view g.SearchEscapeKey = KeyEsc g.NextSearchMatchKey = 'n' g.PrevSearchMatchKey = 'N' - g.playRecording = playRecording + g.playRecording = opts.PlayRecording return g, nil } diff --git a/vendor/github.com/jesseduffield/gocui/tcell_driver.go b/vendor/github.com/jesseduffield/gocui/tcell_driver.go index 59238ed07..545996804 100644 --- a/vendor/github.com/jesseduffield/gocui/tcell_driver.go +++ b/vendor/github.com/jesseduffield/gocui/tcell_driver.go @@ -81,7 +81,7 @@ func registerRuneFallbacks(s tcell.Screen, additional map[rune]string) { } // tcellInitSimulation initializes tcell screen for use. -func (g *Gui) tcellInitSimulation() error { +func (g *Gui) tcellInitSimulation(width int, height int) error { s := tcell.NewSimulationScreen("") if e := s.Init(); e != nil { return e @@ -90,7 +90,7 @@ func (g *Gui) tcellInitSimulation() error { Screen = s // setting to a larger value than the typical terminal size // so that during a test we're more likely to see an item to select in a view. - s.SetSize(100, 100) + s.SetSize(width, height) s.Sync() return nil } diff --git a/vendor/modules.txt b/vendor/modules.txt index e8912c419..5aa0a325b 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -172,7 +172,7 @@ github.com/jesseduffield/go-git/v5/utils/merkletrie/filesystem github.com/jesseduffield/go-git/v5/utils/merkletrie/index github.com/jesseduffield/go-git/v5/utils/merkletrie/internal/frame github.com/jesseduffield/go-git/v5/utils/merkletrie/noder -# github.com/jesseduffield/gocui v0.3.1-0.20230719103719-ea5c8b64cf35 +# github.com/jesseduffield/gocui v0.3.1-0.20230719120401-398f4965241f ## explicit; go 1.12 github.com/jesseduffield/gocui # github.com/jesseduffield/kill v0.0.0-20220618033138-bfbe04675d10 -- cgit v1.2.3