diff --git a/src/core/engine/sidebar.rs b/src/core/engine/sidebar.rs index 5e130e1d..efa16088 100644 --- a/src/core/engine/sidebar.rs +++ b/src/core/engine/sidebar.rs @@ -17,6 +17,21 @@ pub const PANEL_EXTENSIONS: &str = "panel:extensions"; pub const PANEL_AI: &str = "panel:ai"; pub const PANEL_SETTINGS: &str = "bottom:settings"; +/// Activity-bar item id for the hamburger (menu) slot — keyboard index 0. +/// +/// The TUI registers this as `panels[0]` of its live [`quadraui::AppShell`] +/// (`tui_main::shell_app::TuiShellApp::live_shell_config`) and +/// `render::build_activity_bar` mints the same id for its hamburger +/// `ActivityItem`, so it is the one activity-bar id that is *already* shared +/// across the two id spaces described on [`EXT_PANEL_ID_PREFIX`]. Promoted out +/// of `tui_main::shell_app` (#536) so [`Engine::activity_bar_item_id`] can name +/// the slot without the core depending on a backend module. +/// +/// GTK paints no hamburger (`include_hamburger = false`), but the slot still +/// exists in the keyboard sequence there — `k` from Explorer lands on it and +/// `l`/Enter toggles the menu bar. That predates #536 and is preserved by it. +pub const HAMBURGER_PANEL_ID: &str = "activity:menu"; + /// Panel-id prefix for plugin-provided ("extension") sidebar panels — e.g. the /// `git-insights` extension's panel is `"ext:git-insights"` (#557). /// @@ -59,6 +74,25 @@ pub const FIXED_ACTIVITY_PANEL_IDS: [&str; 6] = [ PANEL_AI, ]; +/// Keyboard (toolbar) index of the bottom-pinned Settings item. +/// +/// The activity bar's *painted* order is hamburger, the fixed panels, the +/// dynamic extension panels, then Settings pinned to the bottom — but the +/// legacy `activity_bar_selected` index space numbers Settings **before** the +/// extension panels, because extension panels were added after the built-in +/// indices had already been baked into call sites like +/// `Engine::activity_bar_focus_in_at(7)`. That mismatch is exactly why the +/// up/down stepping used to need bespoke arithmetic; since #536 the stepping +/// is done by quadraui's `AppShell` cursor over the *painted* order and these +/// two constants are all that remains of the index space — a lookup table, +/// not a sequencing rule. +pub const TOOLBAR_IDX_SETTINGS: u16 = FIXED_ACTIVITY_PANEL_IDS.len() as u16 + 1; + +/// First keyboard (toolbar) index occupied by a dynamic extension panel. +/// See [`TOOLBAR_IDX_SETTINGS`] for why extension panels sit *after* Settings +/// in the index space while painting *before* it. +pub const TOOLBAR_IDX_EXT_BASE: u16 = TOOLBAR_IDX_SETTINGS + 1; + impl Engine { /// Activity-bar [`quadraui::PanelDefinition`]s for every plugin-registered /// extension panel, sorted by name (#557). @@ -245,16 +279,14 @@ impl Engine { .active_panel_id() .map(|w| w.as_str()) .unwrap_or(""); - match id { - PANEL_EXPLORER => 1, - PANEL_SEARCH => 2, - PANEL_DEBUG => 3, - PANEL_GIT => 4, - PANEL_EXTENSIONS => 5, - PANEL_AI => 6, - PANEL_SETTINGS => 7, - _ => 1, + if id == PANEL_SETTINGS { + return TOOLBAR_IDX_SETTINGS; } + FIXED_ACTIVITY_PANEL_IDS + .iter() + .position(|p| *p == id) + .map(|i| i as u16 + 1) + .unwrap_or(1) } /// Remove activity bar keyboard focus (return focus to the editor). @@ -262,40 +294,158 @@ impl Engine { self.activity_bar_focused = false; } + /// The activity bar's items **in painted order**, as a throwaway + /// [`quadraui::AppShell`] whose keyboard cursor (quadraui#386) does the + /// stepping for [`Self::activity_bar_move_down`] / [`Self::activity_bar_move_up`]. + /// + /// Panel list and bottom-item list mirror the TUI's live `ShellConfig` + /// (`tui_main::shell_app::TuiShellApp::live_shell_config`) exactly: + /// hamburger, the fixed panels, the dynamic extension panels sorted by + /// name, then Settings pinned to the bottom. `AppShell`'s cursor spans + /// `panels` then `bottom_items` as one sequence and saturates at both ends, + /// which *is* vimcode's ordering — so no vimcode-side arithmetic is left. + /// + /// Built on demand rather than cached as a field: the ext-panel list is + /// derived from `self.ext_panels`, which a plugin can mutate at any point + /// in the session, so deriving it per keypress is what makes it impossible + /// for the nav sequence to go stale. It is a `Vec` of ~8 empty-metadata + /// `PanelDefinition`s built once per `j`/`k`, which is not worth caching. + /// + /// Only ids matter here, so icon/tooltip/title are left empty — nothing + /// paints this shell. + fn activity_nav_shell(&self) -> quadraui::AppShell { + fn def(id: &str) -> quadraui::PanelDefinition { + quadraui::PanelDefinition { + id: quadraui::WidgetId::new(id), + icon: String::new(), + tooltip: String::new(), + title: String::new(), + } + } + let mut panels = Vec::with_capacity(1 + FIXED_ACTIVITY_PANEL_IDS.len()); + panels.push(def(HAMBURGER_PANEL_ID)); + panels.extend(FIXED_ACTIVITY_PANEL_IDS.into_iter().map(def)); + // Same list, same sort order, that both backends' `ShellConfig` + // builders and `render::build_activity_bar` use. + panels.extend(self.ext_activity_panels()); + quadraui::AppShell::new(panels, 0.0).with_bottom_items(vec![def(PANEL_SETTINGS)]) + } + + /// The activity-bar item id at keyboard (toolbar) index `idx`, or `None` + /// when `idx` names no item (e.g. a stale extension index after a + /// `:PluginReload` dropped the panel). + /// + /// Index mapping: 0 = hamburger, 1..=6 = [`FIXED_ACTIVITY_PANEL_IDS`], + /// [`TOOLBAR_IDX_SETTINGS`] = Settings, [`TOOLBAR_IDX_EXT_BASE`]`+ k` = + /// the `k`-th extension panel (sorted by name). + pub fn activity_bar_item_id(&self, idx: u16) -> Option { + if idx == 0 { + return Some(HAMBURGER_PANEL_ID.to_string()); + } + if idx == TOOLBAR_IDX_SETTINGS { + return Some(PANEL_SETTINGS.to_string()); + } + if idx < TOOLBAR_IDX_SETTINGS { + return Some(FIXED_ACTIVITY_PANEL_IDS[idx as usize - 1].to_string()); + } + let k = (idx - TOOLBAR_IDX_EXT_BASE) as usize; + self.sorted_ext_panel_names() + .get(k) + .map(|n| ext_panel_id(n)) + } + + /// Inverse of [`Self::activity_bar_item_id`]. + pub fn activity_bar_idx_for_item_id(&self, id: &str) -> Option { + if id == HAMBURGER_PANEL_ID { + return Some(0); + } + if id == PANEL_SETTINGS { + return Some(TOOLBAR_IDX_SETTINGS); + } + if let Some(i) = FIXED_ACTIVITY_PANEL_IDS.iter().position(|p| *p == id) { + return Some(i as u16 + 1); + } + let name = ext_panel_name_from_id(id)?; + self.sorted_ext_panel_names() + .iter() + .position(|n| n == name) + .map(|i| i as u16 + TOOLBAR_IDX_EXT_BASE) + } + + /// The activity-bar item id currently under the keyboard cursor. + /// + /// `render::build_activity_bar` compares each `ActivityItem`'s panel id + /// against this to set `is_keyboard_selected`, instead of re-deriving the + /// item's numeric toolbar index at the paint site (#536). + pub fn activity_bar_selected_item_id(&self) -> Option { + self.activity_bar_item_id(self.activity_bar_selected) + } + + /// Extension panel names in the order they are painted (sorted by name), + /// matching [`Self::ext_activity_panels`]. + fn sorted_ext_panel_names(&self) -> Vec { + let mut names: Vec = self.ext_panels.keys().cloned().collect(); + names.sort(); + names + } + /// Move the keyboard cursor one position down in the activity bar. pub fn activity_bar_move_down(&mut self) { - let ext_count = self.ext_panels.len() as u16; - let max_ext = if ext_count > 0 { 7 + ext_count } else { 0 }; - let sel = self.activity_bar_selected; - if sel < 6 { - self.activity_bar_selected = sel + 1; - } else if sel == 6 && ext_count > 0 { - self.activity_bar_selected = 8; // first ext panel - } else if sel == 6 { - self.activity_bar_selected = 7; // settings - } else if sel >= 8 && sel < max_ext { - self.activity_bar_selected = sel + 1; - } else if sel >= 8 && sel == max_ext { - self.activity_bar_selected = 7; // settings - } - // sel == 7 (settings) → no movement (already at bottom) + self.activity_bar_step(true); } /// Move the keyboard cursor one position up in the activity bar. pub fn activity_bar_move_up(&mut self) { - let ext_count = self.ext_panels.len() as u16; - let max_ext = if ext_count > 0 { 7 + ext_count } else { 0 }; - let sel = self.activity_bar_selected; - if sel == 7 && ext_count > 0 { - self.activity_bar_selected = max_ext; // settings → last ext - } else if sel == 7 { - self.activity_bar_selected = 6; // settings → AI - } else if sel == 8 { - self.activity_bar_selected = 6; // first ext → AI - } else if sel > 8 { - self.activity_bar_selected = sel - 1; + self.activity_bar_step(false); + } + + /// Step the activity-bar keyboard cursor by delegating to + /// [`quadraui::AppShell`]'s `activity_select_next`/`activity_select_prev` + /// (quadraui#386) over [`Self::activity_nav_shell`]. + /// + /// Translates the stored `activity_bar_selected` index into the shell's + /// painted-order cursor, steps, and translates the resulting *item id* + /// back. + /// + /// A selection that names no item — a stale extension index left behind + /// when `:PluginReload` dropped a panel — is clamped to the last item + /// before stepping, mirroring what `AppShell::remove_panel` does to its + /// own cursor. Without that the cursor would be wedged: every subsequent + /// `j`/`k` would find no id to start from and refuse to move. + fn activity_bar_step(&mut self, forward: bool) { + let mut shell = self.activity_nav_shell(); + let np = shell.panels().len(); + let total = np + shell.bottom_items().len(); + if total == 0 { + return; + } + let cursor = self + .activity_bar_selected_item_id() + .and_then(|cur_id| { + shell + .panels() + .iter() + .position(|p| p.id.as_str() == cur_id) + .or_else(|| { + shell + .bottom_items() + .iter() + .position(|p| p.id.as_str() == cur_id) + .map(|i| np + i) + }) + }) + .unwrap_or(total - 1); + shell.activity_set_cursor(cursor); + if forward { + shell.activity_select_next(); } else { - self.activity_bar_selected = sel.saturating_sub(1); + shell.activity_select_prev(); + } + let Some(next_id) = shell.activity_selected_id().map(|w| w.as_str().to_string()) else { + return; + }; + if let Some(idx) = self.activity_bar_idx_for_item_id(&next_id) { + self.activity_bar_selected = idx; } } @@ -306,53 +456,38 @@ impl Engine { /// `ActivityBarActivation` to perform any backend-specific follow-up /// (e.g. setting `sidebar.has_focus`, closing TUI menu). pub fn activity_bar_activate(&mut self) -> ActivityBarActivation { - let sel = self.activity_bar_selected; self.activity_bar_focused = false; - match sel { - 0 => { - self.toggle_menu_bar(); - ActivityBarActivation::MenuToggled - } - 1..=6 => { - let panel_id = match sel { - 1 => PANEL_EXPLORER, - 2 => PANEL_SEARCH, - 3 => PANEL_DEBUG, - 4 => PANEL_GIT, - 5 => PANEL_EXTENSIONS, - _ => PANEL_AI, - }; - self.ext_panel_has_focus = false; - self.ext_panel_active = None; - self.focus_sidebar_panel(panel_id); - ActivityBarActivation::PanelFocused - } - 7 => { - self.ext_panel_has_focus = false; - self.ext_panel_active = None; - self.focus_sidebar_panel(PANEL_SETTINGS); - ActivityBarActivation::PanelFocused - } - idx => { - let ext_idx = (idx - 8) as usize; - let mut ext_names: Vec<_> = self.ext_panels.keys().cloned().collect(); - ext_names.sort(); - if ext_idx < ext_names.len() { - let name = ext_names[ext_idx].clone(); - if !self.app_shell.sidebar_visible() { - self.app_shell.show_panel(&quadraui::WidgetId::new(&name)); - self.session.explorer_visible = true; - let _ = self.session.save(); - } - self.ext_panel_active = Some(name.clone()); - self.ext_panel_has_focus = true; - self.ext_panel_selected = 0; - self.plugin_event("panel_focus", &name); - ActivityBarActivation::ExtPanelFocused(name) - } else { - ActivityBarActivation::NoOp - } + // #536: dispatch on the item *id* rather than re-deriving `sel - 8` + // here. `activity_bar_item_id` owns the one index↔id table, so an + // out-of-range selection (stale extension index) yields `None` → NoOp, + // exactly as the old `ext_idx < ext_names.len()` guard did. + let Some(id) = self.activity_bar_selected_item_id() else { + return ActivityBarActivation::NoOp; + }; + + if id == HAMBURGER_PANEL_ID { + self.toggle_menu_bar(); + return ActivityBarActivation::MenuToggled; + } + + if let Some(name) = ext_panel_name_from_id(&id) { + let name = name.to_string(); + if !self.app_shell.sidebar_visible() { + self.app_shell.show_panel(&quadraui::WidgetId::new(&name)); + self.session.explorer_visible = true; + let _ = self.session.save(); } + self.ext_panel_active = Some(name.clone()); + self.ext_panel_has_focus = true; + self.ext_panel_selected = 0; + self.plugin_event("panel_focus", &name); + return ActivityBarActivation::ExtPanelFocused(name); } + + // A built-in panel: one of `FIXED_ACTIVITY_PANEL_IDS`, or Settings. + self.ext_panel_has_focus = false; + self.ext_panel_active = None; + self.focus_sidebar_panel(&id); + ActivityBarActivation::PanelFocused } } diff --git a/src/render.rs b/src/render.rs index 9723eed3..63fe671e 100644 --- a/src/render.rs +++ b/src/render.rs @@ -8278,11 +8278,21 @@ pub fn build_activity_bar( active_ext_panel: Option<&str>, ) -> quadraui::ActivityBar { use crate::core::engine::sidebar::{ - PANEL_AI, PANEL_DEBUG, PANEL_EXPLORER, PANEL_EXTENSIONS, PANEL_GIT, PANEL_SEARCH, - PANEL_SETTINGS, + ext_panel_id, HAMBURGER_PANEL_ID, PANEL_AI, PANEL_DEBUG, PANEL_EXPLORER, PANEL_EXTENSIONS, + PANEL_GIT, PANEL_SEARCH, PANEL_SETTINGS, }; - let kbd_sel = |idx: u16| engine.activity_bar_focused && engine.activity_bar_selected == idx; + // #536: the keyboard ring is matched by *panel id*, not by re-deriving each + // item's numeric toolbar index at the paint site. `Engine` owns the single + // index↔id table (`activity_bar_item_id`), and the stepping itself is + // quadraui's `AppShell` cursor (quadraui#386) — so the painted order and + // the navigable order cannot drift apart here. + let kbd_sel_id = if engine.activity_bar_focused { + engine.activity_bar_selected_item_id() + } else { + None + }; + let kbd_sel = |panel_id: &str| kbd_sel_id.as_deref() == Some(panel_id); let sb_visible = engine.app_shell.sidebar_visible(); let has_ext = active_ext_panel.is_some(); let active_id = engine.app_shell.active_panel_id().map(|w| w.as_str()); @@ -8291,54 +8301,42 @@ pub fn build_activity_bar( if include_hamburger { top.push(quadraui::ActivityItem { - id: quadraui::WidgetId::new("activity:menu"), + id: quadraui::WidgetId::new(HAMBURGER_PANEL_ID), icon: icons::HAMBURGER.s().to_string(), tooltip: "Menu".to_string(), is_active: false, - is_keyboard_selected: kbd_sel(0), + is_keyboard_selected: kbd_sel(HAMBURGER_PANEL_ID), }); } - // (toolbar_idx, panel_id, icon, tooltip, activity_id) - // Toolbar-keyboard selection indices: - // 0 = hamburger (TUI only), 1-6 = fixed panels, 7 = settings, 8+ = ext panels. - let fixed: [(u16, &str, &str, &str, &str); 6] = [ + // (panel_id, icon, tooltip, activity_id) + let fixed: [(&str, &str, &str, &str); 6] = [ ( - 1, PANEL_EXPLORER, icons::EXPLORER.s(), "Explorer (Ctrl+Shift+E)", "activity:explorer", ), ( - 2, PANEL_SEARCH, icons::SEARCH.s(), "Search (Ctrl+Shift+F)", "activity:search", ), - (3, PANEL_DEBUG, icons::DEBUG.s(), "Debug", "activity:debug"), + (PANEL_DEBUG, icons::DEBUG.s(), "Debug", "activity:debug"), ( - 4, PANEL_GIT, icons::GIT_BRANCH.s(), "Source Control", "activity:git", ), ( - 5, PANEL_EXTENSIONS, icons::EXTENSIONS.s(), "Extensions", "activity:extensions", ), - ( - 6, - PANEL_AI, - icons::AI_CHAT.s(), - "AI Assistant", - "activity:ai", - ), + (PANEL_AI, icons::AI_CHAT.s(), "AI Assistant", "activity:ai"), ]; // #635 (Stage 6b): `tui_main::shell_app::TuiShellApp::shell_config` derives @@ -8349,34 +8347,34 @@ pub fn build_activity_bar( // apart: a reordering here without updating the shared constant now trips // in any test that exercises `build_activity_bar` (every one does). debug_assert_eq!( - fixed.map(|(_, panel_id, _, _, _)| panel_id), + fixed.map(|(panel_id, _, _, _)| panel_id), crate::core::engine::sidebar::FIXED_ACTIVITY_PANEL_IDS, "build_activity_bar's `fixed` panel-id order must match \ sidebar::FIXED_ACTIVITY_PANEL_IDS" ); - for (toolbar_idx, panel_id, icon, tooltip, activity_id) in fixed { + for (panel_id, icon, tooltip, activity_id) in fixed { top.push(quadraui::ActivityItem { id: quadraui::WidgetId::new(activity_id), icon: icon.to_string(), tooltip: tooltip.to_string(), is_active: sb_visible && !has_ext && active_id == Some(panel_id), - is_keyboard_selected: kbd_sel(toolbar_idx), + is_keyboard_selected: kbd_sel(panel_id), }); } - // Dynamic extension panels (sorted by name; toolbar indices 8+). + // Dynamic extension panels, sorted by name — the same order + // `Engine::ext_activity_panels` (and therefore the keyboard ring) uses. let mut ext_panels: Vec<_> = engine.ext_panels.values().collect(); ext_panels.sort_by(|a, b| a.name.cmp(&b.name)); - for (i, panel) in ext_panels.iter().enumerate() { - let toolbar_idx = 8 + i as u16; + for panel in ext_panels.iter() { let is_active = sb_visible && active_ext_panel == Some(panel.name.as_str()); top.push(quadraui::ActivityItem { id: quadraui::WidgetId::new(format!("activity:ext:{}", panel.name)), icon: panel.resolved_icon().to_string(), tooltip: panel.title.clone(), is_active, - is_keyboard_selected: kbd_sel(toolbar_idx), + is_keyboard_selected: kbd_sel(&ext_panel_id(&panel.name)), }); } @@ -8385,7 +8383,7 @@ pub fn build_activity_bar( icon: icons::SETTINGS.s().to_string(), tooltip: "Settings".to_string(), is_active: sb_visible && !has_ext && active_id == Some(PANEL_SETTINGS), - is_keyboard_selected: kbd_sel(7), + is_keyboard_selected: kbd_sel(PANEL_SETTINGS), }]; quadraui::ActivityBar { diff --git a/src/tui_main/panels.rs b/src/tui_main/panels.rs index 2a2f5aad..b455ec54 100644 --- a/src/tui_main/panels.rs +++ b/src/tui_main/panels.rs @@ -1823,3 +1823,230 @@ mod sc_panel_tests { let _ = render_sc(&e, 10, 3); } } + +// ─── Activity-bar keyboard ring (#536) ─────────────────────────────────────── +// +// Black-box coverage for the migration of the activity-bar keyboard cursor +// onto quadraui's `AppShell` (quadraui#386). Every assertion reads the +// **rasterised** activity-bar strip — the row whose background is the +// selection colour — rather than `Engine::activity_bar_selected`, so a +// selection index that moves correctly but paints on the wrong icon (the +// #587/#592 failure mode: state populated, nothing painted) still fails here. +// +// The ring's ordering is the thing under test: hamburger, the six fixed +// panels, the dynamic extension panels spliced in *before* Settings, and +// Settings pinned last — while the legacy `activity_bar_selected` index space +// numbers Settings at 7 and extension panels at 8+. Before #536 that mismatch +// was reconciled by a hand-rolled `if sel < 6 { … } else if sel == 6 && …` +// chain in `core::engine::sidebar`; it is now `AppShell`'s cursor. +#[cfg(test)] +mod activity_bar_keyboard_ring_tests { + use super::*; + use crate::core::plugin::PanelRegistration; + use ratatui::buffer::Buffer; + + const BAR_W: u16 = 3; + const BAR_H: u16 = 12; + + fn ring_engine() -> Engine { + crate::core::session::suppress_disk_saves(); + let mut e = Engine::new_for_test(); + e.extension_state = crate::core::session::ExtensionState::default(); + e.ext_registry = None; + e.ext_panels.clear(); + e + } + + fn add_ext(e: &mut Engine, name: &str, icon: char) { + e.ext_panels.insert( + name.to_string(), + PanelRegistration { + name: name.to_string(), + title: name.to_string(), + icon, + fallback_icon: Some(icon), + sections: vec![], + }, + ); + } + + /// Paint the activity bar and return `(row, icon_char)` for the single row + /// carrying the keyboard-selection background, or `None` when no row does. + /// + /// `draw_activity_bar` fills the selected row with `bar.selection_bg` + /// (`theme.cursor`) and every other row with `theme.tab_bar_bg`, so the + /// probe is "which row's background is the cursor colour" — the same thing + /// a user sees. The icon glyph comes back with it so the assertions can + /// name the item rather than a bare row number (#555: probe, don't + /// hardcode). + fn painted_ring(engine: &Engine) -> Option<(u16, char)> { + let theme = crate::render::Theme::onedark(); + let sel = ratatui::style::Color::Rgb(theme.cursor.r, theme.cursor.g, theme.cursor.b); + let area = Rect { + x: 0, + y: 0, + width: BAR_W, + height: BAR_H, + }; + let mut buf = Buffer::empty(area); + let sidebar = TuiSidebar::new(); + render_activity_bar(&mut buf, area, &sidebar, &theme, false, engine); + + let mut hit = None; + for y in 0..BAR_H { + if buf[(0, y)].bg == sel { + assert!( + hit.is_none(), + "more than one row painted the selection ring" + ); + hit = Some((y, buf[(1, y)].symbol().chars().next().unwrap_or(' '))); + } + } + hit + } + + /// The ring only paints while the bar holds keyboard focus, and `j` walks + /// the fixed panels top-down from the hamburger. + #[test] + fn ring_paints_only_when_focused_and_j_walks_the_fixed_panels() { + let mut e = ring_engine(); + assert_eq!( + painted_ring(&e), + None, + "no ring should paint while the activity bar is unfocused" + ); + + e.activity_bar_focus_in_at(0); + let (hamburger_row, _) = painted_ring(&e).expect("focusing the bar must paint a ring"); + assert_eq!(hamburger_row, 0, "index 0 is the hamburger, the top row"); + + for expected_row in 1..=6 { + e.activity_bar_move_down(); + let (row, _) = painted_ring(&e).expect("ring must stay painted while stepping"); + assert_eq!( + row, + expected_row, + "j from row {} must land on row {expected_row}", + expected_row - 1 + ); + } + } + + /// With no extension panels, `j` past the last fixed panel (AI) lands on + /// Settings — which paints *pinned to the bottom edge*, not on row 7 — and + /// saturates there. `k` comes straight back to AI. + #[test] + fn ring_steps_from_ai_to_bottom_pinned_settings_and_saturates() { + let mut e = ring_engine(); + e.activity_bar_focus_in_at(6); // AI, the last fixed panel + assert_eq!(painted_ring(&e).map(|(r, _)| r), Some(6)); + + e.activity_bar_move_down(); + assert_eq!( + painted_ring(&e).map(|(r, _)| r), + Some(BAR_H - 1), + "Settings is bottom-pinned, so the ring must jump to the last row" + ); + assert_eq!(e.activity_bar_selected, 7, "Settings is toolbar index 7"); + + e.activity_bar_move_down(); + assert_eq!( + painted_ring(&e).map(|(r, _)| r), + Some(BAR_H - 1), + "j on the bottom-most item must saturate, not wrap to the top" + ); + + e.activity_bar_move_up(); + assert_eq!( + painted_ring(&e).map(|(r, _)| r), + Some(6), + "k from Settings with no extension panels returns to AI" + ); + } + + /// `k` on the top-most item saturates rather than wrapping to Settings. + #[test] + fn ring_saturates_at_the_hamburger() { + let mut e = ring_engine(); + e.activity_bar_focus_in_at(0); + e.activity_bar_move_up(); + assert_eq!(painted_ring(&e).map(|(r, _)| r), Some(0)); + assert_eq!(e.activity_bar_selected, 0); + } + + /// The headline ordering claim: extension panels splice in **between** AI + /// and Settings in painted order (sorted by name), even though the legacy + /// index space numbers them *after* Settings. Walking `j` from AI must + /// visit both extension icons and only then reach Settings. + #[test] + fn ring_splices_extension_panels_between_ai_and_settings() { + let mut e = ring_engine(); + add_ext(&mut e, "zz-last", 'Z'); + add_ext(&mut e, "aa-first", 'A'); + e.activity_bar_focus_in_at(6); // AI + + e.activity_bar_move_down(); + assert_eq!( + painted_ring(&e), + Some((7, 'A')), + "j from AI must land on the first extension panel (sorted by name)" + ); + assert_eq!(e.activity_bar_selected, 8, "…which is toolbar index 8"); + + e.activity_bar_move_down(); + assert_eq!( + painted_ring(&e), + Some((8, 'Z')), + "j must then land on the second extension panel" + ); + assert_eq!(e.activity_bar_selected, 9); + + e.activity_bar_move_down(); + assert_eq!( + painted_ring(&e).map(|(r, _)| r), + Some(BAR_H - 1), + "only after the last extension panel does j reach bottom-pinned Settings" + ); + assert_eq!(e.activity_bar_selected, 7); + + // …and `k` from Settings walks back onto the *last* extension panel. + e.activity_bar_move_up(); + assert_eq!(painted_ring(&e), Some((8, 'Z'))); + assert_eq!(e.activity_bar_selected, 9); + + e.activity_bar_move_up(); + assert_eq!(painted_ring(&e), Some((7, 'A'))); + + e.activity_bar_move_up(); + assert_eq!( + painted_ring(&e).map(|(r, _)| r), + Some(6), + "k off the first extension panel returns to AI, not to Settings" + ); + assert_eq!(e.activity_bar_selected, 6); + } + + /// A selection left pointing at an extension panel that has since been + /// unregistered (`:PluginReload`) must not wedge the cursor: the next + /// `k` has to move somewhere real. Pre-#536 the bespoke `sel > 8` arm + /// stepped to 8; the `AppShell` cursor clamps to the last item first and + /// then steps, landing in the same place. + #[test] + fn ring_recovers_from_a_stale_extension_index() { + let mut e = ring_engine(); + add_ext(&mut e, "only-one", 'O'); + e.activity_bar_focus_in_at(9); // second ext panel — no longer exists + assert_eq!( + painted_ring(&e), + None, + "a selection naming no item paints no ring" + ); + + e.activity_bar_move_up(); + assert_eq!( + e.activity_bar_selected, 8, + "k must recover onto the one extension panel that does exist" + ); + assert_eq!(painted_ring(&e).map(|(r, _)| r), Some(7)); + } +} diff --git a/src/tui_main/shell_app.rs b/src/tui_main/shell_app.rs index 503fc94b..6e89be6b 100644 --- a/src/tui_main/shell_app.rs +++ b/src/tui_main/shell_app.rs @@ -361,12 +361,14 @@ use super::*; /// clippy's `type_complexity` lint. type HoverLinkRects = Vec<(u16, u16, u16, u16, String)>; -/// Activity-bar item id for the menu hamburger — must match the literal -/// `render::build_activity_bar` uses for its own hamburger `ActivityItem` -/// (`"activity:menu"`, `render.rs:8147`) so [`TuiShellApp::shell_config`]'s -/// `PanelDefinition` and [`ShellApp::on_shell_event`]'s hamburger check stay -/// in lockstep with the live `draw_frame` path. -const HAMBURGER_PANEL_ID: &str = "activity:menu"; +/// Activity-bar item id for the menu hamburger. +/// +/// #536 promoted the literal to `core::engine::sidebar` — it is now shared by +/// `render::build_activity_bar`'s hamburger `ActivityItem`, this module's +/// `ShellConfig` `PanelDefinition`, [`ShellApp::on_shell_event`]'s hamburger +/// check, *and* `Engine::activity_bar_item_id`'s keyboard-index-0 slot, so +/// there is exactly one definition rather than one per call site. +use crate::core::engine::sidebar::HAMBURGER_PANEL_ID; /// TUI counterpart to GTK's `App` struct. Owns everything that is a local /// `mut` variable in `event_loop()` today. Fields the (`&self`) diff --git a/tests/extensions.rs b/tests/extensions.rs index 7d95acf3..21d338f1 100644 --- a/tests/extensions.rs +++ b/tests/extensions.rs @@ -2294,18 +2294,85 @@ fn gcc_is_undoable() { use vimcode_core::core::plugin::{PluginCallContext, PluginManager}; -fn plugin_with(code: &str) -> PluginManager { - let dir = std::env::temp_dir().join(format!( - "vc_git_api_test_{}", - code.len() // simple discriminator - )); - let _ = std::fs::remove_dir_all(&dir); +/// A scratch plugin directory that is unique to this call. +/// +/// This used to be keyed on `code.len()`, which is not a discriminator at all: +/// two scripts of the same length share a directory. `git_api_stash_list_returns_table` +/// and `git_api_blame_file_returns_table` both hashed to `vc_git_api_test_174`, so +/// under cargo's parallel test threads one test's `remove_dir_all` could delete the +/// other's `test.lua` in the window before `load_plugins_dir` read it. `read_dir` +/// then fails silently (see `PluginManager::load_plugins_dir`), no command is +/// registered, and the victim's `assert!(found)` panics — a harness race that reads +/// like a real regression. +/// +/// Keying on pid + a monotonic counter makes every call site its own directory, so +/// no two tests can ever contend for one path. +fn plugin_test_dir() -> std::path::PathBuf { + use std::sync::atomic::{AtomicUsize, Ordering}; + static SEQ: AtomicUsize = AtomicUsize::new(0); + std::env::temp_dir().join(format!( + "vc_git_api_test_{}_{}", + std::process::id(), + SEQ.fetch_add(1, Ordering::Relaxed) + )) +} + +/// Load `code` as a one-file plugin, returning the manager and the scratch +/// directory it was loaded from (so tests can assert on path uniqueness). +fn plugin_with_dir(code: &str) -> (PluginManager, std::path::PathBuf) { + let dir = plugin_test_dir(); std::fs::create_dir_all(&dir).unwrap(); let path = dir.join("test.lua"); std::fs::write(&path, code).unwrap(); let mut pm = PluginManager::new().unwrap(); pm.load_plugins_dir(&dir, &[]); - pm + // `load_plugins_dir` reads and executes eagerly, so the files are no longer + // needed once it returns — clean up rather than leaking a dir per call. + let _ = std::fs::remove_dir_all(&dir); + (pm, dir) +} + +fn plugin_with(code: &str) -> PluginManager { + plugin_with_dir(code).0 +} + +#[test] +fn plugin_with_uses_a_distinct_dir_for_same_length_scripts() { + // Regression guard for the flake described on `plugin_test_dir`. These two + // scripts are deliberately the same byte length — the exact condition that + // aliased two tests onto `/tmp/vc_git_api_test_174` and let one test's + // `remove_dir_all` race the other's `load_plugins_dir`. + // + // Against the old `code.len()` keying this assertion is deterministically + // red: both calls resolve to the identical path. Verified by reinstating + // the old scheme locally before committing. + let a = "vimcode.command(\"TestAaa\", function(_) vimcode.message(\"a\") end)"; + let b = "vimcode.command(\"TestBbb\", function(_) vimcode.message(\"b\") end)"; + assert_eq!(a.len(), b.len(), "fixture scripts must be the same length"); + + let (pm_a, dir_a) = plugin_with_dir(a); + let (pm_b, dir_b) = plugin_with_dir(b); + assert_ne!( + dir_a, dir_b, + "same-length scripts must not share a scratch dir" + ); + + // …and both plugins really did load from their own directory. + let (found_a, ctx_a) = pm_a.call_command("TestAaa", "", PluginCallContext::default()); + assert!(found_a, "first plugin's command should be registered"); + assert_eq!(ctx_a.message.as_deref(), Some("a")); + + let (found_b, ctx_b) = pm_b.call_command("TestBbb", "", PluginCallContext::default()); + assert!(found_b, "second plugin's command should be registered"); + assert_eq!(ctx_b.message.as_deref(), Some("b")); +} + +#[test] +fn plugin_scratch_dirs_are_cleaned_up() { + // The old helper left one dir per distinct script length behind in /tmp; + // the unique-path scheme would leak one per call if we did not clean up. + let (_pm, dir) = plugin_with_dir("vimcode.command(\"TestTmp\", function(_) end)"); + assert!(!dir.exists(), "scratch dir {dir:?} should be removed"); } #[test]