Merge pull request #6467 from dotCooCoo/fix/pathops-param-types

Path commands: Offset Path, Simplify, Split Into Grid, Clean Up, Trap and Contour reject a param of the wrong type

crates/engine/src/cmd/addons/contour.rs

@@ -12,8 +12,8 @@ use vectorcraft_doc::NodeKind;
 use vectorcraft_render::effects;
 
 use super::super::edit::{copy_above, selected_roots};
-use super::super::pathops::{len_param, num_param};
-use super::super::{CommandSpec, EngineError, NodeId, Result, Session, bad, cmd, has_selection, str_param};
+use super::super::pathops::{length_given, number_given};
+use super::super::{CommandSpec, EngineError, NodeId, Result, Session, bad, cmd, has_selection, text_param};
 
 const C: &str = "addon.contour.set";
 /// The live effect that does the offsetting (the one Effect › Path › Offset Path applies).
@@ -44,12 +44,12 @@ fn joins_for(corners: Option<&str>) -> Result<&'static str> {
 }
 
 fn contour(s: &mut Session, p: &Value) -> Result<Value> {
-    let offset = len_param(p, "offset").ok_or_else(|| bad(C, "missing `offset` (a distance in points)"))?;
+    let offset = length_given(p, "offset", C)?.ok_or_else(|| bad(C, "missing `offset` (a distance in points)"))?;
     if offset.abs() > MAX_OFFSET {
         return Err(bad(C, "offset out of range"));
     }
-    let joins = joins_for(str_param(p, "corners"))?;
-    let miter = num_param(p, "miterLimit").unwrap_or(4.0).clamp(1.0, 500.0);
+    let joins = joins_for(text_param(p, "corners", C)?)?;
+    let miter = number_given(p, "miterLimit", C)?.unwrap_or(4.0).clamp(1.0, 500.0);
     let effect = effects::new_effect(EFFECT, &json!({"offset": offset, "joins": joins, "miterLimit": miter}))
         .ok_or_else(|| EngineError::Other("Contour: the Offset Path effect is not available".into()))?;
     let roots = selected_roots(s)?;

crates/engine/src/cmd/addons/contour/tests.rs

@@ -158,6 +158,39 @@ fn contour_refuses_what_it_cannot_offset() {
     assert!(err.contains("select paths or shapes"), "{err}");
 }
 
+/// A param of the wrong type is an error naming the param, and leaves the document, the selection
+/// and the undo history as they were. Null keeps a param's default, and `offset` still takes a
+/// string with a unit.
+#[test]
+fn contour_refuses_a_param_of_the_wrong_type() {
+    let mut s = session();
+    rect(&mut s, 100.0, 100.0, 50.0, 40.0);
+    let state = |s: &Session| {
+        let st = s.doc().unwrap();
+        (st.doc.clone(), st.selection.objects.clone(), st.history.undo.len())
+    };
+    let before = state(&s);
+    for (p, want) in [
+        (json!({"offset": true}), "`offset` must be a number, not true"),
+        (json!({"offset": "abc"}), "`offset` must be a number, not \"abc\""),
+        (json!({"offset": [5]}), "`offset` must be a number, not [5]"),
+        (json!({"offset": 5, "corners": 3}), "`corners` must be a string"),
+        (json!({"offset": 5, "miterLimit": "high"}), "`miterLimit` must be a number, not \"high\""),
+        (json!({"offset": 5, "miterLimit": false}), "`miterLimit` must be a number, not false"),
+    ] {
+        let r = s.execute("addon.contour.set", &p);
+        assert!(matches!(&r, Err(EngineError::BadParams { msg, .. }) if msg == want), "{p}: {r:?}");
+        assert!(state(&s) == before, "{p} changed the document, the selection or the undo history");
+    }
+    // A null `offset` is a missing one.
+    let err = s.execute("addon.contour.set", &json!({"offset": null})).unwrap_err().to_string();
+    assert!(err.contains("missing `offset`"), "{err}");
+    // Null corners and miter limit keep sharp corners at 4; the offset takes a unit.
+    let r = s.execute("addon.contour.set", &json!({"offset": "1 in", "corners": null, "miterLimit": null})).unwrap();
+    let fx = effects_of(&s, ids_of(&r, "created")[0]);
+    assert_eq!((fx[0].1["offset"].clone(), fx[0].1["joins"].clone(), fx[0].1["miterLimit"].clone()), (json!(72.0), json!("miter"), json!(4.0)));
+}
+
 /// The border-with-a-gap recipe: two Contours of one shape (the gap, then the border), baked, then
 /// the gap taken out of the border with Minus Front, leaves a ring between the two.
 #[test]

crates/engine/src/cmd/pathops.rs

@@ -16,8 +16,8 @@ use vectorcraft_pathops::{BoolOp, PathfinderOp};
 use super::edit::selected_roots;
 use super::expand::{fill_shape, reshaped, seeded};
 use super::{
-    CommandSpec, EngineError, Result, Session, always, bad, bool_or, checked_ids_or_selection, cmd, has_doc, has_selected_path, has_selection,
-    number_or, str_param, switch_or,
+    CommandSpec, EngineError, Result, Session, always, bad, check_switches, checked_ids_or_selection, cmd, has_doc, has_selected_path, has_selection,
+    number_or, switch_or, text_param,
 };
 
 pub fn specs() -> Vec<CommandSpec> {
@@ -221,6 +221,25 @@ 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}".
+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}"))),
+    }
+}
+
+/// 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.
+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}"))),
+    }
+}
+
 fn ids_json(ids: &[NodeId]) -> Value {
     json!({ "ids": ids.iter().map(|i| i.0).collect::<Vec<_>>() })
 }
@@ -687,18 +706,19 @@ fn trap(s: &mut Session, p: &Value) -> Result<Value> {
     use vectorcraft_render::effects::trap::{TRAP_LABEL, TrapOptions, TrapSource, trap_appearance, traps};
     const ID: &str = "object.pathfinder.trap";
     let mut q = p.clone();
-    if let Some(t) = len_param(p, "thickness") {
+    if let Some(t) = length_given(p, "thickness", ID)? {
         if !(po::TRAP_MIN_WIDTH..=po::TRAP_MAX_WIDTH).contains(&t) {
             return Err(bad(ID, "thickness must be from 0.01 to 5000 pt"));
         }
         q["thickness"] = json!(t);
     }
-    if num_param(p, "heightWidth").is_some_and(|v| !(1.0..=1000.0).contains(&v)) {
+    if number_given(p, "heightWidth", ID)?.is_some_and(|v| !(1.0..=1000.0).contains(&v)) {
         return Err(bad(ID, "heightWidth must be from 1 to 1000 %"));
     }
-    if num_param(p, "tintReduction").is_some_and(|v| !(0.0..=100.0).contains(&v)) {
+    if number_given(p, "tintReduction", ID)?.is_some_and(|v| !(0.0..=100.0).contains(&v)) {
         return Err(bad(ID, "tintReduction must be from 0 to 100 %"));
     }
+    check_switches(p, &["trapsWithProcessColor", "reverseTraps"], ID)?;
     // The effect-only settings stay at their defaults.
     if let Some(o) = q.as_object_mut() {
         o.remove("precision");
@@ -743,21 +763,23 @@ fn trap(s: &mut Session, p: &Value) -> Result<Value> {
 
 // ---------- Offset Path ----------
 
-fn join_param(p: &Value, key: &str) -> po::Join {
-    match str_param(p, key).map(str::to_ascii_lowercase).as_deref() {
-        Some("round") => po::Join::Round,
-        Some("bevel") => po::Join::Bevel,
-        _ => po::Join::Miter,
+fn join_param(p: &Value, key: &str, cmd: &str) -> Result<po::Join> {
+    match text_param(p, key, cmd)?.map(str::to_ascii_lowercase).as_deref() {
+        None | Some("miter") => Ok(po::Join::Miter),
+        Some("round") => Ok(po::Join::Round),
+        Some("bevel") => Ok(po::Join::Bevel),
+        Some(_) => Err(bad(cmd, format!("`{key}` must be one of miter, round, bevel"))),
     }
 }
 
 fn offset_path(s: &mut Session, p: &Value) -> Result<Value> {
-    let delta = len_param(p, "offset").unwrap_or(10.0);
+    const C: &str = "object.path.offsetPath";
+    let delta = length_given(p, "offset", C)?.unwrap_or(10.0);
     if delta.abs() > 1e5 {
-        return Err(bad("object.path.offsetPath", "offset out of range"));
+        return Err(bad(C, "offset out of range"));
     }
-    let join = join_param(p, "joins");
-    let miter = num_param(p, "miterLimit").unwrap_or(4.0).clamp(1.0, 500.0);
+    let join = join_param(p, "joins", C)?;
+    let miter = number_given(p, "miterLimit", C)?.unwrap_or(4.0).clamp(1.0, 500.0);
     let roots = selected_roots(s)?;
     let ids = s.edit("Offset Path", |d, sel| {
         let (mut out, mut above) = (vec![], vec![]);
@@ -1167,23 +1189,21 @@ pub fn auto_simplify_level(s: &Session) -> Result<f64> {
 }
 
 fn simplify(s: &mut Session, p: &Value) -> Result<Value> {
+    const C: &str = "object.path.simplify";
+    let (auto, given_level, given_tolerance) = (switch_or(p, "auto", false, C)?, number_given(p, "level", C)?, length_given(p, "tolerance", C)?);
+    let corner_angle = number_given(p, "cornerAngle", C)?.unwrap_or(30.0).clamp(0.0, 180.0);
+    let (straight_lines, whole) = (switch_or(p, "straightLines", false, C)?, switch_or(p, "wholePaths", false, C)?);
     // With auto, the slider's level is Auto-Simplify's for the selection's size; otherwise it is the level given.
-    let level = if bool_or(p, "auto", false) {
+    let level = if auto {
         Some(auto_simplify_level(s)?)
     } else {
-        num_param(p, "level").map(|l| if l.is_finite() { l.round().clamp(0.0, 100.0) } else { po::SIMPLIFY_LEVEL })
+        given_level.map(|l| if l.is_finite() { l.round().clamp(0.0, 100.0) } else { po::SIMPLIFY_LEVEL })
     };
     let tolerance = match level {
         Some(l) => po::simplify_level_tolerance(l),
-        None => len_param(p, "tolerance").unwrap_or(1.0),
+        None => given_tolerance.unwrap_or(1.0),
     };
-    let opts = po::SimplifyOptions {
-        tolerance: tolerance.clamp(1e-4, 1e4),
-        corner_angle_deg: num_param(p, "cornerAngle").unwrap_or(30.0).clamp(0.0, 180.0),
-        straight_lines: bool_or(p, "straightLines", false),
-        smooth: 0.0,
-    };
-    let whole = bool_or(p, "wholePaths", false);
+    let opts = po::SimplifyOptions { tolerance: tolerance.clamp(1e-4, 1e4), corner_angle_deg: corner_angle, straight_lines, smooth: 0.0 };
     // A path with segments direct-selected simplifies only their runs, and keeps those runs (and the
     // other anchors it had selected) direct-selected.
     let roots = selected_roots(s)?;
@@ -1257,10 +1277,17 @@ fn grid_axis(p: &Value, n: f64, [size, gutter, total]: [&str; 3], extent: f64) -
 }
 
 fn split_into_grid(s: &mut Session, p: &Value) -> Result<Value> {
-    let rows = num_param(p, "rows").unwrap_or(2.0).round();
-    let cols = num_param(p, "columns").unwrap_or(2.0).round();
+    const C: &str = "object.path.splitIntoGrid";
+    let rows = number_given(p, "rows", C)?.unwrap_or(2.0).round();
+    let cols = number_given(p, "columns", C)?.unwrap_or(2.0).round();
     if !(1.0..=500.0).contains(&rows) || !(1.0..=500.0).contains(&cols) {
-        return Err(bad("object.path.splitIntoGrid", "rows and columns must be 1..500"));
+        return Err(bad(C, "rows and columns must be 1..500"));
+    }
+    let add_guides = switch_or(p, "addGuides", false, C)?;
+    // Each distance [`grid_axis`] reads is a number, a string with a unit or null, checked before the
+    // selection, so a bad one is an error whatever is selected.
+    for key in ["height", "rowGutter", "totalHeight", "width", "columnGutter", "totalWidth", "gutter"] {
+        length_given(p, key, C)?;
     }
     let too_small = || EngineError::Other("Split Into Grid: select paths large enough for the grid".into());
     // The selected paths, back to front: one grid over them all replaces them, where the topmost
@@ -1281,7 +1308,7 @@ fn split_into_grid(s: &mut Session, p: &Value) -> Result<Value> {
         return Err(too_small());
     }
     let mut edges: Vec<(bool, f64)> = Vec::new();
-    if bool_or(p, "addGuides", false) {
+    if add_guides {
         // Row-major cells: the first row has the column edges, each row's first cell the row edges.
         let bounds = cells.iter().filter_map(PathData::bounds);
         edges.extend(bounds.clone().take(cols).flat_map(|c| [(true, c.x0), (true, c.x1)]));
@@ -1316,9 +1343,10 @@ fn is_painted(a: &Appearance) -> bool {
 }
 
 fn clean_up(s: &mut Session, p: &Value) -> Result<Value> {
-    let stray = bool_or(p, "strayPoints", true);
-    let unpainted = bool_or(p, "unpaintedObjects", true);
-    let empty_text = bool_or(p, "emptyTextPaths", true);
+    const C: &str = "object.path.cleanUp";
+    let stray = switch_or(p, "strayPoints", true, C)?;
+    let unpainted = switch_or(p, "unpaintedObjects", true, C)?;
+    let empty_text = switch_or(p, "emptyTextPaths", true, C)?;
     let found = {
         let d = &s.doc()?.doc;
         let mut v = vec![];

crates/engine/src/tests/pathops.rs

@@ -844,6 +844,69 @@ fn clean_up_removes_junk() {
     assert!(d.node(unpainted).is_none() && d.node(stray).is_none() && d.node(empty).is_none());
 }
 
+/// Offset Path, Simplify, Split Into Grid and Clean Up refuse a param of the wrong type with an
+/// error naming the param, and leave the document, the selection and the undo history as they were.
+/// Null keeps a param's default, and a distance still takes a string with a unit.
+#[test]
+fn path_commands_refuse_a_param_of_the_wrong_type() {
+    let mut s = session();
+    let a = rect(&mut s, 100.0, 100.0, 50.0, 40.0);
+    s.execute("path.create", &json!({"anchors": [{"x": 5, "y": 5}]})).unwrap();
+    s.execute("select.set", &json!({"ids": [a.0]})).unwrap();
+    let state = |s: &Session| {
+        let st = s.doc().unwrap();
+        (st.doc.clone(), st.selection.objects.clone(), st.history.undo.len())
+    };
+    let before = state(&s);
+    let number = |k: &str, v: &str| format!("`{k}` must be a number, not {v}");
+    let switch = |k: &str| format!("`{k}` must be true or false");
+    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": 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]")),
+        ("object.path.simplify", json!({"auto": "true"}), switch("auto")),
+        ("object.path.simplify", json!({"level": false}), number("level", "false")),
+        ("object.path.simplify", json!({"level": 50, "tolerance": {}}), number("tolerance", "{}")),
+        ("object.path.simplify", json!({"cornerAngle": "sharp"}), number("cornerAngle", "\"sharp\"")),
+        ("object.path.simplify", json!({"straightLines": 1}), switch("straightLines")),
+        ("object.path.simplify", json!({"wholePaths": "no"}), switch("wholePaths")),
+        ("object.path.splitIntoGrid", json!({"rows": "many"}), number("rows", "\"many\"")),
+        ("object.path.splitIntoGrid", json!({"columns": true}), number("columns", "true")),
+        ("object.path.splitIntoGrid", json!({"gutter": false}), number("gutter", "false")),
+        ("object.path.splitIntoGrid", json!({"rowGutter": 2, "gutter": "x"}), number("gutter", "\"x\"")),
+        ("object.path.splitIntoGrid", json!({"height": 5, "totalHeight": [1]}), number("totalHeight", "[1]")),
+        ("object.path.splitIntoGrid", json!({"width": "wide"}), number("width", "\"wide\"")),
+        ("object.path.splitIntoGrid", json!({"columnGutter": true}), number("columnGutter", "true")),
+        ("object.path.splitIntoGrid", json!({"totalWidth": {}}), number("totalWidth", "{}")),
+        ("object.path.splitIntoGrid", json!({"rowGutter": "a"}), number("rowGutter", "\"a\"")),
+        ("object.path.splitIntoGrid", json!({"addGuides": "yes"}), switch("addGuides")),
+        ("object.path.cleanUp", json!({"strayPoints": "false"}), switch("strayPoints")),
+        ("object.path.cleanUp", json!({"unpaintedObjects": 0}), switch("unpaintedObjects")),
+        ("object.path.cleanUp", json!({"emptyTextPaths": "no"}), switch("emptyTextPaths")),
+    ] {
+        let r = s.execute(cmd, &p);
+        assert!(matches!(&r, Err(EngineError::BadParams { msg, .. }) if *msg == want), "{cmd} {p}: {r:?}");
+        assert!(state(&s) == before, "{cmd} {p} changed the document, the selection or the undo history");
+    }
+    // Null takes the default: a 10 pt offset with mitered joins.
+    let r = ids(&s.execute("object.path.offsetPath", &json!({"offset": null, "joins": null, "miterLimit": null})).unwrap());
+    assert!(close(bounds(&s, r[0]), Rect::new(90.0, 90.0, 160.0, 150.0), 1e-2), "{:?}", bounds(&s, r[0]));
+    // A distance with a unit, and a join named in capitals.
+    s.execute("select.set", &json!({"ids": [a.0]})).unwrap();
+    let r = ids(&s.execute("object.path.offsetPath", &json!({"offset": "0.1 in", "joins": "Bevel"})).unwrap());
+    assert!(close(bounds(&s, r[0]), Rect::new(92.8, 92.8, 157.2, 147.2), 1e-2), "{:?}", bounds(&s, r[0]));
+    // Null switches keep Clean Up's defaults, which remove the stray point.
+    let r = s.execute("object.path.cleanUp", &json!({"strayPoints": null, "unpaintedObjects": null, "emptyTextPaths": null})).unwrap();
+    assert_eq!(r["removed"], 1);
+    // Split Into Grid with null counts and a gutter with a unit: two by two, 0.5 in apart.
+    s.execute("select.set", &json!({"ids": [a.0]})).unwrap();
+    let r = ids(&s.execute("object.path.splitIntoGrid", &json!({"rows": null, "columns": null, "gutter": "0.5 in", "addGuides": null})).unwrap());
+    assert_eq!(r.len(), 4);
+    assert!(close(bounds(&s, r[0]), Rect::new(100.0, 100.0, 107.0, 102.0), 1e-9), "{:?}", bounds(&s, r[0]));
+}
+
 #[test]
 fn divide_objects_below_cuts() {
     let mut s = session();

crates/engine/src/tests/trap.rs

@@ -138,15 +138,35 @@ fn no_adjoining_edges_is_a_clear_error_that_changes_nothing() {
 }
 
 #[test]
-fn out_of_range_and_unreadable_numbers_are_refused_or_defaulted() {
+fn out_of_range_and_unreadable_numbers_are_refused() {
     let (mut s, _, _) = abutting();
+    let before = s.doc().unwrap().doc.clone();
+    let undo_len = s.doc().unwrap().history.undo.len();
     for p in [json!({"thickness": 1e300}), json!({"thickness": 0}), json!({"heightWidth": 0}), json!({"tintReduction": 101})] {
         assert!(s.execute("object.pathfinder.trap", &p).is_err(), "{p}");
     }
-    // A thickness that isn't a number falls back to the default.
-    let r = s.execute("object.pathfinder.trap", &json!({"thickness": "NaN", "tintReduction": 100})).unwrap();
+    // A value of the wrong type is refused with an error naming the param, and changes nothing.
+    for (p, want) in [
+        (json!({"thickness": "NaN"}), "`thickness` must be a number, not \"NaN\""),
+        (json!({"thickness": true}), "`thickness` must be a number, not true"),
+        (json!({"heightWidth": [100]}), "`heightWidth` must be a number, not [100]"),
+        (json!({"tintReduction": false}), "`tintReduction` must be a number, not false"),
+        (json!({"trapsWithProcessColor": "yes"}), "`trapsWithProcessColor` must be true or false"),
+        (json!({"reverseTraps": 1}), "`reverseTraps` must be true or false"),
+    ] {
+        let r = s.execute("object.pathfinder.trap", &p);
+        assert!(matches!(&r, Err(EngineError::BadParams { msg, .. }) if msg == want), "{p}: {r:?}");
+    }
+    assert_eq!(s.doc().unwrap().doc, before);
+    assert_eq!(s.doc().unwrap().history.undo.len(), undo_len);
+    // Null keeps the default thickness, 0.25 pt; a thickness with a unit is read as a distance.
+    let r = s.execute("object.pathfinder.trap", &json!({"thickness": null, "reverseTraps": null, "tintReduction": 100})).unwrap();
     let bb = traps(&s, &r)[0].geometric_bounds().unwrap();
     assert!(near(bb.x1, 50.25), "{bb:?}");
+    s.execute("edit.undo", &json!({})).unwrap();
+    let r = s.execute("object.pathfinder.trap", &json!({"thickness": "0.1 in", "tintReduction": 100})).unwrap();
+    let bb = traps(&s, &r)[0].geometric_bounds().unwrap();
+    assert!(near(bb.x1, 57.2), "{bb:?}");
 }
 
 #[test]

docs/mcp.md

@@ -3675,6 +3675,11 @@ dialog `simplifySlider` with field `level`) at the Auto-Simplify level for the s
 opens the Simplify dialog at the slider's tolerance. When OK confirms the dialog with `retain` ticked, the menu item opens
 the dialog on those settings next time.
 
+`object.path.simplify`, `object.path.offsetPath`, `object.path.splitIntoGrid`, `object.path.cleanUp`,
+`object.pathfinder.trap` and `addon.contour.set` reject a param of the wrong type, such as `"offset": true`,
+`"joins": 3`, `"rows": "many"` or `"addGuides": "yes"`, and change nothing. A distance still takes a string with a unit
+(`"5 mm"`). `joins` must be `miter`, `round` or `bevel`. Omitting a param or giving null keeps its default.
+
 ## Smooth
 
 `object.path.smooth` smooths the selected paths (or the objects in `ids`), compound path members included, as one undo