diff --git a/crates/tty7-core/src/core/config.rs b/crates/tty7-core/src/core/config.rs index bdf715f2..4058a7b4 100644 --- a/crates/tty7-core/src/core/config.rs +++ b/crates/tty7-core/src/core/config.rs @@ -110,6 +110,23 @@ impl serde::Serialize for FontFeatures { } } +/// One action's line in `keybindings`. +/// +/// The two shapes mean different things, so a save writes back whichever one +/// was read. +#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Deserialize, Serialize)] +#[serde(untagged)] +pub enum KeybindingOverride { + /// `"NextTab": "cmd-shift-]"` — a chord *beside* the ones the action + /// already has, the way VS Code, Zed and kitty read a line like it (#868). + /// Empty unbinds the action, which is what `""` has always meant here. + Add(String), + /// `"NextTab": ["cmd-shift-]"]` — exactly these chords, replacing the + /// default and the preset's. `[]` unbinds. This is what the Settings page + /// writes, because recording a shortcut there sets it. + Exact(Vec), +} + #[derive(Debug, Clone, Deserialize, Serialize)] #[serde(default)] pub struct Config { @@ -150,7 +167,7 @@ pub struct Config { pub window_backdrop: WindowBackdrop, #[serde(default = "default_true")] pub dim_inactive_panes: bool, - pub keybindings: HashMap, + pub keybindings: HashMap, #[serde(default = "default_preset")] pub keybinding_preset: String, #[serde(default = "default_prefix")] diff --git a/docs/customization/keybindings.mdx b/docs/customization/keybindings.mdx index ab36b884..63ad7344 100644 --- a/docs/customization/keybindings.mdx +++ b/docs/customization/keybindings.mdx @@ -39,13 +39,26 @@ the panel tabs. They are all in the command palette, and all bindable here. ```json { "keybindings": { - "SplitRight": "cmd-d", + "NextTab": "cmd-shift-]", + "SplitRight": ["cmd-d"], "ResizePaneLeft": "ctrl-alt-left", - "ToggleSftp": "cmd-shift-u" + "ToggleFullscreen": "" } } ``` +An action's value takes one of two shapes: + +| Value | Means | +|---|---| +| `"cmd-shift-]"` | **Add** this shortcut. The default keeps working — ⌃ ⇥ still switches tabs | +| `["cmd-d"]` | **Replace**: exactly these shortcuts, nothing else. List several to have several | +| `""` or `[]` | **Unbind** the action | + +Recording a shortcut on the Settings page replaces, so it writes the list +shape. A shortcut you add that another action already uses wins: it is the one +that runs. + The syntax is modifiers joined by `-`, then the key. Chords are separated by a space. diff --git a/src/main.rs b/src/main.rs index a0412c52..a5793199 100644 --- a/src/main.rs +++ b/src/main.rs @@ -742,9 +742,10 @@ mod config_reload_tests { } fn bound_to_split_right(config: &mut Config, key: &str) { - config - .keybindings - .insert("SplitRight".to_string(), key.to_string()); + config.keybindings.insert( + "SplitRight".to_string(), + crate::core::config::KeybindingOverride::Add(key.to_string()), + ); } #[gpui::test] @@ -821,11 +822,10 @@ mod config_reload_tests { ); assert!(announced, "the breakage is announced"); assert_eq!( - cx.global::() - .keybindings - .get("SplitRight") - .map(String::as_str), - Some("ctrl-alt-9"), + cx.global::().keybindings.get("SplitRight"), + Some(&crate::core::config::KeybindingOverride::Add( + "ctrl-alt-9".to_string() + )), "the running config survives the broken file" ); assert_eq!( diff --git a/src/ui/app.rs b/src/ui/app.rs index 5055666a..b27e58c9 100644 --- a/src/ui/app.rs +++ b/src/ui/app.rs @@ -7194,16 +7194,32 @@ impl Tty7App { // 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) + // + // Only that chord moves: the action that had it keeps any others it + // has, since an action can carry several (#868) and emptying it would + // take away keys that were never on this keystroke. An extra default — + // Alt+Enter beside Shift+Enter — follows its action's first chord rather + // than being one of its own, so its owner is unbound outright, as it + // always was. + use crate::ui::keymap::same_chord; + let displaced: Option<(String, Vec)> = crate::ui::keymap::effective_chords(cx) .into_iter() - .chain(crate::ui::keymap::extra_bindings(cx)) - .find(|(a, k)| *a != action && crate::ui::keymap::same_chord(k, &spec)) - .map(|(a, _)| a); + .find(|(a, chords)| *a != action && chords.iter().any(|k| same_chord(k, &spec))) + .map(|(a, chords)| { + let rest = chords.into_iter().filter(|k| !same_chord(k, &spec)); + (a, rest.collect()) + }) + .or_else(|| { + crate::ui::keymap::extra_bindings(cx) + .into_iter() + .find(|(a, k)| *a != action && same_chord(k, &spec)) + .map(|(a, _)| (a, Vec::new())) + }); // A trailing "…" on an action name marks a command that opens // something; it is not punctuation, and inside a sentence it reads as // the sentence trailing off — "Rename Tab… took the shortcut from". let in_prose = |name: &str| name.trim_end_matches('…').to_string(); - let note = displaced.as_ref().map(|other| { + let note = displaced.as_ref().map(|(other, _)| { t_fmt( L10nKey::AppKeybindingDisplacedNote, &[ @@ -7218,11 +7234,17 @@ impl Tty7App { ], ) }); + // Both written as lists. Recording a shortcut sets it — the row showed + // one chord and now shows another — and a bare string in config adds a + // chord beside the default instead (#868). self.update_config(cx, |cfg| { - if let Some(other) = &displaced { - cfg.keybindings.insert(other.clone(), String::new()); + use crate::core::config::KeybindingOverride; + if let Some((other, rest)) = &displaced { + cfg.keybindings + .insert(other.clone(), KeybindingOverride::Exact(rest.clone())); } - cfg.keybindings.insert(action, spec); + cfg.keybindings + .insert(action, KeybindingOverride::Exact(vec![spec])); }); crate::ui::keymap::rebind(cx); if let Some(s) = self.active_settings_mut() { @@ -10441,11 +10463,7 @@ mod keybinding_gpui_tests { // the row showed one chord and now shows another. A bare string in // config adds a chord beside the default (#868), which is not what // the person at the row just did. - wait_for_binding( - &mut vcx, - "NewTab", - serde_json::json!(["secondary-shift-n"]), - ); + wait_for_binding(&mut vcx, "NewTab", serde_json::json!(["secondary-shift-n"])); let recording = app.update_in(&mut vcx, |app, _, _| { app.active_settings().map(|s| s.recording.is_some()) @@ -10471,16 +10489,13 @@ mod keybinding_gpui_tests { } #[gpui::test] - fn recording_a_chord_another_action_also_has_takes_only_that_chord( - cx: &mut TestAppContext, - ) { + fn recording_a_chord_another_action_also_has_takes_only_that_chord(cx: &mut TestAppContext) { let (app, mut vcx) = harness(cx); vcx.update(|_, cx| { - cx.global_mut::().keybindings = - serde_json::from_value(serde_json::json!({ - "NextTab": ["ctrl-tab", "secondary-alt-n"], - })) - .expect("the binding loads"); + cx.global_mut::().keybindings = serde_json::from_value(serde_json::json!({ + "NextTab": ["ctrl-tab", "secondary-alt-n"], + })) + .expect("the binding loads"); crate::ui::keymap::rebind(cx); }); begin_capture(&app, &mut vcx, "NewTab"); diff --git a/src/ui/keymap.rs b/src/ui/keymap.rs index 447a6734..3cf3e14f 100644 --- a/src/ui/keymap.rs +++ b/src/ui/keymap.rs @@ -1,7 +1,7 @@ use gpui::{App, Global, KeyBinding, Keystroke, NoAction}; use crate::core::actions::*; -use crate::core::config::Config; +use crate::core::config::{Config, KeybindingOverride}; use crate::terminal::view::{ AlternatePaste, ClearScrollback, CopyText, FindInTerminal, FindNext, FindPrevious, InsertNewline, InsertNewlineFallback, PasteText, @@ -106,9 +106,9 @@ pub fn rebind(cx: &mut App) { /// own `save()`, which fires on a sidebar drag — so it compares the triple /// before and after and only rebinds when one of these actually moved; /// otherwise each save would rebuild the keymap for nothing (#548). -pub(crate) fn keybinding_config(cx: &App) -> (Vec<(String, String)>, String, String) { +pub(crate) fn keybinding_config(cx: &App) -> (Vec<(String, KeybindingOverride)>, String, String) { let cfg = cx.global::(); - let mut overrides: Vec<(String, String)> = cfg + let mut overrides: Vec<(String, KeybindingOverride)> = cfg .keybindings .iter() .map(|(a, k)| (a.clone(), k.clone())) @@ -881,28 +881,114 @@ fn authored_entry(action: &str) -> Option<(CommandGroup, String)> { }) } -pub(crate) fn effective_bindings(cx: &App) -> Vec<(String, String)> { - let cfg = cx.global::(); - let mut effective: Vec<(String, String)> = default_bindings() - .into_iter() - .map(|(a, k)| (a.to_string(), k.to_string())) - .collect(); - for (action, key) in preset_bindings(&cfg.keybinding_preset, &cfg.prefix) { - set_binding(&mut effective, &action, key); - } - for (action, key) in &cfg.keybindings { - set_binding(&mut effective, action, key.clone()); - } - effective +/// One action and the chords it answers to, split by where they came from: +/// the ones it ships with — its default, or the preset's in its place — and +/// the ones `config.json` names. +/// +/// Kept apart because they install apart. gpui resolves two bindings on one +/// chord to the one added last, and a chord the user asked for by name has to +/// be that one, wherever its action sits in the table. +struct ActionChords { + action: String, + shipped: Vec, + configured: Vec, } -fn set_binding(effective: &mut [(String, String)], action: &str, key: String) { - match effective.iter_mut().find(|(a, _)| a == action) { - Some(slot) => slot.1 = key, - // A hand-edited config.json with a typo used to vanish into this - // branch. The Keybindings page lists every name that works. - None => log::warn!("keybinding for unknown action {action:?} ignored"), +fn resolve_chords( + preset: &str, + prefix: &str, + overrides: &std::collections::HashMap, +) -> Vec { + let mut resolved: Vec = default_bindings() + .into_iter() + .map(|(action, key)| ActionChords { + action: action.to_string(), + shipped: [key] + .into_iter() + .filter(|k| !k.is_empty()) + .map(str::to_string) + .collect(), + configured: Vec::new(), + }) + .collect(); + // A preset is a scheme rather than an addition: its chord takes the + // default's place. + for (action, key) in preset_bindings(preset, prefix) { + if let Some(slot) = resolved.iter_mut().find(|s| s.action == action) { + slot.shipped = vec![key]; + } } + for (action, value) in overrides { + let Some(slot) = resolved.iter_mut().find(|s| s.action == *action) else { + // A hand-edited config.json with a typo used to vanish into this + // branch. The Keybindings page lists every name that works. + log::warn!("keybinding for unknown action {action:?} ignored"); + continue; + }; + match value { + // `""` has always been how a config retires a chord — the docs + // spell `"AlternatePaste": ""`, and Settings wrote it for an action + // whose chord was taken — so it goes on unbinding the action. + KeybindingOverride::Add(key) if key.is_empty() => { + slot.shipped.clear(); + } + // A chord beside the ones the action has, not in place of them + // (#868): `"NextTab": "cmd-shift-]"` is a second way to switch tabs, + // and Ctrl+Tab going dead because of it was the bug. + KeybindingOverride::Add(key) => { + if !slot.shipped.iter().any(|k| same_chord(k, key)) { + slot.configured.push(key.clone()); + } + } + KeybindingOverride::Exact(keys) => { + slot.shipped.clear(); + for key in keys { + if !key.is_empty() && !slot.configured.iter().any(|k| same_chord(k, key)) { + slot.configured.push(key.clone()); + } + } + } + } + } + resolved +} + +/// Every chord as an `(action, chord)` pair, in the order the keymap is built +/// from: all the shipped chords, then all the configured ones, so that a +/// configured chord landing on another action's default is the binding that +/// wins it. An action with no chord at all has no pair. +fn flatten_chords(resolved: &[ActionChords]) -> Vec<(String, String)> { + let pairs = |pick: fn(&ActionChords) -> &Vec| { + resolved + .iter() + .flat_map(move |s| pick(s).iter().map(|k| (s.action.clone(), k.clone()))) + }; + pairs(|s| &s.shipped) + .chain(pairs(|s| &s.configured)) + .collect() +} + +pub(crate) fn effective_bindings(cx: &App) -> Vec<(String, String)> { + let cfg = cx.global::(); + flatten_chords(&resolve_chords( + &cfg.keybinding_preset, + &cfg.prefix, + &cfg.keybindings, + )) +} + +/// Every action in table order with all of its chords, shipped first — an +/// empty list for an action that has none. What a page listing the actions +/// reads; the keymap reads [`effective_bindings`]. +pub(crate) fn effective_chords(cx: &App) -> Vec<(String, Vec)> { + let cfg = cx.global::(); + resolve_chords(&cfg.keybinding_preset, &cfg.prefix, &cfg.keybindings) + .into_iter() + .map(|mut s| { + s.shipped.append(&mut s.configured); + (s.action, s.shipped) + }) + .collect() } fn preset_bindings(preset: &str, prefix: &str) -> Vec<(String, String)> { @@ -1726,14 +1812,16 @@ mod tests { // one line in a `config.json`, so both are asserted against the whole // default table with that line applied — a bare one-entry table would // pass either assertion without the escape hatch working at all. - let mut retired = effective.clone(); - set_binding(&mut retired, "AlternatePaste", String::new()); + let with = |action: &str, value: KeybindingOverride| { + let overrides = std::collections::HashMap::from([(action.to_string(), value)]); + flatten_chords(&resolve_chords("default", "ctrl-b", &overrides)) + }; + let retired = with("AlternatePaste", KeybindingOverride::Add(String::new())); assert!( dispatched(&retired, "ctrl-v", "Terminal").is_empty(), "an emptied AlternatePaste gives Ctrl+V back to the shell" ); - let mut everywhere = effective.clone(); - set_binding(&mut everywhere, "PasteText", "ctrl-v".to_string()); + let everywhere = with("PasteText", KeybindingOverride::Add("ctrl-v".to_string())); for context in ["Terminal", "Terminal alt_screen"] { assert!( dispatched(&everywhere, "ctrl-v", context).contains(&PasteText::name_for_type()), @@ -2200,23 +2288,25 @@ mod gpui_tests { { let cfg = cx.global_mut::(); cfg.keybinding_preset = "tmux".to_string(); - cfg.keybindings - .insert("NewTab".to_string(), "secondary-shift-n".to_string()); + cfg.keybindings.insert( + "NewTab".to_string(), + KeybindingOverride::Add("secondary-shift-n".to_string()), + ); } rebind(cx); let eff = effective_bindings(cx); - let key_of = |action: &str| { + let keys_of = |action: &str| { eff.iter() - .find(|(a, _)| a == action) + .filter(|(a, _)| a == action) .map(|(_, k)| k.clone()) - .unwrap() + .collect::>() }; - assert_eq!(key_of("NewTab"), "secondary-shift-n"); - assert_eq!(key_of("SplitRight"), "ctrl-b %"); + assert_eq!(keys_of("NewTab"), ["ctrl-b c", "secondary-shift-n"]); + assert_eq!(keys_of("SplitRight"), ["ctrl-b %"]); assert_eq!( - key_of("TogglePalette"), - per_platform("secondary-p", "secondary-shift-p") + keys_of("TogglePalette"), + [per_platform("secondary-p", "secondary-shift-p")] ); cx.global_mut::().keybinding_preset = "default".to_string(); @@ -2251,9 +2341,10 @@ mod gpui_tests { assert_eq!(keybinding_config(cx), before); // A real binding edit moves it. - cx.global_mut::() - .keybindings - .insert("RenameTab".to_string(), "ctrl-shift-r".to_string()); + cx.global_mut::().keybindings.insert( + "RenameTab".to_string(), + KeybindingOverride::Add("ctrl-shift-r".to_string()), + ); assert_ne!(keybinding_config(cx), before); // So does the preset, and the prefix. @@ -2280,7 +2371,6 @@ mod gpui_tests { /// What the live keymap dispatches for `keys` typed in a terminal, best /// match first — the first entry is the action a real keypress runs. fn fired(cx: &gpui::App, keys: &str) -> Vec<&'static str> { - use gpui::Action as _; let input: Vec = keys .split(' ') .map(|k| Keystroke::parse(k).expect("the typed keystroke parses")) diff --git a/src/ui/settings.rs b/src/ui/settings.rs index 939f0f2d..0cf89b46 100644 --- a/src/ui/settings.rs +++ b/src/ui/settings.rs @@ -7200,7 +7200,7 @@ impl Tty7App { ) }; let tmux = preset == "tmux"; - let effective = crate::ui::keymap::effective_bindings(cx); + let effective = crate::ui::keymap::effective_chords(cx); let recording = self .active_settings() @@ -7308,7 +7308,7 @@ impl Tty7App { let filtering = !query.is_empty() && section_match_count(section, &query) > 0; let mut grouped: Vec<( crate::ui::palette::CommandGroup, - Vec<(String, String, String)>, + Vec<(String, Vec, String)>, )> = Vec::new(); for (action, key) in effective { if filtering && !keybinding_matches_query(&action, &query) { @@ -7330,7 +7330,7 @@ impl Tty7App { .position(|o| o == g) .unwrap_or(usize::MAX) }); - let rows: Vec<(String, String, String)> = grouped + let rows: Vec<(String, Vec, String)> = grouped .iter() .flat_map(|(_, rows)| rows.iter().cloned()) .collect(); @@ -7414,7 +7414,23 @@ impl Tty7App { .child("—") .into_any_element() } else { - keycaps(&key).into_any_element() + // An action can answer to more than one chord — its default and + // one added beside it in config.json (#868) — and this is the + // one page that lists them, so a row shows every one. + h_flex() + .flex_wrap() + .items_center() + .gap_2() + .children(key.iter().enumerate().map(|(n, spec)| { + h_flex() + .items_center() + .gap_2() + .when(n > 0, |d| { + d.child(div().text_xs().text_color(muted).child("/")) + }) + .child(keycaps(spec)) + })) + .into_any_element() }; let action_for_click = action.clone();