diff --git a/src/main.rs b/src/main.rs index fc9c37d1..98db58e5 100644 --- a/src/main.rs +++ b/src/main.rs @@ -84,65 +84,78 @@ fn spawn_config_watcher(cx: &mut App) { while rx.try_recv().is_ok() {} cx.update(|cx| { - let (config, outcome) = Config::load_with_outcome(); - if outcome.failed() { - // Keep the settings the app is running on: swapping the - // stand-in defaults in would flash the whole UI onto - // defaults, and the load already parked the broken file - // beside the original. It reloads itself the moment the - // file parses again. - if !announced { - announced = true; - notify_config_load_failed(cx, outcome, false); - } - // The theme files this same watcher covers must keep - // hot-reloading: a typo in config.json is no reason for - // theme editing to go dead until the app restarts. They - // read the global config, which is deliberately still the - // one the app is running on. - reload_themes(cx); - cx.refresh_windows(); - return; - } - announced = false; - // The keymap is rebuilt only when a binding actually moved. - // This watcher fires for every write under the config dir — - // including the app's own `save()`, which a sidebar drag or a - // palette open triggers — so reloading bindings on each tick - // would rebuild the whole keymap for nothing, and (before - // `rebind` cleared first) leak a full table per tick (#548). - // Read past the early return above: a load that failed leaves - // the global alone, so there is nothing to compare and the - // user's keys stay in the keymap the app is dispatching on. - let keymap_before = crate::ui::keymap::keybinding_config(cx); - crate::ui::i18n::set_locale(&config.gui_language); - cx.set_global(config); - reload_themes(cx); - crate::ui::theme::apply_cursor_hide_mode(cx); - // The menu bar is built once from the current locale, so editing - // gui_language by hand has to rebuild it the same way the - // in-app language picker does. - crate::ui::theme::set_menus(cx); - crate::ui::windows::WindowRegistry::refresh_locale(cx, None); - // `custom_shells` is only ever hand-edited, so this file is the - // one place it can change from — and the inventory that carries - // it to the new-tab menu is cached per window. - crate::ui::windows::WindowRegistry::refresh_shells(cx); - // A hand-edited keybinding shows up in the settings list off the - // live global immediately; without this it never reaches the - // keymap gpui actually dispatches against, so the key looks - // bound and does nothing until restart. Gated on the triple so - // an unrelated save does not churn it. - if crate::ui::keymap::keybinding_config(cx) != keymap_before { - crate::ui::keymap::rebind(cx); - } - cx.refresh_windows(); + apply_reloaded_config(cx, Config::load_with_outcome(), &mut announced); }); } }) .detach(); } +/// One watcher tick, once the debounce is out: what a reload does to the +/// running app. +/// +/// `announced` is the one-toast-per-breakage latch, which lives across ticks +/// in the watcher. Returns whether the keymap was rebuilt — the watcher has no +/// use for that; it is how a test asks which branch ran. +fn apply_reloaded_config( + cx: &mut App, + load: (Config, crate::core::config::LoadOutcome), + announced: &mut bool, +) -> bool { + let (config, outcome) = load; + if outcome.failed() { + // Keep the settings the app is running on: swapping the stand-in + // defaults in would flash the whole UI onto defaults, and the load + // already parked the broken file beside the original. It reloads + // itself the moment the file parses again. + if !*announced { + *announced = true; + notify_config_load_failed(cx, outcome, false); + } + // The theme files this same watcher covers must keep hot-reloading: a + // typo in config.json is no reason for theme editing to go dead until + // the app restarts. They read the global config, which is deliberately + // still the one the app is running on. + reload_themes(cx); + cx.refresh_windows(); + return false; + } + *announced = false; + // The keymap is rebuilt only when a binding actually moved. This watcher + // fires for every write under the config dir — including the app's own + // `save()`, which a sidebar drag or a palette open triggers — so reloading + // bindings on each tick would rebuild the whole keymap for nothing, and + // (before `rebind` cleared first) leak a full table per tick (#548). + // Read past the early return above: a load that failed leaves the global + // alone, so there is nothing to compare and the user's keys stay in the + // keymap the app is dispatching on. + let keymap_before = crate::ui::keymap::keybinding_config(cx); + crate::ui::i18n::set_locale(&config.gui_language); + cx.set_global(config); + reload_themes(cx); + crate::ui::theme::apply_cursor_hide_mode(cx); + // The menu bar is built once from the current locale, so editing + // gui_language by hand has to rebuild it the same way the in-app language + // picker does. + crate::ui::theme::set_menus(cx); + crate::ui::windows::WindowRegistry::refresh_locale(cx, None); + // `custom_shells` is only ever hand-edited, so this file is the one place + // it can change from — and the inventory that carries it to the new-tab + // menu is cached per window. + crate::ui::windows::WindowRegistry::refresh_shells(cx); + // A hand-edited keybinding shows up in the settings list off the live + // global immediately; without this it never reaches the keymap gpui + // actually dispatches against, so the key looks bound and does nothing + // until restart. Gated on the triple so an unrelated save does not churn + // it. + let rebound = crate::ui::keymap::keybinding_config(cx) != keymap_before; + if rebound { + crate::ui::keymap::rebind(cx); + } + cx.refresh_windows(); + rebound +} + fn is_theme_file(p: &std::path::Path) -> bool { p.parent().and_then(|d| d.file_name()) == Some(std::ffi::OsStr::new("themes")) && p.extension().and_then(|e| e.to_str()).is_some_and(|e| { @@ -556,6 +569,176 @@ fn main() { }); } +/// The watcher tick, from a reloaded file to the keys the app dispatches on. +/// +/// The one thing #548 is for — hand-editing config.json and having the new +/// chord fire without a restart — crosses a file watcher, a debounce and a +/// keymap rebuild, and used to be covered nowhere. These drive +/// `apply_reloaded_config` directly, which is the whole body of the watcher's +/// tick, so the reload path is exercised without waiting on the filesystem. +#[cfg(test)] +mod config_reload_tests { + use super::{apply_reloaded_config, is_theme_file}; + use crate::core::actions::SplitRight; + use crate::core::config::{Config, LoadOutcome}; + use gpui::{Action as _, App, KeyContext, Keystroke, TestAppContext}; + + /// What the live keymap — the one gpui dispatches against — does with + /// `keys` typed in a terminal. + fn dispatched(cx: &App, keys: &str) -> Vec<&'static str> { + let typed = [Keystroke::parse(keys).expect("the typed keystroke parses")]; + let context = [KeyContext::parse("Terminal").expect("the context parses")]; + cx.key_bindings() + .borrow() + .bindings_for_input(&typed, &context) + .0 + .iter() + .map(|b| b.action().name()) + .collect() + } + + /// An app running on `config`, as far as the config watcher can tell: + /// the globals its tick reads, and a keymap built from that config. + fn running_on(cx: &mut App, config: Config) { + let dir = std::env::temp_dir().join(format!("tty7-reload-{}", std::process::id())); + std::fs::create_dir_all(&dir).ok(); + crate::core::config::set_config_dir(dir); + + gpui_component::init(cx); + cx.set_global(config); + crate::ui::windows::WindowRegistry::init(cx); + crate::ui::presets::load_registry(cx); + crate::ui::keymap::init(cx); + } + + fn bound_to_split_right(config: &mut Config, key: &str) { + config + .keybindings + .insert("SplitRight".to_string(), key.to_string()); + } + + #[gpui::test] + fn a_hand_edited_binding_fires_without_a_restart(cx: &mut TestAppContext) { + cx.update(|cx| { + running_on(cx, Config::default()); + assert!( + dispatched(cx, "ctrl-alt-9").is_empty(), + "nothing is on the chord before the edit" + ); + + let mut edited = Config::default(); + bound_to_split_right(&mut edited, "ctrl-alt-9"); + let mut announced = false; + assert!( + apply_reloaded_config(cx, (edited, LoadOutcome::Parsed), &mut announced), + "a moved binding rebuilds the keymap" + ); + + assert_eq!( + dispatched(cx, "ctrl-alt-9"), + vec![SplitRight::name_for_type()], + "the hand-edited chord reaches the keymap gpui dispatches on" + ); + }); + } + + #[gpui::test] + fn a_save_that_moves_no_binding_leaves_the_keymap_alone(cx: &mut TestAppContext) { + cx.update(|cx| { + running_on(cx, Config::default()); + + // The app's own `save()` fires this watcher on every sidebar drag. + let mut unrelated = Config::default(); + unrelated.dim_inactive_panes = !unrelated.dim_inactive_panes; + let mut announced = false; + assert!( + !apply_reloaded_config( + cx, + (unrelated.clone(), LoadOutcome::Parsed), + &mut announced + ), + "an unrelated save must not rebuild the keymap" + ); + assert_eq!( + cx.global::().dim_inactive_panes, + unrelated.dim_inactive_panes, + "the reload still took effect; only the rebind was skipped" + ); + }); + } + + #[gpui::test] + fn a_broken_config_does_not_take_the_users_keys_away(cx: &mut TestAppContext) { + cx.update(|cx| { + let mut user = Config::default(); + bound_to_split_right(&mut user, "ctrl-alt-9"); + running_on(cx, user); + assert_eq!( + dispatched(cx, "ctrl-alt-9"), + vec![SplitRight::name_for_type()] + ); + + // A quarantined load hands back stand-in defaults, and the global + // is deliberately left alone — so nothing may rebind off them. + let mut announced = false; + assert!( + !apply_reloaded_config( + cx, + (Config::default(), LoadOutcome::Quarantined), + &mut announced + ), + "a load that failed has nothing to compare and must not rebind" + ); + assert!(announced, "the breakage is announced"); + assert_eq!( + cx.global::() + .keybindings + .get("SplitRight") + .map(String::as_str), + Some("ctrl-alt-9"), + "the running config survives the broken file" + ); + assert_eq!( + dispatched(cx, "ctrl-alt-9"), + vec![SplitRight::name_for_type()], + "and so do the keys the app is dispatching on" + ); + + // The file parses again: the latch clears, so a second breakage + // can speak up. + let mut fixed = Config::default(); + bound_to_split_right(&mut fixed, "ctrl-alt-8"); + assert!(apply_reloaded_config( + cx, + (fixed, LoadOutcome::Parsed), + &mut announced + )); + assert!(!announced); + assert_eq!( + dispatched(cx, "ctrl-alt-8"), + vec![SplitRight::name_for_type()] + ); + assert!( + dispatched(cx, "ctrl-alt-9").is_empty(), + "the chord the fixed file dropped is retired" + ); + }); + } + + /// The watcher covers the whole config directory, and the themes half of + /// it has to keep reloading even when config.json does not parse. + #[test] + fn only_theme_files_beside_the_config_count_as_themes() { + use std::path::Path; + assert!(is_theme_file(Path::new("/cfg/themes/solar.yaml"))); + assert!(is_theme_file(Path::new("/cfg/themes/solar.YML"))); + assert!(is_theme_file(Path::new("/cfg/themes/solar.itermcolors"))); + assert!(!is_theme_file(Path::new("/cfg/themes/notes.txt"))); + assert!(!is_theme_file(Path::new("/cfg/config.json"))); + assert!(!is_theme_file(Path::new("/cfg/solar.yaml"))); + } +} + #[cfg(all(test, unix))] mod tests { use super::merge_paths; diff --git a/src/ui/keymap.rs b/src/ui/keymap.rs index 90b7ad3d..a514fe95 100644 --- a/src/ui/keymap.rs +++ b/src/ui/keymap.rs @@ -183,22 +183,6 @@ fn action_bindings(effective: &[(String, String)]) -> Vec { bindings } -/// The keystroke and context of every binding `action_bindings` installs. -/// The rebind used to keep this in a global to retire the previous set one -/// `NoAction` at a time; a rebuild replaces the whole map instead, so this is -/// now only the tests' way of asking what a config would install. -#[cfg(test)] -fn bound_keystrokes(effective: &[(String, String)]) -> Vec<(String, Option<&'static str>)> { - let extras = extra_keystrokes(effective); - effective - .iter() - .map(|(a, k)| (a.as_str(), k.as_str())) - .chain(extras.iter().map(|(a, k)| (*a, *k))) - .filter(|(_, k)| !k.is_empty() && keystroke_is_valid(k)) - .map(|(a, k)| (k.to_string(), action_context(a))) - .collect() -} - fn per_platform(mac: &'static str, other: &'static str) -> &'static str { if cfg!(target_os = "macos") { mac @@ -1057,6 +1041,30 @@ fn make_binding(action: &str, keystroke: &str) -> Option { #[cfg(test)] mod tests { use super::*; + use gpui::Action as _; + + /// The actions a keymap built from `action_bindings` dispatches for `keys` + /// typed in `context`, in precedence order — the same lookup gpui performs + /// on a real keypress. + /// + /// Bindings are asserted through this rather than through a mirror of the + /// table, so a chord the app would not really install, or would install in + /// another context, cannot pass. + fn dispatched(effective: &[(String, String)], keys: &str, context: &str) -> Vec<&'static str> { + let mut keymap = gpui::Keymap::default(); + keymap.add_bindings(action_bindings(effective)); + let input: Vec = keys + .split(' ') + .map(|k| Keystroke::parse(k).expect("the typed keystroke parses")) + .collect(); + let context = [gpui::KeyContext::parse(context).expect("the context parses")]; + keymap + .bindings_for_input(&input, &context) + .0 + .iter() + .map(|b| b.action().name()) + .collect() + } #[test] fn every_dispatchable_action_has_a_slot_to_bind_it_in() { @@ -1226,10 +1234,9 @@ mod tests { assert!(extra_keystrokes(&effective).contains(&("InsertNewline", "alt-enter"))); for key in ["shift-enter", "alt-enter"] { assert!(keystroke_is_valid(key), "{key} does not parse"); - assert!(make_binding("InsertNewline", key).is_some()); assert!( - bound_keystrokes(&effective).iter().any(|(k, _)| k == key), - "{key} is not remembered as installed" + dispatched(&effective, key, "Terminal").contains(&InsertNewline::name_for_type()), + "{key} does not reach InsertNewline in a terminal" ); } assert_eq!(key_tokens("shift-enter"), vec![SHIFT, "⏎"]); @@ -1313,10 +1320,12 @@ mod tests { assert!(extra_keystrokes(&effective).contains(&("PasteText", "shift-insert"))); for key in ["ctrl-shift-v", "shift-insert"] { assert!( - bound_keystrokes(&effective) - .iter() - .any(|(k, c)| k == key && *c == Some("Terminal")), - "{key} must be installed in the Terminal context and remembered for rebind" + dispatched(&effective, key, "Terminal").contains(&PasteText::name_for_type()), + "{key} must paste in a terminal" + ); + assert!( + !dispatched(&effective, key, "Workspace").contains(&PasteText::name_for_type()), + "{key} is a terminal chord and must not paste outside one" ); } let rebound = vec![("PasteText".to_string(), "ctrl-alt-v".to_string())]; @@ -1333,9 +1342,16 @@ mod tests { let effective = vec![("InsertNewline".to_string(), "ctrl-o".to_string())]; assert!(extra_keystrokes(&effective).is_empty()); assert_eq!( - bound_keystrokes(&effective), - vec![("ctrl-o".to_string(), Some("Terminal"))] + dispatched(&effective, "ctrl-o", "Terminal"), + vec![InsertNewline::name_for_type()], + "the chord the config asks for is the one the keymap dispatches" ); + for retired in ["shift-enter", "alt-enter"] { + assert!( + dispatched(&effective, retired, "Terminal").is_empty(), + "{retired} is off the action now and must dispatch nothing" + ); + } let unbound = vec![("InsertNewline".to_string(), String::new())]; assert!(extra_keystrokes(&unbound).is_empty()); @@ -1343,19 +1359,36 @@ mod tests { } #[test] - fn bound_keystrokes_remember_the_context_each_binding_was_installed_in() { + fn each_binding_lands_in_the_context_its_action_is_scoped_to() { let effective = vec![ ("InsertNewline".to_string(), "shift-enter".to_string()), ("NewTab".to_string(), "secondary-t".to_string()), ]; + // The newline is a terminal chord, and its default ships the fallback + // beside it; both are scoped, so neither reaches the rest of the app. + let mut newline = dispatched(&effective, "shift-enter", "Terminal"); + newline.sort_unstable(); assert_eq!( - bound_keystrokes(&effective), + newline, vec![ - ("shift-enter".to_string(), Some("Terminal")), - ("secondary-t".to_string(), None), - ("alt-enter".to_string(), Some("Terminal")), + InsertNewline::name_for_type(), + InsertNewlineFallback::name_for_type(), ] ); + assert!(dispatched(&effective, "shift-enter", "Workspace").is_empty()); + assert_eq!( + dispatched(&effective, "alt-enter", "Terminal"), + vec![InsertNewline::name_for_type()], + "the extra chord follows its action's scope" + ); + // A window action has no context: it has to work from a terminal too. + for context in ["Terminal", "Workspace"] { + assert_eq!( + dispatched(&effective, "secondary-t", context), + vec![NewTab::name_for_type()], + "NewTab must not be scoped to {context}" + ); + } } #[test]