From 67adf9d2bdcb07aa4fb4a2f0fd51e6ccc337e2f8 Mon Sep 17 00:00:00 2001 From: simota Date: Fri, 17 Jul 2026 18:54:30 +0900 Subject: [PATCH 1/2] fix(app): rebind sidebar-hotkey as an in-app keybind, not a global hotkey MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit sidebar-hotkey was registered via Carbon RegisterEventHotKey, grabbing its chord (default cmd+shift+s, i.e. "Save As…") system-wide from every other application. Unlike the quick terminal, the sidebar toggle has no reason to fire while noa is unfocused. Feed the value into KeybindEngine::from_config as an in-app rebind of ToggleSidebar instead, with one coherent contract across the engine and the native menu: - Explicit `keybind` entries still win over the rebind; the empty sentinel (`none`) keeps the default chord (it only ever disabled the old global registration). Config reload rebuilds the engine. - KeyTrigger::parse accepts the multi-char token aliases the global-hotkey syntax already allowed (semicolon, backslash, tab, space, escape, JIS yen/ro, punctuation names), so pre-existing config values keep replacing the default chord. - Dedup and conflict checks share the runtime match relation (KeyTrigger::overlaps over the KeyToken::alternates table — [/{, ]/}, and backslash's JIS variants \/¥/_), so a chord can never match a keypress the checks treated as unrelated. - Chords already serving another command (e.g. cmd+t) are rejected with a diagnostic: the fixed menu accelerators mirror those defaults and AppKit dispatches them before winit, so the other command's menu item would win anyway. cmd+, (Preferences, menu-only) is reserved. - The Sidebar menu item's accelerator tracks the engine's *effective* ToggleSidebar chord (chord_for) and resyncs on any engine rebuild; an unbound/unrepresentable chord clears the native key equivalent explicitly (muda 0.19's set_accelerator(None) never touches the NSMenuItem). Drop the now-unused HotkeyAction::Sidebar / UserEvent::ToggleSidebar plumbing; only quick-terminal-hotkey remains a global hotkey. --- crates/noa-app/src/app.rs | 8 +- crates/noa-app/src/app/config.rs | 7 +- crates/noa-app/src/app/config_reload.rs | 24 +- crates/noa-app/src/app/event_loop.rs | 1 - crates/noa-app/src/app/lifecycle.rs | 3 + crates/noa-app/src/app/quick_terminal.rs | 22 +- crates/noa-app/src/cli.rs | 5 +- crates/noa-app/src/commands/key_token.rs | 84 ++++++- crates/noa-app/src/commands/keybind.rs | 271 ++++++++++++++++++++++- crates/noa-app/src/commands/tests.rs | 29 +-- crates/noa-app/src/events.rs | 4 - crates/noa-app/src/macos_hotkey.rs | 3 - crates/noa-app/src/macos_menu.rs | 72 +++++- crates/noa-config/src/lib.rs | 11 +- 14 files changed, 464 insertions(+), 80 deletions(-) diff --git a/crates/noa-app/src/app.rs b/crates/noa-app/src/app.rs index dcfffa59..d147fb50 100644 --- a/crates/noa-app/src/app.rs +++ b/crates/noa-app/src/app.rs @@ -328,10 +328,6 @@ pub struct App { /// seeded from `sidebar-enabled`. A quick-terminal window never shows it /// regardless (`window_sidebar_eligible`, FR-14). sidebar_visible_groups: HashSet, - /// The registered global `sidebar-hotkey`, kept alive for the app's - /// lifetime (dropping it unregisters). `None` until installed, or when no - /// `sidebar-hotkey` is configured / registration failed. - sidebar_hotkey: Option, /// The dedicated branch-poll worker (FR-8/FR-9): receives OSC-7-driven /// cwd-change requests and posts back git branch + project icon as /// [`crate::session_store::SessionDelta::Branch`]. Kept off the io read loop @@ -588,7 +584,8 @@ impl App { let initial_cursor_style = resolve_cursor_style(config.cursor_style, config.cursor_style_blink); let background_image = load_background_image_runtime(&config); - let (keybinds, keybind_diagnostics) = KeybindEngine::from_config(&config.keybinds); + let (keybinds, keybind_diagnostics) = + KeybindEngine::from_config(&config.keybinds, config.sidebar_hotkey.as_deref()); for diagnostic in keybind_diagnostics { log::warn!("config keybind: {diagnostic}"); } @@ -690,7 +687,6 @@ impl App { hotkey_install_attempted: false, secure_input: crate::secure_input::SecureInput::new(), session_store: SessionStore::new(), - sidebar_hotkey: None, branch_poll: Some(crate::branch_poll::spawn(proxy_for_branch_poll)), session_persister: crate::session_persist::SessionPersister::spawn(), sidebar_visible_gate, diff --git a/crates/noa-app/src/app/config.rs b/crates/noa-app/src/app/config.rs index b76be7bb..16f0af1a 100644 --- a/crates/noa-app/src/app/config.rs +++ b/crates/noa-app/src/app/config.rs @@ -151,9 +151,10 @@ pub struct AppConfig { /// `sidebar-font-size`: the session sidebar's own font size in points, /// independent of the terminal grid's `font-size`. pub sidebar_font_size: f32, - /// `sidebar-hotkey`: the global chord that toggles the sidebar for the - /// focused window (FR-13). `None` (or the empty-string "disabled" sentinel) - /// registers no chord. + /// `sidebar-hotkey`: the in-app chord that toggles the sidebar for the + /// focused window — a `ToggleSidebar` rebind fed to + /// `KeybindEngine::from_config`, not a system-wide hotkey. `None` (or the + /// empty-string sentinel) keeps the default `cmd+shift+s` binding. pub sidebar_hotkey: Option, /// `sidebar-preview-lines`: number of trailing output rows shown in each /// sidebar card. `0` disables the preview rows. diff --git a/crates/noa-app/src/app/config_reload.rs b/crates/noa-app/src/app/config_reload.rs index aae10c6a..f303f3fd 100644 --- a/crates/noa-app/src/app/config_reload.rs +++ b/crates/noa-app/src/app/config_reload.rs @@ -185,12 +185,14 @@ impl App { let sidebar_preview_changed = previous.sidebar_preview_lines != applied.sidebar_preview_lines; let sidebar_font_size_changed = previous.sidebar_font_size != applied.sidebar_font_size; - let keybinds_changed = previous.keybinds != applied.keybinds; + let sidebar_hotkey_changed = previous.sidebar_hotkey != applied.sidebar_hotkey; + // `sidebar-hotkey` feeds the keybind engine (in-app chord), so a + // change to either input rebuilds it. + let keybinds_changed = previous.keybinds != applied.keybinds || sidebar_hotkey_changed; let server_restart = decide_server_restart(&previous, &applied); let quick_terminal_hotkey_changed = previous.quick_terminal_hotkey != applied.quick_terminal_hotkey; - let sidebar_hotkey_changed = previous.sidebar_hotkey != applied.sidebar_hotkey; - let hotkeys_changed = quick_terminal_hotkey_changed || sidebar_hotkey_changed; + let hotkeys_changed = quick_terminal_hotkey_changed; self.config = applied; @@ -240,7 +242,10 @@ impl App { self.restart_ipc_server(); } if keybinds_changed { - let (keybinds, diagnostics) = KeybindEngine::from_config(&self.config.keybinds); + let (keybinds, diagnostics) = KeybindEngine::from_config( + &self.config.keybinds, + self.config.sidebar_hotkey.as_deref(), + ); for diagnostic in diagnostics { log::warn!("config reload keybind: {diagnostic}"); } @@ -253,10 +258,19 @@ impl App { if quick_terminal_hotkey_changed && let Some(menu) = self.macos_menu.as_ref() { menu.set_quick_terminal_hotkey(self.config.quick_terminal_hotkey.as_deref()); } + // Any engine rebuild can move the effective ToggleSidebar chord + // (sidebar-hotkey or an explicit keybind), so resync the menu + // accelerator from the engine, not from the raw config value. + if keybinds_changed && let Some(menu) = self.macos_menu.as_ref() { + menu.set_sidebar_chord( + self.keybinds + .chord_for(crate::commands::AppCommand::ToggleSidebar) + .as_deref(), + ); + } } if hotkeys_changed { self.quick_terminal_hotkey = None; - self.sidebar_hotkey = None; self.hotkey_install_attempted = false; } diff --git a/crates/noa-app/src/app/event_loop.rs b/crates/noa-app/src/app/event_loop.rs index 24d87107..e9d14c1e 100644 --- a/crates/noa-app/src/app/event_loop.rs +++ b/crates/noa-app/src/app/event_loop.rs @@ -53,7 +53,6 @@ impl ApplicationHandler for App { self.handle_app_command(event_loop, command, CommandOrigin::App) } UserEvent::ToggleQuickTerminal => self.toggle_quick_terminal(event_loop), - UserEvent::ToggleSidebar => self.toggle_sidebar(), UserEvent::SessionDelta(delta) => { if let crate::session_store::SessionDelta::Bell { id } = &delta { let window_id = WindowId::from(id.window_id.0); diff --git a/crates/noa-app/src/app/lifecycle.rs b/crates/noa-app/src/app/lifecycle.rs index b145a651..cf4fb2c4 100644 --- a/crates/noa-app/src/app/lifecycle.rs +++ b/crates/noa-app/src/app/lifecycle.rs @@ -1418,6 +1418,9 @@ impl App { crate::macos_menu::MacosMenu::install( self.proxy.clone(), self.config.quick_terminal_hotkey.as_deref(), + self.keybinds + .chord_for(crate::commands::AppCommand::ToggleSidebar) + .as_deref(), ) .expect("failed to install macOS app menu"), ); diff --git a/crates/noa-app/src/app/quick_terminal.rs b/crates/noa-app/src/app/quick_terminal.rs index 839dcf71..98d8418a 100644 --- a/crates/noa-app/src/app/quick_terminal.rs +++ b/crates/noa-app/src/app/quick_terminal.rs @@ -353,10 +353,11 @@ impl App { }) } - /// Register the global `quick-terminal-hotkey` and `sidebar-hotkey` once, - /// after the app is running. A no-op per chord when unset or explicitly - /// disabled; a registration failure is logged, not fatal. Both go through - /// the same `parse_hotkey` path (FR-13). + /// Register the global `quick-terminal-hotkey` once, after the app is + /// running. A no-op when unset or explicitly disabled; a registration + /// failure is logged, not fatal. (`sidebar-hotkey` is deliberately not + /// registered here — it is an in-app keybind, applied via + /// `KeybindEngine::from_config`.) pub(super) fn install_global_hotkey_if_needed(&mut self) { if self.hotkey_install_attempted { return; @@ -376,19 +377,6 @@ impl App { None => log::warn!("failed to register quick-terminal-hotkey `{spec}`"), } } - - if let Some(spec) = self.config.sidebar_hotkey.clone() - && !spec.trim().is_empty() - { - match crate::macos_hotkey::GlobalHotKey::register( - &spec, - self.proxy.clone(), - crate::macos_hotkey::HotkeyAction::Sidebar, - ) { - Some(hotkey) => self.sidebar_hotkey = Some(hotkey), - None => log::warn!("failed to register sidebar-hotkey `{spec}`"), - } - } } /// Toggle the quick terminal: reveal it (creating its window on first use) diff --git a/crates/noa-app/src/cli.rs b/crates/noa-app/src/cli.rs index 405368e0..7050c701 100644 --- a/crates/noa-app/src/cli.rs +++ b/crates/noa-app/src/cli.rs @@ -109,7 +109,8 @@ pub fn run_action(action: CliAction) -> anyhow::Result<()> { for diagnostic in diagnostics { eprintln!("{}", diagnostic.message); } - let (keybinds, diagnostics) = KeybindEngine::from_config(&config.keybinds); + let (keybinds, diagnostics) = + KeybindEngine::from_config(&config.keybinds, config.sidebar_hotkey.as_deref()); for diagnostic in diagnostics { eprintln!("config keybind: {diagnostic}"); } @@ -687,7 +688,7 @@ mod tests { #[test] fn list_keybinds_output_uses_the_supplied_effective_engine() { - let (keybinds, diagnostics) = KeybindEngine::from_config(&[ + let (keybinds, diagnostics) = KeybindEngine::from_config_test(&[ noa_config::KeybindConfig::Unbind { trigger: "cmd+t".to_string(), }, diff --git a/crates/noa-app/src/commands/key_token.rs b/crates/noa-app/src/commands/key_token.rs index 392a313a..ea61f1df 100644 --- a/crates/noa-app/src/commands/key_token.rs +++ b/crates/noa-app/src/commands/key_token.rs @@ -38,6 +38,15 @@ impl KeyTrigger { pub(super) fn matches(&self, logical_key: &Key, mods: ModifiersState) -> bool { self.mods.matches(mods) && self.key.matches(logical_key) } + + /// Whether two triggers can match the same keypress. Strictly wider than + /// `==`: character tokens carry alternate logical candidates (`[`/`{`, + /// `]`/`}`, `\`/`¥`/`_` — see [`KeyToken::candidates`]), so dedup and + /// conflict checks must use this relation — structural equality would + /// leave a shadowing binding alive that runtime matching still hits. + pub(super) fn overlaps(&self, other: &Self) -> bool { + self.mods == other.mods && self.key.overlaps(other.key) + } } impl std::fmt::Display for KeyTrigger { @@ -86,11 +95,27 @@ enum KeyToken { impl KeyToken { fn parse(token: &str) -> Result { - if token == "plus" { - return Ok(Self::Character('+')); - } - if token == "grave" || token == "backtick" { - return Ok(Self::Character('`')); + // Multi-char aliases shared with the global-hotkey chord syntax + // (`macos_hotkey::parse_hotkey`), so a chord accepted there — e.g. a + // pre-existing `sidebar-hotkey` value — parses identically here. + let aliased = match token { + "plus" => Some('+'), + "grave" | "backtick" => Some('`'), + "equal" => Some('='), + "minus" => Some('-'), + "leftbracket" => Some('['), + "rightbracket" => Some(']'), + "semicolon" => Some(';'), + "backslash" => Some('\\'), + "comma" => Some(','), + "slash" => Some('/'), + "period" => Some('.'), + "yen" | "jis-yen" | "jis_yen" | "intl-yen" | "intl_yen" => Some('¥'), + "underscore" | "jis-underscore" | "jis_underscore" | "intl-ro" | "intl_ro" => Some('_'), + _ => None, + }; + if let Some(ch) = aliased { + return Ok(Self::Character(ch)); } let mut chars = token.chars(); if let (Some(ch), None) = (chars.next(), chars.next()) { @@ -106,23 +131,59 @@ impl KeyToken { "home" => NamedKeyToken::Home, "end" => NamedKeyToken::End, "enter" | "return" => NamedKeyToken::Enter, + "tab" => NamedKeyToken::Tab, + "space" => NamedKeyToken::Space, + "escape" | "esc" => NamedKeyToken::Escape, _ => return Err(KeybindParseError::UnknownKey(token.to_string())), })) } + /// The alternate logical-key characters `expected` matches beyond its + /// own (case-insensitive) character. The single source of truth for + /// character equivalence — runtime matching ([`Self::matches`]) and + /// trigger overlap ([`Self::overlaps`]) both derive from it, so a chord + /// can never match a keypress that dedup/conflict checks treated as + /// unrelated. `\` covers the JIS variants the retired global hotkey + /// registered physically (ANSI Backslash → `\`, JIS Yen → `¥`, JIS Ro → + /// `_` — see `macos_hotkey::carbon_keycodes`). + fn alternates(expected: char) -> &'static [char] { + match expected { + '[' => &['{'], + ']' => &['}'], + '\\' => &['¥', '_'], + _ => &[], + } + } + fn matches(self, logical_key: &Key) -> bool { match (self, logical_key) { (Self::Character(expected), Key::Character(actual)) => { actual.chars().next().is_some_and(|actual| { actual.eq_ignore_ascii_case(&expected) - || (expected == '[' && actual == '{') - || (expected == ']' && actual == '}') + || Self::alternates(expected).contains(&actual) }) } (Self::Named(expected), Key::Named(actual)) => expected.matches(*actual), _ => false, } } + + /// Whether the two tokens' match sets intersect: equal characters, one + /// character being an alternate of the other, or a shared alternate. + fn overlaps(self, other: Self) -> bool { + match (self, other) { + (Self::Character(a), Self::Character(b)) => { + a.eq_ignore_ascii_case(&b) + || Self::alternates(a).contains(&b) + || Self::alternates(b).contains(&a) + || Self::alternates(a) + .iter() + .any(|ka| Self::alternates(b).contains(ka)) + } + (Self::Named(a), Self::Named(b)) => a == b, + _ => false, + } + } } impl std::fmt::Display for KeyToken { @@ -149,6 +210,9 @@ enum NamedKeyToken { Home, End, Enter, + Tab, + Space, + Escape, } impl NamedKeyToken { @@ -165,6 +229,9 @@ impl NamedKeyToken { Self::Home => "home", Self::End => "end", Self::Enter => "enter", + Self::Tab => "tab", + Self::Space => "space", + Self::Escape => "escape", } } @@ -180,6 +247,9 @@ impl NamedKeyToken { | (Self::Home, NamedKey::Home) | (Self::End, NamedKey::End) | (Self::Enter, NamedKey::Enter) + | (Self::Tab, NamedKey::Tab) + | (Self::Space, NamedKey::Space) + | (Self::Escape, NamedKey::Escape) ) } } diff --git a/crates/noa-app/src/commands/keybind.rs b/crates/noa-app/src/commands/keybind.rs index 3cdc7f8f..78158c39 100644 --- a/crates/noa-app/src/commands/keybind.rs +++ b/crates/noa-app/src/commands/keybind.rs @@ -172,10 +172,66 @@ impl Default for KeybindEngine { } impl KeybindEngine { - pub(crate) fn from_config(configs: &[KeybindConfig]) -> (Self, Vec) { + pub(crate) fn from_config( + configs: &[KeybindConfig], + sidebar_hotkey: Option<&str>, + ) -> (Self, Vec) { let mut engine = Self::default(); let mut diagnostics = Vec::new(); + // `sidebar-hotkey` rebinds `ToggleSidebar` as a plain in-app chord — + // it is deliberately NOT a system-wide hotkey (a global grab would + // steal the chord, e.g. ⇧⌘S "Save As…", from every other app). + // Applied before the `keybind` entries so explicit keybinds still + // win; the empty sentinel (`none`) keeps the default binding, since + // it only ever disabled the former global registration. + if let Some(spec) = sidebar_hotkey + && !spec.trim().is_empty() + { + match KeyTrigger::parse(spec) { + // Reject chords already serving another command — the fixed + // native-menu key equivalents mirror these defaults, and + // AppKit dispatches menu accelerators before winit sees the + // keypress, so silently stealing e.g. cmd+t would leave the + // "New Tab" menu item winning over the sidebar anyway. + // `cmd+,` is reserved menu-side only (Preferences has no + // engine binding). + Ok(trigger) => { + let conflict = engine + .bindings + .iter() + .find(|binding| { + binding.trigger.overlaps(&trigger) + && binding.command != AppCommand::ToggleSidebar + }) + .map(|binding| binding.command.action_name()) + .or_else(|| { + let preferences = + KeyTrigger::parse("cmd+,").expect("reserved chord should parse"); + trigger + .overlaps(&preferences) + .then_some("preferences (app menu)") + }); + if let Some(taken_by) = conflict { + diagnostics.push(format!( + "sidebar-hotkey `{spec}` conflicts with `{taken_by}`; value ignored" + )); + } else { + engine + .bindings + .retain(|binding| binding.command != AppCommand::ToggleSidebar); + engine.bindings.push(KeyBinding { + trigger, + command: AppCommand::ToggleSidebar, + }); + } + } + Err(error) => diagnostics.push(format!( + "invalid sidebar-hotkey `{spec}`: {error}; value ignored" + )), + } + } + for config in configs { match config { KeybindConfig::Clear => engine.bindings.clear(), @@ -213,6 +269,13 @@ impl KeybindEngine { (engine, diagnostics) } + /// [`Self::from_config`] without a `sidebar-hotkey` override, for tests + /// exercising only the `keybind` entries. + #[cfg(test)] + pub(crate) fn from_config_test(configs: &[KeybindConfig]) -> (Self, Vec) { + Self::from_config(configs, None) + } + pub(crate) fn resolve(&self, logical_key: &Key, mods: ModifiersState) -> Option { self.bindings .iter() @@ -243,8 +306,13 @@ impl KeybindEngine { .collect() } + /// Remove every binding whose trigger can match the same keypress as + /// `trigger` (`KeyTrigger::overlaps`, not `==`): resolve() returns the + /// first match, so leaving a merely structurally-different overlapping + /// binding alive would shadow whatever the caller binds next. fn remove_trigger(&mut self, trigger: &KeyTrigger) { - self.bindings.retain(|binding| binding.trigger != *trigger); + self.bindings + .retain(|binding| !binding.trigger.overlaps(trigger)); } } @@ -362,7 +430,7 @@ mod tests { #[test] fn configured_keybinds_override_unbind_and_clear_in_order() { - let (engine, diagnostics) = KeybindEngine::from_config(&[ + let (engine, diagnostics) = KeybindEngine::from_config_test(&[ KeybindConfig::Bind { trigger: "cmd+t".to_string(), action: "new_window".to_string(), @@ -390,7 +458,7 @@ mod tests { Some(AppCommand::SetTabTitle) ); - let (engine, diagnostics) = KeybindEngine::from_config(&[ + let (engine, diagnostics) = KeybindEngine::from_config_test(&[ KeybindConfig::Clear, KeybindConfig::Bind { trigger: "cmd+i".to_string(), @@ -410,7 +478,7 @@ mod tests { #[test] fn invalid_configured_keybinds_do_not_remove_existing_bindings() { - let (engine, diagnostics) = KeybindEngine::from_config(&[ + let (engine, diagnostics) = KeybindEngine::from_config_test(&[ KeybindConfig::Bind { trigger: "cmd+t".to_string(), action: "no.such.action".to_string(), @@ -434,7 +502,7 @@ mod tests { #[test] fn grave_aliases_bind_quick_terminal() { for trigger in ["cmd+grave", "cmd+backtick", "cmd+`"] { - let (engine, diagnostics) = KeybindEngine::from_config(&[ + let (engine, diagnostics) = KeybindEngine::from_config_test(&[ KeybindConfig::Clear, KeybindConfig::Bind { trigger: trigger.to_string(), @@ -479,7 +547,7 @@ mod tests { #[test] fn legacy_combined_overlay_action_names_alias_to_open_theme_picker() { for action in ["open_theme_settings", "theme-settings.open"] { - let (engine, diagnostics) = KeybindEngine::from_config(&[ + let (engine, diagnostics) = KeybindEngine::from_config_test(&[ KeybindConfig::Clear, KeybindConfig::Bind { trigger: "cmd+shift+t".to_string(), @@ -497,4 +565,193 @@ mod tests { ); } } + + // `sidebar-hotkey` is an in-app rebind of `ToggleSidebar`, not a global + // hotkey: the configured chord resolves, the default `cmd+shift+s` stops + // resolving, and explicit `keybind` entries still win over it. + #[test] + fn sidebar_hotkey_rebinds_toggle_sidebar() { + let (engine, diagnostics) = KeybindEngine::from_config(&[], Some("cmd+alt+b")); + assert!(diagnostics.is_empty(), "{diagnostics:?}"); + assert_eq!( + engine.resolve( + &Key::Character("b".into()), + ModifiersState::SUPER | ModifiersState::ALT + ), + Some(AppCommand::ToggleSidebar) + ); + assert_eq!( + engine.resolve( + &Key::Character("s".into()), + ModifiersState::SUPER | ModifiersState::SHIFT + ), + None + ); + + // Explicit keybind entries apply after (and thus override) the + // sidebar-hotkey rebind. + let (engine, diagnostics) = KeybindEngine::from_config( + &[KeybindConfig::Bind { + trigger: "cmd+alt+b".to_string(), + action: "window.new".to_string(), + }], + Some("cmd+alt+b"), + ); + assert!(diagnostics.is_empty(), "{diagnostics:?}"); + assert_eq!( + engine.resolve( + &Key::Character("b".into()), + ModifiersState::SUPER | ModifiersState::ALT + ), + Some(AppCommand::NewWindow) + ); + } + + // Chords written in the global-hotkey syntax (`macos_hotkey::parse_hotkey` + // aliases: semicolon/backslash/tab/space/escape/JIS keys …) stay valid + // now that sidebar-hotkey parses through KeyTrigger — a pre-existing + // config value must keep replacing the default chord. + #[test] + fn sidebar_hotkey_accepts_global_hotkey_chord_aliases() { + for spec in [ + "cmd+semicolon", + "cmd+backslash", + "cmd+tab", + "cmd+space", + "cmd+escape", + "cmd+jis-yen", + "cmd+intl-ro", + ] { + let (engine, diagnostics) = KeybindEngine::from_config(&[], Some(spec)); + assert!(diagnostics.is_empty(), "{spec}: {diagnostics:?}"); + // The default chord is replaced, and the engine reports an + // effective ToggleSidebar chord (what the menu accelerator uses). + assert_eq!( + engine.resolve( + &Key::Character("s".into()), + ModifiersState::SUPER | ModifiersState::SHIFT + ), + None, + "{spec}" + ); + assert!( + engine.chord_for(AppCommand::ToggleSidebar).is_some(), + "{spec}" + ); + } + + let (engine, diagnostics) = KeybindEngine::from_config(&[], Some("cmd+semicolon")); + assert!(diagnostics.is_empty(), "{diagnostics:?}"); + assert_eq!( + engine.resolve(&Key::Character(";".into()), ModifiersState::SUPER), + Some(AppCommand::ToggleSidebar) + ); + } + + // A sidebar-hotkey chord already serving another command is rejected + // (diagnosed, defaults kept): the fixed native-menu accelerators mirror + // those defaults and AppKit dispatches them before winit, so the other + // command's menu item would win over the sidebar anyway. + #[test] + fn sidebar_hotkey_conflicting_with_another_binding_is_rejected() { + for spec in ["cmd+t", "cmd+,"] { + let (engine, diagnostics) = KeybindEngine::from_config(&[], Some(spec)); + assert_eq!(diagnostics.len(), 1, "{spec}: {diagnostics:?}"); + assert!(diagnostics[0].contains("conflicts with"), "{diagnostics:?}"); + // Both the stolen chord's owner and the sidebar default survive. + assert_eq!( + engine.resolve( + &Key::Character("s".into()), + ModifiersState::SUPER | ModifiersState::SHIFT + ), + Some(AppCommand::ToggleSidebar), + "{spec}" + ); + } + let (engine, _) = KeybindEngine::from_config(&[], Some("cmd+t")); + assert_eq!( + engine.resolve(&Key::Character("t".into()), ModifiersState::SUPER), + Some(AppCommand::NewTab) + ); + } + + // `cmd+backslash` keeps matching every logical key the retired global + // hotkey covered physically: ANSI `\`, JIS Yen (`¥`), and JIS Ro (`_`). + #[test] + fn sidebar_hotkey_backslash_matches_jis_key_variants() { + let (engine, diagnostics) = KeybindEngine::from_config(&[], Some("cmd+backslash")); + assert!(diagnostics.is_empty(), "{diagnostics:?}"); + for logical in ["\\", "¥", "_"] { + assert_eq!( + engine.resolve(&Key::Character(logical.into()), ModifiersState::SUPER), + Some(AppCommand::ToggleSidebar), + "{logical}" + ); + } + } + + // Conflict detection and dedup use the runtime match relation + // (`KeyTrigger::overlaps`), not structural equality: a chord that only + // *matches* an existing default (cmd+shift+{ vs cmd+shift+[) is still a + // conflict, and a later explicit keybind on a JIS alternate (`cmd+jis-yen` + // vs an earlier `cmd+backslash`) still removes the binding it would + // otherwise shadow. + #[test] + fn trigger_overlap_governs_conflicts_and_overrides() { + // cmd+shift+{ would always lose to the default cmd+shift+[ (PrevTab) + // at resolve time, so it is rejected like a structural conflict. + for spec in ["cmd+shift+{", "cmd+shift+}"] { + let (engine, diagnostics) = KeybindEngine::from_config(&[], Some(spec)); + assert_eq!(diagnostics.len(), 1, "{spec}: {diagnostics:?}"); + assert!(diagnostics[0].contains("conflicts with"), "{diagnostics:?}"); + assert_eq!( + engine.resolve( + &Key::Character("s".into()), + ModifiersState::SUPER | ModifiersState::SHIFT + ), + Some(AppCommand::ToggleSidebar), + "{spec}" + ); + } + + // A later explicit keybind on cmd+jis-yen replaces the earlier + // overlapping cmd+backslash sidebar binding (explicit entries win). + let (engine, diagnostics) = KeybindEngine::from_config( + &[KeybindConfig::Bind { + trigger: "cmd+jis-yen".to_string(), + action: "window.new".to_string(), + }], + Some("cmd+backslash"), + ); + assert!(diagnostics.is_empty(), "{diagnostics:?}"); + assert_eq!( + engine.resolve(&Key::Character("¥".into()), ModifiersState::SUPER), + Some(AppCommand::NewWindow) + ); + // The whole overlapping backslash binding is gone (dedup and runtime + // matching share one equivalence — no partial physical-candidate + // split), so the ANSI key no longer resolves to the sidebar either. + assert_eq!( + engine.resolve(&Key::Character("\\".into()), ModifiersState::SUPER), + None + ); + } + + // The empty sentinel (`sidebar-hotkey = none`) keeps the default + // `cmd+shift+s` binding; an unparseable chord is diagnosed and ignored. + #[test] + fn sidebar_hotkey_empty_or_invalid_keeps_default_binding() { + for spec in ["", "cmd+not-a-key"] { + let (engine, diagnostics) = KeybindEngine::from_config(&[], Some(spec)); + assert_eq!(diagnostics.len(), usize::from(!spec.is_empty()), "{spec}"); + assert_eq!( + engine.resolve( + &Key::Character("s".into()), + ModifiersState::SUPER | ModifiersState::SHIFT + ), + Some(AppCommand::ToggleSidebar), + "{spec}" + ); + } + } } diff --git a/crates/noa-app/src/commands/tests.rs b/crates/noa-app/src/commands/tests.rs index 53a5f7d8..e9630d47 100644 --- a/crates/noa-app/src/commands/tests.rs +++ b/crates/noa-app/src/commands/tests.rs @@ -220,19 +220,22 @@ fn shift_arrows_map_to_directional_copy_mode_gestures() { #[test] fn copy_mode_actions_are_configurable_and_directional_defaults_are_rebindable() { - let (engine, diagnostics) = KeybindEngine::from_config(&[ - noa_config::KeybindConfig::Unbind { - trigger: "shift+arrowright".to_string(), - }, - noa_config::KeybindConfig::Bind { - trigger: "cmd+y".to_string(), - action: "copy_mode".to_string(), - }, - noa_config::KeybindConfig::Bind { - trigger: "cmd+shift+y".to_string(), - action: "copy_mode:right".to_string(), - }, - ]); + let (engine, diagnostics) = KeybindEngine::from_config( + &[ + noa_config::KeybindConfig::Unbind { + trigger: "shift+arrowright".to_string(), + }, + noa_config::KeybindConfig::Bind { + trigger: "cmd+y".to_string(), + action: "copy_mode".to_string(), + }, + noa_config::KeybindConfig::Bind { + trigger: "cmd+shift+y".to_string(), + action: "copy_mode:right".to_string(), + }, + ], + None, + ); assert!(diagnostics.is_empty(), "{diagnostics:?}"); assert_eq!( engine.resolve(&Key::Named(NamedKey::ArrowRight), ModifiersState::SHIFT), diff --git a/crates/noa-app/src/events.rs b/crates/noa-app/src/events.rs index 54918bd4..96cba576 100644 --- a/crates/noa-app/src/events.rs +++ b/crates/noa-app/src/events.rs @@ -38,10 +38,6 @@ pub enum UserEvent { /// handler thread via the [`winit::event_loop::EventLoopProxy`]). Toggles /// the drop-down quick terminal's visibility. ToggleQuickTerminal, - /// The global session-sidebar hotkey fired (same Carbon mechanism as - /// [`Self::ToggleQuickTerminal`]). Toggles the sidebar on the focused - /// window only (FR-4). - ToggleSidebar, /// A session-sidebar state delta posted by the io thread (last-output /// upsert, unread bell, …). The main thread — which owns the /// [`crate::session_store::SessionStore`] — applies it on receipt diff --git a/crates/noa-app/src/macos_hotkey.rs b/crates/noa-app/src/macos_hotkey.rs index 27883895..8977795f 100644 --- a/crates/noa-app/src/macos_hotkey.rs +++ b/crates/noa-app/src/macos_hotkey.rs @@ -160,7 +160,6 @@ pub(crate) struct GlobalHotKey { #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub(crate) enum HotkeyAction { QuickTerminal, - Sidebar, } impl HotkeyAction { @@ -168,14 +167,12 @@ impl HotkeyAction { fn id(self, slot: u32) -> u32 { (match self { HotkeyAction::QuickTerminal => 1_000, - HotkeyAction::Sidebar => 2_000, }) + slot } fn event(self) -> UserEvent { match self { HotkeyAction::QuickTerminal => UserEvent::ToggleQuickTerminal, - HotkeyAction::Sidebar => UserEvent::ToggleSidebar, } } } diff --git a/crates/noa-app/src/macos_menu.rs b/crates/noa-app/src/macos_menu.rs index 22a45f16..b1c4e105 100644 --- a/crates/noa-app/src/macos_menu.rs +++ b/crates/noa-app/src/macos_menu.rs @@ -2,7 +2,7 @@ use muda::{ CheckMenuItem, ContextMenu, Menu, MenuEvent, MenuItem, PredefinedMenuItem, Submenu, - accelerator::{Accelerator, Code, Modifiers}, + accelerator::{Accelerator, Code, Key as AcceleratorKey, KeyAccelerator, Modifiers}, }; use raw_window_handle::{HasWindowHandle, RawWindowHandle}; use winit::{dpi::PhysicalPosition, event_loop::EventLoopProxy, window::Window}; @@ -54,6 +54,9 @@ pub(crate) struct MacosMenu { split_context_auto_approve: CheckMenuItem, split_context_send_selection: MenuItem, quick_terminal: MenuItem, + /// The "Sidebar" item, retained so its accelerator can track the + /// `sidebar-hotkey` config (see [`MacosMenu::set_sidebar_hotkey`]). + sidebar: MenuItem, } #[derive(Clone, Copy, Debug, Default, PartialEq, Eq)] @@ -86,6 +89,7 @@ impl MacosMenu { pub(crate) fn install( proxy: EventLoopProxy, quick_terminal_hotkey: Option<&str>, + sidebar_chord: Option<&str>, ) -> anyhow::Result { let menu = Menu::new(); let split_context_menu = build_split_context_menu()?; @@ -132,6 +136,12 @@ impl MacosMenu { true, quick_terminal_accelerator(quick_terminal_hotkey), ); + let sidebar = MenuItem::with_id( + AppCommand::ToggleSidebar.menu_id(), + "Sidebar", + true, + sidebar_accelerator(sidebar_chord), + ); let auto_approve = CheckMenuItem::with_id( AppCommand::ToggleAutoApprove.menu_id(), auto_approve_menu_label(false), @@ -353,12 +363,7 @@ impl MacosMenu { ), &open_theme_picker, &quick_terminal, - &MenuItem::with_id( - AppCommand::ToggleSidebar.menu_id(), - "Sidebar", - true, - Some(cmd_shift_accelerator(Code::KeyS)), - ), + &sidebar, &auto_approve, &PredefinedMenuItem::separator(), &MenuItem::with_id( @@ -453,6 +458,7 @@ impl MacosMenu { secure_keyboard_entry, auto_approve, quick_terminal, + sidebar, }) } @@ -484,6 +490,30 @@ impl MacosMenu { } } + /// Reflect the keybind engine's *effective* `ToggleSidebar` chord + /// (`KeybindEngine::chord_for`) in the Sidebar menu item's shortcut + /// column. AppKit dispatches menu key equivalents before winit sees the + /// keypress, so the accelerator must track the engine — not the raw + /// `sidebar-hotkey` value — or a stale/overridden chord would keep + /// toggling the sidebar through menu dispatch. + pub(crate) fn set_sidebar_chord(&self, chord: Option<&str>) { + // muda 0.19's `set_accelerator(None)` never touches the native + // NSMenuItem (its macOS impl only writes when the accelerator is + // `Some`), so an unbound/unrepresentable chord must clear the key + // equivalent explicitly via an empty-key `KeyAccelerator` — otherwise + // the previous chord keeps firing through AppKit menu dispatch. + let result = match sidebar_accelerator(chord) { + Some(accelerator) => self.sidebar.set_accelerator(Some(accelerator)), + None => self.sidebar.set_key_accelerator(Some(KeyAccelerator::new( + None, + AcceleratorKey::Character(String::new()), + ))), + }; + if let Err(err) = result { + log::warn!("failed to update Sidebar menu accelerator: {err}"); + } + } + pub(crate) fn show_split_context_menu( &self, window: &Window, @@ -634,6 +664,16 @@ fn quick_terminal_accelerator(hotkey: Option<&str>) -> Option { hotkey.and_then(accelerator_from_hotkey) } +/// The Sidebar item's accelerator for the keybind engine's effective +/// `ToggleSidebar` chord (`KeybindEngine::chord_for`). `None` (unbound) +/// yields no accelerator; a chord the accelerator path can't represent also +/// yields none (the keybind engine still handles the keypress; only the +/// menu's shortcut column goes blank — never a chord the engine would route +/// elsewhere). +fn sidebar_accelerator(chord: Option<&str>) -> Option { + chord.and_then(accelerator_from_hotkey) +} + fn accelerator_from_hotkey(hotkey: &str) -> Option { let chord = crate::macos_hotkey::parse_hotkey(hotkey)?; Some(Accelerator::new( @@ -878,4 +918,22 @@ mod tests { assert!(quick_terminal_accelerator(Some("cmd+unknown-key")).is_none()); assert!(quick_terminal_accelerator(Some("cmd+t+x")).is_none()); } + + // The accelerator tracks the engine's effective `ToggleSidebar` chord: + // unbound → no accelerator; the engine-default chord and a custom chord + // both map through; a chord the accelerator path can't represent leaves + // the shortcut column blank (never a chord the engine routes elsewhere). + #[test] + fn sidebar_accelerator_tracks_the_effective_engine_chord() { + assert!(sidebar_accelerator(None).is_none()); + assert_eq!( + sidebar_accelerator(Some("cmd+shift+s")), + Some(cmd_shift_accelerator(Code::KeyS)) + ); + assert_eq!( + sidebar_accelerator(Some("cmd+alt+b")), + accelerator_from_hotkey("cmd+alt+b") + ); + assert!(sidebar_accelerator(Some("cmd+unknown-key")).is_none()); + } } diff --git a/crates/noa-config/src/lib.rs b/crates/noa-config/src/lib.rs index 21a1f20b..aca280bf 100644 --- a/crates/noa-config/src/lib.rs +++ b/crates/noa-config/src/lib.rs @@ -596,11 +596,12 @@ pub struct StartupConfig { /// independent of the terminal grid's [`Self::font_size`]. Default /// [`DEFAULT_SIDEBAR_FONT_SIZE`]. pub sidebar_font_size: f32, - /// `sidebar-hotkey`: the chord that toggles the session sidebar for the - /// focused window. Stored verbatim and parsed by the same app-layer chord - /// path as [`Self::quick_terminal_hotkey`]; `none`/`off`/empty normalize to - /// the empty-string sentinel (no hotkey). Defaults to `None` (unbound) — - /// the sidebar is off by default, so no chord is registered until set. + /// `sidebar-hotkey`: the in-app chord that toggles the session sidebar + /// for the focused window. Unlike [`Self::quick_terminal_hotkey`] it is + /// *not* a system-wide hotkey — it rebinds the keybind engine's + /// `ToggleSidebar` chord (default `cmd+shift+s`), so it only fires while + /// noa is focused. `none`/`off`/empty normalize to the empty-string + /// sentinel (keep the default binding). Defaults to `None`. pub sidebar_hotkey: Option, /// `sidebar-preview-lines`: how many trailing output rows each sidebar card /// extracts and renders. `0` disables last-output preview rows. From 6bdb1dad8a180585099cac13032ace65b51eac0e Mon Sep 17 00:00:00 2001 From: simota Date: Fri, 17 Jul 2026 18:54:30 +0900 Subject: [PATCH 2/2] docs(config): document sidebar-hotkey as an in-app rebind MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CONFIGURATION.md and KEYBINDINGS.md described sidebar-hotkey as a system-wide Carbon hotkey disabled by none/empty — the opposite of the new contract. Sync both tables and move it out of the Global System Hotkeys section. --- docs/CONFIGURATION.md | 2 +- docs/KEYBINDINGS.md | 7 ++++++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index 0f58ab96..919d7765 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -198,7 +198,7 @@ See [noa-server-protocol.md](api/noa-server-protocol.md) for scope and attach-ch | `sidebar-enabled` | `true`, `false` | `false` | Initial sidebar visibility for new windows | | `sidebar-width` | finite decimal `200..=600` | `360` | Sidebar width (points) | | `sidebar-font-size` | finite decimal `8..=20` | `11.5` | Session sidebar font size (points) | -| `sidebar-hotkey` | global hotkey chord, or `none` / `off` / `false` | none | System-wide hotkey for the sidebar. An empty value also disables it | +| `sidebar-hotkey` | in-app keybind chord, or `none` / `off` / `false` | none | Rebinds the sidebar toggle's in-app chord (default `cmd+shift+s`). **Not** a system-wide hotkey — it only fires while noa is focused. `none` / an empty value keeps the default chord. A chord already used by another binding (e.g. `cmd+t`) is rejected with a diagnostic | | `sidebar-preview-lines` | integer `0..=20` | `5` | Number of trailing lines shown in a card. `0` means no preview | See [KEYBINDINGS.md](KEYBINDINGS.md#global-system-hotkeys) for the syntax of global hotkey chords and diff --git a/docs/KEYBINDINGS.md b/docs/KEYBINDINGS.md index e68ae89a..5b161f7b 100644 --- a/docs/KEYBINDINGS.md +++ b/docs/KEYBINDINGS.md @@ -167,7 +167,12 @@ when the app isn't focused. Configurable via config. | Config key | Default | Action | |---|---|---| | `quick-terminal-hotkey` | `cmd+grave` (⌘`) | Toggle Quick Terminal | -| `sidebar-hotkey` | none (disabled) | Toggle sidebar | + +`sidebar-hotkey` is **not** a global hotkey: it rebinds the sidebar +toggle's in-app chord (default ⌘⇧S, `sidebar.toggle`) and only fires +while noa is focused. `none` / an empty value keeps the default chord; +a chord already used by another binding is rejected with a diagnostic. +The Sidebar menu item's shortcut follows the effective chord. The syntax is a `+`-separated chord (e.g. `cmd+shift+t`). Modifier aliases: `cmd`/`command`/`super`/`meta`, `ctrl`/`control`, `alt`/`option`,