From 0378eee68798bc89922326912b09fdc6868f7589 Mon Sep 17 00:00:00 2001 From: alehel Date: Tue, 3 Nov 2026 08:18:26 +0100 Subject: [PATCH] test(ui): page modules never read the router; the compiler holds it With the Edit text group, the text layer and link following told what the press is to their page, only the page view reads the router. - The router's methods and `DocView.router` are `pub(in crate::canvas)`: outside `canvas` the compiler refuses a read or a write, aliases included, which the guard's text match could miss. `ROUTER_READERS` keeps only the page view's dispatcher (`page_tool_input`). - Rule (f): no function in a page module is named with "grabs" or "owns", which decide for themselves whether a press is theirs. - CLAUDE.md's "Page presses" rule says so: modules never read the router; they are told. Each rule seen failing against a violation planted in `#[cfg(any())]` code (a `router.keeps_drag()` read in `content_ui`, a `fn fill_grabs` in `fill_sign`), and a real router read from `content_ui` no longer compiles. Co-Authored-By: Claude Opus 5.5 --- CLAUDE.md | 2 +- crates/ui-egui/src/canvas.rs | 5 +-- .../ui-egui/src/canvas/page_pointer/tests.rs | 32 ++++++++++++++++--- crates/ui-egui/src/canvas/page_router.rs | 14 ++++---- 4 files changed, 38 insertions(+), 15 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 5c7034b7a4..4dbeaae1ac 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 press is its own or, for the active tool, nobody's: the page view tells it which (`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. +- **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 press is its own or, for the active tool, nobody's: the page view tells it which (`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. Modules never read the router; they are told. 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.rs b/crates/ui-egui/src/canvas.rs index 14f9a92545..6433e7e092 100644 --- a/crates/ui-egui/src/canvas.rs +++ b/crates/ui-egui/src/canvas.rs @@ -650,8 +650,9 @@ pub struct DocView { pub(crate) signature_drag: crate::signature_drag::SignatureDrag, /// What the page modules claim for the cursor this frame; `document_area` sets it once. pub(crate) cursor: crate::page_cursor::PageCursor, - /// Who the press the pages hold belongs to. - pub(crate) router: crate::page_router::PageRouter, + /// Who the press the pages hold belongs to. Only the page view reads it: it tells each page + /// module what the press is to its page (`PagePress`). + pub(in crate::canvas) router: crate::page_router::PageRouter, /// What the last right-click on a page went to, and on which page: the page's menu shows its /// entries, or the page's own when it went to nothing that has a menu. pub(crate) menu_for: Option<(usize, crate::page_router::Owner)>, diff --git a/crates/ui-egui/src/canvas/page_pointer/tests.rs b/crates/ui-egui/src/canvas/page_pointer/tests.rs index a3a3d8e882..69b86dcdc0 100644 --- a/crates/ui-egui/src/canvas/page_pointer/tests.rs +++ b/crates/ui-egui/src/canvas/page_pointer/tests.rs @@ -525,9 +525,10 @@ fn hit_test_calls(line: &str) -> Vec { /// The functions that call a hit test: (file, function, why). Hit tests are found by name (a /// function named `hit` or `…_hit`, called by path, bare or as a method). The comments' point test -/// has a list of its own (`COMMENT_POINT_TESTERS`); the others by other names (added content's, -/// Edit text's paragraphs', Redact's text test, the text layer's links) are not listed yet, and -/// most are PR 4b's. `PageHits::hits` is the one: the hits +/// has a list of its own (`COMMENT_POINT_TESTERS`). Point tests by other names are not seen: what is +/// left of them paints (a picture's hover outline) or keeps a behaviour no hit stands for (added +/// text's click with the tools that don't pick it up, crop mode's controls). `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] = [( @@ -571,14 +572,24 @@ const COMMENT_POINT_TESTERS: [(&str, &str, &str); 3] = [ const ROUTER_READS: [&str; 4] = ["router.press_for(", "router.keeps_drag(", "PageRouter::press_for(", "PageRouter::keeps_drag("]; /// The functions that read who holds the press: (file, function, why). The page view asks once per -/// page and tells each module what the press is to its page (`PagePress`); an unused entry fails -/// the test. +/// page and tells each module what the press is to its page (`PagePress`): modules never read the +/// router, and outside `canvas` the compiler refuses them (its methods are `pub(in crate::canvas)`). +/// An unused entry fails the test. const ROUTER_READERS: [(&str, &str, &str); 1] = [( "canvas/page_input.rs", "page_tool_input", "the page view: what the press is to each page (`press_for`), and whether its drag is the text's (`keeps_drag`)", )]; +/// Words a page module's functions are not named with: a function that grabs or owns a press decides +/// for itself whether the press is its own, which the page view tells it (`PagePress`). +const PRESS_CLAIMING_WORDS: [&str; 2] = ["grabs", "owns"]; + +/// The function `line` declares, when its name has one of [`PRESS_CLAIMING_WORDS`] in it. +fn press_claiming_fn(line: &str) -> Vec { + declares(line.trim_start()).filter(|name| PRESS_CLAIMING_WORDS.iter().any(|w| name.contains(w))).map(str::to_owned).into_iter().collect() +} + /// What ignores the press the pages hold until the button is up. const PRESS_IGNORES: [&str; 1] = ["press_cancelled = true"]; @@ -668,6 +679,13 @@ fn page_modules_act_on_a_press_only_through_the_router() { place that must ask? Add the function to PAGE_HITS_ASKERS in src/canvas/page_pointer/tests.rs, with why.", "no longer asks the page view's hits: remove it from PAGE_HITS_ASKERS.", )); + // A module is told what the press is to its page; none decides it for itself. + problems.extend(page_module_matches(press_claiming_fn).into_iter().map(|(file, _, n, name)| { + format!( + "src/{file}:{n}: `fn {name}` decides whether a press is its module's. The page view tells each module what the \ + press is to its page (`PagePress`): act on that, in a function named for what it does." + ) + })); problems.extend(misuses( &PRESS_IGNORES, &PRESS_IGNORERS, @@ -726,6 +744,10 @@ fn the_page_guards_read_hit_tests_split_calls_and_aliases() { assert!(hit_test_calls("let all = self.hits(view, p, false);").is_empty()); assert_eq!(declares("pub(crate) const fn crop_hit<'a>(view: &DocView) {"), Some("crop_hit")); assert_eq!(declares("let hit = 1;"), None); + assert_eq!(press_claiming_fn(" pub(crate) fn fill_grabs(view: &DocView) -> bool {"), ["fill_grabs"]); + assert_eq!(press_claiming_fn("fn owns_press() {"), ["owns_press"]); + assert!(press_claiming_fn("fn holds(&self, page: usize) -> bool {").is_empty()); + assert!(press_claiming_fn("let grabs = 1;").is_empty()); let source = "impl PageHits<'_> {\n fn at(&self) {\n self\n .hits(view, p, false)\n }\n}\nfn after() {\n // view.router.owner(0)\n let _ = view\n .router\n .owner(0);\n}\n"; assert_eq!( code_lines(source), diff --git a/crates/ui-egui/src/canvas/page_router.rs b/crates/ui-egui/src/canvas/page_router.rs index 5f4a08a56d..b1ec7ff10a 100644 --- a/crates/ui-egui/src/canvas/page_router.rs +++ b/crates/ui-egui/src/canvas/page_router.rs @@ -182,7 +182,7 @@ pub(crate) struct PageRouter { impl PageRouter { /// A new press on the pages: the last one's owner goes, and until a page claims it the press is /// between the pages. - pub(crate) fn begin(&mut self) { + pub(in crate::canvas) fn begin(&mut self) { self.capture = None; self.began = Some(None); } @@ -192,7 +192,7 @@ impl PageRouter { /// by nobody (the page's text, or a tool that acts anywhere on the page). /// Only a press just begun ([`Self::begin`]) is claimed, by the first page that does: pages /// that touch (the gaps hidden) can share a row of pixels, and a press there is one page's. - pub(crate) fn press(&mut self, page: usize, hits: impl IntoIterator, tool: QuickTool) { + pub(in crate::canvas) fn press(&mut self, page: usize, hits: impl IntoIterator, tool: QuickTool) { if self.began != Some(None) { return; } @@ -201,7 +201,7 @@ impl PageRouter { } /// What `page` has to do with the press the pages hold. - pub(crate) fn press_for(&self, page: usize) -> PagePress { + pub(in crate::canvas) fn press_for(&self, page: usize) -> PagePress { match (self.began, &self.capture) { (None, _) => PagePress::Free, (Some(Some(p)), Some(capture)) if p == page => PagePress::Mine(capture.owner), @@ -212,21 +212,21 @@ impl PageRouter { /// Whether the press is held by someone who keeps its drag: then the page's text is not /// selected by it. - pub(crate) fn keeps_drag(&self) -> bool { + pub(in crate::canvas) fn keeps_drag(&self) -> bool { self.capture.as_ref().is_some_and(|c| !c.declines_drag) } /// The press turned into a drag: an owner that declines its drag lets it go, and from then on /// the press is nobody's (a drag across a highlight selects the text under it; with the Link /// tool, a drag from a link draws a new one). Its click stayed the owner's. - pub(crate) fn drag_started(&mut self) { + pub(in crate::canvas) fn drag_started(&mut self) { if self.capture.as_ref().is_some_and(|c| c.declines_drag) { self.capture = None; } } /// The press is over once the button is up and its release has been seen. - pub(crate) fn settle(&mut self, any_down: bool, edges: bool) { + pub(in crate::canvas) fn settle(&mut self, any_down: bool, edges: bool) { if !any_down && !edges { self.capture = None; self.began = None; @@ -235,7 +235,7 @@ impl PageRouter { /// Drops the press (Escape, 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) { + pub(in crate::canvas) fn cancel(&mut self) { self.capture = None; self.began = None; }