starred/tty7
1
0
Fork 0
mirror of https://github.com/l0ng-ai/tty7.git synced 2026-09-22 00:02:23 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(keymap): add the folded chord beside the written one, never in place of it (#750)

Browse Source 
Substituting the fold was wrong for the numpad. `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-/ arrives as `ctrl-shift-/`, shift intact. The recorder
writes that spec and it dispatches today — folding it to `secondary-?` would
have unbound a working chord, which is the bug this set out to fix.

The convention is not uniform: macOS clears the flag there anyway, since the
numpad glyph is the same shifted or not, and Linux names those keys divide/
multiply/add/subtract, which are not one character and never reached the fold.
No glyph list can tell the two cases apart, so stop trying to choose — install
both spellings. `push_binding` lays down the spec as written and the folded
chord beside it, so the fold can only ever add a keystroke the dispatcher can
reach.

`assign_keybinding` and the default-chord dedup test move to `same_chord`,
which asks whether two specs claim one keystroke rather than comparing folded
text.
This commit is contained in:
l0ng-ai
2026-09-07 19:19:37 +08:00
parent 9d410cb4ec
commit 71f53c8a40
2 changed files with 134 additions and 45 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/ui/app.rs
+6 -7
View File
@@ -6659,16 +6659,15 @@ impl Tty7App {
}
fn assign_keybinding(&mut self, action: String, spec: String, cx: &mut Context<Self>) {
// 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
src/ui/keymap.rs
+128 -38
View File
@@ -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<KeyBinding>, 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<KeyBinding> {
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> {
"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<String> {
let mut ks = Keystroke::parse(chord).ok()?;
if !ks.modifiers.shift {
@@ -1001,20 +1028,38 @@ fn fold_shift_into_glyph(chord: &str) -> Option<String> {
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<String> {
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<Vec<String>> {
@@ -1111,10 +1156,6 @@ fn action_context(action: &str) -> Option<&'static str> {
}
fn make_binding(action: &str, keystroke: &str) -> Option<KeyBinding> {
// The one place every binding is built, and so the one place worth folding
// a `shift-<punctuation>` 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]