From 64e648ae7e009402f65e57e32def53fe8eacea56 Mon Sep 17 00:00:00 2001 From: MortenHusted Date: Wed, 28 Oct 2026 09:06:33 +0100 Subject: [PATCH 1/6] M0.9: command docs list every parameter their commands read Before commands can refuse names their docs don't list (#1413), the docs have to list what the code reads. These are the top-level names the run code (and every helper it hands the params to) reads that its doc left out, all checked against the code; nothing changes behaviour: - creation: node.create (characters, points, closed), frame.create, shape.*, text.create and vector.create (index, name, select), image.place (name) - aliases the code already took: comment.* (id | link), annotation.add and annotation.edit (label | text), text.set (characters | text), text.listStyle (style | list), fill.add and stroke.add (paint | color), proto.interaction.add/set (durationMs | duration_ms, delayMs | delay_ms) - options: file.export and export.update (contentsOnly, includeBoundingBox, svgOutlineText, svgSimplifyStroke), comment.next (frame, resolved), layout.suggest (direction), variable.create (description), vector.edit.widthPoint (side), collab.share (key), collab.manageFile (access, label, link, name), fill.pattern (visible, blend) - docs whose shape hid a name: proto.interaction.add (a stray `}` closed the object before delayMs; dest was only in a trailer), proto.interaction.set (patch fields in a second object), collab.join (a top-level comma inside the link examples), pref.uiScale (commas in the value's description) Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/canvas/vectoredit/width.rs | 2 +- crates/engine/src/cmd/comment.rs | 52 +++++++++++--------- crates/engine/src/cmd/create.rs | 48 +++++++++++++----- crates/engine/src/cmd/edit.rs | 8 +-- crates/engine/src/cmd/file.rs | 2 +- crates/engine/src/cmd/proto.rs | 4 +- crates/engine/src/cmd/structure.rs | 2 +- crates/engine/src/cmd/styles.rs | 2 +- crates/engine/src/cmd/text.rs | 22 ++++++--- crates/engine/src/cmd/view.rs | 2 +- crates/engine/src/dev/annotations.rs | 4 +- crates/engine/src/sharing/live/join.rs | 6 +-- 12 files changed, 95 insertions(+), 59 deletions(-) diff --git a/crates/engine/src/canvas/vectoredit/width.rs b/crates/engine/src/canvas/vectoredit/width.rs index ba51b043..7977965d 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/comment.rs b/crates/engine/src/cmd/comment.rs index d1d360f4..587bd1c6 100644 --- a/crates/engine/src/cmd/comment.rs +++ b/crates/engine/src/cmd/comment.rs @@ -410,7 +410,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()); @@ -420,7 +420,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); @@ -441,7 +441,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)?; @@ -463,7 +463,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) { @@ -474,7 +474,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)?; @@ -489,24 +489,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", @@ -545,7 +551,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); @@ -582,7 +588,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 414a2310..51773629 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 7ec63c69..365e237b 100644 --- a/crates/engine/src/cmd/edit.rs +++ b/crates/engine/src/cmd/edit.rs @@ -245,8 +245,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)), @@ -270,7 +270,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 ), @@ -351,7 +351,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"; diff --git a/crates/engine/src/cmd/file.rs b/crates/engine/src/cmd/file.rs index fed4bff1..2e63b1c9 100644 --- a/crates/engine/src/cmd/file.rs +++ b/crates/engine/src/cmd/file.rs @@ -107,7 +107,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 ), diff --git a/crates/engine/src/cmd/proto.rs b/crates/engine/src/cmd/proto.rs index ce7219f4..32247e82 100644 --- a/crates/engine/src/cmd/proto.rs +++ b/crates/engine/src/cmd/proto.rs @@ -13,7 +13,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 ), @@ -48,7 +48,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 700825f2..58aaa7f9 100644 --- a/crates/engine/src/cmd/structure.rs +++ b/crates/engine/src/cmd/structure.rs @@ -80,7 +80,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 9c2922d2..8f212ea6 100644 --- a/crates/engine/src/cmd/styles.rs +++ b/crates/engine/src/cmd/styles.rs @@ -161,7 +161,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 3822c5c5..4ad26ad4 100644 --- a/crates/engine/src/cmd/text.rs +++ b/crates/engine/src/cmd/text.rs @@ -276,13 +276,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) @@ -371,7 +377,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()?; diff --git a/crates/engine/src/cmd/view.rs b/crates/engine/src/cmd/view.rs index 7364557f..f17cde19 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 e6937d0e..7e4ef921 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/sharing/live/join.rs b/crates/engine/src/sharing/live/join.rs index 972198ef..38101d85 100644 --- a/crates/engine/src/sharing/live/join.rs +++ b/crates/engine/src/sharing/live/join.rs @@ -166,7 +166,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 ), @@ -180,7 +180,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 ), @@ -208,7 +208,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 ), From ff4fe00294bdb53df5dc3c2b80deefede2b9ccdb Mon Sep 17 00:00:00 2001 From: MortenHusted Date: Wed, 28 Oct 2026 09:06:58 +0100 Subject: [PATCH 2/6] M0.9: commands refuse parameter names they don't take (#1413) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A misspelt parameter name was ignored and the command reported success, answering as if the parameter hadn't been given (`comment.list {"filterr": "open"}` listed resolved threads). `Session::execute` now refuses names the command's params doc doesn't list, naming them and what the command takes, before the command runs, so every path into a command (CLI, batches, control channel, MCP, plugins, the UI) gets the check. The names come from the existing docs, parsed once with the registry: each top-level item's first word and the words joined to it by `|`, `/` or `+`, bracketed text and what follows `:`, `=` or `→` being description, alternative groups (`{x, y} | {points}`) included. `ids` and `id` are always taken (`params::targets` reads either); node.set, whose doc is its props list, keeps its own check. The junk sweep runs each junk object as given and cut down to the names a command takes, so the junk values still reach the parsers. Tests that passed names their command never read (comment.resolve's author, collab.host's link, node.get's depth) pass only what it takes. Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/cmd.rs | 112 +++++++++++++++++- crates/engine/src/history_behaviour_tests.rs | 2 +- crates/engine/src/lib.rs | 2 + crates/engine/src/live_tests.rs | 4 +- crates/engine/src/session/exec.rs | 1 + crates/engine/src/tests.rs | 18 ++- crates/engine/src/unknown_params_tests.rs | 85 +++++++++++++ .../ui-egui/src/gradient_shorthand_tests.rs | 2 +- docs/control-protocol.md | 3 +- 9 files changed, 218 insertions(+), 11 deletions(-) create mode 100644 crates/engine/src/unknown_params_tests.rs 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/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 ea135ca1..37fcaf7d 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -484,6 +484,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 add12ac5..90bcb407 100644 --- a/crates/engine/src/session/exec.rs +++ b/crates/engine/src/session/exec.rs @@ -18,6 +18,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/tests.rs b/crates/engine/src/tests.rs index 811724f2..fd64e7ab 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..4124085f --- /dev/null +++ b/crates/engine/src/unknown_params_tests.rs @@ -0,0 +1,85 @@ +//! 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)); + // 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's unknown parameter is refused, naming the step. +#[test] +fn batch_steps_refuse_unknown_parameters() { + let mut s = Session::new(); + 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/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/docs/control-protocol.md b/docs/control-protocol.md index 448a9979..8c03bda5 100644 --- a/docs/control-protocol.md +++ b/docs/control-protocol.md @@ -97,7 +97,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}}} From 27c11858e1aca954705c4f88891f9a45fdef12f8 Mon Sep 17 00:00:00 2001 From: MortenHusted Date: Wed, 28 Oct 2026 09:06:59 +0100 Subject: [PATCH 3/6] M0.9: MCP tools and batch steps refuse names they don't take (#1413) `run_command {"command": "inspect.code", "args": {...}}` ran the command without its params, on the selection. MCP tool arguments are now checked against each tool's input schema (and the objects it describes inside, such as a batch's steps; open objects like `params` take anything), and a step key other than `command`, `params` and `as` is refused in command.batch, figcraft-cli scripts and the MCP batch tool. Co-Authored-By: Claude Opus 5.5 --- apps/figcraft-cli/src/files.rs | 4 +++ crates/engine/src/automation/batch.rs | 12 +++++++++ crates/engine/src/cmd/edit.rs | 1 + crates/engine/src/unknown_params_tests.rs | 7 +++-- crates/mcp/src/tools.rs | 32 +++++++++++++++++++++++ crates/mcp/tests/it/protocol.rs | 7 +++++ docs/cli.md | 4 ++- docs/mcp.md | 4 +++ 8 files changed, 68 insertions(+), 3 deletions(-) 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/cmd/edit.rs b/crates/engine/src/cmd/edit.rs index 365e237b..e1a2e6bd 100644 --- a/crates/engine/src/cmd/edit.rs +++ b/crates/engine/src/cmd/edit.rs @@ -384,6 +384,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/unknown_params_tests.rs b/crates/engine/src/unknown_params_tests.rs index 4124085f..02290ba3 100644 --- a/crates/engine/src/unknown_params_tests.rs +++ b/crates/engine/src/unknown_params_tests.rs @@ -75,10 +75,13 @@ fn every_doc_item_names_a_parameter() { assert!(nameless.is_empty(), "doc items without a name: {nameless:#?}"); } -/// A batch step's unknown parameter is refused, naming the step. +/// 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_parameters() { +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 1b23191e..46cc1a83 100644 --- a/crates/mcp/src/tools.rs +++ b/crates/mcp/src/tools.rs @@ -414,12 +414,44 @@ 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), }; + let tools = 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 617790f4..5cc45209 100644 --- a/crates/mcp/tests/it/protocol.rs +++ b/crates/mcp/tests/it/protocol.rs @@ -75,6 +75,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); 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/mcp.md b/docs/mcp.md index 05dfd1e6..01754735 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -1866,6 +1866,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 From 3567bef862fa9d784a4768a416524711eb867eb7 Mon Sep 17 00:00:00 2001 From: MortenHusted Date: Wed, 28 Oct 2026 09:35:23 +0100 Subject: [PATCH 4/6] #1460: commands and tools that pass their params on list what they pass file.placeImage runs image.place's code and file.exportSvg file.export's, so their docs now list the names those read (width, height, target, mode; scale or size and the export options). MCP's create_node passes points, closed and select to node.create, and pointer and key read ctrl, so their schemas list them too. Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/cmd/file.rs | 26 +++++++++++++++++------ crates/engine/src/unknown_params_tests.rs | 3 +++ crates/mcp/src/tools.rs | 6 +++++- crates/mcp/tests/it/protocol.rs | 4 ++++ 4 files changed, 31 insertions(+), 8 deletions(-) diff --git a/crates/engine/src/cmd/file.rs b/crates/engine/src/cmd/file.rs index 2e63b1c9..c37d49e4 100644 --- a/crates/engine/src/cmd/file.rs +++ b/crates/engine/src/cmd/file.rs @@ -118,13 +118,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? (as file.export)}", + 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", @@ -139,7 +145,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/unknown_params_tests.rs b/crates/engine/src/unknown_params_tests.rs index 02290ba3..f0773f25 100644 --- a/crates/engine/src/unknown_params_tests.rs +++ b/crates/engine/src/unknown_params_tests.rs @@ -51,6 +51,9 @@ fn documented_alternatives_and_empty_params_are_taken() { 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"][..])); diff --git a/crates/mcp/src/tools.rs b/crates/mcp/src/tools.rs index 46cc1a83..b03a0af8 100644 --- a/crates/mcp/src/tools.rs +++ b/crates/mcp/src/tools.rs @@ -154,6 +154,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"], @@ -288,6 +291,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)"), }), @@ -300,7 +304,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, diff --git a/crates/mcp/tests/it/protocol.rs b/crates/mcp/tests/it/protocol.rs index 5cc45209..85401a82 100644 --- a/crates/mcp/tests/it/protocol.rs +++ b/crates/mcp/tests/it/protocol.rs @@ -91,6 +91,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] From 8fadd5d8e5c7387619b7d4980405ac3025691523 Mon Sep 17 00:00:00 2001 From: MortenHusted Date: Wed, 28 Oct 2026 09:46:27 +0100 Subject: [PATCH 5/6] #1460: file.exportSvg's doc says which of file.export's options it takes "(as file.export)" read as all of them, but the PDF and XML options are refused there, so it now says they are file.export's SVG options. Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/cmd/file.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/engine/src/cmd/file.rs b/crates/engine/src/cmd/file.rs index c37d49e4..816890ab 100644 --- a/crates/engine/src/cmd/file.rs +++ b/crates/engine/src/cmd/file.rs @@ -121,7 +121,7 @@ pub fn specs() -> Vec { spec!( "file.exportSvg", "Export selection as SVG", - "{ids?, path?, scale? | size?, contentsOnly?, includeBoundingBox?, svgOutlineText?, svgIdAttribute?, svgSimplifyStroke? (as file.export)}", + "{ids?, path?, scale? | size?, contentsOnly?, includeBoundingBox?, svgOutlineText?, svgIdAttribute?, svgSimplifyStroke? (file.export's SVG options)}", false, |s, p| { let mut q = p.clone(); From fd81433a23d7e7546244939fbf2f43fc4f43c264 Mon Sep 17 00:00:00 2001 From: bflatastic Date: Thu, 29 Oct 2026 11:09:09 +0700 Subject: [PATCH 6/6] #1460: text.style lists the text-decoration and variation keys the Typography panel sends, paragraph spacing on selected characters goes to node.set, MCP builds its tool catalogue once, and a test for the params panels build --- crates/engine/src/cmd/text.rs | 2 +- crates/mcp/src/tools.rs | 5 +- crates/ui-egui/src/lib.rs | 2 + crates/ui-egui/src/panels/export_panel.rs | 4 +- crates/ui-egui/src/panels/typography.rs | 9 +-- crates/ui-egui/src/ui_params_tests.rs | 77 +++++++++++++++++++++++ 6 files changed, 90 insertions(+), 9 deletions(-) create mode 100644 crates/ui-egui/src/ui_params_tests.rs diff --git a/crates/engine/src/cmd/text.rs b/crates/engine/src/cmd/text.rs index 35edfed9..b6b61841 100644 --- a/crates/engine/src/cmd/text.rs +++ b/crates/engine/src/cmd/text.rs @@ -396,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/mcp/src/tools.rs b/crates/mcp/src/tools.rs index b03a0af8..13f02972 100644 --- a/crates/mcp/src/tools.rs +++ b/crates/mcp/src/tools.rs @@ -450,8 +450,9 @@ pub fn call_tool(b: &mut dyn Backend, name: &str, args: &Value) -> ToolResult { Ok(a) => a, Err(e) => return ToolResult::error(e), }; - let tools = 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")) + // 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); diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index ef55faf8..ec4a61ab 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -195,6 +195,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)); +}