From d48db4684ddb63b685beae3db314226936dda9e8 Mon Sep 17 00:00:00 2001 From: Noah Clarkson Date: Mon, 3 Aug 2026 21:21:59 +1200 Subject: [PATCH] fix(keymap): render the primary modifier as Cmd on macOS MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The word style spelled a platform-modifier keystroke "Super+" on macOS — the Linux name for that key — so any surface asking for words rather than glyphs advertised "Super+Shift+R" instead of "Cmd+Shift+R". That is the same class of drift issue #61 was filed about. The tests missed it because they asserted "Ctrl+..." unconditionally, which is only what `secondary-` resolves to off macOS. They now derive the expected modifier from the platform, so the macOS spelling is actually pinned. Bindings written as an explicit `ctrl-` keep asserting Control, since those stay Control everywhere. Caught by CI on macos-14 after the keymap work merged; Windows and Linux were green throughout, which is why this survived local verification. Co-Authored-By: Claude Opus 5 --- .../rgitui_workspace/src/command_palette.rs | 8 +++- crates/rgitui_workspace/src/keymap/display.rs | 39 ++++++++++++------- crates/rgitui_workspace/src/keymap/summary.rs | 25 +++++++++--- 3 files changed, 52 insertions(+), 20 deletions(-) diff --git a/crates/rgitui_workspace/src/command_palette.rs b/crates/rgitui_workspace/src/command_palette.rs index de49f39..f8aed34 100644 --- a/crates/rgitui_workspace/src/command_palette.rs +++ b/crates/rgitui_workspace/src/command_palette.rs @@ -1379,9 +1379,15 @@ mod tests { command.id ); } + // Fetch is bound `secondary-shift-r`, which is Command on macOS. + let primary = if cfg!(target_os = "macos") { + "Cmd+" + } else { + "Ctrl+" + }; assert_eq!( summary.display(CommandId::Fetch).as_deref(), - Some("Ctrl+Shift+R") + Some(format!("{primary}Shift+R").as_str()) ); } diff --git a/crates/rgitui_workspace/src/keymap/display.rs b/crates/rgitui_workspace/src/keymap/display.rs index 6920f3b..11b31c4 100644 --- a/crates/rgitui_workspace/src/keymap/display.rs +++ b/crates/rgitui_workspace/src/keymap/display.rs @@ -101,7 +101,9 @@ fn render(modifiers: &Modifiers, key: &str, style: KeystrokeStyle) -> String { out.push_str("Alt+"); } if modifiers.platform { - out.push_str(if cfg!(target_os = "windows") { + out.push_str(if cfg!(target_os = "macos") { + "Cmd+" + } else if cfg!(target_os = "windows") { "Win+" } else { "Super+" @@ -282,6 +284,8 @@ fn named_key(key: &str, style: KeystrokeStyle) -> Option<&'static str> { "platform" => { if symbols { "⌘" + } else if cfg!(target_os = "macos") { + "Cmd" } else if cfg!(target_os = "windows") { "Win" } else { @@ -312,11 +316,20 @@ fn title_case(value: &str) -> String { mod tests { use super::*; + /// Word-style spelling of what `secondary-` resolves to here: Command on + /// macOS, Control everywhere else. Tests that hard-coded `Ctrl+` passed on + /// Windows and Linux while asserting the wrong thing on macOS. + const PRIMARY_WORD: &str = if cfg!(target_os = "macos") { + "Cmd+" + } else { + "Ctrl+" + }; + #[test] fn the_primary_modifier_follows_the_platform() { assert_eq!( humanize_keystroke("secondary-shift-r", KeystrokeStyle::Words).as_deref(), - Some("Ctrl+Shift+R") + Some(format!("{PRIMARY_WORD}Shift+R").as_str()) ); assert_eq!( humanize_keystroke("secondary-shift-r", KeystrokeStyle::Symbols).as_deref(), @@ -340,20 +353,20 @@ mod tests { #[test] fn named_keys_get_conventional_names() { for (source, words) in [ - ("secondary-enter", "Ctrl+Enter"), - ("escape", "Esc"), - ("f5", "F5"), - ("shift-tab", "Shift+Tab"), - ("secondary-up", "Ctrl+Up"), - ("alt-5", "Alt+5"), - ("space", "Space"), - ("delete", "Delete"), - ("secondary-,", "Ctrl+,"), - ("secondary-[", "Ctrl+["), + ("secondary-enter", format!("{PRIMARY_WORD}Enter")), + ("escape", "Esc".to_owned()), + ("f5", "F5".to_owned()), + ("shift-tab", "Shift+Tab".to_owned()), + ("secondary-up", format!("{PRIMARY_WORD}Up")), + ("alt-5", "Alt+5".to_owned()), + ("space", "Space".to_owned()), + ("delete", "Delete".to_owned()), + ("secondary-,", format!("{PRIMARY_WORD},")), + ("secondary-[", format!("{PRIMARY_WORD}[")), ] { assert_eq!( humanize_keystroke(source, KeystrokeStyle::Words).as_deref(), - Some(words), + Some(words.as_str()), "{source}" ); } diff --git a/crates/rgitui_workspace/src/keymap/summary.rs b/crates/rgitui_workspace/src/keymap/summary.rs index 183edcc..6cc7a79 100644 --- a/crates/rgitui_workspace/src/keymap/summary.rs +++ b/crates/rgitui_workspace/src/keymap/summary.rs @@ -471,6 +471,15 @@ mod tests { BindingSpec::user_binding(keystrokes, Some(context), action) } + /// Word-style spelling of what `secondary-` resolves to here: Command on + /// macOS, Control everywhere else. Only bindings written as `secondary-` + /// follow it — an explicit `ctrl-` binding stays Control on every platform. + const PRIMARY_WORD: &str = if cfg!(target_os = "macos") { + "Cmd+" + } else { + "Ctrl+" + }; + /// The drift that motivated this work: the help used to advertise /// `Ctrl+Shift+F` for Fetch while the registry bound `Ctrl+Shift+R`. #[test] @@ -478,7 +487,7 @@ mod tests { let summary = defaults(); assert_eq!( summary.display(CommandId::Fetch).as_deref(), - Some("Ctrl+Shift+R") + Some(format!("{PRIMARY_WORD}Shift+R").as_str()) ); assert_eq!( summary.display(CommandId::Fetch), @@ -556,7 +565,7 @@ mod tests { fn a_command_with_two_keystrokes_lists_both() { assert_eq!( defaults().display(CommandId::UnstageAll).as_deref(), - Some("Ctrl+Shift+S or Ctrl+U") + Some(format!("{PRIMARY_WORD}Shift+S or {PRIMARY_WORD}U").as_str()) ); } @@ -605,12 +614,16 @@ mod tests { assert!(commit .bindings .iter() - .any(|binding| binding.display == "Ctrl+S" && binding.is_user_defined())); + .any(|binding| binding.display == format!("{PRIMARY_WORD}S") + && binding.is_user_defined())); assert_eq!(summary.display(CommandId::StageAll), None); let warnings = summary.warnings(CommandId::StageAll); assert_eq!(warnings.len(), 1, "{warnings:?}"); - assert!(warnings[0].contains("Ctrl+S"), "{warnings:?}"); + assert!( + warnings[0].contains(&format!("{PRIMARY_WORD}S")), + "{warnings:?}" + ); assert!(warnings[0].contains("keymap.json"), "{warnings:?}"); } @@ -643,7 +656,7 @@ mod tests { // Fetch keeps its own default; only the extra binding was dropped. assert_eq!( summary.display(CommandId::Fetch).as_deref(), - Some("Ctrl+Shift+R") + Some(format!("{PRIMARY_WORD}Shift+R").as_str()) ); assert_eq!( summary.display(CommandId::Pull).as_deref(), @@ -665,7 +678,7 @@ mod tests { // The default comes first: defaults are applied before user bindings. assert_eq!( summary.display(CommandId::OpenRepo).as_deref(), - Some("Ctrl+O or Ctrl+Alt+K Ctrl+Alt+O") + Some(format!("{PRIMARY_WORD}O or Ctrl+Alt+K Ctrl+Alt+O").as_str()) ); }