From 58fa110d2ddcf0a07c857a59130f04f89a89d91b Mon Sep 17 00:00:00 2001 From: alehel Date: Tue, 3 Nov 2026 08:32:21 +0100 Subject: [PATCH] fix(ui): a right-button drag on the pages starts nothing a left one does MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The pages' presses are the primary button's (`PagePointer.edges`, the router's `began`), so a right-button drag was a press nobody held, and every tool that started on egui's `drag_started()`, which counts every button, started on it: the Rectangle tool drew a rectangle, Prepare's selection rectangle selected the fields in it, the Link tool opened Link Properties for a new link, the camera moved its quadrilateral to the page, and the Select tool selected text. On `main` too. `PagePointer.drag_started` is a primary drag of a press the pages hold, started this frame. The page modules read it instead of egui's (comments' shapes, callout and text box, Prepare, the Link tool, the camera, the text layer, image pick, and the Edit text group, whose owned presses were primary already); zoom, crop, Sign area and Redact already asked for the primary button. The guard refuses a bare `.drag_started()` in a page module (`NOT_PAGE_DRAGS` lists the widgets of their own: Organize's thumbnails, the signature pad, the Scan tab), and the router's `drag_started` is `drag_began`, so the rule reads cleanly. Tests, one per family, each seen failing with the fix reverted: `a_right_button_drag_draws_no_rectangle`, `…_selects_no_field`, `…_draws_no_link`, `…_leaves_the_camera_quadrilateral`, `…_selects_no_text`; the guard rule against a planted `#[cfg(any())]` call. Co-Authored-By: Claude Opus 5.5 --- crates/ui-egui/src/canvas/image_pick.rs | 8 +- crates/ui-egui/src/canvas/page_input.rs | 8 +- crates/ui-egui/src/canvas/page_pointer.rs | 8 +- .../ui-egui/src/canvas/page_pointer/tests.rs | 24 +++++- crates/ui-egui/src/canvas/page_router.rs | 6 +- crates/ui-egui/src/tools/camera_ui.rs | 4 +- crates/ui-egui/src/tools/comments/input.rs | 8 +- crates/ui-egui/src/tools/content_ui.rs | 2 +- crates/ui-egui/src/tools/edit_text_ui.rs | 2 +- .../ui-egui/src/tools/edit_text_ui/image.rs | 4 +- crates/ui-egui/src/tools/link_ui.rs | 4 +- crates/ui-egui/src/tools/prepare.rs | 2 +- crates/ui-egui/tests/it/pointer.rs | 73 +++++++++++++++++++ 13 files changed, 125 insertions(+), 28 deletions(-) diff --git a/crates/ui-egui/src/canvas/image_pick.rs b/crates/ui-egui/src/canvas/image_pick.rs index cc938170da..543f5f963d 100644 --- a/crates/ui-egui/src/canvas/image_pick.rs +++ b/crates/ui-egui/src/canvas/image_pick.rs @@ -3,7 +3,7 @@ //! other image as a lossless PNG (with its transparency). The clipboard gets the file, as when a //! file is copied in Finder or Explorer; Copy Image copies the pixels instead. -use egui::{Color32, CornerRadius, Pos2, Stroke}; +use egui::{Color32, CornerRadius, Stroke}; use crate::PdfCraftApp; use crate::canvas::{DocView, PageXform}; @@ -30,11 +30,11 @@ pub(crate) fn page_input( page: usize, info: &DocInfo, doc: &pdfcraft_engine::Document, - pointer: Option, + ptr: &crate::page_pointer::PagePointer, view: &mut DocView, ) { - let Some(p) = pointer.filter(|p| xf.rect.contains(*p)) else { return }; - if resp.drag_started() || resp.double_clicked() || resp.triple_clicked() { + let Some(p) = ptr.pos.filter(|p| xf.rect.contains(*p)) else { return }; + if ptr.drag_started || resp.double_clicked() || resp.triple_clicked() { view.picked_image = None; return; } diff --git a/crates/ui-egui/src/canvas/page_input.rs b/crates/ui-egui/src/canvas/page_input.rs index 36f6f9d3f9..e4227baecd 100644 --- a/crates/ui-egui/src/canvas/page_input.rs +++ b/crates/ui-egui/src/canvas/page_input.rs @@ -115,8 +115,8 @@ pub(super) fn page_tool_input(ui: &mut egui::Ui, view: &mut DocView, frame: &mut } } // A drag that the press's owner declines is nobody's once it starts. - if resp.drag_started_by(egui::PointerButton::Primary) { - view.router.drag_started(); + if ptr.drag_started { + view.router.drag_began(); } // What this page has to do with the press: its owner's, the active tool's, or another page's. let press = view.router.press_for(i); @@ -663,7 +663,7 @@ fn update_selection_on_press(ui: &egui::Ui, view: &mut DocView, text: &PageText, view.selection = Some(Selection::run(gesture.page, first, last)); } } - if gesture.resp.drag_started() && press_here && gesture.text_drag { + if gesture.ptr.drag_started && press_here && gesture.text_drag { view.actions_dismissed = None; view.selection_drag = true; let origin = gesture.ptr.origin.unwrap_or(gesture.pointer); @@ -778,7 +778,7 @@ pub(super) fn page_overlays(ui: &mut egui::Ui, view: &mut DocView, frame: &mut P // A press nobody holds picks the picture under it: a link's click follows the link only (ruling // 5). if tool == QuickTool::Select && input.press == PagePress::Free && !input.consumed { - crate::image_pick::page_input(resp, &xf, i, info, doc, pointer, view); + crate::image_pick::page_input(resp, &xf, i, info, doc, ptr, view); } crate::image_pick::drop_stale(doc, view); crate::image_pick::paint(painter, &xf, i, info, view); diff --git a/crates/ui-egui/src/canvas/page_pointer.rs b/crates/ui-egui/src/canvas/page_pointer.rs index e847009dd0..f3a1b13647 100644 --- a/crates/ui-egui/src/canvas/page_pointer.rs +++ b/crates/ui-egui/src/canvas/page_pointer.rs @@ -49,6 +49,9 @@ pub(crate) struct PagePointer { /// The modifier keys held at this frame's first primary press, or else now: a press with Shift /// or ⌘ adds to a selection. pub modifiers: Modifiers, + /// A primary press the pages hold turned into a drag this frame. egui's own `drag_started()` + /// counts every button, so a right-button drag would start what a left-button drag does. + pub drag_started: bool, } impl PagePointer { @@ -79,9 +82,10 @@ impl PagePointer { resp.dragged() || resp.drag_stopped() || (!modal && (resp.interact_pointer_pos().is_some() || press_origin.is_some_and(|p| on_top(&p)))); let pos = at.filter(|p| held || on_top(p)); if !held { - return Self { pos, origin: None, edges: Vec::new(), released: None, primary_down, any_down, modifiers }; + return Self { pos, origin: None, edges: Vec::new(), released: None, primary_down, any_down, modifiers, drag_started: false }; } - Self { pos, origin: press_origin, edges, released, primary_down, any_down, modifiers } + let drag_started = resp.drag_started_by(PointerButton::Primary); + Self { pos, origin: press_origin, edges, released, primary_down, any_down, modifiers, drag_started } } /// Where the pointer is, when it is inside `r`. diff --git a/crates/ui-egui/src/canvas/page_pointer/tests.rs b/crates/ui-egui/src/canvas/page_pointer/tests.rs index 69b86dcdc0..06e6a720e2 100644 --- a/crates/ui-egui/src/canvas/page_pointer/tests.rs +++ b/crates/ui-egui/src/canvas/page_pointer/tests.rs @@ -271,6 +271,18 @@ fn declares(line: &str) -> Option<&str> { rest.find(|c: char| !(c.is_alphanumeric() || c == '_')).map(|end| &rest[..end]) } +/// egui's drag start, which counts every button: a right-button drag would start what a left one +/// does. Page modules read `PagePointer::drag_started`, the primary button's. +const ANY_BUTTON_DRAG: [&str; 1] = [".drag_started()"]; + +/// Functions in the page modules that start a drag of a widget of their own, not of the pages: +/// (file, function, why). +const NOT_PAGE_DRAGS: [(&str, &str, &str); 3] = [ + ("canvas/organize.rs", "drag_pages", "Organize Pages drags thumbnails, not pages"), + ("tools/fill_sign.rs", "signature_pad", "the signature dialog's drawing pad"), + ("tools/fill_sign/scan.rs", "scan_tab", "the signature dialog's Scan tab"), +]; + /// Ways to set egui's cursor. const CURSOR_WRITES: [&str; 4] = ["set_cursor_icon(", ".cursor_icon = ", "on_hover_cursor(", "on_hover_and_drag_cursor("]; @@ -462,6 +474,14 @@ fn page_modules_read_the_pointer_only_through_page_pointer() { } } } + problems.extend(misuses( + &ANY_BUTTON_DRAG, + &NOT_PAGE_DRAGS, + "starts a drag with any button: a right-button drag would start what a left one does. Read \ + `PagePointer::drag_started` (the primary button's) instead. A widget of its own, not the pages? Add the \ + function to NOT_PAGE_DRAGS in src/canvas/page_pointer/tests.rs, with why.", + "starts no drag of its own any more: remove it from NOT_PAGE_DRAGS.", + )); assert!(problems.is_empty(), "{}", problems.join("\n")); } @@ -486,12 +506,12 @@ fn page_modules_claim_the_cursor_and_never_set_it() { const ROUTER_WRITES: [&str; 10] = [ "router.begin(", "router.press(", - "router.drag_started(", + "router.drag_began(", "router.settle(", "router.cancel(", "PageRouter::begin(", "PageRouter::press(", - "PageRouter::drag_started(", + "PageRouter::drag_began(", "PageRouter::settle(", "PageRouter::cancel(", ]; diff --git a/crates/ui-egui/src/canvas/page_router.rs b/crates/ui-egui/src/canvas/page_router.rs index b1ec7ff10a..1f5a2e5d24 100644 --- a/crates/ui-egui/src/canvas/page_router.rs +++ b/crates/ui-egui/src/canvas/page_router.rs @@ -219,7 +219,7 @@ impl PageRouter { /// The press turned into a drag: an owner that declines its drag lets it go, and from then on /// the press is nobody's (a drag across a highlight selects the text under it; with the Link /// tool, a drag from a link draws a new one). Its click stayed the owner's. - pub(in crate::canvas) fn drag_started(&mut self) { + pub(in crate::canvas) fn drag_began(&mut self) { if self.capture.as_ref().is_some_and(|c| c.declines_drag) { self.capture = None; } @@ -327,11 +327,11 @@ mod tests { let mut r = PageRouter::default(); r.begin(); r.press(1, [Hit { declines_drag: true, ..hit(Layer::Comment, 0) }], QuickTool::Select); - r.drag_started(); + r.drag_began(); assert_eq!((r.press_for(1), r.press_for(0)), (PagePress::Free, PagePress::Elsewhere)); r.begin(); r.press(1, [hit(Layer::Comment, 2)], QuickTool::Select); - r.drag_started(); + r.drag_began(); assert_eq!(r.press_for(1), PagePress::Mine(hit(Layer::Comment, 2).owner)); } } diff --git a/crates/ui-egui/src/tools/camera_ui.rs b/crates/ui-egui/src/tools/camera_ui.rs index 817d77bb9f..8da5351ee0 100644 --- a/crates/ui-egui/src/tools/camera_ui.rs +++ b/crates/ui-egui/src/tools/camera_ui.rs @@ -102,14 +102,14 @@ pub(crate) fn page_input(resp: &egui::Response, xf: &PageXform, ptr: &PagePointe // began, a click where it came up (egui has cleared the press's origin by its release, and the // pointer may have moved on in the same frame). let at = if resp.clicked() { ptr.released } else { origin }; - if (resp.drag_started() || resp.clicked()) + if (ptr.drag_started || resp.clicked()) && press == PagePress::Free && at.is_some_and(|p| xf.rect.contains(p)) && view.camera.is_none_or(|q| q.page != page) { view.camera = Some(CameraQuad::inset(page, xf.pw, xf.ph)); } - if resp.drag_started() + if ptr.drag_started && let PagePress::Mine(Owner::CameraCorner { corner }) = press && let Some(q) = view.camera.as_mut().filter(|q| q.page == page) { diff --git a/crates/ui-egui/src/tools/comments/input.rs b/crates/ui-egui/src/tools/comments/input.rs index 11f6e0549b..d1218cc08b 100644 --- a/crates/ui-egui/src/tools/comments/input.rs +++ b/crates/ui-egui/src/tools/comments/input.rs @@ -194,7 +194,7 @@ fn select_input( }; let cv = &mut view.comments; let mut consumed = false; - if resp.drag_started() + if cx.ptr.drag_started && let (Some((a, part)), Some(o)) = (owner, origin) { match part { @@ -356,7 +356,7 @@ impl Gestures<'_> { if over_page { view.cursor.claim(Layer::ToolArea, egui::CursorIcon::Crosshair); } - if resp.drag_started() + if cx.ptr.drag_started && pressed_here && let Some(o) = origin { @@ -479,7 +479,7 @@ impl Gestures<'_> { if over_page { view.cursor.claim(Layer::ToolArea, egui::CursorIcon::Crosshair); } - if resp.drag_started() + if cx.ptr.drag_started && pressed_here && let Some(o) = origin { @@ -560,7 +560,7 @@ impl Gestures<'_> { // A text box also takes a drag, as in Acrobat: the box is drawn at that size and its // text wraps inside it (#2669). if tool == CommentTool::TextBox { - if resp.drag_started() + if cx.ptr.drag_started && pressed_here && let Some(o) = origin { diff --git a/crates/ui-egui/src/tools/content_ui.rs b/crates/ui-egui/src/tools/content_ui.rs index be6b1a2734..374e3004fd 100644 --- a/crates/ui-egui/src/tools/content_ui.rs +++ b/crates/ui-egui/src/tools/content_ui.rs @@ -485,7 +485,7 @@ pub(crate) fn page_input(cx: PageToolContext<'_>, ccx: ContentCx<'_>, view: &mut } // A drag on what the press was on: a corner resizes the selected item, the item itself moves, // with the tools that pick added content up. - if resp.drag_started() { + if ptr.drag_started { let origin = ptr.origin.unwrap_or(p); match owned.filter(|_| picks_up) { Some((_, Some((left, top)))) => { diff --git a/crates/ui-egui/src/tools/edit_text_ui.rs b/crates/ui-egui/src/tools/edit_text_ui.rs index e7548a2b89..7889ae9110 100644 --- a/crates/ui-egui/src/tools/edit_text_ui.rs +++ b/crates/ui-egui/src/tools/edit_text_ui.rs @@ -770,7 +770,7 @@ pub(crate) fn page_input(cx: PageToolContext<'_>, lines: &[pdfcraft_engine::Text PagePress::Mine(Owner::Paragraph { block, part }) => Some((block, part)), _ => None, }; - if resp.drag_started() + if ptr.drag_started && active.is_none() && let Some(o) = ptr.origin && let Some((block, part)) = owner diff --git a/crates/ui-egui/src/tools/edit_text_ui/image.rs b/crates/ui-egui/src/tools/edit_text_ui/image.rs index ce99f48bac..af190f66fd 100644 --- a/crates/ui-egui/src/tools/edit_text_ui/image.rs +++ b/crates/ui-egui/src/tools/edit_text_ui/image.rs @@ -280,7 +280,7 @@ fn crop_input(cx: &ImageCx<'_>, view: &mut DocView, i: usize, image: &pdfcraft_e // A drag takes what its press was on ([`crop_hit`]). if gate - && resp.drag_started() + && ptr.drag_started && let Some(o) = ptr.origin && let PagePress::Mine(Owner::Picture { part: PicturePart::Crop(part), .. }) = press { @@ -768,7 +768,7 @@ pub(crate) fn image_input(cx: &ImageCx<'_>, images: &[pdfcraft_engine::PageImage // A drag of the selected picture takes what its press was on ([`body_hit`], [`handle_hit`]): // the rotate knob turns it, a grip resizes it, the picture itself moves. if gate - && resp.drag_started() + && ptr.drag_started && let Some(o) = ptr.origin && let PagePress::Mine(Owner::Picture { index, part }) = press && index == i diff --git a/crates/ui-egui/src/tools/link_ui.rs b/crates/ui-egui/src/tools/link_ui.rs index e30614900d..f2e880983d 100644 --- a/crates/ui-egui/src/tools/link_ui.rs +++ b/crates/ui-egui/src/tools/link_ui.rs @@ -115,10 +115,10 @@ pub(crate) fn page_input(cx: PageToolContext<'_>, free: bool, owner: Option, a: egui::Pos2, b: egui::Pos2) { + let button = |pos, pressed| egui::Event::PointerButton { pos, button: egui::PointerButton::Secondary, pressed, modifiers: egui::Modifiers::NONE }; + h.hover_at(a); + h.run_steps(1); + h.event(button(a, true)); + h.run_steps(1); + for k in 1..=4 { + h.hover_at(a + (b - a) * (k as f32 / 4.0)); + h.run_steps(1); + } + h.event(button(b, false)); + h.run_steps(3); +} + +// A right-button drag starts nothing a left-button drag does. egui's drag start counts every +// button, and each of these started on it. + +/// With the Rectangle tool, a right-button drag draws no rectangle. +#[test] +fn a_right_button_drag_draws_no_rectangle() { + let mut h = open(text_pdf(), "150", false); + h.state_mut().quick_tool = QuickTool::Comment(CommentTool::Rectangle); + h.run_steps(4); + let (a, b) = (at(&h, 60.0, 150.0), at(&h, 200.0, 80.0)); + right_drag(&mut h, a, b); + let s = h.state(); + assert!(s.session.get(s.views[0].id).unwrap().info.annotations.is_empty(), "no rectangle is drawn"); +} + +/// With Prepare open, a right-button drag around a field selects it by no selection rectangle. +#[test] +fn a_right_button_drag_selects_no_field() { + let mut h = open(comment_and_checkbox_pdf(), "150", false); + assert!(h.state_mut().execute("form.prepare")); + h.run_steps(4); + let (a, b) = (at(&h, 60.0, 250.0), at(&h, 140.0, 180.0)); + right_drag(&mut h, a, b); + assert!(h.state().views[0].prepare.selected.is_none(), "no field is selected"); +} + +/// With the Link tool, a right-button drag draws no link. +#[test] +fn a_right_button_drag_draws_no_link() { + let mut h = open(text_pdf(), "150", false); + link_tool(&mut h); + let (a, b) = (at(&h, 60.0, 150.0), at(&h, 200.0, 80.0)); + right_drag(&mut h, a, b); + assert!(h.state().link_draft.is_none() && h.state().dialog != Some(Dialog::LinkProps), "no link is drawn"); +} + +/// With the camera, a right-button drag on another page leaves the quadrilateral where it is. +#[test] +fn a_right_button_drag_leaves_the_camera_quadrilateral() { + let mut h = open(two_pages_pdf(), "100", false); + h.state_mut().quick_tool = QuickTool::EnhanceCamera; + h.run_steps(4); + assert_eq!(h.state().views[0].camera.as_ref().map(|q| q.page), Some(0)); + let two = h.state().views[0].page_screen_rect(1).expect("page 2 on screen"); + right_drag(&mut h, two.min + egui::vec2(40.0, 40.0), two.min + egui::vec2(120.0, 100.0)); + assert_eq!(h.state().views[0].camera.as_ref().map(|q| q.page), Some(0), "the quadrilateral stays on page 1"); +} + +/// With the Select tool, a right-button drag across text selects none. +#[test] +fn a_right_button_drag_selects_no_text() { + let mut h = open(text_pdf(), "150", false); + let (a, b) = (at(&h, 45.0, 303.0), at(&h, 200.0, 303.0)); + assert_eq!(wait_for_cursor(&mut h, a, |c| c == egui::CursorIcon::Text), egui::CursorIcon::Text, "the page's text has arrived"); + right_drag(&mut h, a, b); + assert!(h.state().views[0].selected_text().is_none(), "no text is selected"); +} + /// A 300×400 page with a web link along its left edge, x 0..100, y 300..350. fn edge_link_pdf() -> Vec { pdf(&[