From c07b3fad64ca71bc1873bcc1bb9e2256c05d4df0 Mon Sep 17 00:00:00 2001 From: Jesse Duffield Date: Sun, 28 Jan 2024 08:23:09 +1100 Subject: [PATCH 1/4] Ensure file view length is never returned as -1 This was causing issues when obtaining selected items --- pkg/gui/filetree/file_tree.go | 4 +++- pkg/gui/filetree/file_tree_view_model.go | 4 ++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/pkg/gui/filetree/file_tree.go b/pkg/gui/filetree/file_tree.go index 9d3bb580d..b80499cfd 100644 --- a/pkg/gui/filetree/file_tree.go +++ b/pkg/gui/filetree/file_tree.go @@ -5,6 +5,7 @@ import ( "github.com/jesseduffield/lazygit/pkg/commands/models" "github.com/jesseduffield/lazygit/pkg/gui/types" + "github.com/jesseduffield/lazygit/pkg/utils" "github.com/samber/lo" "github.com/sirupsen/logrus" ) @@ -138,7 +139,8 @@ func (self *FileTree) GetAllItems() []*FileNode { } func (self *FileTree) Len() int { - return self.tree.Size(self.collapsedPaths) - 1 // ignoring root + // -1 because we're ignoring the root + return utils.Max(self.tree.Size(self.collapsedPaths)-1, 0) } func (self *FileTree) GetItem(index int) types.HasUrn { diff --git a/pkg/gui/filetree/file_tree_view_model.go b/pkg/gui/filetree/file_tree_view_model.go index 05cc9cb89..439471b01 100644 --- a/pkg/gui/filetree/file_tree_view_model.go +++ b/pkg/gui/filetree/file_tree_view_model.go @@ -54,6 +54,10 @@ func (self *FileTreeViewModel) GetSelectedItemId() string { } func (self *FileTreeViewModel) GetSelectedItems() ([]*FileNode, int, int) { + if self.Len() == 0 { + return nil, 0, 0 + } + startIdx, endIdx := self.GetSelectionRange() nodes := []*FileNode{} From 0f9d9e13d12d37ab419efccc5c217422fb67a765 Mon Sep 17 00:00:00 2001 From: Jesse Duffield Date: Tue, 2 Jan 2024 12:19:31 +1100 Subject: [PATCH 2/4] Show mode-specific keybinding suggestions As part of making lazygit more discoverable, there are certain keys which you almost certainly need to press when you're in a given mode e.g. 'v' to paste commits when cherry-picking. This commit prominently shows these keybinding suggestions alongside the others in the option view. I'm using the same colours for these keybindings as is associated with the mode elsewhere e.g. yellow for rebasing and cyan for cherry-picking. The cherry-picking one is a bit weird because we also use cyan text to show loaders and app status at the bottom left so it may be confusing, but I haven't personally found it awkward from having tested it out myself. Previously we would render these options whenever a new context was activated, but now that we need to re-render options whenever a mode changes, I'm instead rendering them on each screen re-render (i.e. in the layout function). Given how cheap it is to render this text, I think it's fine performance-wise. --- pkg/commands/types/enums/enums.go | 8 + pkg/gui/context.go | 2 - .../controllers/confirmation_controller.go | 16 +- pkg/gui/controllers/global_controller.go | 10 +- pkg/gui/controllers/menu_controller.go | 10 +- .../controllers/merge_conflicts_controller.go | 46 ++--- pkg/gui/layout.go | 2 + pkg/gui/options_map.go | 157 ++++++++++++------ pkg/gui/types/keybindings.go | 28 ++-- pkg/gui/views.go | 1 - pkg/integration/components/common.go | 26 +++ pkg/integration/components/views.go | 4 + pkg/integration/tests/test_list.go | 1 + .../mode_specific_keybinding_suggestions.go | 118 +++++++++++++ 14 files changed, 325 insertions(+), 104 deletions(-) create mode 100644 pkg/integration/tests/ui/mode_specific_keybinding_suggestions.go diff --git a/pkg/commands/types/enums/enums.go b/pkg/commands/types/enums/enums.go index 7e8a81798..68a29d8f0 100644 --- a/pkg/commands/types/enums/enums.go +++ b/pkg/commands/types/enums/enums.go @@ -12,3 +12,11 @@ const ( REBASE_MODE_REBASING REBASE_MODE_MERGING ) + +func (self RebaseMode) IsMerging() bool { + return self == REBASE_MODE_MERGING +} + +func (self RebaseMode) IsRebasing() bool { + return self == REBASE_MODE_INTERACTIVE || self == REBASE_MODE_NORMAL || self == REBASE_MODE_REBASING +} diff --git a/pkg/gui/context.go b/pkg/gui/context.go index d845265a3..be5a720e3 100644 --- a/pkg/gui/context.go +++ b/pkg/gui/context.go @@ -245,8 +245,6 @@ func (self *ContextMgr) ActivateContext(c types.Context, opts types.OnFocusOpts) self.gui.c.GocuiGui().Cursor = v.Editable - self.gui.renderContextOptionsMap(c) - if err := c.HandleFocus(opts); err != nil { return err } diff --git a/pkg/gui/controllers/confirmation_controller.go b/pkg/gui/controllers/confirmation_controller.go index 164af19ec..aa5617fa8 100644 --- a/pkg/gui/controllers/confirmation_controller.go +++ b/pkg/gui/controllers/confirmation_controller.go @@ -24,16 +24,16 @@ func NewConfirmationController( func (self *ConfirmationController) GetKeybindings(opts types.KeybindingsOpts) []*types.Binding { bindings := []*types.Binding{ { - Key: opts.GetKey(opts.Config.Universal.Confirm), - Handler: func() error { return self.context().State.OnConfirm() }, - Description: self.c.Tr.Confirm, - Display: true, + Key: opts.GetKey(opts.Config.Universal.Confirm), + Handler: func() error { return self.context().State.OnConfirm() }, + Description: self.c.Tr.Confirm, + DisplayOnScreen: true, }, { - Key: opts.GetKey(opts.Config.Universal.Return), - Handler: func() error { return self.context().State.OnClose() }, - Description: self.c.Tr.CloseCancel, - Display: true, + Key: opts.GetKey(opts.Config.Universal.Return), + Handler: func() error { return self.context().State.OnClose() }, + Description: self.c.Tr.CloseCancel, + DisplayOnScreen: true, }, { Key: opts.GetKey(opts.Config.Universal.TogglePanel), diff --git a/pkg/gui/controllers/global_controller.go b/pkg/gui/controllers/global_controller.go index ba02ac26a..548c8461e 100644 --- a/pkg/gui/controllers/global_controller.go +++ b/pkg/gui/controllers/global_controller.go @@ -72,6 +72,7 @@ func (self *GlobalController) GetKeybindings(opts types.KeybindingsOpts) []*type Description: self.c.Tr.OpenKeybindingsMenu, Handler: self.createOptionsMenu, ShortDescription: self.c.Tr.Keybindings, + DisplayOnScreen: true, }, { ViewName: "", @@ -112,10 +113,11 @@ func (self *GlobalController) GetKeybindings(opts types.KeybindingsOpts) []*type Handler: self.quitWithoutChangingDirectory, }, { - Key: opts.GetKey(opts.Config.Universal.Return), - Modifier: gocui.ModNone, - Handler: self.escape, - Description: self.c.Tr.Cancel, + Key: opts.GetKey(opts.Config.Universal.Return), + Modifier: gocui.ModNone, + Handler: self.escape, + Description: self.c.Tr.Cancel, + DisplayOnScreen: true, }, { Key: opts.GetKey(opts.Config.Universal.ToggleWhitespaceInDiffView), diff --git a/pkg/gui/controllers/menu_controller.go b/pkg/gui/controllers/menu_controller.go index a64189138..ddd0b2c18 100644 --- a/pkg/gui/controllers/menu_controller.go +++ b/pkg/gui/controllers/menu_controller.go @@ -42,13 +42,13 @@ func (self *MenuController) GetKeybindings(opts types.KeybindingsOpts) []*types. Handler: self.withItem(self.press), GetDisabledReason: self.require(self.singleItemSelected()), Description: self.c.Tr.Execute, - Display: true, + DisplayOnScreen: true, }, { - Key: opts.GetKey(opts.Config.Universal.Return), - Handler: self.close, - Description: self.c.Tr.Close, - Display: true, + Key: opts.GetKey(opts.Config.Universal.Return), + Handler: self.close, + Description: self.c.Tr.Close, + DisplayOnScreen: true, }, } diff --git a/pkg/gui/controllers/merge_conflicts_controller.go b/pkg/gui/controllers/merge_conflicts_controller.go index e0d4cae06..03ca4a10b 100644 --- a/pkg/gui/controllers/merge_conflicts_controller.go +++ b/pkg/gui/controllers/merge_conflicts_controller.go @@ -48,30 +48,30 @@ func (self *MergeConflictsController) GetKeybindings(opts types.KeybindingsOpts) Description: self.c.Tr.SelectNextHunk, }, { - Key: opts.GetKey(opts.Config.Universal.PrevBlock), - Handler: self.withRenderAndFocus(self.PrevConflict), - Description: self.c.Tr.PrevConflict, - Display: true, + Key: opts.GetKey(opts.Config.Universal.PrevBlock), + Handler: self.withRenderAndFocus(self.PrevConflict), + Description: self.c.Tr.PrevConflict, + DisplayOnScreen: true, }, { - Key: opts.GetKey(opts.Config.Universal.NextBlock), - Handler: self.withRenderAndFocus(self.NextConflict), - Description: self.c.Tr.NextConflict, - Display: true, + Key: opts.GetKey(opts.Config.Universal.NextBlock), + Handler: self.withRenderAndFocus(self.NextConflict), + Description: self.c.Tr.NextConflict, + DisplayOnScreen: true, }, { - Key: opts.GetKey(opts.Config.Universal.Undo), - Handler: self.withRenderAndFocus(self.HandleUndo), - Description: self.c.Tr.Undo, - Tooltip: self.c.Tr.UndoMergeResolveTooltip, - Display: true, + Key: opts.GetKey(opts.Config.Universal.Undo), + Handler: self.withRenderAndFocus(self.HandleUndo), + Description: self.c.Tr.Undo, + Tooltip: self.c.Tr.UndoMergeResolveTooltip, + DisplayOnScreen: true, }, { - Key: opts.GetKey(opts.Config.Universal.Edit), - Handler: self.HandleEditFile, - Description: self.c.Tr.EditFile, - Tooltip: self.c.Tr.EditFileTooltip, - Display: true, + Key: opts.GetKey(opts.Config.Universal.Edit), + Handler: self.HandleEditFile, + Description: self.c.Tr.EditFile, + Tooltip: self.c.Tr.EditFileTooltip, + DisplayOnScreen: true, }, { Key: opts.GetKey(opts.Config.Universal.OpenFile), @@ -108,11 +108,11 @@ func (self *MergeConflictsController) GetKeybindings(opts types.KeybindingsOpts) Tag: "navigation", }, { - Key: opts.GetKey(opts.Config.Files.OpenMergeTool), - Handler: self.c.Helpers().WorkingTree.OpenMergeTool, - Description: self.c.Tr.OpenMergeTool, - Tooltip: self.c.Tr.OpenMergeToolTooltip, - Display: true, + Key: opts.GetKey(opts.Config.Files.OpenMergeTool), + Handler: self.c.Helpers().WorkingTree.OpenMergeTool, + Description: self.c.Tr.OpenMergeTool, + Tooltip: self.c.Tr.OpenMergeToolTooltip, + DisplayOnScreen: true, }, { Key: opts.GetKey(opts.Config.Universal.Return), diff --git a/pkg/gui/layout.go b/pkg/gui/layout.go index 02c74b023..823dfe994 100644 --- a/pkg/gui/layout.go +++ b/pkg/gui/layout.go @@ -165,6 +165,8 @@ func (gui *Gui) layout(g *gocui.Gui) error { return err } + gui.renderContextOptionsMap() + outer: for { select { diff --git a/pkg/gui/options_map.go b/pkg/gui/options_map.go index a2f1496bc..01bb3e212 100644 --- a/pkg/gui/options_map.go +++ b/pkg/gui/options_map.go @@ -4,9 +4,13 @@ import ( "fmt" "strings" + "github.com/jesseduffield/lazygit/pkg/gui/context" "github.com/jesseduffield/lazygit/pkg/gui/controllers/helpers" "github.com/jesseduffield/lazygit/pkg/gui/keybindings" + "github.com/jesseduffield/lazygit/pkg/gui/style" "github.com/jesseduffield/lazygit/pkg/gui/types" + "github.com/jesseduffield/lazygit/pkg/theme" + "github.com/jesseduffield/lazygit/pkg/utils" "github.com/samber/lo" ) @@ -14,30 +18,89 @@ type OptionsMapMgr struct { c *helpers.HelperCommon } -func (gui *Gui) renderContextOptionsMap(c types.Context) { +func (gui *Gui) renderContextOptionsMap() { // In demos, we render our own content to this view if gui.integrationTest != nil && gui.integrationTest.IsDemo() { return } mgr := OptionsMapMgr{c: gui.c} - mgr.renderContextOptionsMap(c) + mgr.renderContextOptionsMap() } -// render the options available for the current context at the bottom of the screen -func (self *OptionsMapMgr) renderContextOptionsMap(c types.Context) { - bindingsToDisplay := lo.Filter(c.GetKeybindings(self.c.KeybindingsOpts()), func(binding *types.Binding, _ int) bool { - return binding.Display +// Render the options available for the current context at the bottom of the screen +// STYLE GUIDE: we use the default options fg color for most keybindings. We can +// only use a different color if we're in a specific mode where the user is likely +// to want to press that key. For example, when in cherry-picking mode, we +// want to prominently show the keybinding for pasting commits. +func (self *OptionsMapMgr) renderContextOptionsMap() { + currentContext := self.c.CurrentContext() + + currentContextBindings := currentContext.GetKeybindings(self.c.KeybindingsOpts()) + globalBindings := self.c.Contexts().Global.GetKeybindings(self.c.KeybindingsOpts()) + + allBindings := append(currentContextBindings, globalBindings...) + + bindingsToDisplay := lo.Filter(allBindings, func(binding *types.Binding, _ int) bool { + return binding.DisplayOnScreen && !binding.IsDisabled() }) - var optionsMap []bindingInfo - if len(bindingsToDisplay) == 0 { - optionsMap = self.globalOptions() - } else { - optionsMap = lo.Map(bindingsToDisplay, func(binding *types.Binding, _ int) bindingInfo { - return bindingInfo{ - key: keybindings.LabelFromKey(binding.Key), - description: binding.Description, - } + optionsMap := lo.Map(bindingsToDisplay, func(binding *types.Binding, _ int) bindingInfo { + displayStyle := theme.OptionsFgColor + if binding.DisplayStyle != nil { + displayStyle = *binding.DisplayStyle + } + + description := binding.Description + if binding.ShortDescription != "" { + description = binding.ShortDescription + } + + return bindingInfo{ + key: keybindings.LabelFromKey(binding.Key), + description: description, + style: displayStyle, + } + }) + + // Mode-specific local keybindings + if currentContext.GetKey() == context.LOCAL_COMMITS_CONTEXT_KEY { + if self.c.Modes().CherryPicking.Active() { + optionsMap = utils.Prepend(optionsMap, bindingInfo{ + key: keybindings.Label(self.c.KeybindingsOpts().Config.Commits.PasteCommits), + description: self.c.Tr.PasteCommits, + style: style.FgCyan, + }) + } + + if self.c.Model().BisectInfo.Started() { + optionsMap = utils.Prepend(optionsMap, bindingInfo{ + key: keybindings.Label(self.c.KeybindingsOpts().Config.Commits.ViewBisectOptions), + description: self.c.Tr.ViewBisectOptions, + style: style.FgGreen, + }) + } + } + + // Mode-specific global keybindings + if self.c.Model().WorkingTreeStateAtLastCommitRefresh.IsRebasing() { + optionsMap = utils.Prepend(optionsMap, bindingInfo{ + key: keybindings.Label(self.c.KeybindingsOpts().Config.Universal.CreateRebaseOptionsMenu), + description: self.c.Tr.ViewRebaseOptions, + style: style.FgYellow, + }) + } else if self.c.Model().WorkingTreeStateAtLastCommitRefresh.IsMerging() { + optionsMap = utils.Prepend(optionsMap, bindingInfo{ + key: keybindings.Label(self.c.KeybindingsOpts().Config.Universal.CreateRebaseOptionsMenu), + description: self.c.Tr.ViewMergeOptions, + style: style.FgYellow, + }) + } + + if self.c.Git().Patch.PatchBuilder.Active() { + optionsMap = utils.Prepend(optionsMap, bindingInfo{ + key: keybindings.Label(self.c.KeybindingsOpts().Config.Universal.CreatePatchOptionsMenu), + description: self.c.Tr.ViewPatchOptions, + style: style.FgYellow, }) } @@ -45,49 +108,41 @@ func (self *OptionsMapMgr) renderContextOptionsMap(c types.Context) { } func (self *OptionsMapMgr) formatBindingInfos(bindingInfos []bindingInfo) string { - return strings.Join( - lo.Map(bindingInfos, func(bindingInfo bindingInfo, _ int) string { - return fmt.Sprintf("%s: %s", bindingInfo.key, bindingInfo.description) - }), - ", ") + width := self.c.Views().Options.Width() - 4 // -4 for the padding + var builder strings.Builder + ellipsis := "…" + separator := " | " + + length := 0 + + for i, info := range bindingInfos { + plainText := fmt.Sprintf("%s: %s", info.description, info.key) + + // Check if adding the next formatted string exceeds the available width + if i > 0 && length+len(separator)+len(plainText) > width { + builder.WriteString(theme.OptionsFgColor.Sprint(separator + ellipsis)) + break + } + + formatted := info.style.Sprintf(plainText) + + if i > 0 { + builder.WriteString(theme.OptionsFgColor.Sprint(separator)) + length += len(separator) + } + builder.WriteString(formatted) + length += len(plainText) + } + + return builder.String() } func (self *OptionsMapMgr) renderOptions(options string) { self.c.SetViewContent(self.c.Views().Options, options) } -func (self *OptionsMapMgr) globalOptions() []bindingInfo { - keybindingConfig := self.c.UserConfig.Keybinding - - return []bindingInfo{ - { - key: fmt.Sprintf("%s/%s", keybindings.Label(keybindingConfig.Universal.ScrollUpMain), keybindings.Label(keybindingConfig.Universal.ScrollDownMain)), - description: self.c.Tr.Scroll, - }, - { - key: keybindings.Label(keybindingConfig.Universal.Return), - description: self.c.Tr.Cancel, - }, - { - key: keybindings.Label(keybindingConfig.Universal.Quit), - description: self.c.Tr.Quit, - }, - { - key: keybindings.Label(keybindingConfig.Universal.OptionMenuAlt1), - description: self.c.Tr.Keybindings, - }, - { - key: fmt.Sprintf("%s-%s", keybindings.Label(keybindingConfig.Universal.JumpToBlock[0]), keybindings.Label(keybindingConfig.Universal.JumpToBlock[len(keybindingConfig.Universal.JumpToBlock)-1])), - description: self.c.Tr.Jump, - }, - { - key: fmt.Sprintf("%s/%s", keybindings.Label(keybindingConfig.Universal.ScrollLeft), keybindings.Label(keybindingConfig.Universal.ScrollRight)), - description: self.c.Tr.ScrollLeftRight, - }, - } -} - type bindingInfo struct { key string description string + style style.TextStyle } diff --git a/pkg/gui/types/keybindings.go b/pkg/gui/types/keybindings.go index 7d2c19bd4..db21e7bc9 100644 --- a/pkg/gui/types/keybindings.go +++ b/pkg/gui/types/keybindings.go @@ -11,21 +11,25 @@ type Key interface{} // FIXME: find out how to get `gocui.Key | rune` // is only handled if the given view has focus, or handled globally if the view // is "" type Binding struct { - ViewName string - Handler func() error - Key Key - Modifier gocui.Modifier - Description string + ViewName string + Handler func() error + Key Key + Modifier gocui.Modifier + Description string + // If defined, this is used in place of Description when showing the keybinding + // in the options view at the bottom left of the screen. ShortDescription string Alternative string Tag string // e.g. 'navigation'. Used for grouping things in the cheatsheet OpensMenu bool - // If true, the keybinding will appear at the bottom of the screen. If - // the given view has no bindings with Display: true, the default keybindings - // will be displayed instead. - // TODO: implement this - Display bool + // If true, the keybinding will appear at the bottom of the screen. + // Even if set to true, the keybinding will not be displayed if it is currently + // disabled. We could instead display it with a strikethrough, but there's + // limited realestate to show all the keybindings we want, so we're hiding it instead. + DisplayOnScreen bool + // if unset, the binding will be displayed in the default color. Only applies to the keybinding + // on-screen, not in the keybindings menu. DisplayStyle *style.TextStyle // to be displayed if the keybinding is highlighted from within a menu @@ -39,6 +43,10 @@ type Binding struct { GetDisabledReason func() *DisabledReason } +func (Binding *Binding) IsDisabled() bool { + return Binding.GetDisabledReason != nil && Binding.GetDisabledReason() != nil +} + // A guard is a decorator which checks something before executing a handler // and potentially early-exits if some precondition hasn't been met. type Guard func(func() error) func() error diff --git a/pkg/gui/views.go b/pkg/gui/views.go index be279c9d4..13caa9c7f 100644 --- a/pkg/gui/views.go +++ b/pkg/gui/views.go @@ -94,7 +94,6 @@ func (gui *Gui) createAllViews() error { (*mapping.viewPtr).SelBgColor = theme.GocuiSelectedLineBgColor } - gui.Views.Options.FgColor = theme.OptionsColor gui.Views.Options.Frame = false gui.Views.SearchPrefix.BgColor = gocui.ColorDefault diff --git a/pkg/integration/components/common.go b/pkg/integration/components/common.go index 4f7cd9754..2033a9442 100644 --- a/pkg/integration/components/common.go +++ b/pkg/integration/components/common.go @@ -62,3 +62,29 @@ func (self *Common) SelectPatchOption(matcher *TextMatcher) { self.t.ExpectPopup().Menu().Title(Equals("Patch options")).Select(matcher).Confirm() } + +func (self *Common) ResetBisect() { + self.t.Views().Commits(). + Focus(). + Press(self.t.keys.Commits.ViewBisectOptions). + Tap(func() { + self.t.ExpectPopup().Menu(). + Title(Equals("Bisect")). + Select(Contains("Reset bisect")). + Confirm() + + self.t.ExpectPopup().Confirmation(). + Title(Equals("Reset 'git bisect'")). + Content(Contains("Are you sure you want to reset 'git bisect'?")). + Confirm() + }) +} + +func (self *Common) ResetCustomPatch() { + self.t.GlobalPress(self.t.keys.Universal.CreatePatchOptionsMenu) + + self.t.ExpectPopup().Menu(). + Title(Equals("Patch options")). + Select(Contains("Reset patch")). + Confirm() +} diff --git a/pkg/integration/components/views.go b/pkg/integration/components/views.go index edb2b85b6..873aca650 100644 --- a/pkg/integration/components/views.go +++ b/pkg/integration/components/views.go @@ -147,3 +147,7 @@ func (self *Views) Search() *ViewDriver { func (self *Views) Tooltip() *ViewDriver { return self.regularView("tooltip") } + +func (self *Views) Options() *ViewDriver { + return self.regularView("options") +} diff --git a/pkg/integration/tests/test_list.go b/pkg/integration/tests/test_list.go index 2406b2f78..f4693faef 100644 --- a/pkg/integration/tests/test_list.go +++ b/pkg/integration/tests/test_list.go @@ -269,6 +269,7 @@ var tests = []*components.IntegrationTest{ ui.Accordion, ui.DoublePopup, ui.EmptyMenu, + ui.ModeSpecificKeybindingSuggestions, ui.OpenLinkFailure, ui.RangeSelect, ui.SwitchTabFromMenu, diff --git a/pkg/integration/tests/ui/mode_specific_keybinding_suggestions.go b/pkg/integration/tests/ui/mode_specific_keybinding_suggestions.go new file mode 100644 index 000000000..f11e5fd27 --- /dev/null +++ b/pkg/integration/tests/ui/mode_specific_keybinding_suggestions.go @@ -0,0 +1,118 @@ +package ui + +import ( + "github.com/jesseduffield/lazygit/pkg/config" + . "github.com/jesseduffield/lazygit/pkg/integration/components" + "github.com/jesseduffield/lazygit/pkg/integration/tests/shared" +) + +var ModeSpecificKeybindingSuggestions = NewIntegrationTest(NewIntegrationTestArgs{ + Description: "When in various modes, we should corresponding keybinding suggestions onscreen", + ExtraCmdArgs: []string{}, + Skip: false, + SetupConfig: func(config *config.AppConfig) {}, + SetupRepo: func(shell *Shell) { + shell.CreateNCommits(2) + shell.NewBranch("base-branch") + shared.MergeConflictsSetup(shell) + shell.Checkout("base-branch") + }, + Run: func(t *TestDriver, keys config.KeybindingConfig) { + rebaseSuggestion := "View rebase options: m" + cherryPickSuggestion := "Paste (cherry-pick): V" + bisectSuggestion := "View bisect options: b" + customPatchSuggestion := "View custom patch options: " + mergeSuggestion := "View merge options: m" + + t.Views().Commits(). + Focus(). + Lines( + Contains("commit 02").IsSelected(), + Contains("commit 01"), + ). + Tap(func() { + // These suggestions are mode-specific so are not shown by default + t.Views().Options().Content( + DoesNotContain(rebaseSuggestion). + DoesNotContain(mergeSuggestion). + DoesNotContain(cherryPickSuggestion). + DoesNotContain(bisectSuggestion). + DoesNotContain(customPatchSuggestion), + ) + }). + // Start an interactive rebase + Press(keys.Universal.Edit). + Tap(func() { + // Confirm the rebase suggestion now appears + t.Views().Options().Content(Contains(rebaseSuggestion)) + }). + Press(keys.Commits.CherryPickCopy). + Tap(func() { + // Confirm the cherry pick suggestion now appears + t.Views().Options().Content(Contains(cherryPickSuggestion)) + // Importantly, we show multiple of these suggestions at once + t.Views().Options().Content(Contains(rebaseSuggestion)) + }). + // Cancel the cherry pick + PressEscape(). + Tap(func() { + t.Views().Options().Content(DoesNotContain(cherryPickSuggestion)) + }). + // Cancel the rebase + Tap(func() { + t.Common().AbortRebase() + + t.Views().Options().Content(DoesNotContain(rebaseSuggestion)) + }). + Press(keys.Commits.ViewBisectOptions). + Tap(func() { + t.ExpectPopup().Menu(). + Title(Equals("Bisect")). + Select(MatchesRegexp("Mark.* as bad")). + Confirm() + + t.Views().Options().Content(Contains(bisectSuggestion)) + + // Cancel bisect + t.Common().ResetBisect() + + t.Views().Options().Content(DoesNotContain(bisectSuggestion)) + }). + // Enter commit files view + PressEnter() + + t.Views().CommitFiles(). + IsFocused(). + // Add a commit file to the patch + Press(keys.Universal.Select). + Tap(func() { + t.Views().Options().Content(Contains(customPatchSuggestion)) + + t.Common().ResetCustomPatch() + + t.Views().Options().Content(DoesNotContain(customPatchSuggestion)) + }) + + // Test merge options suggestion + t.Views().Branches(). + Focus(). + NavigateToLine(Contains("first-change-branch")). + Press(keys.Universal.Select). + NavigateToLine(Contains("second-change-branch")). + Press(keys.Branches.MergeIntoCurrentBranch). + Tap(func() { + t.ExpectPopup().Confirmation(). + Title(Equals("Merge")). + Content(Contains("Are you sure you want to merge")). + Confirm() + + t.Common().AcknowledgeConflicts() + + t.Views().Options().Content(Contains(mergeSuggestion)) + + t.Common().AbortMerge() + + t.Views().Options().Content(DoesNotContain(mergeSuggestion)) + }) + }, +}) From 1a38d515d78ceb60c2f88813c2c3ba8708315e48 Mon Sep 17 00:00:00 2001 From: Jesse Duffield Date: Fri, 26 Jan 2024 10:03:04 +1100 Subject: [PATCH 3/4] Display more keybindings on-screen --- .../controllers/basic_commits_controller.go | 3 ++ pkg/gui/controllers/branches_controller.go | 15 ++++++--- .../controllers/commits_files_controller.go | 4 +++ pkg/gui/controllers/files_controller.go | 32 +++++++++++-------- .../controllers/local_commits_controller.go | 32 +++++++++++++------ .../controllers/merge_conflicts_controller.go | 28 +++++++++------- .../controllers/patch_building_controller.go | 7 ++-- .../controllers/patch_explorer_controller.go | 9 +++--- .../controllers/remote_branches_controller.go | 5 +++ pkg/gui/controllers/remotes_controller.go | 11 +++++-- pkg/gui/controllers/staging_controller.go | 27 +++++++++------- pkg/gui/controllers/stash_controller.go | 3 ++ pkg/gui/controllers/status_controller.go | 23 +++++++------ pkg/gui/controllers/submodules_controller.go | 10 ++++-- pkg/gui/controllers/tags_controller.go | 13 +++++--- pkg/gui/controllers/worktrees_controller.go | 9 ++++-- 16 files changed, 151 insertions(+), 80 deletions(-) diff --git a/pkg/gui/controllers/basic_commits_controller.go b/pkg/gui/controllers/basic_commits_controller.go index c62818df9..45239ef68 100644 --- a/pkg/gui/controllers/basic_commits_controller.go +++ b/pkg/gui/controllers/basic_commits_controller.go @@ -52,6 +52,7 @@ func (self *BasicCommitsController) GetKeybindings(opts types.KeybindingsOpts) [ GetDisabledReason: self.require(self.singleItemSelected()), Description: self.c.Tr.Checkout, Tooltip: self.c.Tr.CheckoutCommitTooltip, + DisplayOnScreen: true, }, { Key: opts.GetKey(opts.Config.Commits.CopyCommitAttributeToClipboard), @@ -80,6 +81,7 @@ func (self *BasicCommitsController) GetKeybindings(opts types.KeybindingsOpts) [ Description: self.c.Tr.ViewResetOptions, Tooltip: self.c.Tr.ResetTooltip, OpensMenu: true, + DisplayOnScreen: true, }, { Key: opts.GetKey(opts.Config.Commits.CherryPickCopy), @@ -91,6 +93,7 @@ func (self *BasicCommitsController) GetKeybindings(opts types.KeybindingsOpts) [ "escape": keybindings.Label(opts.Config.Universal.Return), }, ), + DisplayOnScreen: true, }, { Key: opts.GetKey(opts.Config.Commits.ResetCherryPick), diff --git a/pkg/gui/controllers/branches_controller.go b/pkg/gui/controllers/branches_controller.go index 002aca4fb..068238ec7 100644 --- a/pkg/gui/controllers/branches_controller.go +++ b/pkg/gui/controllers/branches_controller.go @@ -47,14 +47,16 @@ func (self *BranchesController) GetKeybindings(opts types.KeybindingsOpts) []*ty self.singleItemSelected(), self.notPulling, ), - Description: self.c.Tr.Checkout, - Tooltip: self.c.Tr.CheckoutTooltip, + Description: self.c.Tr.Checkout, + Tooltip: self.c.Tr.CheckoutTooltip, + DisplayOnScreen: true, }, { Key: opts.GetKey(opts.Config.Universal.New), Handler: self.withItem(self.newBranch), GetDisabledReason: self.require(self.singleItemSelected()), Description: self.c.Tr.NewBranch, + DisplayOnScreen: true, }, { Key: opts.GetKey(opts.Config.Branches.CreatePullRequest), @@ -95,6 +97,7 @@ func (self *BranchesController) GetKeybindings(opts types.KeybindingsOpts) []*ty Description: self.c.Tr.Delete, Tooltip: self.c.Tr.BranchDeleteTooltip, OpensMenu: true, + DisplayOnScreen: true, }, { Key: opts.GetKey(opts.Config.Branches.RebaseBranch), @@ -102,8 +105,9 @@ func (self *BranchesController) GetKeybindings(opts types.KeybindingsOpts) []*ty GetDisabledReason: self.require( self.singleItemSelected(self.notRebasingOntoSelf), ), - Description: self.c.Tr.RebaseBranch, - Tooltip: self.c.Tr.RebaseBranchTooltip, + Description: self.c.Tr.RebaseBranch, + Tooltip: self.c.Tr.RebaseBranchTooltip, + DisplayOnScreen: true, }, { Key: opts.GetKey(opts.Config.Branches.MergeIntoCurrentBranch), @@ -111,6 +115,7 @@ func (self *BranchesController) GetKeybindings(opts types.KeybindingsOpts) []*ty GetDisabledReason: self.require(self.singleItemSelected()), Description: self.c.Tr.Merge, Tooltip: self.c.Tr.MergeBranchTooltip, + DisplayOnScreen: true, }, { Key: opts.GetKey(opts.Config.Branches.FastForward), @@ -136,6 +141,7 @@ func (self *BranchesController) GetKeybindings(opts types.KeybindingsOpts) []*ty GetDisabledReason: self.require(self.singleItemSelected()), Description: self.c.Tr.ViewResetOptions, OpensMenu: true, + DisplayOnScreen: true, }, { Key: opts.GetKey(opts.Config.Branches.RenameBranch), @@ -151,6 +157,7 @@ func (self *BranchesController) GetKeybindings(opts types.KeybindingsOpts) []*ty Tooltip: self.c.Tr.ViewBranchUpstreamOptionsTooltip, ShortDescription: self.c.Tr.Upstream, OpensMenu: true, + DisplayOnScreen: true, }, } } diff --git a/pkg/gui/controllers/commits_files_controller.go b/pkg/gui/controllers/commits_files_controller.go index 3ff1e50e3..e2ed4b3f0 100644 --- a/pkg/gui/controllers/commits_files_controller.go +++ b/pkg/gui/controllers/commits_files_controller.go @@ -43,6 +43,7 @@ func (self *CommitFilesController) GetKeybindings(opts types.KeybindingsOpts) [] GetDisabledReason: self.require(self.singleItemSelected()), Description: self.c.Tr.Checkout, Tooltip: self.c.Tr.CheckoutCommitFileTooltip, + DisplayOnScreen: true, }, { Key: opts.GetKey(opts.Config.Universal.Remove), @@ -50,6 +51,7 @@ func (self *CommitFilesController) GetKeybindings(opts types.KeybindingsOpts) [] GetDisabledReason: self.require(self.singleItemSelected()), Description: self.c.Tr.Remove, Tooltip: self.c.Tr.DiscardOldFileChangeTooltip, + DisplayOnScreen: true, }, { Key: opts.GetKey(opts.Config.Universal.OpenFile), @@ -64,6 +66,7 @@ func (self *CommitFilesController) GetKeybindings(opts types.KeybindingsOpts) [] GetDisabledReason: self.require(self.singleItemSelected()), Description: self.c.Tr.Edit, Tooltip: self.c.Tr.EditFileTooltip, + DisplayOnScreen: true, }, { Key: opts.GetKey(opts.Config.Universal.OpenDiffTool), @@ -79,6 +82,7 @@ func (self *CommitFilesController) GetKeybindings(opts types.KeybindingsOpts) [] Tooltip: utils.ResolvePlaceholderString(self.c.Tr.ToggleAddToPatchTooltip, map[string]string{"doc": constants.Links.Docs.CustomPatchDemo}, ), + DisplayOnScreen: true, }, { Key: opts.GetKey(opts.Config.Files.ToggleStagedAll), diff --git a/pkg/gui/controllers/files_controller.go b/pkg/gui/controllers/files_controller.go index c450b6fe9..504617218 100644 --- a/pkg/gui/controllers/files_controller.go +++ b/pkg/gui/controllers/files_controller.go @@ -43,6 +43,7 @@ func (self *FilesController) GetKeybindings(opts types.KeybindingsOpts) []*types GetDisabledReason: self.require(self.itemsSelected()), Description: self.c.Tr.Stage, Tooltip: self.c.Tr.StageTooltip, + DisplayOnScreen: true, }, { Key: opts.GetKey(opts.Config.Files.OpenStatusFilter), @@ -56,10 +57,11 @@ func (self *FilesController) GetKeybindings(opts types.KeybindingsOpts) []*types OpensMenu: true, }, { - Key: opts.GetKey(opts.Config.Files.CommitChanges), - Handler: self.c.Helpers().WorkingTree.HandleCommitPress, - Description: self.c.Tr.Commit, - Tooltip: self.c.Tr.CommitTooltip, + Key: opts.GetKey(opts.Config.Files.CommitChanges), + Handler: self.c.Helpers().WorkingTree.HandleCommitPress, + Description: self.c.Tr.Commit, + Tooltip: self.c.Tr.CommitTooltip, + DisplayOnScreen: true, }, { Key: opts.GetKey(opts.Config.Files.CommitChangesWithoutHook), @@ -88,6 +90,7 @@ func (self *FilesController) GetKeybindings(opts types.KeybindingsOpts) []*types GetDisabledReason: self.require(self.singleItemSelected()), Description: self.c.Tr.Edit, Tooltip: self.c.Tr.EditFileTooltip, + DisplayOnScreen: true, }, { Key: opts.GetKey(opts.Config.Universal.OpenFile), @@ -109,10 +112,11 @@ func (self *FilesController) GetKeybindings(opts types.KeybindingsOpts) []*types Description: self.c.Tr.RefreshFiles, }, { - Key: opts.GetKey(opts.Config.Files.StashAllChanges), - Handler: self.stash, - Description: self.c.Tr.Stash, - Tooltip: self.c.Tr.StashTooltip, + Key: opts.GetKey(opts.Config.Files.StashAllChanges), + Handler: self.stash, + Description: self.c.Tr.Stash, + Tooltip: self.c.Tr.StashTooltip, + DisplayOnScreen: true, }, { Key: opts.GetKey(opts.Config.Files.ViewStashOptions), @@ -141,6 +145,7 @@ func (self *FilesController) GetKeybindings(opts types.KeybindingsOpts) []*types Description: self.c.Tr.Discard, Tooltip: self.c.Tr.DiscardFileChangesTooltip, OpensMenu: true, + DisplayOnScreen: true, }, { Key: opts.GetKey(opts.Config.Commits.ViewResetOptions), @@ -149,11 +154,12 @@ func (self *FilesController) GetKeybindings(opts types.KeybindingsOpts) []*types OpensMenu: true, }, { - Key: opts.GetKey(opts.Config.Files.ViewResetOptions), - Handler: self.createResetMenu, - Description: self.c.Tr.Reset, - Tooltip: self.c.Tr.FileResetOptionsTooltip, - OpensMenu: true, + Key: opts.GetKey(opts.Config.Files.ViewResetOptions), + Handler: self.createResetMenu, + Description: self.c.Tr.Reset, + Tooltip: self.c.Tr.FileResetOptionsTooltip, + OpensMenu: true, + DisplayOnScreen: true, }, { Key: opts.GetKey(opts.Config.Files.ToggleTreeView), diff --git a/pkg/gui/controllers/local_commits_controller.go b/pkg/gui/controllers/local_commits_controller.go index 25abd2c85..3dc1bae7f 100644 --- a/pkg/gui/controllers/local_commits_controller.go +++ b/pkg/gui/controllers/local_commits_controller.go @@ -64,8 +64,9 @@ func (self *LocalCommitsController) GetKeybindings(opts types.KeybindingsOpts) [ self.canSquashOrFixup, ), ), - Description: self.c.Tr.Squash, - Tooltip: self.c.Tr.SquashTooltip, + Description: self.c.Tr.Squash, + Tooltip: self.c.Tr.SquashTooltip, + DisplayOnScreen: true, }, { Key: opts.GetKey(opts.Config.Commits.MarkCommitAsFixup), @@ -76,8 +77,9 @@ func (self *LocalCommitsController) GetKeybindings(opts types.KeybindingsOpts) [ self.canSquashOrFixup, ), ), - Description: self.c.Tr.Fixup, - Tooltip: self.c.Tr.FixupTooltip, + Description: self.c.Tr.Fixup, + Tooltip: self.c.Tr.FixupTooltip, + DisplayOnScreen: true, }, { Key: opts.GetKey(opts.Config.Commits.RenameCommit), @@ -85,9 +87,10 @@ func (self *LocalCommitsController) GetKeybindings(opts types.KeybindingsOpts) [ GetDisabledReason: self.require( self.singleItemSelected(self.rewordEnabled), ), - Description: self.c.Tr.Reword, - Tooltip: self.c.Tr.CommitRewordTooltip, - OpensMenu: true, + Description: self.c.Tr.Reword, + Tooltip: self.c.Tr.CommitRewordTooltip, + DisplayOnScreen: true, + OpensMenu: true, }, { Key: opts.GetKey(opts.Config.Commits.RenameCommitWithEditor), @@ -105,8 +108,9 @@ func (self *LocalCommitsController) GetKeybindings(opts types.KeybindingsOpts) [ self.midRebaseCommandEnabled, ), ), - Description: self.c.Tr.DropCommit, - Tooltip: self.c.Tr.DropCommitTooltip, + Description: self.c.Tr.DropCommit, + Tooltip: self.c.Tr.DropCommitTooltip, + DisplayOnScreen: true, }, { Key: opts.GetKey(editCommitKey), @@ -118,6 +122,7 @@ func (self *LocalCommitsController) GetKeybindings(opts types.KeybindingsOpts) [ Description: self.c.Tr.EditCommit, ShortDescription: self.c.Tr.Edit, Tooltip: self.c.Tr.EditCommitTooltip, + DisplayOnScreen: true, }, { // The user-facing description here is 'Start interactive rebase' but internally @@ -139,6 +144,14 @@ func (self *LocalCommitsController) GetKeybindings(opts types.KeybindingsOpts) [ ), Description: self.c.Tr.Pick, Tooltip: self.c.Tr.PickCommitTooltip, + // Not displaying this because we only want to display it when a TODO commit + // is selected. A keybinding is displayed in the options view if Display is true, + // and if it's not disabled, but if we disable it whenever a non-TODO commit is + // selected, we'll be preventing pulls from happening within the commits view + // (given they both use the 'p' key). Some approaches that come to mind: + // * Allow a disabled keybinding to conditionally fallback to a global keybinding + // * Allow a separate way of deciding whether a keybinding is displayed in the options view + DisplayOnScreen: false, }, { Key: opts.GetKey(opts.Config.Commits.CreateFixupCommit), @@ -221,6 +234,7 @@ func (self *LocalCommitsController) GetKeybindings(opts types.KeybindingsOpts) [ GetDisabledReason: self.require(self.singleItemSelected(self.canAmend)), Description: self.c.Tr.Amend, Tooltip: self.c.Tr.AmendCommitTooltip, + DisplayOnScreen: true, }, { Key: opts.GetKey(opts.Config.Commits.ResetCommitAuthor), diff --git a/pkg/gui/controllers/merge_conflicts_controller.go b/pkg/gui/controllers/merge_conflicts_controller.go index 03ca4a10b..1f22e62bc 100644 --- a/pkg/gui/controllers/merge_conflicts_controller.go +++ b/pkg/gui/controllers/merge_conflicts_controller.go @@ -28,24 +28,28 @@ func NewMergeConflictsController( func (self *MergeConflictsController) GetKeybindings(opts types.KeybindingsOpts) []*types.Binding { bindings := []*types.Binding{ { - Key: opts.GetKey(opts.Config.Universal.Select), - Handler: self.withRenderAndFocus(self.HandlePickHunk), - Description: self.c.Tr.PickHunk, + Key: opts.GetKey(opts.Config.Universal.Select), + Handler: self.withRenderAndFocus(self.HandlePickHunk), + Description: self.c.Tr.PickHunk, + DisplayOnScreen: true, }, { - Key: opts.GetKey(opts.Config.Main.PickBothHunks), - Handler: self.withRenderAndFocus(self.HandlePickAllHunks), - Description: self.c.Tr.PickAllHunks, + Key: opts.GetKey(opts.Config.Main.PickBothHunks), + Handler: self.withRenderAndFocus(self.HandlePickAllHunks), + Description: self.c.Tr.PickAllHunks, + DisplayOnScreen: true, }, { - Key: opts.GetKey(opts.Config.Universal.PrevItem), - Handler: self.withRenderAndFocus(self.PrevConflictHunk), - Description: self.c.Tr.SelectPrevHunk, + Key: opts.GetKey(opts.Config.Universal.PrevItem), + Handler: self.withRenderAndFocus(self.PrevConflictHunk), + Description: self.c.Tr.SelectPrevHunk, + DisplayOnScreen: true, }, { - Key: opts.GetKey(opts.Config.Universal.NextItem), - Handler: self.withRenderAndFocus(self.NextConflictHunk), - Description: self.c.Tr.SelectNextHunk, + Key: opts.GetKey(opts.Config.Universal.NextItem), + Handler: self.withRenderAndFocus(self.NextConflictHunk), + Description: self.c.Tr.SelectNextHunk, + DisplayOnScreen: true, }, { Key: opts.GetKey(opts.Config.Universal.PrevBlock), diff --git a/pkg/gui/controllers/patch_building_controller.go b/pkg/gui/controllers/patch_building_controller.go index c5141a20e..dbbdb8beb 100644 --- a/pkg/gui/controllers/patch_building_controller.go +++ b/pkg/gui/controllers/patch_building_controller.go @@ -37,9 +37,10 @@ func (self *PatchBuildingController) GetKeybindings(opts types.KeybindingsOpts) Tooltip: self.c.Tr.EditFileTooltip, }, { - Key: opts.GetKey(opts.Config.Universal.Select), - Handler: self.ToggleSelectionAndRefresh, - Description: self.c.Tr.ToggleSelectionForPatch, + Key: opts.GetKey(opts.Config.Universal.Select), + Handler: self.ToggleSelectionAndRefresh, + Description: self.c.Tr.ToggleSelectionForPatch, + DisplayOnScreen: true, }, { Key: opts.GetKey(opts.Config.Universal.Return), diff --git a/pkg/gui/controllers/patch_explorer_controller.go b/pkg/gui/controllers/patch_explorer_controller.go index 1f6ef97dd..9d736df10 100644 --- a/pkg/gui/controllers/patch_explorer_controller.go +++ b/pkg/gui/controllers/patch_explorer_controller.go @@ -92,10 +92,11 @@ func (self *PatchExplorerController) GetKeybindings(opts types.KeybindingsOpts) Description: self.c.Tr.ToggleRangeSelect, }, { - Key: opts.GetKey(opts.Config.Main.ToggleSelectHunk), - Handler: self.withRenderAndFocus(self.HandleToggleSelectHunk), - Description: self.c.Tr.ToggleSelectHunk, - Tooltip: self.c.Tr.ToggleSelectHunkTooltip, + Key: opts.GetKey(opts.Config.Main.ToggleSelectHunk), + Handler: self.withRenderAndFocus(self.HandleToggleSelectHunk), + Description: self.c.Tr.ToggleSelectHunk, + Tooltip: self.c.Tr.ToggleSelectHunkTooltip, + DisplayOnScreen: true, }, { Tag: "navigation", diff --git a/pkg/gui/controllers/remote_branches_controller.go b/pkg/gui/controllers/remote_branches_controller.go index f4998ef28..0cfdfbcd5 100644 --- a/pkg/gui/controllers/remote_branches_controller.go +++ b/pkg/gui/controllers/remote_branches_controller.go @@ -41,6 +41,7 @@ func (self *RemoteBranchesController) GetKeybindings(opts types.KeybindingsOpts) GetDisabledReason: self.require(self.singleItemSelected()), Description: self.c.Tr.Checkout, Tooltip: self.c.Tr.RemoteBranchCheckoutTooltip, + DisplayOnScreen: true, }, { Key: opts.GetKey(opts.Config.Universal.New), @@ -54,6 +55,7 @@ func (self *RemoteBranchesController) GetKeybindings(opts types.KeybindingsOpts) GetDisabledReason: self.require(self.singleItemSelected()), Description: self.c.Tr.Merge, Tooltip: self.c.Tr.MergeBranchTooltip, + DisplayOnScreen: true, }, { Key: opts.GetKey(opts.Config.Branches.RebaseBranch), @@ -61,6 +63,7 @@ func (self *RemoteBranchesController) GetKeybindings(opts types.KeybindingsOpts) GetDisabledReason: self.require(self.singleItemSelected()), Description: self.c.Tr.RebaseBranch, Tooltip: self.c.Tr.RebaseBranchTooltip, + DisplayOnScreen: true, }, { Key: opts.GetKey(opts.Config.Universal.Remove), @@ -68,6 +71,7 @@ func (self *RemoteBranchesController) GetKeybindings(opts types.KeybindingsOpts) GetDisabledReason: self.require(self.singleItemSelected()), Description: self.c.Tr.Delete, Tooltip: self.c.Tr.DeleteRemoteBranchTooltip, + DisplayOnScreen: true, }, { Key: opts.GetKey(opts.Config.Branches.SetUpstream), @@ -75,6 +79,7 @@ func (self *RemoteBranchesController) GetKeybindings(opts types.KeybindingsOpts) GetDisabledReason: self.require(self.singleItemSelected()), Description: self.c.Tr.SetAsUpstream, Tooltip: self.c.Tr.SetAsUpstreamTooltip, + DisplayOnScreen: true, }, { Key: opts.GetKey(opts.Config.Branches.SortOrder), diff --git a/pkg/gui/controllers/remotes_controller.go b/pkg/gui/controllers/remotes_controller.go index 82e3db0e6..37c1273ed 100644 --- a/pkg/gui/controllers/remotes_controller.go +++ b/pkg/gui/controllers/remotes_controller.go @@ -46,11 +46,13 @@ func (self *RemotesController) GetKeybindings(opts types.KeybindingsOpts) []*typ Handler: self.withItem(self.enter), GetDisabledReason: self.require(self.singleItemSelected()), Description: self.c.Tr.ViewBranches, + DisplayOnScreen: true, }, { - Key: opts.GetKey(opts.Config.Universal.New), - Handler: self.add, - Description: self.c.Tr.NewRemote, + Key: opts.GetKey(opts.Config.Universal.New), + Handler: self.add, + Description: self.c.Tr.NewRemote, + DisplayOnScreen: true, }, { Key: opts.GetKey(opts.Config.Universal.Remove), @@ -58,6 +60,7 @@ func (self *RemotesController) GetKeybindings(opts types.KeybindingsOpts) []*typ GetDisabledReason: self.require(self.singleItemSelected()), Description: self.c.Tr.Remove, Tooltip: self.c.Tr.RemoveRemoteTooltip, + DisplayOnScreen: true, }, { Key: opts.GetKey(opts.Config.Universal.Edit), @@ -65,6 +68,7 @@ func (self *RemotesController) GetKeybindings(opts types.KeybindingsOpts) []*typ GetDisabledReason: self.require(self.singleItemSelected()), Description: self.c.Tr.Edit, Tooltip: self.c.Tr.EditRemoteTooltip, + DisplayOnScreen: true, }, { Key: opts.GetKey(opts.Config.Branches.FetchRemote), @@ -72,6 +76,7 @@ func (self *RemotesController) GetKeybindings(opts types.KeybindingsOpts) []*typ GetDisabledReason: self.require(self.singleItemSelected()), Description: self.c.Tr.Fetch, Tooltip: self.c.Tr.FetchRemoteTooltip, + DisplayOnScreen: true, }, } diff --git a/pkg/gui/controllers/staging_controller.go b/pkg/gui/controllers/staging_controller.go index d00f71d9b..13d896be8 100644 --- a/pkg/gui/controllers/staging_controller.go +++ b/pkg/gui/controllers/staging_controller.go @@ -40,16 +40,18 @@ func NewStagingController( func (self *StagingController) GetKeybindings(opts types.KeybindingsOpts) []*types.Binding { return []*types.Binding{ { - Key: opts.GetKey(opts.Config.Universal.Select), - Handler: self.ToggleStaged, - Description: self.c.Tr.Stage, - Tooltip: self.c.Tr.StageSelectionTooltip, + Key: opts.GetKey(opts.Config.Universal.Select), + Handler: self.ToggleStaged, + Description: self.c.Tr.Stage, + Tooltip: self.c.Tr.StageSelectionTooltip, + DisplayOnScreen: true, }, { - Key: opts.GetKey(opts.Config.Universal.Remove), - Handler: self.DiscardSelection, - Description: self.c.Tr.DiscardSelection, - Tooltip: self.c.Tr.DiscardSelectionTooltip, + Key: opts.GetKey(opts.Config.Universal.Remove), + Handler: self.DiscardSelection, + Description: self.c.Tr.DiscardSelection, + Tooltip: self.c.Tr.DiscardSelectionTooltip, + DisplayOnScreen: true, }, { Key: opts.GetKey(opts.Config.Universal.OpenFile), @@ -69,10 +71,11 @@ func (self *StagingController) GetKeybindings(opts types.KeybindingsOpts) []*typ Description: self.c.Tr.ReturnToFilesPanel, }, { - Key: opts.GetKey(opts.Config.Universal.TogglePanel), - Handler: self.TogglePanel, - Description: self.c.Tr.ToggleStagingView, - Tooltip: self.c.Tr.ToggleStagingViewTooltip, + Key: opts.GetKey(opts.Config.Universal.TogglePanel), + Handler: self.TogglePanel, + Description: self.c.Tr.ToggleStagingView, + Tooltip: self.c.Tr.ToggleStagingViewTooltip, + DisplayOnScreen: true, }, { Key: opts.GetKey(opts.Config.Main.EditSelectHunk), diff --git a/pkg/gui/controllers/stash_controller.go b/pkg/gui/controllers/stash_controller.go index 21e4f9cda..8b5e068b5 100644 --- a/pkg/gui/controllers/stash_controller.go +++ b/pkg/gui/controllers/stash_controller.go @@ -38,6 +38,7 @@ func (self *StashController) GetKeybindings(opts types.KeybindingsOpts) []*types GetDisabledReason: self.require(self.singleItemSelected()), Description: self.c.Tr.Apply, Tooltip: self.c.Tr.StashApplyTooltip, + DisplayOnScreen: true, }, { Key: opts.GetKey(opts.Config.Stash.PopStash), @@ -45,6 +46,7 @@ func (self *StashController) GetKeybindings(opts types.KeybindingsOpts) []*types GetDisabledReason: self.require(self.singleItemSelected()), Description: self.c.Tr.Pop, Tooltip: self.c.Tr.StashPopTooltip, + DisplayOnScreen: true, }, { Key: opts.GetKey(opts.Config.Universal.Remove), @@ -52,6 +54,7 @@ func (self *StashController) GetKeybindings(opts types.KeybindingsOpts) []*types GetDisabledReason: self.require(self.singleItemSelected()), Description: self.c.Tr.Drop, Tooltip: self.c.Tr.StashDropTooltip, + DisplayOnScreen: true, }, { Key: opts.GetKey(opts.Config.Universal.New), diff --git a/pkg/gui/controllers/status_controller.go b/pkg/gui/controllers/status_controller.go index a1addf1c0..f5672ce64 100644 --- a/pkg/gui/controllers/status_controller.go +++ b/pkg/gui/controllers/status_controller.go @@ -39,20 +39,23 @@ func (self *StatusController) GetKeybindings(opts types.KeybindingsOpts) []*type Tooltip: self.c.Tr.OpenFileTooltip, }, { - Key: opts.GetKey(opts.Config.Universal.Edit), - Handler: self.editConfig, - Description: self.c.Tr.EditConfig, - Tooltip: self.c.Tr.EditFileTooltip, + Key: opts.GetKey(opts.Config.Universal.Edit), + Handler: self.editConfig, + Description: self.c.Tr.EditConfig, + Tooltip: self.c.Tr.EditFileTooltip, + DisplayOnScreen: true, }, { - Key: opts.GetKey(opts.Config.Status.CheckForUpdate), - Handler: self.handleCheckForUpdate, - Description: self.c.Tr.CheckForUpdate, + Key: opts.GetKey(opts.Config.Status.CheckForUpdate), + Handler: self.handleCheckForUpdate, + Description: self.c.Tr.CheckForUpdate, + DisplayOnScreen: true, }, { - Key: opts.GetKey(opts.Config.Status.RecentRepos), - Handler: self.c.Helpers().Repos.CreateRecentReposMenu, - Description: self.c.Tr.SwitchRepo, + Key: opts.GetKey(opts.Config.Status.RecentRepos), + Handler: self.c.Helpers().Repos.CreateRecentReposMenu, + Description: self.c.Tr.SwitchRepo, + DisplayOnScreen: true, }, { Key: opts.GetKey(opts.Config.Status.AllBranchesLogGraph), diff --git a/pkg/gui/controllers/submodules_controller.go b/pkg/gui/controllers/submodules_controller.go index 42177852c..3cf3b5bf5 100644 --- a/pkg/gui/controllers/submodules_controller.go +++ b/pkg/gui/controllers/submodules_controller.go @@ -46,6 +46,7 @@ func (self *SubmodulesController) GetKeybindings(opts types.KeybindingsOpts) []* Description: self.c.Tr.Enter, Tooltip: utils.ResolvePlaceholderString(self.c.Tr.EnterSubmoduleTooltip, map[string]string{"escape": keybindings.Label(opts.Config.Universal.Return)}), + DisplayOnScreen: true, }, { Key: opts.GetKey(opts.Config.Universal.Select), @@ -58,6 +59,7 @@ func (self *SubmodulesController) GetKeybindings(opts types.KeybindingsOpts) []* GetDisabledReason: self.require(self.singleItemSelected()), Description: self.c.Tr.Remove, Tooltip: self.c.Tr.RemoveSubmoduleTooltip, + DisplayOnScreen: true, }, { Key: opts.GetKey(opts.Config.Submodules.Update), @@ -65,11 +67,13 @@ func (self *SubmodulesController) GetKeybindings(opts types.KeybindingsOpts) []* GetDisabledReason: self.require(self.singleItemSelected()), Description: self.c.Tr.Update, Tooltip: self.c.Tr.SubmoduleUpdateTooltip, + DisplayOnScreen: true, }, { - Key: opts.GetKey(opts.Config.Universal.New), - Handler: self.add, - Description: self.c.Tr.NewSubmodule, + Key: opts.GetKey(opts.Config.Universal.New), + Handler: self.add, + Description: self.c.Tr.NewSubmodule, + DisplayOnScreen: true, }, { Key: opts.GetKey(opts.Config.Universal.Edit), diff --git a/pkg/gui/controllers/tags_controller.go b/pkg/gui/controllers/tags_controller.go index 3992b485e..3d7609f4b 100644 --- a/pkg/gui/controllers/tags_controller.go +++ b/pkg/gui/controllers/tags_controller.go @@ -39,12 +39,14 @@ func (self *TagsController) GetKeybindings(opts types.KeybindingsOpts) []*types. GetDisabledReason: self.require(self.singleItemSelected()), Description: self.c.Tr.Checkout, Tooltip: self.c.Tr.TagCheckoutTooltip, + DisplayOnScreen: true, }, { - Key: opts.GetKey(opts.Config.Universal.New), - Handler: self.create, - Description: self.c.Tr.NewTag, - Tooltip: self.c.Tr.NewTagTooltip, + Key: opts.GetKey(opts.Config.Universal.New), + Handler: self.create, + Description: self.c.Tr.NewTag, + Tooltip: self.c.Tr.NewTagTooltip, + DisplayOnScreen: true, }, { Key: opts.GetKey(opts.Config.Universal.Remove), @@ -53,6 +55,7 @@ func (self *TagsController) GetKeybindings(opts types.KeybindingsOpts) []*types. GetDisabledReason: self.require(self.singleItemSelected()), Tooltip: self.c.Tr.TagDeleteTooltip, OpensMenu: true, + DisplayOnScreen: true, }, { Key: opts.GetKey(opts.Config.Branches.PushTag), @@ -60,6 +63,7 @@ func (self *TagsController) GetKeybindings(opts types.KeybindingsOpts) []*types. GetDisabledReason: self.require(self.singleItemSelected()), Description: self.c.Tr.PushTag, Tooltip: self.c.Tr.PushTagTooltip, + DisplayOnScreen: true, }, { Key: opts.GetKey(opts.Config.Commits.ViewResetOptions), @@ -67,6 +71,7 @@ func (self *TagsController) GetKeybindings(opts types.KeybindingsOpts) []*types. GetDisabledReason: self.require(self.singleItemSelected()), Description: self.c.Tr.Reset, Tooltip: self.c.Tr.ResetTooltip, + DisplayOnScreen: true, OpensMenu: true, }, } diff --git a/pkg/gui/controllers/worktrees_controller.go b/pkg/gui/controllers/worktrees_controller.go index 3dd56a8ae..e9d15c02a 100644 --- a/pkg/gui/controllers/worktrees_controller.go +++ b/pkg/gui/controllers/worktrees_controller.go @@ -37,9 +37,10 @@ func NewWorktreesController( func (self *WorktreesController) GetKeybindings(opts types.KeybindingsOpts) []*types.Binding { bindings := []*types.Binding{ { - Key: opts.GetKey(opts.Config.Universal.New), - Handler: self.add, - Description: self.c.Tr.NewWorktree, + Key: opts.GetKey(opts.Config.Universal.New), + Handler: self.add, + Description: self.c.Tr.NewWorktree, + DisplayOnScreen: true, }, { Key: opts.GetKey(opts.Config.Universal.Select), @@ -47,6 +48,7 @@ func (self *WorktreesController) GetKeybindings(opts types.KeybindingsOpts) []*t GetDisabledReason: self.require(self.singleItemSelected()), Description: self.c.Tr.Switch, Tooltip: self.c.Tr.SwitchToWorktreeTooltip, + DisplayOnScreen: true, }, { Key: opts.GetKey(opts.Config.Universal.Confirm), @@ -65,6 +67,7 @@ func (self *WorktreesController) GetKeybindings(opts types.KeybindingsOpts) []*t GetDisabledReason: self.require(self.singleItemSelected()), Description: self.c.Tr.Remove, Tooltip: self.c.Tr.RemoveWorktreeTooltip, + DisplayOnScreen: true, }, } From bd7fabef1fec4a6826d7833da7a255bdb93a92d8 Mon Sep 17 00:00:00 2001 From: Jesse Duffield Date: Sun, 28 Jan 2024 09:02:01 +1100 Subject: [PATCH 4/4] Reduce the chance of race condition with list cursor Before this commit, we had pkg/integration/tests/submodule/add.go failing with a panic. I'm pretty sure the issue is this: we're now calling quite a few GetDisabledReason calls on each layout() call, and if a background thread happens to update a model slice while we're doing this, we can end up with a selection index that's now out of bounds because it hasn't been clamped to match the new list length. Specifically, here we had the selected index being -1 (the list starts empty and somehow the value is -1 in this case) and then the list gets a new submodule so the length is now 1, but the list cursor doesn't know about this so remains on the old value. Then we confirm the length is greater than zero and try to get the selected submodule and get an out of bounds error. This commit fixes the issue by clamping the selected index whenever we get the length of the list so that it stays in-sync. This is not a perfect solution because the length can change at any time, but it seems to reliably fix the test, and using mutexes didn't seem to make a difference. Note that we're swapping the order of IFileTree and IListCursor in the file tree view model to ensure that the list cursor's Len() method is called (which performs the clamping). Also, comment from the PR: This 'trait' pattern we're using is convenient but can lead to awkward situations. In this case we have both the list view model and the (embedded) list cursor with a Len() method. The list cursor Len() method just calls the list view model Len() method. But I wanted to make it that the list view model now calls ClampSelection() on the list cursor whenever it obtains the length. This will cause an infinite loop because ClampSelection() internally calls Len() (which calls the list view model's Len() method which in turn calls ClampSelection() again, etc). The only reason we were passing the list view model into the list cursor was to supply the length method, so now we're just doing that directly, and letting the list view model delegate the Len() call to the list cursor, which now itself calls ClampSelection. --- pkg/gui/context/list_view_model.go | 6 +---- pkg/gui/context/traits/list_cursor.go | 24 +++++++++++-------- .../filetree/commit_file_tree_view_model.go | 4 ++-- pkg/gui/filetree/file_tree_view_model.go | 4 ++-- 4 files changed, 19 insertions(+), 19 deletions(-) diff --git a/pkg/gui/context/list_view_model.go b/pkg/gui/context/list_view_model.go index bf8c80e23..1cb2ee648 100644 --- a/pkg/gui/context/list_view_model.go +++ b/pkg/gui/context/list_view_model.go @@ -20,15 +20,11 @@ func NewListViewModel[T HasID](getModel func() []T) *ListViewModel[T] { getModel: getModel, } - self.ListCursor = traits.NewListCursor(self) + self.ListCursor = traits.NewListCursor(func() int { return len(getModel()) }) return self } -func (self *ListViewModel[T]) Len() int { - return len(self.getModel()) -} - func (self *ListViewModel[T]) GetSelected() T { if self.Len() == 0 { return Zero[T]() diff --git a/pkg/gui/context/traits/list_cursor.go b/pkg/gui/context/traits/list_cursor.go index e42f1f5e5..3b4ab1c3d 100644 --- a/pkg/gui/context/traits/list_cursor.go +++ b/pkg/gui/context/traits/list_cursor.go @@ -5,10 +5,6 @@ import ( "github.com/jesseduffield/lazygit/pkg/utils" ) -type HasLength interface { - Len() int -} - type RangeSelectMode int const ( @@ -27,15 +23,17 @@ type ListCursor struct { rangeSelectMode RangeSelectMode // value is ignored when rangeSelectMode is RangeSelectModeNone rangeStartIdx int - list HasLength + // Get the length of the list. We use this to clamp the selection so that + // the selected index is always valid + getLength func() int } -func NewListCursor(list HasLength) *ListCursor { +func NewListCursor(getLength func() int) *ListCursor { return &ListCursor{ selectedIdx: 0, rangeStartIdx: 0, rangeSelectMode: RangeSelectModeNone, - list: list, + getLength: getLength, } } @@ -81,8 +79,9 @@ func (self *ListCursor) GetSelectionRangeAndMode() (int, int, RangeSelectMode) { func (self *ListCursor) clampValue(value int) int { clampedValue := -1 - if self.list.Len() > 0 { - clampedValue = utils.Clamp(value, 0, self.list.Len()-1) + length := self.getLength() + if length > 0 { + clampedValue = utils.Clamp(value, 0, length-1) } return clampedValue @@ -114,7 +113,12 @@ func (self *ListCursor) ClampSelection() { } func (self *ListCursor) Len() int { - return self.list.Len() + // The length of the model slice can change at any time, so the selection may + // become out of bounds. To reduce the likelihood of this, we clamp the selection + // whenever we obtain the length of the model. + self.ClampSelection() + + return self.getLength() } func (self *ListCursor) GetRangeStartIdx() (int, bool) { diff --git a/pkg/gui/filetree/commit_file_tree_view_model.go b/pkg/gui/filetree/commit_file_tree_view_model.go index f5cba0edd..99ed8d477 100644 --- a/pkg/gui/filetree/commit_file_tree_view_model.go +++ b/pkg/gui/filetree/commit_file_tree_view_model.go @@ -22,8 +22,8 @@ type ICommitFileTreeViewModel interface { type CommitFileTreeViewModel struct { sync.RWMutex - ICommitFileTree types.IListCursor + ICommitFileTree // this is e.g. the commit for which we're viewing the files ref types.Ref @@ -37,7 +37,7 @@ var _ ICommitFileTreeViewModel = &CommitFileTreeViewModel{} func NewCommitFileTreeViewModel(getFiles func() []*models.CommitFile, log *logrus.Entry, showTree bool) *CommitFileTreeViewModel { fileTree := NewCommitFileTree(getFiles, log, showTree) - listCursor := traits.NewListCursor(fileTree) + listCursor := traits.NewListCursor(fileTree.Len) return &CommitFileTreeViewModel{ ICommitFileTree: fileTree, IListCursor: listCursor, diff --git a/pkg/gui/filetree/file_tree_view_model.go b/pkg/gui/filetree/file_tree_view_model.go index 439471b01..25b3d0edc 100644 --- a/pkg/gui/filetree/file_tree_view_model.go +++ b/pkg/gui/filetree/file_tree_view_model.go @@ -21,15 +21,15 @@ type IFileTreeViewModel interface { // after the files are refreshed type FileTreeViewModel struct { sync.RWMutex - IFileTree types.IListCursor + IFileTree } var _ IFileTreeViewModel = &FileTreeViewModel{} func NewFileTreeViewModel(getFiles func() []*models.File, log *logrus.Entry, showTree bool) *FileTreeViewModel { fileTree := NewFileTree(getFiles, log, showTree) - listCursor := traits.NewListCursor(fileTree) + listCursor := traits.NewListCursor(fileTree.Len) return &FileTreeViewModel{ IFileTree: fileTree, IListCursor: listCursor,