From 204442663438e894fe6d490cd2a81f989c2e6ba5 Mon Sep 17 00:00:00 2001 From: Finn Evers Date: Thu, 22 May 2025 15:51:51 +0200 Subject: [PATCH] gpui: Improve displayed keybinds shown in macOS application menus (#28440) 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](https://github.com/zed-industries/zed/blob/6d1dd109f554579bdf676c14b69deb9e16acc043/crates/zed/src/zed.rs#L1421)). 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](https://github.com/zed-industries/zed/blob/6d1dd109f554579bdf676c14b69deb9e16acc043/crates/gpui/src/keymap.rs#L199-L204) 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 | main_edit | PR_edit | | View-menu | main_view | PR_view | Release Notes: - Improved keybinds displayed for actions in MacOS application menus. --- assets/keymaps/default-macos.json | 20 +++++++-------- assets/keymaps/vim.json | 8 ------ crates/gpui/src/keymap.rs | 13 ++++++---- crates/gpui/src/platform/mac/events.rs | 1 + crates/gpui/src/platform/mac/platform.rs | 32 ++++++++++++++++++------ 5 files changed, 43 insertions(+), 31 deletions(-) diff --git a/assets/keymaps/default-macos.json b/assets/keymaps/default-macos.json index b0811f7d3427493d39ea25b09d89ae2dc0b0ca1c..81aa1d100828391691b7e44fe645a667b1c13e3b 100644 --- a/assets/keymaps/default-macos.json +++ b/assets/keymaps/default-macos.json @@ -1,15 +1,4 @@ [ - // Moved before Standard macOS bindings so that `cmd-w` is not the last binding for - // `workspace::CloseWindow` and displayed/intercepted by macOS - { - "context": "PromptLibrary", - "use_key_equivalents": true, - "bindings": { - "cmd-n": "rules_library::NewRule", - "cmd-shift-s": "rules_library::ToggleDefaultRule", - "cmd-w": "workspace::CloseWindow" - } - }, // Standard macOS bindings { "use_key_equivalents": true, @@ -380,6 +369,15 @@ "shift-backspace": "agent::RemoveSelectedThread" } }, + { + "context": "PromptLibrary", + "use_key_equivalents": true, + "bindings": { + "cmd-n": "rules_library::NewRule", + "cmd-shift-s": "rules_library::ToggleDefaultRule", + "cmd-w": "workspace::CloseWindow" + } + }, { "context": "BufferSearchBar", "use_key_equivalents": true, diff --git a/assets/keymaps/vim.json b/assets/keymaps/vim.json index bba5d2d78ee8645ca89bae4511e88e00f9a94440..4786a4db2229a92782766adf1ca735ed992d917e 100644 --- a/assets/keymaps/vim.json +++ b/assets/keymaps/vim.json @@ -846,13 +846,5 @@ // and Windows. "alt-l": "editor::AcceptEditPrediction" } - }, - { - // Fixes https://github.com/zed-industries/zed/issues/29095 by ensuring that - // the last binding for editor::ToggleComments is not ctrl-c. - "context": "hack_to_fix_ctrl-c", - "bindings": { - "g c": "editor::ToggleComments" - } } ] diff --git a/crates/gpui/src/keymap.rs b/crates/gpui/src/keymap.rs index 4f2b57fc52e71b0010e5297abc41daa2feebacff..4dbad6dc45db958de1e00bf33b304366d46f0212 100644 --- a/crates/gpui/src/keymap.rs +++ b/crates/gpui/src/keymap.rs @@ -206,12 +206,15 @@ impl Keymap { bindings.pop() } - /// Like `bindings_to_display_from_bindings` but takes a `DoubleEndedIterator` and returns a - /// reference. - pub fn binding_to_display_from_bindings_iterator<'a>( - mut bindings: impl DoubleEndedIterator, + /// Returns the first binding present in the iterator, which tends to be the + /// default binding without any key context. This is useful for cases where no + /// key context is available on binding display. Otherwise, bindings with a + /// more specific key context would take precedence and result in a + /// potentially invalid keybind being returned. + pub fn default_binding_from_bindings_iterator<'a>( + mut bindings: impl Iterator, ) -> Option<&'a KeyBinding> { - bindings.next_back() + bindings.next() } } diff --git a/crates/gpui/src/platform/mac/events.rs b/crates/gpui/src/platform/mac/events.rs index 34805c5bb28b1323ae3db5119f8e8104aa567968..58f5d9bc1c29a4ca955cd3b7d0b8f953053eb0ba 100644 --- a/crates/gpui/src/platform/mac/events.rs +++ b/crates/gpui/src/platform/mac/events.rs @@ -30,6 +30,7 @@ pub fn key_to_native(key: &str) -> Cow { let code = match key { "space" => SPACE_KEY, "backspace" => BACKSPACE_KEY, + "escape" => ESCAPE_KEY, "up" => NSUpArrowFunctionKey, "down" => NSDownArrowFunctionKey, "left" => NSLeftArrowFunctionKey, diff --git a/crates/gpui/src/platform/mac/platform.rs b/crates/gpui/src/platform/mac/platform.rs index a59d9d3cdc0ae424b5cec115967ca8985dde03c9..6cfd97ad33f7e9cc3eba0ce5e3e0132375fc3ea8 100644 --- a/crates/gpui/src/platform/mac/platform.rs +++ b/crates/gpui/src/platform/mac/platform.rs @@ -6,8 +6,8 @@ use super::{ }; use crate::{ Action, AnyWindowHandle, BackgroundExecutor, ClipboardEntry, ClipboardItem, ClipboardString, - CursorStyle, ForegroundExecutor, Image, ImageFormat, Keymap, MacDispatcher, MacDisplay, - MacWindow, Menu, MenuItem, PathPromptOptions, Platform, PlatformDisplay, + CursorStyle, ForegroundExecutor, Image, ImageFormat, KeyContext, Keymap, MacDispatcher, + MacDisplay, MacWindow, Menu, MenuItem, PathPromptOptions, Platform, PlatformDisplay, PlatformKeyboardLayout, PlatformTextSystem, PlatformWindow, Result, ScreenCaptureSource, SemanticVersion, Task, WindowAppearance, WindowParams, hash, }; @@ -36,6 +36,7 @@ use core_foundation::{ }; use ctor::ctor; use futures::channel::oneshot; +use itertools::Itertools; use objc::{ class, declare::ClassDecl, @@ -46,7 +47,7 @@ use objc::{ use parking_lot::Mutex; use ptr::null_mut; use std::{ - cell::Cell, + cell::{Cell, LazyCell}, convert::TryInto, ffi::{CStr, OsStr, c_void}, os::{raw::c_char, unix::ffi::OsStrExt}, @@ -293,6 +294,19 @@ impl MacPlatform { actions: &mut Vec>, keymap: &Keymap, ) -> id { + const DEFAULT_CONTEXT: LazyCell> = LazyCell::new(|| { + let mut workspace_context = KeyContext::new_with_defaults(); + workspace_context.add("Workspace"); + let mut pane_context = KeyContext::new_with_defaults(); + pane_context.add("Pane"); + let mut editor_context = KeyContext::new_with_defaults(); + editor_context.add("Editor"); + + pane_context.extend(&editor_context); + workspace_context.extend(&pane_context); + vec![workspace_context] + }); + unsafe { match item { MenuItem::Separator => NSMenuItem::separatorItem(nil), @@ -301,10 +315,14 @@ impl MacPlatform { action, os_action, } => { - let keystrokes = crate::Keymap::binding_to_display_from_bindings_iterator( - keymap.bindings_for_action(action.as_ref()), - ) - .map(|binding| binding.keystrokes()); + let keystrokes = keymap + .bindings_for_action(action.as_ref()) + .find_or_first(|binding| { + binding + .predicate() + .is_none_or(|predicate| predicate.eval(&DEFAULT_CONTEXT)) + }) + .map(|binding| binding.keystrokes()); let selector = match os_action { Some(crate::OsAction::Cut) => selector("cut:"),