From e6170de1a283d3e4eba8aa175178e1435918d0b7 Mon Sep 17 00:00:00 2001 From: Gert Pellin Date: Wed, 14 Oct 2026 17:07:40 +0200 Subject: [PATCH] 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 | 11 ++++++++++ crates/engine/src/cmd/object.rs | 6 ++++++ crates/engine/src/tests.rs | 36 +++++++++++++++++++++++++++++++++ 3 files changed, 53 insertions(+) diff --git a/crates/engine/src/cmd/mod.rs b/crates/engine/src/cmd/mod.rs index df9e70a08..f12e5939f 100644 --- a/crates/engine/src/cmd/mod.rs +++ b/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 { 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) } diff --git a/crates/engine/src/cmd/object.rs b/crates/engine/src/cmd/object.rs index 4f94dcac8..d1628b242 100644 --- a/crates/engine/src/cmd/object.rs +++ b/crates/engine/src/cmd/object.rs @@ -396,11 +396,13 @@ fn transform(s: &mut Session, p: &Value) -> Result { } fn move_cmd(s: &mut Session, p: &Value) -> Result { + 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 { + 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 { + 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 { } fn scale(s: &mut Session, p: &Value) -> Result { + 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 { } fn shear(s: &mut Session, p: &Value) -> Result { + 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 { } fn set_bounds(s: &mut Session, p: &Value) -> Result { + check_numbers(p, &["x", "y", "width", "height"], "object.setBounds")?; let ids = selected_roots(s)?; let st = s.doc()?; if transforms_objects(p) diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 545800a3e..3c51e3a85 100644 --- a/crates/engine/src/tests.rs +++ b/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]