From be34283943e0170fdf508b693b09fc5f38e45e6c Mon Sep 17 00:00:00 2001 From: Marco Arnulfo Date: Sun, 18 Oct 2026 13:15:57 +0200 Subject: [PATCH] crop.set checks the minimum on the crop it stores, so an oversized rect can't come out a sliver (part of #1952) --- crates/engine/src/cmd/develop.rs | 24 ++++++++++++++++-------- crates/engine/src/tests.rs | 9 ++++++--- 2 files changed, 22 insertions(+), 11 deletions(-) diff --git a/crates/engine/src/cmd/develop.rs b/crates/engine/src/cmd/develop.rs index 33e3c4a92..00378dd13 100644 --- a/crates/engine/src/cmd/develop.rs +++ b/crates/engine/src/cmd/develop.rs @@ -524,20 +524,22 @@ pub fn specs() -> Vec { cmd!("crop.set", "Set Crop", [], None, "{rect?: [x0,y0,x1,y1] normalized in the straightened frame, angle?: degrees}", has_active, |s, p| { let id = active(s, "crop.set")?; let (w, h) = crop_dims(s, id); + // the smallest crop a drag of the crop tool can make: below it an export comes out 1×1 (#1952) + let min_side = MIN_CROP_FRACTION * w.min(h) * (1.0 - 1e-9); + let too_small = |r: Rect| { + (r.width() * w < min_side || r.height() * h < min_side) + .then(|| bad("crop.set", format!("rect is smaller than the smallest crop ({} % of the shorter side)", MIN_CROP_FRACTION * 100.0))) + }; edit(s, "crop.set", "Crop", |d| { - if let Some(a) = p.get("rect").and_then(Value::as_array) { + let explicit = p.get("rect").and_then(Value::as_array); + if let Some(a) = explicit { let v: Vec = a.iter().filter_map(Value::as_f64).collect(); if v.len() != 4 { return Err(bad("crop.set", "rect needs 4 numbers")); } let r = Rect::new(v[0].min(v[2]), v[1].min(v[3]), v[0].max(v[2]), v[1].max(v[3])); - // the smallest crop a drag of the crop tool can make: below it an export comes out 1×1 (#1952) - let min_side = MIN_CROP_FRACTION * w.min(h) * (1.0 - 1e-9); - if r.width() * w < min_side || r.height() * h < min_side { - return Err(bad( - "crop.set", - format!("rect is smaller than the smallest crop ({} % of the shorter side)", MIN_CROP_FRACTION * 100.0), - )); + if let Some(e) = too_small(r) { + return Err(e); } d.crop.geometry.rect = r; } @@ -547,6 +549,12 @@ pub fn specs() -> Vec { (None, Some(a)) => d.crop.geometry.with_angle(a, w, h), (_, a) => CropGeometry { angle: a.unwrap_or(d.crop.geometry.angle), ..d.crop.geometry }.constrained(w, h), }; + // a rect far larger than the photo is shrunk to fit it, and can come out a sliver + if explicit.is_some() + && let Some(e) = too_small(d.crop.geometry.rect) + { + return Err(e); + } Ok(()) }) }), diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index f02032634..e3b14f99c 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -566,12 +566,13 @@ fn rotate_aspect_of_degenerate_crop_stays_finite() { } } -/// `crop.set` with a huge rect shrank it by a scale of 0 (`inf * 0`) and stored a NaN crop. +/// `crop.set` with a huge rect shrank it by a scale of 0 (`inf * 0`) and stored a NaN crop. Their +/// maths overflow, so they fall back to a full-size crop and pass the minimum size. #[test] fn crop_set_huge_rect_stays_finite() { for rect in [[0.0, 0.0, 1e308, 1e308], [-1e308, -1e308, 1e308, 1e308]] { let mut s = demo(); - let _ = s.execute("crop.set", &json!({"rect": rect})); + s.execute("crop.set", &json!({"rect": rect})).unwrap(); let r = active_dev(&s).crop.geometry.rect; assert!([r.x0, r.y0, r.x1, r.y1].iter().all(|v| v.is_finite() && (-1e-6..=1.0 + 1e-6).contains(v)), "{rect:?}: {r:?}"); } @@ -579,12 +580,14 @@ fn crop_set_huge_rect_stays_finite() { /// `crop.set` took a crop of no size (or a sliver), and the next export wrote a 1×1 image (#1952). /// It now refuses a crop smaller than a drag of the crop tool can make, and keeps the one it had. +/// A rect far wider than the photo is a sliver too once it is shrunk to fit inside it. #[test] fn crop_set_refuses_a_crop_smaller_than_the_tool_allows() { let mut s = demo(); s.execute("crop.set", &json!({"rect": [0.1, 0.1, 0.9, 0.9]})).unwrap(); let before = active_dev(&s).crop.geometry; - for rect in [[0.5, 0.5, 0.5, 0.5], [0.2, 0.2, 0.2, 0.8], [0.2, 0.4, 0.8, 0.4], [0.0, 0.0, 1e-9, 1.0]] { + for rect in [[0.5, 0.5, 0.5, 0.5], [0.2, 0.2, 0.2, 0.8], [0.2, 0.4, 0.8, 0.4], [0.0, 0.0, 1e-9, 1.0], [0.0, 0.0, 1e6, 1.0], [0.0, 0.0, 1.0, 1e6]] + { let e = s.execute("crop.set", &json!({"rect": rect})).unwrap_err(); assert!(e.to_string().contains("smaller"), "{rect:?}: {e}"); assert_eq!(active_dev(&s).crop.geometry, before, "{rect:?}");