From 2a4032b2fc54652a8b0dae1a07002cb0028b64c3 Mon Sep 17 00:00:00 2001 From: alehel Date: Mon, 2 Nov 2026 18:38:01 +0100 Subject: [PATCH] fix(ui): a press where two pages touch is one page's; the guard sees PageHits::hits Found in review: - **A press where two pages touch.** With the page gaps hidden, pages can share a row of pixels, and a press there was claimed by both: the first page acted on it, then the second took it over, and the text layer could count it twice. Only a press just begun is claimed now, by the first page that does. This was so on `main` too. - **The guard** listed who asks `PageHits::at` and `context_at`, but not `PageHits::hits`, which they wrap. A new call of it in `page_input.rs` now fails, and the guard's doc says what it still can't see. - **Docs.** `Hit::declines_drag` (a link's drag with the Link tool), `over_mark`, and when the Select tool still unselects under another tool's frame. Tests: two pages claiming one press, seen failing without the fix. A planted call of `PageHits::hits` fails the guard. The router's unit tests now begin a press before claiming it, as `draw_pages` does. Co-Authored-By: Claude Opus 5.5 --- crates/ui-egui/src/canvas/page_input.rs | 5 +++-- .../ui-egui/src/canvas/page_pointer/tests.rs | 14 ++++++++----- crates/ui-egui/src/canvas/page_router.rs | 21 ++++++++++++++++++- crates/ui-egui/src/tools/comments/input.rs | 4 ++-- 4 files changed, 34 insertions(+), 10 deletions(-) diff --git a/crates/ui-egui/src/canvas/page_input.rs b/crates/ui-egui/src/canvas/page_input.rs index 647a39d970..39b39e11b0 100644 --- a/crates/ui-egui/src/canvas/page_input.rs +++ b/crates/ui-egui/src/canvas/page_input.rs @@ -236,8 +236,9 @@ fn link_of(owner: Option) -> Option { /// stamps, Fill & Sign, Sign area, zoom and snapshot, the camera's quadrilateral and crop act only /// on a press that began on this page and that no hit won (`press` is Free), and keep their cursor /// and gestures. The camera also drags a corner whose press it holds, and crop's double-click -/// opens Set Page Boxes wherever it is. `over_mark`: a Fill & Sign mark takes the press, or would take one under the -/// pointer. +/// opens Set Page Boxes wherever it is. `over_mark`: with Fill & Sign, a hit holds the press (a +/// mark's, or a Prepare field's), or with no button down a mark lies under the pointer, where a +/// press would pick it up. fn tool_input( ui: &egui::Ui, view: &mut DocView, diff --git a/crates/ui-egui/src/canvas/page_pointer/tests.rs b/crates/ui-egui/src/canvas/page_pointer/tests.rs index 2890a63cb1..4936c81367 100644 --- a/crates/ui-egui/src/canvas/page_pointer/tests.rs +++ b/crates/ui-egui/src/canvas/page_pointer/tests.rs @@ -453,13 +453,17 @@ const HIT_TESTERS: [(&str, &str, &str); 1] = [( "`PageHits::hits`: every module's hits at a point, for the pointer's cursor and tip, a right-click and, once, the press's owner", )]; -/// The page view's asks for every module's hits at a point (`PageHits::at`, `PageHits::context_at`). -const PAGE_HITS_ASKS: [&str; 2] = [".at(", ".context_at("]; +/// The page view's asks for every module's hits at a point (`PageHits::hits`, and `at` and +/// `context_at`, which wrap it). +const PAGE_HITS_ASKS: [&str; 3] = [".hits(", ".at(", ".context_at("]; /// The functions that ask the page view's hits at a point: (file, function, why). `PageHits` is -/// private to `page_input.rs`, so this keeps a new place there from testing a point again; a -/// second ask inside one of these is for review to catch. -const PAGE_HITS_ASKERS: [(&str, &str, &str); 3] = [ +/// private to `page_input.rs`, so this keeps a new function there from asking by these names; a +/// call by path (`PageHits::at(&hits, …)`) and a second ask inside a listed function are for +/// review to catch. +const PAGE_HITS_ASKERS: [(&str, &str, &str); 5] = [ + ("canvas/page_input.rs", "at", "`PageHits::at`: the hits at a point, as a press there would take them"), + ("canvas/page_input.rs", "context_at", "`PageHits::context_at`: what a right-click at a point is on"), ( "canvas/page_input.rs", "page_tool_input", diff --git a/crates/ui-egui/src/canvas/page_router.rs b/crates/ui-egui/src/canvas/page_router.rs index b59aee58be..0863642aee 100644 --- a/crates/ui-egui/src/canvas/page_router.rs +++ b/crates/ui-egui/src/canvas/page_router.rs @@ -90,7 +90,7 @@ pub(crate) struct Hit { /// Beside the pointer where this hit wins (a link's target). pub tip: Option, /// A drag from here is not this owner's: a click on a highlight selects it, a drag across it - /// selects the text underneath. + /// selects the text underneath; with the Link tool, a drag from a link draws a new one. pub declines_drag: bool, } @@ -159,7 +159,12 @@ impl PageRouter { /// A press on `page`: `hits` at the press point decide who it belongs to. A press nobody's /// hit claims is held 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) { + if self.began != Some(None) { + return; + } self.capture = top(hits, tool).map(|h| Capture { owner: h.owner, declines_drag: h.declines_drag }); self.began = Some(Some(page)); } @@ -222,6 +227,7 @@ mod tests { #[test] fn a_press_is_held_until_the_button_is_up_and_its_release_seen() { let mut r = PageRouter::default(); + r.begin(); r.press(2, [hit(Layer::Comment, 7)], QuickTool::Select); assert_eq!(r.owner(2), Some(hit(Layer::Comment, 7).owner)); assert_eq!(r.owner(1), None, "only on its page"); @@ -230,6 +236,7 @@ mod tests { assert!(r.owner(2).is_some(), "the release frame still belongs to the press"); r.settle(false, false); assert_eq!(r.owner(2), None); + r.begin(); r.press(0, [], QuickTool::Select); assert_eq!(r.owner(0), None, "a press nobody claims is held by nobody"); assert!(!r.keeps_drag()); @@ -257,9 +264,21 @@ mod tests { assert_eq!(r.press_for(0), PagePress::Free, "a cancelled press leaves every page to the tool"); } + /// Pages that touch (the gaps hidden) can share a row of pixels: a press there is the first + /// page's to claim it, and the next page's claim is ignored. + #[test] + fn a_press_where_two_pages_touch_is_the_first_pages() { + let mut r = PageRouter::default(); + r.begin(); + r.press(0, [hit(Layer::Comment, 1)], QuickTool::Select); + r.press(1, [hit(Layer::Comment, 2)], QuickTool::Select); + assert_eq!((r.press_for(0), r.press_for(1)), (PagePress::Mine(hit(Layer::Comment, 1).owner), PagePress::Elsewhere)); + } + #[test] fn a_highlight_lets_its_drag_go_to_the_text() { let mut r = PageRouter::default(); + r.begin(); r.press(0, [Hit { declines_drag: true, ..hit(Layer::Comment, 0) }], QuickTool::Select); assert!(r.owner(0).is_some() && !r.keeps_drag()); r.cancel(); diff --git a/crates/ui-egui/src/tools/comments/input.rs b/crates/ui-egui/src/tools/comments/input.rs index 8366e36453..4c4251907b 100644 --- a/crates/ui-egui/src/tools/comments/input.rs +++ b/crates/ui-egui/src/tools/comments/input.rs @@ -13,8 +13,8 @@ use crate::page_router::{CommentPart, Hit, Owner, PagePress}; /// tool, Fill & Sign over its marks, a shape tool over the selected shape's handles) is that /// comment's, and one nobody holds is a commenting tool's. When another tool took this frame /// (`taken`: Prepare, added content, which still decide for themselves), the commenting tools leave -/// it to that tool, as before, though a click there still unselects. Returns `true` when the -/// pointer gesture belongs to commenting (text selection must ignore it). +/// it to that tool, as before; the Select tool's click there still unselects. Returns `true` when +/// the pointer gesture belongs to commenting (text selection must ignore it). pub(crate) fn page_input(ui: &egui::Ui, resp: &egui::Response, cx: &PageCx<'_>, press: PagePress, taken: bool, view: &mut DocView) -> bool { let (pointer, origin) = (cx.ptr.pos, cx.ptr.origin); let page_rect = cx.xf.rect;