#785: [retry] #47 Stage 1: move struct App + ShellApp impl out of src/gtk/ into a backend-neutral src/app.rs - #791
Merged
JDonaghy merged 4 commits intoSep 3, 2026
Conversation
Moves `struct App`, all four of its inherent `impl` blocks and
`impl quadraui::ShellApp for App` out of `src/gtk/mod.rs` into a new
top-level `src/app.rs`, together with the three GDK-key -> `UiEvent`
mappers, `setup_gtk_clipboard`, `PendingFileDialog`, and the
`DeferredQueue` / `DeferredAction` / `GtkAccelHost` deferral seam and
`register_panel_accelerators` (all of which are GTK-free despite their
names, and are used only by `App`).
`src/gtk/mod.rs` is left with `run()`, `build_shell_config()` and the
genuinely GTK-only helpers (UI font metrics, close-glyph metrics, tab-bar
pixel-hit conversion, h-scrollbar geometry) plus its own three
`#[cfg(test)]` modules.
src/gtk 9650 -> 2584 (-7066)
src/app.rs 0 -> 7131
total 41406 -> 41471 (+65: module doc + re-stated imports)
Nothing is deleted and no behaviour changes -- this is a pure relocation.
`crate::gtk` re-exports `App` as `pub(crate)`, so `super::App` keeps
resolving inside `click.rs` / `testing.rs`; the items `App` still reaches
for in `crate::gtk::{self, click, css, util}` were promoted from private /
`pub(super)` to `pub(crate)`, and `App`'s fields likewise (the #646
`GtkDriver` harness reads 20+ of them).
`src/app.rs` is still `#[cfg(feature = "gui")]`. Its module doc records
exactly why, so the next stage does not have to re-derive it: four
platform-typed fields (`settings_monitor`, `window`, `css_provider`,
`backend`), ~11 platform hook call sites (glib idle/timeout, CSS provider
reload + `gtk4::Settings` dark preference, window title/size/maximize/
minimize/close, CSD capture, `gtk4::FileDialog`), and a dependency on
`crate::gtk::{click, css, util}` -- whose `build_editor_click_context`,
`load_css` and pixbuf/glib-log helpers mean they cannot be lifted
wholesale without being split first. Per the Platform-Neutrality Rule
those want quadraui-side infrastructure, not new per-backend code here.
Refs #785, #47.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… it) The Test stage reported `breadcrumb_path_paints_dimmer_than_editor_body_text` red, ~1 run in 30 under load, against a #785 diff that is a verified pure move (`struct App` out of `src/gtk/mod.rs` into `src/app.rs`; normalising both trees for comments/imports/rustfmt wrapping shows no logic change). Two independent pre-existing hazards, both reproduced and both fixed: 1. `brightest()` assumes *grayscale* antialiasing — one coverage fraction driving all three channels — so the max-luminance pixel carries the unblended pen. Under subpixel (RGB) antialiasing, which fontconfig enables on this machine, each channel gets its own coverage from a 3-tap LCD filter and no pixel of a 10pt stem saturates all three at once. The same painted rect read (100, 104, 113), (80, 112, 121) and (108, 112, 98) across runs — one pen, 23/255 of spread, against a tolerance of 10. New `channel_ceiling()` takes each channel's maximum independently. That keeps the "a blend can never overshoot the pen" invariant the module leans on while letting different pixels supply different channels. The comparison also becomes asymmetric (+4 / -12) instead of ±10: above the pen is physically impossible and so is the load-bearing half, below is only coverage loss. The two bounds together reject #7f848e at *every* coverage rather than at the one this machine happens to produce — it previously missed by 1/255 on two channels, now by 5, while the correct colour keeps 4 in hand at both ends. 2. `test_open_folder_resets_cwd` / `test_open_workspace_parses_json` leaked the *process* working directory (`Engine::open_folder` chdirs, by design) into every later test in the run. The GTK harness paints a real file explorer rooted at the process CWD, so with it leaked the sidebar renders `VIMCODE_TEST_OPEN_FOLDER` and an empty tree instead of the repo — a different frame around the probe, reached only when the scheduler ran the two in the wrong order. Bisected to that test by name. Both now hold a `CwdGuard` that restores on `Drop` (so a panicking assert can't skip it), with `open_folder_does_not_leak_the_process_cwd` as the regression guard. RED-first, both re-run rather than assumed: reverting `breadcrumb_fg` to `#7f848e` makes the probe read (117, 123, 132) and fail; deleting the guard from the new test makes it fail on the restore assertion. Verification: 80 consecutive full `cargo test --lib` runs under the same 24-way CPU load that reproduced the flake — 0 failures (baseline 1/30, and 1/31 on the first stress attempt). Full `cargo test` (GUI on), `cargo fmt`, `cargo clippy -- -D warnings` and `cargo clippy --no-default-features -- -D warnings` all clean; no new warnings. Also corrects two figures #701 recorded in the module doc that re-measuring disproved — the reverted-colour reading, and the claim that the body-text ratio assertion catches the same regression (it does not; #7f848e measures ~0.535 against a 0.55 ceiling, so the colour assertion is the only catcher). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…aints Test-stage failure: `tab_hover_tooltip_paints_below_tab_row_not_inside_it` red, 1 of 2564, on a diff that touches neither tooltips nor the explorer. Same root cause as the breadcrumb flake fixed in 5feae7b, one layer deeper. `cargo test` runs a target's tests in one process across many threads, and the working directory is per-process. `Engine::open_folder` / `open_workspace` chdir it by design; the GTK harness *reads* it on every frame (explorer root, `ext_panel` relative paths, `buffers.rs`'s fallback base). 5feae7b's `CwdGuard` restored the directory on `Drop`, which closed the leak into tests that started **after** a writer — and did nothing for tests painting **during** the writer's window. That is the same bug reached by a different scheduling order, and it is what went red here: the tooltip test compares two separately-painted frames for pixel equality, and another test's chdir landed between them. Reproduced 15 runs out of 15 with: cargo test --lib -- tab_hover_tooltip_paints_below \ test_lsp_flush_clears_diagnostics_by_canonical_path \ test_open_folder_resets_cwd test_open_workspace_parses_json The paint side was confirmed deterministically rather than inferred: a scratch single-threaded test painting the same engine twice with a `set_current_dir("/tmp")` between the two found the top 200 rows across the full 1400px width differing — exactly the band that assert_eq compares. Fix — new `src/test_cwd.rs` (test-only module): one process-wide `RwLock`. Writers take it exclusively for as long as they hold the directory (`CwdGuard`, moved here, still restoring on `Drop`); readers take it shared (`CwdReadGuard`, held by `gtk::testing::Harness` for its whole lifetime, so every frame it can paint is covered). Readers still run fully in parallel with each other — the only new serialisation is reader-vs-writer, and there are four writers in the suite. Reader acquisition is thread-local reentrant, because `std::sync::RwLock` parks new readers behind a queued writer and holding two harnesses at once is a normal pattern here. Poisoning is ignored, so one panicking test stays one red test. `test_lsp_flush_clears_diagnostics_by_canonical_path` was the fourth writer and had no guard at all — it chdir'd by hand and restored only on the happy path. It now takes `CwdGuard` too; before that it failed *itself* 15/15 in the repro above, resolving its relative path against another test's directory. Coverage (three new tests, each stated RED-first in its doc): - `harness_holds_the_cwd_claim_while_it_can_paint` — the regression guard; fails against this branch's unfixed tree. - `cwd_guard_excludes_readers_for_its_whole_lifetime` - `read_guard_excludes_writers_and_is_reentrant` None assert on *release*, deliberately: another thread may hold the process-wide lock at any instant, so such an assertion would be a new flake, while a guard that never released would hang the suite outright. Verification: 40 consecutive `cargo test --lib` runs under 24-way CPU load, 0 failures and no hangs; the 15/15 repro above now 20/20 green. Full `cargo test` (GUI on) green, `cargo fmt`, `cargo clippy -- -D warnings`, `cargo clippy --no-default-features -- -D warnings` and `cargo clippy --features test-support -- -D warnings` all clean. Test-infrastructure only — no production code changes, so no black-box behaviour test is owed under CLAUDE.md's Testing rule. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The review flagged RefCell/fs/Rc/EngineAction/Theme as dead top-level imports in mod.rs. They weren't actually dead: click.rs, css.rs, and util.rs each declare `use super::*;` and consumed these names through that glob re-export. Deleting them outright (as literally suggested) breaks the build — verified by trying it. Moved each import to the submodule that actually needs it instead: - click.rs: EngineAction, RefCell, Rc (prod code) + Theme (cfg(test) only, gated to avoid an unused-import warning in the default build) - css.rs: Theme - util.rs: fs Net effect matches the review's intent — mod.rs no longer carries imports that look orphaned at that scope — without breaking the `use super::*;` submodules that were quietly relying on them. cargo build --lib and cargo clippy -- -D warnings (the mandatory pre-commit gate) are both clean. All 130 gtk:: tests pass, including the previously-failing tab_hover_tooltip_paints_below_tab_row_not_inside_it. 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 #785
Automated PR opened by coordinator for review of issue #785.