fix(ui): the Link tool's click told too; a moved comment keeps its selection

Found in review:
- **The Link tool's click** acted on any press: with Prepare open, a click on a field also
  unselected the selected link, and a click that began off the page did too. It now acts only on
  a press on a link, or one nobody holds.
- **A right-click while a comment is moved** selected the comment under it mid-drag, though the
  release still moved the first. Comments now keep their selection during a move, resize or
  reshape, as Prepare does.
- The hits under the pointer before the tools act are taken only on the pointer's page, and the
  Link tool's right-click reuses them.
- The router has one source for the press's page (`Capture` no longer carries its own).
- The guard lists which functions may ask which comment lies under a point (`comment_at`); the
  guard finds hit tests by name, and its docs say so.
- Docs made true: what `PagePress::Mine` promises, what `tool_input` gates, and that a dialog does
  not cancel the pages' press (only Escape and automation's cancel do).

Tests: the two fixes, and ruling 1 for added text (a click on it unselects the comment), each seen
failing with its fix reverted; the drawing-tool right-click test now checks the comment's menu.

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:25 +01:00
1 parent ae62b66304
commit 47ece03a54
6 files changed
+106 -18

No files matched your search

+12 -7
View File
@@ -99,7 +99,7 @@ pub(super) fn page_tool_input(ui: &mut egui::Ui, view: &mut DocView, frame: &mut
// What lies under the pointer before the tools act, asked once: for a click's release, a
// right-click, and Fill & Sign's preview.
let wants_under = resp.clicked() || resp.secondary_clicked() || matches!(tool, QuickTool::Fill(_)) && !ptr.any_down;
let under = pointer.filter(|_| wants_under).map(|p| hits.at(view, p)).unwrap_or_default();
let under = pointer.filter(|p| wants_under && xf.rect.contains(*p)).map(|p| hits.at(view, p)).unwrap_or_default();
let released_over = match press {
PagePress::Mine(owner) if resp.clicked() => under.iter().any(|h| h.owner == owner),
_ => false,
@@ -227,7 +227,8 @@ fn link_of(owner: Option<Owner>) -> Option<usize> {
/// The active tool's own input on page `i`: the one gate for presses nobody holds. Measure, the
/// 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. `over_mark`: a Fill & Sign mark takes the press, or would take one under the
/// 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.
fn tool_input(
ui: &egui::Ui,
@@ -323,7 +324,7 @@ fn tool_input(
/// selected; links take theirs from what this returns.
/// `under`: the hits at `p` before the tools act.
fn right_click(hits: &PageHits<'_>, under: &[crate::page_router::Hit], view: &mut DocView, p: Pos2) -> Option<Owner> {
let context = hits.context_at(view, p);
let context = hits.context_at(view, p, under);
// Add text takes every right-click on a page it can change, Edit text those on its own
// paragraphs and pictures, the Link tool those on a link, and a Prepare field tool the whole
// page, as before: comments see none of them.
@@ -333,7 +334,10 @@ fn right_click(hits: &PageHits<'_>, under: &[crate::page_router::Hit], view: &mu
QuickTool::Link => hits.can_modify && matches!(context, Some(Owner::Link { .. })),
_ => hits.field_tool.is_some(),
};
if !tool_takes_it {
// A comment being moved, resized or reshaped keeps its selection until its release.
let comment_dragged =
matches!(view.comments.gesture, Some(comments::Gesture::Move { .. } | comments::Gesture::Resize { .. } | comments::Gesture::Reshape { .. }));
if !tool_takes_it && !comment_dragged {
let comment = match context {
Some(Owner::Comment { index, .. }) => Some(index),
_ => None,
@@ -400,11 +404,12 @@ impl PageHits<'_> {
/// tool reaching comments and links, as a comment's menu shows whatever the tool. A highlight
/// over a link is the highlight's ([`comments::context_hit`]).
/// The Link tool edits links and reaches no comment: on a document it can change, its
/// right-click is what its press there would take, when that is anything.
fn context_at(&self, view: &DocView, p: Pos2) -> Option<Owner> {
/// right-click is what its press there would take (`under`, the hits at `p`), when that is
/// anything.
fn context_at(&self, view: &DocView, p: Pos2, under: &[crate::page_router::Hit]) -> Option<Owner> {
if self.tool == QuickTool::Link
&& self.can_modify
&& let Some(press) = top(self.hits(view, p, false), QuickTool::Link)
&& let Some(press) = top(under.iter().cloned(), QuickTool::Link)
{
return Some(press.owner);
}
@@ -407,7 +407,7 @@ const ROUTER_WRITES: [&str; 4] = ["router.begin(", "router.press(", "router.sett
const ROUTER_WRITERS: [(&str, &str, &str); 3] = [
("canvas/page_input.rs", "page_tool_input", "the hits at the press point decide its owner, once per press"),
("canvas.rs", "draw_pages", "the press is over once the button is up and its release seen, and a new one drops the last owner"),
("canvas.rs", "cancel_pointer_gestures", "every pointer gesture's cancel: Escape, a dialog, automation's cancel"),
("canvas.rs", "cancel_pointer_gestures", "every pointer gesture's cancel: Escape, automation's cancel"),
];
/// Ways to make a `Hit`.
@@ -430,7 +430,9 @@ fn hit_test_calls(line: &str) -> Vec<String> {
.collect()
}
/// The functions that call a hit test: (file, function, why). `PageHits::hits` is the one: the hits
/// 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
/// 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] = [(
@@ -439,6 +441,16 @@ 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 comments' point test, which their hit tests are built on: which comment lies under a point.
const COMMENT_POINT_TESTS: [&str; 1] = [".comment_at("];
/// The functions that ask which comment lies under a point: (file, function, why).
const COMMENT_POINT_TESTERS: [(&str, &str, &str); 3] = [
("tools/comments/input.rs", "hit", "the comments' hit test"),
("tools/comments/input.rs", "context_hit", "the comments' hit test for a right-click"),
("tools/comments/paint.rs", "paint_page", "a redaction mark shows its fill under the pointer with every tool, as comments' tips show"),
];
/// What reads who holds the press the pages hold.
const ROUTER_READS: [&str; 3] = ["router.press_for(", "router.owner(", "router.keeps_drag("];
@@ -521,6 +533,14 @@ fn page_modules_act_on_a_press_only_through_the_router() {
Add the function to ROUTER_READERS in src/canvas/page_pointer/tests.rs, with why.",
"no longer reads who holds the press: remove it from ROUTER_READERS.",
));
problems.extend(misuses(
&COMMENT_POINT_TESTS,
&COMMENT_POINT_TESTERS,
"asks which comment lies under a point. The comments' hit tests answer that for a press and a right-click: \
act on what the page view tells you, rather than testing the point again. A new place that must? Add the \
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(
&PRESS_IGNORES,
&PRESS_IGNORERS,
+6 -6
View File
@@ -112,7 +112,8 @@ pub(crate) fn top(hits: impl IntoIterator<Item = Hit>, tool: QuickTool) -> Optio
/// What one page has to do with the press the pages hold.
#[derive(Clone, Copy, Debug, PartialEq, Eq)]
pub(crate) enum PagePress {
/// The press began on this page, and the owner's hit won it: only its module acts on it.
/// The press began on this page, and the owner's hit won it: its module acts on it. Others
/// 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
@@ -135,7 +136,6 @@ impl PagePress {
/// A press the pages hold, and who it belongs to.
#[derive(Clone, Debug, PartialEq)]
pub(crate) struct Capture {
pub page: usize,
pub owner: Owner,
pub declines_drag: bool,
}
@@ -159,7 +159,7 @@ 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).
pub(crate) fn press(&mut self, page: usize, hits: impl IntoIterator<Item = Hit>, tool: QuickTool) {
self.capture = top(hits, tool).map(|h| Capture { page, owner: h.owner, declines_drag: h.declines_drag });
self.capture = top(hits, tool).map(|h| Capture { owner: h.owner, declines_drag: h.declines_drag });
self.began = Some(Some(page));
}
@@ -167,7 +167,7 @@ impl PageRouter {
pub(crate) fn press_for(&self, page: usize) -> PagePress {
match (self.began, &self.capture) {
(None, _) => PagePress::Free,
(Some(Some(p)), Some(c)) if p == page => PagePress::Mine(c.owner),
(Some(Some(p)), Some(capture)) if p == page => PagePress::Mine(capture.owner),
(Some(Some(p)), None) if p == page => PagePress::Free,
_ => PagePress::Elsewhere,
}
@@ -175,7 +175,7 @@ impl PageRouter {
/// Who holds the press on `page`.
pub(crate) fn owner(&self, page: usize) -> Option<Owner> {
self.capture.as_ref().filter(|c| c.page == page).map(|c| c.owner)
self.press_for(page).owner()
}
/// Whether the press is held by someone who keeps its drag: then the page's text is not
@@ -192,7 +192,7 @@ impl PageRouter {
}
}
/// Drops the press (Escape, a dialog, automation's cancel): every page is the active tool's again
/// 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) {
self.capture = None;
+1 -1
View File
@@ -2038,7 +2038,7 @@ fn image_handles_account_for_document_and_view_rotation() {
}
}
/// Cancelling the pages' pointer gestures (Escape, a dialog, automation's cancel) lets go of a
/// Cancelling the pages' pointer gestures (Escape, automation's cancel) lets go of a
/// camera corner being dragged: the quadrilateral stays, and no corner follows the pointer.
#[test]
fn cancelling_pointer_gestures_lets_go_of_a_camera_corner() {
+2 -1
View File
@@ -132,7 +132,8 @@ pub(crate) fn page_input(cx: PageToolContext<'_>, free: bool, owner: Option<usiz
lv.context = Some((page, index));
return true;
}
if resp.clicked() || resp.double_clicked() {
// The tool's own click: on a link, or one nobody holds.
if (resp.clicked() || resp.double_clicked()) && (free || owner.is_some()) {
let now = ui.input(|i| i.time);
// A second click on the same link soon after the first is a double-click.
let double = resp.double_clicked() || owner.is_some_and(|k| lv.last_click.is_some_and(|(t, p, l)| p == page && l == k && now - t < 0.5));
+63 -1
View File
@@ -1128,6 +1128,7 @@ fn a_right_click_on_a_comment_with_a_drawing_tool_opens_its_menu() {
let on_comment = at(&h, 100.0, 305.0);
right_click(&mut h, on_comment);
assert_eq!(h.state().views[0].comments.selected, Some((0, 0)), "the comment is selected for its menu");
assert!(h.query_all_by_label("Reply").next().is_some(), "and its menu is open");
}
/// The check box `agree` of [`checkbox_pdf`], with a rectangle comment at x 150..250, y 100..150.
@@ -1406,7 +1407,7 @@ fn edge_link_pdf() -> Vec<u8> {
}
/// The Link tool's margin round a link stops at the page's edge: beside the page, where a press
/// does nothing, no hand promises one.
/// does nothing, no hand promises one (as before the margin moved into the one link test).
#[test]
fn the_link_tools_hand_stops_at_the_pages_edge() {
let mut h = open(edge_link_pdf(), "150", false);
@@ -1441,3 +1442,64 @@ fn a_right_click_with_a_field_tool_selects_no_comment() {
right_click(&mut h, on_comment);
assert_eq!(h.state().views[0].comments.selected, None);
}
/// With the Link tool a click that began off the page (in the gutter beside it) is no page's: the
/// selected link stays selected. It used to unselect it.
#[test]
fn a_click_that_began_off_the_page_keeps_the_selected_link() {
let mut h = open(link_pdf(), "150", false);
link_tool(&mut h);
let on_link = at(&h, 150.0, 325.0);
mouse_click(&mut h, on_link);
assert_eq!(h.state().views[0].links.selected, Some((0, 0)), "the link is selected");
let left = page(&h).min.x;
slipping_click(&mut h, egui::pos2(left - 2.0, on_link.y), egui::pos2(left + 3.0, on_link.y));
assert_eq!(h.state().views[0].links.selected, Some((0, 0)), "a click that began off the page leaves it selected");
}
/// A right-click while a comment is moved leaves the moved comment selected: the move ends on
/// it. The right-click used to select the comment under it mid-drag.
#[test]
fn a_right_click_during_a_comment_move_keeps_the_moved_comment_selected() {
let mut h = open(stacked_comments_pdf(), "150", false);
// "Bottom note" alone, then "Top note" alone.
let (bottom, top) = (at(&h, 60.0, 160.0), at(&h, 190.0, 290.0));
h.hover_at(bottom);
h.run_steps(1);
h.drag_at(bottom);
h.run_steps(1);
h.hover_at(bottom + egui::vec2(0.0, 20.0));
h.run_steps(1);
for pressed in [true, false] {
h.event(egui::Event::PointerButton { pos: top, button: egui::PointerButton::Secondary, pressed, modifiers: egui::Modifiers::NONE });
}
h.run_steps(1);
let end = bottom + egui::vec2(0.0, 40.0);
h.hover_at(end);
h.run_steps(1);
h.drop_at(end);
h.run_steps(3);
assert_eq!(h.state().views[0].comments.selected, Some((0, 0)), "the moved comment stays selected");
}
/// A click on added text leaves the selected comment unselected, as a click on a form field does
/// (ruling 1). It used to keep it selected.
#[test]
fn a_click_on_added_text_deselects_the_selected_comment() {
let mut h = open(comment_on_text_pdf(), "150", false);
assert!(h.state_mut().apply_edit(pdfcraft_engine::Edit::AddText {
page: 0,
text: pdfcraft_engine::AddedText { rect: [40.0, 195.0, 140.0, 210.0], text: "Added".into(), ..Default::default() },
}));
// The Edit panel open, with the Select tool: added text is picked up.
assert!(h.state_mut().execute("edit.text"));
h.run_steps(3);
h.state_mut().quick_tool = QuickTool::Select;
h.run_steps(3);
let on_comment = at(&h, 150.0, 315.0);
mouse_click(&mut h, on_comment);
assert_eq!(h.state().views[0].comments.selected, Some((0, 0)), "the comment is selected");
let on_added = at(&h, 90.0, 202.0);
mouse_click(&mut h, on_added);
assert_eq!(h.state().views[0].comments.selected, None, "a click on the added text leaves it unselected");
}