From d423dd03ec48300f186462627ef9bbbbf37b2481 Mon Sep 17 00:00:00 2001 From: FredApps <153173854+FredApps@users.noreply.github.com> Date: Sun, 1 Nov 2026 14:57:42 +0100 Subject: [PATCH] fix(ui): keep image grips and flips aligned with the visible page Use matrix resizing whenever image axes are turned or reflected on screen, including document and view rotation. Share the visible grip geometry for cursor claims and hit routing, and use only the drawn rotate knob. Map flip labels to the visible axis and defer Cut selection clearing to its result. Regressions cover all page/view rotation combinations and 64 outward drags across every grip, asserting fixed anchors and pointer tracking for upright and rotated images. Co-Authored-By: Codex GPT-6.1 --- crates/ui-egui/src/canvas.rs | 30 +++++-- crates/ui-egui/src/canvas/page_router.rs | 3 +- crates/ui-egui/src/canvas/tests.rs | 35 ++++++++ .../ui-egui/src/tools/edit_text_ui/image.rs | 83 ++++++++++--------- crates/ui-egui/tests/it/editing.rs | 76 +++++++++++++++++ 5 files changed, 179 insertions(+), 48 deletions(-) diff --git a/crates/ui-egui/src/canvas.rs b/crates/ui-egui/src/canvas.rs index 6eb694f3aa..8c91ba0cb3 100644 --- a/crates/ui-egui/src/canvas.rs +++ b/crates/ui-egui/src/canvas.rs @@ -152,6 +152,21 @@ pub struct ImageObb { } impl ImageObb { + pub(crate) fn handle_cursor(&self, handle: ImageHandle) -> egui::CursorIcon { + let direction = self.handle_pos(handle) - self.screen_center; + match handle { + ImageHandle::TopLeft | ImageHandle::TopRight | ImageHandle::BottomLeft | ImageHandle::BottomRight => { + if direction.x * direction.y > 0.0 { + egui::CursorIcon::ResizeNwSe + } else { + egui::CursorIcon::ResizeNeSw + } + } + _ if direction.x.abs() > direction.y.abs() => egui::CursorIcon::ResizeHorizontal, + _ => egui::CursorIcon::ResizeVertical, + } + } + pub(crate) fn contains(&self, visible: Rect, p: Pos2) -> bool { (visible.is_positive() && visible.contains(p)) || crate::edit_text_ui::quad_contains(self.corners(), p) } @@ -184,14 +199,19 @@ impl ImageObb { let h_v = (c * c + d * d).sqrt().max(1e-6); let u_axis = (a / w_u, b / w_u); let v_axis = (c / h_v, d / h_v); - let is_rotated = b.abs() > 1e-4 || c.abs() > 1e-4; + // The fast rectangle path interprets labels in screen space. Use matrix + // resizing whenever the image's user-space axes are turned or reflected + // on screen, including page /Rotate and View > Rotate View. + let u = s10 - s00; + let v = s01 - s00; + let is_rotated = u.y.abs() > 1e-4 || v.x.abs() > 1e-4 || u.x < 0.0 || v.y > 0.0; Self { s00, s10, s11, s01, s_bc, s_rc, s_tc, s_lc, s_rot, screen_center, u_axis, v_axis, is_rotated } } pub fn from_image(im: &pdfcraft_engine::PageImage, xf: &PageXform, info: &DocInfo, page: usize) -> Self { - let [_, b, c, ..] = im.matrix; - let is_rot = b.abs() > 1e-4 || c.abs() > 1e-4; + let [a, b, c, d, ..] = im.matrix; + let is_rot = b.abs() > 1e-4 || c.abs() > 1e-4 || a < 0.0 || d < 0.0; let bounds = image_selectable_bounds(im); let matrix = if !is_rot && bounds[2] > bounds[0] && bounds[3] > bounds[1] { [bounds[2] - bounds[0], 0.0, 0.0, bounds[3] - bounds[1], bounds[0], bounds[1]] @@ -2594,7 +2614,8 @@ 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); - crate::edit_text_ui::image_context_menu(ui, view, &images, sel.page, sel.index, can_paste_image, &mut image_action, 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; } if view.comments.selected.is_some() { @@ -3868,7 +3889,6 @@ fn apply_frame_requests(app: &mut PdfCraftApp, index: usize, ui: &mut egui::Ui, } Some(crate::edit_text_ui::ImageAction::CopyFile(page, index)) => app.copy_page_image_file(page, index), Some(crate::edit_text_ui::ImageAction::Cut(page, image)) => { - app.views[index].image_selection = None; app.cut_page_image(page, image); } Some(crate::edit_text_ui::ImageAction::Paste(page, at)) => { diff --git a/crates/ui-egui/src/canvas/page_router.rs b/crates/ui-egui/src/canvas/page_router.rs index 06d3c48568..8b91cc865c 100644 --- a/crates/ui-egui/src/canvas/page_router.rs +++ b/crates/ui-egui/src/canvas/page_router.rs @@ -42,8 +42,7 @@ pub(crate) enum Owner { pub(crate) enum PicturePart { /// The picture: a click selects it, a drag moves the selected one. Body, - /// A corner of the selected picture (left top, right top, left bottom, right bottom): a drag - /// resizes it. + /// A resize grip in `ImageObb::handles` order (four corners and four edge midpoints). Corner(usize), /// The selected picture's rotate knob: a drag turns it. Knob, diff --git a/crates/ui-egui/src/canvas/tests.rs b/crates/ui-egui/src/canvas/tests.rs index 634a885111..4761584735 100644 --- a/crates/ui-egui/src/canvas/tests.rs +++ b/crates/ui-egui/src/canvas/tests.rs @@ -2002,3 +2002,38 @@ fn a_moved_picture_is_drawn_from_the_fresh_raster_while_a_tile_is_still_stale() h.state_mut().views[0].stale_tiles.clear(); h.state_mut().views[0].tiles.clear(); } + +#[test] +fn image_handles_account_for_document_and_view_rotation() { + for page_rotation in [0, 90, 180, 270] { + for view_rotation in [0, 90, 180, 270] { + let (width, height) = if page_rotation % 180 == 0 { (300.0, 400.0) } else { (400.0, 300.0) }; + let info = DocInfo { + pages: vec![pdfcraft_render::PageInfo { + width, + height, + label: String::new(), + crop: [0.0, 0.0, 300.0, 400.0], + rotation: page_rotation, + }], + ..Default::default() + }; + let size = if view_rotation % 180 == 0 { vec2(width, height) } else { vec2(height, width) }; + let xf = PageXform { rect: Rect::from_min_size(pos2(10.0, 20.0), size), rot: view_rotation, pw: width, ph: height }; + let obb = ImageObb::from_matrix([100.0, 0.0, 0.0, 50.0, 50.0, 70.0], &xf, &info, 0); + let total = (page_rotation + view_rotation) % 360; + assert_eq!(obb.is_rotated, total != 0, "page={page_rotation} view={view_rotation}: user-labelled grips need matrix resize when turned"); + for (handle, point) in obb.handles() { + let direction = point - obb.screen_center; + let cursor = match handle { + ImageHandle::TopLeft | ImageHandle::TopRight | ImageHandle::BottomLeft | ImageHandle::BottomRight => { + if direction.x * direction.y > 0.0 { egui::CursorIcon::ResizeNwSe } else { egui::CursorIcon::ResizeNeSw } + } + _ if direction.x.abs() > direction.y.abs() => egui::CursorIcon::ResizeHorizontal, + _ => egui::CursorIcon::ResizeVertical, + }; + assert_eq!(obb.handle_cursor(handle), cursor, "page={page_rotation} view={view_rotation} handle={handle:?}"); + } + } + } +} 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 e95e5dbbc6..2be091c1e6 100644 --- a/crates/ui-egui/src/tools/edit_text_ui/image.rs +++ b/crates/ui-egui/src/tools/edit_text_ui/image.rs @@ -1,5 +1,5 @@ use super::{ACCENT, Color32, CornerRadius, DocInfo, DocView, Edit, MAX_OUTLINES, PagePointer, PageXform, Pos2, Rect, Stroke, pos2, user_box}; -use crate::canvas::{ImageObb, PainterExt}; +use crate::canvas::{ImageObb, PainterExt, image_screen_rect}; use crate::image_engine::{CROP_ACCENT, apply_crop, apply_resize, apply_resize_proportional, draw_handle, draw_rule_of_thirds}; use crate::page_cursor::Layer; use crate::page_router::{Hit, Owner, PicturePart}; @@ -319,7 +319,6 @@ fn crop_input(cx: &ImageCx<'_>, view: &mut DocView, i: usize, image: &pdfcraft_e let done_resp = ui.interact(done, done_id, egui::Sense::click()); let cancel_resp = ui.interact(cancel, cancel_id, egui::Sense::click()); let reset_resp = reset.map(|r| ui.interact(r, reset_id, egui::Sense::click())); - let t = crate::theme::Tokens::get(ui.ctx()); // Done button (accent colored) @@ -450,14 +449,16 @@ pub(crate) fn handle_hit( p: Pos2, ) -> Option { let s = view.image_selection.as_ref().filter(|s| s.page == page && s.crop.is_none())?; - let b = images.get(s.index)?.visible_rect.map(|r| xf.user_rect(info, page, r.map(|v| v as f32)))?; + let image = images.get(s.index)?; + let b = image_screen_rect(image, xf, info, page); + let obb = ImageObb::from_image(image, xf, info, page); let picture = |part| Owner::Picture { index: s.index, part }; - if rotate_knob(b, clip).0.distance(p) < 8.0 { + let knob = if obb.is_rotated { obb.s_rot } else { rotate_knob(b, clip).0 }; + if knob.distance(p) < 8.0 { return Some(Hit::new(picture(PicturePart::Knob), Layer::Handle, egui::CursorIcon::Grab)); } - let corner = [b.left_top(), b.right_top(), b.left_bottom(), b.right_bottom()].iter().position(|c| c.distance(p) < 8.0)?; - let icon = if matches!(corner, 0 | 3) { egui::CursorIcon::ResizeNwSe } else { egui::CursorIcon::ResizeNeSw }; - Some(Hit::new(picture(PicturePart::Corner(corner)), Layer::Handle, icon)) + let (grip, (handle, _)) = obb.handles().into_iter().enumerate().find(|(_, (_, point))| point.distance(p) < 8.0)?; + Some(Hit::new(picture(PicturePart::Corner(grip)), Layer::Handle, obb.handle_cursor(handle))) } /// The picture under `p` (its shape, not only its box): a click selects it, a drag moves the @@ -619,6 +620,7 @@ pub(crate) fn image_input(cx: &ImageCx<'_>, images: &[pdfcraft_engine::PageImage return crop_input(cx, view, i, image); } let (knob, stem) = rotate_knob(b, ui.clip_rect()); + let shown_knob = if obb.is_rotated { obb.s_rot } else { knob }; if view.image_selection.as_ref().is_some_and(|s| s.drag.is_none()) { picture_frame(painter, &obb, b, Color32::TRANSPARENT, Stroke::new(1.5, ACCENT)); if obb.is_rotated { @@ -643,31 +645,15 @@ pub(crate) fn image_input(cx: &ImageCx<'_>, images: &[pdfcraft_engine::PageImage } } let rot_id = ui.scope_id().with("img_handle").with(8); - let rot_rect = Rect::from_center_size(if obb.is_rotated { obb.s_rot } else { knob }, egui::vec2(12.0, 12.0)); + let rot_rect = Rect::from_center_size(shown_knob, egui::vec2(12.0, 12.0)); let rot_resp = ui.interact(rot_rect, rot_id, egui::Sense::drag()); let rot_drag = allow_interaction && rot_resp.drag_started(); if allow_interaction && let Some(p) = hover_pos { - if obb.s_rot.distance(p) < 8.0 || knob.distance(p) < 8.0 { + if shown_knob.distance(p) < 8.0 { view.cursor.claim(Layer::Handle, egui::CursorIcon::Grab); } else if let Some((h, _)) = obb.handles().iter().find(|(_, pos)| pos.distance(p) < 8.0) { - let icon = if obb.is_rotated { - match h { - ImageHandle::TopLeft | ImageHandle::TopRight | ImageHandle::BottomLeft | ImageHandle::BottomRight => { - let v = obb.handle_pos(*h) - obb.screen_center; - if v.x * v.y > 0.0 { egui::CursorIcon::ResizeNwSe } else { egui::CursorIcon::ResizeNeSw } - } - ImageHandle::TopCenter | ImageHandle::BottomCenter if obb.v_axis.0.abs() > obb.v_axis.1.abs() => { - egui::CursorIcon::ResizeHorizontal - } - ImageHandle::RightCenter | ImageHandle::LeftCenter if obb.u_axis.1.abs() > obb.u_axis.0.abs() => { - egui::CursorIcon::ResizeVertical - } - _ => h.cursor_icon(), - } - } else { - h.cursor_icon() - }; + let icon = obb.handle_cursor(*h); view.cursor.claim(Layer::Handle, icon); } } @@ -681,14 +667,14 @@ pub(crate) fn image_input(cx: &ImageCx<'_>, images: &[pdfcraft_engine::PageImage } } else if rot_drag { if let Some(s) = view.image_selection.as_mut() { - let o = origin.unwrap_or(if obb.is_rotated { obb.s_rot } else { knob }); + let o = origin.unwrap_or(shown_knob); s.drag = Some(ImageDrag { start: o, target: DragTarget::Rotate, initial_rect: b, initial_matrix: image.matrix }); s.turning = true; } } else if resp.drag_started() && let Some(o) = origin { - if obb.s_rot.distance(o) < 8.0 || knob.distance(o) < 8.0 { + if shown_knob.distance(o) < 8.0 { if let Some(s) = view.image_selection.as_mut() { s.drag = Some(ImageDrag { start: o, target: DragTarget::Rotate, initial_rect: b, initial_matrix: image.matrix }); s.turning = true; @@ -708,7 +694,7 @@ pub(crate) fn image_input(cx: &ImageCx<'_>, images: &[pdfcraft_engine::PageImage if view.image_selection.as_ref().is_some_and(|s| s.turning) { let shift = ui.input(|i| i.modifiers.shift); let centre = obb.screen_center; - let rot_knob = if obb.is_rotated { obb.s_rot } else { knob }; + let rot_knob = shown_knob; let drag_start = view.image_selection.as_ref().and_then(|s| s.drag).map_or(rot_knob, |d| d.start); let degrees = hover_pos.map_or(0.0, |p| turn_degrees_delta(centre, drag_start, p, shift)); let (sin, cos) = degrees.to_radians().sin_cos(); @@ -804,7 +790,7 @@ pub(crate) fn image_input(cx: &ImageCx<'_>, images: &[pdfcraft_engine::PageImage } let icon = match drag.target { - DragTarget::Handle(handle) => handle.cursor_icon(), + DragTarget::Handle(handle) => obb.handle_cursor(handle), DragTarget::Move => egui::CursorIcon::Grabbing, _ => egui::CursorIcon::Move, }; @@ -864,7 +850,7 @@ pub(crate) fn image_input(cx: &ImageCx<'_>, images: &[pdfcraft_engine::PageImage let offset_u = if u < 0.0 { new_w } else { 0.0 }; let offset_v = if v < 0.0 { new_h } else { 0.0 }; let p00_new = (anchor.0 - offset_u * u_axis.0 - offset_v * v_axis.0, anchor.1 - offset_u * u_axis.1 - offset_v * v_axis.1); - let icon = handle.cursor_icon(); + let icon = obb.handle_cursor(handle); let mat = Matrix([new_w * u_axis.0, new_w * u_axis.1, new_h * v_axis.0, new_h * v_axis.1, p00_new.0, p00_new.1]); (mat, icon) } @@ -941,14 +927,11 @@ pub(crate) fn image_input(cx: &ImageCx<'_>, images: &[pdfcraft_engine::PageImage painter.rect_stroke(b, CornerRadius::ZERO, Stroke::new(1.5, ACCENT.gamma_multiply(0.7)), egui::StrokeKind::Outside); } } - let corner = hit_obb.handles().iter().position(|(_, c)| c.distance(p) < 10.0); - let cursor = match (selected == Some(hit), corner) { - (true, Some(0 | 4)) => egui::CursorIcon::ResizeNwSe, - (true, Some(2 | 6)) => egui::CursorIcon::ResizeNeSw, - (true, Some(1 | 5)) => egui::CursorIcon::ResizeVertical, - (true, Some(3 | 7)) => egui::CursorIcon::ResizeHorizontal, - (true, None) => egui::CursorIcon::Grab, - _ => egui::CursorIcon::PointingHand, + let grip = hit_obb.handles().into_iter().find(|(_, point)| point.distance(p) < 10.0); + let cursor = if selected == Some(hit) { + grip.map_or(egui::CursorIcon::Grab, |(handle, _)| hit_obb.handle_cursor(handle)) + } else { + egui::CursorIcon::PointingHand }; view.cursor.claim(Layer::Text, cursor); let clicked = match view.router.owner(page) { @@ -975,6 +958,7 @@ pub(crate) fn image_context_menu( can_paste: bool, action: &mut Option, at: Option<[f64; 2]>, + rotation: u16, ) { let Some(img) = images.get(hit) else { return }; use pdfcraft_engine::ImageEdit as E; @@ -1039,10 +1023,10 @@ pub(crate) fn image_context_menu( view.pending_edit = edit(E::RemoveCrop); } if item(ui, true, tl!("Flip horizontal")) { - view.pending_edit = edit(E::Flip { horizontal: true }); + view.pending_edit = edit(E::Flip { horizontal: flip_user_axis(true, rotation) }); } if item(ui, true, tl!("Flip vertical")) { - view.pending_edit = edit(E::Flip { horizontal: false }); + view.pending_edit = edit(E::Flip { horizontal: flip_user_axis(false, rotation) }); } ui.menu_button(tl!("Arrange image"), |ui| { @@ -1091,3 +1075,20 @@ pub(crate) fn image_context_menu( view.pending_edit = edit(E::Delete); } } + +fn flip_user_axis(horizontal: bool, rotation: u16) -> bool { + horizontal ^ (rotation % 180 == 90) +} + +#[cfg(test)] +mod screen_orientation_tests { + use super::*; + + #[test] + fn flip_labels_follow_the_screen_axis() { + for rotation in [0, 90, 180, 270] { + assert_eq!(flip_user_axis(true, rotation), rotation % 180 == 0); + assert_eq!(flip_user_axis(false, rotation), rotation % 180 != 0); + } + } +} diff --git a/crates/ui-egui/tests/it/editing.rs b/crates/ui-egui/tests/it/editing.rs index 75ad3f7dbe..ae32d2864e 100644 --- a/crates/ui-egui/tests/it/editing.rs +++ b/crates/ui-egui/tests/it/editing.rs @@ -9289,3 +9289,79 @@ fn escape_in_an_open_paragraph_selects_its_box_for_delete() { assert_eq!(blocks(&h)[0].text, "Page 1"); } } + +#[test] +fn outward_image_grip_drags_grow_on_rotated_pages() { + for rotation in [0, 90, 180, 270] { + for image_turn in [0.0, 25.0] { + for grip in 0..8 { + let mut png = Vec::new(); + image::RgbImage::from_pixel(100, 80, image::Rgb([100, 150, 200])) + .write_to(&mut std::io::Cursor::new(&mut png), image::ImageFormat::Png) + .unwrap(); + let mut h = Harness::builder().with_size(egui::vec2(1400.0, 900.0)).build_eframe(move |_cc| { + let mut app = PdfCraftApp::new(); + app.set_option("language", "en").unwrap(); + app.open_bytes("grips.png", None, png.clone()).unwrap(); + app + }); + h.run_steps(4); + h.state_mut().apply_edit(pdfcraft_engine::Edit::EditPageImage { + page: 0, + index: 0, + change: pdfcraft_engine::ImageEdit::Move([20.0, 20.0, 80.0, 60.0]), + }); + h.state_mut().apply_edit(pdfcraft_engine::Edit::RotatePages { pages: vec![0], degrees: rotation }); + if image_turn != 0.0 { + h.state_mut().apply_edit(pdfcraft_engine::Edit::EditPageImage { + page: 0, + index: 0, + change: pdfcraft_engine::ImageEdit::RotateBy(image_turn), + }); + } + h.state_mut().views[0].fit = pdfcraft_ui_egui::canvas::Fit::Page; + assert!(h.state_mut().execute("edit.edit_text")); + h.state_mut().views[0].image_selection = Some(pdfcraft_ui_egui::ImageSelection::at(0, 0)); + h.run_steps(3); + let view = &h.state().views[0]; + let doc = h.state().session.get(view.id).unwrap(); + let before = doc.page_images(0)[0].clone(); + let obb = pdfcraft_ui_egui::canvas::ImageObb::from_image(&before, &view.page_xform(0).unwrap(), &doc.info, 0); + let (handle, start) = obb.handles()[grip]; + let opposite = obb.handles()[(grip + 4) % 8].0; + let finish = start + (start - obb.screen_center) * 0.15; + assert!(view.viewport_rect().contains(start) && view.viewport_rect().contains(finish), "test grips must be visible"); + h.hover_at(start); + h.run_steps(1); + h.drag_at(start); + h.run_steps(1); + let started = h.state().views[0].image_selection.clone(); + h.hover_at(finish); + h.run_steps(2); + let moved = h.state().views[0].image_selection.clone(); + h.drop_at(finish); + h.run_steps(3); + let view = &h.state().views[0]; + let doc = h.state().session.get(view.id).unwrap(); + let images = doc.page_images(0); + let after = &images[0]; + let after_obb = pdfcraft_ui_egui::canvas::ImageObb::from_image(after, &view.page_xform(0).unwrap(), &doc.info, 0); + assert!( + after_obb.handle_pos(opposite).distance(obb.handle_pos(opposite)) < 1.0, + "fixed grip moved: rotation={rotation} image_turn={image_turn} grip={grip}" + ); + assert!( + after_obb.handle_pos(handle).distance(finish) < 1.0, + "dragged grip missed pointer: rotation={rotation} image_turn={image_turn} grip={grip}" + ); + let area = |m: [f64; 6]| (m[0] * m[3] - m[1] * m[2]).abs(); + assert!( + area(after.matrix) > area(before.matrix) * 1.05, + "outward grip shrank or ignored: rotation={rotation} image_turn={image_turn} grip={grip}: {:?} => {:?}; started={started:?} moved={moved:?}", + before.matrix, + after.matrix + ); + } + } + } +}