From bf3e6b12edf631d95ae7c53a5934de7a21ffd80c Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Tue, 10 Nov 2026 13:31:54 -0800 Subject: [PATCH] Path commands: Offset Path, Simplify, Split Into Grid, Clean Up, Trap and Contour reject a param of the wrong type object.path.offsetPath, object.path.simplify, object.path.splitIntoGrid, object.path.cleanUp, object.pathfinder.trap and addon.contour.set read their params with helpers that fell back to the default for a value of the wrong type: "offset": true offset by 10 pt, "joins": 3 gave mitered joins, "addGuides": "yes" added no guides. Each such value is now an error naming the param, and the document and the undo history stay as they were. An omitted or null param keeps its default, and a distance still takes a string with a unit ("5 mm"). An unknown `joins` name is refused as well. --- crates/engine/src/cmd/addons/contour.rs | 10 +-- crates/engine/src/cmd/addons/contour/tests.rs | 33 +++++++ crates/engine/src/cmd/pathops.rs | 90 ++++++++++++------- crates/engine/src/tests/pathops.rs | 63 +++++++++++++ crates/engine/src/tests/trap.rs | 26 +++++- docs/mcp.md | 5 ++ 6 files changed, 188 insertions(+), 39 deletions(-) diff --git a/crates/engine/src/cmd/addons/contour.rs b/crates/engine/src/cmd/addons/contour.rs index 46df381ed..34b944342 100644 --- a/crates/engine/src/cmd/addons/contour.rs +++ b/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 { - 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)?; diff --git a/crates/engine/src/cmd/addons/contour/tests.rs b/crates/engine/src/cmd/addons/contour/tests.rs index 900d110d5..3983ed942 100644 --- a/crates/engine/src/cmd/addons/contour/tests.rs +++ b/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] diff --git a/crates/engine/src/cmd/pathops.rs b/crates/engine/src/cmd/pathops.rs index c18412c1d..a8306d25c 100644 --- a/crates/engine/src/cmd/pathops.rs +++ b/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 { @@ -221,6 +221,25 @@ pub(crate) fn len_param(p: &Value, key: &str) -> Option { } } +/// 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> { + 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> { + 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::>() }) } @@ -687,18 +706,19 @@ fn trap(s: &mut Session, p: &Value) -> Result { 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 { // ---------- 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 { + 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 { - 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 { } fn simplify(s: &mut Session, p: &Value) -> Result { + 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 { - 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 { 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 { - 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![]; diff --git a/crates/engine/src/tests/pathops.rs b/crates/engine/src/tests/pathops.rs index a5bd60490..57094355c 100644 --- a/crates/engine/src/tests/pathops.rs +++ b/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(); diff --git a/crates/engine/src/tests/trap.rs b/crates/engine/src/tests/trap.rs index b2ae09908..aa0a1a75d 100644 --- a/crates/engine/src/tests/trap.rs +++ b/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] diff --git a/docs/mcp.md b/docs/mcp.md index 566f2e368..b8e37de33 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -3648,6 +3648,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