fix KeymapEditor

crates/gpui/src/app.rs

@@ -429,8 +429,8 @@ impl App {
     }
 
     /// Get the current keyboard mapper.
-    pub fn keyboard_mapper(&self) -> &dyn PlatformKeyboardMapper {
+    pub fn keyboard_mapper(&self) -> &Rc<dyn PlatformKeyboardMapper> {
-        self.keyboard_mapper.as_ref()
+        &self.keyboard_mapper
     }
 
     /// Invokes a handler when the current keyboard layout changes

crates/gpui/src/platform/keystroke.rs

@@ -219,31 +219,7 @@ impl Keystroke {
 
     /// Produces a representation of this key that Parse can understand.
     pub fn unparse(&self) -> String {
-        let mut str = String::new();
+        unparse(&self.modifiers, &self.key)
-        if self.modifiers.function {
-            str.push_str("fn-");
-        }
-        if self.modifiers.control {
-            str.push_str("ctrl-");
-        }
-        if self.modifiers.alt {
-            str.push_str("alt-");
-        }
-        if self.modifiers.platform {
-            #[cfg(target_os = "macos")]
-            str.push_str("cmd-");
-
-            #[cfg(any(target_os = "linux", target_os = "freebsd"))]
-            str.push_str("super-");
-
-            #[cfg(target_os = "windows")]
-            str.push_str("win-");
-        }
-        if self.modifiers.shift {
-            str.push_str("shift-");
-        }
-        str.push_str(&self.key);
-        str
     }
 
     /// Returns true if this keystroke left
@@ -304,6 +280,11 @@ impl KeybindingKeystroke {
             display_key: key,
         }
     }
+
+    /// Produces a representation of this key that Parse can understand.
+    pub fn unparse(&self) -> String {
+        unparse(&self.display_modifiers, &self.display_key)
+    }
 }
 
 fn is_printable_key(key: &str) -> bool {
@@ -668,3 +649,32 @@ fn display_key(key: &str, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
     };
     f.write_char(key)
 }
+
+#[inline]
+fn unparse(modifiers: &Modifiers, key: &str) -> String {
+    let mut result = String::new();
+    if modifiers.function {
+        result.push_str("fn-");
+    }
+    if modifiers.control {
+        result.push_str("ctrl-");
+    }
+    if modifiers.alt {
+        result.push_str("alt-");
+    }
+    if modifiers.platform {
+        #[cfg(target_os = "macos")]
+        result.push_str("cmd-");
+
+        #[cfg(any(target_os = "linux", target_os = "freebsd"))]
+        result.push_str("super-");
+
+        #[cfg(target_os = "windows")]
+        result.push_str("win-");
+    }
+    if modifiers.shift {
+        result.push_str("shift-");
+    }
+    result.push_str(&key);
+    result
+}

crates/settings/src/keymap_file.rs

@@ -398,7 +398,7 @@ impl KeymapFile {
             context,
             use_key_equivalents,
             action_input_string.map(SharedString::from),
-            cx.keyboard_mapper(),
+            cx.keyboard_mapper().as_ref(),
         ) {
             Ok(key_binding) => key_binding,
             Err(InvalidKeystrokeError { keystroke }) => {
@@ -600,6 +600,7 @@ impl KeymapFile {
         mut operation: KeybindUpdateOperation<'a>,
         mut keymap_contents: String,
         tab_size: usize,
+        keyboard_mapper: &dyn gpui::PlatformKeyboardMapper,
     ) -> Result<String> {
         match operation {
             // if trying to replace a keybinding that is not user-defined, treat it as an add operation
@@ -639,7 +640,7 @@ impl KeymapFile {
                 .action_value()
                 .context("Failed to generate target action JSON value")?;
             let Some((index, keystrokes_str)) =
-                find_binding(&keymap, &target, &target_action_value)
+                find_binding(&keymap, &target, &target_action_value, keyboard_mapper)
             else {
                 anyhow::bail!("Failed to find keybinding to remove");
             };
@@ -674,7 +675,7 @@ impl KeymapFile {
                 .context("Failed to generate source action JSON value")?;
 
             if let Some((index, keystrokes_str)) =
-                find_binding(&keymap, &target, &target_action_value)
+                find_binding(&keymap, &target, &target_action_value, keyboard_mapper)
             {
                 if target.context == source.context {
                     // if we are only changing the keybinding (common case)
@@ -774,7 +775,7 @@ impl KeymapFile {
             }
             let use_key_equivalents = from.and_then(|from| {
                 let action_value = from.action_value().context("Failed to serialize action value. `use_key_equivalents` on new keybinding may be incorrect.").log_err()?;
-                let (index, _) = find_binding(&keymap, &from, &action_value)?;
+                let (index, _) = find_binding(&keymap, &from, &action_value, keyboard_mapper)?;
                 Some(keymap.0[index].use_key_equivalents)
             }).unwrap_or(false);
             if use_key_equivalents {
@@ -801,6 +802,7 @@ impl KeymapFile {
             keymap: &'b KeymapFile,
             target: &KeybindUpdateTarget<'a>,
             target_action_value: &Value,
+            keyboard_mapper: &dyn gpui::PlatformKeyboardMapper,
         ) -> Option<(usize, &'b str)> {
             let target_context_parsed =
                 KeyBindingContextPredicate::parse(target.context.unwrap_or("")).ok();
@@ -816,8 +818,11 @@ impl KeymapFile {
                 for (keystrokes_str, action) in bindings {
                     let Ok(keystrokes) = keystrokes_str
                         .split_whitespace()
-                        .map(Keystroke::parse)
+                        .map(|source| {
-                        .collect::<Result<Vec<_>, _>>()
+                            let keystroke = Keystroke::parse(source)?;
+                            Ok(KeybindingKeystroke::new(keystroke, false, keyboard_mapper))
+                        })
+                        .collect::<Result<Vec<_>, InvalidKeystrokeError>>()
                     else {
                         continue;
                     };
@@ -825,7 +830,7 @@ impl KeymapFile {
                         || !keystrokes
                             .iter()
                             .zip(target.keystrokes)
-                            .all(|(a, b)| a.should_match(b))
+                            .all(|(a, b)| a.inner.should_match(b))
                     {
                         continue;
                     }
@@ -840,7 +845,7 @@ impl KeymapFile {
     }
 }
 
-#[derive(Clone)]
+#[derive(Clone, Debug)]
 pub enum KeybindUpdateOperation<'a> {
     Replace {
         /// Describes the keybind to create
@@ -934,7 +939,10 @@ impl<'a> KeybindUpdateTarget<'a> {
     fn keystrokes_unparsed(&self) -> String {
         let mut keystrokes = String::with_capacity(self.keystrokes.len() * 8);
         for keystroke in self.keystrokes {
-            keystrokes.push_str(&keystroke.inner.unparse());
+            // The reason use `keystroke.unparse()` instead of `keystroke.inner.unparse()`
+            // here is that, we want the user to use `ctrl-shift-4` instread of `ctrl-$`
+            // by default on Windows.
+            keystrokes.push_str(&keystroke.unparse());
             keystrokes.push(' ');
         }
         keystrokes.pop();
@@ -952,7 +960,7 @@ impl<'a> KeybindUpdateTarget<'a> {
     }
 }
 
-#[derive(Clone, Copy, Default, PartialEq, Eq, PartialOrd, Ord)]
+#[derive(Clone, Copy, Default, PartialEq, Eq, PartialOrd, Ord, Debug)]
 pub enum KeybindSource {
     User,
     Vim,
@@ -1042,8 +1050,13 @@ mod tests {
         operation: KeybindUpdateOperation,
         expected: impl ToString,
     ) {
-        let result = KeymapFile::update_keybinding(operation, input.to_string(), 4)
+        let result = KeymapFile::update_keybinding(
-            .expect("Update succeeded");
+            operation,
+            input.to_string(),
+            4,
+            &gpui::DummyKeyboardMapper,
+        )
+        .expect("Update succeeded");
         pretty_assertions::assert_eq!(expected.to_string(), result);
     }
 

crates/settings_ui/src/keybindings.rs

@@ -14,9 +14,9 @@ use gpui::{
     Action, AppContext as _, AsyncApp, Axis, ClickEvent, Context, DismissEvent, Entity,
     EventEmitter, FocusHandle, Focusable, Global, IsZero,
     KeyBindingContextPredicate::{And, Descendant, Equal, Identifier, Not, NotEqual, Or},
-    KeyContext, KeybindingKeystroke, Keystroke, MouseButton, Point, ScrollStrategy,
+    KeyContext, KeybindingKeystroke, Keystroke, MouseButton, PlatformKeyboardMapper, Point,
-    ScrollWheelEvent, Stateful, StyledText, Subscription, Task, TextStyleRefinement, WeakEntity,
+    ScrollStrategy, ScrollWheelEvent, Stateful, StyledText, Subscription, Task,
-    actions, anchored, deferred, div,
+    TextStyleRefinement, WeakEntity, actions, anchored, deferred, div,
 };
 use language::{Language, LanguageConfig, ToOffset as _};
 use notifications::status_toast::{StatusToast, ToastIcon};
@@ -1206,8 +1206,11 @@ impl KeymapEditor {
                 .read(cx)
                 .get_scrollbar_offset(Axis::Vertical),
         ));
-        cx.spawn(async move |_, _| remove_keybinding(to_remove, &fs, tab_size).await)
+        let keyboard_mapper = cx.keyboard_mapper().clone();
-            .detach_and_notify_err(window, cx);
+        cx.spawn(async move |_, _| {
+            remove_keybinding(to_remove, &fs, tab_size, keyboard_mapper.as_ref()).await
+        })
+        .detach_and_notify_err(window, cx);
     }
 
     fn copy_context_to_clipboard(
@@ -2320,6 +2323,7 @@ impl KeybindingEditorModal {
         }).unwrap_or(Ok(()))?;
 
         let create = self.creating;
+        let keyboard_mapper = cx.keyboard_mapper().clone();
 
         cx.spawn(async move |this, cx| {
             let action_name = existing_keybind.action().name;
@@ -2332,6 +2336,7 @@ impl KeybindingEditorModal {
                 new_action_args.as_deref(),
                 &fs,
                 tab_size,
+                keyboard_mapper.as_ref(),
             )
             .await
             {
@@ -3006,6 +3011,7 @@ async fn save_keybinding_update(
     new_args: Option<&str>,
     fs: &Arc<dyn Fs>,
     tab_size: usize,
+    keyboard_mapper: &dyn PlatformKeyboardMapper,
 ) -> anyhow::Result<()> {
     let keymap_contents = settings::KeymapFile::load_keymap_file(fs)
         .await
@@ -3048,9 +3054,13 @@ async fn save_keybinding_update(
 
     let (new_keybinding, removed_keybinding, source) = operation.generate_telemetry();
 
-    let updated_keymap_contents =
+    let updated_keymap_contents = settings::KeymapFile::update_keybinding(
-        settings::KeymapFile::update_keybinding(operation, keymap_contents, tab_size)
+        operation,
-            .map_err(|err| anyhow::anyhow!("Could not save updated keybinding: {}", err))?;
+        keymap_contents,
+        tab_size,
+        keyboard_mapper,
+    )
+    .map_err(|err| anyhow::anyhow!("Could not save updated keybinding: {}", err))?;
     fs.write(
         paths::keymap_file().as_path(),
         updated_keymap_contents.as_bytes(),
@@ -3071,6 +3081,7 @@ async fn remove_keybinding(
     existing: ProcessedBinding,
     fs: &Arc<dyn Fs>,
     tab_size: usize,
+    keyboard_mapper: &dyn PlatformKeyboardMapper,
 ) -> anyhow::Result<()> {
     let Some(keystrokes) = existing.keystrokes() else {
         anyhow::bail!("Cannot remove a keybinding that does not exist");
@@ -3094,9 +3105,13 @@ async fn remove_keybinding(
     };
 
     let (new_keybinding, removed_keybinding, source) = operation.generate_telemetry();
-    let updated_keymap_contents =
+    let updated_keymap_contents = settings::KeymapFile::update_keybinding(
-        settings::KeymapFile::update_keybinding(operation, keymap_contents, tab_size)
+        operation,
-            .context("Failed to update keybinding")?;
+        keymap_contents,
+        tab_size,
+        keyboard_mapper,
+    )
+    .context("Failed to update keybinding")?;
     fs.write(
         paths::keymap_file().as_path(),
         updated_keymap_contents.as_bytes(),

crates/settings_ui/src/ui_components/keystroke_input.rs

@@ -304,7 +304,7 @@ impl KeystrokeInput {
         }
 
         let mut keystroke =
-            KeybindingKeystroke::new(keystroke.clone(), false, cx.keyboard_mapper());
+            KeybindingKeystroke::new(keystroke.clone(), false, cx.keyboard_mapper().as_ref());
         if let Some(last) = self.keystrokes.last()
             && last.display_key.is_empty()
             && (!self.search || self.previous_modifiers.modified())