zed-industries
gpui: Improve displayed keybinds shown in MacOS application menus
Closes #28164
This PR adresses inproper keybinds being shown in MacOS application menus. The issue arises because the keybinds shown in MacOS application menus are unaware of keybind contexts (they are only ever updated on a keymap-change). Thus, using the keybind that was added last in the keymap can result in incorrect keybindings being shown quite frequently, as they might belong to a different context not generally available (applies the same for the default keymap as well as for user-keymaps).
For example, the linked issue arises because the keybind found last in the iterator is https://github.com/zed-industries/zed/blob/6d1dd109f554579bdf676c14b69deb9e16acc043/assets/keymaps/vim.json#L759, which is not even available in most contexts (and, additionally, the e of escape is rendered here as a keybind which seems to be a seperate issue).
Additionally, this would result in inconsistent behavior with some Vim-keybinds. A vim-keybind would be used only when available but otherwise the default binding would be shown (see Undo and Redo as an example below), which seems inconsistent.
This PR fixes this by instead using the first keybind found in keymaps, which is expected to be the keybind available in most contexts. Additionally, this allows rendering some more keybinds for actions which vim-keybind cannot be displayed (Find In Project for example) .This seems to be more reasonable until this related comment is resolved.
This includes a revert of #25878 as well. With this change, the change made in #25878 becomes obsolete and would also regress the behavior back to the state prior to that PR.
main |
This PR | |
|---|---|---|
| Edit-menu | ||
| View-menu |
Release Notes:
- Improved keybinds displayed for actions in MacOS application menus.
Hm, I think I agree partially but I might be missing some things.
I think an issue with the current approach is that for every keymap change, we potentionally run into the same issue you tried to fix in #25878. The most recent example would be https://github.com/zed-industries/zed/pull/29167, which happened due to https://github.com/zed-industries/zed/pull/28103. With every new addition to the keymap, we might run into the same issue, given that these changes will most likely be added to the bottom of the list or a non-default keymap which will be loaded last. Admittedly, this approach does not fully eliminate the issue but I would argue changes to the top end of the base keymap are much more rare, since these keybinds are well established and might never change. It feels definitely less cumbersome compared to potentially always having to add a workaround for changes or additions to the base keymap or any other keymap.
Furthermore, if a user decides to reassign their keybind, this would still be respected by this change. Should an action be manually unassigned, we update the shown keybinds in https://github.com/zed-industries/zed/blob/dbeef58f22167acfb795de985ba69cf9984392b2/crates/zed/src/zed.rs#L1384-L1396 so this would still work properly. Might have misunderstood you here though.
Additionally, reordering with the approach in this PR would still work. You would just need to move the problematic keybind further down the list as opposed to up, which is currently the case.
For a better approach though, it might be best that we go the usual approach of using keybind_for_action_in with the current focus handle provided. Then again, I am unsure about the performance implications of this, since updating the menu bindings on every focus change seems somewhat expensive and might require some further changes, but could work.
Alternatively, we could provide a fake generic focus handle which immitates focus for the workspace for the keybind_for_action_in call. This would stop keybinds with a too specific context being shown at all, which for example is the case in the attached issue. However, this might still lead to some rare cases of inaccurate keybinds but would probably be much better overall. Most likely somewhere inbetween the current approach and the approach I described before.
Happy to hear what you think.
Seems that the original issue is fixed? https://github.com/zed-industries/zed/issues/28164#issuecomment-2854731811
The original issue was fixed when Vim mode is off, ~~but a new issue with the displayed keybindings has appeared: https://github.com/zed-industries/zed/issues/28164#issuecomment-2854921866.~~
~~(The displayed keybindings no longer update in the Menu Bar after adding custom keybindings and then removing them.)~~
This is incorrect; I was causing invalid JSON to occur when I commented the entire keymap.json file.
The original issue still persists in v0.184.10 when Vim mode is on.
Seems that the original issue is fixed? #28164 (comment)
It is not, see https://github.com/zed-industries/zed/issues/28164#issuecomment-2856219330
I've now added a static and "generic" key context to match the actions' predicates against. This ensures that we actually only show keybindings that actually are or will at least be valid for most cases.
We could still try passing the currently active context stack for resolving the keybinds, but I am not sure whether this has a big advantage - I think it might even be confusing if we have frequently changing keybinds in the applications menus, or if we end up showing keybinds that are only valid in very few contexts.
Let me know what you think.
This seems like a reasonable approach. Passing through a static key context to filter out unnecessary bindings is a good idea. Willing to try this out in it's current state and revert if necessary. I definitely appreciate that we won't have to move bindings around with this approach for every complaint we get. Thanks for sticking with this!
