-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Add alternative Confirm and Return hotkeys for confirmation windows #2768
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
base: master
Are you sure you want to change the base?
Conversation
|
We have had alternate mappings for confirming (and for cancelling, I think) but it was decided that why would we complicate things, who would ever need 3 ways to confirm something so it was removed. I'm not sure the decision will be reversed. |
|
It's a good point about it being more convenient to reach for a closer letter key than reaching for enter/escape when you're not in a prompt. And I like this being opt-in so that it doesn't cost us any realestate by default. That was my reason for wanting to remove the alternatives in the first place. Something worth checking is what happens when the confirmation key matches the key on a menu item? We'd want the confirmation key to take precedence. |
|
Thanks for response!
Oh, I see, so this was a supported feature until recently 🤔 Though my PR is a little bit different because it has much narrower scope, I got your point. As for 'who would ever need', well... I don't know, I still keep pushing all kinds of keys (y/n/x/q) to confirm/cancel actions in lazygit 😅 I guess that's my muscle memory :) Previous implementation was much more widely used throughout the app, so I totally understand that it may be causing conflicts in some places. What I found a little bit confusing is that though
🙏 As far as I can see, confirmations are creating new contexts, so it doesn't overlap with any other hotkeys. For example even global "exit app" hotkey doesn't work when "confirmation window" is opened. So I suppose everything should be fine 🤔 PS: Maybe these hotkeys are TOO specific, but to be honest lazygit just got a lot more intuitive for me when I changed this :) |
But what about if you're in a menu? E.g. if confirm-alt is 'y' and you go to the commits view and press '?' then 'y' does it invoke the first menu item or the yank menu item (which has 'y' as a keybinding)? |
In this PR I only added these alternative hotkeys to the y/n-type confirmation windows. So this change doesn't affect "help window" in any way. That means, that pressing "y" there (and everywhere else, excluding y/n-type confirmation windows) would indeed execute "yank" action, as expected. I also think maybe it would be better to change the names of keybindings from What do you think? |
7808230 to
c8a0fe5
Compare
|
I think we should add it to menus as well for the sake of consistency, and we can just grey-out keybindings that match on the key (see |
Hmm... I'm actually not sure now :) If we add "alternatives" also to the menus, it would be not very much different from how it was initially in lazygit (and as @mark2185 mentioned earlier, this was removed with absolutely valid reasons) I totally get that we need to be consistent, but to be honest, our current That's why I thought it may be useful to add more granular hotkeys to use in some contexts, like prompts (and to call these hotkeys accordingly, not just But these additional hotkeys indeed may look TOO specific. PS: I also think if we'll "strike out" some hotkeys out of the menu, it would make things a little bit less convenient, because in this case we cannot just press, let's say |
Is it not possible to add again the alternative return keybinding? 'cos sometimes it is more comfy to use |
|
I wanted to remap <esc> is litterally one of the most common buttons that you now need to press, and unless you remap your keyboard it's one of the furthest ones away from the homerow. Right now I'm falling back to remapping it to <c-q> EDIT: I created a PR that does this now in #3750 |
|
I am using |
### 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 #2611 Closes #2767 Closes #3471 Related: #2768
### 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
Motivation:
Currently we have
confirm(enter) andreturn(esc) key bindings which are being used everywhere extensively. Some localization are even assuming that we'll never remap these keys (and for a good reason), eg: https://github.com/jesseduffield/lazygit/blob/master/pkg/i18n/english.go#L828Indeed, for many cases (such as search fields, input prompts, etc) any other hotkeys doesn't makes much sense (especially if they're just a single English letter)
But sometimes it may be convenient to use keys, located closer than enter/esc to confirm/cancel simple actions.
In this PR I added two new configuration options (unset by default), so it will be possible to configure alternatives for these actions:
confirm-altandreturn-altPS: Previously I opened an issue, but discovered that I can fix this by myself without knowing golang :) #2767
Please check if the PR fulfills these requirements
go run scripts/cheatsheet/main.go generate)docs/Config.md) have been updated if necessary