diff --git a/crates/engine/src/cmd/develop.rs b/crates/engine/src/cmd/develop.rs index 8f197429c..e0aaaf957 100644 --- a/crates/engine/src/cmd/develop.rs +++ b/crates/engine/src/cmd/develop.rs @@ -2,7 +2,7 @@ use lightcraft_catalog::{Op, PhotoId, Version}; use lightcraft_develop::{DevelopSettings, Preset, Section, SettingsGroup, Treatment, Upright, WbMode, controls}; -use lightcraft_geom::{CropGeometry, Point, Rect, crop_fit_angle}; +use lightcraft_geom::{CropGeometry, CropHandle, Point, Rect, crop_fit_angle, drag_crop}; use serde_json::{Value, json}; use super::{CommandSpec, always, bad, bool_or, cmd, f64_or, f64_req, has_active, has_clipboard, has_selection, ok, str_param}; @@ -37,6 +37,32 @@ fn crop_dims(s: &Session, id: PhotoId) -> (f64, f64) { if o.swaps_axes() { (h, w) } else { (w, h) } } +/// `v` as a list of numbers; `None` unless it is an array of nothing but numbers. +fn numbers(v: Option<&Value>) -> Option> { + v?.as_array()?.iter().map(Value::as_f64).collect() +} + +/// A width × height lock stored as hundredths. Rejects non-finite, non-positive and +/// extreme (beyond 20:1) shapes, which would give an empty or unusable crop. +fn aspect_tuple(c: &str, x: f64, y: f64) -> Result<(u32, u32)> { + let ratio = x / y; + if !(x.is_finite() + && y.is_finite() + && x > 0.0 + && y > 0.0 + && ratio.is_finite() + && (1.0 / lightcraft_geom::MAX_RATIO..=lightcraft_geom::MAX_RATIO).contains(&ratio)) + { + return Err(bad(c, "aspect needs two positive numbers with a ratio between 1:20 and 20:1")); + } + let to = |v: f64| (v * 100.0).round(); + let (a, b) = (to(x), to(y)); + if a < 1.0 || b < 1.0 || a > f64::from(u32::MAX) || b > f64::from(u32::MAX) { + return Err(bad(c, "aspect numbers are out of range")); + } + Ok((a as u32, b as u32)) +} + fn set_aspect(d: &mut DevelopSettings, w: f64, h: f64, aspect: Option<(u32, u32)>) { d.crop.aspect = aspect; let a = aspect.map(|(aw, ah)| { @@ -395,6 +421,46 @@ pub fn specs() -> Vec { Ok(()) }) }), + cmd!( + "crop.drag", + "Drag Crop Handle", + [], + None, + "{handle: topLeft|topRight|bottomRight|bottomLeft|top|right|bottom|left|move | 0..8, from: [x,y], to: [x,y] (normalized, straightened frame), start?: [x0,y0,x1,y1] (default: the current crop)} — drags a crop handle; stops at the image edge, keeps the opposite edges, honours the aspect lock", + has_active, + |s, p| { + let id = active(s, "crop.drag")?; + let (w, h) = crop_dims(s, id); + let handle = match p.get("handle") { + Some(Value::String(n)) => CropHandle::from_name(n), + Some(v) => v.as_u64().and_then(|i| u8::try_from(i).ok()).and_then(CropHandle::from_index), + None => None, + } + .ok_or_else(|| bad("crop.drag", "unknown handle"))?; + let point = |k: &str| -> Result { + match numbers(p.get(k)).as_deref() { + Some([x, y]) if x.is_finite() && y.is_finite() => Ok(Point::new(*x, *y)), + _ => Err(bad("crop.drag", format!("`{k}` needs [x, y]"))), + } + }; + let (from, to) = (point("from")?, point("to")?); + let given = match (p.get("start"), numbers(p.get("start")).as_deref()) { + (None | Some(Value::Null), _) => None, + (_, Some(&[x0, y0, x1, y1])) if [x0, y0, x1, y1].iter().all(|v| v.is_finite()) => { + Some(Rect::new(x0.min(x1), y0.min(y1), x0.max(x1), y0.max(y1))) + } + _ => return Err(bad("crop.drag", "`start` needs 4 numbers")), + }; + edit(s, "crop.drag", "Crop", |d| { + let start = CropGeometry { rect: given.unwrap_or(d.crop.geometry.rect), angle: d.crop.geometry.angle }; + // A locked crop keeps the shape it has: the lock stores a width:height tuple whose + // orientation is reinterpreted per image, so the frame itself is the truth. + let ratio = d.crop.aspect.and(Some(start.rect_px(w, h).aspect())).filter(|a| a.is_finite() && *a > 0.0); + d.crop.geometry = drag_crop(start, handle, from, to, w, h, ratio); + Ok(()) + }) + } + ), cmd!( "crop.straighten", "Straighten", @@ -470,11 +536,12 @@ pub fn specs() -> Vec { let (x, y) = a.split_once('x').ok_or_else(|| bad("crop.aspect", "use WxH"))?; let (x, y): (f64, f64) = (x.parse().map_err(|_| bad("crop.aspect", "bad number"))?, y.parse().map_err(|_| bad("crop.aspect", "bad number"))?); - Some(((x * 100.0) as u32, (y * 100.0) as u32)) - } - Some(Value::Array(a)) if a.len() == 2 => { - Some(((a[0].as_f64().unwrap_or(1.0) * 100.0) as u32, (a[1].as_f64().unwrap_or(1.0) * 100.0) as u32)) + Some(aspect_tuple("crop.aspect", x, y)?) } + Some(v @ Value::Array(_)) => match numbers(Some(v)).as_deref() { + Some(&[x, y]) => Some(aspect_tuple("crop.aspect", x, y)?), + _ => return Err(bad("crop.aspect", "use [w, h] with two numbers")), + }, _ => return Err(bad("crop.aspect", "missing aspect")), }; edit(s, "crop.aspect", "Crop Aspect", |d| { diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 7dcd60c1a..5e2cd5210 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -382,6 +382,61 @@ fn crop_aspect_lock_current_and_toggle() { assert!(active_dev(&s).crop.aspect.is_none(), "toggle unlocks"); } +// Feature: dragging crop handles (issue 295) +#[test] +fn crop_drag_stops_at_the_image_edge_and_keeps_the_anchor() { + let mut s = demo(); + s.execute("crop.set", &json!({"rect": [0.2, 0.2, 0.8, 0.8]})).unwrap(); + s.execute("crop.drag", &json!({"handle": "bottomRight", "from": [0.8, 0.8], "to": [1.4, 0.7]})).unwrap(); + let r = active_dev(&s).crop.geometry.rect; + assert!((r.x0 - 0.2).abs() < 1e-6 && (r.y0 - 0.2).abs() < 1e-6, "anchor moved: {r:?}"); + assert!((r.x1 - 1.0).abs() < 1e-6 && (r.y1 - 0.7).abs() < 1e-6, "{r:?}"); +} + +#[test] +fn crop_drag_keeps_a_locked_ratio_on_every_handle() { + let mut s = demo(); + s.execute("crop.aspect", &json!({"aspect": "16x9"})).unwrap(); + let d = active_dev(&s); + assert!(d.crop.aspect.is_some()); + let want = d.crop.geometry.rect.aspect(); + let r = d.crop.geometry.rect; + for (handle, to) in [("left", [r.x0 + 0.1, 0.5]), ("top", [0.5, r.y0 + 0.05]), ("bottomRight", [1.3, 1.3]), ("right", [0.9, 0.5])] { + s.execute("crop.drag", &json!({"handle": handle, "from": [0.5, 0.5], "to": to, "start": [r.x0, r.y0, r.x1, r.y1]})).unwrap(); + let g = active_dev(&s).crop.geometry.rect; + assert!((g.aspect() - want).abs() < 1e-6, "{handle}: {} vs {want}", g.aspect()); + } +} + +#[test] +fn crop_drag_rejects_unknown_handles_and_bad_points() { + let mut s = demo(); + assert!(s.execute("crop.drag", &json!({"handle": "sideways", "from": [0.5, 0.5], "to": [0.6, 0.6]})).is_err()); + assert!(s.execute("crop.drag", &json!({"handle": "move", "from": [0.5], "to": [0.6, 0.6]})).is_err()); + assert!(s.execute("crop.drag", &json!({"handle": "move", "to": [0.6, 0.6]})).is_err()); +} + +#[test] +fn crop_aspect_rejects_degenerate_custom_ratios() { + let mut s = demo(); + let before = active_dev(&s).crop; + for bad in [json!([0.004, 3]), json!([3, 0]), json!([-1, 2]), json!([1000, 0.01]), json!(["a", 2]), json!([1, 2, 3])] { + assert!(s.execute("crop.aspect", &json!({"aspect": bad.clone()})).is_err(), "{bad}"); + } + assert!(s.execute("crop.aspect", &json!({"aspect": "0x5"})).is_err()); + let after = active_dev(&s).crop; + assert_eq!(before.geometry, after.geometry, "a rejected ratio leaves the crop alone"); + s.execute("crop.aspect", &json!({"aspect": [2.5, 1]})).unwrap(); + assert!(!active_dev(&s).crop.geometry.rect.is_empty()); +} + +#[test] +fn crop_drag_rejects_non_numeric_points() { + let mut s = demo(); + assert!(s.execute("crop.drag", &json!({"handle": "move", "from": ["a", 0.5, 0.5], "to": [0.6, 0.6]})).is_err()); + assert!(s.execute("crop.drag", &json!({"handle": "move", "from": [0.5, 0.5], "to": [0.6, 0.6], "start": [0.1, "x", 0.5, 0.5]})).is_err()); +} + #[test] fn paste_from_previous_and_copy_paste_metadata() { let mut s = demo(); diff --git a/crates/geom/src/crop_drag.rs b/crates/geom/src/crop_drag.rs new file mode 100644 index 000000000..f600de6d3 --- /dev/null +++ b/crates/geom/src/crop_drag.rs @@ -0,0 +1,483 @@ +//! Interactive crop editing: what dragging a crop handle does. +//! +//! The crop rectangle lives in the straightened (rotated) frame and must stay inside the rotated +//! image. A drag is a *domain operation* on the crop, not a UI detail: the handle being dragged +//! decides which edges are anchored, an optional aspect lock decides the shape, and the image +//! bounds decide how far the drag may go. [`drag_crop`] answers all three in one place so the app, +//! CLI, control channel and MCP agree. + +use crate::{CropGeometry, Point, Rect}; + +/// Smallest crop side, as a fraction of the image's shorter side. +pub const MIN_CROP_FRACTION: f64 = 0.02; + +/// Most elongated locked crop shape (width:height or its inverse) the drag supports. +pub const MAX_RATIO: f64 = 20.0; + +/// A grab point on the crop frame. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum CropHandle { + TopLeft, + TopRight, + BottomRight, + BottomLeft, + Top, + Right, + Bottom, + Left, + /// Drag the whole frame. + Move, +} + +impl CropHandle { + /// The overlay's numbering: 0..3 corners (clockwise from top-left), 4..7 edges (top, right, + /// bottom, left), 8 = move. + pub fn from_index(i: u8) -> Option { + Some(match i { + 0 => CropHandle::TopLeft, + 1 => CropHandle::TopRight, + 2 => CropHandle::BottomRight, + 3 => CropHandle::BottomLeft, + 4 => CropHandle::Top, + 5 => CropHandle::Right, + 6 => CropHandle::Bottom, + 7 => CropHandle::Left, + 8 => CropHandle::Move, + _ => return None, + }) + } + + /// Parse a command-parameter name (`"topLeft"`, `"top"`, `"move"`, …). + pub fn from_name(n: &str) -> Option { + Some(match n { + "topLeft" => CropHandle::TopLeft, + "topRight" => CropHandle::TopRight, + "bottomRight" => CropHandle::BottomRight, + "bottomLeft" => CropHandle::BottomLeft, + "top" => CropHandle::Top, + "right" => CropHandle::Right, + "bottom" => CropHandle::Bottom, + "left" => CropHandle::Left, + "move" => CropHandle::Move, + _ => return None, + }) + } +} + +/// Drag `handle` of `start` from `grab` to `pointer` (both normalized, in the straightened frame) +/// on a `w × h` image. `aspect` is the locked width/height ratio in pixels, if any. +/// +/// Guarantees (for any input, including hostile numbers): +/// - the result lies inside the rotated image; +/// - the edges opposite the handle stay where they were (a move keeps the size); +/// - with `aspect`, the frame keeps that shape on every handle (when `start` had it); +/// - the drag stops at the image edge instead of pushing the frame around, and slides along it. +pub fn drag_crop(start: CropGeometry, handle: CropHandle, grab: Point, pointer: Point, w: f64, h: f64, aspect: Option) -> CropGeometry { + let sane = |v: f64| v.is_finite(); + let r0 = start.rect; + if ![w, h, grab.x, grab.y, pointer.x, pointer.y, start.angle, r0.x0, r0.y0, r0.x1, r0.y1].iter().copied().all(sane) + || w <= 0.0 + || h <= 0.0 + || r0.is_empty() + || aspect.is_some_and(|a| !(a.is_finite() && a > 0.0)) + { + return start; + } + // Start from a valid frame, then work in pixels of the straightened frame. + let mut base = start.constrained(w, h); + if base.angle == 0.0 { + // absorb the 1e-6 tolerance of `is_within_image` so the exact test below accepts the start + let r = base.rect; + base.rect = Rect::new(r.x0.clamp(0.0, 1.0), r.y0.clamp(0.0, 1.0), r.x1.clamp(0.0, 1.0), r.y1.clamp(0.0, 1.0)); + } + let sr = base.rect_px(w, h); + // Moving further than twice the image never matters; clamping keeps the searches well-conditioned. + let (dx, dy) = (((pointer.x - grab.x) * w).clamp(-2.0 * w, 2.0 * w), ((pointer.y - grab.y) * h).clamp(-2.0 * h, 2.0 * h)); + // Ratios beyond 20:1 make even the minimum frame larger than the image; treat them as 20:1. + let aspect = aspect.map(|a| a.clamp(1.0 / MAX_RATIO, MAX_RATIO)); + // Unrotated, the frame must be exactly inside the image (`is_within_image` tolerates 1e-6, which + // would leak out as a sliver past the edge and, after any clamp, a shifted ratio). + let feasible = |r: Rect| { + if base.angle == 0.0 { + r.x0 >= 0.0 && r.y0 >= 0.0 && r.x1 <= w && r.y1 <= h + } else { + CropGeometry { rect: r.scale(1.0 / w, 1.0 / h), angle: base.angle }.is_within_image(w, h) + } + }; + let min_side = MIN_CROP_FRACTION * w.min(h); + + let out = match (handle, aspect) { + (CropHandle::Move, _) => { + let slide = |cur: Rect, ax: f64, ay: f64| { + let t = bisect(0.0, 1.0, |t| feasible(cur.translate(crate::Vec2::new(ax * t, ay * t)))); + cur.translate(crate::Vec2::new(ax * t, ay * t)) + }; + let cur = slide(sr, dx, 0.0); + slide(cur, 0.0, dy) + } + (hd, None) => { + let (mx0, mx1, my0, my1) = sides(hd); + let grow = |cur: Rect, tx: f64, ty: f64| { + // `tx`/`ty`: how far the moving edges would go; interpolate towards it + let target = Rect::new( + if mx0 { (cur.x0 + tx).min(cur.x1 - min_side) } else { cur.x0 }, + if my0 { (cur.y0 + ty).min(cur.y1 - min_side) } else { cur.y0 }, + if mx1 { (cur.x1 + tx).max(cur.x0 + min_side) } else { cur.x1 }, + if my1 { (cur.y1 + ty).max(cur.y0 + min_side) } else { cur.y1 }, + ); + let at = |t: f64| lerp_rect(cur, target, t); + at(bisect(0.0, 1.0, |t| feasible(at(t)))) + }; + let cur = grow(sr, dx, 0.0); + grow(cur, 0.0, dy) + } + (hd, Some(a)) => { + let (mx0, _, my0, _) = sides(hd); + let (min_w, min_h) = (min_side.max(min_side * a), min_side.max(min_side / a)); + let c = sr.center(); + // Anchor: the opposite corner / edge midpoint. `build(n, o)` is the frame of width `n` that + // keeps it, shifted by `o` along the anchored edge (edge handles only). + let (target_w, build): (f64, Box Rect>) = match hd { + CropHandle::Top | CropHandle::Bottom => { + let (ay, sy, edge) = if my0 { (sr.y1, -1.0, sr.y0) } else { (sr.y0, 1.0, sr.y1) }; + let nh = (sy * (edge + dy - ay)).max(min_h); + (nh * a, Box::new(move |nw, o| vband(c.x + o, ay, sy, nw, a))) + } + CropHandle::Left | CropHandle::Right => { + let (ax, sx, edge) = if mx0 { (sr.x1, -1.0, sr.x0) } else { (sr.x0, 1.0, sr.x1) }; + let nw = (sx * (edge + dx - ax)).max(min_w); + (nw, Box::new(move |nw, o| hband(ax, sx, c.y + o, nw, a))) + } + _ => { + let (ax, sx, ex) = if mx0 { (sr.x1, -1.0, sr.x0) } else { (sr.x0, 1.0, sr.x1) }; + let (ay, sy, ey) = if my0 { (sr.y1, -1.0, sr.y0) } else { (sr.y0, 1.0, sr.y1) }; + let (wx, wy) = (sx * (ex + dx - ax), sy * (ey + dy - ay)); + let nw = ((wx + wy * a) / 2.0).max(min_w); + (nw, Box::new(move |nw, _| corner_rect(ax, sx, ay, sy, nw, a))) + } + }; + let target_w = target_w.min(2.0 * w.max(h * a)); + let slides = matches!(hd, CropHandle::Top | CropHandle::Bottom | CropHandle::Left | CropHandle::Right); + let extent = if matches!(hd, CropHandle::Top | CropHandle::Bottom) { w } else { h }; + // Frame of width `n`, centred on the anchored edge if it fits, else slid the least distance. + let place = |n: f64| -> Option { + let at = |o: f64| build(n, o); + if feasible(at(0.0)) { + return Some(at(0.0)); + } + if !slides { + return None; + } + const STEPS: u32 = 128; + let step = extent / STEPS as f64; + for k in 1..=STEPS { + for sign in [1.0, -1.0] { + let hi = sign * k as f64 * step; + if feasible(at(hi)) { + // closest feasible offset: bisect between the last infeasible and this one + let (mut bad, mut good) = (sign * (k - 1) as f64 * step, hi); + for _ in 0..40 { + let mid = (bad + good) / 2.0; + if feasible(at(mid)) { + good = mid; + } else { + bad = mid; + } + } + return Some(at(good)); + } + } + } + None + }; + if place(min_w).is_some() { + let n = bisect_range(min_w, target_w.max(min_w), |n| place(n).is_some()); + place(n).unwrap_or(sr) + } else { + sr + } + } + }; + let rect = out.scale(1.0 / w, 1.0 / h); + if [rect.x0, rect.y0, rect.x1, rect.y1].iter().all(|v| v.is_finite()) { CropGeometry { rect, angle: base.angle } } else { start } +} + +/// Which sides a handle moves: (left, right, top, bottom). +fn sides(h: CropHandle) -> (bool, bool, bool, bool) { + use CropHandle::*; + ( + matches!(h, TopLeft | BottomLeft | Left), + matches!(h, TopRight | BottomRight | Right), + matches!(h, TopLeft | TopRight | Top), + matches!(h, BottomLeft | BottomRight | Bottom), + ) +} + +fn lerp_rect(a: Rect, b: Rect, t: f64) -> Rect { + let l = |p: f64, q: f64| p + (q - p) * t; + Rect::new(l(a.x0, b.x0), l(a.y0, b.y0), l(a.x1, b.x1), l(a.y1, b.y1)) +} + +/// Frame of width `nw` and ratio `a` extending from corner (`ax`, `ay`) in directions `sx`, `sy`. +fn corner_rect(ax: f64, sx: f64, ay: f64, sy: f64, nw: f64, a: f64) -> Rect { + let nh = nw / a; + Rect::from_points(Point::new(ax, ay), Point::new(ax + sx * nw, ay + sy * nh)) +} + +/// Frame whose top/bottom edge (at `ay`, growing in direction `sy`) is anchored, centred on `cx`. +fn vband(cx: f64, ay: f64, sy: f64, nw: f64, a: f64) -> Rect { + let nh = nw / a; + Rect::from_points(Point::new(cx - nw / 2.0, ay), Point::new(cx + nw / 2.0, ay + sy * nh)) +} + +/// Frame whose left/right edge (at `ax`, growing in direction `sx`) is anchored, centred on `cy`. +fn hband(ax: f64, sx: f64, cy: f64, nw: f64, a: f64) -> Rect { + let nh = nw / a; + Rect::from_points(Point::new(ax, cy - nh / 2.0), Point::new(ax + sx * nw, cy + nh / 2.0)) +} + +/// Largest `t` in `[lo, hi]` for which `ok` holds, given that it holds at `lo` and the set of good +/// `t` is an interval (the feasible region is convex). +fn bisect_range(lo: f64, hi: f64, ok: impl Fn(f64) -> bool) -> f64 { + if ok(hi) { + return hi; + } + let (mut good, mut bad) = (lo, hi); + for _ in 0..48 { + let mid = (good + bad) / 2.0; + if ok(mid) { + good = mid; + } else { + bad = mid; + } + } + good +} + +fn bisect(lo: f64, hi: f64, ok: impl Fn(f64) -> bool) -> f64 { + bisect_range(lo, hi, ok) +} + +#[cfg(test)] +mod tests { + use super::*; + use CropHandle::*; + + const W: f64 = 600.0; + const H: f64 = 400.0; + const EPS: f64 = 1e-6; + + fn geo(x0: f64, y0: f64, x1: f64, y1: f64) -> CropGeometry { + CropGeometry { rect: Rect::new(x0, y0, x1, y1), angle: 0.0 } + } + fn p(x: f64, y: f64) -> Point { + Point::new(x, y) + } + fn drag(g: CropGeometry, hd: CropHandle, from: Point, to: Point, aspect: Option) -> CropGeometry { + drag_crop(g, hd, from, to, W, H, aspect) + } + fn px_aspect(g: &CropGeometry) -> f64 { + g.rect.width() * W / (g.rect.height() * H) + } + fn near(a: f64, b: f64) -> bool { + (a - b).abs() < EPS + } + + // Feature: dragging a crop handle + // Scenario: a free corner drag inside the image + #[test] + fn given_free_crop_when_corner_dragged_inside_then_corner_follows_and_opposite_corner_stays() { + let g = drag(geo(0.2, 0.2, 0.8, 0.8), BottomRight, p(0.8, 0.8), p(0.9, 0.7), None); + assert!(near(g.rect.x0, 0.2) && near(g.rect.y0, 0.2)); + assert!(near(g.rect.x1, 0.9) && near(g.rect.y1, 0.7)); + } + + // Scenario: dragging past the right edge (the weird behaviour of issue 295) + #[test] + fn given_corner_dragged_past_image_edge_then_it_stops_at_the_edge_and_anchor_stays() { + let g = drag(geo(0.2, 0.2, 0.8, 0.8), BottomRight, p(0.8, 0.8), p(1.3, 0.7), None); + assert!(near(g.rect.x0, 0.2) && near(g.rect.y0, 0.2), "anchor moved: {:?}", g.rect); + assert!(near(g.rect.x1, 1.0), "stops at the edge: {:?}", g.rect); + assert!(near(g.rect.y1, 0.7), "other axis still follows the pointer: {:?}", g.rect); + } + + #[test] + fn given_locked_aspect_when_corner_dragged_past_edge_then_ratio_and_anchor_hold() { + let a = 1.5; + let g = drag(geo(0.2, 0.2, 0.8, 0.2 + 0.6 * W / H / a), BottomRight, p(0.8, 0.6), p(1.4, 0.9), Some(a)); + assert!(near(g.rect.x0, 0.2) && near(g.rect.y0, 0.2), "{:?}", g.rect); + assert!(near(px_aspect(&g), a), "{}", px_aspect(&g)); + assert!(g.rect.x1 <= 1.0 + EPS && g.rect.y1 <= 1.0 + EPS); + assert!(g.rect.x1 > 0.95 || g.rect.y1 > 0.95, "grows until it touches an edge: {:?}", g.rect); + } + + #[test] + fn given_locked_aspect_when_edge_dragged_then_ratio_holds_and_frame_grows_about_its_midline() { + let a = 2.0; + let start = geo(0.3, 0.3, 0.7, 0.3 + 0.4 * W / H / a); + let g = drag(start, Bottom, p(0.5, start.rect.y1), p(0.5, start.rect.y1 + 0.1), Some(a)); + assert!(near(g.rect.y0, start.rect.y0), "opposite edge stays"); + assert!(near(g.rect.center().x, start.rect.center().x), "grows symmetrically sideways"); + assert!(near(px_aspect(&g), a)); + assert!(g.rect.height() > start.rect.height()); + } + + #[test] + fn given_free_crop_when_edge_dragged_then_only_that_edge_moves() { + let g = drag(geo(0.2, 0.2, 0.8, 0.8), Left, p(0.2, 0.5), p(0.1, 0.9), None); + assert!(near(g.rect.x0, 0.1) && near(g.rect.y0, 0.2) && near(g.rect.x1, 0.8) && near(g.rect.y1, 0.8)); + } + + #[test] + fn given_move_past_edge_then_size_is_kept_and_frame_slides_along_the_edge() { + let start = geo(0.5, 0.25, 0.9, 0.65); + let g = drag(start, Move, p(0.7, 0.45), p(1.3, 0.55), None); + assert!(near(g.rect.width(), 0.4) && near(g.rect.height(), 0.4)); + assert!(near(g.rect.x1, 1.0), "flush with the right edge: {:?}", g.rect); + assert!(near(g.rect.y0, 0.35), "still follows the pointer vertically: {:?}", g.rect); + } + + #[test] + fn given_edge_dragged_across_the_opposite_edge_then_crop_keeps_a_minimum_size() { + let g = drag(geo(0.2, 0.2, 0.8, 0.8), Right, p(0.8, 0.5), p(-0.5, 0.5), None); + assert!(g.rect.width() * W >= MIN_CROP_FRACTION * H - EPS && g.rect.x1 > g.rect.x0); + assert!(near(g.rect.x0, 0.2)); + } + + #[test] + fn given_straightened_image_when_dragged_outside_then_crop_stays_inside_the_rotated_image() { + let start = crate::crop_fit_angle(W, H, 12.0, Some(1.5)); + let c = start.rect.center(); + for hd in [TopLeft, TopRight, BottomRight, BottomLeft, Top, Right, Bottom, Left, Move] { + for aspect in [None, Some(1.5)] { + let g = drag(start, hd, c, p(3.0, -3.0), aspect); + assert!(g.is_within_image(W, H), "{hd:?} {aspect:?}: {:?}", g.rect); + assert_eq!(g.angle, 12.0); + } + } + } + + #[test] + fn given_hostile_numbers_then_the_start_crop_is_returned_not_a_panic() { + let s = geo(0.2, 0.2, 0.8, 0.8); + for bad in [f64::NAN, f64::INFINITY, f64::NEG_INFINITY] { + assert_eq!(drag(s, BottomRight, p(0.8, 0.8), p(bad, 0.5), None), s); + } + assert_eq!(drag_crop(s, BottomRight, p(0.8, 0.8), p(0.9, 0.9), 0.0, 400.0, None), s); + assert_eq!(drag(s, BottomRight, p(0.8, 0.8), p(0.9, 0.9), Some(f64::NAN)), s); + assert_eq!(drag(s, BottomRight, p(0.8, 0.8), p(0.9, 0.9), Some(-1.0)), s); + } + + // Scenario: a locked edge drag when the frame already touches the image edge (review finding) + #[test] + fn given_locked_frame_flush_with_the_top_when_right_edge_dragged_out_then_it_grows_sliding_down() { + let a = 1.5; + let start = geo(0.2, 0.0, 0.5, 0.3 * W / H * 0.0 + 0.3 * W / (a * H)); + let g = drag(start, Right, p(0.5, 0.2), p(0.9, 0.2), Some(a)); + assert!(near(g.rect.x0, 0.2), "anchor edge stays: {:?}", g.rect); + assert!(g.rect.width() > start.rect.width() + 0.05, "it must grow: {:?}", g.rect); + assert!(near(px_aspect(&g), a)); + assert!(g.is_within_image(W, H)); + } + + #[test] + fn given_full_fit_straightened_crop_when_locked_edge_dragged_then_it_still_respects_the_image() { + let start = crate::crop_fit_angle(W, H, 20.0, Some(1.5)); + let g = drag(start, Right, p(0.9, 0.5), p(1.5, 0.5), Some(1.5)); + assert!(g.is_within_image(W, H) && near(px_aspect(&g), 1.5)); + } + + // Scenario: extreme or hostile magnitudes + #[test] + fn given_an_extreme_locked_ratio_then_the_frame_can_still_be_moved() { + let start = geo(0.1, 0.4, 0.9, 0.4 + 0.8 * W / (60.0 * H)); + let g = drag(start, Move, p(0.5, 0.4), p(0.5, 0.7), Some(60.0)); + assert!(g.rect.y0 > 0.5, "moved: {:?}", g.rect); + } + + #[test] + fn given_a_huge_finite_pointer_then_a_locked_corner_still_grows_to_the_edge() { + let a = 1.5; + let start = geo(0.2, 0.2, 0.5, 0.2 + 0.3 * W / (a * H)); + let g = drag(start, BottomRight, p(0.5, 0.4), p(1e300, 1e300), Some(a)); + assert!(g.rect.x1 > 0.95 || g.rect.y1 > 0.95, "{:?}", g.rect); + let g = drag(start, BottomRight, p(0.5, 0.4), p(1e300, 1e300), None); + assert!(near(g.rect.x1, 1.0) && near(g.rect.y1, 1.0), "{:?}", g.rect); + } + + #[test] + fn given_an_unrotated_image_then_results_never_leave_the_unit_square() { + let g = drag(geo(0.2, 0.2, 0.8, 0.8), BottomRight, p(0.8, 0.8), p(5.0, 5.0), None); + assert!(g.rect.x1 <= 1.0 && g.rect.y1 <= 1.0 && g.rect.x0 >= 0.0 && g.rect.y0 >= 0.0, "{:?}", g.rect); + } + + #[test] + fn handle_numbering_matches_the_overlay() { + assert_eq!(CropHandle::from_index(0), Some(TopLeft)); + assert_eq!(CropHandle::from_index(6), Some(Bottom)); + assert_eq!(CropHandle::from_index(8), Some(Move)); + assert_eq!(CropHandle::from_index(9), None); + assert_eq!(CropHandle::from_name("bottomLeft"), Some(BottomLeft)); + assert_eq!(CropHandle::from_name("nope"), None); + } + + mod props { + use super::*; + use proptest::prelude::*; + + fn any_handle() -> impl Strategy { + (0u8..9).prop_map(|i| CropHandle::from_index(i).unwrap_or(Move)) + } + + proptest! { + /// Whatever the pointer does, the crop stays valid; the anchor and the ratio hold. + #[test] + fn drag_is_always_inside_anchored_and_shaped( + angle in -45.0f64..45.0, + ratio in prop::option::of(0.3f64..4.0), + hd in any_handle(), + tx in -4.0f64..5.0, ty in -4.0f64..5.0, + shrink in 0.2f64..1.0, + ) { + let base = crate::crop_fit_angle(W, H, angle, ratio); + // a smaller start crop so there is room to move + let c = base.rect.center(); + let start = CropGeometry { rect: Rect::from_center(c, base.rect.width() * shrink, base.rect.height() * shrink), angle }; + prop_assume!(start.is_within_image(W, H)); + let g = drag_crop(start, hd, c, p(tx, ty), W, H, ratio); + prop_assert!(g.is_within_image(W, H), "{:?} -> {:?}", start.rect, g.rect); + prop_assert!(g.rect.width() > 0.0 && g.rect.height() > 0.0); + let min = MIN_CROP_FRACTION * W.min(H) - 1e-6; + prop_assert!(g.rect.width() * W >= min.min(start.rect.width() * W) && g.rect.height() * H >= min.min(start.rect.height() * H), "min size {:?}", g.rect); + if let Some(a) = ratio { + prop_assert!((px_aspect(&g) - a).abs() < 1e-6 * a.max(1.0), "ratio {a}: {}", px_aspect(&g)); + } + // anchors: edges opposite the handle do not move + let (s, r) = (start.rect, g.rect); + let keep_l = matches!(hd, TopRight | BottomRight | Right); + let keep_r = matches!(hd, TopLeft | BottomLeft | Left); + let keep_t = matches!(hd, BottomLeft | BottomRight | Bottom); + let keep_b = matches!(hd, TopLeft | TopRight | Top); + { + if keep_l { prop_assert!((r.x0 - s.x0).abs() < 1e-6, "x0"); } + if keep_r { prop_assert!((r.x1 - s.x1).abs() < 1e-6, "x1"); } + if keep_t { prop_assert!((r.y0 - s.y0).abs() < 1e-6, "y0"); } + if keep_b { prop_assert!((r.y1 - s.y1).abs() < 1e-6, "y1"); } + } + if hd == Move { + prop_assert!((r.width() - s.width()).abs() < 1e-9 && (r.height() - s.height()).abs() < 1e-9); + } + } + + #[test] + fn drag_never_panics_on_arbitrary_numbers( + x0 in -2.0f64..3.0, y0 in -2.0f64..3.0, x1 in -2.0f64..3.0, y1 in -2.0f64..3.0, + angle in -90.0f64..90.0, tx in -1e9f64..1e9, ty in -1e9f64..1e9, + ratio in prop::option::of(-1.0f64..10.0), hd in any_handle(), + ) { + let g = drag_crop(CropGeometry { rect: Rect::new(x0, y0, x1, y1), angle }, hd, p(0.5, 0.5), p(tx, ty), W, H, ratio); + prop_assert!(g.rect.x0.is_finite() && g.rect.y1.is_finite()); + } + } + } +} diff --git a/crates/geom/src/lib.rs b/crates/geom/src/lib.rs index 6de121b00..1d81279d9 100644 --- a/crates/geom/src/lib.rs +++ b/crates/geom/src/lib.rs @@ -6,10 +6,12 @@ #![deny(clippy::unwrap_used, clippy::expect_used, clippy::panic, clippy::unimplemented, clippy::todo, clippy::unreachable)] mod crop; +mod crop_drag; mod homography; mod real; pub use crop::{CropGeometry, crop_fit_angle, max_inscribed_scale}; +pub use crop_drag::{CropHandle, MAX_RATIO, MIN_CROP_FRACTION, drag_crop}; pub use homography::Homography; pub use real::{Interval, Real}; diff --git a/crates/ui-egui/src/panels/detail.rs b/crates/ui-egui/src/panels/detail.rs index 8ee7bfbf1..c1257fafa 100644 --- a/crates/ui-egui/src/panels/detail.rs +++ b/crates/ui-egui/src/panels/detail.rs @@ -945,42 +945,14 @@ fn crop_overlay(app: &mut LightcraftApp, ui: &mut egui::Ui, resp: &egui::Respons { match app.gesture.clone() { Some(Gesture::CropHandle { handle, start, angle }) => { + // The crop model decides what the drag does (anchor, aspect lock, image bounds): see + // `lightcraft_geom::drag_crop`; the UI only reports where the pointer started and is. let n = to_straight(map.norm(q), angle, frame); - let o = resp.interact_pointer_pos().map(|q0| q0 - resp.drag_delta()).unwrap_or(q); - let _ = o; - let mut r = start; let orig = ui.input(|i| i.pointer.press_origin()).map(|q0| to_straight(map.norm(q0), angle, frame)).unwrap_or(n); - let (dx, dy) = (n.x - orig.x, n.y - orig.y); - match handle { - 0 => (r.x0, r.y0) = (start.x0 + dx, start.y0 + dy), - 1 => (r.x1, r.y0) = (start.x1 + dx, start.y0 + dy), - 2 => (r.x1, r.y1) = (start.x1 + dx, start.y1 + dy), - 3 => (r.x0, r.y1) = (start.x0 + dx, start.y1 + dy), - 4 => r.y0 = start.y0 + dy, - 5 => r.x1 = start.x1 + dx, - 6 => r.y1 = start.y1 + dy, - 7 => r.x0 = start.x0 + dx, - _ => r = start.translate(lightcraft_geom::Vec2::new(dx, dy)), - } - // aspect lock - if let Some((aw, ah)) = d.crop.aspect - && handle < 4 - { - let (iw, ih) = (frame.ow, frame.oh); - let a = if (iw >= ih) == (aw >= ah) { aw as f64 / ah as f64 } else { ah as f64 / aw as f64 }; - let w_px = (r.x1 - r.x0).abs() * iw; - let h_px = w_px / a; - let hn = h_px / ih; - if handle == 0 || handle == 1 { - r.y0 = r.y1 - hn; - } else { - r.y1 = r.y0 + hn; - } - } - let rr = lightcraft_geom::Rect::new(r.x0.min(r.x1), r.y0.min(r.y1), r.x0.max(r.x1), r.y0.max(r.y1)); - if rr.width() > 0.02 && rr.height() > 0.02 { - let _ = app.run("crop.set", json!({"rect": [rr.x0, rr.y0, rr.x1, rr.y1]})); - } + let _ = app.run( + "crop.drag", + json!({"handle": handle, "from": [orig.x, orig.y], "to": [n.x, n.y], "start": [start.x0, start.y0, start.x1, start.y1]}), + ); } Some(Gesture::CropRotate { start_angle, a0 }) => { let c = map.screen(Point::new(0.5, 0.5)); diff --git a/crates/ui-egui/src/panels/right.rs b/crates/ui-egui/src/panels/right.rs index 118451868..419682e05 100644 --- a/crates/ui-egui/src/panels/right.rs +++ b/crates/ui-egui/src/panels/right.rs @@ -62,33 +62,99 @@ fn padded(ui: &mut egui::Ui, add: impl FnOnce(&mut egui::Ui)) { egui::Frame::NONE.inner_margin(egui::Margin { left: 24, right: 22, top: 6, bottom: 6 }).show(ui, add); } +/// The aspect presets of the Crop panel: (menu label, `crop.aspect` value). +const ASPECT_PRESETS: [(&str, &str); 10] = [ + ("Free", "free"), + ("Original", "original"), + ("1 × 1", "1x1"), + ("4 × 5 / 8 × 10", "4x5"), + ("8.5 × 11", "8.5x11"), + ("5 × 7", "5x7"), + ("2 × 3 / 4 × 6", "2x3"), + ("4 × 3", "4x3"), + ("16 × 9", "16x9"), + ("16 × 10", "16x10"), +]; + +/// What the aspect button says for the stored lock (`aspect` = width × 100, height × 100): +/// a preset's name when the ratio matches one in either orientation, "Original" for the photo's own +/// shape, otherwise the ratio as `1.37 : 1`. +fn aspect_label(aspect: Option<(u32, u32)>, original: Option) -> String { + let Some((w, h)) = aspect else { return "Free".into() }; + if w == 0 || h == 0 { + return "Free".into(); + } + let r = w as f64 / h as f64; + let is = |q: f64| q.is_finite() && q > 0.0 && ((r - q).abs() / q < 0.004 || (1.0 / r - q).abs() / q < 0.004); + if original.is_some_and(is) { + return "Original".into(); + } + for (label, key) in ASPECT_PRESETS { + if let Some((x, y)) = key.split_once('x') + && let (Ok(x), Ok(y)) = (x.parse::(), y.parse::()) + && is(x / y) + { + return label.to_string(); + } + } + format!("{:.2} : 1", r.max(1.0 / r)) +} + +/// "Custom" row of the aspect menu: two number fields and an Apply button (`crop.aspect` `[w, h]`). +fn custom_aspect(app: &mut LightcraftApp, ui: &mut egui::Ui) { + let key = egui::Id::new("crop-custom-aspect"); + let (mut w, mut h): (String, String) = ui.data_mut(|d| d.get_temp(key)).unwrap_or_else(|| ("3".into(), "2".into())); + let mut apply = false; + ui.horizontal(|ui| { + ui.label(crate::i18n::tr("Custom")); + let a = ui.add(egui::TextEdit::singleline(&mut w).desired_width(34.0)); + register(ui.ctx(), "field:cropCustomW", a.rect); + ui.label("×"); + let b = ui.add(egui::TextEdit::singleline(&mut h).desired_width(34.0)); + register(ui.ctx(), "field:cropCustomH", b.rect); + let go = text_button(ui, "cropCustomApply", "Apply", false); + apply = go.clicked() || ((a.lost_focus() || b.lost_focus()) && ui.input(|i| i.key_pressed(egui::Key::Enter))); + }); + if apply { + // Accept `3`, `2.5` and `3,5`; anything else (zero, negative, text, huge) is ignored. + let num = |t: &str| t.trim().replace(',', ".").parse::().ok().filter(|v| v.is_finite() && *v > 0.0 && *v <= 1000.0); + if let (Some(x), Some(y)) = (num(&w), num(&h)) + && (1.0 / lightcraft_geom::MAX_RATIO..=lightcraft_geom::MAX_RATIO).contains(&(x / y)) + { + let _ = app.run("crop.aspect", json!({"aspect": [x, y]})); + ui.close(); + } + } + ui.data_mut(|d| d.insert_temp(key, (w, h))); +} + fn crop(app: &mut LightcraftApp, ui: &mut egui::Ui, id: PhotoId) { let d = app.session.develop_of(id).unwrap_or_default(); header(ui, "Crop"); padded(ui, |ui| { + let original = app.session.catalog.photo(id).map(|p| { + let (w, h) = (p.width.max(1) as f64, p.height.max(1) as f64); + if d.orientation.swaps_axes() { h / w } else { w / h } + }); ui.horizontal(|ui| { ui.label(crate::i18n::tr("Aspect Ratio")); - let cur = d.crop.aspect.map(|(w, h)| format!("{} × {}", w as f64 / 100.0, h as f64 / 100.0)).unwrap_or_else(|| "Free".into()); - let r = ui.add(egui::Button::new(crate::i18n::tr(&cur)).frame(false)); - register(ui.ctx(), "button:cropAspect", r.rect); + let cur = aspect_label(d.crop.aspect, original); + let t = Tokens::get(ui.ctx()); + let r = crate::widgets::dropdown(ui, "cropAspect", crate::i18n::tr(&cur), t.font(12.5), t.text); + egui::Popup::menu(&r).show(|ui| { - for (label, a) in [ - ("Free", "free"), - ("Original", "original"), - ("1 × 1", "1x1"), - ("4 × 5 / 8 × 10", "4x5"), - ("8.5 × 11", "8.5x11"), - ("5 × 7", "5x7"), - ("2 × 3 / 4 × 6", "2x3"), - ("4 × 3", "4x3"), - ("16 × 9", "16x9"), - ("16 × 10", "16x10"), - ] { + for (label, a) in ASPECT_PRESETS { if ui.button(crate::i18n::tr(label)).clicked() { let _ = app.run("crop.aspect", json!({"aspect": a})); } } + ui.separator(); + custom_aspect(app, ui); }); + let locked = d.crop.aspect.is_some(); + if text_button(ui, "cropLock", if locked { "Locked" } else { "Lock" }, locked).clicked() { + let _ = app.run("crop.aspect", json!({"aspect": "toggle"})); + } }); ui.add_space(6.0); ui.horizontal_wrapped(|ui| { @@ -971,6 +1037,23 @@ fn activity(app: &mut LightcraftApp, ui: &mut egui::Ui, id: PhotoId) { #[cfg(test)] mod tests { + // Feature: the aspect button tells what is locked + #[test] + fn aspect_label_names_presets_in_either_orientation() { + assert_eq!(super::aspect_label(None, Some(1.5)), "Free"); + assert_eq!(super::aspect_label(Some((1600, 900)), None), "16 × 9"); + assert_eq!(super::aspect_label(Some((900, 1600)), None), "16 × 9", "portrait 16:9"); + assert_eq!(super::aspect_label(Some((400, 500)), None), "4 × 5 / 8 × 10"); + } + + #[test] + fn aspect_label_prefers_original_over_a_coinciding_preset_and_never_shows_pixel_sizes() { + assert_eq!(super::aspect_label(Some((300, 200)), Some(1.5)), "Original"); + assert_eq!(super::aspect_label(Some((600_000, 400_000)), Some(1.5)), "Original"); + assert_eq!(super::aspect_label(Some((137, 100)), Some(1.5)), "1.37 : 1"); + assert_eq!(super::aspect_label(Some((0, 5)), None), "Free"); + } + #[test] fn short_times() { assert_eq!(super::short_time("2026-09-30T12:00:05"), "Sep 30, 2026, 12:00 PM"); diff --git a/docs/parity.md b/docs/parity.md index 2b58a6297..6c5f92a55 100644 --- a/docs/parity.md +++ b/docs/parity.md @@ -287,8 +287,8 @@ under the UI command that wraps it (a no-op error); the UI command now wins. | Id | Feature | Tier | Status | Evidence | Missing / notes | |---|---|---|---|---|---| -| LR-CROP-RECT | Crop rectangle | P0 | ✅ | `cmd:crop.set`, `crates/ui-egui/src/panels/detail.rs` | double-click inside the box applies the crop (like Return) | -| LR-CROP-ASPECT | Aspect ratios | P0 | ✅ | `cmd:crop.aspect`, `cmd:crop.rotateAspect` | no "As Shot"; custom ratio via command params only | +| LR-CROP-RECT | Crop rectangle | P0 | ✅ | `cmd:crop.set`, `cmd:crop.drag`, `crates/geom/src/crop_drag.rs`, `crates/ui-egui/src/panels/detail.rs` | double-click inside the box applies the crop (like Return) | +| LR-CROP-ASPECT | Aspect ratios | P0 | ✅ | `cmd:crop.aspect`, `cmd:crop.rotateAspect`, `crates/ui-egui/src/panels/right.rs` | menu with presets + custom W × H, visible Lock toggle (also A), button names the active ratio; every handle honours the lock and the drag stops at the image edge instead of shifting the frame (issue #295); "Original" is the photo's own shape (no separate As Shot) | | LR-CROP-STRAIGHTEN | Straighten tool | P0 | ✅ | `cmd:crop.straighten`, `cmd:crop.autoStraighten` | Straighten Tool button: drag along a horizon/vertical; double-click or Auto levels automatically | | LR-CROP-AUTO | Auto straighten | P1 | ✅ | `cmd:crop.autoStraighten` | crop-angle leveling from detected horizon/plumb lines (consensus required) | | LR-CROP-ANGLE | Angle slider | P0 | ✅ | `ctl:crop.angle` | |