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 58d235b4c..ac1afed6b 100644 --- a/docs/mcp.md +++ b/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