Transform commands: a string for an optional number is an error (#1952)

object.setBounds read x, y, width and height with f64_or, which falls back
to the current value for anything that isn't a JSON number, so
{"width": "60 mm"} returned ok and changed nothing. object.move,
object.nudge, object.rotate, object.scale (sy) and object.shear did the
same with their optional numbers. A value that is neither a number nor
null is now a BadParams error naming the key, as a missing required
number already is.

crates/engine/src/cmd/mod.rs

@@ -308,6 +308,17 @@ pub(crate) fn f64_or(p: &Value, key: &str, default: f64) -> f64 {
 pub(crate) fn f64_req(p: &Value, key: &str, cmd: &str) -> Result<f64> {
     p.get(key).and_then(Value::as_f64).ok_or_else(|| bad(cmd, format!("missing number `{key}`")))
 }
+/// Error unless each of `keys` that `p` gives is a number (or null): an optional number read with
+/// [`f64_or`] would otherwise fall back to its default for a string such as `"60 mm"` (#1952).
+pub(crate) fn check_numbers(p: &Value, keys: &[&str], cmd: &str) -> Result<()> {
+    for key in keys {
+        match p.get(*key) {
+            None | Some(Value::Null) | Some(Value::Number(_)) => {}
+            Some(v) => return Err(bad(cmd, format!("`{key}` must be a number, not {v}"))),
+        }
+    }
+    Ok(())
+}
 pub(crate) fn bool_or(p: &Value, key: &str, default: bool) -> bool {
     p.get(key).and_then(Value::as_bool).unwrap_or(default)
 }

crates/engine/src/cmd/object.rs

@@ -396,11 +396,13 @@ fn transform(s: &mut Session, p: &Value) -> Result<Value> {
 }
 
 fn move_cmd(s: &mut Session, p: &Value) -> Result<Value> {
+    check_numbers(p, &["dx", "dy"], "object.move")?;
     let ids = selected_roots(s)?;
     apply_transform(s, "Move", ids, Affine::translate((f64_or(p, "dx", 0.0), f64_or(p, "dy", 0.0))), p, true)
 }
 
 fn nudge(s: &mut Session, p: &Value) -> Result<Value> {
+    check_numbers(p, &["dx", "dy"], "object.nudge")?;
     let k = s.prefs.keyboard_increment * if bool_or(p, "big", false) { 10.0 } else { 1.0 };
     let dx = f64_or(p, "dx", 0.0) * k;
     let dy = f64_or(p, "dy", 0.0) * k;
@@ -428,6 +430,7 @@ fn about(o: Point, a: Affine) -> Affine {
 }
 
 fn rotate(s: &mut Session, p: &Value) -> Result<Value> {
+    check_numbers(p, &["angle"], "object.rotate")?;
     let ids = selected_roots(s)?;
     let o = origin_of(s, p, &ids)?;
     let mut angle = f64_or(p, "angle", 0.0);
@@ -440,6 +443,7 @@ fn rotate(s: &mut Session, p: &Value) -> Result<Value> {
 }
 
 fn scale(s: &mut Session, p: &Value) -> Result<Value> {
+    check_numbers(p, &["sy"], "object.scale")?;
     let ids = selected_roots(s)?;
     let o = origin_of(s, p, &ids)?;
     let sx = f64_req(p, "sx", "object.scale")? / 100.0;
@@ -470,6 +474,7 @@ fn reflect(s: &mut Session, p: &Value) -> Result<Value> {
 }
 
 fn shear(s: &mut Session, p: &Value) -> Result<Value> {
+    check_numbers(p, &["angle"], "object.shear")?;
     let ids = selected_roots(s)?;
     let o = origin_of(s, p, &ids)?;
     let t = f64_or(p, "angle", 0.0).clamp(-89.0, 89.0).to_radians().tan();
@@ -1436,6 +1441,7 @@ fn distribute_spacing(s: &mut Session, p: &Value) -> Result<Value> {
 }
 
 fn set_bounds(s: &mut Session, p: &Value) -> Result<Value> {
+    check_numbers(p, &["x", "y", "width", "height"], "object.setBounds")?;
     let ids = selected_roots(s)?;
     let st = s.doc()?;
     if transforms_objects(p)

crates/engine/src/tests.rs

@@ -914,6 +914,42 @@ fn transforms_act_on_direct_selected_anchors() {
     assert_eq!(points(&s), [(25.0, 0.0), (75.0, 0.0), (100.0, 100.0), (0.0, 100.0)]);
 }
 
+/// #1952: an optional number given as a string (`"60 mm"`) is an error naming the key, not a value
+/// silently ignored; the object and its undo history stay as they were.
+#[test]
+fn a_string_for_an_optional_transform_number_is_an_error() {
+    let mut s = session();
+    let a = rect(&mut s, 10.0, 10.0, 50.0, 40.0);
+    s.execute("select.set", &json!({"ids": [a.0]})).unwrap();
+    let before = history(&s);
+    let bounds = |s: &Session| s.doc().unwrap().doc.bounds_of(&[a], false).unwrap();
+    let box0 = bounds(&s);
+    let calls = [
+        ("object.setBounds", json!({"width": "60 mm"})),
+        ("object.setBounds", json!({"x": "abc"})),
+        ("object.move", json!({"dx": "5"})),
+        ("object.nudge", json!({"dy": true})),
+        ("object.rotate", json!({"angle": "45"})),
+        ("object.scale", json!({"sx": 50, "sy": "50%"})),
+        ("object.shear", json!({"angle": [10]})),
+    ];
+    for (cmd, p) in calls {
+        let key = p.as_object().unwrap().keys().rfind(|k| !p[*k].is_number()).unwrap().clone();
+        match s.execute(cmd, &p) {
+            Err(EngineError::BadParams { cmd: c, msg }) => {
+                assert_eq!(c, cmd);
+                assert!(msg.contains(&format!("`{key}`")), "{cmd}: {msg}");
+            }
+            other => panic!("{cmd} {p}: expected BadParams, got {other:?}"),
+        }
+        assert_eq!(history(&s), before, "{cmd} {p}");
+        assert_eq!(bounds(&s), box0, "{cmd} {p}");
+    }
+    // Numbers (and null, as when the key is left out) still work.
+    s.execute("object.setBounds", &json!({"width": 60, "height": null})).unwrap();
+    assert!((bounds(&s).width() - 60.0).abs() < 1e-9);
+}
+
 /// #1824: with anchors selected with Direct Selection, the Transform panel's X/Y/W/H
 /// (`object.setBounds`) are the box around those anchors, and only they move or scale.
 #[test]