diff --git a/src/ui/app.rs b/src/ui/app.rs index 1086b640..c2df8c76 100644 --- a/src/ui/app.rs +++ b/src/ui/app.rs @@ -6659,16 +6659,15 @@ impl Tty7App { } fn assign_keybinding(&mut self, action: String, spec: String, cx: &mut Context) { - // Compared as the keymap will build them, not as they are spelled: a - // recorded `secondary-}` and a config's `secondary-shift-]` are one - // chord, and only the folded form tells you so. Left unfolded, the - // displacement never fires and both bindings survive onto the same - // keystroke, where which one wins is arbitrary (#750). - let chord = crate::ui::keymap::dispatchable_spec(&spec); + // Compared as chords, not as spellings: a recorded `secondary-}` and a + // config's `secondary-shift-]` are one keystroke written two ways, and + // only `same_chord` sees it. Compared as text, the displacement never + // fires and both bindings survive onto that keystroke, where which one + // wins is arbitrary (#750). let displaced = crate::ui::keymap::effective_bindings(cx) .into_iter() .chain(crate::ui::keymap::extra_bindings(cx)) - .find(|(a, k)| crate::ui::keymap::dispatchable_spec(k) == chord && *a != action) + .find(|(a, k)| *a != action && crate::ui::keymap::same_chord(k, &spec)) .map(|(a, _)| a); // A trailing "…" on an action name marks a command that opens // something; it is not punctuation, and inside a sentence it reads as diff --git a/src/ui/keymap.rs b/src/ui/keymap.rs index b3f9a621..8080e973 100644 --- a/src/ui/keymap.rs +++ b/src/ui/keymap.rs @@ -163,12 +163,31 @@ pub(crate) fn extra_bindings(cx: &App) -> Vec<(String, String)> { .collect() } +/// Installs `key` for `action`, and alongside it the chord the platform will +/// actually deliver when the two are spelled differently — `secondary-shift-]` +/// is pressed as `secondary-}`, and only the second ever reaches the +/// dispatcher. Both go in rather than one replacing the other, so a spec that +/// was already live stays live whatever the backend does with Shift (#750). +/// +/// Answers whether the action was known at all, which is the caller's cue to +/// warn about a name it cannot bind. +fn push_binding(bindings: &mut Vec, action: &str, key: &str) -> bool { + let Some(binding) = make_binding(action, key) else { + return false; + }; + bindings.push(binding); + if let Some(folded) = folded_spec(key) + && let Some(alias) = make_binding(action, &folded) + { + bindings.push(alias); + } + true +} + fn action_bindings(effective: &[(String, String)]) -> Vec { let mut bindings = Vec::new(); for (action, key) in extra_keystrokes(effective) { - if let Some(b) = make_binding(action, key) { - bindings.push(b); - } + push_binding(&mut bindings, action, key); } for (action, key) in effective { if key.is_empty() { @@ -192,18 +211,14 @@ fn action_bindings(effective: &[(String, String)]) -> Vec { "keybinding '{key}' for '{action}' takes a control code away from the shell" ); } - match make_binding(action, key) { - Some(b) => { - bindings.push(b); - if is_default_insert_newline_binding(action, key) { - bindings.push(KeyBinding::new( - INSERT_NEWLINE_DEFAULT, - InsertNewlineFallback, - Some("Terminal"), - )); - } - } - None => log::warn!("ignoring keybinding: unknown action '{action}'"), + if !push_binding(&mut bindings, action, key) { + log::warn!("ignoring keybinding: unknown action '{action}'"); + } else if is_default_insert_newline_binding(action, key) { + bindings.push(KeyBinding::new( + INSERT_NEWLINE_DEFAULT, + InsertNewlineFallback, + Some("Terminal"), + )); } } bindings @@ -982,7 +997,19 @@ const SHIFTED_GLYPHS: [(char, char); 21] = [ /// for, folds onto it too — the flag is simply redundant there. /// /// The table is US-layout, and a chord it does not know is left alone rather -/// than guessed at: this can only turn a dead spec into a live one. +/// than guessed at. +/// +/// The fold is *added* to a spec's bindings, never substituted for them, +/// because the convention is not universal. Windows leaves Shift intact on +/// the numpad's `/ * + -`: `need_to_convert_to_shifted_key` lists the OEM +/// keys and `VK_0..VK_9` but none of `VK_DIVIDE`/`VK_MULTIPLY`/`VK_ADD`/ +/// `VK_SUBTRACT`, so `get_keystroke_key` falls through to `get_key_from_vkey` +/// and Ctrl+Shift+numpad-/ really does arrive as `ctrl-shift-/`. Substituting +/// `ctrl-?` for it would unbind a chord that works today — the same class of +/// bug this is fixing. macOS clears the flag there anyway (the numpad glyph +/// is the same shifted or not, so `chars_with_shift` takes the branch that +/// drops it) and Linux names those keys `divide`/`multiply`/`add`/`subtract`, +/// which are not one character and never reach this. fn fold_shift_into_glyph(chord: &str) -> Option { let mut ks = Keystroke::parse(chord).ok()?; if !ks.modifiers.shift { @@ -1001,20 +1028,38 @@ fn fold_shift_into_glyph(chord: &str) -> Option { spec_from_keystroke(&ks) } -/// `spec` with every chord folded by `fold_shift_into_glyph`, ready to hand to -/// `KeyBinding::new`. Anything already canonical comes back untouched. -pub(crate) fn dispatchable_spec(spec: &str) -> String { - let mut out = String::with_capacity(spec.len()); +/// `spec` with every chord folded, or `None` when it is already the shape the +/// platform delivers and there is nothing to add. +pub(crate) fn folded_spec(spec: &str) -> Option { + let mut folded = String::with_capacity(spec.len()); + let mut changed = false; for chord in spec.split_whitespace() { - if !out.is_empty() { - out.push(' '); + if !folded.is_empty() { + folded.push(' '); } match fold_shift_into_glyph(chord) { - Some(folded) => out.push_str(&folded), - None => out.push_str(chord), + Some(chord) => { + folded.push_str(&chord); + changed = true; + } + None => folded.push_str(chord), } } - out + changed.then_some(folded) +} + +/// Whether two specs claim the same keystroke — spelled the same way, or one +/// the fold of the other. `secondary-shift-]` and `secondary-}` are one chord +/// written twice, and a rebind that cannot see that leaves both installed on +/// it, where which one fires is arbitrary (#750). +pub(crate) fn same_chord(a: &str, b: &str) -> bool { + if a == b { + return true; + } + let (folded_a, folded_b) = (folded_spec(a), folded_spec(b)); + folded_a.as_deref() == Some(b) + || folded_b.as_deref() == Some(a) + || (folded_a.is_some() && folded_a == folded_b) } pub(crate) fn key_chords(spec: &str) -> Vec> { @@ -1111,10 +1156,6 @@ fn action_context(action: &str) -> Option<&'static str> { } fn make_binding(action: &str, keystroke: &str) -> Option { - // The one place every binding is built, and so the one place worth folding - // a `shift-` spec into the chord the dispatcher will see. - let folded = dispatchable_spec(keystroke); - let keystroke = folded.as_str(); Some(match action { "NewTab" => KeyBinding::new(keystroke, NewTab, None), "NewWorkspace" => KeyBinding::new(keystroke, NewWorkspace, None), @@ -1768,20 +1809,21 @@ mod tests { // `ScmCommit` and `ToggleFullscreen` both take secondary-enter on that // basis. Two bindings sharing a chord *and* a context is still a bug, // because then which one fires is arbitrary. - // Compared folded, since `secondary-shift-]` and `secondary-}` are two - // spellings of one keystroke and would otherwise both slip through. - let mut seen: Vec<(&str, String, Option<&'static str>)> = Vec::new(); + // `same_chord`, not string equality: `secondary-shift-]` and + // `secondary-}` are two spellings of one keystroke and both install it. + let mut seen: Vec<(&str, &str, Option<&'static str>)> = Vec::new(); for (action, spec) in default_bindings() { if spec.is_empty() { continue; } - let chord = dispatchable_spec(spec); let context = action_context(action); - if let Some((other, _, _)) = seen.iter().find(|(_, s, c)| *s == chord && *c == context) + if let Some((other, _, _)) = seen + .iter() + .find(|(_, s, c)| same_chord(s, spec) && *c == context) { - panic!("{action} and {other} both claim {chord} in context {context:?}"); + panic!("{action} and {other} both claim {spec} in context {context:?}"); } - seen.push((action, chord, context)); + seen.push((action, spec, context)); } } @@ -1856,10 +1898,58 @@ mod tests { "secondary--", "ctrl-b n", ] { - assert_eq!(dispatchable_spec(spec), spec, "{spec} was rewritten"); + assert_eq!(folded_spec(spec), None, "{spec} was rewritten"); } // A prefix chord folds per chord, not as a whole. - assert_eq!(dispatchable_spec("ctrl-b shift-5"), "ctrl-b %"); + assert_eq!(folded_spec("ctrl-b shift-5").as_deref(), Some("ctrl-b %")); + } + + #[test] + fn a_chord_the_platform_delivers_with_shift_intact_stays_bound() { + // Windows leaves Shift on the numpad's `/ * + -` — they are absent + // from gpui's `need_to_convert_to_shifted_key`, so Ctrl+Shift+numpad-/ + // arrives as `ctrl-shift-/` and the recorder writes exactly that. + // Folding is an addition, never a substitution, so that spec has to + // survive the treatment `secondary-shift-]` gets (#750). + let numpad = Keystroke { + modifiers: gpui::Modifiers { + control: cfg!(not(target_os = "macos")), + platform: cfg!(target_os = "macos"), + shift: true, + alt: false, + function: false, + }, + key: "/".to_string(), + key_char: None, + }; + assert_eq!( + spec_from_keystroke(&numpad).as_deref(), + Some("secondary-shift-/") + ); + let effective = [("NextTab".to_string(), "secondary-shift-/".to_string())]; + assert_eq!( + dispatched_keystrokes(&effective, std::slice::from_ref(&numpad), ""), + vec![NextTab::name_for_type()], + "the numpad chord lost the binding written for it" + ); + // And the main row's `?`, which the same spec also stands for, still + // reaches the action — the fold added it rather than taking over. + assert_eq!( + dispatched_keystrokes(&effective, &[delivered_with_secondary("?")], ""), + vec![NextTab::name_for_type()], + ); + } + + #[test] + fn two_spellings_of_one_chord_are_recognised_as_one() { + // What `assign_keybinding` leans on to displace the action already + // holding a chord, whichever way either of them is written down. + assert!(same_chord("secondary-shift-]", "secondary-}")); + assert!(same_chord("secondary-}", "secondary-shift-]")); + assert!(same_chord("secondary-shift-}", "secondary-shift-]")); + assert!(same_chord("secondary-t", "secondary-t")); + assert!(!same_chord("secondary-shift-]", "secondary-shift-[")); + assert!(!same_chord("secondary-}", "secondary-{")); } #[test]