Transform Each, Rasterize, Mosaic, Trim Marks and Rearrange: a number given as text is refused

Transform Each, Rasterize, Create Object Mosaic, Create Trim Marks and
Rearrange All Artboards read their optional numbers with f64_or, which
falls back to a default for a value that is not a JSON number.
object.transformEach {"scaleH": "50"} left the selected objects at
their size and reported success. Rasterize did the same with ppi,
Create Object Mosaic did the same with width, height, spacingX and
spacingY, Create Trim Marks did the same with offset, length, bleed and
weight, and Rearrange All Artboards did the same with spacing. Crop
Image cropped to the artboard when rect was a string or another value
that is not an array.

These commands now fail with "`<key>` must be a number, not <value>"
for a value that is not a number or null, as the transform commands
do, and change nothing. Transform Each checks scaleH, scaleV, moveH,
moveV and rotate. Crop Image refuses a rect that is not an array of
four numbers with "rect must be [x, y, width, height]", the message it
already gave for an array of the wrong length; a null rect crops to the
artboard, as a missing one does.

Docs: docs/mcp.md, docs/target-app-parity.md.

crates/engine/src/cmd/menucmds.rs

@@ -341,6 +341,7 @@ pub(crate) fn transform_each_again(s: &mut Session, p: &Value) -> Result<Value>
 /// repeats; a dialog's preview records it as it commits (`Session::commit_interaction`).
 fn transform_each_as(s: &mut Session, p: &Value, label: &str) -> Result<Value> {
     let roots = selected_roots(s)?;
+    check_numbers(p, &["scaleH", "scaleV", "moveH", "moveV", "rotate"], "object.transformEach")?;
     // Bounded so coordinates stay finite (and the document stays serializable).
     let sh = f64_or(p, "scaleH", 100.0).clamp(-100_000.0, 100_000.0);
     let sv = f64_or(p, "scaleV", 100.0).clamp(-100_000.0, 100_000.0);
@@ -480,6 +481,7 @@ impl Rasterized {
 /// `cmd`, each defaulting to `doc`'s raster effects settings: over the bounds of what the art draws
 /// ([`vectorcraft_render::drawn_bounds`]) and the room around them, in whole pixels.
 pub(crate) fn rasterize_art(doc: &vectorcraft_doc::Document, nodes: Vec<Node>, p: &Value, cmd: &str) -> Result<Rasterized> {
+    check_numbers(p, &["ppi"], cmd)?;
     let ppi = f64_or(p, "ppi", doc.raster_effects_ppi);
     if !(1.0..=2400.0).contains(&ppi) {
         return Err(bad(cmd, "ppi must be between 1 and 2400"));
@@ -600,6 +602,7 @@ fn object_mosaic(s: &mut Session, p: &Value) -> Result<Value> {
     let colors = vectorcraft_trace::mosaic(&raster, cols, rows);
     // Tiles in image space, placed by the image transform (scaled to a new size if asked).
     let b = node.geometric_bounds().ok_or_else(|| bad(C, "image has no bounds"))?;
+    check_numbers(p, &["width", "height", "spacingX", "spacingY"], C)?;
     let (nw, nh) = (f64_or(p, "width", b.width()).max(0.1), f64_or(p, "height", b.height()).max(0.1));
     let resize = Affine::translate(b.origin().to_vec2())
         * Affine::scale_non_uniform(nw / b.width().max(1e-9), nh / b.height().max(1e-9))
@@ -671,8 +674,8 @@ fn crop_image(s: &mut Session, p: &Value) -> Result<Value> {
         return trim_image(s, id, (**im).clone());
     }
     let ib = node.geometric_bounds().ok_or_else(|| bad(C, "image has no bounds"))?;
-    let rect = match p.get("rect").and_then(Value::as_array) {
-        Some(a) if a.len() == 4 => {
+    let rect = match p.get("rect").filter(|v| !v.is_null()) {
+        Some(Value::Array(a)) if a.len() == 4 => {
             let v: Vec<f64> = a.iter().filter_map(Value::as_f64).collect();
             if v.len() != 4 {
                 return Err(bad(C, "rect must be [x, y, width, height]"));
@@ -832,6 +835,7 @@ impl Session {
 
 fn trim_marks(s: &mut Session, p: &Value) -> Result<Value> {
     const C: &str = "object.createTrimMarks";
+    check_numbers(p, &["offset", "length", "bleed", "weight"], C)?;
     let style = match str_param(p, "style") {
         Some(v) => MarkStyle::parse(v).ok_or_else(|| bad(C, "`style` must be roman or japanese"))?,
         None => s.crop_mark_style(),
@@ -1001,6 +1005,7 @@ fn rearrange_artboards(s: &mut Session, p: &Value) -> Result<Value> {
         L::Row => (1, n_ab),
         L::Column => (n_ab, 1),
     };
+    check_numbers(p, &["spacing"], CMD)?;
     let spacing = f64_or(p, "spacing", 20.0).clamp(-1.0e5, 1.0e5);
     let move_art = bool_or(p, "moveArtwork", true);
     let locked_and_hidden = s.prefs.move_locked_with_artboard;

crates/engine/src/tests.rs

@@ -2452,6 +2452,29 @@ fn object_mosaic_refuses_columns_and_rows_that_are_not_whole_numbers() {
     assert_eq!(r["tiles"], json!(2));
 }
 
+/// A new width or height, or a spacing, given as text or as another value that is not a number is an error naming the
+/// key, and no mosaic is made.
+#[test]
+fn object_mosaic_refuses_a_size_or_spacing_given_as_text() {
+    let mut s = session();
+    rect(&mut s, 0.0, 0.0, 100.0, 50.0);
+    s.execute("object.rasterize", &json!({"ppi": 72})).unwrap();
+    let undo = |s: &Session| s.doc().unwrap().history.undo.len();
+    let before = undo(&s);
+    for (p, msg) in [
+        (json!({"width": "200"}), "`width` must be a number, not \"200\""),
+        (json!({"columns": 2, "height": "50 pt"}), "`height` must be a number, not \"50 pt\""),
+        (json!({"spacingX": "4"}), "`spacingX` must be a number, not \"4\""),
+        (json!({"spacingY": [4]}), "`spacingY` must be a number, not [4]"),
+    ] {
+        let r = s.execute("object.createObjectMosaic", &p);
+        assert!(matches!(&r, Err(EngineError::BadParams { msg: m, .. }) if m == msg), "{p}: {r:?}");
+        assert_eq!(undo(&s), before, "{p}");
+    }
+    let r = s.execute("object.createObjectMosaic", &json!({"columns": 2, "rows": 1, "width": 200, "spacingX": null})).unwrap();
+    assert_eq!(r["tiles"], json!(2));
+}
+
 #[test]
 fn snap_to_pixel_rounds_drawing_and_moves() {
     let mut s = session();

crates/engine/src/tests_menucmds.rs

@@ -83,6 +83,13 @@ fn close(a: f64, b: f64) -> bool {
     (a - b).abs() < 1e-6
 }
 
+/// `cmd` with `p` fails with the error for `key`, which `p` gives as something other than a number.
+fn refuses_non_number(s: &mut Session, cmd: &str, p: &Value, key: &str) {
+    let r = s.execute(cmd, p);
+    let want = format!("`{key}` must be a number, not {}", p[key]);
+    assert!(matches!(&r, Err(EngineError::BadParams { msg, .. }) if *msg == want), "{cmd} {p}: {r:?}");
+}
+
 // ---------- Object → Lock / Hide ----------
 
 #[test]
@@ -399,6 +406,29 @@ fn transform_each_refuses_a_reference_or_seed_that_is_not_a_whole_number() {
     assert_eq!(bounds(&s, a), Rect::new(0.0, 0.0, 50.0, 50.0));
 }
 
+/// Transform Each refuses a scale, move or rotation given as text or as another value that is not a
+/// number, and the object stays as it was; numbers for the scales and the horizontal move transform it.
+#[test]
+fn transform_each_refuses_a_number_given_as_text() {
+    let mut s = session();
+    let a = rect(&mut s, 0.0, 0.0, 100.0, 100.0);
+    sel(&mut s, &[a]);
+    let before = (undo_len(&s), bounds(&s, a));
+    for (key, p) in [
+        ("scaleH", json!({"scaleH": "50"})),
+        ("scaleV", json!({"scaleH": 50, "scaleV": "50%"})),
+        ("moveH", json!({"moveH": "10"})),
+        ("moveV", json!({"moveH": 10, "moveV": [5]})),
+        ("rotate", json!({"rotate": "45"})),
+        ("rotate", json!({"moveH": 10, "rotate": true})),
+    ] {
+        refuses_non_number(&mut s, "object.transformEach", &p, key);
+        assert_eq!((undo_len(&s), bounds(&s, a)), before, "{p}");
+    }
+    s.execute("object.transformEach", &json!({"scaleH": 50, "scaleV": 50, "moveH": 10, "moveV": null})).unwrap();
+    assert_eq!(bounds(&s, a), Rect::new(35.0, 25.0, 85.0, 75.0));
+}
+
 /// #1910: Transform Each's Reflect X flips left-right; Reflect Y flips top-bottom (same as
 /// Effect › Distort › Transform and Illustrator's Transform Each checkboxes).
 #[test]
@@ -571,6 +601,22 @@ fn rasterize_replaces_selection_with_image() {
     assert_eq!(selected(&s), vec![img]);
 }
 
+/// Rasterize refuses a resolution given as text or as another value that is not a number, and the
+/// selected art stays as it was.
+#[test]
+fn rasterize_refuses_a_ppi_given_as_text() {
+    let mut s = session();
+    let a = rect(&mut s, 10.0, 20.0, 40.0, 30.0);
+    sel(&mut s, &[a]);
+    let before = (undo_len(&s), node(&s, a));
+    for p in [json!({"ppi": "144"}), json!({"ppi": "300 ppi", "padding": 0}), json!({"ppi": [72]})] {
+        refuses_non_number(&mut s, "object.rasterize", &p, "ppi");
+        assert_eq!((undo_len(&s), node(&s, a)), before, "{p}");
+    }
+    let img = id_of(&s.execute("object.rasterize", &json!({"ppi": 144, "padding": 0})).unwrap());
+    assert!(matches!(node(&s, img).kind, NodeKind::Image(_)));
+}
+
 #[test]
 fn rasterize_says_when_an_image_could_not_be_decoded() {
     let mut s = session();
@@ -608,6 +654,25 @@ fn crop_image_to_rect() {
     assert!(s.execute("object.cropImage", &json!({})).is_err());
 }
 
+/// Crop Image refuses a `rect` that is not an array, such as text, and the image stays as it was; a
+/// null `rect` crops to the artboard as a missing one does.
+#[test]
+fn crop_image_refuses_a_rect_given_as_text() {
+    let mut s = session();
+    let a = rect(&mut s, -20.0, -20.0, 100.0, 100.0);
+    sel(&mut s, &[a]);
+    let img = id_of(&s.execute("object.rasterize", &json!({"ppi": 72})).unwrap());
+    let before = (undo_len(&s), node(&s, img));
+    for p in [json!({"rect": "0, 0, 20, 20"}), json!({"rect": {"x": 0, "y": 0, "width": 20, "height": 20}}), json!({"rect": 20})] {
+        let r = s.execute("object.cropImage", &p);
+        assert!(matches!(&r, Err(EngineError::BadParams { msg, .. }) if msg == "rect must be [x, y, width, height]"), "{p}: {r:?}");
+        assert_eq!((undo_len(&s), node(&s, img)), before, "{p}");
+    }
+    s.execute("object.cropImage", &json!({"rect": null})).unwrap();
+    let b = bounds(&s, img);
+    assert!(close(b.x0, 0.0) && close(b.y0, 0.0), "{b:?}");
+}
+
 #[test]
 fn crop_image_disabled_without_image() {
     let mut s = session();
@@ -656,6 +721,30 @@ fn trim_marks_around_selection() {
     assert!(close(b.x0, vb.x0 - 30.0) && close(b.y1, vb.y1 + 30.0), "{b:?} {vb:?}");
 }
 
+/// Create Trim Marks refuses an offset, length, bleed or weight given as text or as another value
+/// that is not a number, around the selection and around the artboards, and makes no marks.
+#[test]
+fn trim_marks_refuse_a_number_given_as_text() {
+    let mut s = session();
+    let a = rect(&mut s, 100.0, 100.0, 200.0, 100.0);
+    let calls = [
+        ("offset", json!({"offset": "10"})),
+        ("length", json!({"length": "20 pt"})),
+        ("bleed", json!({"style": "japanese", "bleed": "3"})),
+        ("weight", json!({"weight": true})),
+        ("offset", json!({"allArtboards": true, "offset": {"value": 10}})),
+    ];
+    for ids in [vec![a], vec![]] {
+        sel(&mut s, &ids);
+        let before = (undo_len(&s), s.doc().unwrap().doc.layers[0].children().unwrap().len());
+        for (key, p) in &calls {
+            refuses_non_number(&mut s, "object.createTrimMarks", p, key);
+            assert_eq!((undo_len(&s), s.doc().unwrap().doc.layers[0].children().unwrap().len()), before, "{p}");
+        }
+    }
+    s.execute("object.createTrimMarks", &json!({"offset": 10, "length": 20, "bleed": null})).unwrap();
+}
+
 // ---------- Convert to Shape ----------
 
 #[test]
@@ -791,6 +880,22 @@ fn rearrange_refuses_columns_that_are_not_a_whole_number() {
     assert_eq!((r["rows"].clone(), r["columns"].clone()), (json!(2), json!(1)));
 }
 
+/// Rearrange All Artboards refuses a spacing given as text or as another value that is not a number,
+/// and the artboards stay where they were.
+#[test]
+fn rearrange_refuses_a_spacing_given_as_text() {
+    let mut s = session();
+    s.execute("artboard.new", &json!({"x": 1000, "y": 0, "width": 400, "height": 300})).unwrap();
+    let boards = |s: &Session| s.doc().unwrap().doc.artboards.iter().map(|a| a.rect).collect::<Vec<_>>();
+    let before = (boards(&s), undo_len(&s));
+    for p in [json!({"spacing": "50"}), json!({"columns": 1, "spacing": "50 pt"}), json!({"layout": "row", "spacing": false})] {
+        refuses_non_number(&mut s, "artboard.rearrange", &p, "spacing");
+        assert_eq!((boards(&s), undo_len(&s)), before, "{p}");
+    }
+    s.execute("artboard.rearrange", &json!({"columns": 1, "spacing": 50})).unwrap();
+    assert_eq!(boards(&s)[1], Rect::new(0.0, 650.0, 400.0, 950.0));
+}
+
 /// Fit to Artwork Bounds fits each artboard it is given to the art on it, in one undo step: an
 /// object partly on an artboard counts for each artboard it overlaps, art flush with an edge from
 /// outside or on no artboard doesn't count, and an artboard with no art stays as it is.