#758: Converge keyboard dispatch slice 3: terminal / PTY key routing (#351) - #775
Merged
JDonaghy merged 4 commits intoSep 3, 2026
Merged
Conversation
… both backends `render::route_terminal_key` states the rung once; both backends call it and have nothing bespoke left. `render::route_terminal_resize` does the same for the "window resized → resize the PTYs" half. **quadraui first (`CLAUDE.md`, and the issue's own note).** quadraui#343 has landed on the pinned rev (`TerminalSession::paste` / `bracketed_paste_enabled`), so the bracketed-paste rung is now an *adoption*: `Engine::terminal_paste` delegates to it instead of wrapping unconditionally, which stops literal `[200~` bytes leaking into programs that don't strip them. quadraui#342 (the keyboard→PTY encoder) has **not** landed, so the encoder stays in `core::engine::terminal_ops::key_to_pty_bytes` — platform-neutral `core`, not per-backend code. When #342 lands, that fn is the single call site to swap and neither backend changes. **What diverged.** TUI hand-rolled ~75 lines inside `handle_key_pressed`. GTK's twin was deleted outright by the #540 Relm4→ShellApp cutover — the whole `if engine.borrow().terminal_has_focus { … }` block lived in the `view!`'s `EventControllerKey` closure and nothing replaced it. Since #540, **every key typed into a focused terminal on GTK fell through to `Engine::handle_key` and ran a vim command on the editor buffer** (#471): Ctrl+F opened the editor's find/replace overlay, `x` deleted a character from the file. The other half of #471 was that old arm's `sender.input(Msg::Resize)` after each keypress, whose handler resized *every* pane to the full panel width — in split mode that reflowed the half-width panes while they were still painted narrow, so freshly typed text in the right pane wrapped out of view. The router performs no resize, and `route_terminal_resize` is split-aware, so neither half can return. Also reconciled: `canonical_terminal_key_name` accepts both backends' spellings (`PageUp`/`Page_Up`, `BackTab`/`ISO_Left_Tab`, `Enter`/`Return`) and strips the `Shift_` prefix `translate_key` adds for the editor — which is why the old TUI arm bypassed `translate_key` and re-derived names from the raw `KeyCode`. That bypass is gone. The GTK rung sits directly below the focus owners and *above* the debug F-keys, matching TUI's ladder: `vim`/`htop` inside the terminal must get F5 themselves. Black-box coverage, both backends, asserting on rendered output only: - `terminal_ctrl_f_opens_the_painted_find_bar{,_via_shell_app}` — the painted `" FIND: …"` toolbar text (`render::build_terminal_toolbar`), then the typed query reaching it. Never `terminal_find_active`. - `focused_terminal_swallows_editor_keys{_on_gtk,_via_shell_app}` — the painted buffer line, with a positive control (same fixture, terminal unfocused, the identical key must delete the character). - `route_terminal_resize_keeps_a_split_split` — the PTYs' own `cols()`, which is what the painter reads back. - `canonical_terminal_key_name_reconciles_both_backends_spellings`. **Verified RED against unfixed `develop`** (`CLAUDE.md` rule 2): with the two `route_terminal_key` calls and the `route_terminal_resize` body removed, all five behaviour tests fail; restored, all pass. Entry-point production lines (`handle_key_press` + `dispatch_gtk_panel_ accelerator` + `handle_key_pressed` + `handle_focus_owner_key` + `dispatch_panel_accelerator_sizeless`): **1,423 → 1,389**, a strict decrease even though GTK *gained* a rung it did not have. `cargo build && cargo clippy -- -D warnings && cargo fmt --check` EXIT=0. Beyond the issue's file list: `core/engine/terminal_ops.rs` (new `Engine::terminal_paste`) and `core/engine/keys.rs` (`route_paste`'s terminal branch now calls it) — required for the quadraui#343 adoption. Refs #758, #734, #351, #471, quadraui#342, quadraui#343. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ag test `tui_editor_text_drag_paints_a_selection_through_the_shared_drag_router` measured its probe column off `driver_with_shell`'s *first* painted frame and only clicked afterwards. That frame is painted straight from the test `config()` helper, which leaves quadraui's generic 20-column `default_sidebar_width` in place instead of mirroring `TuiShellApp::shell_config`'s #634 clamp of `SIDEBAR_WIDTH` (30); the end-of-dispatch `set_sidebar_width` sync in `handle()` then widens the sidebar on the first event of any kind, so every frame-1 column is 10 cells left of where the same content sits from frame 2 onwards. The stale probe only *diverged* when the sidebar's own content differed across those 10 columns, which is why it read as a flake: the explorer lists whatever `std::env::current_dir()` holds, and `core::engine::tests::test_open_folder_resets_cwd` leaves the process cwd pointing at an empty temp dir (`Engine::open_folder` calls `std::env::set_current_dir`). Under that test order the probe landed on blank sidebar background, whose style a drag cannot change, and the assertion fired — reproducible with cargo test --lib -- --test-threads=1 --exact \ core::engine::tests::test_open_folder_resets_cwd \ tui_main::shell_app::tests::tui_editor_text_drag_paints_a_selection_through_the_shared_drag_router Fix: dispatch a no-op Escape before measuring, so every coordinate comes from the settled layout. Re-verified the test is still RED against the unfixed router (`armed_target: drag_state.is_active()`), so it has not been weakened into a test that cannot fail. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The reviewer is right that cd6e840 was not a real fix: it adjusted only the test's setup, and the reported failure mode ("both probes same non-selection style") is not what a stale-column probe produces. This replaces it with a fix to the actual fragility. What was NOT the bug: the drag-routing production code. `armed_target: render::drag_state_arms_scrollbar(drag_state)` is already in place on both backends (src/tui_main/mouse.rs:816, src/gtk/mod.rs:3483), and reverting just that one expression back to `drag_state.is_active()` still turns this test RED — re-verified this round, then restored. So the #756-review regression the test exists to pin is genuinely fixed; the test itself was the thing that could not be trusted. What WAS wrong: the test measured painted editor geometry off a `TuiShellApp::new(None)`, which is ambient in three separate ways — exactly the "green on the author's box, red elsewhere" class this repo already documented in #615/#634: 1. `Engine::new()` reads the developer's real `~/.config/vimcode/{settings,session}.json` and decides sidebar visibility from it (visible vs hidden shifts every editor column by SIDEBAR_WIDTH), and `Engine::startup(None)` then calls `restore_session_files()`, which reopens whatever files and splits that session lists — so even the editor window count was machine-dependent. `tests::app_with_sidebar_open`'s own doc comment says anything measuring editor geometry "must pin this itself rather than inherit it"; this test did not. 2. `TuiDriver::click` is a bare MouseDown with no release, and mouse.rs's editor arm promotes a second MouseDown on the *same cell* within `Duration::from_millis(400)` to `engine.mouse_double_click`. Parking the cursor and then pressing on that same cell raced wall-clock time: fast machine took the word-select-then-extend path, a loaded one took two plain clicks. Same trap quadraui#592 added `TuiDriver::double_click` to avoid. 3. A single hardcoded probe column asserted a guess about which cell the selection lands on rather than the property under test. Fixes, in order: * `TuiShellApp::new` is refactored into a shared `from_engine` body plus a `#[cfg(test)] new_for_test()` that feeds it `Engine::new_for_test()` — the shell-level twin of the engine-level constructor added for #615. No behaviour change to `new`; the two share one body so they cannot drift. Available to every other geometry-measuring test in this module too. * The test builds from `new_for_test()`, asserts the setup sanity it depends on (exactly one unsplit window), parks the cursor one cell *left* of the drag press so `is_double` is deterministically false no matter how slow the machine, and asserts across the whole swept span instead of one column. The Escape sidebar-width settle from cd6e840 is kept (that mechanism is real) but its rationale is folded into the doc comment. Honest caveat: I could not reproduce the reported red on this machine under any of — fresh $HOME with no vimcode config, empty/foreign process cwd, a hostile ambient session.json, the double-click detector forced off, --test-threads=1, or 20+ repeats of the full suite in both lanes. So this removes the whole ambient-input class the symptom points at rather than pinning one confirmed trigger. Gate on this commit, both lanes: cargo build clean, cargo test 0 failed (2512 lib + all integration targets, GUI lane), cargo clippy -- -D warnings and cargo clippy --no-default-features -- -D warnings clean, cargo fmt --check clean. The six `unused import`/`unused variable` warnings in core/engine/tests.rs, core/git.rs and gtk/testing.rs are pre-existing and byte-identical on cd6e840 (verified by stashing) — not from this diff. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…structor
Blocking review finding: `TuiShellApp::new_for_test()` did not actually close
the ambient-disk-state class its own doc comment describes. Swapping in
`Engine::new_for_test()` only fixes problem (1) — the two *global*
`~/.config/vimcode/{settings,session}.json` reads that drive the
sidebar-visible decision. Problem (2) was left live: `from_engine`
unconditionally called `engine.startup(file_path.as_deref())` for both
constructors, and with `file_path == None` that always runs
`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 rather than on the in-memory
`Settings`/`SessionState` the engine was built with. Unlike its
`save_for_workspace` counterpart, `load_for_workspace` has no `cfg(test)`
stub (and must not get one: `test_restore_session_files_opens_separate_tabs`
and `test_restore_session_does_not_bleed_across_workspaces` legitimately
write a workspace session file and assert it *is* restored).
So on any machine that has a real `~/.config/vimcode/sessions/<hash>.json`
saved for the checkout the test binary runs in — entirely plausible for a
self-hosting editor whose developers edit it with itself — `new_for_test()`
still restored that file's open files and splits. Two or more listed files
send `restore_session_files` down its flat-fallback path, `new_tab()`
allocates a fresh `WindowId` per extra file, and `windows.len()` grows past
1 — tripping the setup-sanity assertion in
`tui_editor_text_drag_paints_a_selection_through_the_shared_drag_router` and
leaving that test RED on exactly that class of machine.
Fix (the reviewer's suggested shape): split `Engine::startup` into
`startup` / `startup_without_session_restore` over a shared
`startup_inner(file_path, restore_session)`, and thread a `restore_session`
flag through `TuiShellApp::from_engine` — `true` for `new`, `false` for
`new_for_test`. The deterministic constructor now does only
`plugin_init` / `ext_refresh` / explicit-path-open, so the resulting engine
depends on nothing but in-memory defaults. Production behaviour is
byte-identical: `new` still calls `startup`, and GTK is untouched.
Coverage: `test_startup_without_session_restore_ignores_workspace_session`
in `src/core/engine/tests.rs`. It writes a two-file workspace session for a
temp cwd, then asserts in two halves — a *control* half proving
`restore_session_files()` really does restore that fixture (so a green
subject half means the restore was skipped, not that the fixture was inert),
and the subject half asserting `startup_without_session_restore(None)` adds
no tabs, no windows and no buffers for either listed file.
RED verified: with `startup_without_session_restore` reverted to
`startup_inner(file_path, true)`, the new test fails with
"startup_without_session_restore must not reopen the workspace session's
files, left: 2, right: 1". Restored, it passes.
Gate: `cargo build`, `cargo test` (2513 lib + all integration targets, 0
failed), `cargo clippy -- -D warnings`, `cargo fmt -- --check` all clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
JDonaghy
added a commit
that referenced
this pull request
Sep 6, 2026
Bumps the quadraui pin to develop tip (9eede7f), which carries #775's Theme::from_vscode_json / Color::try_from_hex_over lift, and drops the now-stale vt100 [patch.crates-io] shim quadraui#795 made obsolete upstream (its own Cargo.toml documents the fix; keeping the patch pointed at a rev whose repo root no longer vendors vt100-0.16.2-patched/ broke `cargo check` outright). Replaces vimcode's local `Color` struct+impl with a `pub use quadraui::Color` re-export plus a small `ColorExt` trait and three free functions (`hex`, `try_from_hex`, `try_from_hex_over`) that keep the pre-#775 panic-on-literal / 3-digit-shorthand call sites working unchanged. Deletes `to_q_color` / `to_quadraui_color` (now identity conversions) and their 62 render.rs + 14 backend call sites. Scope note: does NOT delegate `Theme::from_vscode_json`'s `colors` mapping to `quadraui::Theme::from_vscode_json`, and does NOT delete `strip_json_comments` or the theme-directory helpers, despite #829 listing those. quadraui's loader has diverged from vimcode's for its own primitive set (e.g. title_fg <- titleBar.activeForeground, header_bg <- sideBarSectionHeader.background) where vimcode uses different VS Code keys for the conceptually-similar fields (fuzzy_ title_fg <- syntax palette, status_bg <- statusBar.background). A field-name reverse-mapping would silently change which VS Code key feeds which vimcode field — the opposite of #829's "assert rendered chrome colours match before and after" bar. Documented in a comment above `from_vscode_json`; #829 should stay open for that remaining, higher-risk chunk. Internal refactor, no user-visible behavior change: verified via the existing test_try_from_hex/test_lighten_darken/test_from_vscode_json/ test_strip_json_comments unit tests, all 200 render:: tests, all 129 gtk::testing:: tests (incl. the chrome/vscode_dimming/scrollbar paint tests exercising the touched call sites), and 34 tui_main::render_impl tests — all green on both the default (gui) and --no-default-features lanes. cargo fmt / cargo clippy -D warnings clean on both lanes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #758
Automated PR opened by coordinator for review of issue #758.