From c1d34fc9eaaf9caaaa3eb1c165870e1a9991e5af Mon Sep 17 00:00:00 2001 From: alehel Date: Mon, 2 Nov 2026 18:11:07 +0100 Subject: [PATCH] fix(ui): commenting tools leave a frame another tool took; Escape keeps the menu's anchor true Found in review: - **A double-click another tool took finished a polygon.** Round 5 kept commenting tools off a press another tool had taken, but a polygon, polyline or cloud finishes on a double-click or Enter whoever has the press. With Prepare open, a double-click on a field opened its properties and committed the polygon being clicked out. Added text opened for editing did the same with the Edit panel open. The commenting tools now skip such a frame entirely, as `main`'s consumed chain did. The comments' own presses and a click's unselecting still run. - **The menu's anchor after Escape.** While a press Escape cancelled is held, the page's tools are skipped, and a right-click left the page menu on the page right-clicked before. It now anchors where it is, as every other right-click does. - **The guard.** It now lists who asks the page view's hits at a point (`PageHits::at` and `context_at`), so a new place in `page_input.rs` can't test a point again. It takes a file that uses the comments' page context (`PageCx`) for a page module. Its doc of the point tests it doesn't list is now true. - **Docs and visibility.** True docs for `PagePress::Free`, where a press is off the pages, and the camera's hit. `link_ui::screen_rect` is private. Tests: a polygon left unfinished by a double-click on a Prepare field, and a right-click after Escape anchoring the menu. Each was seen failing without its fix. A planted ask of the page view's hits, and a planted unlisted file using `PageCx`, each fail the guard. Co-Authored-By: Claude Opus 5.5 --- crates/ui-egui/src/canvas.rs | 8 +++- .../ui-egui/src/canvas/page_pointer/tests.rs | 39 ++++++++++++++++--- crates/ui-egui/src/canvas/page_router.rs | 7 ++-- crates/ui-egui/src/tools/camera_ui.rs | 4 +- crates/ui-egui/src/tools/comments/input.rs | 11 ++++-- crates/ui-egui/src/tools/link_ui.rs | 2 +- crates/ui-egui/tests/it/pointer.rs | 30 +++++++++++--- crates/ui-egui/tests/it/prepare.rs | 27 +++++++++++++ 8 files changed, 105 insertions(+), 23 deletions(-) diff --git a/crates/ui-egui/src/canvas.rs b/crates/ui-egui/src/canvas.rs index 6cb3211c7c..6775702384 100644 --- a/crates/ui-egui/src/canvas.rs +++ b/crates/ui-egui/src/canvas.rs @@ -1498,14 +1498,18 @@ fn draw_page(ui: &mut egui::Ui, view: &mut DocView, frame: &mut PageFrame<'_>, i } let slot = PageSlot { i, r, xf }; - // A cancelled press takes nothing until the button is up. + let pcx = comments::PageCx { page: i, xf: &xf, ptr, info, tool, prefs, allowed, hidden: comments_hidden, redact_mark }; + // A cancelled press takes nothing until the button is up, though the page's menu still opens + // where a right-click is. let input = if view.press_cancelled { + if let Some(p) = pointer.filter(|p| frame.resp.secondary_clicked() && xf.rect.contains(*p)) { + comments::anchor(&pcx, p, view); + } PageInput { consumed: true, area_tool: false, hover: None, released_over: false } } else { page_tool_input(ui, view, frame, &slot) }; let painter = ui.painter(); - let pcx = comments::PageCx { page: i, xf: &xf, ptr, info, tool, prefs, allowed, hidden: comments_hidden, redact_mark }; let preview_target = (matches!(tool, QuickTool::Select | QuickTool::Fill(_)) && !comments_hidden).then_some(view.comments.selected).flatten(); view.signature_drag.prepare(ui.ctx(), doc, preview_target, scale); view.signature_drag.paint(painter, &pcx, &view.comments, view.pending_edit.as_ref()); diff --git a/crates/ui-egui/src/canvas/page_pointer/tests.rs b/crates/ui-egui/src/canvas/page_pointer/tests.rs index 1c41858651..2890a63cb1 100644 --- a/crates/ui-egui/src/canvas/page_pointer/tests.rs +++ b/crates/ui-egui/src/canvas/page_pointer/tests.rs @@ -190,9 +190,11 @@ const PAGE_MODULES: [&str; 37] = [ "tools/stamps_ui.rs", ]; -/// What a file that acts on the pages uses: the pointer, a tool's page context, what a press is to -/// a page, or the router's hits and owners. Such a file is a page module. -const PAGE_MODULE_MARKS: [&str; 4] = ["PagePointer", "PageToolContext", "PagePress", "page_router::"]; +/// What a file that acts on the pages uses: the pointer, a tool's or the comments' page context, +/// what a press is to a page, or the router's hits and owners. Such a file is a page module. One +/// that acts on the pages through the page view's response and a bare point alone uses none of +/// them (`canvas/image_pick.rs`): PAGE_MODULES lists it by hand. +const PAGE_MODULE_MARKS: [&str; 5] = ["PagePointer", "PageToolContext", "PageCx", "PagePress", "page_router::"]; /// The router's own files, which define what page modules use. const ROUTER_FILES: [&str; 3] = ["canvas/page_pointer.rs", "canvas/page_router.rs", "canvas/page_cursor.rs"]; @@ -439,8 +441,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); a point test by another -/// name has a list of its own (`COMMENT_POINT_TESTERS`). `PageHits::hits` is the one: the hits +/// 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 /// 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] = [( @@ -449,6 +453,23 @@ 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 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] = [ + ( + "canvas/page_input.rs", + "page_tool_input", + "the hits at the press point decide its owner, once per press; those under the pointer before the tools act, once a \ + frame, serve a click's release, a right-click and Fill & Sign's preview", + ), + ("canvas/page_input.rs", "claim_under", "what lies under the pointer after the tools act: its cursor, its tip and hover painting"), + ("canvas/page_input.rs", "right_click", "what a right-click is on"), +]; + /// The comments' point test, which their hit tests are built on: which comment lies under a point. const COMMENT_POINT_TESTS: [&str; 1] = [".comment_at("]; @@ -550,6 +571,14 @@ fn page_modules_act_on_a_press_only_through_the_router() { function to COMMENT_POINT_TESTERS in src/canvas/page_pointer/tests.rs, with why.", "no longer asks which comment lies under a point: remove it from COMMENT_POINT_TESTERS.", )); + problems.extend(misuses( + &PAGE_HITS_ASKS, + &PAGE_HITS_ASKERS, + "asks the page view's hits at a point. The hits at the press point decide its owner once, and those under the \ + pointer are taken once before the tools act and once after: use those (`under`, `PageInput.hover`). A new \ + 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.", + )); problems.extend(misuses( &PRESS_IGNORES, &PRESS_IGNORERS, diff --git a/crates/ui-egui/src/canvas/page_router.rs b/crates/ui-egui/src/canvas/page_router.rs index c689f9fde5..b59aee58be 100644 --- a/crates/ui-egui/src/canvas/page_router.rs +++ b/crates/ui-egui/src/canvas/page_router.rs @@ -116,8 +116,9 @@ pub(crate) enum PagePress { /// only let go of their own state (a click on anything but the selected comment unselects it). Mine(Owner), /// No button is down, or the press began on this page and nobody's hit won it: the active - /// tool's. Also every page's while a press that began off the pages is held, and after the - /// press was dropped ([`PageRouter::cancel`]) until the button is up. + /// tool's. Also every page's while a press the page view does not hold is down (one that began + /// over a panel), and after the press was dropped ([`PageRouter::cancel`]) until the button is + /// up. Free, /// The press began on another page, or off the pages (between them, or beside one). Elsewhere, @@ -144,7 +145,7 @@ pub(crate) struct Capture { #[derive(Debug, Default)] pub(crate) struct PageRouter { capture: Option, - /// A press is held, and the page it began on (`None`: between the pages). + /// A press is held, and the page it began on (`None`: off the pages, between them or beside one). began: Option>, } diff --git a/crates/ui-egui/src/tools/camera_ui.rs b/crates/ui-egui/src/tools/camera_ui.rs index c220f8aea3..ce88da25cf 100644 --- a/crates/ui-egui/src/tools/camera_ui.rs +++ b/crates/ui-egui/src/tools/camera_ui.rs @@ -62,8 +62,8 @@ fn screen(xf: &PageXform, c: [f32; 2]) -> Pos2 { /// The corner of the quadrilateral under `p` on `page`, the nearest within reach: the one hit test /// of the camera tool, for the cursor and the press. `None` once the page is enhanced, on a page -/// without the quadrilateral, or off the page (a press there is no page's), though a corner at its -/// edge reaches past it. +/// without the quadrilateral, or off the page (a press there is no page's), even where a corner at +/// its edge would reach past it. pub(crate) fn hit(xf: &PageXform, page: usize, view: &DocView, p: Pos2) -> Option { if view.camera_done.is_some() || !xf.rect.contains(p) { return None; diff --git a/crates/ui-egui/src/tools/comments/input.rs b/crates/ui-egui/src/tools/comments/input.rs index ccb12c7676..8366e36453 100644 --- a/crates/ui-egui/src/tools/comments/input.rs +++ b/crates/ui-egui/src/tools/comments/input.rs @@ -11,13 +11,14 @@ use crate::page_router::{CommentPart, Hit, Owner, PagePress}; /// Comment input on one page, before text selection runs. `press` is what this page has to do with /// the press the pages hold: a press on a comment where the tool reaches it ([`hit`]: the Select /// 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, unless another tool took it this frame -/// (`taken`: Prepare's selection rectangle, added content), though a click there still unselects. -/// Returns `true` when the pointer gesture belongs to commenting (text selection must ignore it). +/// 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). 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; - let free = press == PagePress::Free && !taken; + let free = press == PagePress::Free; let pressed_here = free && origin.is_some_and(|o| page_rect.contains(o)); let over_page = pointer.is_some_and(|p| page_rect.contains(p)); let owned = match press { @@ -37,6 +38,8 @@ pub(crate) fn page_input(ui: &egui::Ui, resp: &egui::Response, cx: &PageCx<'_>, match cx.tool { // The selected shape's handles, and Fill & Sign's marks, take their own presses. QuickTool::Comment(_) | QuickTool::Fill(_) if owned.is_some() || moving => select_input(ui, resp, cx, view, press, pointer, origin), + // Neither a press nor a double-click or a key another tool took this frame. + QuickTool::Comment(_) if taken => false, QuickTool::Comment(tool) if tool.draws() => gestures.draw_shape(view, tool), QuickTool::Comment(tool) if tool.clicks_points() => gestures.click_points(view, tool), QuickTool::Comment(CommentTool::Callout) => gestures.callout(view), diff --git a/crates/ui-egui/src/tools/link_ui.rs b/crates/ui-egui/src/tools/link_ui.rs index 069564afff..041edaf1c7 100644 --- a/crates/ui-egui/src/tools/link_ui.rs +++ b/crates/ui-egui/src/tools/link_ui.rs @@ -49,7 +49,7 @@ pub struct LinkDraft { pub style: LinkStyle, } -pub(crate) fn screen_rect(xf: &PageXform, info: &DocInfo, page: usize, r: [f64; 4]) -> Rect { +fn screen_rect(xf: &PageXform, info: &DocInfo, page: usize, r: [f64; 4]) -> Rect { xf.user_rect(info, page, [r[0] as f32, r[1] as f32, r[2] as f32, r[3] as f32]) } diff --git a/crates/ui-egui/tests/it/pointer.rs b/crates/ui-egui/tests/it/pointer.rs index d412496fcf..1fc7c51dd3 100644 --- a/crates/ui-egui/tests/it/pointer.rs +++ b/crates/ui-egui/tests/it/pointer.rs @@ -1547,11 +1547,9 @@ fn a_long_press_on_a_comment_selects_it_for_its_menu() { h.run_steps(2); } -/// A right-click while the primary button is held anchors the page's menu where it is (Delete page, -/// Paste): the held press leaves only the selections alone. It used to keep the menu on the page -/// right-clicked before. -#[test] -fn a_right_click_with_the_button_held_anchors_the_menu_on_its_page() { +/// With the page menu anchored on page 1, presses on page 2 and holds the button, presses Escape +/// first with `escape`, right-clicks on page 2, and gives the page the menu is anchored on. +fn menu_page_after_a_right_click_with_the_button_held(escape: bool) -> Option { let mut h = open(two_pages_pdf(), "100", false); h.run_steps(2); h.state_mut().views[0].comments.context_at = Some((0, [10.0, 10.0])); @@ -1563,14 +1561,34 @@ fn a_right_click_with_the_button_held_anchors_the_menu_on_its_page() { h.run_steps(1); h.hover_at(press + egui::vec2(40.0, 0.0)); h.run_steps(1); + if escape { + h.key_press(egui::Key::Escape); + h.run_steps(1); + } let menu = press + egui::vec2(100.0, 60.0); for pressed in [true, false] { h.event(egui::Event::PointerButton { pos: menu, button: egui::PointerButton::Secondary, pressed, modifiers: egui::Modifiers::NONE }); } h.run_steps(1); - assert_eq!(h.state().views[0].comments.context_at.map(|(page, _)| page), Some(1), "the menu is page 2's"); + let page = h.state().views[0].comments.context_at.map(|(page, _)| page); h.drop_at(press + egui::vec2(40.0, 0.0)); h.run_steps(2); + page +} + +/// A right-click while the primary button is held anchors the page's menu where it is (Delete page, +/// Paste): the held press leaves only the selections alone. It used to keep the menu on the page +/// right-clicked before. +#[test] +fn a_right_click_with_the_button_held_anchors_the_menu_on_its_page() { + assert_eq!(menu_page_after_a_right_click_with_the_button_held(false), Some(1), "the menu is page 2's"); +} + +/// So does a right-click while a press Escape cancelled is held, though the cancelled press takes +/// nothing else. +#[test] +fn a_right_click_after_escape_cancelled_the_press_anchors_the_menu_on_its_page() { + assert_eq!(menu_page_after_a_right_click_with_the_button_held(true), Some(1), "the menu is page 2's"); } /// With Edit text a right-click on added text reaches comments, as before (Edit text keeps those on diff --git a/crates/ui-egui/tests/it/prepare.rs b/crates/ui-egui/tests/it/prepare.rs index 51999431d9..749eaac69a 100644 --- a/crates/ui-egui/tests/it/prepare.rs +++ b/crates/ui-egui/tests/it/prepare.rs @@ -1617,3 +1617,30 @@ fn with_prepare_open_a_shape_tool_drag_draws_no_comment() { let squares = s.session.get(s.views[0].id).unwrap().info.annotations.iter().filter(|a| a.subtype == "Square").count(); assert_eq!(squares, 0, "no rectangle under Prepare's selection rectangle"); } + +/// With Prepare open, a double-click on a field is the field's (it opens its properties) and leaves +/// a polygon being clicked out unfinished, as before. It used to finish the polygon too. +#[test] +fn with_prepare_open_a_double_click_on_a_field_leaves_the_polygon_unfinished() { + let mut h = harness(); + h.state_mut().execute("form.prepare"); + h.run_steps(3); + assert!(h.state_mut().execute("comment.polygon")); + h.run_steps(3); + // Three points below the fields. + for (x, y) in [(40.0, 260.0), (200.0, 260.0), (120.0, 360.0)] { + let p = at(&h, x, y); + h.hover_at(p); + h.run_steps(1); + h.drag_at(p); + h.run_steps(1); + h.drop_at(p); + h.run_steps(3); + } + let on_field = field_rect(&h, "name").center(); + crate::support::input::double_click(&mut h, on_field); + let s = h.state(); + let polygons = s.session.get(s.views[0].id).unwrap().info.annotations.iter().filter(|a| a.subtype == "Polygon").count(); + assert_eq!(polygons, 0, "the polygon is not finished"); + assert!(matches!(s.views[0].comments.gesture, Some(pdfcraft_ui_egui::comments::Gesture::Draw { .. })), "it is still being clicked out"); +}