diff --git a/crates/engine/src/cmd/docmenu.rs b/crates/engine/src/cmd/docmenu.rs index db792ab1c6..ae3fc521f0 100644 --- a/crates/engine/src/cmd/docmenu.rs +++ b/crates/engine/src/cmd/docmenu.rs @@ -13,8 +13,8 @@ use super::edit::selected_roots; use super::pathops::Beside; use super::select::{first_selectable, is_selectable, selectable_ids, walk_selectable}; use super::{ - CommandSpec, EngineError, Point, Result, Session, bad, bool_or, checked_ids_param, cmd, f64_or, f64_req, has_doc, has_selection, id_param, ok, - str_param, switch_or, + CommandSpec, EngineError, Point, Result, Session, bad, checked_ids_param, cmd, f64_req, has_doc, has_selection, id_param, number_or, ok, + str_param, switch_or, text_param, }; pub fn specs() -> Vec { @@ -482,7 +482,7 @@ fn save_selection(s: &mut Session, p: &Value) -> Result { let st = s.doc()?; let ids = st.selection.objects.clone(); let saved = &st.doc.saved_selections; - let name = str_param(p, "name").and_then(SavedSelection::clean_name).unwrap_or_else(|| SavedSelection::default_name(saved)); + let name = text_param(p, "name", C)?.and_then(SavedSelection::clean_name).unwrap_or_else(|| SavedSelection::default_name(saved)); let existing = saved.iter().position(|x| x.name == name); if existing.is_none() && saved.len() >= SavedSelection::MAX { return Err(bad(C, format!("a document keeps at most {} saved selections", SavedSelection::MAX))); @@ -577,10 +577,12 @@ fn edit_saved(s: &mut Session, p: &Value) -> Result { for e in edits { let name = str_param(e, "name").ok_or_else(|| bad(C, "missing name"))?; let i = *index.get(name).ok_or_else(|| bad(C, format!("no saved selection `{name}`")))?; + let delete = switch_or(e, "delete", false, C)?; + let new_name = text_param(e, "newName", C)?; let Some(slot) = after.get_mut(i) else { continue }; - if bool_or(e, "delete", false) { + if delete { *slot = None; - } else if let Some(n) = str_param(e, "newName").and_then(SavedSelection::clean_name) + } else if let Some(n) = new_name.and_then(SavedSelection::clean_name) && let Some(x) = slot { x.name = n; @@ -764,7 +766,7 @@ fn clear_guides(s: &mut Session, _: &Value) -> Result { fn guide_add(s: &mut Session, p: &Value) -> Result { let pos = f64_req(p, "pos", "guide.add")?; - let vertical = bool_or(p, "vertical", false); + let vertical = switch_or(p, "vertical", false, "guide.add")?; let artboard = match p.get("artboard").filter(|v| !v.is_null()) { Some(v) => { let boards = &s.doc()?.doc.artboards; @@ -845,7 +847,7 @@ fn guide_select(s: &mut Session, p: &Value) -> Result { let list = p.get("indexes").and_then(Value::as_array).ok_or_else(|| bad("guide.select", "missing indexes"))?; let ids = guide_ids(&s.doc()?.doc); let picked = list.iter().map(|v| guide_index_in(&ids, Some(v), "guide.select")).collect::>>()?; - if bool_or(p, "toggle", false) { + if switch_or(p, "toggle", false, "guide.select")? { // Added to (or taken from) what is selected, the art included. s.select(|_, sel| { for i in picked { @@ -924,7 +926,9 @@ pub(crate) fn guide_move(s: &mut Session, p: &Value) -> Result { if moving.is_empty() { return Err(bad("guide.move", "no guide selected (or give an index)")); } - let (dx, dy, copy) = (f64_or(p, "dx", 0.0), f64_or(p, "dy", 0.0), bool_or(p, "copy", false)); + let dx = number_or(p, "dx", 0.0, "guide.move")?; + let dy = number_or(p, "dy", 0.0, "guide.move")?; + let copy = switch_or(p, "copy", false, "guide.move")?; if !s.doc()?.selection.is_empty() { let ids = super::edit::selected_roots(s)?; return super::object::apply_transform( diff --git a/crates/engine/src/cmd/edit.rs b/crates/engine/src/cmd/edit.rs index 26d0f4d247..67ea3d7bcb 100644 --- a/crates/engine/src/cmd/edit.rs +++ b/crates/engine/src/cmd/edit.rs @@ -9,9 +9,9 @@ use vectorcraft_geom::{Affine, Point, Vec2}; use super::clipboard::SwatchChoices; use super::{ - CommandSpec, Result, Session, always, bad, bool_or, can_clear_history, can_redo, can_undo, checked_ids_param, clock_date, cmd, date_param, - f64_or, f64_req, finite_numbers, has_clipboard, has_doc, has_selection, has_selection_guides_or_artboard_tool, has_selection_or_guides, id_param, - index, ok, point_param, str_param, switch_or, + CommandSpec, Result, Session, always, bad, can_clear_history, can_redo, can_undo, check_numbers, checked_ids_param, clock_date, cmd, date_param, + f64_req, finite_numbers, has_clipboard, has_doc, has_selection, has_selection_guides_or_artboard_tool, has_selection_or_guides, id_param, index, + number_or, ok, point_param, str_param, switch_or, }; use crate::{Clipboard, EngineError, inspect}; @@ -237,7 +237,7 @@ pub fn specs() -> Vec { } fn file_close(s: &mut Session, p: &Value) -> Result { - let i = p.get("index").and_then(Value::as_u64).map(index).or(s.active_index()).ok_or(EngineError::NoDocument)?; + let i = opt_u64(p, "file.close", "index")?.map(index).or(s.active_index()).ok_or(EngineError::NoDocument)?; if !s.close_document(i) { return Err(bad("file.close", "no such document")); } @@ -269,7 +269,7 @@ fn doc_move(s: &mut Session, p: &Value) -> Result { fn doc_node(s: &mut Session, p: &Value) -> Result { let id = id_param(p, "id").ok_or_else(|| bad("document.node", "missing id"))?; let mut opts = slice_opts(p, "document.node")?; - let summary = bool_or(p, "summary", false); + let summary = switch_or(p, "summary", false, "document.node")?; if !summary && (opts.depth.is_some() || opts.child_limit.is_some()) { // The full object JSON is never truncated: a slice without `summary` would // otherwise silently return the whole subtree. @@ -663,7 +663,8 @@ fn undo_group_begin(s: &mut Session, p: &Value) -> Result { } fn undo_group_end(s: &mut Session, p: &Value) -> Result { - s.end_undo_group(bool_or(p, "cancel", false)); + let cancel = switch_or(p, "cancel", false, "edit.undoGroup.end")?; + s.end_undo_group(cancel); ok() } @@ -1231,6 +1232,9 @@ impl PasteMode { fn paste(s: &mut Session, p: &Value, mode: PasteMode) -> Result { let choices = SwatchChoices::parse(mode.label(), p)?; + if mode == PasteMode::Offset { + check_numbers(p, &["dx", "dy"], "edit.paste")?; + } // Taken out of the session for the edit (and put back whatever happens). let clip = std::mem::take(&mut s.clipboard); let r = paste_clip(s, p, mode, &clip, &choices); @@ -1385,7 +1389,7 @@ fn paste_clip(s: &mut Session, p: &Value, mode: PasteMode, clip: &Clipboard, cho (_, Some((_, dv))) => vec![Affine::translate(dv)], (PasteMode::Offset | PasteMode::Library | PasteMode::Drag, None) => vec![match point_param(p, "center") { Some(c) => clip.bounds().map_or(Affine::IDENTITY, |b| Affine::translate(c - b.center())), - None => Affine::translate((f64_or(p, "dx", off), f64_or(p, "dy", off))), + None => Affine::translate((number_or(p, "dx", off, mode.label())?, number_or(p, "dy", off, mode.label())?)), }], (PasteMode::AllArtboards, None) => { let src = clip.source_artboard.or_else(|| st.doc.artboards.first().map(|a| a.rect)).map(|r| r.origin()).unwrap_or_default(); @@ -1560,8 +1564,8 @@ fn layer_named(d: &mut Document, name: &str) -> NodeId { fn duplicate(s: &mut Session, p: &Value) -> Result { let roots = selected_roots(s)?; - let dx = f64_or(p, "dx", 0.0); - let dy = f64_or(p, "dy", 0.0); + let dx = number_or(p, "dx", 0.0, "edit.duplicate")?; + let dy = number_or(p, "dy", 0.0, "edit.duplicate")?; let ids = s.edit("Duplicate", |d, sel| duplicate_in(d, sel, &roots, Affine::translate((dx, dy))))?; Ok(json!({ "ids": ids.iter().map(|i| i.0).collect::>() })) } diff --git a/crates/engine/src/cmd/slices.rs b/crates/engine/src/cmd/slices.rs index 06ec067934..29a357f730 100644 --- a/crates/engine/src/cmd/slices.rs +++ b/crates/engine/src/cmd/slices.rs @@ -13,8 +13,8 @@ use vectorcraft_doc::{Appearance, CellAlign, CellVAlign, Document, Node, NodeKin use vectorcraft_geom::{Rect, shapes}; use super::{ - CommandSpec, EngineError, NodeId, Result, Session, bad, bool_or, cmd, f64_or, f64_req, has_doc, has_selection, id_param, ids_param, index, - switch_or, targets, + CommandSpec, EngineError, NodeId, Result, Session, bad, cmd, f64_req, has_doc, has_selection, id_param, ids_param, index, number_or, switch_or, + targets, }; /// The most slices one Divide Slices makes. @@ -380,7 +380,8 @@ fn from_selection(s: &mut Session, p: &Value) -> Result { fn duplicate(s: &mut Session, p: &Value) -> Result { let targets = selected(s)?; - let (dx, dy) = (f64_or(p, "dx", 10.0), f64_or(p, "dy", 10.0)); + let dx = number_or(p, "dx", 10.0, "object.slice.duplicate")?; + let dy = number_or(p, "dy", 10.0, "object.slice.duplicate")?; let ids = s.edit("Duplicate Slice", |d, sel| { let mut out = vec![]; for id in &targets { @@ -737,7 +738,7 @@ fn delete(s: &mut Session, p: &Value) -> Result { fn select(s: &mut Session, p: &Value) -> Result { let slices = p.get("slices").filter(|v| !v.is_null()).cloned().unwrap_or(json!([])); let ids = slice_targets(s, &json!({ "slices": slices }), "object.slice.select")?; - let toggle = bool_or(p, "toggle", false); + let toggle = switch_or(p, "toggle", false, "object.slice.select")?; s.select(|_, sel| { let mut now = if toggle { sel.slices.clone() } else { vec![] }; for id in ids { diff --git a/crates/engine/src/tests/menucmds.rs b/crates/engine/src/tests/menucmds.rs index e9a30c57e1..fccbe4a2d3 100644 --- a/crates/engine/src/tests/menucmds.rs +++ b/crates/engine/src/tests/menucmds.rs @@ -4107,6 +4107,60 @@ fn invalid_clear_guides_parameters_delete_nothing() { assert_eq!(guides(&s).len(), 1); } +/// Run `cmd` with `params`, expect an error naming `msg`, and check that the documents, the undo +/// history, the guides, the saved selections and the selection are as they were. +fn refuses_wrong_type(s: &mut Session, cmd: &str, params: Value, msg: &str) { + let state = |s: &Session| { + let st = s.doc().unwrap(); + (s.documents().len(), undo_len(s), guides(s), st.doc.saved_selections.clone(), st.selection.clone()) + }; + let before = state(s); + match s.execute(cmd, ¶ms) { + Ok(v) => panic!("{cmd} accepted {params}: {v}"), + Err(e) => assert!(e.to_string().contains(msg), "{cmd} {params}: {e}"), + } + assert!(state(s) == before, "{cmd} changed something for {params}"); +} + +#[test] +fn edit_guide_and_slice_commands_refuse_params_of_the_wrong_type() { + let mut s = session(); + let a = rect(&mut s, 10.0, 10.0, 40.0, 20.0); + s.execute("guide.add", &json!({"vertical": true, "pos": 200})).unwrap(); + let slice = s.execute("object.slice.fromSelection", &json!({"ids": [a.0]})).unwrap()["id"].as_u64().unwrap(); + sel(&mut s, &[a]); + s.execute("select.save", &json!({"name": "A"})).unwrap(); + s.execute("edit.copy", &json!({})).unwrap(); + for (cmd, params, msg) in [ + ("edit.duplicate", json!({"dx": "5"}), "`dx` must be a number, not \"5\""), + ("edit.duplicate", json!({"dy": true}), "`dy` must be a number, not true"), + ("edit.paste", json!({"dx": "5"}), "`dx` must be a number, not \"5\""), + ("edit.paste", json!({"dy": [1]}), "`dy` must be a number, not [1]"), + ("document.node", json!({"id": a.0, "summary": "yes"}), "`summary` must be true or false"), + ("file.close", json!({"index": "0"}), "`index` must be a non-negative integer"), + ("guide.add", json!({"pos": 50, "vertical": "true"}), "`vertical` must be true or false"), + ("guide.select", json!({"indexes": [0], "toggle": 1}), "`toggle` must be true or false"), + ("select.save", json!({"name": 5}), "`name` must be a string"), + ("select.editSaved", json!({"name": "A", "delete": "yes"}), "`delete` must be true or false"), + ("select.editSaved", json!({"name": "A", "newName": 5}), "`newName` must be a string"), + ] { + refuses_wrong_type(&mut s, cmd, params, msg); + } + s.execute("guide.select", &json!({"indexes": [0]})).unwrap(); + refuses_wrong_type(&mut s, "guide.move", json!({"dx": "10"}), "`dx` must be a number, not \"10\""); + refuses_wrong_type(&mut s, "guide.move", json!({"dy": 5, "copy": "yes"}), "`copy` must be true or false"); + s.execute("object.slice.select", &json!({"slices": [slice]})).unwrap(); + refuses_wrong_type(&mut s, "object.slice.duplicate", json!({"dx": "10"}), "`dx` must be a number, not \"10\""); + refuses_wrong_type(&mut s, "object.slice.select", json!({"slices": [slice], "toggle": "no"}), "`toggle` must be true or false"); + s.execute("edit.undoGroup.begin", &json!({})).unwrap(); + refuses_wrong_type(&mut s, "edit.undoGroup.end", json!({"cancel": "yes"}), "`cancel` must be true or false"); + // Null is read as a key left out. + s.execute("edit.undoGroup.end", &json!({"cancel": null})).unwrap(); + s.execute("guide.select", &json!({"indexes": [0]})).unwrap(); + s.execute("guide.move", &json!({"dx": 10, "dy": null, "copy": null})).unwrap(); + assert_eq!(guides(&s), vec![(true, 210.0)]); +} + /// When ruler guides and art are selected together, moving the art moves the guides as well; /// copy leaves both in place and moves the copies; single undo reverts both. #[test] diff --git a/docs/mcp.md b/docs/mcp.md index 3ff5e4c402..1d94f0b918 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -1472,6 +1472,14 @@ 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 ``. +The Edit, document, guide and slice commands also check the type of each param and change nothing when one is +wrong. A value that is not a number gets `` `dx` must be a number, not "10" ``: `dx` and `dy` in `edit.duplicate`, +`edit.paste`, `guide.move` and `object.slice.duplicate`. A value that is not true or false gets +`` `toggle` must be true or false ``: `summary` in `document.node`, `cancel` in `edit.undoGroup.end`, `vertical` in +`guide.add`, `toggle` in `guide.select` and `object.slice.select`, `copy` in `guide.move` and `delete` in +`select.editSaved`. `name` in `select.save` and `newName` in `select.editSaved` must be strings, and `index` in +`file.close` must be a non-negative integer. A null is read as a key left out. + `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