From e69d1221ff4bc588d28edc4afe30ff7cfe85191f Mon Sep 17 00:00:00 2001 From: alehel Date: Tue, 3 Nov 2026 14:41:30 +0100 Subject: [PATCH] fix(ui): a right-click between the pages deselects the comment; Paste keeps to the picture's page Two of CodeRabbit's findings on #5803. A right-click off the pages cleared only the menu's anchor and kept the selected comment, so the menu that opened was that comment's, not the current page's. A right-click on a page's empty part deselects it, so one that no page takes (`PageFrame.right_clicked`) now does the same. A selected Prepare field keeps its menu off the pages, as it does on a page's empty part; the comment there says so. Where pages touch, the first page takes a right-click on the row they share and anchors the menu there, but a picture on the second page under it still opens its menu. Paste from that menu centred the image on page 1's point, which on page 2 is at its foot. The image menu now uses the anchor only on the picture's own page, and otherwise pastes as Ctrl+V does. Tests, each seen failing with its fix reverted: `a_right_click_between_the_pages_opens_the_page_menu_not_the_selected_comments`, `paste_from_a_picture_menu_where_two_pages_touch_keeps_off_the_first_pages_point`. Co-Authored-By: Claude Opus 5.5 --- crates/ui-egui/src/canvas.rs | 24 ++++++++++-- crates/ui-egui/tests/it/pointer.rs | 60 ++++++++++++++++++++++++++++++ 2 files changed, 81 insertions(+), 3 deletions(-) diff --git a/crates/ui-egui/src/canvas.rs b/crates/ui-egui/src/canvas.rs index 5b22a38956..a1eeeafadb 100644 --- a/crates/ui-egui/src/canvas.rs +++ b/crates/ui-egui/src/canvas.rs @@ -1662,7 +1662,9 @@ fn draw_pages( // Set again below when the right-click lands on a link. view.links.context = None; // Set again below by the page right-clicked: a right-click off the pages (between them, - // beside one) anchors no page's menu, which then acts on the current page. + // beside one) anchors no page's menu, which then acts on the current page. It deselects the + // comment too (below), as one on a page's empty part does. A selected Prepare field keeps + // its menu, off the pages as on them. view.comments.context_at = None; let mut over_image = false; if tool == QuickTool::EditText { @@ -1734,8 +1736,22 @@ fn draw_pages( draw_page(ui, view, &mut frame, i); } let PageFrame { - wanted, visible_now, mut current, best_overlap, current_overlap, current_height, pressed_page, calc_hover, out: page_out, .. + wanted, + visible_now, + mut current, + best_overlap, + current_overlap, + current_height, + pressed_page, + calc_hover, + out: page_out, + right_clicked, + .. } = frame; + // A right-click no page took deselects the comment, so the page's menu opens, not the comment's. + if resp.secondary_clicked() && !right_clicked { + view.comments.selected = None; + } let PageOut { mut canvas_action, clicked_link, @@ -1905,7 +1921,9 @@ fn draw_pages( })); if is_image_menu && let Some(sel) = view.image_selection.clone() { let images = cached_edit_images(view, doc, sel.page).to_vec(); - let at = view.comments.context_at.map(|(_, at)| at); + // Paste lands where the menu opened only on the picture's page: where pages touch, the + // first page takes the right-click, and its point means nothing on the second. + let at = view.comments.context_at.filter(|(page, _)| *page == sel.page).map(|(_, at)| at); let rotation = (view.rotation % 360 + info.pages.get(sel.page).map_or(0, |p| p.rotation % 360)) % 360; crate::edit_text_ui::image_context_menu(ui, view, &images, sel.page, sel.index, can_paste_image, &mut image_action, at, rotation); return; diff --git a/crates/ui-egui/tests/it/pointer.rs b/crates/ui-egui/tests/it/pointer.rs index d01a74cff0..50801d903f 100644 --- a/crates/ui-egui/tests/it/pointer.rs +++ b/crates/ui-egui/tests/it/pointer.rs @@ -1726,6 +1726,66 @@ fn a_right_click_between_the_pages_anchors_no_pages_menu() { assert_eq!(h.state().views[0].comments.context_at, None); } +/// Two 300×400 pages, a rectangle comment on the first. +fn two_pages_and_a_comment_pdf() -> Vec { + pdf(&[ + CATALOG, + "<< /Type /Pages /Kids [3 0 R 4 0 R] /Count 2 /MediaBox [0 0 300 400] >>", + "<< /Type /Page /Parent 2 0 R /Annots [5 0 R] >>", + "<< /Type /Page /Parent 2 0 R >>", + "<< /Type /Annot /Subtype /Square /Rect [30 290 200 320] /C [1 0 0] /F 4 /T (Reviewer) >>", + ]) +} + +/// A right-click between the pages selects no comment, as one on a page's empty part does not, so +/// the menu that opens is the current page's. The selected comment's menu used to open there. +#[test] +fn a_right_click_between_the_pages_opens_the_page_menu_not_the_selected_comments() { + let mut h = open(two_pages_and_a_comment_pdf(), "100", false); + h.state_mut().views[0].comments.selected = Some((0, 0)); + h.run_steps(2); + assert_eq!(h.state().views[0].comments.selected, Some((0, 0)), "the comment is selected"); + let (one, two) = { + let v = &h.state().views[0]; + (v.page_screen_rect(0).expect("page 1 on screen"), v.page_screen_rect(1).expect("page 2 on screen")) + }; + let gap = egui::pos2(one.center().x, (one.max.y + two.min.y) / 2.0); + assert!(!one.contains(gap) && !two.contains(gap), "between the pages: {one:?} {two:?}"); + right_click(&mut h, gap); + assert_eq!(h.state().views[0].comments.selected, None); + assert!(h.query_by_label("Rotate view counterclockwise").is_some(), "the page menu opens"); + assert!(h.query_by_label("Reply").is_none(), "not the comment's"); +} + +/// Where pages touch, a right-click on the row they share is page 1's, and a picture on page 2 there +/// opens its menu, whose Paste then places the image as Ctrl+V does. It used to centre the image on +/// page 1's point, which on page 2 lies at its foot. +#[test] +fn paste_from_a_picture_menu_where_two_pages_touch_keeps_off_the_first_pages_point() { + let (mut h, seam) = touching_pages(|app| { + app.system_clipboard = false; + app.quick_tool = QuickTool::EditText; + }); + let clip = || pdfcraft_ui_egui::Clip::Image { width: 60, height: 40, rgba: [50u8, 150, 250, 255].repeat(60 * 40) }; + h.state_mut().clipboard_override = Some(clip()); + assert!(h.state_mut().paste_image_at(1, [100.0, 400.0]), "a picture across page 2's top edge"); + h.state_mut().clipboard_override = Some(clip()); + h.state_mut().views[0].image_selection = None; + h.run_steps(4); + right_click(&mut h, seam); + assert_eq!(h.state().views[0].comments.context_at.map(|(page, _)| page), Some(0), "page 1 takes the right-click"); + assert_eq!(h.state().views[0].image_selection.as_ref().map(|s| s.page), Some(1), "page 2's picture opens its menu"); + h.get_by_label("Paste").click(); + h.run_steps(3); + let pasted = { + let s = h.state(); + s.session.get(s.views[0].id).unwrap().page_images(1) + }; + assert_eq!(pasted.len(), 2, "the image is pasted on page 2"); + let r = pasted[1].rect; + assert!((r[1] + r[3]) / 2.0 > 100.0, "not centred at page 2's foot: {r:?}"); +} + /// With the camera tool, a click on another page moves the quadrilateral there, as a drag from it /// does. The click was located by its press's origin, which egui has cleared by the release frame, /// so it did nothing (on `main` too).