fix(keybindings): give Alt+1..9 a way out, and stop one bad line resetting the file (#901)

Alt+1..9 are vim's tab keys, and tty7 takes all nine for Go to Tab. Two things stood between the reporter and getting them back. **Nothing in the app could leave an action unbound.** Backspace on a Keybindings row that has recorded nothing *reset* the row — dropped the override so the action gets its shipped chord back. On a row nobody had overridden, which is every row the first time it is looked at, that is a no-op: pressing it over Alt+1 left Alt+1 sitting exactly where it was, which reads as the default restoring itself. `config.json` has spelled "no chord" as `[]` since #868, but no gesture wrote it. Backspace now writes that empty list. The row falls to `—` and grows the **Reset** button every overridden row has, which is the way back to the default. The capture hint names the key, and the docs say what it is for. **A keybinding line serde could not read failed the whole `Config`.** `keybindings` is a hand-edited map and was strict, so `"ActivateTab1": null` — or a number, or an object — quarantined `config.json` and started the app on built-in defaults. Every rebinding in the file then read as its shipped default, and the next settings write persisted those defaults over what the user had written. It now reads one entry at a time, like every other hand-edited nested key here: the lines that name a shortcut bind, a line that does not is logged and skipped. Tests, each failing on the unfixed code: - `ui::app::keybinding_gpui_tests::backspace_on_a_row_unbinds_the_action_rather_than_restoring_its_default` - `core::config::tests::a_keybinding_line_that_cannot_be_read_does_not_take_the_config_with_it` and `ui::keymap::gpui_tests::alt_digits_can_be_moved_off_the_tab_actions_for_good` pins the merge and a save/reload round trip: a list replaces the shipped Alt+1, `[]` leaves nothing, and neither comes back after a restart. Fixes #901 Claude-Session: https://claude.ai/code/session_01JRqYZ9E153WpSHGS2AW3BM
2026-09-21 16:02:20 +00:00 · 2026-09-21 22:42:45 +08:00
parent ffce219ffa
commit a723e71c9a
7 changed files with 223 additions and 7 deletions
@@ -167,6 +167,15 @@ pub struct Config {
    pub window_backdrop: WindowBackdrop,
    #[serde(default = "default_true")]
    pub dim_inactive_panes: bool,
+    /// Lenient one entry at a time, for the same reason the nested keys below
+    /// are: this is hand-edited, and it used to be all-or-nothing. A single
+    /// value serde could not read — `"ActivateTab1": null`, a number, an object
+    /// — failed the whole `Config`, which quarantines `config.json` and starts
+    /// the app on built-in defaults; every rebinding in the file then read as
+    /// its shipped default, and the next settings write persisted those
+    /// defaults over what the user wrote (#901). A line that cannot be read is
+    /// skipped with a warning, and the rest of the map still binds.
+    #[serde(default, deserialize_with = "de_keybindings")]
    pub keybindings: HashMap<String, KeybindingOverride>,
    #[serde(default = "default_preset")]
    pub keybinding_preset: String,
@@ -1213,6 +1222,37 @@ fn default_sidebar_width() -> f32 {

 pub const MAX_SCROLLBACK: usize = 100_000;

+/// `keybindings`, read one line at a time.
+///
+/// [`de_lenient`] is all-or-nothing per field, which for a map means one bad
+/// line throwing away every good one. Here each entry stands on its own: the
+/// ones that name a shortcut or a list of shortcuts bind, the ones that do not
+/// are logged and dropped. A `keybindings` that is not an object at all falls
+/// back to an empty map rather than failing the file.
+fn de_keybindings<'de, D>(deserializer: D) -> Result<HashMap<String, KeybindingOverride>, D::Error>
+where
+    D: serde::Deserializer<'de>,
+{
+    let value = serde_json::Value::deserialize(deserializer)?;
+    let serde_json::Value::Object(entries) = value else {
+        log::warn!("ignoring `keybindings` {value}: expected an object of action name to shortcut");
+        return Ok(HashMap::new());
+    };
+    let mut bindings = HashMap::with_capacity(entries.len());
+    for (action, raw) in entries {
+        match KeybindingOverride::deserialize(&raw) {
+            Ok(binding) => {
+                bindings.insert(action, binding);
+            }
+            Err(e) => log::warn!(
+                "ignoring keybinding for {action:?}: {raw} is not a shortcut, \
+                 a list of shortcuts, or \"\" to unbind ({e})"
+            ),
+        }
+    }
+    Ok(bindings)
+}
+
 pub(crate) fn de_lenient<'de, D, T>(deserializer: D) -> Result<T, D::Error>
 where
    D: serde::Deserializer<'de>,
@@ -1968,6 +2008,34 @@ mod tests {
        assert_eq!(serde_json::to_value(&cfg.keybindings).unwrap(), written);
    }

+    #[test]
+    fn a_keybinding_line_that_cannot_be_read_does_not_take_the_config_with_it() {
+        // #901: one unreadable line used to fail the whole `Config`. The loader
+        // then quarantines config.json and starts on built-in defaults, so
+        // every rebinding in the file — the point of the file — came back as
+        // the shipped default, and the next settings write made that permanent.
+        let cfg: Config = serde_json::from_str(
+            r#"{"font_size": 20.0,
+                "keybindings": {"ActivateTab1": null, "ActivateTab2": 2,
+                                "ActivateTab3": {"key": "alt-shift-3"},
+                                "ActivateTab4": [], "NextTab": "ctrl-alt-]"}}"#,
+        )
+        .expect("a bad keybinding line must not fail the file");
+        assert_eq!(cfg.font_size, 20.0, "the rest of the file still loads");
+        assert_eq!(
+            serde_json::to_value(&cfg.keybindings).unwrap(),
+            serde_json::json!({"ActivateTab4": [], "NextTab": "ctrl-alt-]"}),
+            "the readable lines survive and the unreadable ones are dropped"
+        );
+
+        // And a `keybindings` that is not a map at all is no reason to hand
+        // the user back default fonts, themes and everything else.
+        let odd: Config = serde_json::from_str(r#"{"font_size": 20.0, "keybindings": []}"#)
+            .expect("a keybindings of the wrong shape must not fail the file");
+        assert_eq!(odd.font_size, 20.0);
+        assert!(odd.keybindings.is_empty());
+    }
+
    fn pin_config_dir() {
        let dir = std::env::temp_dir().join(format!("tty7-covtest-{}", std::process::id()));
        std::fs::create_dir_all(&dir).ok();
@@ -19,7 +19,13 @@ Click a shortcut and press the new keys. It saves after a brief pause.
 | Press keys | Set the binding |
 | Press more keys | Chain a sequence — <kbd>⌃ B</kbd> then <kbd>X</kbd> |
 | <kbd>Esc</kbd> | Cancel |
-| <kbd>⌫</kbd> | Remove the last key — or, pressed first, reset the shortcut to its default |
+| <kbd>⌫</kbd> | Remove the last key — or, pressed first, leave the action with **no shortcut** |
+
+Leaving an action unbound is how you hand a key back to whatever is running in
+the terminal: <kbd>⌥ 1</kbd>…<kbd>⌥ 9</kbd> jump between tabs by default, and
+vim wants them for its own tabs. Clear the nine **Go to Tab** rows and the
+digits go straight through. A cleared row shows `—` and grows a **Reset**
+button, which puts the default back.

 **Restore all defaults** at the bottom undoes every rebinding at once. There is
 no undo for that one.
@@ -74,8 +80,22 @@ space.
 | `cmd` · `ctrl` · `alt` · `shift` | Literal modifiers |
 | `ctrl-b n` | A two-key sequence |

-An unknown action name or an invalid keystroke is skipped with a warning in the
-log rather than breaking the rest of your bindings.
+To keep a key for the program running in the terminal, unbind the action that
+holds it. vim's tab keys, for instance:
+
+```json
+{
+  "keybindings": {
+    "ActivateTab1": [], "ActivateTab2": [], "ActivateTab3": [],
+    "ActivateTab4": [], "ActivateTab5": [], "ActivateTab6": [],
+    "ActivateTab7": [], "ActivateTab8": [], "ActivateTab9": []
+  }
+}
+```
+
+An unknown action name, an invalid keystroke, or a line that is neither a
+shortcut nor a list of them is skipped with a warning in the log rather than
+breaking the rest of your bindings.

 The full action list is on the [keyboard shortcuts](/reference/keyboard-shortcuts)
 page.
@@ -7134,7 +7134,7 @@ impl Tty7App {
                    cx.notify();
                } else {
                    self.stop_recording(cx);
-                    self.reset_keybinding(action, cx);
+                    self.unbind_keybinding(action, cx);
                }
                return;
            }
@@ -7253,6 +7253,34 @@ impl Tty7App {
        cx.notify();
    }

+    /// Takes every chord off an action, and keeps it off.
+    ///
+    /// ⌫ on a row that has recorded nothing used to *reset* it — drop the
+    /// override so the action gets its shipped chord back. On a row nobody has
+    /// overridden, which is every row the first time it is looked at, that is a
+    /// no-op: someone pressing Backspace over Alt+1 to be rid of it watched
+    /// Alt+1 sit exactly where it was and read it as the default restoring
+    /// itself (#901). Nothing anywhere in the app said "this action should have
+    /// no key", though `config.json` has spelled it `[]` since #868.
+    ///
+    /// So ⌫ writes that empty list, and the **Reset** button beside the row —
+    /// which appears the moment an action is overridden, this way included — is
+    /// the way back to the default.
+    pub(crate) fn unbind_keybinding(&mut self, action: String, cx: &mut Context<Self>) {
+        self.update_config(cx, |cfg| {
+            cfg.keybindings.insert(
+                action,
+                crate::core::config::KeybindingOverride::Exact(Vec::new()),
+            );
+        });
+        crate::ui::keymap::rebind(cx);
+        if let Some(s) = self.active_settings_mut() {
+            s.recording = None;
+            s.rebinding_note = None;
+        }
+        cx.notify();
+    }
+
    pub(crate) fn reset_keybinding(&mut self, action: String, cx: &mut Context<Self>) {
        self.update_config(cx, |cfg| {
            cfg.keybindings.remove(&action);
@@ -10525,6 +10553,56 @@ mod keybinding_gpui_tests {
        );
    }

+    /// #901, the half that happens in the UI: Alt+1…9 belongs to vim, and the
+    /// only gesture in the app that looks like "take this shortcut away" used
+    /// to *reset* the row instead — a no-op on a row nobody had overridden,
+    /// so the default appeared to restore itself however many times it was
+    /// pressed.
+    #[gpui::test]
+    fn backspace_on_a_row_unbinds_the_action_rather_than_restoring_its_default(
+        cx: &mut TestAppContext,
+    ) {
+        let (app, mut vcx) = harness(cx);
+        let shipped = vcx
+            .update(|_, cx| crate::ui::keymap::effective_key("ActivateTab1", cx))
+            .expect("Go to Tab 1 ships with a chord");
+
+        begin_capture(&app, &mut vcx, "ActivateTab1");
+        vcx.simulate_keystrokes("backspace");
+        wait_for_binding(&mut vcx, "ActivateTab1", serde_json::json!([]));
+
+        vcx.update(|_, cx| {
+            assert_eq!(
+                crate::ui::keymap::effective_key("ActivateTab1", cx),
+                None,
+                "the row has no chord left to show"
+            );
+            let typed = [gpui::Keystroke::parse(&shipped).expect("the chord parses")];
+            let context = [gpui::KeyContext::parse("Terminal").expect("the context parses")];
+            assert!(
+                cx.key_bindings()
+                    .borrow()
+                    .bindings_for_input(&typed, &context)
+                    .0
+                    .is_empty(),
+                "{shipped} must reach the terminal now, not the tab switcher"
+            );
+        });
+
+        // Reversible, and by the button that is already on the row: an
+        // overridden action — unbound counts — shows **Reset**.
+        app.update_in(&mut vcx, |app, _, cx| {
+            app.reset_keybinding("ActivateTab1".to_string(), cx)
+        });
+        vcx.update(|_, cx| {
+            assert_eq!(
+                crate::ui::keymap::effective_key("ActivateTab1", cx).as_deref(),
+                Some(shipped.as_str()),
+                "Reset is the way back to the shipped chord"
+            );
+        });
+    }
+
    #[gpui::test]
    fn escape_cancels_capture_without_writing(cx: &mut TestAppContext) {
        let (app, mut vcx) = harness(cx);
@@ -628,7 +628,7 @@ pub fn translate_en(key: L10nKey) -> &'static str {
            "tmux remaps pane/tab actions onto prefix sequences (e.g. Ctrl-B then C)."
        }
        L10nKey::SettingsPrefix => "Prefix",
-        L10nKey::SettingsPressKeys => "Press keys…",
+        L10nKey::SettingsPressKeys => "Press keys… · ⌫ for no shortcut",
        L10nKey::SettingsPauseToSaveEsc => "pause to save · Esc",
        L10nKey::SettingsKeybindingsIntroDesc => {
            "Click a shortcut, then press the new keys — it saves after a brief pause. Chain keys for a sequence like Ctrl-B then X. Esc cancels; Backspace removes the last key, or resets to default if pressed first."
@@ -635,7 +635,7 @@ pub fn translate_ja(key: L10nKey) -> Option<&'static str> {
            "tmux では、ペイン/タブの操作をプレフィックスキーの後に行います（例: Ctrl-B の後に C）"
        }
        L10nKey::SettingsPrefix => "プレフィックスキー",
-        L10nKey::SettingsPressKeys => "キーを入力…",
+        L10nKey::SettingsPressKeys => "キーを入力… · ⌫ でショートカットなし",
        L10nKey::SettingsPauseToSaveEsc => "一時停止して保存 · Esc",
        L10nKey::SettingsKeybindingsIntroDesc => {
            "ショートカットをクリックして新しいキーを押すと、少し間を置いて保存されます。Ctrl-B の後に X のようなシーケンスはキーを続けて入力。Esc でキャンセル、Backspace は最後のキーを削除し、最初に押すとデフォルトに戻します"
@@ -554,7 +554,7 @@ pub fn translate_zh(key: L10nKey) -> Option<&'static str> {
            "tmux 预设把窗格/标签页操作映射为前缀序列（例如 Ctrl-B 后按 C）。"
        }
        L10nKey::SettingsPrefix => "前缀",
-        L10nKey::SettingsPressKeys => "按下按键…",
+        L10nKey::SettingsPressKeys => "按下按键… · ⌫ 表示不设快捷键",
        L10nKey::SettingsPauseToSaveEsc => "暂停以保存 · Esc",
        L10nKey::SettingsKeybindingsIntroDesc => {
            "点击某个快捷键，再按下新按键，短暂停顿后保存。连续按键可组成序列，例如 Ctrl-B 后按 X。Esc 取消；Backspace 移除最后一个按键，最先按下则重置为默认。"
@@ -2462,6 +2462,56 @@ mod gpui_tests {
        });
    }

+    /// #901: Alt+1…9 is how vim switches tabs, and tty7's default was eating
+    /// the whole row of them. Both halves of what the reporter wanted have to
+    /// hold, and hold across a restart — the complaint was that the default
+    /// "keeps coming back".
+    #[gpui::test]
+    fn alt_digits_can_be_moved_off_the_tab_actions_for_good(cx: &mut TestAppContext) {
+        use gpui::Action as _;
+        cx.update(|cx| {
+            let default_chord = per_platform("secondary-1", "alt-1");
+            running_on_json(
+                cx,
+                r#"{"keybindings": {"ActivateTab1": ["alt-shift-1"],
+                                    "ActivateTab2": []}}"#,
+            );
+            assert!(
+                fired(cx, default_chord).is_empty(),
+                "a list replaces the shipped chord, so the digit reaches the shell"
+            );
+            assert!(
+                fired(cx, per_platform("secondary-2", "alt-2")).is_empty(),
+                "and an empty list leaves the action with no chord at all"
+            );
+            assert_eq!(
+                fired(cx, "alt-shift-1").first(),
+                Some(&ActivateTab1::name_for_type()),
+                "the chord asked for in its place is live"
+            );
+            assert_eq!(effective_key("ActivateTab2", cx), None);
+
+            // The half that reads as "老是自动恢复": whatever the app saves has
+            // to load back as the same thing. A `Config` serialized and read
+            // again is exactly what a restart does with `config.json`.
+            let saved =
+                serde_json::to_string(&**cx.global::<Config>()).expect("the config serializes");
+            cx.set_global(Config(
+                serde_json::from_str(&saved).expect("the saved config parses"),
+            ));
+            rebind(cx);
+            assert!(
+                fired(cx, default_chord).is_empty(),
+                "the shipped chord must not come back across a save and reload"
+            );
+            assert_eq!(
+                fired(cx, "alt-shift-1").first(),
+                Some(&ActivateTab1::name_for_type()),
+            );
+            assert_eq!(effective_key("ActivateTab2", cx), None);
+        });
+    }
+
    #[gpui::test]
    fn a_chord_added_under_the_tmux_preset_joins_the_preset_chord(cx: &mut TestAppContext) {
        use gpui::Action as _;