Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Validate user config keybindings #4275

Merged
merged 5 commits into from
Feb 21, 2025
+219 −70
Merged

Validate user config keybindings #4275

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
67bb7f6
Move labelByKey to config package
stefanhaller Feb 16, 2025
130801d
Add a few missing keybindings to docs/keybindings/Custom_Keybindings.md
stefanhaller Feb 16, 2025
f3791e6
Validate keybindings
stefanhaller Feb 16, 2025
a5f78d3
Validate that Universal.JumpToBlock array has 5 elements
stefanhaller Feb 16, 2025
5979b40
Validate keys of custom commands
stefanhaller Feb 16, 2025
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
4 changes: 4 additions & 0 deletions docs/keybindings/Custom_Keybindings.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,15 @@
| `<pgup>` | Pgup |
| `<pgdown>` | Pgdn |
| `<up>` | ArrowUp |
| `<s-up>` | ShiftArrowUp |
| `<down>` | ArrowDown |
| `<s-down>` | ShiftArrowDown |
| `<left>` | ArrowLeft |
| `<right>` | ArrowRight |
| `<tab>` | Tab |
| `<backtab>` | Backtab |
| `<enter>` | Enter |
| `<a-enter>` | AltEnter |
| `<esc>` | Esc |
| `<backspace>` | Backspace |
| `<c-space>` | CtrlSpace |
Expand Down
93 changes: 93 additions & 0 deletions pkg/config/keynames.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
package config

import (
"strings"
"unicode/utf8"

"github.com/jesseduffield/gocui"
"github.com/samber/lo"
)

// NOTE: if you make changes to this table, be sure to update
// docs/keybindings/Custom_Keybindings.md as well

var LabelByKey = map[gocui.Key]string{
gocui.KeyF1: "<f1>",
gocui.KeyF2: "<f2>",
gocui.KeyF3: "<f3>",
gocui.KeyF4: "<f4>",
gocui.KeyF5: "<f5>",
gocui.KeyF6: "<f6>",
gocui.KeyF7: "<f7>",
gocui.KeyF8: "<f8>",
gocui.KeyF9: "<f9>",
gocui.KeyF10: "<f10>",
gocui.KeyF11: "<f11>",
gocui.KeyF12: "<f12>",
gocui.KeyInsert: "<insert>",
gocui.KeyDelete: "<delete>",
gocui.KeyHome: "<home>",
gocui.KeyEnd: "<end>",
gocui.KeyPgup: "<pgup>",
gocui.KeyPgdn: "<pgdown>",
gocui.KeyArrowUp: "<up>",
gocui.KeyShiftArrowUp: "<s-up>",
gocui.KeyArrowDown: "<down>",
gocui.KeyShiftArrowDown: "<s-down>",
gocui.KeyArrowLeft: "<left>",
gocui.KeyArrowRight: "<right>",
gocui.KeyTab: "<tab>", // <c-i>
gocui.KeyBacktab: "<backtab>",
gocui.KeyEnter: "<enter>", // <c-m>
gocui.KeyAltEnter: "<a-enter>",
gocui.KeyEsc: "<esc>", // <c-[>, <c-3>
gocui.KeyBackspace: "<backspace>", // <c-h>
gocui.KeyCtrlSpace: "<c-space>", // <c-~>, <c-2>
gocui.KeyCtrlSlash: "<c-/>", // <c-_>
gocui.KeySpace: "<space>",
gocui.KeyCtrlA: "<c-a>",
gocui.KeyCtrlB: "<c-b>",
gocui.KeyCtrlC: "<c-c>",
gocui.KeyCtrlD: "<c-d>",
gocui.KeyCtrlE: "<c-e>",
gocui.KeyCtrlF: "<c-f>",
gocui.KeyCtrlG: "<c-g>",
gocui.KeyCtrlJ: "<c-j>",
gocui.KeyCtrlK: "<c-k>",
gocui.KeyCtrlL: "<c-l>",
gocui.KeyCtrlN: "<c-n>",
gocui.KeyCtrlO: "<c-o>",
gocui.KeyCtrlP: "<c-p>",
gocui.KeyCtrlQ: "<c-q>",
gocui.KeyCtrlR: "<c-r>",
gocui.KeyCtrlS: "<c-s>",
gocui.KeyCtrlT: "<c-t>",
gocui.KeyCtrlU: "<c-u>",
gocui.KeyCtrlV: "<c-v>",
gocui.KeyCtrlW: "<c-w>",
gocui.KeyCtrlX: "<c-x>",
gocui.KeyCtrlY: "<c-y>",
gocui.KeyCtrlZ: "<c-z>",
gocui.KeyCtrl4: "<c-4>", // <c-\>
gocui.KeyCtrl5: "<c-5>", // <c-]>
gocui.KeyCtrl6: "<c-6>",
gocui.KeyCtrl8: "<c-8>",
Comment on lines +71 to +74
Copy link
Contributor

Choose a reason for hiding this comment

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

Unrelated to the body of this work, but why are <c-1> and others not considered valid keys? I see them in the comments up above, but don't know what that means.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This comes from commit 2e66d87. The goal of that commit was to ensure that the keys you see in the global ? menu are exactly what you put in your config file. And the approach it took to ensure this was to have two inverse maps for labelByKey and keyByLabel. The problem with this approach is that there are several labels that map to the same key (e.g. backspace and ctrl-h both map to ascii 8), so you can't put them both in the labelByKey map. We also have #2953 about this.

It would be nice to fix this, and I suppose the way to do that is to manually maintain two separate maps for labelToKey and keyToLabel, where labelToKey can have more entries. And then we need to add a field to the types.Binding struct that stores the original string so that we can show it in the menu. This would still not allow to bind one action to backspace and another to ctrl-h, but it would at least allow binding things to ctrl-h at all (or ctrl-3). This should really work since those are the actual keys that a user presses to invoke the action. (Although I'm not sure about ctrl-3; that's the same as Esc, and this could be confusing.)

gocui.MouseWheelUp: "mouse wheel up",
gocui.MouseWheelDown: "mouse wheel down",
}

var KeyByLabel = lo.Invert(LabelByKey)

func isValidKeybindingKey(key string) bool {
runeCount := utf8.RuneCountInString(key)
if key == "<disabled>" {
return true
}

if runeCount > 1 {
_, ok := KeyByLabel[strings.ToLower(key)]
return ok
}

return true
}
74 changes: 74 additions & 0 deletions pkg/config/user_config_validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,12 @@ package config

import (
"fmt"
"log"
"reflect"
"slices"
"strings"

"github.com/jesseduffield/lazygit/pkg/constants"
)

func (config *UserConfig) Validate() error {
Expand All @@ -15,6 +19,12 @@ func (config *UserConfig) Validate() error {
[]string{"none", "onlyArrow", "arrowAndNumber"}); err != nil {
return err
}
if err := validateKeybindings(config.Keybinding); err != nil {
return err
}
if err := validateCustomCommands(config.CustomCommands); err != nil {
return err
}
return nil
}

Expand All @@ -25,3 +35,67 @@ func validateEnum(name string, value string, allowedValues []string) error {
allowedValuesStr := strings.Join(allowedValues, ", ")
return fmt.Errorf("Unexpected value '%s' for '%s'. Allowed values: %s", value, name, allowedValuesStr)
}

func validateKeybindingsRecurse(path string, node any) error {
value := reflect.ValueOf(node)
if value.Kind() == reflect.Struct {
for _, field := range reflect.VisibleFields(reflect.TypeOf(node)) {
var newPath string
if len(path) == 0 {
newPath = field.Name
} else {
newPath = fmt.Sprintf("%s.%s", path, field.Name)
}
if err := validateKeybindingsRecurse(newPath,
value.FieldByName(field.Name).Interface()); err != nil {
return err
}
}
} else if value.Kind() == reflect.Slice {
for i := 0; i < value.Len(); i++ {
if err := validateKeybindingsRecurse(
fmt.Sprintf("%s[%d]", path, i), value.Index(i).Interface()); err != nil {
return err
}
}
} else if value.Kind() == reflect.String {
key := node.(string)
if !isValidKeybindingKey(key) {
return fmt.Errorf("Unrecognized key '%s' for keybinding '%s'. For permitted values see %s",
key, path, constants.Links.Docs.CustomKeybindings)
}
} else {
log.Fatalf("Unexpected type for property '%s': %s", path, value.Kind())
}
return nil
}

func validateKeybindings(keybindingConfig KeybindingConfig) error {
if err := validateKeybindingsRecurse("", keybindingConfig); err != nil {
return err
}

if len(keybindingConfig.Universal.JumpToBlock) != 5 {
return fmt.Errorf("keybinding.universal.jumpToBlock must have 5 elements; found %d.",
len(keybindingConfig.Universal.JumpToBlock))
}

return nil
}

func validateCustomCommandKey(key string) error {
if !isValidKeybindingKey(key) {
return fmt.Errorf("Unrecognized key '%s' for custom command. For permitted values see %s",
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't believe that we have an equivalent to the path variable that we can use to point them to where in their config they messed up, but maybe using the CustomCommand.Command, or the index in the slice could be helpful to somebody.

Particularly because we do string.ToLower what they send to use, so if someone creates a custom command with key <CC-4>, and then we give them an error message about <cc-4>, and they do a case sensitive search for where that is in their config. Might be confusing.

That example is starting to seem a bit contrived though....

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 briefly thought about adding the command to the error message, but commands can sometimes be very long, so this could be unwieldy. Also, I don't follow your concern about the case sensitive search; the tolower happens inside of isValidKeybindingKey, so the error message contains the actual key in its original case. A case sensitive search will find it in the config file, and I think this is good enough.

Copy link
Contributor

Choose a reason for hiding this comment

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

The highlighting of the line of code leads to some confusion on that 2nd part.

We do strings.ToLower(key) before populating the %s.

Copy link
Contributor

Choose a reason for hiding this comment

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

And if a valid concern, also applies higher up in the file on the other error message.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh right, I totally missed that. It doesn't make sense to lower-case there, removing in 9fd487b and 0126e72.

(I had copied this from here without thinking; it doesn't make sense there either, but I'm not bothering to change it there, as the fatal should never trigger any more now.)

key, constants.Links.Docs.CustomKeybindings)
}
return nil
}

func validateCustomCommands(customCommands []CustomCommand) error {
for _, customCommand := range customCommands {
if err := validateCustomCommandKey(customCommand.Key); err != nil {
return err
}
}
return nil
}
45 changes: 45 additions & 0 deletions pkg/config/user_config_validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package config

import (
"strings"
"testing"

"github.com/stretchr/testify/assert"
Expand Down Expand Up @@ -29,6 +30,50 @@ func TestUserConfigValidate_enums(t *testing.T) {
{value: "invalid_value", valid: false},
},
},
{
name: "Keybindings",
setup: func(config *UserConfig, value string) {
config.Keybinding.Universal.Quit = value
},
testCases: []testCase{
{value: "", valid: true},
{value: "<disabled>", valid: true},
{value: "q", valid: true},
{value: "<c-c>", valid: true},
{value: "invalid_value", valid: false},
},
},
{
name: "JumpToBlock keybinding",
setup: func(config *UserConfig, value string) {
config.Keybinding.Universal.JumpToBlock = strings.Split(value, ",")
},
testCases: []testCase{
{value: "", valid: false},
{value: "1,2,3", valid: false},
{value: "1,2,3,4,5", valid: true},
{value: "1,2,3,4,invalid", valid: false},
{value: "1,2,3,4,5,6", valid: false},
},
},
{
name: "Custom command keybinding",
setup: func(config *UserConfig, value string) {
config.CustomCommands = []CustomCommand{
{
Key: value,
Command: "echo 'hello'",
},
}
},
testCases: []testCase{
{value: "", valid: true},
{value: "<disabled>", valid: true},
{value: "q", valid: true},
{value: "<c-c>", valid: true},
{value: "invalid_value", valid: false},
},
},
}

for _, s := range scenarios {
Expand Down
73 changes: 3 additions & 70 deletions pkg/gui/keybindings/keybindings.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,78 +7,11 @@ import (
"unicode/utf8"

"github.com/jesseduffield/gocui"
"github.com/jesseduffield/lazygit/pkg/config"
"github.com/jesseduffield/lazygit/pkg/constants"
"github.com/jesseduffield/lazygit/pkg/gui/types"
"github.com/samber/lo"
)

var labelByKey = map[gocui.Key]string{
gocui.KeyF1: "<f1>",
gocui.KeyF2: "<f2>",
gocui.KeyF3: "<f3>",
gocui.KeyF4: "<f4>",
gocui.KeyF5: "<f5>",
gocui.KeyF6: "<f6>",
gocui.KeyF7: "<f7>",
gocui.KeyF8: "<f8>",
gocui.KeyF9: "<f9>",
gocui.KeyF10: "<f10>",
gocui.KeyF11: "<f11>",
gocui.KeyF12: "<f12>",
gocui.KeyInsert: "<insert>",
gocui.KeyDelete: "<delete>",
gocui.KeyHome: "<home>",
gocui.KeyEnd: "<end>",
gocui.KeyPgup: "<pgup>",
gocui.KeyPgdn: "<pgdown>",
gocui.KeyArrowUp: "<up>",
gocui.KeyShiftArrowUp: "<s-up>",
gocui.KeyArrowDown: "<down>",
gocui.KeyShiftArrowDown: "<s-down>",
gocui.KeyArrowLeft: "<left>",
gocui.KeyArrowRight: "<right>",
gocui.KeyTab: "<tab>", // <c-i>
gocui.KeyBacktab: "<backtab>",
gocui.KeyEnter: "<enter>", // <c-m>
gocui.KeyAltEnter: "<a-enter>",
gocui.KeyEsc: "<esc>", // <c-[>, <c-3>
gocui.KeyBackspace: "<backspace>", // <c-h>
gocui.KeyCtrlSpace: "<c-space>", // <c-~>, <c-2>
gocui.KeyCtrlSlash: "<c-/>", // <c-_>
gocui.KeySpace: "<space>",
gocui.KeyCtrlA: "<c-a>",
gocui.KeyCtrlB: "<c-b>",
gocui.KeyCtrlC: "<c-c>",
gocui.KeyCtrlD: "<c-d>",
gocui.KeyCtrlE: "<c-e>",
gocui.KeyCtrlF: "<c-f>",
gocui.KeyCtrlG: "<c-g>",
gocui.KeyCtrlJ: "<c-j>",
gocui.KeyCtrlK: "<c-k>",
gocui.KeyCtrlL: "<c-l>",
gocui.KeyCtrlN: "<c-n>",
gocui.KeyCtrlO: "<c-o>",
gocui.KeyCtrlP: "<c-p>",
gocui.KeyCtrlQ: "<c-q>",
gocui.KeyCtrlR: "<c-r>",
gocui.KeyCtrlS: "<c-s>",
gocui.KeyCtrlT: "<c-t>",
gocui.KeyCtrlU: "<c-u>",
gocui.KeyCtrlV: "<c-v>",
gocui.KeyCtrlW: "<c-w>",
gocui.KeyCtrlX: "<c-x>",
gocui.KeyCtrlY: "<c-y>",
gocui.KeyCtrlZ: "<c-z>",
gocui.KeyCtrl4: "<c-4>", // <c-\>
gocui.KeyCtrl5: "<c-5>", // <c-]>
gocui.KeyCtrl6: "<c-6>",
gocui.KeyCtrl8: "<c-8>",
gocui.MouseWheelUp: "mouse wheel up",
gocui.MouseWheelDown: "mouse wheel down",
}

var keyByLabel = lo.Invert(labelByKey)

func Label(name string) string {
return LabelFromKey(GetKey(name))
}
Expand All @@ -90,7 +23,7 @@ func LabelFromKey(key types.Key) string {
case rune:
keyInt = int(key)
case gocui.Key:
value, ok := labelByKey[key]
value, ok := config.LabelByKey[key]
if ok {
return value
}
Expand All @@ -105,7 +38,7 @@ func GetKey(key string) types.Key {
if key == "<disabled>" {
return nil
} else if runeCount > 1 {
binding, ok := keyByLabel[strings.ToLower(key)]
binding, ok := config.KeyByLabel[strings.ToLower(key)]
if !ok {
log.Fatalf("Unrecognized key %s for keybinding. For permitted values see %s", strings.ToLower(key), constants.Links.Docs.CustomKeybindings)
} else {
Expand Down