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 <noreply@anthropic.com>
This commit is contained in:
alehelandClaude Opus 5.5 committed 2026-11-02 20:39:07 +01:00
1 parent ed8a10e83e
commit 734cb4c13a
4 files changed
+50 -20

No files matched your search

+1 -1
View File
@@ -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.
+6 -6
View File
@@ -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<crate::page_router::Hit> {
let PageHits {
+39 -11
View File
@@ -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<String>) -> Vec<(&'static str, Option<String>, usize, String)> {
@@ -271,11 +290,9 @@ fn page_module_matches(find: impl Fn(&str) -> Vec<String>) -> 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<String>) -> 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<String> {
.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);"));
+4 -2
View File
@@ -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]