From 922c1fcae579509f9443a94b03d855824ae71c81 Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Wed, 11 Nov 2026 17:39:46 -0800 Subject: [PATCH] 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 | 20 ++++++----- crates/engine/src/cmd/edit.rs | 22 +++++++----- crates/engine/src/cmd/slices.rs | 9 ++--- crates/engine/src/tests/menucmds.rs | 54 +++++++++++++++++++++++++++++ docs/mcp.md | 8 +++++ 5 files changed, 92 insertions(+), 21 deletions(-) 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 7a8a91f0af..dd01a3aeab 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); @@ -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 { 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 5826c5c8c4..ea7e75fe5c 100644 --- a/docs/mcp.md +++ b/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