Allow keybindings to configure more than one key#5634
Merged
Conversation
Shell.RunShellCommand was passing os.Environ() to its child, while its sibling runCommandWithOutputAndEnv has used the minimal NewTestEnvironment since late 2023 when env isolation was introduced; the sh path was just missed. This matters when integration tests run from inside a `git rebase -x` exec in a linked worktree: git sets GIT_DIR=<main>/.git/worktrees/<name> for the exec, and it leaks all the way down through bash, just, go test, and the test process, into every git invocation RunShellCommand spawns. cmd.Dir becomes irrelevant — git resolves GIT_DIR over cwd-based discovery, with the work-tree taken from the gitdir file (i.e. the worktree root). So `git checkout -b conflict` in a test fixture creates the branch on the real worktree and switches its HEAD, hijacking the in-progress rebase and trashing the working tree. (In the main worktree git doesn't set GIT_DIR for rebase exec, which is why the bug was only visible from linked worktrees.) Using self.env also incidentally restores GIT_CONFIG_GLOBAL for shell commands, so commits made via RunShellCommand are now authored by the test config's CI identity rather than whatever the host's ~/.gitconfig resolves to.
Should have been removed in 74a6ea8.
It always returned nil.
For legacy reasons, OptionMenu was set to `<disabled>`, and OptionMenuAlt1 to `?`. This doesn't make a lot of sense any more; get rid of OptionMenuAlt1 and bind OptionMenu to `?` by default. This is a breaking change for users who rebound OptionMenuAlt1 in their config, but it doesn't strike me as very likely, and it's easy enough to fix.
Constructing a menu item key from a literal character requires
gocui.NewKeyRune('r'), which is a bit noisy. Add a private menuKey helper in
both the controllers and helpers packages so the common case in either reads as
menuKey('r'). Duplicating the one-liner is cheaper than a cross-package import
dependency and avoids forcing every controller file to qualify the call.
The reason for doing this now is that we are going to change MenuItem.Key to a
slice of keys later in the branch, which means we'd have to add `[]gocui.Key{`
at each call site, making them even more noisy. With the menuKey helper we can
just change its signature and leave all clients unchanged.
This is a pure refactor in preparation for letting users configure multiple alternate bindings for a single command. Every Binding still has exactly one key, so nothing changes visibly: the cheatsheet, the on-screen options bar, and the keybindings menu all render identically. When a Binding ends up with multiple keys, the on-screen options bar will show only the first (to avoid clutter); the cheatsheet will show all of them (in a later commit). For now both paths take Key[0]. MenuItem.Key is changed in the same way, it also has a slice of keys now. In this commit we keep the name `Key` in Binding, KeybindingOpts and MenuItem, instead of renaming them to `Keys` right away, in order to keep the diff a bit more readable. We'll do the rename separately in the next commit.
This is a straight rename with no other code changes. Doing it in a separate commit to keep the diff of the previous one somewhat readable.
The cheatsheet has been showing only the first key of each binding since Binding.Key became Binding.Keys; collapse the list back into a single comma-separated cell so users can see all the alternates at a glance once bindings start carrying more than one key.
Each user-configurable keybinding is currently a single string in the YAML config. To let users assign alternate keys to a command, introduce a Keybinding type that decodes from either a scalar (the existing single-key form, kept for backward compatibility and for a simpler config file) or a sequence of strings. Marshalling collapses single-element slices back to a scalar so configs and generated docs round-trip cleanly. JSONSchema describes the type as a oneOf union so editors validate either form; subsequent commits will inline the union into the generated schema and start using Keybinding as the field type.
Until now every keybinding config field was a plain string. That meant a user
couldn't ask for two keys to invoke a command — the config silently accepted
only one form.
Convert every string-typed field across all 13 KeybindingXxxConfig structs to
Keybinding so the union type extends to every command. Defaults wrap their
single-key value in Keybinding{...} so the generated Config.md still renders one
scalar key per binding.
The alt fields keep their separate Binding registrations for now: this commit
does not yet introduce the merge mechanism that folds them into the main field —
that comes in a follow-up. Consumers previously calling opts.GetKeys on a string
field now call opts.GetKeys on the Keybinding, or take .String() / Keys[0] where
a single value is needed.
Adds a Keybinding.String helper for rendering, schema-generator work that
inlines the Keybinding union into each consuming property, and a unit test
covering the user-facing scalar/sequence YAML forms for quit.
JumpToBlock is special: each of its 5 elements is the binding for one side window (status / files / branches / commits / stash), not an alternate for a single command. Change the field from []string to []Keybinding so each window slot can have alternates of its own. The schema becomes "an array of 5 keybindings, each itself a string or array of strings", which falls out cleanly from how the Keybinding type inlines into the generated schema. Existing configs (a flat array of 5 strings) keep validating because each element is unmarshalled through Keybinding's scalar-or-sequence decoder.
CustomCommand.Key and CustomCommandMenuOption.Key are user-configured keybindings just like the built-in ones. Converting them to the Keybinding type lets a user assign multiple keys to the same custom command, e.g. `key: [a, b]`, the same way they would for any other keybinding. The validator iterates over the elements rather than checking a single string, the binding registration goes through GetValidatedKeyBindingKeys to register every alternate, and the existing error messages use .String() so a multi-key binding renders sensibly. CustomCommandPrompt.Key (a form field name, not a keybinding) stays a plain string.
Now that quit accepts multiple keys, the historical quit-alt1 field is redundant: existing configs that set it should keep working without the user having to migrate, but the lazygit code shouldn't have to register the alt binding separately. Add a merge step that runs after the user config is loaded (and from NewDummyAppConfig, which the cheatsheet generator and integration tests go through) folding the alt value into the main key list. Mark QuitAlt1 deprecated so it disappears from the generated Config.md example, while staying in the JSON schema with a description so editors can still steer users toward the new form. Note that instead of marking the alt config as deprecated, we could have added a migrator that changes users' config files and gets rid of the alt config for good. I decided not to do that, because this would render the config file invalid for older versions of lazygit, which would then refuse to start; and that's annoying when bisecting bugs. We'll keep the deprecated configs in the code for a year or so, and then add the migrator. The next commit will fold the remaining ~15 -alt-style fields the same way; the helper is shaped to keep that mechanical.
Previously the patch_explorer and merge_conflicts controllers reused Universal.PrevBlock/NextBlock for moving between hunks (or conflicts) in the main view, sharing keys with the global side-window cycle. The two operations are conceptually distinct: cycling side windows is a global navigation gesture, while next/prev hunk acts on the diff in the main view. Tying them together also blocks adding <tab>/<backtab> as side- window-cycle keys, because <tab> already means "toggle panel" in the staging view. Add Main.PrevHunk/NextHunk to the existing KeybindingMainConfig (which already groups bindings for the main view across staging, patch building, and merge conflicts) and switch both controllers to it. The defaults match the active key set those controllers had before (<left>/<right>/h/l), so the user-visible behavior is unchanged.
Convert the remaining *Alt/*Alt[12] sibling fields (PrevItem/NextItem, GotoTop/GotoBottom, PrevBlock/NextBlock, ScrollUpMain/ScrollDownMain, OptionMenu, ConfirmInEditor, DiffingMenu) so the merge mechanism folds their values into the corresponding main multi-key binding at config load. The redundant alt-only Binding registrations across the various controllers and the global keybindings file are gone: the merged main field already carries every key, so the for-loop in SetKeybinding registers them all.
We append them with a blank line to an existing tooltip if the item already has one, or create a new tooltip if not.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Keybindings in the user config file can now either be a single string as before, or a list of strings, in which case the action can be triggered by any of them.
This has two benefits:
<esc>key: some users are used to typing it asctrl+[because this used to be possible in the legacy terminal protocol. With recent changes to lazygit's keybinding system this no longer works out of the box, since<esc>andctrl+[are now distinct keys that can be bound separately. But users who want the two to behave the same (as in the old days) can now doXyzAltbindings that we used to have, for cases where we already wanted to have alternate keybindings by default (for example<up>/<down>andj/kfor going up and down in a list). These can now be expressed a lot more elegantly, and require less code.The
XyzAltbindings are marked as deprecated but they are still supported. We could instead have added amigrator that changes users' config files and gets rid of the alt configs for good already; I decided not to do that, because this would render the config file invalid for older versions of lazygit, which would then refuse to start; and that's annoying when bisecting bugs. We'll keep the deprecated configs in the code for a year or so, and then add the migrator.