Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 18 additions & 4 deletions pkg/gui/context/menu_context.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ type MenuViewModel struct {
promptLines []string
columnAlignment []utils.Alignment
allowFilteringKeybindings bool
keybindingsTakePrecedence bool
*FilteredListViewModel[*types.MenuItem]
}

Expand Down Expand Up @@ -117,6 +118,10 @@ func (self *MenuViewModel) SetAllowFilteringKeybindings(allow bool) {
self.allowFilteringKeybindings = allow
}

func (self *MenuViewModel) SetKeybindingsTakePrecedence(value bool) {
self.keybindingsTakePrecedence = value
}

// TODO: move into presentation package
func (self *MenuViewModel) GetDisplayStrings(_ int, _ int) [][]string {
menuItems := self.FilteredListViewModel.GetItems()
Expand Down Expand Up @@ -205,10 +210,19 @@ func (self *MenuContext) GetKeybindings(opts types.KeybindingsOpts) []*types.Bin
}
})

// appending because that means the menu item bindings have lower precedence.
// So if a basic binding is to escape from the menu, we want that to still be
// what happens when you press escape. This matters when we're showing the menu
// for all keybindings of say the files context.
if self.keybindingsTakePrecedence {
// This is used for all normal menus except the keybindings menu. In this case we want the
// bindings of the menu items to have higher precedence than the builtin bindings; this
// allows assigning a keybinding to a menu item that overrides a non-essential binding such
// as 'j', 'k', 'H', 'L', etc. This is safe to do because the essential bindings such as
// confirm and return have already been removed from the menu items in this case.
return append(menuItemBindings, basicBindings...)
}

// For the keybindings menu we didn't remove the essential bindings from the menu items, because
// it is important to see all bindings (as a cheat sheet for what the keys are when the menu is
// not open). Therefore we want the essential bindings to have higher precedence than the menu
// item bindings.
return append(basicBindings, menuItemBindings...)
}

Expand Down
12 changes: 6 additions & 6 deletions pkg/gui/controllers/options_menu_action.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,12 +46,12 @@ func (self *OptionsMenuAction) Call() error {
appendBindings(navigation, &types.MenuSection{Title: self.c.Tr.KeybindingsMenuSectionNavigation, Column: 1})

return self.c.Menu(types.CreateMenuOptions{
Title: self.c.Tr.Keybindings,
Items: menuItems,
HideCancel: true,
ColumnAlignment: []utils.Alignment{utils.AlignRight, utils.AlignLeft},
AllowFilteringKeybindings: true,
KeepConfirmKeybindings: true,
Title: self.c.Tr.Keybindings,
Items: menuItems,
HideCancel: true,
ColumnAlignment: []utils.Alignment{utils.AlignRight, utils.AlignLeft},
AllowFilteringKeybindings: true,
KeepConflictingKeybindings: true,
})
}

Expand Down
14 changes: 11 additions & 3 deletions pkg/gui/menu_panel.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"github.com/jesseduffield/lazygit/pkg/gui/keybindings"
"github.com/jesseduffield/lazygit/pkg/gui/types"
"github.com/jesseduffield/lazygit/pkg/theme"
"github.com/samber/lo"
)

// note: items option is mutated by this function
Expand All @@ -21,7 +22,13 @@ func (gui *Gui) createMenu(opts types.CreateMenuOptions) error {
}

maxColumnSize := 1
confirmKey := keybindings.GetKey(gui.c.UserConfig().Keybinding.Universal.ConfirmMenu)

essentialKeys := []types.Key{
keybindings.GetKey(gui.c.UserConfig().Keybinding.Universal.ConfirmMenu),
keybindings.GetKey(gui.c.UserConfig().Keybinding.Universal.Return),
keybindings.GetKey(gui.c.UserConfig().Keybinding.Universal.PrevItem),
keybindings.GetKey(gui.c.UserConfig().Keybinding.Universal.NextItem),
}

for _, item := range opts.Items {
if item.LabelColumns == nil {
Expand All @@ -34,8 +41,8 @@ func (gui *Gui) createMenu(opts types.CreateMenuOptions) error {

maxColumnSize = max(maxColumnSize, len(item.LabelColumns))

// Remove all item keybindings that are the same as the confirm binding
if item.Key == confirmKey && !opts.KeepConfirmKeybindings {
// Remove all item keybindings that are the same as one of the essential bindings
if !opts.KeepConflictingKeybindings && lo.Contains(essentialKeys, item.Key) {
item.Key = nil
}
}
Expand All @@ -51,6 +58,7 @@ func (gui *Gui) createMenu(opts types.CreateMenuOptions) error {
gui.State.Contexts.Menu.SetMenuItems(opts.Items, opts.ColumnAlignment)
gui.State.Contexts.Menu.SetPrompt(opts.Prompt)
gui.State.Contexts.Menu.SetAllowFilteringKeybindings(opts.AllowFilteringKeybindings)
gui.State.Contexts.Menu.SetKeybindingsTakePrecedence(!opts.KeepConflictingKeybindings)
gui.State.Contexts.Menu.SetSelection(0)

gui.Views.Menu.Title = opts.Title
Expand Down
14 changes: 7 additions & 7 deletions pkg/gui/types/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -150,13 +150,13 @@ const (
)

type CreateMenuOptions struct {
Title string
Prompt string // a message that will be displayed above the menu options
Items []*MenuItem
HideCancel bool
ColumnAlignment []utils.Alignment
AllowFilteringKeybindings bool
KeepConfirmKeybindings bool // if true, the keybindings that match the confirm binding will not be removed from menu items
Title string
Prompt string // a message that will be displayed above the menu options
Items []*MenuItem
HideCancel bool
ColumnAlignment []utils.Alignment
AllowFilteringKeybindings bool
KeepConflictingKeybindings bool // if true, the keybindings that match essential bindings such as confirm or return will not be removed from menu items
}

type CreatePopupPanelOpts struct {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
package custom_commands

import (
"github.com/jesseduffield/lazygit/pkg/config"
. "github.com/jesseduffield/lazygit/pkg/integration/components"
)

var CustomCommandsSubmenuWithSpecialKeybindings = NewIntegrationTest(NewIntegrationTestArgs{
Description: "Using custom commands from a custom commands menu with keybindings that conflict with builtin ones",
ExtraCmdArgs: []string{},
Skip: false,
SetupRepo: func(shell *Shell) {},
SetupConfig: func(cfg *config.AppConfig) {
cfg.GetUserConfig().CustomCommands = []config.CustomCommand{
{
Key: "x",
Description: "My Custom Commands",
CommandMenu: []config.CustomCommand{
{
Key: "j",
Context: "global",
Command: "echo j",
Output: "popup",
},
{
Key: "H",
Context: "global",
Command: "echo H",
Output: "popup",
},
{
Key: "y",
Context: "global",
Command: "echo y",
Output: "popup",
},
{
Key: "<down>",
Context: "global",
Command: "echo down",
Output: "popup",
},
},
},
}
cfg.GetUserConfig().Keybinding.Universal.ConfirmMenu = "y"
},
Run: func(t *TestDriver, keys config.KeybindingConfig) {
t.Views().Files().
Focus().
IsEmpty().
Press("x").
Tap(func() {
t.ExpectPopup().Menu().
Title(Equals("My Custom Commands")).
Lines(
Contains("j echo j"),
Contains("H echo H"),
Contains(" echo y"),
Contains(" echo down"),
)
t.GlobalPress("j")
t.ExpectPopup().Alert().Title(Equals("echo j")).Content(Equals("j")).Confirm()
}).
Press("x").
Tap(func() {
t.ExpectPopup().Menu().
Title(Equals("My Custom Commands"))
t.GlobalPress("H")
t.ExpectPopup().Alert().Title(Equals("echo H")).Content(Equals("H")).Confirm()
})
Comment on lines +49 to +71
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding a negative test. For example by checking that pressing y and/or <down> does not trigger their respective options.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did think about that, but decided it's not necessary to complicate the test with that. Knowing how our menus work, it's enough to verify that the keybindings don't show up in the UI, we can then be sure they also don't trigger.

},
})
1 change: 1 addition & 0 deletions pkg/integration/tests/test_list.go
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,7 @@ var tests = []*components.IntegrationTest{
custom_commands.BasicCommand,
custom_commands.CheckForConflicts,
custom_commands.CustomCommandsSubmenu,
custom_commands.CustomCommandsSubmenuWithSpecialKeybindings,
custom_commands.FormPrompts,
custom_commands.GlobalContext,
custom_commands.MenuFromCommand,
Expand Down