diff --git a/apps/figcraft-cli/src/files.rs b/apps/figcraft-cli/src/files.rs index 1ba9d60a..8e689ec0 100644 --- a/apps/figcraft-cli/src/files.rs +++ b/apps/figcraft-cli/src/files.rs @@ -366,6 +366,7 @@ pub fn parse_script(text: &str) -> Result, String> { .enumerate() .map(|(i, s)| { let cmd = s.get("command").and_then(Value::as_str).ok_or_else(|| format!("step {i}: `command` (a string) is required"))?; + figcraft_engine::batch::refuse_unknown_step_keys(i, &s).map_err(|e| e.to_string())?; let params = match s.get("params") { None | Some(Value::Null) => json!({}), Some(p @ Value::Object(_)) => p.clone(), @@ -443,6 +444,9 @@ mod tests { assert!(parse_script("[{\"command\":\"a\",\"params\":3}]").is_err()); assert!(parse_script("not json\n").is_err()); assert!(parse_script("42").is_err()); + // `args` for `params` would run the step without them (#1413). + let e = parse_script(r#"[{"command":"a"},{"command":"b","args":{"x":1}}]"#).unwrap_err().to_string(); + assert!(e.contains("step 1: unknown key `args`"), "{e}"); } #[test] diff --git a/crates/engine/src/automation/batch.rs b/crates/engine/src/automation/batch.rs index 5c4550ec..aa11a2bb 100644 --- a/crates/engine/src/automation/batch.rs +++ b/crates/engine/src/automation/batch.rs @@ -39,6 +39,18 @@ pub fn step_name(step: usize, json: &Value) -> Result> { } } +/// Refuse a step key other than `command`, `params` and `as` (`step` is its index, for the +/// error): `args` in place of `params` would run the command without them. +pub fn refuse_unknown_step_keys(step: usize, json: &Value) -> Result<()> { + let Some(o) = json.as_object() else { return Ok(()) }; + let unknown: Vec = o.keys().filter(|k| !matches!(k.as_str(), "command" | "params" | "as")).map(|k| format!("`{k}`")).collect(); + if unknown.is_empty() { + return Ok(()); + } + let s = if unknown.len() == 1 { "" } else { "s" }; + Err(bad("command.batch", format!("step {step}: unknown key{s} {} (a step takes command, params and as)", unknown.join(", ")))) +} + /// The steps of one batch run so far: their results, and which step each `as` name belongs to. /// A nested `command.batch` step gets its own scope, so its references are to its own steps. #[derive(Clone, Debug, Default)] diff --git a/crates/engine/src/canvas/vectoredit/width.rs b/crates/engine/src/canvas/vectoredit/width.rs index 13c7be0c..0e565cc9 100644 --- a/crates/engine/src/canvas/vectoredit/width.rs +++ b/crates/engine/src/canvas/vectoredit/width.rs @@ -244,7 +244,7 @@ pub(crate) fn specs() -> Vec { vec![spec!( "vector.edit.widthPoint", "Width point", - "{action?: add|set|remove, index?, t? (0-1 along the path), width? (px) | scale? (× stroke weight)} — add needs t (width defaults to the current one); set moves or resizes point `index`; with `side` only that side of the path changes (looking along it): `width` is then the distance from the centre line to that edge and `scale` that side's scale (× stroke weight, as if both sides were that wide)", + "{action?: add|set|remove, index?, t? (0-1 along the path), width? (px) | scale? (× stroke weight), side? (left|right)} — add needs t (width defaults to the current one); set moves or resizes point `index`; with `side` only that side of the path changes (looking along it): `width` is then the distance from the centre line to that edge and `scale` that side's scale (× stroke weight, as if both sides were that wide)", true, width_point )] diff --git a/crates/engine/src/cmd.rs b/crates/engine/src/cmd.rs index a247ed52..c046cd19 100644 --- a/crates/engine/src/cmd.rs +++ b/crates/engine/src/cmd.rs @@ -63,6 +63,8 @@ macro_rules! spec { struct Registry { list: Vec, index: HashMap<&'static str, usize>, + /// The parameter names each command's doc lists (`list` order; see [`doc_param_names`]). + names: Vec>>, } fn registry() -> &'static Registry { @@ -123,7 +125,8 @@ fn registry() -> &'static Registry { log::warn!("duplicate command id {}", s.id); } } - Registry { list, index } + let names = list.iter().map(|s| doc_param_names(s.params)).collect(); + Registry { list, index, names } }) } @@ -136,6 +139,113 @@ pub fn find_command(id: &str) -> Option<&'static CommandSpec> { r.index.get(id).and_then(|i| r.list.get(*i)) } +/// The parameter names command `id` takes, from its doc; `None` when the doc isn't a `{…}` list +/// (`node.set` documents its props, and refuses the ones it doesn't know itself). +pub fn param_names(id: &str) -> Option<&'static [&'static str]> { + let r = registry(); + r.index.get(id).and_then(|i| r.names.get(*i)).and_then(|n| n.as_deref()) +} + +/// Refuse params naming something the command doesn't take: a misspelt name would otherwise be +/// ignored and the command answer as if it hadn't been given. `ids` and `id` are always taken: +/// the layers a command acts on (`params::targets` reads either). +pub fn refuse_unknown_params(id: &str, params: &Value) -> Result<()> { + let (Some(names), Some(given)) = (param_names(id), params.as_object()) else { return Ok(()) }; + let taken = |k: &str| matches!(k, "ids" | "id") || names.contains(&k); + let unknown: Vec = given.keys().filter(|k| !taken(k)).map(|k| format!("`{k}`")).collect(); + if unknown.is_empty() { + return Ok(()); + } + let s = if unknown.len() == 1 { "" } else { "s" }; + let takes = if names.is_empty() { "none".to_string() } else { names.join(", ") }; + Err(crate::bad(id, format!("unknown parameter{s} {} (it takes {takes})", unknown.join(", ")))) +} + +/// The top-level items of a `{…}` params doc (`{ids?, direction? (horizontal|vertical)}` → +/// `ids?`, ` direction? (horizontal|vertical)`), with those of the alternatives after it +/// (`{x, y} | {points}`); `None` when the doc doesn't start with one. +pub(crate) fn doc_items(doc: &'static str) -> Option> { + let mut body = doc.trim_start().strip_prefix('{')?; + let (mut items, mut depth, mut start) = (Vec::new(), 0usize, 0); + loop { + let mut rest = None; + for (i, c) in body.char_indices() { + match c { + '{' | '[' | '(' => depth += 1, + '}' | ']' | ')' if depth > 0 => depth -= 1, + '}' | ',' if depth == 0 => { + items.push(body.get(start..i).unwrap_or_default()); + start = i + c.len_utf8(); + if c == '}' { + rest = body.get(start..); + break; + } + } + _ => {} + } + } + let rest = rest?; + match rest.trim_start().strip_prefix('|').and_then(|r| r.trim_start().strip_prefix('{')) { + Some(next) => (body, start) = (next, 0), + None => return Some(items), + } + } +} + +/// The names one doc item gives: its first word and the words joined to it by `|`, `/` or `+` +/// (`id?/ids?`, `path | data (base64) + name`, `value? + newValue?`), without the `?` of optional +/// ones. Bracketed text describes a name, and what follows a `:`, `=` or `→` its value. +pub(crate) fn item_names(item: &'static str) -> Vec<&'static str> { + let mut outside = Vec::new(); + let (mut depth, mut start) = (0usize, None); + for (i, c) in item.char_indices() { + let value_follows = depth == 0 && matches!(c, ':' | '=' | '→'); + let in_word = depth == 0 && !value_follows && !c.is_whitespace() && !matches!(c, '(' | '[' | '{' | ')' | ']' | '}'); + match c { + '(' | '[' | '{' => depth += 1, + ')' | ']' | '}' => depth = depth.saturating_sub(1), + _ => {} + } + match (in_word, start) { + (true, None) => start = Some(i), + (false, Some(s)) => { + outside.push(item.get(s..i).unwrap_or_default()); + start = None; + } + _ => {} + } + if value_follows { + break; + } + } + if let Some(s) = start { + outside.push(item.get(s..).unwrap_or_default()); + } + let is_name = + |n: &&str| n.chars().next().is_some_and(|c| c.is_ascii_alphabetic() || c == '_') && n.chars().all(|c| c.is_ascii_alphanumeric() || c == '_'); + let mut words = outside.into_iter(); + let (mut names, mut word) = (Vec::new(), words.next()); + while let Some(w) = word { + names.extend(w.split(['/', '|']).map(|n| n.trim_end_matches('?')).filter(is_name)); + word = match words.next() { + Some("|" | "/" | "+") => words.next(), + _ => None, + }; + } + names +} + +/// Every name a `{…}` params doc lists, once each (see [`item_names`]). +fn doc_param_names(doc: &'static str) -> Option> { + let mut names = Vec::new(); + for n in doc_items(doc)?.into_iter().flat_map(item_names) { + if !names.contains(&n) { + names.push(n); + } + } + Some(names) +} + /// `{"ok": true}` shorthand. pub fn ok() -> Result { Ok(serde_json::json!({"ok": true})) diff --git a/crates/engine/src/cmd/comment.rs b/crates/engine/src/cmd/comment.rs index 3fde2b09..baac9ab3 100644 --- a/crates/engine/src/cmd/comment.rs +++ b/crates/engine/src/cmd/comment.rs @@ -576,7 +576,7 @@ pub fn specs() -> Vec { Ok(json!({"id": id, "link": link(id)})) } ), - spec!("comment.reply", "Reply to comment", "{id, text, author?}", true, |s, p| { + spec!("comment.reply", "Reply to comment", "{id | link, text, author?}", true, |s, p| { let id = req_comment("comment.reply", s, p)?; let who = author(s, p); let r = Message::new(who.clone(), text("comment.reply", p)?, now_secs()); @@ -586,7 +586,7 @@ pub fn specs() -> Vec { c.touched_by(&who); Ok(json!({"reply": c.replies.len() - 1})) }), - spec!("comment.edit", "Edit comment", "{id, text, reply? (index; omit for the first message), author?}", true, |s, p| { + spec!("comment.edit", "Edit comment", "{id | link, text, reply? (index; omit for the first message), author?}", true, |s, p| { let id = req_comment("comment.edit", s, p)?; let t = text("comment.edit", p)?; let who = author(s, p); @@ -607,7 +607,7 @@ pub fn specs() -> Vec { spec!( "comment.react", "React to comment", - "{id, emoji, reply? (index; omit for the first message), on? (default: toggle), author?}", + "{id | link, emoji, reply? (index; omit for the first message), on? (default: toggle), author?}", true, |s, p| { let id = req_comment("comment.react", s, p)?; @@ -629,7 +629,7 @@ pub fn specs() -> Vec { Ok(json!({"on": on})) } ), - spec!("comment.resolve", "Resolve comment", "{id, resolved? (default true)}", true, |s, p| { + spec!("comment.resolve", "Resolve comment", "{id | link, resolved? (default true)}", true, |s, p| { let id = req_comment("comment.resolve", s, p)?; let v = boolean(p, "resolved").unwrap_or(true); if let Some(c) = s.doc_mut()?.comment_mut(id) { @@ -640,7 +640,7 @@ pub fn specs() -> Vec { spec!( "comment.move", "Move comment", - "{id, x, y, anchor? (default true: pin to the innermost frame under the point; false: to the page; or a node id)}", + "{id | link, x, y, anchor? (default true: pin to the innermost frame under the point; false: to the page; or a node id)}", true, |s, p| { let id = req_comment("comment.move", s, p)?; @@ -655,24 +655,30 @@ pub fn specs() -> Vec { ok() } ), - spec!("comment.delete", "Delete comment", "{id, reply? (index: delete one reply; omit to delete the thread), author?}", true, |s, p| { - let id = req_comment("comment.delete", s, p)?; - let who = author(s, p); - let doc = s.doc_mut()?; - let c = doc.comment_mut(id).ok_or_else(|| bad("comment.delete", "no comment"))?; - match message_index("comment.delete", c, p)? { - 0 => { - own("comment.delete", &c.first, &who, "delete this thread")?; - doc.comments.retain(|c| c.id != id); - } - i => { - let m = c.message(i).ok_or_else(|| bad("comment.delete", "no message"))?; - own("comment.delete", m, &who, "delete this reply")?; - c.replies.remove(i - 1); + spec!( + "comment.delete", + "Delete comment", + "{id | link, reply? (index: delete one reply; omit to delete the thread), author?}", + true, + |s, p| { + let id = req_comment("comment.delete", s, p)?; + let who = author(s, p); + let doc = s.doc_mut()?; + let c = doc.comment_mut(id).ok_or_else(|| bad("comment.delete", "no comment"))?; + match message_index("comment.delete", c, p)? { + 0 => { + own("comment.delete", &c.first, &who, "delete this thread")?; + doc.comments.retain(|c| c.id != id); + } + i => { + let m = c.message(i).ok_or_else(|| bad("comment.delete", "no message"))?; + own("comment.delete", m, &who, "delete this reply")?; + c.replies.remove(i - 1); + } } + ok() } - ok() - }), + ), spec!( "comment.markRead", "Mark comment as read", @@ -711,7 +717,7 @@ pub fn specs() -> Vec { spec!( "comment.next", "Next comment", - "{id? (the thread to step from; omitted: the first or last), dir? (1 next, -1 previous; default 1), page?, filter?, sort?, query? (the list, as comment.list), author?}", + "{id? (the thread to step from; omitted: the first or last), dir? (1 next, -1 previous; default 1), page?, filter?, sort?, query?, frame?, resolved? (the list, as comment.list), author?}", false, |s, p| { let dir = num(p, "dir").unwrap_or(1.0); @@ -748,7 +754,7 @@ pub fn specs() -> Vec { } Ok(json!({"zoom": v.zoom})) }), - spec!("comment.link", "Copy link to comment", "{id}", false, |s, p| { + spec!("comment.link", "Copy link to comment", "{id | link}", false, |s, p| { let id = req_comment("comment.link", s, p)?; Ok(json!({"link": link(id)})) }), diff --git a/crates/engine/src/cmd/create.rs b/crates/engine/src/cmd/create.rs index 2e64ce04..c4c97252 100644 --- a/crates/engine/src/cmd/create.rs +++ b/crates/engine/src/cmd/create.rs @@ -53,37 +53,61 @@ pub fn specs() -> Vec { spec!( "node.create", "Create layer", - "{type: rectangle|ellipse|polygon|star|line|arrow|vector|text|frame|section|component|slice, x?, y? (relative to the parent frame; groups don't count), width?, height?, parent? (id; default: the page), index?, name?, text? (for text), path? (SVG path data for vector), props? {node.set keys}, select? (default true)}", + "{type: rectangle|ellipse|polygon|star|line|arrow|vector|text|frame|section|component|slice, x?, y? (relative to the parent frame; groups don't count), width?, height?, parent? (id; default: the page), index?, name?, text? | characters? (for text), path? | points? (vector: SVG path data | [[x,y],..]), closed? (vector), props? {node.set keys}, select? (default true)}", true, create ), spec!( "frame.create", "Create frame", - "{preset? (name from FRAME_PRESETS), x?, y?, width?, height?, parent?, name?, props?} (a preset without x/y lands to the right of the page's frames)", + "{preset? (name from FRAME_PRESETS), x?, y?, width?, height?, parent?, index?, name?, props?, select? (default true)} (a preset without x/y lands to the right of the page's frames)", true, frame_create ), - spec!("shape.rectangle", "Rectangle", "{x, y, width, height, parent?, props?}", true, |s, p| create_kind(s, p, "rectangle")), - spec!("shape.ellipse", "Ellipse", "{x, y, width, height, parent?, props?}", true, |s, p| create_kind(s, p, "ellipse")), - spec!("shape.polygon", "Polygon", "{x, y, width, height, parent?, props? {pointCount}}", true, |s, p| create_kind(s, p, "polygon")), - spec!("shape.star", "Star", "{x, y, width, height, parent?, props? {pointCount, innerRadius}}", true, |s, p| create_kind(s, p, "star")), - spec!("shape.line", "Line", "{x, y, width (length), parent?, props? {rotation}}", true, |s, p| create_kind(s, p, "line")), - spec!("shape.arrow", "Arrow", "{x, y, width (length), parent?, props?}", true, |s, p| create_kind(s, p, "arrow")), - spec!("text.create", "Text", "{text, x, y, parent?, width? (fixed width wraps), props? (fontSize, fontFamily, …)}", true, |s, p| { - create_kind(s, p, "text") + spec!("shape.rectangle", "Rectangle", "{x, y, width, height, parent?, index?, name?, props?, select? (default true)}", true, |s, p| { + create_kind(s, p, "rectangle") }), + spec!("shape.ellipse", "Ellipse", "{x, y, width, height, parent?, index?, name?, props?, select? (default true)}", true, |s, p| create_kind( + s, p, "ellipse" + )), + spec!( + "shape.polygon", + "Polygon", + "{x, y, width, height, parent?, index?, name?, props? {pointCount}, select? (default true)}", + true, + |s, p| create_kind(s, p, "polygon") + ), + spec!( + "shape.star", + "Star", + "{x, y, width, height, parent?, index?, name?, props? {pointCount, innerRadius}, select? (default true)}", + true, + |s, p| create_kind(s, p, "star") + ), + spec!("shape.line", "Line", "{x, y, width (length), parent?, index?, name?, props? {rotation}, select? (default true)}", true, |s, p| { + create_kind(s, p, "line") + }), + spec!("shape.arrow", "Arrow", "{x, y, width (length), parent?, index?, name?, props?, select? (default true)}", true, |s, p| create_kind( + s, p, "arrow" + )), + spec!( + "text.create", + "Text", + "{text | characters, x, y, parent?, index?, name? (default: the first line), width? (fixed width wraps), props? (fontSize, fontFamily, …), select? (default true)}", + true, + |s, p| { create_kind(s, p, "text") } + ), spec!( "vector.create", "Vector", - "{path (SVG path data) | points [[x,y],..], closed?, x?, y?, width?, height? (scale the path; one keeps its proportion), parent?, props?}", + "{path (SVG path data) | points [[x,y],..], closed?, x?, y?, width?, height? (scale the path; one keeps its proportion), parent?, index?, name?, props?, select? (default true)}", true, |s, p| create_kind(s, p, "vector") ), spec!( "image.place", "Place image", - "{data (base64) | path, x?, y?, width?, height?, parent?, target? (an existing layer: the image is added on top of its fills instead of making a layer), mode? (fill|fit|crop|tile)} → {id, image, width, height, imageProfile (its embedded colour profile; null: taken as sRGB)}", + "{data (base64) | path, x?, y?, width?, height?, parent?, name? (default Image), target? (an existing layer: the image is added on top of its fills instead of making a layer), mode? (fill|fit|crop|tile)} → {id, image, width, height, imageProfile (its embedded colour profile; null: taken as sRGB)}", true, place_image ), diff --git a/crates/engine/src/cmd/edit.rs b/crates/engine/src/cmd/edit.rs index d24600c9..886b9516 100644 --- a/crates/engine/src/cmd/edit.rs +++ b/crates/engine/src/cmd/edit.rs @@ -248,8 +248,8 @@ pub fn specs() -> Vec { s.doc_mut()?.reorder(i, idx)?; ok() }), - spec!("fill.add", "Add fill", "{ids?, paint? (default #D9D9D9 or white for frames)}", true, |s, p| paint_list(s, p, true, true)), - spec!("stroke.add", "Add stroke", "{ids?, paint? (default black)}", true, |s, p| paint_list(s, p, false, true)), + spec!("fill.add", "Add fill", "{ids?, paint? | color? (default #D9D9D9 or white for frames)}", true, |s, p| paint_list(s, p, true, true)), + spec!("stroke.add", "Add stroke", "{ids?, paint? | color? (default black)}", true, |s, p| paint_list(s, p, false, true)), spec!("fill.remove", "Remove fill", "{ids?, index}", true, |s, p| paint_list(s, p, true, false)), spec!("stroke.remove", "Remove stroke", "{ids?, index}", true, |s, p| paint_list(s, p, false, false)), spec!("fill.set", "Set fill", "{ids?, index?, paint | color, opacity?, visible?, blend?}", true, |s, p| paint_set(s, p, true)), @@ -273,7 +273,7 @@ pub fn specs() -> Vec { spec!( "fill.pattern", "Pattern fill", - "{source (layer id), ids?, index? (default the top fill), tile? (grid|horizontalHex|verticalHex), scale? (1 = the source's size), spacing? [x, y], align? (start|center|end), opacity?} the source layer tiled; it updates as the source changes", + "{source (layer id), ids?, index? (default the top fill), tile? (grid|horizontalHex|verticalHex), scale? (1 = the source's size), spacing? [x, y], align? (start|center|end), opacity?, visible?, blend?} the source layer tiled; it updates as the source changes", true, pattern_fill ), @@ -354,7 +354,7 @@ pub fn specs() -> Vec { spec!( "export.update", "Edit export setting", - "{ids?, index, size? (2x|512w|512h|{by, value}), suffix?, format? (png|jpg|svg|pdf|webp|xml: Android Vector Drawable), bleed? (PDF: points, or with a unit: \"3mm\", \"0.125in\", \"9pt\"), cropMarks?, bleedMarks?, cmyk? (PDF print options), vdCompat? (XML: bestEffort|exact|ignore)}", + "{ids?, index, size? (2x|512w|512h|{by, value}), suffix?, format? (png|jpg|svg|pdf|webp|xml: Android Vector Drawable), bleed? (PDF: points, or with a unit: \"3mm\", \"0.125in\", \"9pt\"), cropMarks?, bleedMarks?, cmyk? (PDF print options), vdCompat? (XML: bestEffort|exact|ignore), contentsOnly?, includeBoundingBox?, svgOutlineText?, svgIdAttribute?, svgSimplifyStroke?}", true, |s, p| { let cmd = "export.update"; @@ -387,6 +387,7 @@ pub fn specs() -> Vec { for (i, c) in a.iter().enumerate() { let cmd = c.get("command").and_then(Value::as_str).ok_or_else(|| bad("command.batch", format!("step {i} has no `command`")))?; + crate::batch::refuse_unknown_step_keys(i, c)?; let params = c.get("params").cloned().unwrap_or(json!({})); out.push(BatchStep { name: crate::batch::step_name(i, c)?, ..BatchStep::new(cmd, params) }); } diff --git a/crates/engine/src/cmd/file.rs b/crates/engine/src/cmd/file.rs index 576452c5..30d2ceac 100644 --- a/crates/engine/src/cmd/file.rs +++ b/crates/engine/src/cmd/file.rs @@ -110,7 +110,7 @@ pub fn specs() -> Vec { spec!( "file.export", "Export", - "{ids? (default selection, else the page), format? (png|jpg|svg|pdf|webp|xml (an Android Vector Drawable, with `warnings` for what it changed or left out), or fig: the whole file as .fig, experimental, with `warnings` for what was left out; other apps may not open every file), scale? (default 1) | size? (2x|512w|512h or {by: scale|width|height, value}: a scale, or a width or height in pixels), path? (else returns base64 `data`), bleed? (PDF: points, or with a unit: \"3mm\", \"0.125in\", \"9pt\"), cropMarks?, bleedMarks?, cmyk? (PDF print options), svgIdAttribute? (SVG ids, XML android:name), vdCompat? (XML: bestEffort (default: approximate with a warning)|exact (leave out what can't be exact, with a warning)|ignore (no warnings))}", + "{ids? (default selection, else the page), format? (png|jpg|svg|pdf|webp|xml (an Android Vector Drawable, with `warnings` for what it changed or left out), or fig: the whole file as .fig, experimental, with `warnings` for what was left out; other apps may not open every file), scale? (default 1) | size? (2x|512w|512h or {by: scale|width|height, value}: a scale, or a width or height in pixels), path? (else returns base64 `data`), bleed? (PDF: points, or with a unit: \"3mm\", \"0.125in\", \"9pt\"), cropMarks?, bleedMarks?, cmyk? (PDF print options), contentsOnly? (default true; false also draws the layers that show in their area), includeBoundingBox? (true: only the layers' own boxes, cutting off shadows and outside strokes), svgOutlineText? (default true), svgSimplifyStroke? (default true), svgIdAttribute? (SVG ids, XML android:name), vdCompat? (XML: bestEffort (default: approximate with a warning)|exact (leave out what can't be exact, with a warning)|ignore (no warnings))}", false, export ), @@ -121,13 +121,19 @@ pub fn specs() -> Vec { false, export_layers ), - spec!("file.exportSvg", "Export selection as SVG", "{ids?, path?}", false, |s, p| { - let mut q = p.clone(); - if let Some(o) = q.as_object_mut() { - o.insert("format".into(), json!("svg")); + spec!( + "file.exportSvg", + "Export selection as SVG", + "{ids?, path?, scale? | size?, contentsOnly?, includeBoundingBox?, svgOutlineText?, svgIdAttribute?, svgSimplifyStroke? (file.export's SVG options)}", + false, + |s, p| { + let mut q = p.clone(); + if let Some(o) = q.as_object_mut() { + o.insert("format".into(), json!("svg")); + } + export(s, &q) } - export(s, &q) - }), + ), spec!( "file.exportFramesPdf", "Export frames to PDF", @@ -142,7 +148,13 @@ pub fn specs() -> Vec { true, import ), - spec!("file.placeImage", "Place image", "{path | data, x?, y?}", true, super::create::place_image), + spec!( + "file.placeImage", + "Place image", + "{path | data, x?, y?, width?, height?, parent?, name?, target?, mode? (as image.place)}", + true, + super::create::place_image + ), spec!("file.close", "Close tab", "{index?, force? (discard unsaved changes)}", false, close), spec!("file.moveTab", "Move tab", "{from, to} (indices among open documents)", false, |s, p| { let from = req_index("file.moveTab", p, "from")?; diff --git a/crates/engine/src/cmd/proto.rs b/crates/engine/src/cmd/proto.rs index d0822db9..3f51bb5e 100644 --- a/crates/engine/src/cmd/proto.rs +++ b/crates/engine/src/cmd/proto.rs @@ -16,7 +16,7 @@ pub fn specs() -> Vec { spec!( "proto.interaction.add", "Add interaction", - "{id?, trigger? {type: onClick|onHover|onPress|onDrag|mouseEnter|mouseLeave|mouseDown|mouseUp|afterDelay {ms}|keyPress {key (a key or a combination: Shift+K, Ctrl+Alt+Delete; or a controller button: Gamepad A|B|X|Y|LB|RB|LT|RT|View|Menu|LS|RS|Up|Down|Left|Right|Home)}}, action {type: navigate {preserveScroll?}|back|scrollTo {offset? [x, y] (where the layer ends up from the top-left)}|openOverlay {position?, anchor? screen|triggerInside|triggerOutside (default screen; the trigger ones measure the position preset on the layer that opens the overlay: outside puts top presets above it and bottom presets below it, edges lined up), offset? [x, y] (position manual: the overlay's top-left from the top-left of the layer that opens it; without it, right below that layer), close_on_outside? (default true), background?}|swapOverlay|closeOverlay|openUrl {url}|changeTo|setVariable {variable, value | expression (text, e.g. \"count + 1\")}|setVariableMode {collection, mode}|conditional {blocks [{condition? | expression? (text, e.g. \"count >= 3\"), actions}]}, dest?}, actions? [..], transition? {type: instant|dissolve|smartAnimate|moveIn|moveOut|push|slideIn|slideOut, direction?: left|right|top|bottom}, durationMs?, easing? linear|easeIn|easeOut|easeInOut|easeInBack|easeOutBack|easeInOutBack|gentle|quick|bouncy|slow|{customBezier {x1, y1, x2, y2}}|{customSpring {mass, stiffness, damping}}}, delayMs? (mouse enter, leave, down and up: how long after the gesture the actions run, up to 600000)} — or {id, dest} for a click-navigate. Returns {index}", + "{id?, trigger? {type: onClick|onHover|onPress|onDrag|mouseEnter|mouseLeave|mouseDown|mouseUp|afterDelay {ms}|keyPress {key (a key or a combination: Shift+K, Ctrl+Alt+Delete; or a controller button: Gamepad A|B|X|Y|LB|RB|LT|RT|View|Menu|LS|RS|Up|Down|Left|Right|Home)}}, action {type: navigate {preserveScroll?}|back|scrollTo {offset? [x, y] (where the layer ends up from the top-left)}|openOverlay {position?, anchor? screen|triggerInside|triggerOutside (default screen; the trigger ones measure the position preset on the layer that opens the overlay: outside puts top presets above it and bottom presets below it, edges lined up), offset? [x, y] (position manual: the overlay's top-left from the top-left of the layer that opens it; without it, right below that layer), close_on_outside? (default true), background?}|swapOverlay|closeOverlay|openUrl {url}|changeTo|setVariable {variable, value | expression (text, e.g. \"count + 1\")}|setVariableMode {collection, mode}|conditional {blocks [{condition? | expression? (text, e.g. \"count >= 3\"), actions}]}, dest?}, actions? [..], dest? (without action: a click-navigate to it), transition? {type: instant|dissolve|smartAnimate|moveIn|moveOut|push|slideIn|slideOut, direction?: left|right|top|bottom}, durationMs? | duration_ms?, easing? linear|easeIn|easeOut|easeInOut|easeInBack|easeOutBack|easeInOutBack|gentle|quick|bouncy|slow|{customBezier {x1, y1, x2, y2}}|{customSpring {mass, stiffness, damping}}, delayMs? | delay_ms? (mouse enter, leave, down and up: how long after the gesture the actions run, up to 600000)} — or {id, dest} for a click-navigate. Returns {index}", true, add ), @@ -51,7 +51,7 @@ pub fn specs() -> Vec { spec!( "proto.interaction.set", "Edit interaction", - "{id?, index, interaction {..}} replaces it; or patch fields: {id?, index, trigger?, action? (first action), actions?, transition?, durationMs?, easing?, delayMs?}", + "{id?, index, interaction? {..} (replaces it whole), trigger?, action? (first action), actions?, transition?, durationMs? | duration_ms?, easing?, delayMs? | delay_ms?} patch fields on top of the current interaction (or of interaction)", true, set ), diff --git a/crates/engine/src/cmd/structure.rs b/crates/engine/src/cmd/structure.rs index 5bfb088a..6b203c59 100644 --- a/crates/engine/src/cmd/structure.rs +++ b/crates/engine/src/cmd/structure.rs @@ -83,7 +83,7 @@ pub fn specs() -> Vec { } ok() }), - spec!("layout.suggest", "Suggest auto layout", "{ids?}", true, layout_add), + spec!("layout.suggest", "Suggest auto layout", "{ids?, direction? (horizontal|vertical|grid)}", true, layout_add), spec!("object.moveToPage", "Move to page", "{ids?, page}", true, move_to_page), spec!("object.setThumbnail", "Set as thumbnail", "{id?}", true, |s, p| { let id = targets(s, p).first().copied().ok_or_else(|| bad("object.setThumbnail", "select a frame"))?; diff --git a/crates/engine/src/cmd/styles.rs b/crates/engine/src/cmd/styles.rs index 0066602a..b99abc2b 100644 --- a/crates/engine/src/cmd/styles.rs +++ b/crates/engine/src/cmd/styles.rs @@ -164,7 +164,7 @@ pub fn specs() -> Vec { spec!( "variable.create", "Create variable", - "{collection, name, type: color|number|string|boolean, value (default mode) | values {modeName: value}}", + "{collection, name, type: color|number|string|boolean, value (default mode) | values {modeName: value}, description?}", true, var_create ), diff --git a/crates/engine/src/cmd/text.rs b/crates/engine/src/cmd/text.rs index 5f00fbe7..ec219db3 100644 --- a/crates/engine/src/cmd/text.rs +++ b/crates/engine/src/cmd/text.rs @@ -279,13 +279,19 @@ pub fn specs() -> Vec { .ok_or_else(|| bad("text.decoration", "`decoration`: none|underline|strikethrough"))?; style(s, p, |st| st.decoration = d) }), - spec!("text.listStyle", "List style", "{style: none|bulleted|numbered, ids?} while editing: the selected paragraphs", true, |s, p| { - let l: ListStyle = get(p, "style") - .or_else(|| get(p, "list")) - .and_then(super::props::enum_of) - .ok_or_else(|| bad("text.listStyle", "`style`: none|bulleted|numbered"))?; - set_list(s, p, l, false) - }), + spec!( + "text.listStyle", + "List style", + "{style | list: none|bulleted|numbered, ids?} while editing: the selected paragraphs", + true, + |s, p| { + let l: ListStyle = get(p, "style") + .or_else(|| get(p, "list")) + .and_then(super::props::enum_of) + .ok_or_else(|| bad("text.listStyle", "`style`: none|bulleted|numbered"))?; + set_list(s, p, l, false) + } + ), spec!("text.paragraphIndent", "Paragraph indent", "{indent (px), ids?}", true, |s, p| { let v = finite(req_num("text.paragraphIndent", p, "indent")?).clamp(0.0, 10_000.0); para(s, p, |t| t.paragraph_indent = v) @@ -374,7 +380,7 @@ pub fn specs() -> Vec { spec!("text.endPreview", "End text preview", "{} puts the document back as it was before text.preview → {ended}", false, |s, _| { Ok(json!({"ended": crate::textpreview::end(s)})) }), - spec!("text.set", "Set text", "{id?, characters}", true, |s, p| { + spec!("text.set", "Set text", "{id?, characters | text}", true, |s, p| { let c = string(p, "characters").or_else(|| string(p, "text")).ok_or_else(|| bad("text.set", "`characters` required"))?; let ids = req_targets("text.set", s, p)?; let doc = s.doc_mut()?; @@ -390,7 +396,7 @@ pub fn specs() -> Vec { spec!( "text.style", "Style text range", - "{id?, start?, end?, fontFamily?, fontStyle?, fontWeight?, fontSize?, lineHeight?, letterSpacing?, textDecoration?, textCase?, hyperlink?, textFills?}", + "{id?, start?, end?, fontFamily?, fontStyle?, fontWeight?, fontVariations? | variationSettings?, fontSize?, lineHeight?, letterSpacing?, textDecoration?, textDecorationStyle?, textDecorationThickness?, textDecorationOffset?, textDecorationSkipInk?, textDecorationColor?, textCase?, hyperlink?, textFills?}", true, |s, p| { let ids = req_targets("text.style", s, p)?; diff --git a/crates/engine/src/cmd/view.rs b/crates/engine/src/cmd/view.rs index 97b947a5..f8cde2e3 100644 --- a/crates/engine/src/cmd/view.rs +++ b/crates/engine/src/cmd/view.rs @@ -373,7 +373,7 @@ pub fn specs() -> Vec { spec!( "pref.uiScale", "Interface size", - "{scale?: a size from 0.75 to 3 relative to the system's display scale, 1 being the system's own size, so 1.25 is 125 %, not 125 (outside the range it stops at the nearest end; \"auto\" is 1; omit: just read it)} → {scale}: how large the whole interface is drawn; the canvas zoom is separate", + "{scale? (a size from 0.75 to 3 relative to the system's display scale: 1 is the system's own size, so 1.25 is 125 %, not 125; outside the range it stops at the nearest end; \"auto\" is 1; omit: just read it)} → {scale}: how large the whole interface is drawn; the canvas zoom is separate", false, |s, p| { match get(p, "scale") { diff --git a/crates/engine/src/dev/annotations.rs b/crates/engine/src/dev/annotations.rs index 0b27d03e..45b867ab 100644 --- a/crates/engine/src/dev/annotations.rs +++ b/crates/engine/src/dev/annotations.rs @@ -247,7 +247,7 @@ pub fn specs() -> Vec { spec!( "annotation.add", "Add annotation", - "{id? (default the first selected layer), label? (the note), properties?: [width|height|top|left|rotation|opacity|cornerRadius|direction|gap|padding|alignment|fontFamily|fontStyle|fontSize|lineHeight|letterSpacing|textAlign|fill|stroke|strokeWeight|effects], category? (id or name)} → {id, index} (a note, pinned properties or both)", + "{id? (default the first selected layer), label? | text? (the note), properties?: [width|height|top|left|rotation|opacity|cornerRadius|direction|gap|padding|alignment|fontFamily|fontStyle|fontSize|lineHeight|letterSpacing|textAlign|fill|stroke|strokeWeight|effects], category? (id or name)} → {id, index} (a note, pinned properties or both)", true, |s, p| { let cmd = "annotation.add"; @@ -270,7 +270,7 @@ pub fn specs() -> Vec { spec!( "annotation.edit", "Edit annotation", - "{id?, index, label?, properties?: [..] (replaces the pinned properties), category? (id or name; null: none)}", + "{id?, index, label? | text?, properties?: [..] (replaces the pinned properties), category? (id or name; null: none)}", true, |s, p| { let cmd = "annotation.edit"; diff --git a/crates/engine/src/history_behaviour_tests.rs b/crates/engine/src/history_behaviour_tests.rs index 45e0c7bf..8cd5612e 100644 --- a/crates/engine/src/history_behaviour_tests.rs +++ b/crates/engine/src/history_behaviour_tests.rs @@ -188,7 +188,7 @@ fn threads_replies_and_resolving() { |c| c.replies.iter().map(|m| m.text.as_str()).eq(["Sure", "Done"]), "replies stay in the order they were written", ), - ("comment.resolve", json!({"id": c, "author": "Sam"}), |c| c.resolved, "anyone in the file can resolve a thread, not just its author"), + ("comment.resolve", json!({"id": c}), |c| c.resolved, "anyone in the file can resolve a thread, not just its author (it takes no author)"), ("comment.resolve", json!({"id": c, "resolved": false}), |c| !c.resolved, "a resolved thread can be reopened"), ("comment.resolve", json!({"id": c}), |c| c.resolved, "resolving again marks it resolved"), ( diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 0db81fd1..6e794066 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -495,6 +495,8 @@ mod tiff_tests; #[cfg(test)] mod transform_behaviour_tests; #[cfg(test)] +mod unknown_params_tests; +#[cfg(test)] mod variable_tests; #[cfg(test)] mod variables_behaviour_tests; diff --git a/crates/engine/src/live_tests.rs b/crates/engine/src/live_tests.rs index 5cdccf69..1bae1b10 100644 --- a/crates/engine/src/live_tests.rs +++ b/crates/engine/src/live_tests.rs @@ -158,8 +158,8 @@ fn viewport(page: NodeId, x: f64, y: f64, w: f64, h: f64) -> Presence { #[test] fn without_a_link_live_sessions_are_unavailable() { let mut s = Session::new(); - for cmd in ["collab.host", "collab.join"] { - let e = s.execute(cmd, &json!({"link": "figcraft://join?host=a:1&file=f&token=t"})).unwrap_err(); + for (cmd, p) in [("collab.host", json!({})), ("collab.join", json!({"link": "figcraft://join?host=a:1&file=f&token=t"}))] { + let e = s.execute(cmd, &p).unwrap_err(); assert!(matches!(e, EngineError::Disabled(..)), "{cmd}: {e}"); } assert_eq!(s.execute("collab.leave", &json!({})).unwrap()["ended"], false); diff --git a/crates/engine/src/session/exec.rs b/crates/engine/src/session/exec.rs index 44af2a05..a9d491e2 100644 --- a/crates/engine/src/session/exec.rs +++ b/crates/engine/src/session/exec.rs @@ -49,6 +49,7 @@ impl Session { /// as it was. pub fn execute(&mut self, id: &str, params: &Value) -> Result { let spec = find_command(id).ok_or_else(|| EngineError::UnknownCommand(id.to_string()))?; + crate::cmd::refuse_unknown_params(id, params)?; board::refuse_on_board(self, id)?; // Revert replaces the document's uid: refuse before ending previews or syncing branches. if id == "file.revert" && self.live().doc_uid.is_some() { diff --git a/crates/engine/src/sharing/live/join.rs b/crates/engine/src/sharing/live/join.rs index e08afcc6..1742382a 100644 --- a/crates/engine/src/sharing/live/join.rs +++ b/crates/engine/src/sharing/live/join.rs @@ -168,7 +168,7 @@ pub fn specs() -> Vec { spec!( "collab.share", "Share with a join code", - "{via?: \"online\" (default: the open file is kept on the server in Preferences, `pref.publishServer`, and the server hands out the codes) | \"local\" (this app hosts it for people on the same network; no server), access?: \"edit\" (default) | \"view\" (online only: which code `code` is), server? (online: another server's address, with ?key=… when it needs its key), name? (online: the file's name there), port?, bind?: \"lan\" (default) | \"local\" (this machine only) | an IP address (local)} → the open file's session as `collab.peers` gives it: `code` is what others type to join (`collab.join {code}`, the Join field), `link` an invite link. Local: at once, the code carries the host's IPv4 address, port and secret (K7M2Q-…, 20 symbols). Online: status \"connecting\" first; once live `code` is the server's (K7M-2QX-9TD) and `share` is {via, access, shared, editCode, viewCode, editLink, viewLink}: links made for others (the file's own links in `published` stay yours); `code` stays null when the server is older than join codes (give `share.editLink` / `share.viewLink` instead). Called again while shared: only switches which code `code` is (`access`). Called after `collab.stop` on a file that is still on the server: new links and codes for that same file (nothing is published twice). `collab.stop` stops sharing", + "{via?: \"online\" (default: the open file is kept on the server in Preferences, `pref.publishServer`, and the server hands out the codes) | \"local\" (this app hosts it for people on the same network; no server), access?: \"edit\" (default) | \"view\" (online only: which code `code` is), server? (online: another server's address, with ?key=… when it needs its key), key? (online: the server's key, when the address has none), name? (online: the file's name there), port?, bind?: \"lan\" (default) | \"local\" (this machine only) | an IP address (local)} → the open file's session as `collab.peers` gives it: `code` is what others type to join (`collab.join {code}`, the Join field), `link` an invite link. Local: at once, the code carries the host's IPv4 address, port and secret (K7M2Q-…, 20 symbols). Online: status \"connecting\" first; once live `code` is the server's (K7M-2QX-9TD) and `share` is {via, access, shared, editCode, viewCode, editLink, viewLink}: links made for others (the file's own links in `published` stay yours); `code` stays null when the server is older than join codes (give `share.editLink` / `share.viewLink` instead). Called again while shared: only switches which code `code` is (`access`). Called after `collab.stop` on a file that is still on the server: new links and codes for that same file (nothing is published twice). `collab.stop` stops sharing", false, share ), @@ -182,7 +182,7 @@ pub fn specs() -> Vec { spec!( "collab.join", "Join live session", - "{code: a join code (K7M-2QX-9TD from a server, asked at the server in Preferences; a longer one for the local network; spaces, dashes and case don't matter, O reads as 0 and I or L as 1) | link: figcraft://join?host=…&file=…&token=…, figcraft://join?server=ws(s)://…&file=…&token=… or ws(s)://host:port/?file=…&token=… (a session an app hosts, or a server's share link; a code here works too)} → the new session: {session, status: \"connecting\", …}; its file opens in a tab of its own (other files' sessions go on) and `collab.peers` tells when it's live (`sessions` lists it until then, and if it fails)", + "{code | link (a join code: K7M-2QX-9TD from a server, asked at the server in Preferences; a longer one for the local network; spaces, dashes and case don't matter, O reads as 0 and I or L as 1; or an invite link: figcraft://join?host=…&file=…&token=…, figcraft://join?server=ws(s)://…&file=…&token=… or ws(s)://host:port/?file=…&token=…, from a session an app hosts or a server's share link; either key takes either)} → the new session: {session, status: \"connecting\", …}; its file opens in a tab of its own (other files' sessions go on) and `collab.peers` tells when it's live (`sessions` lists it until then, and if it fails)", false, join ), @@ -210,7 +210,7 @@ pub fn specs() -> Vec { spec!( "collab.manageFile", "Manage published file", - "{file (its id), server?, owner? (the server's address and the owner token you got when you published the file without the key; each defaults to what is recorded for one of your online files, see `collab.files`), action: links | createLink {access?: view (default) | edit, label?} | revoke {link (a link's id)} | share (others get new links and join codes) | unshare (the links and codes others got are revoked; the file stays yours on the server) | rename {name} | remove (the file is deleted from the server and leaves your online files)} → {links: [{id, access, label, created}]} | {id, access, link (a new share link, shown once)} | {shared, code, viewCode, editLink, viewLink} | {done}", + "{file (its id), server?, owner? (the server's address and the owner token you got when you published the file without the key; each defaults to what is recorded for one of your online files, see `collab.files`), action: links | createLink | revoke | share (others get new links and join codes) | unshare (the links and codes others got are revoked; the file stays yours on the server) | rename | remove (the file is deleted from the server and leaves your online files), access? (createLink: view (default) | edit), label? (createLink), link? (revoke: a link's id), name? (rename)} → {links: [{id, access, label, created}]} | {id, access, link (a new share link, shown once)} | {shared, code, viewCode, editLink, viewLink} | {done}", false, manage_file ), diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index b330b8e4..65376f70 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -478,11 +478,19 @@ fn every_command_survives_junk_params() { continue; } for j in &junk { - let mut s = sess(); - let _ = s.execute("shape.rectangle", &json!({"x": 0, "y": 0, "width": 10, "height": 10})); - let _ = s.execute("text.create", &json!({"text": "t"})); - if let Err(EngineError::Internal { cmd, msg }) = s.execute(spec.id, j) { - panic!("{cmd} panicked on {j}: {msg}"); + // Each junk object as given (names the command doesn't take are refused) and cut down + // to the names it takes, so the junk values still reach its parsers. + let mut taken = j.clone(); + if let (Some(names), Some(o)) = (crate::cmd::param_names(spec.id), taken.as_object_mut()) { + o.retain(|k, _| matches!(k.as_str(), "ids" | "id") || names.contains(&k.as_str())); + } + for params in if taken == *j { vec![j] } else { vec![j, &taken] } { + let mut s = sess(); + let _ = s.execute("shape.rectangle", &json!({"x": 0, "y": 0, "width": 10, "height": 10})); + let _ = s.execute("text.create", &json!({"text": "t"})); + if let Err(EngineError::Internal { cmd, msg }) = s.execute(spec.id, params) { + panic!("{cmd} panicked on {params}: {msg}"); + } } } } diff --git a/crates/engine/src/unknown_params_tests.rs b/crates/engine/src/unknown_params_tests.rs new file mode 100644 index 00000000..f0773f25 --- /dev/null +++ b/crates/engine/src/unknown_params_tests.rs @@ -0,0 +1,91 @@ +//! A parameter name a command doesn't take is refused, not ignored: a misspelt name would make the +//! command answer as if it hadn't been given (#1413). + +use serde_json::{Value, json}; + +use crate::Session; +use crate::cmd::{all_commands, param_names}; + +fn err(s: &mut Session, cmd: &str, params: Value) -> String { + match s.execute(cmd, ¶ms) { + Err(e) => e.to_string(), + Ok(v) => panic!("{cmd} {params} ran: {v}"), + } +} + +/// The issue's repro: a misspelt `filter` listed the resolved thread as if no filter were given. +#[test] +fn a_misspelt_parameter_is_refused_and_named() { + let mut s = Session::new(); + s.execute("comment.add", &json!({"x": 10, "y": 10, "text": "a"})).unwrap(); + s.execute("comment.resolve", &json!({"id": 1})).unwrap(); + let e = err(&mut s, "comment.list", json!({"filterr": "open"})); + assert!(e.contains("unknown parameter `filterr`") && e.contains("it takes page, filter, sort"), "{e}"); + assert_eq!(s.execute("comment.list", &json!({"filter": "open"})).unwrap()["comments"].as_array().map(Vec::len), Some(0)); + let e = err(&mut s, "edit.undo", json!({"steps": 2, "stepz": 1})); + assert!(e.contains("unknown parameters `steps`, `stepz` (it takes none)"), "{e}"); +} + +/// `ids` and `id` name the layers a command acts on, documented or not (`params::targets` reads +/// either), so `{id}` keeps working where a doc only says `ids?`. +#[test] +fn ids_and_id_are_always_taken() { + let mut s = Session::new(); + let r = crate::params::id_of(&s.execute("shape.rectangle", &json!({"x": 0, "y": 0, "width": 10, "height": 10})).unwrap()["id"]).unwrap(); + assert_eq!(param_names("align.left"), Some(&["ids"][..])); + s.execute("align.left", &json!({"id": r})).unwrap(); + s.execute("edit.undo", &json!({"ids": []})).unwrap(); +} + +/// Names a doc gives as alternatives (`id? | ids?`, `id?/ids?`, `path | data`) are all taken, and +/// `null` params or no params at all stay fine. +#[test] +fn documented_alternatives_and_empty_params_are_taken() { + assert_eq!(param_names("vector.fillRule").map(|n| n.contains(&"id") && n.contains(&"ids")), Some(true)); + assert_eq!(param_names("text.setFeatures").map(|n| n.contains(&"id") && n.contains(&"ids")), Some(true)); + assert_eq!(param_names("file.import").map(|n| n.contains(&"path") && n.contains(&"data") && n.contains(&"name")), Some(true)); + assert_eq!(param_names("file.export").map(|n| n.contains(&"scale") && n.contains(&"size")), Some(true)); + assert_eq!(param_names("collab.share").map(|n| n.contains(&"via") && !n.contains(&"local")), Some(true)); + assert_eq!(param_names("component.editProperty").map(|n| n.contains(&"value") && n.contains(&"newValue")), Some(true)); + // Docs whose shape used to hide names: a stray brace, a comma in an example, a second object. + assert_eq!(param_names("proto.interaction.add").map(|n| n.contains(&"delayMs") && n.contains(&"dest")), Some(true)); + assert_eq!(param_names("collab.join"), Some(&["code", "link"][..])); + assert_eq!(param_names("proto.interaction.set").map(|n| n.contains(&"trigger") && n.contains(&"interaction")), Some(true)); + // Commands that run another command's code take its names too. + assert_eq!(param_names("file.placeImage").map(|n| ["width", "height", "target", "mode"].iter().all(|k| n.contains(k))), Some(true)); + assert_eq!(param_names("file.exportSvg").map(|n| ["scale", "size", "contentsOnly", "svgOutlineText"].iter().all(|k| n.contains(k))), Some(true)); + // Alternative groups: `{x, y} | {points: …}`. + assert_eq!(param_names("vector.edit.paint"), Some(&["x", "y", "points"][..])); + assert_eq!(param_names("vector.edit.cut"), Some(&["segment", "t", "vertex", "from", "to"][..])); + // `node.set` documents its props and checks them itself. + assert_eq!(param_names("node.set"), None); + let mut s = Session::new(); + for p in [json!(null), json!({})] { + s.execute("select.all", &p).unwrap(); + } +} + +/// Every top-level item of every `{…}` params doc gives a name, so no parameter a doc describes is +/// refused. +#[test] +fn every_doc_item_names_a_parameter() { + let nameless: Vec = all_commands() + .iter() + .flat_map(|c| crate::cmd::doc_items(c.params).unwrap_or_default().into_iter().map(move |i| (c.id, i))) + .filter(|(_, i)| !i.trim().is_empty() && crate::cmd::item_names(i).is_empty()) + .map(|(id, i)| format!("{id}: `{i}`")) + .collect(); + assert!(nameless.is_empty(), "doc items without a name: {nameless:#?}"); +} + +/// A batch step key other than `command`, `params` and `as` (`args` for `params`) is refused, and +/// so is a step's unknown parameter, naming the step. +#[test] +fn batch_steps_refuse_unknown_keys_and_parameters() { + let mut s = Session::new(); + let e = err(&mut s, "command.batch", json!({"commands": [{"command": "select.all"}, {"command": "inspect.code", "args": {"id": 2}}]})); + assert!(e.contains("step 1: unknown key `args`"), "{e}"); + let e = + err(&mut s, "command.batch", json!({"commands": [{"command": "select.all"}, {"command": "comment.list", "params": {"filterr": "open"}}]})); + assert!(e.contains("comment.list") && e.contains("step 1: unknown parameter `filterr`"), "{e}"); +} diff --git a/crates/mcp/src/tools.rs b/crates/mcp/src/tools.rs index 8b794907..d2bc592c 100644 --- a/crates/mcp/src/tools.rs +++ b/crates/mcp/src/tools.rs @@ -155,6 +155,9 @@ pub fn tool_definitions() -> Vec { "name": string("Layer name"), "text": string("Characters, for type text"), "path": string("SVG path data, for type vector (e.g. \"M0 0 L100 0 L50 80 Z\")"), + "points": {"type": "array", "items": {"type": "array", "items": {"type": "number"}}, "description": "For type vector, instead of path: the points [[x, y], ...]"}, + "closed": boolean("For type vector: close the path"), + "select": boolean("Select the new layer (default true)"), "props": {"type": "object", "description": "Any set_properties keys applied at creation, e.g. {\"fill\":\"#3366FF\",\"cornerRadius\":12,\"fontSize\":24,\"layout\":\"vertical\",\"gap\":8,\"padding\":16}"}, }), &["type"], @@ -289,6 +292,7 @@ pub fn tool_definitions() -> Vec { "shift": boolean("Shift held"), "alt": boolean("Alt/Option held"), "cmd": boolean("Cmd (macOS) / Ctrl held"), + "ctrl": boolean("Ctrl held (the Control key on macOS)"), "pressure": num("Pen pressure 0-1 (the Pencil records it as the stroke's width profile; a drag starts with it)"), "pressureTo": num("Pen pressure 0-1 at a drag's end (interpolated along it)"), }), @@ -301,7 +305,7 @@ pub fn tool_definitions() -> Vec { "Press key", "Press a key or shortcut as if typed on the canvas: a menu shortcut (e.g. {\"key\":\"G\",\"cmd\":true} groups, {\"key\":\"A\",\"alt\":true,\"shift\":false} …), a tool key (R, O, T, F, V…), Escape, Delete, Enter, arrows (nudge; shift = big nudge). While editing text, keys go to the text editor (Backspace, Delete, Left, Right, Home, End, Enter, Escape, SelectAll).", obj( - json!({"key": string("Key name: a letter/digit, Escape, Delete, Backspace, Enter, ArrowLeft…"), "shift": boolean("Shift"), "alt": boolean("Alt/Option"), "cmd": boolean("Cmd/Ctrl")}), + json!({"key": string("Key name: a letter/digit, Escape, Delete, Backspace, Enter, ArrowLeft…"), "shift": boolean("Shift"), "alt": boolean("Alt/Option"), "cmd": boolean("Cmd/Ctrl"), "ctrl": boolean("Ctrl (the Control key on macOS)")}), &["key"], ), false, @@ -434,12 +438,45 @@ fn friendly_params(p: &mut Value) { // ---------- dispatch ---------- +/// Refuse argument names `schema` doesn't list, at the top and in the objects it describes inside +/// (a batch's steps); objects it leaves open (`params`) take any. `path` leads the names in the +/// error (`commands[0].`). +fn refuse_unknown_args(schema: &Value, args: &Value, path: &str) -> Result<(), String> { + if let (Some(props), Some(given)) = (schema.get("properties").and_then(Value::as_object), args.as_object()) { + let unknown: Vec = given.keys().filter(|k| !props.contains_key(*k)).map(|k| format!("`{path}{k}`")).collect(); + if !unknown.is_empty() { + let s = if unknown.len() == 1 { "" } else { "s" }; + let takes: Vec<&str> = props.keys().map(String::as_str).collect(); + let takes = if takes.is_empty() { "none".to_string() } else { takes.join(", ") }; + return Err(format!("unknown argument{s} {} (it takes {takes})", unknown.join(", "))); + } + for (k, v) in given { + if let Some(inner) = props.get(k) { + refuse_unknown_args(inner, v, &format!("{path}{k}."))?; + } + } + } + if let (Some(items), Some(list)) = (schema.get("items"), args.as_array()) { + for (i, v) in list.iter().enumerate() { + refuse_unknown_args(items, v, &format!("{}[{i}].", path.trim_end_matches('.')))?; + } + } + Ok(()) +} + /// Run one tool. Every failure becomes an `isError` result (never a panic). pub fn call_tool(b: &mut dyn Backend, name: &str, args: &Value) -> ToolResult { let a = match args_obj(args) { Ok(a) => a, Err(e) => return ToolResult::error(e), }; + // Built once: the catalogue is the same on every call. + static TOOLS: std::sync::LazyLock> = std::sync::LazyLock::new(tool_definitions); + if let Some(schema) = TOOLS.iter().find(|t| t.get("name").and_then(Value::as_str) == Some(name)).and_then(|t| t.get("inputSchema")) + && let Err(e) = refuse_unknown_args(schema, &Value::Object(a.clone()), "") + { + return ToolResult::error(e); + } let r = match name { "list_commands" => list_commands(b, &a), "run_command" => run_command(b, &a), diff --git a/crates/mcp/tests/it/protocol.rs b/crates/mcp/tests/it/protocol.rs index b4396c36..4dc2672e 100644 --- a/crates/mcp/tests/it/protocol.rs +++ b/crates/mcp/tests/it/protocol.rs @@ -83,6 +83,13 @@ fn tools_call_happy_and_error_paths() { assert!(c.err("pointer", json!({"x": 1})).contains("`y`")); assert!(c.err("type_text", json!({"text": "hi"})).contains("not editing text")); assert!(c.err("open_file", json!({"path": "/definitely/not/here.figcraft"})).contains("here.figcraft")); + // Names a tool or a command doesn't take are refused, not ignored (#1413). + let e = c.err("run_command", json!({"command": "inspect.code", "args": {"id": id, "language": "html"}})); + assert!(e.contains("unknown argument `args` (it takes command, params)"), "{e}"); + let e = c.err("batch", json!({"commands": [{"command": "comment.list", "args": {"filter": "open"}}]})); + assert!(e.contains("unknown argument `commands[0].args`"), "{e}"); + let e = c.err("run_command", json!({"command": "comment.list", "params": {"filterr": "open"}})); + assert!(e.contains("unknown parameter `filterr`"), "{e}"); // Protocol-level errors. let r = c.rpc("tools/call", json!({"name": "no_such_tool", "arguments": {}})); assert_eq!(r["error"]["code"], -32602); @@ -92,6 +99,10 @@ fn tools_call_happy_and_error_paths() { assert_eq!(r["error"]["code"], -32602); // The document is unharmed by all of that. assert_eq!(c.page_children().len(), 1); + // Arguments the tools pass on are in their schemas, so they still go through. + c.ok("create_node", json!({"type": "vector", "points": [[0, 0], [10, 0], [5, 8]], "closed": true, "select": false})); + c.ok("key", json!({"key": "Escape", "ctrl": false})); + c.ok("pointer", json!({"kind": "hover", "x": 1, "y": 1, "ctrl": false})); } #[test] diff --git a/crates/ui-egui/src/gradient_shorthand_tests.rs b/crates/ui-egui/src/gradient_shorthand_tests.rs index db368f8e..0e5a9f0f 100644 --- a/crates/ui-egui/src/gradient_shorthand_tests.rs +++ b/crates/ui-egui/src/gradient_shorthand_tests.rs @@ -14,7 +14,7 @@ fn engine_execute_takes_the_gradient_shorthand() { let r = exec(&mut u, "shape.rectangle", json!({"x": 0, "y": 0, "width": 200, "height": 100})).unwrap()["id"].clone(); let short = json!({"type": "linearGradient", "stops": ["#ff0000", "#0000ff"], "from": [0, 0.5], "to": [1, 0.5]}); exec(&mut u, "node.set", json!({"ids": [r], "fills": [short]})).unwrap(); - let n = exec(&mut u, "node.get", json!({"id": r, "depth": 0})).unwrap(); + let n = exec(&mut u, "node.get", json!({"id": r})).unwrap(); let f = &n["fills"][0]; assert_eq!((f["type"].clone(), f["kind"].clone(), f["width"].clone()), (json!("gradient"), json!("linear"), json!([0.0, 1.5])), "{n}"); assert_eq!(f["stops"].as_array().map(Vec::len), Some(2), "{n}"); diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index ae4245c7..ce957f28 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -202,6 +202,8 @@ mod text_enter_tests; #[cfg(test)] mod titlebar_tests; #[cfg(test)] +mod ui_params_tests; +#[cfg(test)] mod ui_scale_tests; #[cfg(test)] mod variables_export_ui_tests; diff --git a/crates/ui-egui/src/panels/export_panel.rs b/crates/ui-egui/src/panels/export_panel.rs index cec8ebc0..06b34f2c 100644 --- a/crates/ui-egui/src/panels/export_panel.rs +++ b/crates/ui-egui/src/panels/export_panel.rs @@ -247,12 +247,12 @@ pub(crate) fn print_options(ui: &mut Ui, id: Id, p: &PrintSettings) -> Option Value { +pub(crate) fn print_params(p: &PrintSettings) -> Value { json!({"bleed": p.bleed(), "cropMarks": p.crop_marks, "bleedMarks": p.bleed_marks, "cmyk": p.cmyk}) } /// `file.export` params for `setting`: its size, format and options (PDF: its print options). -fn setting_params(setting: &ExportSetting) -> Value { +pub(crate) fn setting_params(setting: &ExportSetting) -> Value { let mut params = serde_json::to_value(setting).unwrap_or_else(|_| json!({})); if let Value::Object(m) = &mut params { m.remove("print"); diff --git a/crates/ui-egui/src/panels/typography.rs b/crates/ui-egui/src/panels/typography.rs index a95af934..299bb57b 100644 --- a/crates/ui-egui/src/panels/typography.rs +++ b/crates/ui-egui/src/panels/typography.rs @@ -158,10 +158,11 @@ fn path_row(app: &mut FigApp, ui: &mut Ui, doc: &Document, sel: &[NodeId], ids: } /// Set character `props` on the selected chars of the text being edited, else on whole layers -/// (through `node.set`, which records overrides inside instances). -fn commit_text(app: &mut FigApp, ids: &[NodeId], range: Option<(NodeId, usize, usize)>, props: Value) { +/// (through `node.set`, which records overrides inside instances). Paragraph spacing is always +/// the whole layer's: `text.style` doesn't take it. +pub(crate) fn commit_text(app: &mut FigApp, ids: &[NodeId], range: Option<(NodeId, usize, usize)>, props: Value) { match range { - Some((_, a, b)) if a != b => { + Some((_, a, b)) if a != b && props.get("paragraphSpacing").is_none() => { app.run("text.style", text_style_params(ids, range, props)); } _ => app.set_props(ids, props), @@ -170,7 +171,7 @@ fn commit_text(app: &mut FigApp, ids: &[NodeId], range: Option<(NodeId, usize, u /// `text.style` params for `props` on the selected chars of the text being edited, else on the /// whole of every layer in `ids` (previews; commits go through [`commit_text`]). -fn text_style_params(ids: &[NodeId], range: Option<(NodeId, usize, usize)>, mut props: Value) -> Value { +pub(crate) fn text_style_params(ids: &[NodeId], range: Option<(NodeId, usize, usize)>, mut props: Value) -> Value { match range { Some((id, a, b)) if a != b => { props["id"] = json!(id); diff --git a/crates/ui-egui/src/ui_params_tests.rs b/crates/ui-egui/src/ui_params_tests.rs new file mode 100644 index 00000000..6ee71dfc --- /dev/null +++ b/crates/ui-egui/src/ui_params_tests.rs @@ -0,0 +1,77 @@ +//! Params the panels build at run time name only what their commands take: a name a command +//! doesn't list is refused (#1413), which would break the control that sent it. + +use figcraft_doc::{ExportFormat, ExportSetting, NodeId, PrintSettings, VdCompat}; +use figcraft_engine::Session; +use serde_json::{Value, json}; + +use crate::{FigApp, NoServices}; + +fn app_with(cmd: &str, params: Value) -> (FigApp, NodeId) { + let mut s = Session::new(); + let id = NodeId(s.execute(cmd, ¶ms).unwrap()["id"].as_u64().unwrap()); + (FigApp::new(s, Box::new(NoServices)), id) +} + +/// The Export section: a setting with every option off its default exports in every format, and +/// its edits (the setting's own keys, the PDF print options) apply. +#[test] +fn export_settings_with_every_option_are_taken() { + let (mut app, id) = app_with("shape.rectangle", json!({"x": 0, "y": 0, "width": 10, "height": 10})); + app.session.execute("export.add", &json!({"ids": [id]})).unwrap(); + let print = PrintSettings { bleed: 3.0, crop_marks: true, bleed_marks: true, cmyk: true }; + for format in ExportFormat::ALL { + let setting = ExportSetting { + format, + print, + contents_only: false, + include_bounding_box: true, + svg_outline_text: false, + svg_id_attribute: true, + svg_simplify_stroke: false, + vd_compat: VdCompat::Exact, + ..ExportSetting::default() + }; + let mut p = crate::export_panel::setting_params(&setting); + p["ids"] = json!([id]); + app.session.execute("file.export", &p).unwrap_or_else(|e| panic!("file.export {p}: {e}")); + p["index"] = json!(0); + app.session.execute("export.update", &p).unwrap_or_else(|e| panic!("export.update {p}: {e}")); + } + let mut p = crate::export_panel::print_params(&print); + p["ids"] = json!([id]); + p["index"] = json!(0); + app.session.execute("export.update", &p).unwrap_or_else(|e| panic!("export.update {p}: {e}")); +} + +/// The Typography section on selected characters: every character property it sets goes through +/// `text.style`, and paragraph spacing (the whole layer's) through `node.set`. +#[test] +fn typography_edits_on_selected_characters_are_taken() { + let (mut app, id) = app_with("text.create", json!({"text": "Typography", "x": 0, "y": 0})); + let range = Some((id, 0, 4)); + for (key, v) in [ + ("fontFamily", json!("Inter")), + ("fontStyle", json!("Bold")), + ("fontSize", json!(20)), + ("lineHeight", json!(24)), + ("letterSpacing", json!(1)), + ("textDecoration", json!("underline")), + ("textDecorationStyle", json!("wavy")), + ("textDecorationThickness", json!(2)), + ("textDecorationOffset", json!(3)), + ("textDecorationSkipInk", json!(false)), + ("textDecorationColor", json!("#FF0000")), + ("textCase", json!("upper")), + ] { + crate::typography::commit_text(&mut app, &[id], range, json!({ key: v })); + assert_eq!(app.ui.last_error, None, "{key}"); + } + // A named instance of a variable font: its style, and the custom axis values cleared. + crate::typography::commit_text(&mut app, &[id], range, json!({"fontStyle": "Regular", "fontVariations": null})); + assert_eq!(app.ui.last_error, None, "fontVariations"); + crate::typography::commit_text(&mut app, &[id], range, json!({"paragraphSpacing": 12})); + assert_eq!(app.ui.last_error, None, "paragraphSpacing"); + let spacing = app.session.doc().unwrap().node(id).and_then(|n| n.kind.text()).map(|t| t.paragraph_spacing); + assert_eq!(spacing, Some(12.0)); +} diff --git a/docs/cli.md b/docs/cli.md index 39863bea..80bab2a4 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -133,7 +133,9 @@ Runs commands in a headless session (a new document, or `--in`), printing one JS ``` The whole script is **one undo step**, and it is **all-or-nothing**, exactly like running the same -commands through `command.batch` (the MCP tool `batch`, the control channel, plugins). A step named +commands through `command.batch` (the MCP tool `batch`, the control channel, plugins). A step key +other than `command`, `params` and `as`, or a parameter the command's doc (`figcraft-cli commands`) +doesn't list, fails the script with an error naming it (`ids` and `id` are always taken). A step named with `"as"` can be used by later steps: `{"$ref": "name"}` in a step's params is the `id` in that step's result (`{"$step": n}` does the same by zero-based position), and `"field"` takes another key or a slash path such as `ids/0`. So a step can target a layer an earlier step of the same script diff --git a/docs/control-protocol.md b/docs/control-protocol.md index 64534074..02388ace 100644 --- a/docs/control-protocol.md +++ b/docs/control-protocol.md @@ -99,7 +99,8 @@ Run any command (the same ids as the menus, shortcuts, Actions palette, CLI and calls never open dialogs: dialog requests are dropped. Returns the command's result (creation commands return `{"id": N}`). Paints (`fills`, `strokes`, `paint`) take a colour, the full paint JSON or the gradient shorthand `{"type":"linearGradient","stops":["#hex","#hex"],"from"?,"to"?}` -(as in `docs/mcp.md`). +(as in `docs/mcp.md`). A parameter the command's doc (`engine.commands`) doesn't list is an error +naming it and the ones it takes, never ignored; `ids` and `id` are always taken. ```json {"id":1,"method":"engine.execute","params":{"command":"shape.rectangle","params":{"x":10,"y":10,"width":100,"height":50}}} diff --git a/docs/mcp.md b/docs/mcp.md index 25fe21fa..aca097c1 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -1894,6 +1894,10 @@ turned off 0°. Tool failures come back as results with `isError: true` and a message; protocol problems (unknown tool, malformed JSON, wrong parameter types) are JSON-RPC errors. Nothing crashes the server. +A name a tool or command doesn't take (`run_command {args}` for `params`, a step key other than +`command`, `params` and `as`, or a parameter its `list_commands` entry doesn't list, such as +`comment.list {filterr}`) is an error that lists the names it does take, never ignored; `ids` and +`id` are taken by every command. ## Resources