-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Allow using 'j', 'k', 'H', or 'L' as keybindings in custom command menus #5131
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
|
| return handle(self.c.Git().WorkingTree.StageFile, self.c.Tr.Actions.ResolveConflictByKeepingFile) | ||
| }, | ||
| Key: 'p', | ||
| Key: 'k', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is changing keys for actions not considered a breaking change? Either way maybe we could consider changing it in another PR to keep this one to the point?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mentioned this in the commit message; I don't think the muscle memory issue is a concern here because it's not a frequently encountered dialog.
Moving it to a separate PR sounds good though, see #5132.
| 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() | ||
| }) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
The test shows two problems: a <down> keybinding is not removed from a menu item (the 'y' binding is removed though, which is correct), and keybindings such as 'j' and 'H' don't work. We will fix both of these separately in the following commits.
This is not really super important because we are very unlikely to assign a key such as esc or up/down to a menu item. However, users might do this in a custom commands menu, and in that case it is important that the builtin keys still work; or they might remap those builtin commands to other keys, in which case they might conflict with single-letter keys in normal menus.
After the change in the previous commit this expresses better what it is about.
…onfirm/esc This makes it possible to use 'j', 'k', 'H' or 'L' as menu item keybindings.
Now that we can use 'k' as a menu item binding (this was fixed in #5131), use it for the "keep" entry in the merge menu. I don't think this will be a problem for people's muscle memory, given that this menu is not encountered every day; and it's simply the better keybinding. This reverts commit b32b552.
4a2bc96 to
344d386
Compare
Now that we can use 'k' as a menu item binding (this was fixed in #5131), use it for the "keep" entry in the merge menu. I don't think this will be a problem for people's muscle memory, given that this menu is not encountered every day; and it's simply the better keybinding. This reverts commit b32b552.
…5132) Now that we can use 'k' as a menu item binding (this was fixed in #5131), use it for the "keep" entry in the merge menu. I don't think this will be a problem for people's muscle memory, given that this menu is not encountered every day; and it's simply the better keybinding. This reverts commit b32b552.
This MR contains the following updates: | Package | Update | Change | |---|---|---| | [jesseduffield/lazygit](https://github.com/jesseduffield/lazygit) | minor | `v0.57.0` → `v0.58.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.58.0`](https://github.com/jesseduffield/lazygit/releases/tag/v0.58.0) [Compare Source](jesseduffield/lazygit@v0.57.0...v0.58.0) <!-- Release notes generated using configuration in .github/release.yml at v0.58.0 --> #### What's Changed ##### Enhancements 🔥 - Add keys for command log menu items by [@​PeterCardenas](https://github.com/PeterCardenas) in [#​5096](jesseduffield/lazygit#5096) - Add Codeberg as a supported git hosting service by [@​yaadata](https://github.com/yaadata) in [#​5130](jesseduffield/lazygit#5130) - Change keybinding of "keep" item in Merge Conflict menu back to 'k' by [@​stefanhaller](https://github.com/stefanhaller) in [#​5132](jesseduffield/lazygit#5132) - Support custom keybindings in custom command menu prompts by [@​HerrNaN](https://github.com/HerrNaN) in [#​5129](jesseduffield/lazygit#5129) - Show an error when checking out a file would overwrite local modifications by [@​stefanhaller](https://github.com/stefanhaller) in [#​5154](jesseduffield/lazygit#5154) ##### Fixes 🔧 - Remove confirmation for opening the merge tool by [@​stefanhaller](https://github.com/stefanhaller) in [#​5094](jesseduffield/lazygit#5094) - Allow using 'j', 'k', 'H', or 'L' as keybindings in custom command menus by [@​stefanhaller](https://github.com/stefanhaller) in [#​5131](jesseduffield/lazygit#5131) - Prevent many hyperlinks from launching while mouse moving by [@​stefanhaller](https://github.com/stefanhaller) in [#​5133](jesseduffield/lazygit#5133) - Fix the main view display after reverting a commit by [@​stefanhaller](https://github.com/stefanhaller) in [#​5138](jesseduffield/lazygit#5138) - Avoid scrolling the selection into view on refresh by [@​stefanhaller](https://github.com/stefanhaller) in [#​5134](jesseduffield/lazygit#5134) - Fix rendering of certain emojis by [@​stefanhaller](https://github.com/stefanhaller) in [#​5116](jesseduffield/lazygit#5116) ##### Docs 📖 - Fix small issues with the Breaking Changes texts by [@​stefanhaller](https://github.com/stefanhaller) in [#​5114](jesseduffield/lazygit#5114) - Add a note about delta's `--navigate` option not working in lazygit by [@​stefanhaller](https://github.com/stefanhaller) in [#​5155](jesseduffield/lazygit#5155) - Update docs and schema for release by [@​stefanhaller](https://github.com/stefanhaller) in [#​5168](jesseduffield/lazygit#5168) ##### I18n 🌎 - Update translations from Crowdin by [@​stefanhaller](https://github.com/stefanhaller) in [#​5167](jesseduffield/lazygit#5167) ##### Performance Improvements 📊 - Fix annoying UI stalls after refresh (e.g. background fetch) when the reflog is very long by [@​stefanhaller](https://github.com/stefanhaller) in [#​5135](jesseduffield/lazygit#5135) #### New Contributors - [@​yaadata](https://github.com/yaadata) made their first contribution in [#​5130](jesseduffield/lazygit#5130) - [@​HerrNaN](https://github.com/HerrNaN) made their first contribution in [#​5129](jesseduffield/lazygit#5129) **Full Changelog**: <jesseduffield/lazygit@v0.57.0...v0.58.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:eyJjcmVhdGVkSW5WZXIiOiI0Mi42OS4yIiwidXBkYXRlZEluVmVyIjoiNDIuNjkuMiIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOlsiUmVub3ZhdGUgQm90IiwiYXV0b21hdGlvbjpib3QtYXV0aG9yZWQiLCJkZXBlbmRlbmN5LXR5cGU6Om1pbm9yIl19-->
Previously they would be shown as keybindings in the menu, but they didn't work because their builtin functionality (select next/prev line, scroll view left/right) would take precedence.
This will allow us to revert #4934; doing that in a separate PR, see #5132.