Merge pull request #9280 from davidmann4/fix/crop-invalid-rect

Crop: refuse an invalid explicit rectangle instead of cropping to the selection (#9279)

crates/engine/src/cmd/image_cmds.rs

@@ -780,6 +780,7 @@ fn crop_with(s: &mut Session, p: &Value, keep_background: bool) -> Result<Value>
     let unlock = !delete && !content_aware && !keep_background;
     // Values beyond i32 would wrap through the narrowing casts into a rectangle unrelated to
     // the numbers passed; reject them (see `commands::int_i32`).
+    let given = ["x", "y", "width", "height"].into_iter().any(|k| p.get(k).is_some_and(|v| !v.is_null()));
     let explicit = match (
         crate::commands::int_i32("image.crop", p, "x")?,
         crate::commands::int_i32("image.crop", p, "y")?,
@@ -800,6 +801,14 @@ fn crop_with(s: &mut Session, p: &Value, keep_background: bool) -> Result<Value>
             };
             Some(Rect::new(x, y, end(x, w, "x", "width")?, end(y, h, "y", "height")?))
         }
+        // A rectangle that is passed but incomplete or empty is refused: falling back to the
+        // selection would crop somewhere the caller didn't ask for (#9279).
+        _ if given => {
+            return Err(bad(
+                "image.crop",
+                "`x`, `y`, `width` and `height` go together: pass all four as numbers of pixels, with `width` and `height` rounding to at least 1",
+            ));
+        }
         _ => None,
     };
     let target = crop_target(p)?;
@@ -1231,7 +1240,7 @@ pub fn specs() -> Vec<CommandSpec> {
             "image.crop",
             "Crop",
             ["Image"],
-            r##"{"x":px,"y":px,"width":px,"height":px,"angle":-3600..3600=0,"deleteCroppedPixels":bool=true,"contentAware":bool=false,"targetWidth":px,"targetHeight":px,"resolution":ppi,"resample":"bicubic|bilinear|nearest|lanczos|preserveDetails"="bicubic"} → {x,y,width,height,documentWidth,documentHeight,resolution} (targetWidth/targetHeight, both or neither: the cropped area is resampled to exactly that size, as Image Size, in the same undo step; resolution: the document's new ppi; deleteCroppedPixels false: the pixels past the frame are kept and, unless contentAware, the Background becomes a normal layer, "Layer 0", so an extension is transparent on every layer (with contentAware the Background stays); contentAware: the Crop tool's Fill › Content-Aware Fill uses surrounding pixels to fill areas uncovered by a rotated or extending crop. The fill targets the Background, or the active pixel layer if there is no Background. With Delete Cropped Pixels on, it also targets a separate active pixel layer if it is visible and unlocked.)"##,
+            r##"{"x":px,"y":px,"width":px,"height":px,"angle":-3600..3600=0,"deleteCroppedPixels":bool=true,"contentAware":bool=false,"targetWidth":px,"targetHeight":px,"resolution":ppi,"resample":"bicubic|bilinear|nearest|lanczos|preserveDetails"="bicubic"} → {x,y,width,height,documentWidth,documentHeight,resolution} (x/y/width/height, all four or none: without them the selection's bounds are cropped to; targetWidth/targetHeight, both or neither: the cropped area is resampled to exactly that size, as Image Size, in the same undo step; resolution: the document's new ppi; deleteCroppedPixels false: the pixels past the frame are kept and, unless contentAware, the Background becomes a normal layer, "Layer 0", so an extension is transparent on every layer (with contentAware the Background stays); contentAware: the Crop tool's Fill › Content-Aware Fill uses surrounding pixels to fill areas uncovered by a rotated or extending crop. The fill targets the Background, or the active pixel layer if there is no Background. With Delete Cropped Pixels on, it also targets a separate active pixel layer if it is visible and unlocked.)"##,
             has_doc,
             crop
         ),

crates/engine/src/cmd/image_cmds/tests.rs

@@ -429,6 +429,101 @@ fn crop_to_explicit_rect_can_extend_canvas() {
     assert_eq!(doc(&s).layers[1].surface().unwrap().pixel(20, 5), vec![1.0, 0.0, 0.0, 1.0]);
 }
 
+/// A rectangle that is passed but invalid is refused, not replaced by the selection (#9279).
+#[test]
+fn crop_refuses_a_bad_explicit_rect_instead_of_cropping_to_the_selection() {
+    let bad = [
+        json!({"x": 10, "y": 10, "width": 0, "height": 50}),
+        json!({"x": 10, "y": 10, "width": 20, "height": 0}),
+        json!({"x": 10, "y": 10, "width": -50, "height": 50}),
+        json!({"x": 10, "y": 10, "width": "100", "height": 50}),
+        json!({"x": 10, "y": 10, "width": true, "height": 50}),
+        json!({"width": 20, "height": 10}),
+        json!({"x": 1, "y": 1}),
+        json!({"x": 1, "y": null, "width": 20, "height": 10}),
+        json!({"height": 10}),
+    ];
+    for p in bad {
+        for selection in [true, false] {
+            let mut s = session();
+            if selection {
+                s.execute("select.rect", json!({"x": 5, "y": 2, "width": 20, "height": 10})).unwrap();
+            }
+            let (size, past) = (doc(&s).size, s.active().unwrap().history.past_len());
+            let err = s.execute("image.crop", p.clone()).unwrap_err();
+            assert!(matches!(err, EngineError::BadParams { .. }), "{p} (selection: {selection}): {err:?}");
+            assert_eq!(doc(&s).size, size, "{p} (selection: {selection})");
+            assert_eq!(doc(&s).selection.is_some(), selection, "{p}: the selection stays");
+            assert_eq!(s.active().unwrap().history.past_len(), past, "{p} (selection: {selection})");
+        }
+    }
+    // No rectangle at all (or every side null) still crops to the selection.
+    for p in [json!({}), json!({"x": null, "y": null, "width": null, "height": null})] {
+        let mut s = session();
+        s.execute("select.rect", json!({"x": 5, "y": 2, "width": 20, "height": 10})).unwrap();
+        s.execute("image.crop", p.clone()).unwrap();
+        assert_eq!(doc(&s).size, Size::new(20, 10), "{p}");
+    }
+    // Whole-number floats, as the Crop tool sends them, are a valid rectangle.
+    let mut s = session();
+    s.execute("select.rect", json!({"x": 5, "y": 2, "width": 20, "height": 10})).unwrap();
+    s.execute("image.crop", json!({"x": 1.0, "y": 1.0, "width": 30.0, "height": 15.0})).unwrap();
+    assert_eq!(doc(&s).size, Size::new(30, 15));
+    // Fractional pixels round, like the rectangle's `x`/`y` and every other pixel param
+    // (`commands::int`); a side that rounds to 0 is refused.
+    let mut s = session();
+    s.execute("image.crop", json!({"x": 0.4, "y": 0.6, "width": 20.6, "height": 9.5})).unwrap();
+    assert_eq!(doc(&s).size, Size::new(21, 10));
+    let err = s.execute("image.crop", json!({"x": 0, "y": 0, "width": 0.4, "height": 5})).unwrap_err();
+    assert!(matches!(err, EngineError::BadParams { .. }), "{err:?}");
+    assert_eq!(doc(&s).size, Size::new(21, 10));
+}
+
+/// The explicit rectangle of #9279's edge cases, each with a selection elsewhere that a fallback
+/// would crop to: `0` is a given origin, numeric strings are not numbers (refused, as `x`/`y`
+/// already were), and a rectangle partly past the canvas is used as passed, never clamped.
+#[test]
+fn crop_rect_edge_cases_never_fall_back_to_the_selection() {
+    let with_selection = || {
+        let mut s = session();
+        s.execute("select.rect", json!({"x": 5, "y": 2, "width": 20, "height": 10})).unwrap();
+        s
+    };
+    // `x`/`y` of 0 count as given: the red square (10, 5)..(20, 15) stays where it was, where a
+    // crop to the selection would have moved it to (5, 3).
+    let mut s = with_selection();
+    s.execute("image.crop", json!({"x": 0, "y": 0, "width": 15, "height": 10})).unwrap();
+    assert_eq!(doc(&s).size, Size::new(15, 10));
+    assert_eq!(doc(&s).layers[1].surface().unwrap().content_bounds(), Rect::new(10, 5, 15, 10));
+    // Numeric strings are refused on every side, with or without a selection.
+    for p in [
+        json!({"x": "0", "y": "0", "width": "15", "height": "10"}),
+        json!({"x": "0", "y": 0, "width": 15, "height": 10}),
+        json!({"x": 0, "y": 0, "width": 15, "height": "10"}),
+    ] {
+        for mut s in [with_selection(), session()] {
+            let err = s.execute("image.crop", p.clone()).unwrap_err();
+            assert!(matches!(err, EngineError::BadParams { .. }), "{p}: {err:?}");
+            assert_eq!(doc(&s).size, Size::new(40, 20), "{p}");
+        }
+    }
+    // Partly outside the canvas: the canvas grows to the rectangle, not to its overlap with the
+    // canvas or the selection.
+    let mut s = with_selection();
+    s.execute("image.crop", json!({"x": -10, "y": -5, "width": 30, "height": 40})).unwrap();
+    assert_eq!(doc(&s).size, Size::new(30, 40));
+    assert_eq!(doc(&s).layers[1].surface().unwrap().pixel(20, 10), vec![1.0, 0.0, 0.0, 1.0]);
+    // Reveal All passes all four sides through the same check (here x = y = 0: the hidden pixels
+    // are past the right and bottom edges), so a selection doesn't change what it reveals.
+    let mut s = with_selection();
+    s.execute("image.canvasSize", json!({"width": 12, "height": 8, "anchor": "topLeft"})).unwrap();
+    s.execute("select.rect", json!({"x": 2, "y": 2, "width": 4, "height": 4})).unwrap();
+    assert_eq!(s.execute("image.revealAll", json!({})).unwrap()["changed"], true);
+    let size = doc(&s).size;
+    assert!(size.width >= 20 && size.height >= 15, "{size:?}: the red square is revealed");
+    assert_eq!(doc(&s).layers[1].surface().unwrap().pixel(19, 14), vec![1.0, 0.0, 0.0, 1.0]);
+}
+
 #[test]
 fn crop_extension_fills_the_background_with_the_background_colour() {
     let mut shrink = Session::new();