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 <noreply@anthropic.com>
This commit is contained in:
alehelandClaude Opus 5.5 committed 2026-11-02 20:39:35 +01:00
1 parent f0caaf9259
commit c1d34fc9ea
8 files changed
+105 -23

No files matched your search

+6 -2
View File
@@ -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());
@@ -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<String> {
}
/// 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,
+4 -3
View File
@@ -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<Capture>,
/// 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<Option<usize>>,
}
+2 -2
View File
@@ -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<Hit> {
if view.camera_done.is_some() || !xf.rect.contains(p) {
return None;
+7 -4
View File
@@ -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),
+1 -1
View File
@@ -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])
}
+24 -6
View File
@@ -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<usize> {
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
+27
View File
@@ -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");
}