From 24bc48704a49439e1548050fda2204f3e6a8cb43 Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Sun, 23 Aug 2026 07:49:42 +0800 Subject: [PATCH] fix(keymap): register the fixed bindings before the config, not after MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit gpui resolves a keystroke by sorting the matches on context depth and then on registration index, later winning: matched_bindings.sort_by(|(depth_a, ix_a, _), (depth_b, ix_b, _)| { depth_b.cmp(depth_a).then(ix_b.cmp(ix_a)) }); `rebuild_keymap` added the config's bindings and then the fixed ones, so a fixed binding won any tie. Six of the seven are scoped — `Terminal`, `Switcher`, `Palette` — and win on depth whatever the order. The seventh is global: `secondary-+`, the font-size step. A config that asked for that chord got `IncreaseFontSize` instead, with no error, nothing in the keybindings UI, and nothing in the log — which is the silence this tree has already gone out of its way to remove from an unparseable chord, a clamped setting and an unread config key. Swapping the two lines fixes it and changes nothing else: depth is compared before index, so the scoped six still win. And it matches what "fixed" was ever for, which the comment on `fixed_bindings` states — they are not in `effective_bindings`, so a rebuild replaying only the config would drop them. That is an argument about existing, not about outranking. Found by checking the reverse direction of the shortcuts page: every chord it prints against every chord tty7 binds. Nothing was undocumented — `⌘ C`, `⌘ V`, `⌃ R` and the arrow shorthands are keys tty7 answers without a keymap action, which is why a strict guard there would be wrong — but `⌘ +` turned out to be bound twice, and following that up is what surfaced the ordering. --- CHANGELOG.md | 12 +++++++++++ src/ui/keymap.rs | 52 +++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 63 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a415a947..6dc2e8a5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -217,6 +217,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 which is the wrong conclusion to reach while reading a log to work out when something happened. +- **A keybinding you set for `⌘+` is no longer silently overridden.** gpui + resolves a keystroke by context depth and then by registration order, later + winning, and tty7's fixed bindings were registered after the config — so the + one *global* entry among them, the font-size step on `⌘+`, beat anything a + config asked for on that chord, with no error and nothing in the keybindings + UI to explain it. The fixed bindings are registered first now. The others are + unaffected: they are scoped to Terminal, Switcher and Palette, which are + deeper contexts than a config binding can name, and depth is compared before + order. What "fixed" was for is that they exist at all — they are not in the + config table, so a rebuild replaying only the config would drop them — not + that they outrank what somebody typed. + - **A commit author's name draws on one row.** git refuses a control character in a *branch* name and accepts one in an author name without comment — `git -c user.name=$'Bad\rName' commit` succeeds and diff --git a/src/ui/keymap.rs b/src/ui/keymap.rs index 4c24580b..83cb3d8f 100644 --- a/src/ui/keymap.rs +++ b/src/ui/keymap.rs @@ -80,8 +80,21 @@ fn rebuild_keymap(cx: &mut App) { .try_global::() .map(|base| base.0.clone()) .unwrap_or_default(); - bindings.extend(action_bindings(&effective)); + // Fixed first, the config after it. gpui sorts a keystroke's matches by + // context depth and then by *index*, later winning — so adding these last + // let the one global entry among them, `secondary-+`, beat a config that + // asked for that chord. The user got `IncreaseFontSize` and no word about + // why, which is the same silence this tree already went to some trouble to + // remove from an unparseable chord and a clamped setting. + // + // The other fixed bindings are unaffected: `Terminal`, `Switcher` and + // `Palette` are deeper contexts than a config binding's, and depth is + // compared before index. What "fixed" was ever meant to buy them is that + // they *exist* — they are not in `effective_bindings`, so a rebuild that + // replayed only the config would drop them — not that they outrank what + // somebody typed. bindings.extend(fixed_bindings()); + bindings.extend(action_bindings(&effective)); cx.clear_key_bindings(); cx.bind_keys(bindings); } @@ -1511,6 +1524,42 @@ mod tests { #[cfg(not(target_os = "macos"))] const CTRL: &str = "Ctrl"; + /// A fixed binding does not outrank the config. + /// + /// gpui resolves a keystroke by context depth first and then by index, + /// with the later-added binding winning. `fixed_bindings` were added after + /// the config, so the one *global* entry among them — `secondary-+` for + /// the font step — could not be rebound: a config asking for that chord + /// got `IncreaseFontSize` instead, silently. This tree has already gone to + /// some trouble to stop a keybinding being ignored without a word. + /// + /// The context-scoped ones are unaffected by the order, because depth is + /// compared first, and they need to win: a `tab` bound in `Terminal` or + /// `Switcher` is deeper than anything the config can express. + /// + /// Read from the source, because the property is the order of two lines + /// and exercising it would mean standing up a gpui `App` to press a key + /// into. + #[test] + fn the_fixed_bindings_are_registered_before_the_config_not_after() { + const SRC: &str = include_str!("keymap.rs"); + let start = SRC + .find("fn rebuild_keymap(") + .expect("the keymap is rebuilt here"); + let body = &SRC[start..]; + let body = &body[..body.find("\n}\n").expect("the function ends")]; + + let fixed = body.find("bindings.extend(fixed_bindings())").expect("fixed"); + let config = body + .find("bindings.extend(action_bindings(") + .expect("the config's own bindings"); + assert!( + fixed < config, + "the fixed bindings are added after the config, so the global one \ + among them shadows a chord the user asked for" + ); + } + #[test] fn key_tokens_maps_modifiers_to_glyphs() { assert_eq!(key_tokens("secondary-t"), vec![SECONDARY, "T"]); @@ -2206,3 +2255,4 @@ mod gpui_tests { }); } } +