Merge pull request #6477 from dotCooCoo/fix/text-style-param-types

Type: character formats, Ruby and Move Type Path Handle reject a param of the wrong type

crates/engine/src/cmd/pathops.rs

@@ -221,25 +221,41 @@ pub(crate) fn len_param(p: &Value, key: &str) -> Option<f64> {
     }
 }
 
-/// The number `p` gives `cmd` for `key`, as [`num_param`] reads it, or `None` when `p` leaves it out
-/// or gives null. Any other value, such as `true` or `"wide"`, is an error: "`{key}` must be a
-/// number, not {v}".
+/// The number `p` gives `cmd` for `key`, or `None` when `p` leaves it out or gives null. A string
+/// counts only when all of it is a number (`"20"`). Any other value, such as `true`, `"wide"` or
+/// `"20abc"`, is an error: "`{key}` must be a number, not {v}".
 pub(crate) fn number_given(p: &Value, key: &str, cmd: &str) -> Result<Option<f64>> {
     match p.get(key) {
         None | Some(Value::Null) => Ok(None),
-        Some(v) => num_param(p, key).map(Some).ok_or_else(|| bad(cmd, format!("`{key}` must be a number, not {v}"))),
+        Some(v) => match v {
+            Value::String(s) => whole_number(s),
+            _ => num_param(p, key),
+        }
+        .map(Some)
+        .ok_or_else(|| bad(cmd, format!("`{key}` must be a number, not {v}"))),
     }
 }
 
-/// The distance `p` gives `cmd` for `key`, as [`len_param`] reads it (`"5 mm"` included), or `None`
-/// when `p` leaves it out or gives null. Any other value is an error, as [`number_given`] gives it.
+/// The distance `p` gives `cmd` for `key`, or `None` when `p` leaves it out or gives null. A string
+/// counts only when all of it is a length that [`vectorcraft_doc::Unit::parse`] reads (`"5 mm"`) or a
+/// number. Any other value, such as `"20abc"`, is an error, as [`number_given`] gives it.
 pub(crate) fn length_given(p: &Value, key: &str, cmd: &str) -> Result<Option<f64>> {
     match p.get(key) {
         None | Some(Value::Null) => Ok(None),
-        Some(v) => len_param(p, key).map(Some).ok_or_else(|| bad(cmd, format!("`{key}` must be a number, not {v}"))),
+        Some(v) => match v {
+            Value::String(s) => vectorcraft_doc::Unit::Points.parse(s).filter(|v| v.is_finite()).or_else(|| whole_number(s)),
+            _ => num_param(p, key),
+        }
+        .map(Some)
+        .ok_or_else(|| bad(cmd, format!("`{key}` must be a number, not {v}"))),
     }
 }
 
+/// `s` as a finite number when all of it, spaces around it aside, is one.
+fn whole_number(s: &str) -> Option<f64> {
+    s.trim().parse::<f64>().ok().filter(|v| v.is_finite())
+}
+
 fn ids_json(ids: &[NodeId]) -> Value {
     json!({ "ids": ids.iter().map(|i| i.0).collect::<Vec<_>>() })
 }

crates/engine/src/cmd/textedit.rs

@@ -12,7 +12,7 @@ use super::stroke::StrokeChange;
 use super::typecmd::refresh_bounds;
 use super::{
     CommandSpec, EngineError, Result, Session, bad, bool_or, check_numbers, check_switches, cmd, color_value, has_doc, has_selection, id_param,
-    index, number_or, point_param, str_param, switch_or, targets,
+    index, number_or, point_param, str_param, switch_or, targets, text_param,
 };
 
 pub fn specs() -> Vec<CommandSpec> {
@@ -568,7 +568,8 @@ pub(crate) fn change_font_axes(st: &mut CharStyle, change: &AxisChange) {
 /// `features: ["dlig", "-liga", …]` → the canonical tag list (differences from the defaults).
 pub(crate) fn features_param(p: &Value, cmd: &str) -> Result<Option<Vec<String>>> {
     let Some(v) = p.get("features").filter(|v| !v.is_null()) else { return Ok(None) };
-    let tags: Vec<&str> = v.as_array().ok_or_else(|| bad(cmd, "features must be a list of tags"))?.iter().filter_map(Value::as_str).collect();
+    let not_tags = || bad(cmd, "features must be a list of tags");
+    let tags: Vec<&str> = v.as_array().ok_or_else(not_tags)?.iter().map(|t| t.as_str().ok_or_else(not_tags)).collect::<Result<_>>()?;
     if let Some(t) = tags.iter().find(|t| !vectorcraft_text::OtFeatures::known_tag(t)) {
         return Err(bad(
             cmd,
@@ -708,8 +709,8 @@ impl CharChange {
             stroke_opts.weight = Some(w.clamp(0.0, 1000.0));
         }
         let c = Self {
-            font: str_param(p, "font").map(str::to_string),
-            style: str_param(p, "style").map(str::to_string),
+            font: text_param(p, "font", cmd)?.map(str::to_string),
+            style: text_param(p, "style", cmd)?.map(str::to_string),
             size: num("size"),
             leading: auto_or_num(p, "leading", cmd)?,
             tracking: num("tracking"),

crates/engine/src/cmd/typecmd.rs

@@ -8,10 +8,10 @@ use vectorcraft_doc::{AreaFit, Document, Justify, Node, NodeId, NodeKind, RubyAl
 use vectorcraft_geom::{Affine, PathData, Vec2};
 
 use super::edit::selected_roots;
-use super::pathops::{len_param, num_param, shape_node};
+use super::pathops::{len_param, length_given, number_given, shape_node};
 use super::{
     CommandSpec, EngineError, Result, Session, bad, check_numbers, cmd, color_value, f64_req, has_doc, has_selection, id_param, index, number_or, ok,
-    str_param, switch_or, targets,
+    str_param, switch_or, targets, text_param,
 };
 
 pub fn specs() -> Vec<CommandSpec> {
@@ -249,16 +249,16 @@ fn ruby(s: &mut Session, p: &Value) -> Result<Value> {
     let ids = text_targets(s, p, C)?;
     let range = TextRange::parse(p, C)?;
     let remove = switch_or(p, "remove", false, C)?;
-    let reading = str_param(p, "text").map(str::trim).filter(|t| !t.is_empty()).map(str::to_string);
+    let reading = text_param(p, "text", C)?.map(str::trim).filter(|t| !t.is_empty()).map(str::to_string);
     if !remove && reading.is_none() {
         return Err(bad(C, "give `text` (the reading) or `remove`"));
     }
-    let position = match str_param(p, "position").map(str::to_ascii_lowercase).as_deref() {
+    let position = match text_param(p, "position", C)?.map(str::to_ascii_lowercase).as_deref() {
         None | Some("above") | Some("right") => RubyPosition::Above,
         Some("below") | Some("left") => RubyPosition::Below,
         Some(_) => return Err(bad(C, "`position` is above or below")),
     };
-    let align = match str_param(p, "align").map(str::to_ascii_lowercase).as_deref() {
+    let align = match text_param(p, "align", C)?.map(str::to_ascii_lowercase).as_deref() {
         None | Some("center") | Some("centre") => RubyAlign::Center,
         Some("start") | Some("left") | Some("top") => RubyAlign::Start,
         Some("end") | Some("right") | Some("bottom") => RubyAlign::End,
@@ -812,9 +812,9 @@ pub(crate) fn justify_param(v: &str) -> Option<Justify> {
 
 fn set_style(s: &mut Session, p: &Value) -> Result<Value> {
     const C: &str = "text.setStyle";
-    let font = str_param(p, "font").map(str::to_string);
-    let style = str_param(p, "style").map(str::to_string);
-    let size = len_param(p, "size");
+    let font = text_param(p, "font", C)?.map(str::to_string);
+    let style = text_param(p, "style", C)?.map(str::to_string);
+    let size = length_given(p, "size", C)?;
     if size.is_some_and(|v| v <= 0.0) {
         return Err(bad(C, "size must be positive"));
     }
@@ -823,8 +823,8 @@ fn set_style(s: &mut Session, p: &Value) -> Result<Value> {
         Some(Value::String(a)) if a.eq_ignore_ascii_case("auto") => Some(None),
         Some(_) => Some(Some(len_param(p, "leading").ok_or_else(|| bad(C, "leading must be a number or \"auto\""))?.clamp(0.1, 5000.0))),
     };
-    let tracking = num_param(p, "tracking");
-    let justify = match str_param(p, "justify") {
+    let tracking = number_given(p, "tracking", C)?;
+    let justify = match text_param(p, "justify", C)? {
         Some(j) => Some(justify_param(j).ok_or_else(|| bad(C, "justify must be auto|left|center|right|justifyAll"))?),
         None => None,
     };
@@ -1282,7 +1282,7 @@ fn set_path_handle(s: &mut Session, p: &Value) -> Result<Value> {
     let id = id_param(p, "id").ok_or_else(|| bad(C, "missing id"))?;
     let si = super::draw2::usize_opt(p, "subpath", C)?.unwrap_or(0);
     let ai = p.get("anchor").and_then(Value::as_u64).map(index).ok_or_else(|| bad(C, "missing anchor"))?;
-    let out = str_param(p, "which") != Some("in");
+    let out = super::object::choice(p, C, "which", &["in", "out"])? != Some("in");
     let pos = vectorcraft_geom::Point::new(f64_req(p, "x", C)?, f64_req(p, "y", C)?);
     let independent = switch_or(p, "independent", false, C)?;
     let node = s.doc()?.doc.node(id);

crates/engine/src/tests/pathops.rs

@@ -863,6 +863,8 @@ fn path_commands_refuse_a_param_of_the_wrong_type() {
     for (cmd, p, want) in [
         ("object.path.offsetPath", json!({"offset": true}), number("offset", "true")),
         ("object.path.offsetPath", json!({"offset": "wide"}), number("offset", "\"wide\"")),
+        ("object.path.offsetPath", json!({"offset": "10abc"}), number("offset", "\"10abc\"")),
+        ("object.path.offsetPath", json!({"offset": 5, "miterLimit": "4x"}), number("miterLimit", "\"4x\"")),
         ("object.path.offsetPath", json!({"offset": 5, "joins": 3}), "`joins` must be a string".to_string()),
         ("object.path.offsetPath", json!({"offset": 5, "joins": "pointy"}), "`joins` must be one of miter, round, bevel".to_string()),
         ("object.path.offsetPath", json!({"offset": 5, "miterLimit": [4]}), number("miterLimit", "[4]")),

crates/engine/src/tests/textedit.rs

@@ -467,6 +467,84 @@ fn type_commands_refuse_a_number_or_boolean_given_as_text() {
     assert!(!t.ruby.is_empty() && t.ruby.iter().all(|r| !r.overhang && r.size_percent == 50.0), "{:?}", t.ruby);
 }
 
+/// The Character commands, Ruby and Move Type Path Handle refuse a param of the wrong type with an
+/// error naming the param, also when another param would change something, and leave the
+/// document, its revision, the selection and the undo history as they were. Null counts as a param
+/// left out, which keeps its default, and `size` and `leading` take a length with a unit.
+#[test]
+fn character_ruby_and_path_handle_refuse_a_param_of_the_wrong_type() {
+    use crate::tests::{history, not_a_number, select_with_a_step_to_redo};
+    let mut s = session();
+    let id = text(&mut s, "abc");
+    let path = s.execute("path.create", &json!({"anchors": [{"x": 100, "y": 300, "in": [50, 300], "out": [150, 300]}, {"x": 400, "y": 300}]}));
+    let on_path = s.execute("text.createInPath", &json!({"path": path.unwrap()["id"], "mode": "onPath", "text": "Along"})).unwrap()["id"].clone();
+    select_with_a_step_to_redo(&mut s, &[id]);
+    let string = |k: &str| format!("`{k}` must be a string");
+    let tags = "features must be a list of tags".to_string();
+    let which = "`which` must be one of in, out".to_string();
+    let handle = |which: Value| json!({"id": on_path, "anchor": 0, "which": which, "x": 100, "y": 250});
+    let mut calls = vec![
+        ("text.setStyle", json!({"font": 5}), string("font")),
+        ("text.setStyle", json!({"style": true}), string("style")),
+        ("text.setStyle", json!({"size": 30, "style": ["Bold"]}), string("style")),
+        ("text.setStyle", json!({"justify": 1}), string("justify")),
+        ("text.setStyle", json!({"size": 30, "justify": {"value": "center"}}), string("justify")),
+        ("text.setStyle", json!({"features": ["dlig", 5]}), tags.clone()),
+        ("text.setStyle", json!({"leading": true}), "leading must be a number or \"auto\"".to_string()),
+        ("text.setRangeStyle", json!({"id": id.0, "font": 5}), string("font")),
+        ("text.setRangeStyle", json!({"id": id.0, "size": 30, "style": false}), string("style")),
+        ("text.setRangeStyle", json!({"id": id.0, "features": [null]}), tags),
+        ("type.ruby", json!({"text": 5}), string("text")),
+        ("type.ruby", json!({"remove": true, "text": ["a"]}), string("text")),
+        ("type.ruby", json!({"text": "a", "position": 1}), string("position")),
+        ("type.ruby", json!({"text": "a", "align": true}), string("align")),
+        ("text.setPathHandle", handle(json!(1)), which.clone()),
+        ("text.setPathHandle", handle(json!("up")), which),
+    ];
+    calls.extend(
+        [
+            ("text.setStyle", json!({"size": true}), "size"),
+            ("text.setStyle", json!({"size": "big"}), "size"),
+            ("text.setStyle", json!({"size": "20abc"}), "size"),
+            ("text.setStyle", json!({"tracking": "20abc"}), "tracking"),
+            ("text.setStyle", json!({"font": "Inter", "size": [12]}), "size"),
+            ("text.setStyle", json!({"tracking": "wide"}), "tracking"),
+            ("text.setStyle", json!({"size": 30, "tracking": false}), "tracking"),
+        ]
+        .map(|(cmd, p, key)| {
+            let (_, p, msg) = not_a_number(cmd, p, key);
+            (cmd, p, msg)
+        }),
+    );
+    let state = |s: &Session| {
+        let st = s.doc().unwrap();
+        (st.doc.clone(), st.revision, st.selection.objects.clone(), history(s))
+    };
+    let before = state(&s);
+    let wrong: Vec<String> = calls
+        .iter()
+        .filter_map(|(cmd, p, want)| {
+            let r = s.execute(cmd, p);
+            let refused = matches!(&r, Err(EngineError::BadParams { msg, .. }) if msg == want);
+            let unchanged = state(&s) == before;
+            (!refused || !unchanged).then(|| format!("{cmd} {p}: {r:?}, unchanged: {unchanged}"))
+        })
+        .collect();
+    assert!(wrong.is_empty(), "not refused: {wrong:#?}");
+    // Null keeps each default; a size and a leading with a unit are lengths.
+    let r = json!({"font": null, "style": null, "justify": null, "tracking": null, "features": null, "leading": null, "size": "5 mm"});
+    s.execute("text.setStyle", &r).unwrap();
+    let st = obj(&s, id).runs[0].style.clone();
+    assert!((st.size - 5.0 * 72.0 / 25.4).abs() < 1e-6 && st.tracking == 0.0 && st.leading.is_none(), "{st:?}");
+    s.execute("text.setStyle", &json!({"size": "12 pt", "leading": "18 pt", "tracking": 50})).unwrap();
+    let st = obj(&s, id).runs[0].style.clone();
+    assert_eq!((st.size, st.leading, st.tracking), (12.0, Some(18.0), 50.0));
+    s.execute("type.ruby", &json!({"text": "a", "position": null, "align": null})).unwrap();
+    let ruby = obj(&s, id).ruby;
+    assert!(ruby.iter().all(|r| r.position == vectorcraft_doc::RubyPosition::Above && r.align == vectorcraft_doc::RubyAlign::Center), "{ruby:?}");
+    s.execute("text.setPathHandle", &handle(Value::Null)).unwrap();
+}
+
 /// Where each glyph of type object `id` is drawn from (its origin in font units), document space.
 fn glyph_pens(s: &Session, id: NodeId) -> Vec<(usize, Point)> {
     let t = obj(s, id);

docs/mcp.md

@@ -1371,7 +1371,7 @@ naming the key, and change nothing: `radius` in `shape.rectangle`; `sides` and `
 in `shape.flare`; `decay` and `segments` in `shape.spiral`; `rows` and `columns` in `shape.rectangularGrid`; `concentric`
 and `radial` in `shape.polarGrid`; `size` and the `width`, `height` and `fitMinPercent` of `area` in `text.create`;
 `size` and `fitMinPercent` in `text.createInPath`; `start`, `end`, `size`, `tracking`,
-`baselineShift`, `hScale`, `vScale`, `rotation` and `strokeWidth` in `text.setRangeStyle`; `start` and `end` in
+`baselineShift`, `hScale`, `vScale`, `rotation` and `strokeWidth` in `text.setRangeStyle`; `strokeWidth` in `text.setStyle`; `start` and `end` in
 `text.editRange` and `text.getRange`; `at` in `text.startList`; `start`, `hScale`, `vScale` and `rotation` in `text.touchType`; `start` in
 `text.glyphAlternates` and `text.setGlyphAlternate`; `by`, `start` and
 `end` in `type.step`; `by` in `type.size.increase` and `type.size.decrease`; `size` in `type.ruby`; `width`, `height`
@@ -1383,6 +1383,15 @@ and `amount` in `color.harmony`; `angle` in `paint.editGradient` and in the `gra
 `width` and `tolerance` in `livePaint.strokeEdge`. A null is read as a key left out. A gradient stop that names a
 swatch refuses a `tint` that is not a number with `` stop 0: `tint` must be a number ``.
 
+`text.setStyle` refuses a `size` that is neither a number nor a length with a unit (`"12 pt"`, `"5 mm"`) with
+`` `size` must be a number, not true ``, and a `tracking` that is neither a number nor a number written as a string
+(`"20"`) with `` `tracking` must be a number, not false ``. It and `text.setRangeStyle` refuse a `font` or `style` that is not a string
+with `` `font` must be a string ``, and a `features` list with an entry that is not a string with
+`features must be a list of tags`; `text.setStyle` refuses such a `justify` too. `type.ruby` refuses a `text`,
+`position` or `align` that is not a string, and `text.setPathHandle` refuses a `which` other than `in` or `out` with
+`` `which` must be one of in, out ``. Each leaves the document, the selection and the undo history as they were, also
+when the call gives another param it could apply. A null is read as a key left out.
+
 A byte offset must be a whole number of at least 0. `at` in `text.startList`, and `start` and `end` in
 `text.editRange`, `text.getRange` and `text.setRangeStyle`, and in `type.step` and `addon.type.textTweaker` given an `id`,
 refuse a negative or fractional number with