Commands: Edit, guide and slice params of the wrong type are refused

These commands read optional numbers and flags with f64_or and bool_or,
so a value such as "10" or "yes" ran the command with the default. They
now return an error and leave the document, the selection and the undo
history as they were. Omitting a key or giving null behaves as before.

- edit.duplicate, edit.paste, guide.move, object.slice.duplicate: dx, dy
- guide.move: copy
- document.node: summary
- edit.undoGroup.end: cancel
- guide.add: vertical
- guide.select, object.slice.select: toggle
- select.editSaved: delete; newName must be a string
- select.save: name must be a string
- file.close: index must be a non-negative integer

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<CommandSpec> {
@@ -482,7 +482,7 @@ fn save_selection(s: &mut Session, p: &Value) -> Result<Value> {
     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<Value> {
     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<Value> {
 
 fn guide_add(s: &mut Session, p: &Value) -> Result<Value> {
     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<Value> {
     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::<Result<Vec<_>>>()?;
-    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<Value> {
     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(

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<CommandSpec> {
 }
 
 fn file_close(s: &mut Session, p: &Value) -> Result<Value> {
-    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<Value> {
 fn doc_node(s: &mut Session, p: &Value) -> Result<Value> {
     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<Value> {
 }
 
 fn undo_group_end(s: &mut Session, p: &Value) -> Result<Value> {
-    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<Value> {
     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);
@@ -1315,7 +1319,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();
@@ -1489,8 +1493,8 @@ fn layer_named(d: &mut Document, name: &str) -> NodeId {
 
 fn duplicate(s: &mut Session, p: &Value) -> Result<Value> {
     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::<Vec<_>>() }))
 }

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<Value> {
 
 fn duplicate(s: &mut Session, p: &Value) -> Result<Value> {
     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<Value> {
 fn select(s: &mut Session, p: &Value) -> Result<Value> {
     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 {

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, &params) {
+        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]

docs/mcp.md

@@ -1418,6 +1418,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