From 734cb4c13a4aa2e2ba1ae201f9fcf905d227d3ee Mon Sep 17 00:00:00 2001 From: alehel Date: Mon, 2 Nov 2026 12:26:10 +0100 Subject: [PATCH] test(ui): the page guards read past a test module in the middle of a file The page guards (pointer reads, cursor writes, and presses) left a file's tests out by cutting it at its first `#[cfg(test)] mod`. `edit_text_ui/image.rs` has a test module at line 231, so its lines 245-1116 were never read: `crop_input`, `image_input` and the picture hit tests among them. A hit test called there, a router write or a raw pointer read passed every guard. `outside_test_modules` now leaves out each test module up to its closing brace, and the guards read the rest. Nothing in the newly read code breaks a rule. A unit test pins the scanner, and a bare `crop_hit(` planted in `image_input` now fails the press guard (it passed before). The guard's messages and docs, CLAUDE.md's "Page presses" line, and the docs of `page_tool_input`, `PageHits::hits` and `PageRouter::cancel` now say what the code does: modules are told what a press is to their page (`PagePress`), and `PageHits::hits` is the one function that asks the hit tests. `comments::context_hit(` joins the hit tests a press handler must not call. Co-Authored-By: Claude Opus 5.5 --- CLAUDE.md | 2 +- crates/ui-egui/src/canvas/page_input.rs | 12 ++--- .../ui-egui/src/canvas/page_pointer/tests.rs | 50 +++++++++++++++---- crates/ui-egui/src/canvas/page_router.rs | 6 ++- 4 files changed, 50 insertions(+), 20 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 7b1d7f5023..47a80b86bc 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -23,7 +23,7 @@ PdfCraft is a clean-room, open-source, Rust-native PDF application targeting Ado - **Layering.** Nothing below L7 depends on egui/winit/eframe/rfd (`plan/architecture.md` §3). `cos`/`filters`/`crypt`/`arlington` stay standalone. - **Page input.** Page modules read `PagePointer` (`crates/ui-egui/src/page_pointer.rs`), never egui's raw pointer state (`hover_pos`, `interact_pos`, `latest_pos`, `press_origin`, button reads); `page_pointer::tests::page_modules_read_the_pointer_only_through_page_pointer` enforces it. - **Page cursor.** Page modules claim the cursor for what is under the pointer (`view.cursor.claim(Layer::…, icon)`, `crates/ui-egui/src/canvas/page_cursor.rs`), never set it (`set_cursor_icon`, `cursor_icon =`, `on_hover_cursor`); `document_area` sets it once, from the claim on the highest layer the active tool reaches. `page_pointer::tests::page_modules_claim_the_cursor_and_never_set_it` enforces it. -- **Page presses.** A page module decides what of its own is under a point in one hit test, which returns a `Hit` (owner, layer, cursor, tip; `crates/ui-egui/src/canvas/page_router.rs`) and gives its cursor, its tip and its press. It acts on a press only when the router says the press is its own (`view.router.owner(page)`), never by testing the press point again or by asking another module whether it wants it. Escape cancels any press the pages hold (`cancel_on_escape`), and that press is ignored until the button is up. `page_pointer::tests::page_modules_act_on_a_press_only_through_the_router` enforces it. +- **Page presses.** A page module decides what of its own is under a point in one hit test, which returns a `Hit` (owner, layer, cursor, tip; `crates/ui-egui/src/canvas/page_router.rs`) and gives its cursor, its tip and its press. It acts on a press only when the router says the press is its own: the page view tells it what the press is to its page (`PagePress`: its own, nobody's, or another page's), and it never tests the press point again or asks another module whether it wants it. Escape cancels any press the pages hold (`cancel_on_escape`), and that press is ignored until the button is up. `page_pointer::tests::page_modules_act_on_a_press_only_through_the_router` enforces it. - **Fidelity.** The PDF object graph is the model. Preserve unknown data. Saves are incremental unless a full rewrite is required. Never silently drop or repair data without recording it. - **Never panic.** Every PDF, script, MCP call, settings file and keystroke is untrusted input, and none of it may crash the app or lose the user's work. This outranks feature work. Return `Result` (the crate's error enum) and propagate with `?`, or fall back leniently and record it. In non-test code: - No `unwrap`/`expect`/`panic!`/`unreachable!`/`todo!` unless it is provably infallible, with a comment saying why. diff --git a/crates/ui-egui/src/canvas/page_input.rs b/crates/ui-egui/src/canvas/page_input.rs index 38c20196d9..a2f71d97c8 100644 --- a/crates/ui-egui/src/canvas/page_input.rs +++ b/crates/ui-egui/src/canvas/page_input.rs @@ -35,9 +35,9 @@ pub(crate) struct PageToolContext<'a> { pub info: &'a DocInfo, } -/// Comments: tools, selection, moving and resizing come before text selection. Form fields take -/// clicks first with the Select tool (as Acrobat fills fields in every viewing mode); then -/// comments; then text selection. +/// The tools' input on one page. The hits at the press point decide who holds the press, and each +/// module is told what the press is to this page (`PagePress`): its own, nobody's (the active tool's, +/// through `tool_input`), or another page's. Text selection runs after, on what nobody took. pub(super) fn page_tool_input(ui: &mut egui::Ui, view: &mut DocView, frame: &mut PageFrame<'_>, slot: &PageSlot) -> PageInput { let PageEnv { added, @@ -391,9 +391,9 @@ impl PageHits<'_> { /// The hits at `p`, bottom up: links, comments; Edit text's pictures under its paragraphs (text /// on top of artwork takes the press), added content, the objects selected together and the - /// open paragraph's frame, the selected picture's grips over them all; then form fields - /// (Prepare's, being moved, over the comments; filled ones under added content and the - /// comments that move). + /// open paragraph's frame, the selected picture's grips over them all; the camera tool's corners; + /// then form fields (Prepare's, being moved, over the comments; filled ones under added content + /// and the comments that move). In crop mode, only the picture being cropped. /// For a right-click (`context`), comments by [`comments::context_hit`]. fn hits(&self, view: &DocView, p: Pos2, context: bool) -> Vec { let PageHits { diff --git a/crates/ui-egui/src/canvas/page_pointer/tests.rs b/crates/ui-egui/src/canvas/page_pointer/tests.rs index 48bb408bad..69abae12cf 100644 --- a/crates/ui-egui/src/canvas/page_pointer/tests.rs +++ b/crates/ui-egui/src/canvas/page_pointer/tests.rs @@ -264,6 +264,25 @@ fn uses(line: &str, name: &str) -> bool { line.match_indices(name).any(|(at, _)| name.starts_with('.') || !line[..at].ends_with(|c: char| c.is_alphanumeric() || c == '_')) } +/// The lines of `source` outside its test modules, numbered from 1. Tests drive egui's input and +/// read its output themselves. A test module (`#[cfg(test)]` then `mod name {`) ends at the first +/// line that is a lone `}`, as rustfmt lays a top-level item out, so the code after a test module +/// in the middle of a file is still read; `#[cfg(test)]` then `mod name;` is one line. +fn outside_test_modules(source: &str) -> Vec<(usize, &str)> { + let mut kept = Vec::new(); + let mut lines = source.lines().enumerate().map(|(n, line)| (n + 1, line)).peekable(); + while let Some((n, line)) = lines.next() { + if line == "#[cfg(test)]" && lines.peek().is_some_and(|(_, next)| next.starts_with("mod ")) { + if lines.next().is_some_and(|(_, module)| module.ends_with('{')) { + lines.by_ref().find(|(_, line)| *line == "}"); + } + continue; + } + kept.push((n, line)); + } + kept +} + /// Each name `find` finds in a line of the page modules' code (their tests left out), as (file, /// the top-level function or `impl` method it is in, line, name). fn page_module_matches(find: impl Fn(&str) -> Vec) -> Vec<(&'static str, Option, usize, String)> { @@ -271,11 +290,9 @@ fn page_module_matches(find: impl Fn(&str) -> Vec) -> Vec<(&'static str, let mut found = Vec::new(); for file in PAGE_MODULES { let source = std::fs::read_to_string(src.join(file)).expect("page module").replace("\r\n", "\n"); - // Tests drive egui's input and read its output themselves. - let code = source.split("#[cfg(test)]\nmod ").next().unwrap_or_default(); // The top-level function, or method of an `impl`, that each line is in. let (mut function, mut in_impl) = (None, false); - for (n, line) in code.lines().enumerate() { + for (n, line) in outside_test_modules(&source) { if line.starts_with("impl") { (function, in_impl) = (None, true); } else if line == "}" { @@ -287,7 +304,7 @@ fn page_module_matches(find: impl Fn(&str) -> Vec) -> Vec<(&'static str, continue; } for name in find(line) { - found.push((file, function.map(str::to_owned), n + 1, name)); + found.push((file, function.map(str::to_owned), n, name)); } } } @@ -414,8 +431,9 @@ fn hit_test_calls(line: &str) -> Vec { .collect() } -/// The functions that call a hit test: (file, function, why). Only `PageHits::at` asks at the -/// press point; the others ask where the pointer is. +/// The functions that call a hit test: (file, function, why). `PageHits::hits` is the one: the hits +/// at the press point decide its owner, and those under the pointer give its cursor, its tip, hover +/// painting and a right-click. const HIT_TESTERS: [(&str, &str, &str); 1] = [( "canvas/page_input.rs", "hits", @@ -431,11 +449,12 @@ const PRESS_IGNORERS: [(&str, &str, &str); 1] = /// The module hit tests, called by path: a press handler calls none of them (HIT_TESTERS says who /// may). -const PAGE_HIT_CALLS: [&str; 11] = [ +const PAGE_HIT_CALLS: [&str; 12] = [ "link_ui::hit(", "prepare::hit(", "forms_ui::hit(", "comments::hit(", + "comments::context_hit(", "camera_ui::hit(", "edit_text_ui::hit(", "edit_text_ui::picture_hit(", @@ -461,7 +480,7 @@ fn routed_press_handler(code: &str) -> bool { /// so what the cursor promised is what the press does. Where each module tested the press /// point again, or asked another whether it wanted the press, the answers disagreed: a press on /// one check box released over the next ticked the second, and a drag could be taken over -/// halfway. Only `PageHits::at` asks the hit tests at the press point. Escape is the one way to +/// halfway. Only `PageHits::hits` asks the hit tests. Escape is the one way to /// drop a press and ignore it until the button is up. #[test] fn page_modules_act_on_a_press_only_through_the_router() { @@ -469,7 +488,7 @@ fn page_modules_act_on_a_press_only_through_the_router() { &ROUTER_WRITES, &ROUTER_WRITERS, "decides, keeps or drops the press the pages hold. Only the hits at the press point decide its owner, once \ - (src/canvas/page_router.rs): a page module reads `view.router.owner(page)`. Drop a press with \ + (src/canvas/page_router.rs), and the page view tells each module what the press is to its page (`PagePress`). Drop a press with \ `DocView::cancel_pointer_gestures`. A new place that must? Add the function to ROUTER_WRITERS in \ src/canvas/page_pointer/tests.rs, with why.", "no longer decides, keeps or drops the press: remove it from ROUTER_WRITERS.", @@ -499,8 +518,8 @@ fn page_modules_act_on_a_press_only_through_the_router() { problems.extend(misuses_in( page_module_matches(hit_test_calls), &HIT_TESTERS, - "calls a hit test. The hits at the press point decide its owner once, in `PageHits::at`: act on the press when \ - `view.router.owner(page)` says it is yours, rather than testing its point again. Testing where the pointer \ + "calls a hit test. The hits at the press point decide its owner once, in `PageHits::hits`: act on the press when \ + the page view tells you it is yours (`PagePress::Mine`), rather than testing its point again. Testing where the pointer \ is (a tooltip, a click released over what it was pressed on)? Add the function to HIT_TESTERS in \ src/canvas/page_pointer/tests.rs, with why.", "calls no hit test any more: remove it from HIT_TESTERS.", @@ -516,6 +535,15 @@ fn page_modules_act_on_a_press_only_through_the_router() { assert!(problems.is_empty(), "{}", problems.join("\n")); } +/// A test module in the middle of a file hides only itself from the page guards: the code after +/// it is read too. The guards used to stop at the first test module. +#[test] +fn the_page_guards_read_past_a_test_module_in_the_middle_of_a_file() { + let source = "fn before() {}\n#[cfg(test)]\nmod tests {\n fn inside() {}\n}\n\nfn after() {}\n#[cfg(test)]\nmod more;\nfn last() {}\n"; + let kept: Vec<(usize, &str)> = outside_test_modules(source); + assert_eq!(kept, [(1, "fn before() {}"), (6, ""), (7, "fn after() {}"), (10, "fn last() {}")]); +} + #[test] fn the_page_press_guard_rejects_missing_owners_and_second_hit_tests() { assert!(routed_press_handler("let owner = view.router.owner(page);")); diff --git a/crates/ui-egui/src/canvas/page_router.rs b/crates/ui-egui/src/canvas/page_router.rs index 23eae443e9..447289ccd0 100644 --- a/crates/ui-egui/src/canvas/page_router.rs +++ b/crates/ui-egui/src/canvas/page_router.rs @@ -191,7 +191,8 @@ impl PageRouter { } } - /// Drops the press (Escape, a dialog, automation's cancel). + /// Drops the press (Escape, a dialog, automation's cancel): every page is the active tool's again + /// ([`PagePress::Free`]). Page input skips a press Escape cancelled until the button is up. pub(crate) fn cancel(&mut self) { self.capture = None; self.began = None; @@ -250,7 +251,8 @@ mod tests { assert_eq!((r.press_for(1), r.press_for(2)), (PagePress::Free, PagePress::Free)); r.begin(); r.cancel(); - assert_eq!(r.press_for(0), PagePress::Free, "a cancelled press is nobody's"); + // Page input skips a press Escape cancelled until the button is up (`press_cancelled`). + assert_eq!(r.press_for(0), PagePress::Free, "a cancelled press leaves every page to the tool"); } #[test]