Allow using 'j', 'k', 'H', or 'L' as keybindings in custom command menus #5131
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.
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.
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.
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.
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.
stefanhaller
force-pushed
the
fix-menu-keybindings
branch
from
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.
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.