From 37113c0c13feedfba28fb13eb020bd73682c3cb1 Mon Sep 17 00:00:00 2001 From: NOWN-DEV <257771410+NOWN-DEV@users.noreply.github.com> Date: Wed, 14 Oct 2026 15:24:30 +0200 Subject: [PATCH] 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 | 31 ++++- .../engine/src/cmd/sketch_text_move_tests.rs | 119 ++++++++++++++++++ docs/book/src/guide/sketching.md | 4 +- 3 files changed, 147 insertions(+), 7 deletions(-) diff --git a/crates/engine/src/cmd/sketch_create.rs b/crates/engine/src/cmd/sketch_create.rs index 47eb8fd1..3aac1027 100644 --- a/crates/engine/src/cmd/sketch_create.rs +++ b/crates/engine/src/cmd/sketch_create.rs @@ -145,21 +145,40 @@ fn text_style(p: &Value, cmd: &str, old: TextStyle) -> Result { 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> { + 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, TextStyle)>) -> Result { 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 { 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"))?; diff --git a/crates/engine/src/cmd/sketch_text_move_tests.rs b/crates/engine/src/cmd/sketch_text_move_tests.rs index 0d715c47..b9bb2e24 100644 --- a/crates/engine/src/cmd/sketch_text_move_tests.rs +++ b/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", ¶ms).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", ¶ms).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:?}"); +} diff --git a/docs/book/src/guide/sketching.md b/docs/book/src/guide/sketching.md index 8e19ff14..d51ae61a 100644 --- a/docs/book/src/guide/sketching.md +++ b/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