diff --git a/src/core/engine/keys.rs b/src/core/engine/keys.rs index bdef889d..3b3d0d02 100644 --- a/src/core/engine/keys.rs +++ b/src/core/engine/keys.rs @@ -7723,10 +7723,7 @@ impl Engine { } } } else if !text.is_empty() { - self.terminal_write(b"\x1b[200~"); - self.terminal_write(text.as_bytes()); - self.terminal_write(b"\x1b[201~"); - self.poll_terminal(); + self.terminal_paste(text); } return; } diff --git a/src/core/engine/mod.rs b/src/core/engine/mod.rs index 0650ec4f..8edea21f 100644 --- a/src/core/engine/mod.rs +++ b/src/core/engine/mod.rs @@ -4080,6 +4080,39 @@ impl Engine { /// registry, then either open the CLI-supplied path or restore the /// previous session. Both TUI and GTK call this identically. pub fn startup(&mut self, file_path: Option<&Path>) { + self.startup_inner(file_path, true); + } + + /// [`Engine::startup`] minus the per-workspace session restore — the + /// constructor deterministic-geometry tests must use. + /// + /// [`Engine::new_for_test`] is **not** sufficient on its own. It builds + /// the engine from in-memory `Settings::default()` / `SessionState::default()`, + /// but `startup(None)` then calls `restore_session_files()`, which performs + /// a *second, independent* disk read — + /// `SessionState::load_for_workspace(&self.cwd)` — keyed on the process's + /// `current_dir()` at construction time and entirely unrelated to whichever + /// `Settings`/`SessionState` the engine was built with. + /// `SessionState::save_for_workspace` is stubbed out under `cfg(test)`, but + /// `load_for_workspace` deliberately is not (several tests write a workspace + /// session file and assert it is restored). So on a machine that happens to + /// have a real `~/.config/vimcode/sessions/.json` for the checkout the + /// test binary runs in — entirely plausible for a self-hosting editor whose + /// developers edit it with itself — `startup(None)` reopens that session's + /// files and splits, `new_tab()` allocates a fresh `WindowId` per extra + /// file, and `windows.len()` stops being 1. Geometry measured against that + /// layout is then machine-dependent. + /// + /// This entry point skips the restore entirely, so the resulting engine + /// depends on nothing but its in-memory defaults and the explicit + /// `file_path` argument. + pub fn startup_without_session_restore(&mut self, file_path: Option<&Path>) { + self.startup_inner(file_path, false); + } + + /// Shared body of [`Engine::startup`] and + /// [`Engine::startup_without_session_restore`]. + fn startup_inner(&mut self, file_path: Option<&Path>, restore_session: bool) { self.plugin_init(); self.ext_refresh(); if let Some(path) = file_path { @@ -4088,7 +4121,7 @@ impl Engine { } else { let _ = self.open_file_with_mode(path, OpenMode::Permanent); } - } else { + } else if restore_session { self.restore_session_files(); } } diff --git a/src/core/engine/terminal_ops.rs b/src/core/engine/terminal_ops.rs index 6ef15d9c..b6f870b8 100644 --- a/src/core/engine/terminal_ops.rs +++ b/src/core/engine/terminal_ops.rs @@ -807,6 +807,22 @@ impl Engine { } } + /// Paste `text` into the active pane's PTY, then poll it so the echo + /// lands in the frame the caller is about to paint. + /// + /// Delegates the bracketed-paste decision to quadraui's + /// `TerminalSession::paste` (quadraui#343/#415), which wraps in + /// `ESC[200~ … ESC[201~` only when the child has actually enabled DEC + /// private mode 2004. Both call sites used to hand-roll an + /// *unconditional* wrap, which leaked literal `[200~` bytes into programs + /// that do not strip them (`cat`, `less`, a shell without a line editor). + pub fn terminal_paste(&mut self, text: &str) { + if let Some(term) = self.active_terminal_mut() { + term.paste(text); + } + self.poll_terminal(); + } + /// Resize all terminal panes (shared panel height). pub fn terminal_resize(&mut self, cols: u16, rows: u16) { for slot in &mut self.terminal_panes { diff --git a/src/core/engine/tests.rs b/src/core/engine/tests.rs index a6e1f7bd..0fc3e746 100644 --- a/src/core/engine/tests.rs +++ b/src/core/engine/tests.rs @@ -348,6 +348,90 @@ fn test_qa_bang_force_quits() { assert_eq!(action, EngineAction::Quit); } +/// `Engine::startup_without_session_restore` must ignore a per-workspace +/// session file that `Engine::startup` would have honoured. +/// +/// This is the ambient-input class `TuiShellApp::new_for_test` exists to close: +/// `Engine::new_for_test()` only replaces the two *global* config reads +/// (`settings.json` / `session.json`), while `restore_session_files()` does a +/// second, independent `SessionState::load_for_workspace(&self.cwd)` read keyed +/// on the process's cwd. On a machine that has ever saved a session for the +/// checkout the test binary runs in, that read reopens real files and splits +/// and `windows.len()` stops being 1 — which is exactly what the TUI drag test's +/// setup-sanity assertion trips on. +/// +/// The first half of this test is the control: it proves the fixture session +/// file on disk really is restorable, so a green second half means the restore +/// was *skipped*, not that the fixture was inert. +#[test] +fn test_startup_without_session_restore_ignores_workspace_session() { + use crate::core::session::SessionState; + let dir = std::env::temp_dir(); + let p1 = dir.join("vimcode_no_restore_a.txt"); + let p2 = dir.join("vimcode_no_restore_b.txt"); + std::fs::write(&p1, "aaa").unwrap(); + std::fs::write(&p2, "bbb").unwrap(); + + let workspace_dir = dir.join("vimcode_no_restore_test_ws"); + std::fs::create_dir_all(&workspace_dir).unwrap(); + let mut ws_session = SessionState::default(); + ws_session.open_files = vec![p1.clone(), p2.clone()]; + ws_session.active_file = Some(p2.clone()); + // save_for_workspace is a no-op under #[cfg(test)], so write directly. + let session_path = SessionState::session_path_for_workspace(&workspace_dir); + if let Some(parent) = session_path.parent() { + std::fs::create_dir_all(parent).unwrap(); + } + std::fs::write( + &session_path, + serde_json::to_string_pretty(&ws_session).unwrap(), + ) + .unwrap(); + + // Control: the on-disk session really does restore two tabs. + let mut restoring = Engine::new_for_test(); + restoring.cwd = workspace_dir.clone(); + restoring.settings.swap_file = false; + restoring.restore_session_files(); + assert_eq!( + restoring.active_group().tabs.len(), + 2, + "control: the fixture workspace session must be restorable, otherwise \ + the assertion below proves nothing" + ); + + // Subject: the deterministic startup path must not touch it at all. + let mut deterministic = Engine::new_for_test(); + deterministic.cwd = workspace_dir.clone(); + deterministic.settings.swap_file = false; + let tabs_before = deterministic.active_group().tabs.len(); + let windows_before = deterministic.windows.len(); + deterministic.startup_without_session_restore(None); + assert_eq!( + deterministic.active_group().tabs.len(), + tabs_before, + "startup_without_session_restore must not reopen the workspace session's files" + ); + assert_eq!( + deterministic.windows.len(), + windows_before, + "startup_without_session_restore must not allocate windows for restored files" + ); + assert!( + !deterministic + .buffer_manager + .iter() + .any(|(_, s)| s.file_path.as_deref() == Some(p1.as_path()) + || s.file_path.as_deref() == Some(p2.as_path())), + "no buffer should have been created for a session-listed file" + ); + + let _ = std::fs::remove_file(&p1); + let _ = std::fs::remove_file(&p2); + let _ = std::fs::remove_file(&session_path); + let _ = std::fs::remove_dir(&workspace_dir); +} + #[test] fn test_restore_session_files_opens_separate_tabs() { use crate::core::session::SessionState; diff --git a/src/gtk/mod.rs b/src/gtk/mod.rs index db362613..ada204e7 100644 --- a/src/gtk/mod.rs +++ b/src/gtk/mod.rs @@ -1888,7 +1888,14 @@ impl App { } #[allow(clippy::too_many_lines)] - fn handle_key_press(&mut self, key_name: String, unicode: Option, ctrl: bool, alt: bool) { + fn handle_key_press( + &mut self, + key_name: String, + unicode: Option, + ctrl: bool, + shift: bool, + alt: bool, + ) { // ── Shared modal keyboard rung (#734 slice 1) ────────────────── // `render::route_modal_key` states the spell-suggestion → dialog → // context-menu ladder once for both backends. GTK used to hand-roll @@ -1962,6 +1969,30 @@ impl App { return; } + // ── Shared terminal (PTY) keyboard rung (#758 / #734 slice 3) ── + // GTK had no terminal rung at all between #540 and this change: the + // block that forwarded keys to the PTY lived in the Relm4 `view!`'s + // `EventControllerKey` closure and was deleted with it, so every key + // typed into a focused terminal fell through to `Engine::handle_key` + // and ran vim commands on the *editor buffer* (#471). It sits here, + // directly below the focus owners and above the debug F-keys, so the + // ladder matches TUI's exactly: `route_focus_key` already returns + // `FocusKeyRoute::None` while the terminal holds focus, and a focused + // terminal must take F5/F9/F10/F11 to the PTY rather than the + // debugger — which is what `vim`/`htop` running inside it expect. + if render::route_terminal_key( + &mut self.engine.borrow_mut(), + &key_name, + unicode, + ctrl, + shift, + alt, + ) { + self.sync_plus_register_to_clipboard(); + self.draw_needed.set(true); + return; + } + // Debug F-keys must reach the engine regardless of which panel // has focus — F5 (continue), F9 (breakpoint), F10 (step over), // F11 (step in) are global debugger commands. @@ -6875,13 +6906,19 @@ impl quadraui::ShellApp for App { } }; if !key_name.is_empty() || unicode.is_some() { - self.handle_key_press(key_name, unicode, modifiers.ctrl, modifiers.alt); + self.handle_key_press( + key_name, + unicode, + modifiers.ctrl, + modifiers.shift, + modifiers.alt, + ); } } UiEvent::CharTyped(c) => { // Ctrl-modified characters arrive via KeyPressed; CharTyped is // for IME-composed printable characters only. - self.handle_key_press(c.to_string(), Some(c), false, false); + self.handle_key_press(c.to_string(), Some(c), false, false, false); } UiEvent::Accelerator(id, _mods) => { let id_str = id.as_str().to_string(); diff --git a/src/gtk/testing.rs b/src/gtk/testing.rs index dbc69360..2dc7ac3b 100644 --- a/src/gtk/testing.rs +++ b/src/gtk/testing.rs @@ -2762,6 +2762,115 @@ mod sidebar_panel_clicks { ); } + // ── #758 / #734 slice 3: the shared terminal (PTY) keyboard rung ─────── + + /// GTK half of `terminal_ctrl_f_opens_the_painted_find_bar_via_shell_app` + /// (`tui_main/shell_app.rs`): with the terminal focused, Ctrl+F must open + /// the *terminal's* find bar, and subsequent characters must land in that + /// bar's query — through `App::handle` -> `handle_key_press` -> + /// `render::route_terminal_key` -> `Engine::handle_terminal_key`. + /// + /// GTK had **no terminal keyboard rung at all** between #540 and #758: + /// the `if engine.borrow().terminal_has_focus { … }` block lived in the + /// Relm4 `view!`'s `EventControllerKey` closure and was deleted with it, + /// so Ctrl+F opened the editor's find/replace overlay and every other key + /// ran a vim command on the buffer while the user was looking at a shell + /// prompt (#471). + /// + /// Asserts on rendered output (`CLAUDE.md` rule 1): the terminal + /// toolbar's painted `" FIND: …"` text (`render::build_terminal_toolbar`, + /// drawn by `draw_terminal_panel`), never `terminal_find_active`. + /// + /// **Verified RED against unfixed `develop`:** without the + /// `render::route_terminal_key` call, Ctrl+F falls through to + /// `Engine::handle_key` and no `"FIND:"` ever paints — the second + /// assertion fires. + #[test] + fn terminal_ctrl_f_opens_the_painted_find_bar() { + let mut engine = Engine::new_for_test(); + engine.settings.use_nerd_fonts = false; + // `terminal_new_tab` opens the panel and focuses it. + engine.terminal_new_tab(80, 10); + + let mut h = harness(engine, 1400, 900); + h.driver.render(); + assert!( + !h.driver.screen_contains("FIND:"), + "precondition: the terminal toolbar starts as a tab strip; painted: {:?}", + h.driver.painted_texts() + ); + + h.driver.ctrl_char('f'); + h.driver.render(); + assert!( + h.driver.screen_contains("FIND:"), + "Ctrl+F with the terminal focused must open the terminal find bar, \ + not the editor find/replace overlay; painted: {:?}", + h.driver.painted_texts() + ); + + h.driver.type_char('z'); + h.driver.render(); + assert!( + h.driver.screen_contains("FIND: z"), + "characters typed after Ctrl+F must reach the terminal find query \ + through the shared router; painted: {:?}", + h.driver.painted_texts() + ); + } + + /// A focused terminal must swallow ordinary keys so they never reach the + /// editor buffer. This is the user-visible shape of the missing GTK rung: + /// typing `x` at a shell prompt deleted a character from the *file*. + /// + /// Asserts on the painted buffer text, with a positive control — the same + /// key on the same fixture with the terminal unfocused must delete the + /// character — so a fixture whose text could not change would fail. + /// + /// **Verified RED against unfixed `develop`:** without the router call + /// the first `x` reaches `Engine::handle_key`, the painted line drops to + /// `QXWTERMGTK758` immediately, and the second assertion fires. + #[test] + fn focused_terminal_swallows_editor_keys_on_gtk() { + let build = |focused: bool| { + let mut engine = Engine::new_for_test(); + engine.settings.use_nerd_fonts = false; + engine.buffer_mut().insert(0, "ZQXWTERMGTK758\n"); + engine.terminal_new_tab(80, 6); + engine.terminal_has_focus = focused; + harness(engine, 1400, 900) + }; + + let mut h = build(true); + h.driver.render(); + assert!( + h.driver.screen_contains("ZQXWTERMGTK758"), + "precondition: the buffer line must paint; painted: {:?}", + h.driver.painted_texts() + ); + + h.driver.type_char('x'); + h.driver.render(); + assert!( + h.driver.screen_contains("ZQXWTERMGTK758"), + "`x` with the terminal focused must go to the PTY, not delete a \ + character from the editor buffer; painted: {:?}", + h.driver.painted_texts() + ); + + let mut control = build(false); + control.driver.render(); + control.driver.type_char('x'); + control.driver.render(); + assert!( + control.driver.screen_contains("QXWTERMGTK758") + && !control.driver.screen_contains("ZQXWTERMGTK758"), + "control: with the terminal unfocused `x` must delete the first \ + character; painted: {:?}", + control.driver.painted_texts() + ); + } + /// The sidebar hover rung must exist **on this backend at all**. /// /// Before #754 the Source Control toolbar's hover highlight was driven by diff --git a/src/render.rs b/src/render.rs index 6ebe130d..5cb7c2ea 100644 --- a/src/render.rs +++ b/src/render.rs @@ -3056,6 +3056,191 @@ pub fn activity_bar_key_action(key: &str, ctrl: bool) -> ActivityBarKeyAction { } } +// ─── Terminal (PTY) keyboard rung (#758 / #734 slice 3, #351, #471) ────────── +// +// The rung directly beneath [`route_focus_key`]: once no modal overlay and no +// focus owner has claimed the key, a focused embedded terminal takes it before +// the editor ever sees it. +// +// **Where the encoder lives.** quadraui's `TerminalSession` already owns the +// *mouse* encoder (`encode_mouse` / `forward_mouse`) and, since quadraui#343, +// bracketed paste (`paste` / `bracketed_paste_enabled`) — but quadraui#342 +// ("lift the keyboard → PTY encoder out of the example into the engine") has +// **not** landed on the pinned rev, so there is no upstream +// `TerminalSession::key_bytes`. The key encoder therefore stays where it +// already is: [`crate::core::engine::terminal_ops::key_to_pty_bytes`], which is +// platform-neutral `core` code and satisfies `CLAUDE.md`'s neutrality rule +// exactly as well. When quadraui#342 lands, `key_to_pty_bytes` is the single +// call site to swap; nothing in either backend changes. +// +// **What diverged.** TUI hand-rolled this rung inside `handle_key_pressed` +// (~80 lines). GTK's twin was deleted outright by the #540 Relm4→ShellApp +// cutover — the whole `if engine.borrow().terminal_has_focus { … }` block lived +// in the per-window `EventControllerKey` closure that went with the Relm4 +// `view!`, and nothing replaced it. Since #540, GTK keys typed into a focused +// terminal have fallen through to `Engine::handle_key` and edited the *buffer* +// instead of reaching the PTY. Three concrete disagreements this router ends: +// +// 1. **GTK forwarded nothing at all.** Typing in the terminal ran vim normal +// mode commands on the editor buffer. This is the live half of **#471** — +// the other half was the old GTK arm's `sender.input(Msg::Resize)` after +// every terminal keypress, whose handler called +// `terminal_resize(full_panel_cols, …)` on *every* pane. In split mode that +// reflowed the half-width panes to the full panel width on each keystroke +// while they were still painted into their narrow rects, so freshly typed +// text in the right pane wrapped off the painted area and "disappeared". +// This router performs no resize, and [`route_terminal_resize`] is +// split-aware, so neither half can come back. +// 2. **Key-name spelling.** TUI reached past `translate_key` and re-derived +// names from the raw crossterm `KeyCode` (`"Page_Up"`, `"ISO_Left_Tab"`) +// because `translate_key`'s editor-facing names (`"Shift_Up"`, +// `"Shift_Return"`) have no PTY encoding; GTK speaks `"PageUp"` / +// `"BackTab"`. [`canonical_terminal_key_name`] accepts both spellings, so +// PageUp scrolls the scrollback on GTK too (it previously did not exist, +// and would have fallen through to a raw `ESC[5~` write had it). +// 3. **`SendToPty` follow-through.** TUI polled the PTY immediately after the +// write so the echo landed in the same frame; the old GTK arm did not, and +// relied on the next poll tick. The router always polls. +// +// Both backends now call [`route_terminal_key`] and do nothing else for this +// rung. + +/// Canonicalise a backend key name into the spelling +/// [`crate::core::engine::terminal_ops::key_to_pty_bytes`] and +/// `Engine::handle_terminal_key` expect. +/// +/// The two backends name the same physical keys differently, and TUI's +/// `translate_key` additionally prefixes shifted navigation keys with `Shift_` +/// for the *editor*'s benefit — a prefix the PTY encoder has no arm for, which +/// is why TUI used to bypass `translate_key` entirely here. Shift is already +/// carried as its own `shift` argument, so the prefix is pure noise on this +/// rung and is stripped. +pub fn canonical_terminal_key_name(key_name: &str) -> &str { + let base = key_name.strip_prefix("Shift_").unwrap_or(key_name); + match base { + // GTK's `NamedKey::Enter` mapping and the GDK keypad name. + "Enter" | "KP_Enter" => "Return", + // GTK says "PageUp"; X11/GDK says "Page_Up"/"Prior"; TUI says "Page_Up". + "PageUp" | "Prior" | "KP_Page_Up" => "Page_Up", + "PageDown" | "Next" | "KP_Page_Down" => "Page_Down", + // GTK's `NamedKey::BackTab`; TUI/X11 spell it "ISO_Left_Tab". + "BackTab" => "ISO_Left_Tab", + other => other, + } +} + +/// The shared terminal (PTY) keyboard rung — one implementation, both backends +/// (#758 / #734 slice 3). +/// +/// Returns `true` when the focused terminal claimed the key, in which case the +/// caller must stop dispatching and repaint. Returns `false` when no terminal +/// has focus, leaving the key to the rungs below. +/// +/// `key_name` may be spelled in either backend's dialect — see +/// [`canonical_terminal_key_name`]. `unicode` is the resolved character (for +/// Ctrl combos, the *unshifted* letter, matching both backends' translation +/// layers). +/// +/// The engine decides *what* the key means +/// ([`Engine::handle_terminal_key`](crate::core::Engine::handle_terminal_key)); +/// this function performs the side effects that used to be duplicated in the +/// backends — clipboard read/write through the engine's own callbacks, the PTY +/// write, and the follow-up poll. +pub fn route_terminal_key( + engine: &mut Engine, + key_name: &str, + unicode: Option, + ctrl: bool, + shift: bool, + alt: bool, +) -> bool { + use crate::core::engine::TerminalKeyAction; + + if !engine.terminal_has_focus { + return false; + } + + let canon = canonical_terminal_key_name(key_name); + match engine.handle_terminal_key(canon, unicode, ctrl, shift, alt) { + TerminalKeyAction::CopySelection => { + let text = engine.active_terminal().and_then(|t| t.selected_text()); + if let Some(ref text) = text { + if let Some(ref cb) = engine.clipboard_write { + let _ = cb(text); + } + engine.message = "Copied".to_string(); + } + } + TerminalKeyAction::PasteClipboard => { + // System clipboard first, then the `+` and unnamed registers — + // the fallback chain TUI had and GTK never did. + let paste_text = engine + .clipboard_read + .as_ref() + .and_then(|cb| cb().ok()) + .filter(|t| !t.is_empty()) + .or_else(|| { + engine + .registers + .get(&'+') + .map(|(t, _)| t.clone()) + .filter(|t| !t.is_empty()) + }) + .or_else(|| { + engine + .registers + .get(&'"') + .map(|(t, _)| t.clone()) + .filter(|t| !t.is_empty()) + }); + if let Some(text) = paste_text { + engine.terminal_paste(&text); + } else { + engine.message = "Nothing to paste".to_string(); + } + } + TerminalKeyAction::SendToPty(data) => { + engine.terminal_write(&data); + engine.poll_terminal(); + } + TerminalKeyAction::Handled | TerminalKeyAction::Ignore => {} + } + true +} + +/// The shared "window resized → resize the PTYs" rung (#758 / #734 slice 3). +/// +/// `panel_cols` is the *whole* terminal panel's width in cells. +/// +/// `Engine::terminal_resize` resizes **every** pane to `panel_cols`, which is +/// right for tabs and wrong for a split: the two visible panes are painted at +/// roughly half the panel width each, so resizing them to the full width +/// reflows their contents off the painted area — the resize half of **#471**. +/// This router keeps a split's per-pane widths, honouring an in-progress +/// divider drag (`terminal_split_left_cols`) when one is set. +pub fn route_terminal_resize(engine: &mut Engine, panel_cols: u16, rows: u16) { + let panel_cols = panel_cols.max(2); + if engine.terminal_split && engine.terminal_panes.len() >= 2 { + let left = if engine.terminal_split_left_cols > 0 { + engine + .terminal_split_left_cols + .clamp(1, panel_cols.saturating_sub(1)) + } else { + panel_cols / 2 + }; + let right = panel_cols.saturating_sub(left).max(1); + engine.terminal_panes[0].session.resize(left, rows); + engine.terminal_panes[1].session.resize(right, rows); + // Panes 3+ are hidden tabs; they get the full panel width they will + // be painted at once the split closes. + for slot in engine.terminal_panes.iter_mut().skip(2) { + slot.session.resize(panel_cols, rows); + } + } else { + engine.terminal_resize(panel_cols, rows); + } +} + // ─── Chrome mouse rung (#752 / #733 slice 2) ───────────────────────────────── // // The rung directly beneath [`route_modal_overlay_click`]: once no modal @@ -22392,6 +22577,59 @@ mod tests { assert!(!e.terminal_split, "second toggle should disable split"); } + /// #758 / #471 (resize half): a window resize must not reflow a *split* + /// terminal's panes to the full panel width. + /// + /// `Engine::terminal_resize` — what `UiEvent::WindowResized` used to call + /// directly — resizes every pane to the panel width, so both halves of a + /// split silently became as wide as the whole panel while still being + /// painted into their narrow rects; anything typed past the painted + /// boundary wrapped out of view. `route_terminal_resize` splits the width + /// instead. Asserts on the PTY's own reported `cols()`, which is what the + /// painter reads back (`build_terminal_panel` -> `TerminalPanel:: + /// content_cols`). + #[test] + fn route_terminal_resize_keeps_a_split_split() { + let mut e = test_engine("hello\n"); + e.terminal_new_tab(80, 24); + e.terminal_toggle_split(80, 24); + assert!(e.terminal_split, "fixture must be in split mode"); + assert_eq!(e.terminal_panes.len(), 2, "split must have two panes"); + + route_terminal_resize(&mut e, 100, 24); + assert_eq!( + ( + e.terminal_panes[0].session.cols(), + e.terminal_panes[1].session.cols() + ), + (50, 50), + "a 100-column panel must be split between the two panes, not \ + handed to each of them in full (#471)" + ); + + // An in-progress divider drag pins the left width; the right pane + // takes the remainder. + e.terminal_split_set_drag_cols(30); + route_terminal_resize(&mut e, 100, 24); + assert_eq!( + ( + e.terminal_panes[0].session.cols(), + e.terminal_panes[1].session.cols() + ), + (30, 70), + "a dragged divider width must survive a resize" + ); + + // Not split: every pane gets the whole panel, as before. + e.terminal_split_set_drag_cols(0); + e.terminal_close_split(100, 24); + route_terminal_resize(&mut e, 90, 24); + assert!( + e.terminal_panes.iter().all(|s| s.session.cols() == 90), + "outside split mode every pane still takes the full panel width" + ); + } + /// Tab drag and drop between groups creates a new split. #[test] fn test_behavior_tab_drag_drop_creates_split() { diff --git a/src/tui_main/shell_app.rs b/src/tui_main/shell_app.rs index 17cb7509..3f41a6cc 100644 --- a/src/tui_main/shell_app.rs +++ b/src/tui_main/shell_app.rs @@ -516,7 +516,64 @@ impl TuiShellApp { /// `tui_main::run()` currently does before entering raw mode /// (`mod.rs:641`-`:678`) — none of it needs a terminal or backend. pub fn new(file_path: Option) -> Self { - let mut engine = Engine::new(); + Self::from_engine(Engine::new(), file_path, true) + } + + /// Test-only deterministic twin of [`TuiShellApp::new`]. + /// + /// [`TuiShellApp::new`] is **ambient in two places at once**, and both of + /// them move the editor pane's geometry: + /// + /// 1. `Engine::new()` reads the developer's real + /// `~/.config/vimcode/{settings,session}.json` off disk, and uses + /// `session.explorer_visible || settings.explorer_visible_on_startup` + /// to decide whether to `app_shell.hide_sidebar()` before it returns + /// (#615/#634 — see [`tests::app_with_sidebar_open`]'s doc comment for + /// the five tests that already learned this the hard way). A machine + /// that has ever opened the explorer boots the sidebar *visible*; a + /// fresh checkout or CI runner boots it *hidden*, shifting every + /// editor column by `SIDEBAR_WIDTH`. + /// 2. `Engine::startup(None)` then calls `restore_session_files()`, which + /// reopens whatever files/splits are listed in the *per-workspace* + /// session file for the process's `current_dir()` — so even the number + /// of editor *windows* is machine-dependent. + /// + /// Any test that measures painted editor geometry (`find_bounds` on a + /// fixture line, `style_at` on a specific column) must therefore build the + /// app from in-memory defaults instead of inheriting the developer's box. + /// + /// Both halves need an explicit fix, and swapping the engine constructor + /// alone only fixes (1): + /// + /// * (1) is `Engine::new_for_test()`, which substitutes in-memory + /// `Settings::default()` / `SessionState::default()` for the two global + /// config reads. + /// * (2) is **not** covered by that. `restore_session_files()` performs its + /// own independent `SessionState::load_for_workspace(&self.cwd)` disk + /// read, keyed on `current_dir()` rather than on whatever state the + /// engine was built from, and unlike its `save_for_workspace` counterpart + /// it has no `cfg(test)` stub (some tests legitimately assert a written + /// workspace session *is* restored). Running the test binary from a + /// checkout that has a real `~/.config/vimcode/sessions/.json` — + /// entirely plausible for a self-hosting editor — would therefore restore + /// that session's files and splits, pushing `windows.len()` past 1. So + /// this constructor routes through + /// [`Engine::startup_without_session_restore`] instead. + /// + /// Everything else runs the *same* `from_engine` body as the production + /// constructor, so the two cannot drift. + #[cfg(test)] + pub fn new_for_test() -> Self { + Self::from_engine(Engine::new_for_test(), None, false) + } + + /// Shared body of [`TuiShellApp::new`] and [`TuiShellApp::new_for_test`] — + /// everything except *where the engine's initial state came from*. + /// + /// `restore_session` is `true` for the production constructor and `false` + /// for [`TuiShellApp::new_for_test`]; see that method for why skipping the + /// per-workspace session restore is required for determinism. + fn from_engine(mut engine: Engine, file_path: Option, restore_session: bool) -> Self { let msv_metrics = quadraui::MsvLayoutMetrics { header_size: 1.0, divider_size: 0.0, @@ -542,7 +599,11 @@ impl TuiShellApp { engine.settings.use_nerd_fonts = false; } icons::set_nerd_fonts(engine.settings.use_nerd_fonts); - engine.startup(file_path.as_deref()); + if restore_session { + engine.startup(file_path.as_deref()); + } else { + engine.startup_without_session_restore(file_path.as_deref()); + } setup_tui_clipboard(&mut engine); let pending_startup_msg = if nerd_font_missing { @@ -2353,26 +2414,43 @@ impl ShellApp for TuiShellApp { }, ) } - // ── Bracketed paste (mirrors mod.rs:3032-:3035) ───────── + // ── Bracketed paste (#758 / #734 slice 3) ─────────────── // The runner maps crossterm's `Event::Paste` to // `UiEvent::ClipboardPaste`; without this arm a paste into - // the TUI is silently dropped. + // the TUI is silently dropped. The rung itself is already + // shared: `Engine::route_paste` is the one focus-priority + // paste router both backends call (GTK's identical arm is in + // `gtk/mod.rs`'s `UiEvent::ClipboardPaste`), and its terminal + // branch now delegates the bracketed-paste decision to + // quadraui's `TerminalSession::paste` (quadraui#343/#415) + // instead of wrapping unconditionally. The extra + // `sync_tui_clipboard` is TUI-only by necessity: crossterm + // has no clipboard, so the `+` register is the backing store + // (GTK reads the real system clipboard). UiEvent::ClipboardPaste(ref text) => { self.engine.route_paste(text); sync_tui_clipboard(&mut self.engine, &mut self.last_clipboard_content); Reaction::Redraw } - // ── Resize → PTY resize (mirrors mod.rs:3036-:3045) ───── + // ── Resize → PTY resize (#758 / #734 slice 3) ─────────── // The runner already debounces the crossterm resize burst // (`RESIZE_SETTLE`) and re-reads the real terminal size for // painting every frame, so only the embedded shell's own // SIGWINCH needs forwarding here. The legacy loop's // accompanying `terminal.clear()` has no shell-runner // equivalent — see the Ctrl+L note in `handle_key_pressed`. + // + // `render::route_terminal_resize` rather than + // `Engine::terminal_resize`: the latter resizes *every* pane + // to the full panel width, which reflows a split's + // half-width panes off the area they are painted into (#471). UiEvent::WindowResized { viewport } => { let term_rows = self.engine.session.terminal_panel_rows; - self.engine - .terminal_resize(viewport.width as u16, term_rows); + render::route_terminal_resize( + &mut self.engine, + viewport.width as u16, + term_rows, + ); Reaction::Redraw } // #602 (gap 2): dispatch through the legacy `mouse::handle_mouse` @@ -3748,82 +3826,17 @@ fn handle_key_pressed( return Reaction::Redraw; } - // ── Terminal (PTY) key routing (#351, mirrors mod.rs:2439-:2513) ──── - // The engine decides the action; the backend performs the clipboard I/O - // and the PTY writes. - if engine.terminal_has_focus { - use crate::core::engine::TerminalKeyAction; - let mut tui_fn_buf = String::new(); - let (kn, uc) = match key_event.code { - KeyCode::Enter => ("Return", None), - KeyCode::Backspace => ("BackSpace", None), - KeyCode::Esc => ("Escape", None), - KeyCode::Tab => ("Tab", None), - KeyCode::BackTab => ("ISO_Left_Tab", None), - KeyCode::Up => ("Up", None), - KeyCode::Down => ("Down", None), - KeyCode::Left => ("Left", None), - KeyCode::Right => ("Right", None), - KeyCode::Home => ("Home", None), - KeyCode::End => ("End", None), - KeyCode::Delete => ("Delete", None), - KeyCode::Insert => ("Insert", None), - KeyCode::PageUp => ("Page_Up", None), - KeyCode::PageDown => ("Page_Down", None), - KeyCode::F(n) => { - tui_fn_buf = format!("F{n}"); - (tui_fn_buf.as_str(), None) - } - KeyCode::Char(c) => ("", Some(c)), - _ => ("", None), - }; - let shift = key_event.modifiers.contains(KeyModifiers::SHIFT); - let alt = key_event.modifiers.contains(KeyModifiers::ALT); - match engine.handle_terminal_key(kn, uc, ctrl, shift, alt) { - TerminalKeyAction::CopySelection => { - let text = engine.active_terminal().and_then(|t| t.selected_text()); - if let Some(ref text) = text { - if let Some(ref cb) = engine.clipboard_write { - let _ = cb(text); - } - engine.message = "Copied".to_string(); - } - } - TerminalKeyAction::PasteClipboard => { - let paste_text = engine - .clipboard_read - .as_ref() - .and_then(|cb| cb().ok()) - .filter(|t| !t.is_empty()) - .or_else(|| { - engine - .registers - .get(&'+') - .map(|(t, _)| t.clone()) - .filter(|t| !t.is_empty()) - }) - .or_else(|| { - engine - .registers - .get(&'"') - .map(|(t, _)| t.clone()) - .filter(|t| !t.is_empty()) - }); - if let Some(text) = paste_text { - engine.terminal_write(b"\x1b[200~"); - engine.terminal_write(text.as_bytes()); - engine.terminal_write(b"\x1b[201~"); - engine.poll_terminal(); - } else { - engine.message = "Nothing to paste".to_string(); - } - } - TerminalKeyAction::SendToPty(data) => { - engine.terminal_write(&data); - engine.poll_terminal(); - } - TerminalKeyAction::Handled | TerminalKeyAction::Ignore => {} - } + // ── Terminal (PTY) key routing (#758 / #734 slice 3, #351) ───────── + // One shared rung, `render::route_terminal_key`, replaces the ~75-line + // block that used to live here (and the GTK twin the #540 cutover had + // deleted outright). It also lets `translate_key`'s own `key_name` reach + // the PTY encoder: the old block bypassed it and re-derived names from + // the raw `KeyCode`, because `translate_key` spells shifted navigation + // keys `Shift_Up`/`Shift_Return` — `canonical_terminal_key_name` strips + // that prefix (shift is passed separately) so the bypass is unnecessary. + let shift = key_event.modifiers.contains(KeyModifiers::SHIFT); + let alt = key_event.modifiers.contains(KeyModifiers::ALT); + if render::route_terminal_key(engine, &key_name, unicode, ctrl, shift, alt) { return Reaction::Redraw; } @@ -5564,43 +5577,106 @@ mod tests { /// /// Same probe-a-swept-cell's-*style* technique as the GTK twin (`CLAUDE.md` /// testing rule 1: assert on rendered output, not on state). + /// + /// # Why this test is built the way it is + /// + /// It measures painted *editor geometry*, which puts it squarely in the + /// #615/#634 blast radius, and the first two attempts at it were red on + /// other machines while green here. Three separate ambient inputs had to + /// go before it was reproducible anywhere: + /// + /// 1. **Ambient engine state.** `TuiShellApp::new(None)` runs the real + /// `Engine::new()` (reads `~/.config/vimcode/{settings,session}.json`, + /// and hides the sidebar unless that session says otherwise) *and* + /// `Engine::startup(None)` → `restore_session_files()` (reopens the + /// developer's own files and splits). Sidebar visible vs hidden alone + /// moves every editor column by `SIDEBAR_WIDTH`; a restored split moves + /// the pane outright. [`TuiShellApp::new_for_test`] is the fix — see + /// its doc comment. + /// 2. **The sidebar-width settle.** `driver_with_shell` paints frame 1 + /// straight from the [`config`] helper, which leaves quadraui's generic + /// 20-column `default_sidebar_width` in place rather than mirroring + /// `TuiShellApp::shell_config`'s #634 clamp to `SIDEBAR_WIDTH`. The + /// end-of-dispatch `set_sidebar_width(self.sidebar_width)` sync in + /// `handle()` re-widens it on the first event of *any* kind, so a + /// column measured off frame 1 is stale from frame 2 onwards. The + /// `Escape` below settles it before anything is measured. + /// 3. **Wall-clock double-click detection.** `TuiDriver::click` is a bare + /// `MouseDown` (no release), and `mouse.rs`'s editor arm promotes a + /// second `MouseDown` to `engine.mouse_double_click` when it lands on + /// the *same cell* within `Duration::from_millis(400)` of the first. + /// Parking the cursor and then pressing on that same cell therefore + /// raced real time: fast machine → word-select-then-extend, loaded + /// machine → two plain clicks. The park click below is deliberately one + /// cell left of the drag press so `last_click_pos` differs and + /// `is_double` is `false` regardless of how long the two dispatches + /// take (the same "don't race the 400ms detector" lesson quadraui#592 + /// baked into `TuiDriver::double_click`). + /// + /// The assertion sweeps the whole dragged span rather than one hardcoded + /// probe column, so it states the property under test ("some cell the drag + /// swept changed how it paints") instead of a guess about which cell the + /// selection lands on. #[test] fn tui_editor_text_drag_paints_a_selection_through_the_shared_drag_router() { - let mut app = TuiShellApp::new(None); + // (1) Deterministic engine state — no ambient settings/session. + let mut app = TuiShellApp::new_for_test(); let mut text = String::new(); for i in 0..40 { text.push_str(&format!("line {i} content that is reasonably long\n")); } app.engine.buffer_mut().insert(0, &text); + assert_eq!( + app.engine.windows.len(), + 1, + "setup sanity: this test measures editor-pane geometry, so it needs \ + exactly one unsplit window — `new_for_test` must not have restored \ + an ambient session's splits" + ); let mut driver = driver_with_shell(app, config(), 100, 24); + // (2) Settle the sidebar width before measuring anything. + driver.press_named(quadraui::NamedKey::Escape); + let bounds = driver .find_bounds("line 5 content") .expect("the fixture line should be painted"); + let row = bounds.y as u16; let row_y = bounds.y + bounds.height / 2.0; + let park_x = bounds.x; let start_x = bounds.x + 1.0; - let probe_x = (bounds.x + 6.0) as u16; let end_x = bounds.x + 12.0; + let swept: Vec = (start_x as u16 + 1..=end_x as u16).collect(); + assert!( + !swept.is_empty(), + "setup sanity: the drag must sweep at least one cell" + ); - // Park the cursor on the row first (a plain click, not a drag) so the - // "before" sample already includes any cursor-line highlight — the - // only thing left for the gesture below to change is the selection. - driver.click(start_x, row_y); - let before = driver.style_at(probe_x, row_y as u16); + // (3) Park the cursor on the row first — one cell *left* of the drag + // press, so the press below can never be promoted to a double-click. + // The "before" sample then already includes any cursor-line highlight, + // leaving the selection as the only thing the gesture can change. + driver.click(park_x, row_y); + let before: Vec<_> = swept.iter().map(|&x| driver.style_at(x, row)).collect(); - // Press to the left of the probe and drag past it while held — the - // press arms `DragTarget::TextSelection` on the shared `DragState`, - // and the very next `Drag` event is the one the regression swallowed. + // Press to the left of the swept span and drag across it while held — + // the press arms `DragTarget::TextSelection` on the shared + // `DragState`, and the very next `Drag` event is the one the + // regression swallowed. driver.mouse_down(start_x, row_y); driver.mouse_move(end_x, row_y); driver.mouse_up(end_x, row_y); - let after = driver.style_at(probe_x, row_y as u16); + let after: Vec<_> = swept.iter().map(|&x| driver.style_at(x, row)).collect(); assert_ne!( - before, after, - "a held drag across the editor text must repaint the swept cell \ - with the selection style; both probes read {before:?} at \ - column {probe_x}, row {row_y}" + before, + after, + "a held drag across the editor text must repaint at least one \ + swept cell with the selection style, but columns {:?} of row \ + {row} paint identically before and after the drag ({before:?}); \ + screen:\n{}", + swept, + driver.screen() ); } @@ -5744,6 +5820,138 @@ mod tests { ); } + // ── #758 / #734 slice 3: the shared terminal (PTY) keyboard rung ─────── + + /// TUI half of `terminal_ctrl_f_opens_the_painted_find_bar` (`gtk/ + /// testing.rs`): with the terminal focused, Ctrl+F must open the + /// *terminal's* find bar, and subsequent characters must land in that + /// bar's query — all the way through `driver_with_shell` -> + /// `TuiShellApp::handle` -> `handle_key_pressed` -> + /// `render::route_terminal_key` -> `Engine::handle_terminal_key`. + /// + /// Asserts on rendered output (`CLAUDE.md` rule 1): the terminal + /// toolbar's painted `" FIND: …"` text + /// (`render::build_terminal_toolbar`), not `terminal_find_active`. A test + /// on the flag would pass against a backend that flipped it while the + /// tab strip still painted — the #587/#592 failure shape. + /// + /// **Verified RED against unfixed `develop`:** deleting the + /// `render::route_terminal_key` call from `handle_key_pressed` makes + /// Ctrl+F fall through to `Engine::handle_key`, which opens the + /// *editor*'s find/replace overlay instead; the `"FIND:"` assertion + /// fires. (This rung existed on TUI before the slice — it is GTK that + /// had none — so the removed-fix control is the router call itself.) + #[test] + fn terminal_ctrl_f_opens_the_painted_find_bar_via_shell_app() { + let mut app = TuiShellApp::new(None); + // `terminal_new_tab` opens the panel and focuses it. + app.engine.terminal_new_tab(80, 10); + + let mut driver = driver_with_shell(app, config(), 80, 24); + driver.render(); + assert!( + !driver.screen_contains("FIND:"), + "precondition: the terminal toolbar starts as a tab strip; screen:\n{}", + driver.screen() + ); + + driver.ctrl_char('f'); + driver.render(); + assert!( + driver.screen_contains("FIND:"), + "Ctrl+F with the terminal focused must open the terminal find bar, \ + not the editor find/replace overlay; screen:\n{}", + driver.screen() + ); + + driver.type_char('z'); + driver.render(); + assert!( + driver.screen_contains("FIND: z"), + "characters typed after Ctrl+F must reach the terminal find query \ + through the shared router; screen:\n{}", + driver.screen() + ); + } + + /// A focused terminal must swallow ordinary keys so they never reach the + /// editor buffer — the divergence that made GTK unusable (there, `x` ran + /// vim's delete-char on the file while the user thought they were typing + /// into a shell). Stated once here for TUI so the pair is symmetric, and + /// so the router's `false` return (the "no terminal focus" path) is + /// covered too. + /// + /// Asserts on the painted buffer text with a positive control: clearing + /// `terminal_has_focus` and repeating the *identical* key must delete the + /// character, so a fixture whose text simply could not change would fail + /// the second half. + #[test] + fn focused_terminal_swallows_editor_keys_via_shell_app() { + // Same fixture twice, differing only in `terminal_has_focus`. + let build = |focused: bool| { + let mut app = TuiShellApp::new(None); + app.engine.buffer_mut().insert(0, "ZQXWTERM758\n"); + app.engine.terminal_new_tab(80, 6); + app.engine.terminal_has_focus = focused; + driver_with_shell(app, config(), 80, 24) + }; + + let mut driver = build(true); + driver.render(); + assert!( + driver.screen_contains("ZQXWTERM758"), + "precondition: the buffer line must paint; screen:\n{}", + driver.screen() + ); + + driver.type_char('x'); + driver.render(); + assert!( + driver.screen_contains("ZQXWTERM758"), + "`x` with the terminal focused must go to the PTY, not delete a \ + character from the editor buffer; screen:\n{}", + driver.screen() + ); + + // Positive control: the same key on the same fixture, terminal + // unfocused, must edit — so a buffer that simply could not change + // would fail here. + let mut control = build(false); + control.render(); + control.type_char('x'); + control.render(); + assert!( + control.screen_contains("QXWTERM758") && !control.screen_contains("ZQXWTERM758"), + "control: with the terminal unfocused `x` must delete the first \ + character; screen:\n{}", + control.screen() + ); + } + + /// The `Shift_`-prefixed names `translate_key` hands the editor + /// (`Shift_Up`, `Shift_Return`, …) have no PTY encoding — which is why + /// the old TUI arm bypassed `translate_key` and re-derived names from the + /// raw crossterm `KeyCode`. `render::canonical_terminal_key_name` strips + /// the prefix (shift travels as its own argument) and reconciles the two + /// backends' spellings of the same physical keys, so the bypass is gone. + #[test] + fn canonical_terminal_key_name_reconciles_both_backends_spellings() { + use crate::render::canonical_terminal_key_name as canon; + // TUI's editor-facing shift prefix. + assert_eq!(canon("Shift_Up"), "Up"); + assert_eq!(canon("Shift_Return"), "Return"); + // GTK's `NamedKey` spellings vs TUI's / X11's. + assert_eq!(canon("PageUp"), "Page_Up"); + assert_eq!(canon("PageDown"), "Page_Down"); + assert_eq!(canon("BackTab"), "ISO_Left_Tab"); + assert_eq!(canon("Enter"), "Return"); + // Already-canonical names and bare characters pass through. + assert_eq!(canon("Page_Up"), "Page_Up"); + assert_eq!(canon("ISO_Left_Tab"), "ISO_Left_Tab"); + assert_eq!(canon("F5"), "F5"); + assert_eq!(canon("a"), "a"); + } + // ── #754 (mouse ladder slice 4: panels) ──────────────────────────────── /// The bottom panel's shared tab strip must switch which panel is