fix(ui): a press Prepare or added content took draws no comment; menus open where right-clicked

Found in review:
- **Comments acted on presses another tool had taken.** Comments run every frame now, and a
  commenting tool acted on any press nobody held, so with Prepare open a Rectangle drag drew a
  rectangle as well as Prepare's selection rectangle. Added content, with the Edit panel open,
  could likewise open a note composer under the Note tool. `main` skipped comments when another
  module took the frame. They are now told (`taken`), and their drawing and placing tools leave
  such a press alone. A click there still unselects.
- **The page menu's anchor went stale.** A right-click while the primary button was held skipped
  the call that also anchors the page menu, so Delete page, Paste and Copy Image acted on the page
  right-clicked before. Every right-click on a page now anchors the menu there, whoever takes it,
  and only the selections wait for the release. That includes the right-clicks Add text, Edit
  text, the Link tool and a field tool take, which used to keep the previous anchor.
- **Edit text's right-clicks.** Edit text took right-clicks on added content too. It now takes
  those on its paragraphs, pictures and the objects selected together, and comments see a
  right-click on added text, as before.
- **Camera corners.** A camera corner at the page's edge showed its grab cursor past the edge,
  where a press is no page's. Its hit now stops at the page, as a link's does.
- One source for a comment's layer, dead code in Add text's right-click, and true docs for
  `PagePress::Elsewhere` and the "Page presses" rule.

Tests: a shape-tool drag with Prepare open, a right-click with the button held on another page,
a right-click on added text with Edit text, the menu anchored by an Edit text right-click, and a
camera corner at the page's edge. Each was seen failing with its fix reverted.

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:32 +01:00
1 parent aa415811d7
commit f0caaf9259
9 files changed
+127 -37

No files matched your search

+1 -1
View File
@@ -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 router says the press is its own: the page view tells it what the press is to its page (`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. 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.
+23 -17
View File
@@ -201,9 +201,12 @@ pub(super) fn page_tool_input(ui: &mut egui::Ui, view: &mut DocView, frame: &mut
view.links.context = Some((i, index));
}
// Comments, every frame: their own presses, a commenting tool's, and a click elsewhere that
// leaves them unselected.
let on_comment = comments::page_input(ui, resp, &pcx, press, view);
let consumed = on_objects || on_edit_text || on_link || on_content || boxing || on_field || on_comment;
// leaves them unselected. A press nobody holds that another tool took this frame (Prepare's
// selection rectangle, added content, which still decide for themselves) is not also a
// commenting tool's, as before.
let taken = on_objects || on_edit_text || on_link || on_content || boxing || on_field;
let on_comment = comments::page_input(ui, resp, &pcx, press, taken, view);
let consumed = taken || on_comment;
let hover = pointer.and_then(|p| claim_under(&hits, view, p, frame.env.unobstructed));
PageInput { consumed, area_tool, hover, released_over }
}
@@ -324,33 +327,36 @@ fn tool_input(
}
}
/// A right-click at `p`: it goes to what lies on top there ([`PageHits::context_at`]), which the
/// page's menu then acts on. A comment there is selected, and none elsewhere; a Prepare field is
/// selected; links take theirs from what this returns.
/// A right-click at `p`: the page's menu opens there, and it goes to what lies on top there
/// ([`PageHits::context_at`]), which the menu then acts on. A comment there is selected, and none
/// elsewhere; a Prepare field is selected; links take theirs from what this returns.
/// `under`: the hits at `p` before the tools act. `press_held`: the primary button is held, and the
/// selections its press acts on are left alone until its release.
fn right_click(hits: &PageHits<'_>, under: &[crate::page_router::Hit], view: &mut DocView, p: Pos2, press_held: bool) -> Option<Owner> {
let context = hits.context_at(view, p, under);
// Add text takes every right-click on a page it can change, Edit text those on anything of its
// own (paragraphs, pictures, added content, the objects selected together and their grips), the
// Link tool those on a link, and a Prepare field tool the whole page, as before: comments see
// none of them.
// Delete page, Paste and Copy Image act where the menu opened, whoever takes the right-click.
comments::anchor(hits.pcx, p, view);
if press_held {
return context;
}
// Add text takes every right-click on a page it can change, Edit text those on its own
// paragraphs and pictures (the objects selected together, and crop mode's picture, among them),
// the Link tool those on a link, and a Prepare field tool the whole page, as before: comments
// see none of them.
let tool_takes_it = match hits.tool {
QuickTool::AddText => hits.can_modify || top(under.iter().cloned(), hits.tool).is_some(),
QuickTool::EditText => top(under.iter().cloned(), hits.tool).is_some(),
QuickTool::AddText => hits.can_modify,
QuickTool::EditText => under.iter().any(|h| matches!(h.owner, Owner::Paragraph { .. } | Owner::Picture { .. } | Owner::Objects { .. })),
QuickTool::Link => hits.can_modify && matches!(context, Some(Owner::Link { .. })),
_ => hits.field_tool.is_some(),
};
if !tool_takes_it && !press_held {
if !tool_takes_it {
let comment = match context {
Some(Owner::Comment { index, .. }) => Some(index),
_ => None,
};
comments::context(hits.pcx, p, comment, view);
comments::context(hits.pcx, comment, view);
}
if let Some(Owner::PrepareField { field, widget }) = context
&& !press_held
{
if let Some(Owner::PrepareField { field, widget }) = context {
crate::prepare::context(hits.form, field, widget, view);
}
context
+1 -1
View File
@@ -119,7 +119,7 @@ pub(crate) enum PagePress {
/// 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.
Free,
/// The press began on another page, or between the pages.
/// The press began on another page, or off the pages (between them, or beside one).
Elsewhere,
}
+11
View File
@@ -2047,3 +2047,14 @@ fn cancelling_pointer_gestures_lets_go_of_a_camera_corner() {
v.cancel_pointer_gestures();
assert_eq!(v.camera.map(|q| q.grab), Some(None));
}
/// A camera corner at the page's edge is hit on the page only: a press off the page is no page's,
/// so its grab cursor does not show there.
#[test]
fn a_camera_corner_at_the_page_edge_is_hit_on_the_page_only() {
let mut v = view(1, PageLayout::Single, true);
v.camera = Some(crate::camera_ui::CameraQuad { page: 0, corners: [[0.0, 0.0], [300.0, 0.0], [300.0, 400.0], [0.0, 400.0]], grab: None });
let xf = PageXform { rect: Rect::from_min_size(pos2(10.0, 20.0), egui::vec2(300.0, 400.0)), rot: 0, pw: 300.0, ph: 400.0 };
assert!(crate::camera_ui::hit(&xf, 0, &v, pos2(14.0, 24.0)).is_some(), "on the page");
assert!(crate::camera_ui::hit(&xf, 0, &v, pos2(6.0, 16.0)).is_none(), "just off it");
}
+4 -3
View File
@@ -61,10 +61,11 @@ 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, or on a page
/// without the quadrilateral.
/// 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.
pub(crate) fn hit(xf: &PageXform, page: usize, view: &DocView, p: Pos2) -> Option<Hit> {
if view.camera_done.is_some() {
if view.camera_done.is_some() || !xf.rect.contains(p) {
return None;
}
let q = view.camera.filter(|q| q.page == page)?;
+1 -1
View File
@@ -37,7 +37,7 @@ pub(crate) use geometry::{corner_ratio, page_rotation, resized};
pub use geometry::{drawn_text_box_rect, text_box_rect};
#[cfg(test)]
use input::hover_tip;
pub(crate) use input::{context, context_hit, describe, hit, keys, nudge_keys, page_after_text, page_input};
pub(crate) use input::{anchor, context, context_hit, describe, hit, keys, nudge_keys, page_after_text, page_input};
pub use menu::{CanvasAction, fill_picker, recent_custom_colours, remember_custom_colour, status_badge, swatch_grid};
#[cfg(test)]
use menu::{RECENT_CUSTOM, same_colour};
+22 -13
View File
@@ -11,12 +11,13 @@ 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. 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, view: &mut DocView) -> bool {
/// 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).
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;
let free = press == PagePress::Free && !taken;
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 {
@@ -98,11 +99,15 @@ pub(crate) fn hit(cx: &PageCx<'_>, view: &DocView, p: Pos2, on_link: bool) -> Op
}
let markup = is_markup(&a.subtype);
let icon = if cx.allowed && !markup { egui::CursorIcon::Move } else { egui::CursorIcon::PointingHand };
// What a drag moves whole (a note, a shape, a signature, a Fill & Sign mark) is placed on the
// page, over a form field; text markup lies on the text, under one.
let layer = if a.fill_sign || !markup { Layer::Placed } else { Layer::Comment };
// Text markup does not move: a click selects it, a drag across it selects the text.
Some(Hit { declines_drag: markup, ..Hit::new(Owner::Comment { index: a.index, part: CommentPart::Body }, layer, icon) })
Some(Hit { declines_drag: markup, ..Hit::new(Owner::Comment { index: a.index, part: CommentPart::Body }, layer(a), icon) })
}
/// The layer a comment's body lies on: what a drag moves whole (a note, a shape, a signature, a
/// Fill & Sign mark) is placed on the page, over a form field; text markup lies on the text, under
/// one.
fn layer(a: &Annotation) -> Layer {
if a.fill_sign || !is_markup(&a.subtype) { Layer::Placed } else { Layer::Comment }
}
/// The comment a right-click at `p` is on: the topmost, with every tool, as a comment's menu shows
@@ -113,14 +118,18 @@ pub(crate) fn context_hit(cx: &PageCx<'_>, p: Pos2) -> Option<Hit> {
return None;
}
let a = cx.comment_at(p)?;
let layer = if a.fill_sign || !is_markup(&a.subtype) { Layer::Placed } else { Layer::Comment };
Some(Hit::new(Owner::Comment { index: a.index, part: CommentPart::Body }, layer, egui::CursorIcon::Default))
Some(Hit::new(Owner::Comment { index: a.index, part: CommentPart::Body }, layer(a), egui::CursorIcon::Default))
}
/// A right-click at `p` on this page: the page's menu is anchored there (Copy Image), and the
/// comment it is on ([`context_hit`], when it wins there) is selected, or none.
pub(crate) fn context(cx: &PageCx<'_>, p: Pos2, index: Option<usize>, view: &mut DocView) {
/// A right-click at `p` on this page: the page's menu is anchored there (Delete page, Paste, Copy
/// Image, a note added here).
pub(crate) fn anchor(cx: &PageCx<'_>, p: Pos2, view: &mut DocView) {
view.comments.context_at = Some((cx.page, cx.to_user(p)));
}
/// A right-click on comment `index` of this page ([`context_hit`], when it wins there) selects it,
/// and one on anything else selects none.
pub(crate) fn context(cx: &PageCx<'_>, index: Option<usize>, view: &mut DocView) {
view.comments.selected = index.map(|i| (cx.page, i));
}
+46 -1
View File
@@ -1419,7 +1419,8 @@ fn the_link_tools_hand_stops_at_the_pages_edge() {
}
/// With Edit text a right-click on a paragraph is Edit text's, as before, though a comment lies
/// over it: the comment is not selected.
/// over it: the comment is not selected. The page's menu is anchored there all the same (Paste on
/// the picture menu, Delete page); it used to stay where the last right-click comments saw was.
#[test]
fn a_right_click_with_edit_text_on_a_paragraph_under_a_comment_selects_no_comment() {
let mut h = open(comment_on_text_pdf(), "150", false);
@@ -1429,6 +1430,7 @@ fn a_right_click_with_edit_text_on_a_paragraph_under_a_comment_selects_no_commen
wait_for_cursor(&mut h, on_text, |c| c != egui::CursorIcon::Default);
right_click(&mut h, on_text);
assert_eq!(h.state().views[0].comments.selected, None);
assert_eq!(h.state().views[0].comments.context_at.map(|(page, _)| page), Some(0), "the menu is anchored on the page");
}
/// With a Prepare field tool a right-click is the tool's, as before: a comment under it is not
@@ -1544,3 +1546,46 @@ fn a_long_press_on_a_comment_selects_it_for_its_menu() {
one_frame(&mut h, [touch(egui::TouchPhase::End, top), button(top, false), egui::Event::PointerGone]);
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() {
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]));
let two = h.state().views[0].page_screen_rect(1).expect("page 2 on screen");
let press = two.min + egui::vec2(50.0, 30.0);
h.hover_at(press);
h.run_steps(1);
h.drag_at(press);
h.run_steps(1);
h.hover_at(press + egui::vec2(40.0, 0.0));
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");
h.drop_at(press + egui::vec2(40.0, 0.0));
h.run_steps(2);
}
/// With Edit text a right-click on added text reaches comments, as before (Edit text keeps those on
/// its own paragraphs and pictures): the selected comment is unselected.
#[test]
fn a_right_click_with_edit_text_on_added_text_unselects_the_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() },
}));
h.state_mut().quick_tool = QuickTool::EditText;
h.run_steps(4);
h.state_mut().views[0].comments.selected = Some((0, 0));
let on_added = at(&h, 90.0, 202.0);
right_click(&mut h, on_added);
assert_eq!(h.state().views[0].comments.selected, None);
}
+18
View File
@@ -1599,3 +1599,21 @@ fn a_right_click_during_a_field_drag_moves_the_dragged_field() {
assert_eq!(field_rect(&h, "city"), city, "the field right-clicked stays where it was");
assert_ne!(field_rect(&h, "name"), name, "the dragged field moved");
}
/// With Prepare open, a drag with a shape tool on the page's empty part draws Prepare's selection
/// rectangle and no comment, as before: Prepare takes the press nobody holds. It used to draw both.
#[test]
fn with_prepare_open_a_shape_tool_drag_draws_no_comment() {
let mut h = harness();
h.state_mut().execute("form.prepare");
h.run_steps(3);
assert!(h.state_mut().execute("comment.square"));
h.run_steps(3);
assert!(h.state().is_preparing(), "picking the tool leaves Prepare open");
// Below the fields.
let (from, to) = (at(&h, 40.0, 260.0), at(&h, 200.0, 360.0));
drag(&mut h, from, to);
let s = h.state();
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");
}