diff --git a/crates/engine/src/cmd.rs b/crates/engine/src/cmd.rs index d4fb2c39cd..19695fd4ff 100644 --- a/crates/engine/src/cmd.rs +++ b/crates/engine/src/cmd.rs @@ -559,6 +559,15 @@ pub(crate) fn given_object_ids(p: &Value, command: &str) -> Result usize { usize::try_from(v).unwrap_or(usize::MAX) } +/// The whole number of 0 or more that `p` gives for `key`, read with [`index`], or `None` when `p` +/// leaves it out or gives null. Any other value, such as `"3"`, `2.5`, `-1` or `true`, is an error +/// under `cmd`: "`{key}` must be a whole number". +pub(crate) fn index_opt(p: &Value, key: &str, cmd: &str) -> Result> { + match p.get(key) { + None | Some(Value::Null) => Ok(None), + Some(v) => v.as_u64().map(|n| Some(index(n))).ok_or_else(|| bad(cmd, format!("`{key}` must be a whole number"))), + } +} /// Validate all explicit targets before a command changes the document or selection. pub(crate) fn checked_ids_param(s: &Session, p: &Value, key: &str, command: &str) -> Result> { checked_nodes_param(s, p, key, command, false) diff --git a/crates/engine/src/cmd/edit.rs b/crates/engine/src/cmd/edit.rs index 576b05ffd6..9ecb5c3984 100644 --- a/crates/engine/src/cmd/edit.rs +++ b/crates/engine/src/cmd/edit.rs @@ -568,12 +568,11 @@ fn switch_units(s: &mut Session, _: &Value) -> Result { Ok(json!({ "units": next.label() })) } -/// The `artboard` param of the ruler origin commands: an artboard index, 0 when not given. +/// The `artboard` param of the ruler origin commands: an artboard index, 0 when left out or null. A +/// value that is not a whole number is "`artboard` must be a whole number"; one past the artboards +/// is "index out of range". fn artboard_param(s: &Session, p: &Value, c: &str) -> Result { - let index = match p.get("artboard") { - None => 0, - Some(v) => v.as_u64().and_then(|n| usize::try_from(n).ok()).ok_or_else(|| bad(c, "index out of range"))?, - }; + let index = super::index_opt(p, "artboard", c)?.unwrap_or(0); if index >= s.doc()?.doc.artboards.len() { return Err(bad(c, "index out of range")); } @@ -583,14 +582,15 @@ fn artboard_param(s: &Session, p: &Value, c: &str) -> Result { fn ruler_origin(s: &mut Session, p: &Value) -> Result { let d = &s.doc()?.doc; // A document without artboards measures from its global zero point. - let index = if d.artboards.is_empty() && p.get("artboard").is_none() { 0 } else { artboard_param(s, p, "document.rulerOrigin")? }; + let index = + if d.artboards.is_empty() && p.get("artboard").is_none_or(Value::is_null) { 0 } else { artboard_param(s, p, "document.rulerOrigin")? }; Ok(json!(s.doc()?.doc.ruler_origin_for(index))) } fn set_ruler_origin(s: &mut Session, p: &Value) -> Result { const C: &str = "document.setRulerOrigin"; let origin = Point::new(f64_req(p, "x", C)?, f64_req(p, "y", C)?); - let index = if s.doc()?.doc.rulers == RulerMode::Global && p.get("artboard").is_none() { 0 } else { artboard_param(s, p, C)? }; + let index = if s.doc()?.doc.rulers == RulerMode::Global && p.get("artboard").is_none_or(Value::is_null) { 0 } else { artboard_param(s, p, C)? }; change_ruler_origin(s, index, Some(origin), "Ruler Origin") } diff --git a/crates/engine/src/cmd/library.rs b/crates/engine/src/cmd/library.rs index ae8a7ef6b2..ced9347bc1 100644 --- a/crates/engine/src/cmd/library.rs +++ b/crates/engine/src/cmd/library.rs @@ -21,7 +21,7 @@ use vectorcraft_color::{Color, GradientPaint, Paint}; use vectorcraft_doc::{Appearance, Node, NodeId, TextStyleDef}; use super::clipboard::SwatchChoices; -use super::{CommandSpec, EngineError, Result, Session, always, bad, cmd, color_value, has_doc, ids_param, index, ok, point_param, str_param}; +use super::{CommandSpec, EngineError, Result, Session, always, bad, cmd, color_value, has_doc, ids_param, index_opt, ok, point_param, str_param}; use crate::Clipboard; /// The extension of a library file. @@ -1649,11 +1649,11 @@ fn move_group(s: &mut Session, p: &Value) -> Result { const C: &str = "library.moveGroup"; let id = library_id(s, p, C)?; let group = group_param(p, C)?; - let to = p.get("to").and_then(Value::as_u64).ok_or_else(|| bad(C, "missing `to` (an index)"))?; + let to = index_opt(p, "to", C)?.ok_or_else(|| bad(C, "missing `to` (an index)"))?; let (name, at) = s.libraries.change(&id, |lib| { let i = group_at(lib, group, C)?; let g = lib.groups.remove(i); - let at = index(to).min(lib.groups.len()); + let at = to.min(lib.groups.len()); let name = g.name.clone(); lib.groups.insert(at, g); Ok((name, at)) diff --git a/crates/engine/src/cmd/panelcmds.rs b/crates/engine/src/cmd/panelcmds.rs index f9c4b10049..c6731118d7 100644 --- a/crates/engine/src/cmd/panelcmds.rs +++ b/crates/engine/src/cmd/panelcmds.rs @@ -88,7 +88,7 @@ pub fn specs() -> Vec { fn artboard_reorder(s: &mut Session, p: &Value) -> Result { const C: &str = "artboard.reorder"; let i = artboard_index(&s.doc()?.doc, p, C)?; - let to = p.get("to").and_then(Value::as_u64).map(index).ok_or_else(|| bad(C, "missing to"))?; + let to = super::index_opt(p, "to", C)?.ok_or_else(|| bad(C, "missing to"))?; s.edit("Reorder Artboards", |d, _| { if i >= d.artboards.len() { return Err(EngineError::Other("no such artboard".into())); diff --git a/crates/engine/src/cmd/path.rs b/crates/engine/src/cmd/path.rs index 33bde936bf..9d7b42cca2 100644 --- a/crates/engine/src/cmd/path.rs +++ b/crates/engine/src/cmd/path.rs @@ -12,7 +12,7 @@ use super::draw2::usize_opt; use super::pathops::Beside; use super::{ CommandSpec, Result, Session, bad, check_numbers, check_switches, cmd, f64_or, f64_req, finite_numbers, has_anchors, has_doc, has_selected_path, - has_selection, ids_param, index, number_or, object_param, ok, str_param, switch_or, targets, + has_selection, ids_param, index_opt, number_or, object_param, ok, str_param, switch_or, targets, }; use crate::EngineError; @@ -170,7 +170,7 @@ pub(super) fn path_mut(d: &mut vectorcraft_doc::Document, id: NodeId) -> Result< fn append_anchor(s: &mut Session, p: &Value) -> Result { let id = object_param(p, "id", "path.appendAnchor")?.ok_or_else(|| bad("path.appendAnchor", "missing id"))?; let a = anchor_from_json(p, "path.appendAnchor")?; - let subpath = p.get("subpath").and_then(Value::as_u64).map(|u| u as usize); + let subpath = index_opt(p, "subpath", "path.appendAnchor")?; s.edit("Pen", |d, sel| { let path = path_mut(d, id)?; let si = match subpath { @@ -208,7 +208,7 @@ fn append_anchor(s: &mut Session, p: &Value) -> Result { /// `at` first moves the anchor, its handles with it (the Pen's closing drag with Space held). fn close(s: &mut Session, p: &Value) -> Result { let id = object_param(p, "id", "path.close")?.ok_or_else(|| bad("path.close", "missing id"))?; - let subpath = p.get("subpath").and_then(Value::as_u64).map(|u| u as usize); + let subpath = index_opt(p, "subpath", "path.close")?; let handle = |key: &str| match p.get(key).filter(|h| !h.is_null()) { None => Ok(None), Some(h) => finite_numbers(h).map(|[x, y]| Some(Point::new(x, y))).ok_or_else(|| bad("path.close", format!("`{key}` must be [x, y]"))), @@ -310,7 +310,7 @@ pub(crate) fn transform_anchors(s: &mut Session, label: &str, xf: vectorcraft_ge fn set_handle(s: &mut Session, p: &Value) -> Result { let id = object_param(p, "id", "path.setHandle")?.ok_or_else(|| bad("path.setHandle", "missing id"))?; let si = usize_opt(p, "subpath", "path.setHandle")?.unwrap_or(0); - let ai = p.get("anchor").and_then(Value::as_u64).map(index).ok_or_else(|| bad("path.setHandle", "missing anchor"))?; + let ai = index_opt(p, "anchor", "path.setHandle")?.ok_or_else(|| bad("path.setHandle", "missing anchor"))?; let out = str_param(p, "which") != Some("in"); let pos = Point::new(f64_req(p, "x", "path.setHandle")?, f64_req(p, "y", "path.setHandle")?); let independent = switch_or(p, "independent", false, "path.setHandle")?; @@ -337,7 +337,7 @@ fn set_handle(s: &mut Session, p: &Value) -> Result { fn move_anchor(s: &mut Session, p: &Value) -> Result { let id = object_param(p, "id", "path.moveAnchor")?.ok_or_else(|| bad("path.moveAnchor", "missing id"))?; let si = usize_opt(p, "subpath", "path.moveAnchor")?.unwrap_or(0); - let ai = p.get("anchor").and_then(Value::as_u64).map(index).ok_or_else(|| bad("path.moveAnchor", "missing anchor"))?; + let ai = index_opt(p, "anchor", "path.moveAnchor")?.ok_or_else(|| bad("path.moveAnchor", "missing anchor"))?; let pos = Point::new(f64_req(p, "x", "path.moveAnchor")?, f64_req(p, "y", "path.moveAnchor")?); // An anchor already at (x, y) changes nothing and records no undo step. if let Some(a) = anchor_of(s, id, si, ai)? @@ -453,7 +453,7 @@ pub(super) fn paths_in(d: &vectorcraft_doc::Document, ids: &[NodeId]) -> Vec Result { const C: &str = "path.reverse"; let want = p.get("reversed").map(|v| v.as_bool().ok_or_else(|| bad(C, "`reversed` must be true or false"))).transpose()?; - let subpath = p.get("subpath").and_then(Value::as_u64).map(|u| u as usize); + let subpath = index_opt(p, "subpath", C)?; let ids = paths_in(&s.doc()?.doc, &targets(s, p)?); if ids.is_empty() { return Err(bad(C, "select paths")); @@ -1784,7 +1784,7 @@ fn reshape(s: &mut Session, p: &Value) -> Result { fn insert_anchor(s: &mut Session, p: &Value) -> Result { let id = object_param(p, "id", "path.insertAnchor")?.ok_or_else(|| bad("path.insertAnchor", "missing id"))?; let si = usize_opt(p, "subpath", "path.insertAnchor")?.unwrap_or(0); - let seg = p.get("segment").and_then(Value::as_u64).map(index).ok_or_else(|| bad("path.insertAnchor", "missing segment"))?; + let seg = index_opt(p, "segment", "path.insertAnchor")?.ok_or_else(|| bad("path.insertAnchor", "missing segment"))?; let t = number_or(p, "t", 0.5, "path.insertAnchor")?.clamp(0.0, 1.0); let idx = s.edit("Add Anchor Point", |d, sel| { let path = path_mut(d, id)?; diff --git a/crates/engine/src/cmd/style.rs b/crates/engine/src/cmd/style.rs index 370664f795..bb45bf0fca 100644 --- a/crates/engine/src/cmd/style.rs +++ b/crates/engine/src/cmd/style.rs @@ -21,7 +21,7 @@ use vectorcraft_color::Paint; use vectorcraft_doc::{Appearance, DEFAULT_GRAPHIC_STYLE, Document, GraphicStyle, Node, NodeId, NodeKind, Shared}; use vectorcraft_geom::Rect; -use super::{CommandSpec, Result, Session, always, bad, bool_or, cmd, has_doc, index, object_param, ok, str_param, unique_name}; +use super::{CommandSpec, Result, Session, always, bad, bool_or, cmd, has_doc, index_opt, object_param, ok, str_param, unique_name}; use crate::EngineError; pub fn specs() -> Vec { @@ -585,10 +585,10 @@ fn style_merge(s: &mut Session, p: &Value) -> Result { fn style_move(s: &mut Session, p: &Value) -> Result { let cmd = "graphicStyle.move"; let name = str_param(p, "name").ok_or_else(|| bad(cmd, "missing name"))?; - let to = p.get("to").and_then(Value::as_u64).ok_or_else(|| bad(cmd, "missing `to` (an index)"))?; + let to = index_opt(p, "to", cmd)?.ok_or_else(|| bad(cmd, "missing `to` (an index)"))?; let d = &s.doc()?.doc; let from = style_index(d, name)?; - let to = index(to).min(d.graphic_styles.len() - 1); + let to = to.min(d.graphic_styles.len() - 1); if from == to { return ok(); } diff --git a/crates/engine/src/cmd/typecmd.rs b/crates/engine/src/cmd/typecmd.rs index 6b14c98916..b3fab4a2bc 100644 --- a/crates/engine/src/cmd/typecmd.rs +++ b/crates/engine/src/cmd/typecmd.rs @@ -12,8 +12,8 @@ use vectorcraft_geom::{Affine, PathData, Vec2}; use super::edit::selected_roots; use super::pathops::{length_given, number_given, shape_node}; use super::{ - CommandSpec, EngineError, Result, Session, bad, check_numbers, cmd, color_value, f64_req, has_doc, has_selection, index, number_or, object_param, - ok, str_param, switch_or, targets, text_param, + CommandSpec, EngineError, Result, Session, bad, check_numbers, cmd, color_value, f64_req, has_doc, has_selection, index, index_opt, number_or, + object_param, ok, str_param, switch_or, targets, text_param, }; pub fn specs() -> Vec { @@ -1376,7 +1376,7 @@ fn set_path_handle(s: &mut Session, p: &Value) -> Result { const C: &str = "text.setPathHandle"; let id = object_param(p, "id", C)?.ok_or_else(|| bad(C, "missing id"))?; let si = super::draw2::usize_opt(p, "subpath", C)?.unwrap_or(0); - let ai = p.get("anchor").and_then(Value::as_u64).map(index).ok_or_else(|| bad(C, "missing anchor"))?; + let ai = index_opt(p, "anchor", C)?.ok_or_else(|| bad(C, "missing anchor"))?; let out = super::object::choice(p, C, "which", &["in", "out"])? != Some("in"); let pos = vectorcraft_geom::Point::new(f64_req(p, "x", C)?, f64_req(p, "y", C)?); let independent = switch_or(p, "independent", false, C)?; diff --git a/crates/engine/src/tests/params.rs b/crates/engine/src/tests/params.rs index 8a33376075..52cb314de0 100644 --- a/crates/engine/src/tests/params.rs +++ b/crates/engine/src/tests/params.rs @@ -849,3 +849,75 @@ fn journal_entries_pass_the_check() { assert!(check_params(id, params).is_ok(), "{id} {params}"); } } + +/// An index of a path, type, panel, library, style or ruler command given as something other than a +/// whole number of 0 or more (`"3"`, `2.5`, `-1`, `true`, `[]`, `{}`) is refused as the wrong type, +/// naming its key. A required one is not reported as missing, and an optional one (`subpath` of +/// `path.appendAnchor`, `path.close` and `path.reverse`, `artboard` of the ruler origin commands) is +/// not read as its default. A required one left out or null is still missing, and an optional one left +/// out or null keeps its default. Neither changes the document or records an undo step. +#[test] +fn a_malformed_index_is_not_missing_or_its_default() { + let mut s = Session::new(); + s.execute("file.new", &json!({})).unwrap(); + let id = s.execute("path.create", &json!({"d": "M0 0 L10 0 M20 0 L30 0"})).unwrap()["id"].clone(); + s.execute("library.create", &json!({"name": "Brand"})).unwrap(); + s.execute("library.createGroup", &json!({"name": "Warm"})).unwrap(); + let doc = s.doc().unwrap().doc.clone(); + let undo = s.doc().unwrap().history.undo.len(); + let with = |base: &serde_json::Value, key: &str, v: Option| { + let mut p = base.clone(); + let o = p.as_object_mut().unwrap(); + match v { + Some(v) => o.insert(key.into(), v), + None => o.remove(key), + }; + p + }; + let refused = |s: &mut Session, cmd: &str, p: &serde_json::Value, want: &str| match s.execute(cmd, p) { + Err(EngineError::BadParams { cmd: c, msg }) => assert_eq!((c.as_str(), msg.as_str()), (cmd, want), "{p}"), + other => panic!("{cmd} {p}: {other:?}"), + }; + let malformed = [json!("3"), json!(2.5), json!(-1), json!(true), json!([]), json!({})]; + let handle = json!({"id": id, "x": 5, "y": 5}); + let required = [ + ("path.setHandle", handle.clone(), "anchor", "missing anchor"), + ("path.moveAnchor", handle.clone(), "anchor", "missing anchor"), + ("path.insertAnchor", json!({"id": id}), "segment", "missing segment"), + ("text.setPathHandle", handle.clone(), "anchor", "missing anchor"), + ("artboard.reorder", json!({"index": 0}), "to", "missing to"), + ("library.moveGroup", json!({"group": "Warm"}), "to", "missing `to` (an index)"), + ("graphicStyle.move", json!({"name": "Default"}), "to", "missing `to` (an index)"), + ]; + for (cmd, base, key, missing) in &required { + for v in &malformed { + refused(&mut s, cmd, &with(base, key, Some(v.clone())), &format!("`{key}` must be a whole number")); + } + for p in [with(base, key, None), with(base, key, Some(json!(null)))] { + refused(&mut s, cmd, &p, missing); + } + } + let optional = [ + ("path.appendAnchor", handle.clone(), "subpath"), + ("path.close", json!({"id": id}), "subpath"), + ("path.reverse", json!({"ids": [id]}), "subpath"), + ("document.rulerOrigin", json!({}), "artboard"), + ("document.setRulerOrigin", json!({"x": 5, "y": 5}), "artboard"), + ("document.resetRulerOrigin", json!({}), "artboard"), + ]; + for (cmd, base, key) in &optional { + for v in &malformed { + refused(&mut s, cmd, &with(base, key, Some(v.clone())), &format!("`{key}` must be a whole number")); + } + } + // A whole number that names no artboard is still out of range. + refused(&mut s, "document.resetRulerOrigin", &json!({"artboard": 1}), "index out of range"); + assert_eq!(s.doc().unwrap().doc, doc); + assert_eq!(s.doc().unwrap().history.undo.len(), undo); + // Left out or null, an optional one keeps its default. + for (cmd, base, key) in &optional { + for p in [with(base, key, None), with(base, key, Some(json!(null)))] { + s.execute(cmd, &p).unwrap_or_else(|e| panic!("{cmd} {p}: {e}")); + } + } +} diff --git a/docs/mcp.md b/docs/mcp.md index 4507bfdbf3..ca77311c58 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -1804,6 +1804,13 @@ commands fail and change nothing, and `appearance.removeItem` removes nothing. A the item last. A text `start` or `end` is past the end of the text. `object.slice.divide` refuses such a `rows` or `columns`, `recolor.reduce` keeps each color in its own row for such a `colors`, and `layer.setProps` refuses such a `color`. `object.setBounds` reads a `reference` past 8 as 8, as `object.transformEach` does. +An index or place given as a value that is not a whole number of 0 or more, such as `"3"`, `2.5`, `-1` or `true`, is +refused with `` `anchor` must be a whole number ``, naming the key: `anchor` of `path.setHandle`, `path.moveAnchor` and +`text.setPathHandle`, `segment` of `path.insertAnchor`, `to` of `artboard.reorder`, `library.moveGroup` and +`graphicStyle.move`, `subpath` of `path.appendAnchor`, `path.close` and `path.reverse`, and `artboard` of +`document.rulerOrigin`, `document.setRulerOrigin` and `document.resetRulerOrigin`. A required one left out or null is +reported as missing (`missing anchor`). Left out or null, `subpath` takes the command's default and `artboard` is 0. +Nothing changes and no undo step is recorded. The blend, envelope and gradient mesh commands refuse a string or another value that is not a number with the same message, as in `` `steps` must be a number, not "8" ``, and change nothing: `steps`, `distance` and `value` in `object.blend.make` and `object.blend.options`; `bend`, `h`, `v`, `horizontalDistortion` and `verticalDistortion` in diff --git a/docs/target-app-parity.md b/docs/target-app-parity.md index a5b3d62925..7e36c7f318 100644 --- a/docs/target-app-parity.md +++ b/docs/target-app-parity.md @@ -1119,6 +1119,7 @@ Moved from the ROADMAP's "Shipped so far" on 2026-10-10. Update it when a featur `path.curvatureEdit`, `path.removeAnchor`, `path.convertAnchor`, `path.reshapeSegment`, `path.split`, `path.setHandle`, `path.moveAnchor` and `path.insertAnchor` refuse a `subpath` that is not a whole number of 0 or more, and `path.split` refuses such an `anchor` or `segment`; without a `subpath` they edit subpath 0. A value given that is not a whole number, such as `"1"` or `1.5`, is refused with `` `subpath` must be a whole number ``, naming its key, as are a required index of the Appearance panel and Smart Pen commands, `sourceDocument` of `edit.dropSelection` and `index` and `to` of `document.activate` and `document.move`; a required one left out or null is reported as missing (`tests::params::a_malformed_required_integer_is_not_reported_as_missing`). `path.reshapeSegment` refuses a delta of 1e9 pt or longer, as `object.blend.spine.reshapeSegment` does, and `path.convertAnchor` and `path.freehand` refuse a point whose x or y is 1e9 pt or more in size. `path.reshapeSegment`, `object.blend.spine.reshapeSegment`, `path.convertAnchor`, `path.curvatureEdit`, `path.insertAnchor`, `path.moveAnchors`, `path.reshape`, `path.close` and the Pencil, Curvature, Knife, Eraser, Path Eraser, Smooth, Join and Blob Brush commands refuse a number, point or handle given as text, such as "10", and leave the document unchanged; `path.convertAnchor` refuses an `x` without a `y`, or a `y` without an `x`. + `path.setHandle`, `path.moveAnchor`, `text.setPathHandle` (`anchor`), `path.insertAnchor` (`segment`), `artboard.reorder`, `library.moveGroup`, `graphicStyle.move` (`to`), `path.appendAnchor`, `path.close`, `path.reverse` (`subpath`) and the ruler origin commands (`artboard`) refuse a value that is not a whole number of 0 or more, such as `"3"` or `2.5`, with `` `anchor` must be a whole number ``, naming the key, instead of reporting it as missing or reading it as the default subpath or artboard; a null `artboard` is artboard 0 (`tests::params::a_malformed_index_is_not_missing_or_its_default`). `path.close` and `path.setHandle` (`independent`), `path.moveAnchors` (`patterns`), the Pencil (`closed`, `keepSelected`, `fill`), Curvature (`closed` and a point's `corner`), `path.curvatureEdit` (`corner`), `path.reshapeSegment` (`perpendicular`) and the Blob Brush (`keepSelected`, `mergeOnlySelection`, `merge`) refuse a value other than true, false or null, such as "true", and leave the document unchanged. Direct Selection's marquee selects the frame anchors of area type that lie inside it, and a click on a frame anchor selects that one; Shift toggles them. The selected frame anchors show filled. A drag of one of them moves all of them, the arrow keys nudge them, and the type reflows at its size (`text.reshapeArea`, which takes `items` to move the anchors of several frames in one step), as [Create text](http://web.archive.org/web/20220127090642/https://helpx.adobe.com/illustrator/using/creating-text.html) and [Selecting objects](http://web.archive.org/web/20211230015528/https://helpx.adobe.com/illustrator/using/selecting-objects.html) describe. When the selection also holds other objects, path anchors or ruler guides, or when another change to the document or the selection follows the pick, the type is selected whole. A press on a selected object's centre point (Attributes panel › Show Center) selects the object whole, and a drag from it moves the object as one Move, held by that point (#4891; test `the_centre_point_selects_and_moves_the_object_whole`).