- 
          
- 
                Notifications
    You must be signed in to change notification settings 
- Fork 2.3k
Make it possible to rebind the Confirm keybinding #4860
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.
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
Conversation
| Coverage summary from CodacySee diff coverage on Codacy
 Coverage variation details
 Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:  Diff coverage details
 Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:  See your quality gate settings Change summary preferencesFootnotes
 | 
9eb21f6    to
    987d41d      
    Compare
  
    | You do not emumerate the list of dialogs/prompts it will apply to, so I'm not sure if it will also close #2009. Please advise? Thank you for your work. | 
| 
 It applies to all popups that show a message (not editable, not a menu); that's why the keybinding is called  | 
So far, confirmations and prompts were handled by the same view, context, and controller, with a bunch of conditional code based on whether the view is editable. This was more or less ok so far, since it does save a little bit of code duplication; however, now we need separate views, because we don't have dynamic keybindings, but we want to map "confirm" to different keys in confirmations (the "universal.confirm" user config) and prompts (hard-coded to enter, because it doesn't make sense to customize it there). It also allows us to get rid of the conditional code, which is a nice benefit; and the code duplication is actually not *that* bad.
Done in a separate commit because the diff is already so long.
Rebinding the universal.confirm keybinding currently doesn't make sense, because the rebound key would also be used for editable prompts, which means you would only be able to bind it to a ctrl key (not "y", which is desirable for some people), and also it would allow you to enter a line feed in a branch name. Fix this by always using enter for editable prompts.
Like with the previous commit, it doesn't make sense to use any other key than enter for the search prompt.
The universal.confirm keybinding is the wrong one to use for this, we want universal.goInto instead. They are both bound to "enter" by default, but when remapping confirm to "y" we don't want to use that for entering worktrees.
This one doesn't make a difference in practice because we don't remap the key in tests, but if we would, then this would no longer work correctly. It's just more correct this way.
It seems useful to have the flexibility to remap "enter" in confirmations to "y", but keep "enter" for menus and suggestions (even though we sometimes use menus as confirmations, but it's still good to give users the choice).
…rm key This is needed when remapping the confirmMenu key to, say, "y", and there's a menu that has an item with a "y" binding. This already worked correctly (confirm takes precedence, as desired), but it's still confusing to see the item binding.
987d41d    to
    b3a3410      
    Compare
  
    This MR contains the following updates: | Package | Update | Change | |---|---|---| | [jesseduffield/lazygit](https://github.com/jesseduffield/lazygit) | minor | `v0.54.2` -> `v0.55.0` | MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot). **Proposed changes to behavior should be submitted there as MRs.** --- ### Release Notes <details> <summary>jesseduffield/lazygit (jesseduffield/lazygit)</summary> ### [`v0.55.0`](https://github.com/jesseduffield/lazygit/releases/tag/v0.55.0) [Compare Source](jesseduffield/lazygit@v0.54.2...v0.55.0) <!-- Release notes generated using configuration in .github/release.yml at v0.55.0 --> #### What's Changed ##### Enhancements 🔥 - Allow filtering the keybindings menu by keybinding by [@​stefanhaller](https://github.com/stefanhaller) in [#​4821](jesseduffield/lazygit#4821) - Add support for suspending LazyGit with Ctrl+Z on Unix systems by [@​cowboy8625](https://github.com/cowboy8625) in [#​4757](jesseduffield/lazygit#4757) - Add "CopyToClipboard" command to `ConfirmationController` by [@​kyu08](https://github.com/kyu08) in [#​4810](jesseduffield/lazygit#4810) - Add a user config for using git's external diff command for paging by [@​stefanhaller](https://github.com/stefanhaller) in [#​4832](jesseduffield/lazygit#4832) - Log the hash of dropped stashes by [@​stefanhaller](https://github.com/stefanhaller) in [#​4850](jesseduffield/lazygit#4850) ##### Fixes 🔧 - Fix right-alignment of divergence from base branch for branch checked out in a worktree by [@​stefanhaller](https://github.com/stefanhaller) in [#​4824](jesseduffield/lazygit#4824) - Support Azure DevOps vs-ssh.visualstudio.com SSH remotes as hosting provider by [@​Kahitar](https://github.com/Kahitar) in [#​4822](jesseduffield/lazygit#4822) - Improve display of "esc" keybinding in the keybindings status bar by [@​stefanhaller](https://github.com/stefanhaller) in [#​4819](jesseduffield/lazygit#4819) - Use external diff command in stashes panel by [@​stefanhaller](https://github.com/stefanhaller) in [#​4836](jesseduffield/lazygit#4836) - Remove the git.paging.useConfig option by [@​stefanhaller](https://github.com/stefanhaller) in [#​4837](jesseduffield/lazygit#4837) - Don't auto-forward branches that are checked out in another worktree by [@​stefanhaller](https://github.com/stefanhaller) in [#​4833](jesseduffield/lazygit#4833) - Fix dropping range selection of filtered stashes by [@​stefanhaller](https://github.com/stefanhaller) in [#​4849](jesseduffield/lazygit#4849) - Fix rare crash in interactive rebase (merge command without comment) by [@​stefanhaller](https://github.com/stefanhaller) in [#​4872](jesseduffield/lazygit#4872) - Make it possible to rebind the Confirm keybinding by [@​stefanhaller](https://github.com/stefanhaller) in [#​4860](jesseduffield/lazygit#4860) ##### Maintenance ⚙️ - Pass only Git-tracked Go files to gofumpt by [@​kyu08](https://github.com/kyu08) in [#​4809](jesseduffield/lazygit#4809) - Update donation wording so that it's clear there's no strings attached by [@​jesseduffield](https://github.com/jesseduffield) in [#​4827](jesseduffield/lazygit#4827) - Enhance MR/Issue templates readability by [@​kyu08](https://github.com/kyu08) in [#​4829](jesseduffield/lazygit#4829) - Run label check workflow only on label events and open pr event by [@​kyu08](https://github.com/kyu08) in [#​4830](jesseduffield/lazygit#4830) ##### Docs 📖 - Add installation with gah by [@​marverix](https://github.com/marverix) in [#​4820](jesseduffield/lazygit#4820) - docs(VISION): fix "Dicoverability" typo by [@​Rudxain](https://github.com/Rudxain) in [#​4866](jesseduffield/lazygit#4866) - Add dev container feature as installation method to README by [@​HenningLorenzen-ext-bayer](https://github.com/HenningLorenzen-ext-bayer) in [#​4876](jesseduffield/lazygit#4876) ##### I18n 🌎 - Update translations from Crowdin by [@​stefanhaller](https://github.com/stefanhaller) in [#​4873](jesseduffield/lazygit#4873) #### New Contributors - [@​marverix](https://github.com/marverix) made their first contribution in [#​4820](jesseduffield/lazygit#4820) - [@​Kahitar](https://github.com/Kahitar) made their first contribution in [#​4822](jesseduffield/lazygit#4822) - [@​cowboy8625](https://github.com/cowboy8625) made their first contribution in [#​4757](jesseduffield/lazygit#4757) - [@​Rudxain](https://github.com/Rudxain) made their first contribution in [#​4866](jesseduffield/lazygit#4866) - [@​HenningLorenzen-ext-bayer](https://github.com/HenningLorenzen-ext-bayer) made their first contribution in [#​4876](jesseduffield/lazygit#4876) **Full Changelog**: <jesseduffield/lazygit@v0.54.2...v0.55.0> </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this MR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0MS45Ny40IiwidXBkYXRlZEluVmVyIjoiNDEuOTcuNCIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOlsiUmVub3ZhdGUgQm90Il19-->
### PR Description Remapping `keybinding.universal.confirm` from `<enter>` to something like `y` is currently impossible because the same keybinding is also used to confirm prompts (e.g. "New branch") and the search prompt. Fix this by hard-coding enter for those; it doesn't really make sense to use any other key for prompts. While at it, add separate bindings for `confirmMenu` and `confirmSuggestion` for those who would like to have different keys for these. Of these, `confirmMenu` could be a little tricky because menus are sometimes used purely as a choice (e.g. in "Amend commit attribute" or the global keybindings menu), in which case you might want to use `<enter>`, but other times as a substitute for a confirmation (e.g. for "Delete branch"), in which case you might want to remap to `y`. I don't have a great idea what to do about that, to be honest. Feedback welcome. In this PR we only take care of Confirm, which many people seem to be concerned about. We might consider doing something similar for Esc, but it seems less urgent, and I'm out of time now. 😄 This seemingly simple change required some serious refactoring under the hood, so thorough testing would be good to ensure we didn't break anything. Closes jesseduffield#2611 Closes jesseduffield#2767 Closes jesseduffield#3471 Related: jesseduffield#2768
PR Description
Remapping
keybinding.universal.confirmfrom<enter>to something likeyis currently impossible because the same keybinding is also used to confirm prompts (e.g. "New branch") and the search prompt. Fix this by hard-coding enter for those; it doesn't really make sense to use any other key for prompts.While at it, add separate bindings for
confirmMenuandconfirmSuggestionfor those who would like to have different keys for these. Of these,confirmMenucould be a little tricky because menus are sometimes used purely as a choice (e.g. in "Amend commit attribute" or the global keybindings menu), in which case you might want to use<enter>, but other times as a substitute for a confirmation (e.g. for "Delete branch"), in which case you might want to remap toy. I don't have a great idea what to do about that, to be honest. Feedback welcome.In this PR we only take care of Confirm, which many people seem to be concerned about. We might consider doing something similar for Esc, but it seems less urgent, and I'm out of time now. 😄
This seemingly simple change required some serious refactoring under the hood, so thorough testing would be good to ensure we didn't break anything.
Closes #2611
Closes #2767
Closes #3471
Related: #2768