fix: sketch.text refuses inputs it doesn't understand (#1057)

sketch.text and sketch.edit_text took at, height, angle and text with a silent default when a value had the wrong kind, so a height of "H" made 5 mm text and at: ["H", 25] put it at the origin. An input that is given (not null) now has to be what it names, or the command is refused with the input's name, and a null at no longer passes as the origin.

crates/engine/src/cmd/sketch_create.rs

@@ -145,21 +145,40 @@ fn text_style(p: &Value, cmd: &str, old: TextStyle) -> Result<TextStyle> {
     Ok(style)
 }
 
+/// What a text's `at` must be.
+const AT_MUST: &str = "`at` must be the baseline start [x, y]";
+
+/// The number given for `key`: `None` when it is left out or null, and an error when it is
+/// anything but a number (not read as left out).
+fn given_num(p: &Value, cmd: &str, key: &str, what: &str) -> Result<Option<f64>> {
+    match p.get(key) {
+        None | Some(Value::Null) => Ok(None),
+        Some(_) => num(p, key).map(Some).ok_or_else(|| bad(cmd, format!("`{key}` must be {what}"))),
+    }
+}
+
 fn text_source(p: &Value, cmd: &str, old: Option<(String, Vec2, f64, f64, Option<String>, TextStyle)>) -> Result<LinkSource> {
     let (t0, at0, h0, a0, f0, s0) = old.unwrap_or((String::new(), Vec2::ZERO, 5.0, 0.0, None, TextStyle::default()));
-    let text = str_(p, "text").map(str::to_string).unwrap_or(t0);
+    let text = match p.get("text") {
+        None | Some(Value::Null) => t0,
+        Some(Value::String(t)) => t.clone(),
+        Some(_) => return Err(bad(cmd, "`text` must be a string")),
+    };
     if text.trim().is_empty() {
         return Err(bad(cmd, "`text` must not be empty"));
     }
     if text.chars().count() > solidcraft_sketch::MAX_TEXT_CHARS {
         return Err(bad(cmd, "text too long"));
     }
-    let at = p.get("at").and_then(crate::params::vec2).unwrap_or(at0);
-    let height = num(p, "height").unwrap_or(h0);
+    let at = match p.get("at") {
+        None | Some(Value::Null) => at0,
+        Some(v) => crate::params::vec2(v).ok_or_else(|| bad(cmd, AT_MUST))?,
+    };
+    let height = given_num(p, cmd, "height", "a length in mm")?.unwrap_or(h0);
     if !(height > 1e-6 && height < 1e5) {
         return Err(bad(cmd, "`height` must be positive"));
     }
-    let angle = num(p, "angle").map(f64::to_radians).unwrap_or(a0);
+    let angle = given_num(p, cmd, "angle", "an angle in degrees")?.map_or(a0, f64::to_radians);
     let font = match p.get("font") {
         None | Some(Value::Null) => f0,
         Some(Value::String(f)) if f.trim().is_empty() => None,
@@ -178,8 +197,8 @@ fn text_source(p: &Value, cmd: &str, old: Option<(String, Vec2, f64, f64, Option
 
 fn text(s: &mut Session, p: &Value) -> Result<Value> {
     let cmd = "sketch.text";
-    if p.get("at").is_none() {
-        return Err(bad(cmd, "`at` must be the baseline start [x, y]"));
+    if p.get("at").is_none_or(Value::is_null) {
+        return Err(bad(cmd, AT_MUST));
     }
     let src = text_source(p, cmd, None)?;
     let id = s.active_sketch.ok_or_else(|| bad(cmd, "no sketch is being edited"))?;

crates/engine/src/cmd/sketch_text_move_tests.rs

@@ -214,3 +214,122 @@ fn text_style_parameters_are_checked() {
     let LinkSource::Text { style, .. } = &s.doc.sketch(1).unwrap().link(&link).unwrap().source else { panic!() };
     assert!(style.italic && style.letter_spacing == 1.5);
 }
+
+/// `base` with the keys of `extra` set (a null `extra` value is sent as null).
+fn with(mut base: Value, extra: &Value) -> Value {
+    if let (Some(base), Some(extra)) = (base.as_object_mut(), extra.as_object()) {
+        for (key, value) in extra {
+            base.insert(key.clone(), value.clone());
+        }
+    }
+    base
+}
+
+const AT: &str = "`at` must be the baseline start [x, y]";
+const HEIGHT: &str = "`height` must be a length in mm";
+const ANGLE: &str = "`angle` must be an angle in degrees";
+const TEXT: &str = "`text` must be a string";
+
+/// Inputs of `sketch.text` and `sketch.edit_text` that were once read as "not given": a height of
+/// "H" drew the text 5 mm high, `at: ["H", 25]` put it at the origin. Each with its message.
+fn not_understood() -> Vec<(Value, &'static str)> {
+    vec![
+        (json!({"height":"H"}), HEIGHT),
+        (json!({"height":true}), HEIGHT),
+        (json!({"height":[10]}), HEIGHT),
+        (json!({"height":1e10}), HEIGHT),
+        (json!({"height":0}), "`height` must be positive"),
+        (json!({"height":-3}), "`height` must be positive"),
+        (json!({"angle":"A"}), ANGLE),
+        (json!({"angle":[15]}), ANGLE),
+        (json!({"angle":1e10}), ANGLE),
+        (json!({"at":["H",25]}), AT),
+        (json!({"at":[1]}), AT),
+        (json!({"at":[1,2,3]}), AT),
+        (json!({"at":[1,null]}), AT),
+        (json!({"at":[1e10,0]}), AT),
+        (json!({"at":"origin"}), AT),
+        (json!({"at":7}), AT),
+        (json!({"text":5}), TEXT),
+        (json!({"text":true}), TEXT),
+        (json!({"text":["I"]}), TEXT),
+    ]
+}
+
+#[test]
+fn sketch_text_refuses_inputs_it_does_not_understand() {
+    let (mut s, _, _) = setup();
+    let before = s.doc.to_json();
+    let undo = s.undo.len();
+    let base = json!({"text":"I","at":[0,0],"height":10});
+    let mut cases = not_understood();
+    // `at` is required: left out or null is refused too (null no longer means the origin).
+    cases.push((json!({"at":null}), AT));
+    for (extra, error) in cases {
+        let params = with(base.clone(), &extra);
+        let e = s.execute("sketch.text", &params).unwrap_err().to_string();
+        assert!(e.contains(error), "{params}: {e}");
+        assert_eq!(s.doc.to_json(), before, "{params}");
+        assert_eq!(s.undo.len(), undo, "{params}");
+    }
+    let e = s.execute("sketch.text", &json!({"text":"I","height":10})).unwrap_err().to_string();
+    assert!(e.contains(AT), "{e}");
+    // No text at all is still the empty-text error.
+    let e = s.execute("sketch.text", &json!({"at":[0,0]})).unwrap_err().to_string();
+    assert!(e.contains("`text` must not be empty"), "{e}");
+    assert_eq!(s.doc.to_json(), before);
+}
+
+#[test]
+fn sketch_edit_text_refuses_inputs_it_does_not_understand() {
+    let (mut s, link, _) = setup();
+    let before = s.doc.to_json();
+    let undo = s.undo.len();
+    for (extra, error) in not_understood() {
+        let params = with(json!({"link":link}), &extra);
+        for live in [false, true] {
+            let params = with(params.clone(), &json!({"live":live}));
+            let e = s.execute("sketch.edit_text", &params).unwrap_err().to_string();
+            assert!(e.contains(error), "{params}: {e}");
+            assert_eq!(s.doc.to_json(), before, "{params}");
+            assert_eq!(s.undo.len(), undo, "{params}");
+        }
+    }
+}
+
+fn text_source_of(s: &Session, link: &str) -> (String, Vec2, f64, f64) {
+    let LinkSource::Text { text, at, height, angle, .. } = &s.doc.sketch(1).unwrap().link(link).unwrap().source else { panic!() };
+    (text.clone(), *at, *height, *angle)
+}
+
+#[test]
+fn sketch_text_inputs_left_out_or_null_keep_their_defaults() {
+    let mut s = Session::default();
+    run(&mut s, "sketch.create", json!({"plane":"XY"}));
+    // Optional inputs left out, or null: the defaults (5 mm, no angle). `at` as {x, y} is fine.
+    for (params, at) in [
+        (json!({"text":"I","at":[4,6]}), Vec2::new(4.0, 6.0)),
+        (json!({"text":"I","at":{"x":4,"y":6},"height":null,"angle":null,"font":null}), Vec2::new(4.0, 6.0)),
+    ] {
+        let t = run(&mut s, "sketch.text", params);
+        let (text, got, height, angle) = text_source_of(&s, t["link"].as_str().unwrap());
+        assert_eq!((text.as_str(), height, angle), ("I", 5.0, 0.0));
+        assert!(got.dist(at) < 1e-12, "{got:?}");
+    }
+    // Given values are used, an angle of 0 and a negative position among them.
+    let t = run(&mut s, "sketch.text", json!({"text":"I","at":[-4.5,6],"height":12.5,"angle":0}));
+    let (_, at, height, angle) = text_source_of(&s, t["link"].as_str().unwrap());
+    assert!(at.dist(Vec2::new(-4.5, 6.0)) < 1e-12 && height == 12.5 && angle == 0.0, "{at:?} {height} {angle}");
+    let t = run(&mut s, "sketch.text", json!({"text":"I","at":[0,0],"angle":90}));
+    assert!((text_source_of(&s, t["link"].as_str().unwrap()).3 - std::f64::consts::FRAC_PI_2).abs() < 1e-12);
+
+    // An edit keeps what it leaves out, or sends as null, and changes what it gives.
+    let (mut s, link, _) = setup();
+    let old = text_source_of(&s, &link);
+    run(&mut s, "sketch.edit_text", json!({"link":link,"text":null,"at":null,"height":null,"angle":null}));
+    assert_eq!(text_source_of(&s, &link), old);
+    run(&mut s, "sketch.edit_text", json!({"link":link,"height":20,"angle":0}));
+    let (text, at, height, angle) = text_source_of(&s, &link);
+    assert_eq!((text.as_str(), height, angle), ("II", 20.0, 0.0));
+    assert!(at.dist(Vec2::new(3.0, 4.0)) < 1e-12, "{at:?}");
+}

docs/book/src/guide/sketching.md

@@ -46,7 +46,9 @@ circle and update the outline when the inline **Sides** value changes.
 `sketch.edit_text` take a `font`, the family or full name of an installed font ("DejaVu Sans") or the path
 of a `.ttf` / `.otf` file, and the **Edit Text** dialog has a **Font** field for it. An unknown font is an
 error, and the font is kept with the text in the design, so the text stays editable in it. `SOLIDCRAFT_FONT_DIRS`
-adds folders to the ones searched for installed fonts.
+adds folders to the ones searched for installed fonts. The same two commands refuse a `text` that is not a
+string, an `at` that is not two numbers, and a `height` or `angle` that is not a number (or a `height` that is not
+positive) with a message, instead of drawing the text with a default; left out, they keep their defaults.
 
 Right-click a letter's filled interior or outline and choose **Edit Text**. You can also
 start **Edit Text** first and click the letter. All outlines belonging to that text are