From 2eabf45ba5d2a97f74f01ece00eac312a2befcb9 Mon Sep 17 00:00:00 2001 From: Sabbir Ahmed <13796403+sabbirshawon@users.noreply.github.com> Date: Thu, 15 Oct 2026 04:05:36 +0600 Subject: [PATCH] =?UTF-8?q?fix(ui):=20=E2=87=A7=20marquee=20square=20keeps?= =?UTF-8?q?=20equal=20sides=20on=20the=20pixel=20grid=20(#5472)=20(#5506)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The ⇧ square (and a 1:1 fixed ratio) is built in continuous document coordinates, and `marquee_px` then snapped each axis to the pixel grid on its own: floor the near edge, ceil the far one. A press between pixels, dragged 100.5 px, snapped x to 101 and y to 102, so the readout and the committed selection were off by one. `marquee_preview_px` now grows the shorter side to the longer one, away from the press point, so the square still covers every pixel the drag touched. Ants, readout and commit share this path. Fixes #5472 --- crates/ui-egui/src/canvas.rs | 52 ++++++++++++++++++++++++++++- crates/ui-egui/src/marquee_tests.rs | 17 ++++++++++ 2 files changed, 68 insertions(+), 1 deletion(-) diff --git a/crates/ui-egui/src/canvas.rs b/crates/ui-egui/src/canvas.rs index 09d7dcc8ec..2e44b83b43 100644 --- a/crates/ui-egui/src/canvas.rs +++ b/crates/ui-egui/src/canvas.rs @@ -244,7 +244,10 @@ pub(crate) fn marquee_preview_px(o: &crate::state::ToolOptions, d: &Drag, zoom: } let last = d.points.last().map_or(d.start, |p| [p[0], p[1]]); let (a, b) = marquee_corners(o, d, last); - let r = marquee_px(a, b); + let mut r = marquee_px(a, b); + if marquee_is_square(o, d) { + r = square_px(r, d.start, last); + } (r[2] > r[0] && r[3] > r[1]).then_some(r) } @@ -253,6 +256,37 @@ pub fn marquee_px(a: [f64; 2], b: [f64; 2]) -> [f64; 4] { [a[0].min(b[0]).floor(), a[1].min(b[1]).floor(), a[0].max(b[0]).ceil(), a[1].max(b[1]).ceil()] } +/// Whether the marquee drag `d` is constrained to a square (or circle): ⇧ during the drag with +/// the Normal style, or a Fixed Ratio of 1:1. Mirrors the branches of `chrome_ui::marquee_end`. +fn marquee_is_square(o: &crate::state::ToolOptions, d: &Drag) -> bool { + let (w, h) = (f64::from(o.marquee_width), f64::from(o.marquee_height)); + match o.marquee_style.as_str() { + "fixedSize" => false, + "fixedRatio" if w > 0.0 && h > 0.0 => w == h, + _ => d.marquee_mods().0, + } +} + +/// A constrained square `r` (`marquee_px` of its corners) with equal sides on whole pixels. +/// +/// The square is built in continuous coordinates, and snapping each axis to the pixel grid on +/// its own (floor the near edge, ceil the far one) made one side a pixel longer than the other +/// whenever the press point sat between pixels: a ⇧ drag read 101 × 102 (#5472). The shorter +/// side grows to the longer one, away from the press point (`start`, towards `end`), so the +/// square still covers every pixel the drag touched. +pub fn square_px(r: [f64; 4], start: [f64; 2], end: [f64; 2]) -> [f64; 4] { + let [x0, y0, x1, y1] = r; + let (w, h) = (x1 - x0, y1 - y0); + let n = w.max(h); + let grow = |lo: f64, hi: f64, extent: f64, from: f64, to: f64| { + let d = (n - extent).max(0.0); + if to < from { (lo - d, hi) } else { (lo, hi + d) } + }; + let (x0, x1) = grow(x0, x1, w, start[0], end[0]); + let (y0, y1) = grow(y0, y1, h, start[1], end[1]); + [x0, y0, x1, y1] +} + /// The one-pixel line `[x0, y0, x1, y1]` a Single Row (Single Column) Marquee at `p` selects: the /// pixel row (column) under `p`, kept on the canvas, across the whole `size`. `None` for other /// tools, an empty canvas or a point that is not a number. @@ -5935,6 +5969,22 @@ mod tabs_tests; #[cfg(test)] mod tests { + /// #5472: a ⇧ square pressed between pixels snapped to 101 × 102; the short side grows to + /// match, on the side the drag points to. + #[test] + fn square_px_equalises_the_sides_away_from_the_press_point() { + use super::{marquee_px, square_px}; + // Down-right drag: the continuous square (50.3, 50.7)..(150.8, 151.2) snaps unevenly. + let r = marquee_px([50.3, 50.7], [150.8, 151.2]); + assert_eq!(r, [50.0, 50.0, 151.0, 152.0], "uneven before the fix"); + assert_eq!(square_px(r, [50.3, 50.7], [150.8, 90.0]), [50.0, 50.0, 152.0, 152.0]); + // Up-left drag: the short side grows towards lower coordinates. + let r = marquee_px([150.8, 151.2], [50.3, 50.7]); + assert_eq!(square_px(r, [150.8, 151.2], [60.0, 50.7]), [49.0, 50.0, 151.0, 152.0]); + // Already square: unchanged. + assert_eq!(square_px([10.0, 20.0, 30.0, 40.0], [10.0, 20.0], [30.0, 40.0]), [10.0, 20.0, 30.0, 40.0]); + } + #[test] fn display_image_matches_egui_unmultiplied_conversion() { // Several parallel pieces and a ragged one; translucent, out-of-range and NaN values. diff --git a/crates/ui-egui/src/marquee_tests.rs b/crates/ui-egui/src/marquee_tests.rs index 51b16916e5..40e9bbd392 100644 --- a/crates/ui-egui/src/marquee_tests.rs +++ b/crates/ui-egui/src/marquee_tests.rs @@ -147,6 +147,23 @@ fn shift_during_the_drag_draws_a_square_and_the_readout_follows() { assert!(h.query_by_label("W:").is_none() && h.query_by_label_contains(" px").is_none(), "the readout goes away with the drag"); } +/// #5472: a ⇧ square pressed between pixels read 101 × 102 and selected that, because each axis +/// snapped to the pixel grid on its own. The readout and the selection are square, and the +/// Elliptical Marquee's circle too. +#[test] +fn shift_square_pressed_between_pixels_has_equal_sides() { + for tool in [Tool::RectMarquee, Tool::EllipseMarquee] { + let mut h = harness(tool); + press_at(&mut h, 50.3, 50.7, Modifiers::NONE); + mods(&mut h, Modifiers::SHIFT); + move_to(&mut h, 150.8, 90.0); + assert!(readout(&h, 102, 102), "{tool:?}: the square covers every pixel it touched, on both axes"); + release_at(&mut h, 150.8, 90.0, Modifiers::SHIFT); + mods(&mut h, Modifiers::NONE); + assert_eq!(selection(&h), Rect::new(50, 50, 152, 152), "{tool:?}"); + } +} + /// #204: Preferences › Tools › Show Transformation Values off hides the readout, not the marquee. #[test] fn show_transformation_values_off_hides_the_readout() {