diff --git a/ROADMAP.md b/ROADMAP.md index 8a0f82071..2b2fe3383 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -129,6 +129,7 @@ list) · [roadmap.md](docs/roadmap.md) (milestones, plan) · [parity-checklist.m Newest first. +- **2026-10-13** · Commands refuse parameter keys their params description doesn't name (#776): the control channel, MCP (`-32602`, every `batch` step checked before the first runs) and `designcraft-cli --cmd` answer with an “unknown parameter” error naming the nearest or the accepted keys instead of running with defaults; journals still replay. Descriptions of the table, anchored-object, data-merge and placing commands now name every key they read. No readiness percentage change. - **2026-10-12** · macOS Dock Quit uses the same unsaved-document prompts as window and menu Quit. Repeated requests keep the current answers; Cancel retains every tab and unsaved recovery checkpoint, and completed Save/Don't Save answers exit through the ordinary event loop. No readiness percentage change. - **2026-10-12** · Orderly native exit writes the latest recovery state after finalizing current quit approvals; incomplete or stale plans keep their documents, and Undo to the saved state clears stale recovery. Unavailable or failed recovery is reported. Cancelable macOS Dock Quit remains open (#544). No readiness percentage change. - **2026-10-12** · Recovered documents keep a checkpoint until saved, closed or reverted, including a second crash before autosave. Restarted document IDs and explicit recovery folders preserve other snapshots; failed republishing retains the original. No readiness percentage change. diff --git a/apps/designcraft-cli/src/main.rs b/apps/designcraft-cli/src/main.rs index dd62cd91d..17ba8e857 100644 --- a/apps/designcraft-cli/src/main.rs +++ b/apps/designcraft-cli/src/main.rs @@ -252,6 +252,8 @@ fn run(args: &[String]) -> Result<(), String> { let (id, p) = c.split_once('=').unwrap_or((&c, "{}")); let p: Value = serde_json::from_str(p).map_err(|e| format!("--cmd {id}: {e}"))?; let p = designcraft_engine::script::resolve(&p, &results).map_err(|e| format!("--cmd {id}: {e}"))?; + // A key the command's description doesn't name is an error, not a default (#776). + designcraft_engine::check_params(id, &p).map_err(|e| e.to_string())?; let r = s.execute(id, &p).map_err(|e| e.to_string())?; results.push(r.clone()); if !r.is_null() { diff --git a/apps/designcraft-cli/tests/cli.rs b/apps/designcraft-cli/tests/cli.rs index 92f68ab56..dd187108e 100644 --- a/apps/designcraft-cli/tests/cli.rs +++ b/apps/designcraft-cli/tests/cli.rs @@ -67,6 +67,22 @@ fn run_cmd_references_and_describe() { assert!(v.as_array().unwrap().iter().all(|c| c.to_string().contains("footnote"))); } +/// #776: a key the command doesn't take fails the run (`--cmd`) or the step (`script`) instead of +/// being ignored. +#[test] +fn unknown_params_fail() { + let out = cli().args(["run", "--cmd", r#"style.list={"bogus":1}"#]).output().unwrap(); + assert!(!out.status.success()); + assert!(String::from_utf8_lossy(&out.stderr).contains("unknown parameter `bogus`; the command takes no parameters")); + let mut child = cli().args(["script", "-"]).stdin(Stdio::piped()).stdout(Stdio::piped()).stderr(Stdio::piped()).spawn().unwrap(); + child.stdin.take().unwrap().write_all(b"document.inspect\nfile.new {\"widht\": 300}\n").unwrap(); + let out = child.wait_with_output().unwrap(); + assert!(!out.status.success()); + let v: serde_json::Value = serde_json::from_slice(&out.stdout).unwrap(); + assert_eq!(v["failedIndex"], 1); + assert!(v.to_string().contains("did you mean `width`?"), "{v}"); +} + /// `designcraft-cli --help` printed usage to stderr and exited 1; asking for help is not an /// error (#243). It now goes to stdout and succeeds, while an unknown command keeps the same /// usage on stderr and fails. diff --git a/crates/engine/src/cmd/anchored.rs b/crates/engine/src/cmd/anchored.rs index 8c85c7062..6ad49f8c8 100644 --- a/crates/engine/src/cmd/anchored.rs +++ b/crates/engine/src/cmd/anchored.rs @@ -18,7 +18,7 @@ pub fn specs() -> Vec { "Insert Anchored Object…", ["Object", "Anchored Object"], None, - "{ids, story?, pos? (default: the text insertion point), position?: inline|aboveLine|custom, yOffset?, align?: left|center|right, spaceBefore?, spaceAfter?, xRelative?/yRelative?: anchor|textFrame|columnEdge|pageMargin|pageEdge, xOffset?, objectPoint?/refPoint?: 0..8, keepWithinColumn?} — moves the items into the text", + "{ids, story?, pos? (default: the text insertion point), position?: inline|aboveLine|custom, yOffset?, align?: left|center|right, spaceBefore?, spaceAfter?, xRelative? | yRelative?: anchor|textFrame|columnEdge|pageMargin|pageEdge, xOffset?, objectPoint? | refPoint?: 0..8, keepWithinColumn?} — moves the items into the text", has_doc, insert ), @@ -27,7 +27,7 @@ pub fn specs() -> Vec { "Anchored Object Options…", ["Object", "Anchored Object"], None, - "{story?, index? (default: the object at the insertion point), position?: inline|aboveLine|custom, yOffset?, align?: left|center|right, spaceBefore?, spaceAfter?, xRelative?/yRelative?: anchor|textFrame|columnEdge|pageMargin|pageEdge, xOffset?, objectPoint?/refPoint?: 0..8, keepWithinColumn?}", + "{story?, index? (default: the object at the insertion point), position?: inline|aboveLine|custom, yOffset?, align?: left|center|right, spaceBefore?, spaceAfter?, xRelative? | yRelative?: anchor|textFrame|columnEdge|pageMargin|pageEdge, xOffset?, objectPoint? | refPoint?: 0..8, keepWithinColumn?}", has_doc, options ), diff --git a/crates/engine/src/cmd/datamerge/mod.rs b/crates/engine/src/cmd/datamerge/mod.rs index 3b3e1dd3b..265f2d62d 100644 --- a/crates/engine/src/cmd/datamerge/mod.rs +++ b/crates/engine/src/cmd/datamerge/mod.rs @@ -44,7 +44,7 @@ pub fn specs() -> Vec { "Data Fields", [], None, - "{csv? | rows? | path? | bytes?, name?} → [{name, kind, uses}]; no params reads the linked source and does not attach one", + "{csv? | rows? | path? | bytes? | base64?, name?, delimiter?, sheet?} → [{name, kind, uses}]; no params reads the linked source and does not attach one", has_doc, fields_cmd ), @@ -74,7 +74,7 @@ pub fn specs() -> Vec { "Create Merged Document…", [], None, - "{csv? | rows? | path? | bytes?, name?, records?, one?, range?, perPage?, arrange?, insets?, columnSpacing?, rowSpacing?, fitting?, center?, linkImages?, limit?} → {records, pages, missingImages, oversetStories, warnings}", + "{csv? | rows? | path? | bytes? | base64?, name?, delimiter?, sheet?, records?, one?, range?, perPage?, arrange?, insets?, columnSpacing?, rowSpacing?, fitting?, center?, linkImages?, limit?, spread?: removed (passing it is an error)} → {records, pages, missingImages, oversetStories, warnings}", has_doc, merge_cmd ), diff --git a/crates/engine/src/cmd/export.rs b/crates/engine/src/cmd/export.rs index 8e0f04d69..4bf2a759f 100644 --- a/crates/engine/src/cmd/export.rs +++ b/crates/engine/src/cmd/export.rs @@ -882,7 +882,7 @@ mod print_group_tests { fn print_transparency_groups_blend_in_cmyk() { let mut s = page_with(&logo()); let text = |s: &mut Session| { - let r = s.execute("file.exportPdf", &json!({"compress": false})).unwrap(); + let r = s.execute("file.exportPdf", &json!({"compressImages": false})).unwrap(); String::from_utf8_lossy(&base64_decode(r["base64"].as_str().unwrap())).into_owned() }; let t = text(&mut s); diff --git a/crates/engine/src/cmd/file.rs b/crates/engine/src/cmd/file.rs index a1919b89e..d793b8a8d 100644 --- a/crates/engine/src/cmd/file.rs +++ b/crates/engine/src/cmd/file.rs @@ -13,7 +13,7 @@ use crate::{DocState, EngineError, Result, Session}; pub fn specs() -> Vec { vec![ cmd!(noundo "file.new", "Document…", ["File", "New"], Some("Cmd+N"), - "{preset?: \"Letter\"|\"A4\"|…, width?, height?, pages?, facingPages?, columns?, gutter?, margins?: number|{top,bottom,inside,outside}, bleed?, title?}", + "{preset?: \"Letter\"|\"A4\"|…, width?, height?, pages?, facingPages?, columns?, gutter?, margins?: number|{top,bottom,inside,outside}, bleed?, title?, primaryTextFrame?: bool}", always, file_new), cmd!(noundo "file.newSample", "Sample Document", ["Help"], None, "{} — a multi-page magazine sample", always, file_sample), cmd!(query "file.presets", "Document Presets", [], None, "{}", always, |_, _| Ok(serde_json::to_value(PRESETS).unwrap_or_default())), @@ -102,7 +102,7 @@ pub fn specs() -> Vec { "Place…", ["File"], Some("Cmd+D"), - "{path?|base64?, name?, frame?: id (place into), spread?, x?, y?, width?, pdfPage?: n (1-based, Image Import Options), pdfCrop?: crop|trim|bleed|art|media, layoutPage?: n (IDML / .designcraft: that page's objects as a group)} — places an image (into the selected empty frame if any); text files (.txt, .docx, .rtf, .md) and Excel workbooks (.xlsx, as a table) go into the insertion point, the selected frame or a new frame on `page`/`rect` — {autoflow?: adds pages with threaded frames until the text fits, removeStyles?, styleMap?: {imported name: document style}, styleConflicts?: useExisting|redefine|autoRename}", + "{path?|base64?, name?, frame?: id (place into), spread?, x?, y?, width?, height? (video/audio), pdfPage?: n (1-based, Image Import Options), pdfCrop?: crop|trim|bleed|art|media, layoutPage?: n (IDML / .designcraft: that page's objects as a group), page?, rect?: [x0,y0,x1,y1] (text: a new frame there), autoflow?: adds pages with threaded frames until the text fits, removeStyles?, styleMap?: {imported name: document style}, styleConflicts?: useExisting|redefine|autoRename} — places an image (into the selected empty frame if any); text files (.txt, .docx, .rtf, .md) and Excel workbooks (.xlsx, as a table) go into the insertion point, the selected frame or a new frame on `page`/`rect`", has_doc, file_place ), diff --git a/crates/engine/src/cmd/find.rs b/crates/engine/src/cmd/find.rs index 5dc0bc8ad..ef9157c5f 100644 --- a/crates/engine/src/cmd/find.rs +++ b/crates/engine/src/cmd/find.rs @@ -25,7 +25,7 @@ pub fn specs() -> Vec { "Change All (Object)", [], None, - "{…find.objects criteria, change: {fill?, stroke?, strokeWeight?, opacity?}} → {changed}", + "{…the criteria of find.objects, change: {fill?, stroke?, strokeWeight?, opacity?}} → {changed}", has_doc, |s, p| { let ids = find_objects(&s.doc()?.doc, p); @@ -65,7 +65,7 @@ pub fn specs() -> Vec { has_doc, change ), - cmd!(noundo "find.next", "Find Next", ["Edit"], Some("Cmd+Alt+F"), "{find, grep?, …} — selects the next match after the caret", has_doc, find_next), + cmd!(noundo "find.next", "Find Next", ["Edit"], Some("Cmd+Alt+F"), "{find, grep?, caseSensitive?, wholeWord?} — selects the next match after the caret", has_doc, find_next), ] } diff --git a/crates/engine/src/cmd/mathexpr.rs b/crates/engine/src/cmd/mathexpr.rs index d9ce51148..ecee82729 100644 --- a/crates/engine/src/cmd/mathexpr.rs +++ b/crates/engine/src/cmd/mathexpr.rs @@ -40,7 +40,7 @@ pub fn specs() -> Vec { "Insert Math Expression…", ["Type"], None, - "{latex, size?: points (12), x?, y?, spread?} — at the text insertion point (inline), else as a frame; or {id, latex} to edit one", + "{latex, size?: points (12), x?, y?, spread?} or {id, latex, size?} — at the text insertion point (inline), else as a frame; with `id`: edits that equation", has_doc, insert ), diff --git a/crates/engine/src/cmd/mod.rs b/crates/engine/src/cmd/mod.rs index 31e4544f4..80d8f295b 100644 --- a/crates/engine/src/cmd/mod.rs +++ b/crates/engine/src/cmd/mod.rs @@ -34,6 +34,7 @@ mod notes; mod object; mod overrides; mod package; +pub mod params; mod path_type; mod paths; mod pdflayers; diff --git a/crates/engine/src/cmd/object.rs b/crates/engine/src/cmd/object.rs index aa5a2cc01..2ab043774 100644 --- a/crates/engine/src/cmd/object.rs +++ b/crates/engine/src/cmd/object.rs @@ -32,7 +32,7 @@ pub fn specs() -> Vec { "Create Frame", [], None, - "{spread?: index | {kind: doc|parent, index} (a parent spread: index into `parents` of document.inspect), rect: [x0,y0,x1,y1] (spread coords), shape?: rectangle|ellipse|polygon, content?: graphic|text|unassigned, sides?: 6, text?: string, caret?: bool, vertical?: bool (text: a new vertical story)}", + "{spread?: index | {kind: doc|parent, index} (a parent spread: index into `parents` of document.inspect), rect: [x0,y0,x1,y1] (spread coords), shape?: rectangle|ellipse|polygon, content?: graphic|text|unassigned, sides?: 6, starInset?: 0–100 (%), text?: string, caret?: bool, vertical?: bool (text: a new vertical story)}", has_doc, frame_create ), @@ -532,7 +532,7 @@ pub fn specs() -> Vec { "Text Frame Options…", ["Object"], Some("Cmd+B"), - "{columns?, gutter?, inset?: number|[t,l,b,r] (null keeps that side), verticalJustification?: top|center|bottom|justify, firstBaseline?, autoSize?, ignoreWrap?, balanceColumns?, columnRule?: bool, columnRuleWeight? (pt), columnRuleColor? (swatch name), columnRuleTint? (0..1), columnRuleOffset? (pt, horizontal), columnRuleTopInset? (pt), columnRuleBottomInset? (pt), vertical?: bool (sets the story direction of the frames' stories, as Type ▸ Story Direction), ids?}", + "{columns?, gutter?, inset?: number|[t,l,b,r] (null keeps that side), verticalJustification?: top|center|bottom|justify, firstBaseline?, autoSize?, columnsKind?: fixedNumber|fixedWidth|flexibleWidth, ignoreWrap?, balanceColumns?, columnRule?: bool, columnRuleWeight? (pt), columnRuleColor? (swatch name), columnRuleTint? (0..1), columnRuleOffset? (pt, horizontal), columnRuleTopInset? (pt), columnRuleBottomInset? (pt), vertical?: bool (sets the story direction of the frames' stories, as Type ▸ Story Direction), ids?}", has_selection, text_frame_options ), @@ -599,7 +599,7 @@ pub fn specs() -> Vec { has_selection, fitting_options ), - cmd!("object.rename", "Rename", [], None, "{id, name}", has_doc, |s, p| { + cmd!("object.rename", "Rename", [], None, "{id | ids?, name}", has_doc, |s, p| { let name = str_param(p, "name").unwrap_or("").to_string(); set_flag(s, p, move |i| i.name = name.clone(), false) }), @@ -682,7 +682,7 @@ pub fn specs() -> Vec { "Move Gap", [], None, - "{at: [x, y] (spread), delta | dx/dy, spread?} — Gap tool: the gap at `at` between objects (or an object and the page edge) moves by `delta`; the objects on both sides resize → {axis, changed}", + "{at: [x, y] (spread), delta?: pt | dx? | dy?: a drag (the part along the gap's axis counts), spread?} — Gap tool: the gap at `at` between objects (or an object and the page edge) moves by `delta`; the objects on both sides resize → {axis, changed}", has_doc, gap_move ), diff --git a/crates/engine/src/cmd/params.rs b/crates/engine/src/cmd/params.rs new file mode 100644 index 000000000..8a33752c1 --- /dev/null +++ b/crates/engine/src/cmd/params.rs @@ -0,0 +1,230 @@ +//! The parameter keys a command accepts, read from its params description (#776). +//! +//! A description starts with the params object: `{x, width?: pt, name?|title?: …, sides=6, …}`, or +//! with the objects a command takes instead of one another: `{path} or {name, dataBase64}`, +//! `{name} | {names: […]}`. Each entry of an object starts with a key, or with alternatives split +//! by `|`; a `?` marks an optional one, and a `:`, `=` or `(` starts what the entry says about it. +//! An entry that starts with `…` stands for keys listed elsewhere: those of each command it names +//! after `as` or `of` (`…settings as document.new`), or, naming none, any key. Objects inside a +//! value aren't described key by key, so only the top level is checked. +//! +//! Callers outside the app (the control channel, MCP, the CLI) are answered with a parameter error +//! for a key the description doesn't name ([`check_params`]): a misspelt `widht` no longer runs the +//! command with its default. [`Session::execute`](crate::Session::execute) itself takes what it is +//! given, so the journal of an older version still replays. +//! +//! Ported from VectorCraft (storytold/vectorcraft#1442). + +use serde_json::Value; + +use super::{bad, find_command}; +use crate::Result; + +/// What a params description says about the keys of the params object. +#[derive(Debug, Default, PartialEq)] +pub struct ParamKeys<'a> { + /// The keys it names. + pub keys: Vec<&'a str>, + /// Its `…` entries, without the `…`. + pub more: Vec<&'a str>, +} + +/// The text between the braces of the object `doc` starts with and what follows it, or why it +/// has none. +fn object_body(doc: &str) -> std::result::Result<(&str, &str), String> { + // (Named, not written in the `Err`: these are for the tests of the descriptions, not messages + // the app shows, which the message catalogs would have to translate.) + let (no_object, not_closed) = ("the description doesn't start with the params object `{…}`", "the params object `{…}` isn't closed"); + if !doc.starts_with('{') { + return Err(no_object.into()); + } + let (mut depth, mut quote) = (0usize, None); + for (i, c) in doc.char_indices() { + match (quote, c) { + (Some(q), _) if c == q => quote = None, + (Some(_), _) => {} + (None, '"' | '`') => quote = Some(c), + (None, '{' | '[' | '(') => depth += 1, + (None, '}' | ']' | ')') => { + depth = depth.saturating_sub(1); + if depth == 0 { + return Ok((doc.get(1..i).unwrap_or_default(), doc.get(i + 1..).unwrap_or_default())); + } + } + _ => {} + } + } + Err(not_closed.into()) +} + +/// `s` split at each `sep` that is outside brackets, quotes and backticks. +fn split_top(s: &str, sep: char) -> Vec<&str> { + let (mut out, mut depth, mut quote, mut start) = (Vec::new(), 0usize, None, 0); + for (i, c) in s.char_indices() { + match (quote, c) { + (Some(q), _) if c == q => quote = None, + (Some(_), _) => {} + (None, '"' | '`') => quote = Some(c), + (None, '{' | '[' | '(') => depth += 1, + (None, '}' | ']' | ')') => depth = depth.saturating_sub(1), + (None, _) if c == sep && depth == 0 => { + out.push(s.get(start..i).unwrap_or_default()); + start = i + c.len_utf8(); + } + _ => {} + } + } + out.push(s.get(start..).unwrap_or_default()); + out +} + +fn is_key(s: &str) -> bool { + let mut chars = s.chars(); + chars.next().is_some_and(|c| c.is_ascii_alphabetic() || c == '_') && chars.all(|c| c.is_ascii_alphanumeric() || c == '_') +} + +/// The keys a params description names, or what in it doesn't read as one. +pub fn doc_keys(doc: &str) -> std::result::Result, String> { + let mut out = ParamKeys::default(); + let (mut body, mut rest) = object_body(doc)?; + let mut entries = split_top(body, ','); + // `{…} or {…}`, `{…} | {…}`: the objects the command takes instead. + while let Some(next) = rest.trim_start().strip_prefix("or ").or_else(|| rest.trim_start().strip_prefix("| ")).filter(|r| r.starts_with('{')) { + (body, rest) = object_body(next)?; + entries.extend(split_top(body, ',')); + } + for entry in entries.into_iter().map(str::trim).filter(|e| !e.is_empty()) { + if let Some(more) = entry.strip_prefix('…') { + out.more.push(more.trim()); + continue; + } + // `a?|b?: note` names two keys, and so does `a?: note | b?: note`; in `a?: x|y` the `y` + // is a value. + let mut noted = false; + for alternative in split_top(entry, '|') { + let head = alternative.split([':', '=', '(']).next().unwrap_or_default(); + let rest = alternative.get(head.len()..).unwrap_or_default(); + let name = head.trim().trim_end_matches('?').trim_end(); + if !noted { + if !is_key(name) { + let start: String = entry.chars().take(60).collect(); + let why = format!("`{start}` doesn't start with a parameter name"); + return Err(why); + } + out.keys.push(name); + noted = !rest.is_empty(); + } else if is_key(name) && (head.trim_end().ends_with('?') || rest.starts_with(':')) { + out.keys.push(name); + } + } + } + Ok(out) +} + +/// The commands whose keys a `…` entry stands for: those it names after `as` or `of` (`settings as +/// file.new`, `the PDF options of document.exportPdf`), not one it only points to (`see …`). +fn named_commands(more: &str) -> impl Iterator + '_ { + let words: Vec<&str> = more.split_whitespace().collect(); + let id = |w: &str| find_command(w.trim_matches(|c: char| !c.is_ascii_alphanumeric())); + (1..words.len()).filter_map(move |i| id(words.get(i)?).filter(|_| matches!(words.get(i - 1), Some(&"as" | &"of")))) +} + +/// Every key command `doc` accepts, the keys of the commands its `…` entries name included. +/// `None`: any key (a `…` entry naming no command, or a description that doesn't parse, which +/// the tests rule out). +fn accepted(doc: &'static str, depth: usize) -> Option> { + let parsed = doc_keys(doc).ok()?; + let mut keys = parsed.keys; + // A command that acts on `ids` takes one object as `id` too (see `cmd::targets`). + if keys.contains(&"ids") && !keys.contains(&"id") { + keys.push("id"); + } + for more in parsed.more { + let mut named = named_commands(more).peekable(); + // A reference that leads back here would never end: descriptions refer one or two deep. + if named.peek().is_none() || depth > 4 { + return None; + } + for spec in named { + // A command that isn't checked yet doesn't vouch for the ones that take its keys. + if UNCHECKED.contains(&spec.id) { + return None; + } + keys.extend(accepted(spec.params, depth + 1)?); + } + } + Some(keys) +} + +/// The edits (insert, delete, replace, swap of neighbours) that turn `a` into `b`, ignoring case. +fn distance(a: &str, b: &str) -> usize { + let (a, b): (Vec, Vec) = (a.chars().map(|c| c.to_ascii_lowercase()).collect(), b.chars().map(|c| c.to_ascii_lowercase()).collect()); + let (mut before, mut prev): (Vec, Vec) = (Vec::new(), (0..=b.len()).collect()); + for (i, ca) in a.iter().enumerate() { + let mut row = vec![i + 1]; + for (j, cb) in b.iter().enumerate() { + let at = |r: &[usize], k: usize| r.get(k).copied().unwrap_or(usize::MAX / 2); + let mut d = (at(&prev, j) + usize::from(ca != cb)).min(at(&prev, j + 1) + 1).min(at(&row, j) + 1); + if i > 0 && j > 0 && a.get(i - 1) == Some(cb) && b.get(j - 1) == Some(ca) { + d = d.min(at(&before, j - 1) + 1); + } + row.push(d); + } + before = std::mem::replace(&mut prev, row); + } + prev.last().copied().unwrap_or(0) +} + +/// The key of `keys` a misspelt `key` most likely meant: the nearest one, when it is near. +fn nearest<'a>(key: &str, keys: &[&'a str]) -> Option<&'a str> { + let near = keys.iter().map(|k| (distance(key, k), *k)).min()?; + (near.0 <= (key.chars().count() / 3).max(1)).then_some(near.1) +} + +/// `params` has only keys the description `doc` of command `id` names, or the parameter error +/// naming the first one it doesn't, with the nearest accepted key or the accepted ones. +pub fn check_keys(id: &str, doc: &'static str, params: &Value) -> Result<()> { + let Value::Object(given) = params else { return Ok(()) }; + if given.is_empty() { + return Ok(()); + } + let Some(keys) = accepted(doc, 0) else { return Ok(()) }; + match given.keys().find(|k| !keys.contains(&k.as_str())) { + Some(key) => Err(unknown(id, key, &keys)), + None => Ok(()), + } +} + +/// The parameter error of command `id` for `key`, which isn't one of `keys`: with the nearest of +/// them, or all of them. +fn unknown(id: &str, key: &str, keys: &[&str]) -> crate::EngineError { + bad( + id, + match nearest(key, keys) { + Some(near) => format!("unknown parameter `{key}`; did you mean `{near}`?"), + None if keys.is_empty() => format!("unknown parameter `{key}`; the command takes no parameters"), + None => format!("unknown parameter `{key}`; the command takes: {}", keys.join(", ")), + }, + ) +} + +/// Commands whose params aren't checked yet (#776): they read keys in ways the audit of the +/// descriptions couldn't follow (a list of keys in a loop, a whole object read as settings), so +/// their descriptions aren't proven to name every key they take. Sorted; take a command out once +/// its description is checked against its code. +pub const UNCHECKED: &[&str] = &[ + // Take the attributes under `attrs` or as the params object itself; the attribute setters + // refuse a key they don't know. + "type.char", + "type.para", +]; + +/// [`check_keys`] for an engine command. A command the engine doesn't have passes: running it +/// reports that. +pub fn check_params(id: &str, params: &Value) -> Result<()> { + let Some(spec) = find_command(id) else { return Ok(()) }; + if UNCHECKED.contains(&id) { + return Ok(()); + } + check_keys(id, spec.params, params) +} diff --git a/crates/engine/src/cmd/prefs.rs b/crates/engine/src/cmd/prefs.rs index 68a5cf152..e8bdd47cd 100644 --- a/crates/engine/src/cmd/prefs.rs +++ b/crates/engine/src/cmd/prefs.rs @@ -48,7 +48,7 @@ pub fn specs() -> Vec { "Preferences", [], None, - "{showHiddenCharacters?, typographersQuotes?, polygonSides?, starInset?, scaleStrokes?, dimensionsIncludeStroke?, transformationsAreTotals?, absolutePageNumbers?, highlightHj?, highlightKeeps?, highlightCustomTracking?, highlightSubstitutedFonts?, richBlackOutput?, favoriteFonts?: [family], showFontNamesInEnglish?} → all application preferences", + "{showHiddenCharacters?, typographersQuotes?, polygonSides?, starInset?, scaleStrokes?, dimensionsIncludeStroke?, transformationsAreTotals?, absolutePageNumbers?, highlightHj?, highlightKeeps?, highlightCustomTracking?, highlightSubstitutedFonts?, richBlackOutput?, favoriteFonts?: [family], recentFonts?: [family], showFontNamesInEnglish?, smartTextReflow?, autocorrect?, autocorrectList?: [[word, correction]], showAddedText?, showNoteAnchors?, recoveryMinutes?} → all application preferences", super::always, |s, p| { let cur = serde_json::to_value(&s.prefs).map_err(|e| bad("prefs.set", e.to_string()))?; diff --git a/crates/engine/src/cmd/style.rs b/crates/engine/src/cmd/style.rs index c4d20f099..7af194318 100644 --- a/crates/engine/src/cmd/style.rs +++ b/crates/engine/src/cmd/style.rs @@ -1260,7 +1260,12 @@ mod color_tests { let mut session = fill_session(); session.execute("style.paragraph.apply", &json!({"name": "Existing"})).unwrap(); session.execute("style.character.apply", &json!({"name": "Existing"})).unwrap(); - assert_fill_rejected(&mut session, command, json!({"name": "Existing", "rename": "Renamed", "chars": {"fill": fill, "size": 24}})); + let mut params = json!({"name": "Existing", "chars": {"fill": fill, "size": 24}}); + // Only the edit commands take `rename`. + if command.ends_with(".edit") { + params["rename"] = json!("Renamed"); + } + assert_fill_rejected(&mut session, command, params); } } } @@ -1611,7 +1616,7 @@ mod export_tag_tests { assert!(html.contains("

Origins

"), "{html}"); assert!(html.contains("word"), "{html}"); // Tagged PDF: a heading element and a paragraph element. - let r = s.execute("file.exportPdf", &json!({"tagged": true, "compress": false})).unwrap(); + let r = s.execute("file.exportPdf", &json!({"tagged": true, "compressImages": false})).unwrap(); let pdf = super::super::file::base64_decode(r["base64"].as_str().unwrap()); let text = String::from_utf8_lossy(&pdf); assert!(text.contains("/S/H1") && text.contains("/S/P"), "structure elements"); @@ -1728,12 +1733,20 @@ mod sample_color_tests { #[test] fn sampling_and_explicit_colour_use_the_same_text_application_path() { for command in ["color.sample", "object.color"] { + // Each command gets its own key: the eyedropper samples at `at`, object.color takes `color`. + let params = |at: [f64; 2], color: &str, extra: serde_json::Value| { + let mut p = if command == "color.sample" { json!({"at": at}) } else { json!({"color": color}) }; + if let (Some(p), Some(extra)) = (p.as_object_mut(), extra.as_object()) { + p.extend(extra.clone()); + } + p + }; let (mut s, _) = session_with_image(); let f = s.execute("frame.create", &json!({"rect": [300, 300, 500, 360], "content": "text"})).unwrap(); let story = designcraft_doc::StoryId(f["story"].as_u64().unwrap()); s.execute("text.select", &json!({"story": story.0, "anchor": 0})).unwrap(); let undo = s.doc().unwrap().history.undo.len(); - let r = s.execute(command, &json!({"at": [100, 100], "color": "#ff0000"})).unwrap(); + let r = s.execute(command, ¶ms([100.0, 100.0], "#ff0000", json!({}))).unwrap(); assert_eq!(s.doc().unwrap().history.undo.len(), undo + 1, "{command}"); s.execute("text.insert", &json!({"text": "Hi"})).unwrap(); let text = s.doc().unwrap().doc.story(story).unwrap(); @@ -1741,11 +1754,11 @@ mod sample_color_tests { // Explicit ids override text targeting, including with a text selection active. let fid = ItemId(f["id"].as_u64().unwrap()); s.execute("text.select", &json!({"story": story.0, "anchor": 0, "focus": 2})).unwrap(); - let r = s.execute(command, &json!({"at": [180, 100], "color": "#0000ff", "id": fid.0})).unwrap(); + let r = s.execute(command, ¶ms([180.0, 100.0], "#0000ff", json!({"id": fid.0}))).unwrap(); assert_eq!(s.doc().unwrap().doc.item(fid).unwrap().fill.swatch, r["swatch"].as_str().unwrap()); let text = s.doc().unwrap().doc.story(story).unwrap(); assert_ne!(text.format_after(0).over.fill.as_deref(), r["swatch"].as_str(), "text unchanged: {command}"); - let r = s.execute(command, &json!({"at": [180, 100], "color": "#0000ff", "target": "stroke"})).unwrap(); + let r = s.execute(command, ¶ms([180.0, 100.0], "#0000ff", json!({"target": "stroke"}))).unwrap(); let text = s.doc().unwrap().doc.story(story).unwrap(); assert_eq!(text.format_after(0).over.stroke.as_deref(), r["swatch"].as_str(), "selected text stroke: {command}"); } diff --git a/crates/engine/src/cmd/table.rs b/crates/engine/src/cmd/table.rs index ad306be8e..9a62ce242 100644 --- a/crates/engine/src/cmd/table.rs +++ b/crates/engine/src/cmd/table.rs @@ -29,7 +29,7 @@ fn style_specs() -> Vec { "New Cell Style…", [], None, - "{name, fromSelection?: bool (the target cell's look), fill?, tint?, insets?: n|[t,l,b,r], vj?: top|center|bottom|justify, stroke?: {weight, color, tint?}, paragraphStyle?} → {name}", + "{name, fromSelection?: bool (the target cell's look), fill?, tint?, insets?: n|[t,l,b,r], vj?: top|center|bottom|justify, stroke?: {weight, color, tint?}, paragraphStyle?, basedOn?: cell style name, story?, table?, rows?: [a, b] | row?, cols?: [a, b] | col? (the target cells; default: the selection)} → {name}", has_doc, |s, p| cell_style_create(s, p) ), @@ -38,7 +38,7 @@ fn style_specs() -> Vec { "Apply Cell Style", [], None, - "{name, story?, table?} — the target cells (the whole table when the cursor is in it)", + "{name, story?, table?, rows?: [a,b] | row?, cols?: [a,b] | col?} — the target cells (the whole table when the cursor is in it)", in_table, cell_style_apply ), @@ -56,11 +56,19 @@ fn style_specs() -> Vec { "New Table Style…", [], None, - "{name, header?, body?, footer?, leftColumn?, rightColumn?: cell style names, border?: {weight, color}, altRows?: {first, firstColor, next, nextColor}, spaceBefore?, spaceAfter?} → {name}", + "{name, basedOn?: table style name, header?, body?, footer?, leftColumn?, rightColumn?: cell style names, border?: {weight, color}, altRows?: {first, firstColor, next, nextColor}, spaceBefore?, spaceAfter?} → {name}", has_doc, table_style_create ), - cmd!("style.table.apply", "Apply Table Style", [], None, "{name, story?, table?}", in_table, table_style_apply), + cmd!( + "style.table.apply", + "Apply Table Style", + [], + None, + "{name, story?, table?, rows?: [a,b] | row?, cols?: [a,b] | col?}", + in_table, + table_style_apply + ), cmd!( "style.table.edit", "Table Style Options…", @@ -80,7 +88,7 @@ fn table_specs() -> Vec { "Create Table", [], Some("Cmd+Alt+Shift+T"), - "{rows?: body rows (4), cols? (4), headerRows? (0), footerRows? (0), width?} — at the text insertion point", + "{rows?: body rows (4), cols? | columns? (4), headerRows? (0), footerRows? (0), width?} — at the text insertion point", has_text, insert ), @@ -89,7 +97,7 @@ fn table_specs() -> Vec { "Convert Text to Table", ["Table"], None, - "{columnSeparator?: tab|comma} — selected paragraphs become rows", + "{columnSeparator?: tab|comma, width?: table width (default: the column)} — selected paragraphs become rows", has_text, convert_from_text ), @@ -98,20 +106,100 @@ fn table_specs() -> Vec { "Convert Table to Text", ["Table"], None, - "{} — cells separated by tabs, rows by paragraphs", + "{story?, table?, rows?: [a, b] | row?, cols?: [a, b] | col? (the target cells; default: the selection)} — cells separated by tabs, rows by paragraphs", in_table, convert_to_text ), - cmd!("table.insertRow", "Insert Row", [], None, "{where?: above|below, count? (1)}", in_table, |s, p| insert_row(s, p, None)), - cmd!("table.insertRowAbove", "Insert Row Above", ["Table", "Insert"], None, "{count?}", in_table, |s, p| insert_row(s, p, Some(true))), - cmd!("table.insertRowBelow", "Insert Row Below", ["Table", "Insert"], None, "{count?}", in_table, |s, p| insert_row(s, p, Some(false))), - cmd!("table.insertColumn", "Insert Column", [], None, "{where?: left|right, count? (1), width?}", in_table, |s, p| insert_col(s, p, None)), - cmd!("table.insertColumnLeft", "Insert Column Left", ["Table", "Insert"], None, "{count?}", in_table, |s, p| insert_col(s, p, Some(true))), - cmd!("table.insertColumnRight", "Insert Column Right", ["Table", "Insert"], None, "{count?}", in_table, |s, p| insert_col(s, p, Some(false))), - cmd!("table.deleteRow", "Delete Row", ["Table", "Delete"], None, "{} — the rows of the target cells", in_table, delete_rows), - cmd!("table.deleteColumn", "Delete Column", ["Table", "Delete"], None, "{} — the columns of the target cells", in_table, delete_cols), - cmd!("table.delete", "Delete Table", ["Table", "Delete"], None, "{}", in_table, delete_table), - cmd!("table.merge", "Merge Cells", ["Table"], None, "{} — merge the target cell range", in_table, merge), + cmd!( + "table.insertRow", + "Insert Row", + [], + None, + "{where?: above|below, count? (1), story?, table?, rows?: [a, b] | row?, cols?: [a, b] | col? (the target cells; default: the selection)}", + in_table, + |s, p| insert_row(s, p, None) + ), + cmd!( + "table.insertRowAbove", + "Insert Row Above", + ["Table", "Insert"], + None, + "{count?, story?, table?, rows?: [a, b] | row?, cols?: [a, b] | col? (the target cells; default: the selection)}", + in_table, + |s, p| insert_row(s, p, Some(true)) + ), + cmd!( + "table.insertRowBelow", + "Insert Row Below", + ["Table", "Insert"], + None, + "{count?, story?, table?, rows?: [a, b] | row?, cols?: [a, b] | col? (the target cells; default: the selection)}", + in_table, + |s, p| insert_row(s, p, Some(false)) + ), + cmd!( + "table.insertColumn", + "Insert Column", + [], + None, + "{where?: left|right, count? (1), width?, story?, table?, rows?: [a, b] | row?, cols?: [a, b] | col? (the target cells; default: the selection)}", + in_table, + |s, p| insert_col(s, p, None) + ), + cmd!( + "table.insertColumnLeft", + "Insert Column Left", + ["Table", "Insert"], + None, + "{count?, width?, story?, table?, rows?: [a, b] | row?, cols?: [a, b] | col? (the target cells; default: the selection)}", + in_table, + |s, p| insert_col(s, p, Some(true)) + ), + cmd!( + "table.insertColumnRight", + "Insert Column Right", + ["Table", "Insert"], + None, + "{count?, width?, story?, table?, rows?: [a, b] | row?, cols?: [a, b] | col? (the target cells; default: the selection)}", + in_table, + |s, p| insert_col(s, p, Some(false)) + ), + cmd!( + "table.deleteRow", + "Delete Row", + ["Table", "Delete"], + None, + "{story?, table?, rows?: [a, b] | row?, cols?: [a, b] | col? (the target cells; default: the selection)} — the rows of the target cells", + in_table, + delete_rows + ), + cmd!( + "table.deleteColumn", + "Delete Column", + ["Table", "Delete"], + None, + "{story?, table?, rows?: [a, b] | row?, cols?: [a, b] | col? (the target cells; default: the selection)} — the columns of the target cells", + in_table, + delete_cols + ), + cmd!( + "table.delete", + "Delete Table", + ["Table", "Delete"], + None, + "{story?, table?, rows?: [a, b] | row?, cols?: [a, b] | col? (the target cells; default: the selection)}", + in_table, + delete_table + ), + cmd!( + "table.merge", + "Merge Cells", + ["Table"], + None, + "{story?, table?, rows?: [a, b] | row?, cols?: [a, b] | col? (the target cells; default: the selection)} — merge the target cell range", + in_table, + merge + ), cmd!( "table.dropCells", "Drag Rows or Columns", @@ -126,7 +214,7 @@ fn table_specs() -> Vec { "Convert Cell to Graphic Cell", ["Table", "Convert Cell Type"], None, - "{path | base64+name, fit?: proportional|fill (default proportional)} — an image in the target cell (its text is kept but hidden)", + "{path? | base64?, name?: with base64, fit?: proportional|fill (default proportional), story?, table?, rows?: [a, b] | row?, cols?: [a, b] | col? (the target cells; default: the selection)} — an image in the target cell (its text is kept but hidden)", in_table, place_graphic ), @@ -135,7 +223,7 @@ fn table_specs() -> Vec { "Convert Cell to Text Cell", ["Table", "Convert Cell Type"], None, - "{} — drop the target cells' graphics", + "{story?, table?, rows?: [a, b] | row?, cols?: [a, b] | col? (the target cells; default: the selection)} — drop the target cells' graphics", in_table, |s, p| { let g = target(s, p, "table.textCell")?; @@ -156,29 +244,43 @@ fn table_specs() -> Vec { "Move Row", [], None, - "{from?, to} (0-based; from defaults to the target row) — like dragging a row", + "{from?, to, story?, table?, rows?: [a, b] | row?, cols?: [a, b] | col? (the target cells; default: the selection)} (0-based; from defaults to the target row) — like dragging a row", in_table, |s, p| move_rc(s, p, true) ), - cmd!("table.moveColumn", "Move Column", [], None, "{from?, to} (0-based; from defaults to the target column)", in_table, |s, p| move_rc( - s, p, false - )), + cmd!( + "table.moveColumn", + "Move Column", + [], + None, + "{from?, to, story?, table?, rows?: [a, b] | row?, cols?: [a, b] | col? (the target cells; default: the selection)} (0-based; from defaults to the target column)", + in_table, + |s, p| move_rc(s, p, false) + ), cmd!( "table.sortRows", "Sort", ["Table"], None, - "{column?: 0-based (default: the target cell's), descending?: bool} — body rows by that column's text (numbers numerically)", + "{column?: 0-based (default: the target cell's), descending?: bool, story?, table?, rows?: [a, b] | row?, cols?: [a, b] | col? (the target cells; default: the selection)} — body rows by that column's text (numbers numerically)", in_table, sort_rows ), - cmd!("table.unmerge", "Unmerge Cells", ["Table"], None, "{}", in_table, unmerge), + cmd!( + "table.unmerge", + "Unmerge Cells", + ["Table"], + None, + "{story?, table?, rows?: [a, b] | row?, cols?: [a, b] | col? (the target cells; default: the selection)}", + in_table, + unmerge + ), cmd!( "table.splitHorizontally", "Split Cell Horizontally", ["Table"], None, - "{} — each target cell becomes two, one above the other", + "{story?, table?, rows?: [a, b] | row?, cols?: [a, b] | col? (the target cells; default: the selection)} — each target cell becomes two, one above the other", in_table, |s, p| split(s, p, true) ), @@ -187,7 +289,7 @@ fn table_specs() -> Vec { "Split Cell Vertically", ["Table"], None, - "{} — each target cell becomes two, side by side", + "{story?, table?, rows?: [a, b] | row?, cols?: [a, b] | col? (the target cells; default: the selection)} — each target cell becomes two, side by side", in_table, |s, p| split(s, p, false) ), @@ -196,19 +298,43 @@ fn table_specs() -> Vec { "Cell Options", [], None, - "{fill?: swatch, tint?, insets?: n | [t,l,b,r], vj?: top|center|bottom|justify, text?, stroke?: {weight?, color?, tint?, type?: solid|dashed|dotted, edges?: all|outer|inner|top|left|bottom|right}}", + "{fill?: swatch, tint?, insets?: n | [t,l,b,r], vj? | verticalJustification?: top|center|bottom|justify, text?, stroke?: {weight?, color?, tint?, type?: solid|dashed|dotted, edges?: all|outer|inner|top|left|bottom|right}, story?, table?, rows?: [a, b] | row?, cols?: [a, b] | col? (the target cells; default: the selection)}", in_table, set_cell ), - cmd!("table.setRowHeight", "Row Height", [], None, "{height, mode?: atLeast|exactly}", in_table, set_row_height), - cmd!("table.setColumnWidth", "Column Width", [], None, "{width}", in_table, set_col_width), - cmd!("table.distributeColumns", "Distribute Columns Evenly", ["Table"], None, "{}", in_table, distribute_cols), + cmd!( + "table.setRowHeight", + "Row Height", + [], + None, + "{height, mode?: atLeast|exactly, story?, table?, rows?: [a, b] | row?, cols?: [a, b] | col? (the target cells; default: the selection)}", + in_table, + set_row_height + ), + cmd!( + "table.setColumnWidth", + "Column Width", + [], + None, + "{width, story?, table?, rows?: [a, b] | row?, cols?: [a, b] | col? (the target cells; default: the selection)}", + in_table, + set_col_width + ), + cmd!( + "table.distributeColumns", + "Distribute Columns Evenly", + ["Table"], + None, + "{story?, table?, rows?: [a, b] | row?, cols?: [a, b] | col? (the target cells; default: the selection)}", + in_table, + distribute_cols + ), cmd!( "table.distributeRows", "Distribute Rows Evenly", ["Table"], None, - "{story?, table?, rows?: [a,b]} — equal fixed heights preserving the selected rows' composed total height; may overset cell text; unmerge row-spanning cells first", + "{story?, table?, rows?: [a,b] | row?, cols?: [a,b] | col?} — equal fixed heights preserving the selected rows' composed total height; may overset cell text; unmerge row-spanning cells first", in_table, distribute_rows ), @@ -217,23 +343,23 @@ fn table_specs() -> Vec { "Table Options", [], None, - "{direction?: leftToRight|rightToLeft, border?: {weight?, color?, tint?, type?}, spaceBefore?, spaceAfter?, headerRows?, footerRows?, repeatHeader?, repeatFooter?, altRows?: {first, firstColor, firstTint, next, nextColor, nextTint, skipFirst, skipLast} | null, altCols?: … | null}", + "{direction?: leftToRight|rightToLeft, border?: {weight?, color?, tint?, type?}, spaceBefore?, spaceAfter?, headerRows?, footerRows?, repeatHeader?, repeatFooter?, altRows?: {first, firstColor, firstTint, next, nextColor, nextTint, skipFirst, skipLast} | null, altCols?: … | null, story?, table?, rows?: [a, b] | row?, cols?: [a, b] | col? (the target cells; default: the selection)}", in_table, options ), - cmd!(noundo "table.select", "Select Cells", [], None, "{story?, table?, rows?: [a,b], cols?: [a,b], what?: cell|row|column|table}", in_table_or_ids, select), - cmd!(noundo "table.selectTable", "Select Table", ["Table", "Select"], None, "{}", in_table, |s, p| { + cmd!(noundo "table.select", "Select Cells", [], None, "{story?, table?, rows?: [a,b] | row?, cols?: [a,b] | col?, what?: cell|row|column|table}", in_table_or_ids, select), + cmd!(noundo "table.selectTable", "Select Table", ["Table", "Select"], None, "{story?, table?, rows?: [a, b] | row?, cols?: [a, b] | col? (the target cells; default: the selection)}", in_table, |s, p| { select(s, &super::with_param(p, "what", json!("table"))) }), - cmd!(noundo "table.selectRow", "Select Row", ["Table", "Select"], None, "{}", in_table, |s, p| { + cmd!(noundo "table.selectRow", "Select Row", ["Table", "Select"], None, "{story?, table?, rows?: [a, b] | row?, cols?: [a, b] | col? (the target cells; default: the selection)}", in_table, |s, p| { select(s, &super::with_param(p, "what", json!("row"))) }), - cmd!(noundo "table.selectColumn", "Select Column", ["Table", "Select"], None, "{}", in_table, |s, p| { + cmd!(noundo "table.selectColumn", "Select Column", ["Table", "Select"], None, "{story?, table?, rows?: [a, b] | row?, cols?: [a, b] | col? (the target cells; default: the selection)}", in_table, |s, p| { select(s, &super::with_param(p, "what", json!("column"))) }), cmd!(noundo "table.nextCell", "Next Cell", [], None, "{}", in_cell, |s, _| step_cell(s, true)), cmd!(noundo "table.prevCell", "Previous Cell", [], None, "{}", in_cell, |s, _| step_cell(s, false)), - cmd!(query "table.get", "Get Table", [], None, "{story?, table?} → rows, columns, cells (text, spans, fill), options", in_table_or_ids, get), + cmd!(query "table.get", "Get Table", [], None, "{story?, table?, rows?: [a,b] | row?, cols?: [a,b] | col?} → rows, columns, cells (text, spans, fill), options", in_table_or_ids, get), ] } diff --git a/crates/engine/src/cmd/text.rs b/crates/engine/src/cmd/text.rs index b57e9e88a..0911a3657 100644 --- a/crates/engine/src/cmd/text.rs +++ b/crates/engine/src/cmd/text.rs @@ -68,7 +68,7 @@ pub fn specs() -> Vec { "Story Column Direction", ["Type", "Story Column Direction"], None, - "{story?, direction: leftToRight|rightToLeft} — column progression, independent of paragraph direction", + "{story?, frame?: a text frame of the story, direction: leftToRight|rightToLeft} — column progression, independent of paragraph direction", has_doc, |s, p| { let sid = story_of(s, p).ok_or_else(|| bad("story.setDirection", "no story"))?; @@ -129,7 +129,7 @@ pub fn specs() -> Vec { v["lines"] = json!(cs.line_count()); Ok(v) }), - cmd!("type.fillWithPlaceholder", "Fill with Placeholder Text", ["Type"], None, "{frame?}", has_text_or_frames, fill_placeholder), + cmd!("type.fillWithPlaceholder", "Fill with Placeholder Text", ["Type"], None, "{frame?, story?}", has_text_or_frames, fill_placeholder), cmd!( "type.char", "Character Formatting", diff --git a/crates/engine/src/cmd/xref.rs b/crates/engine/src/cmd/xref.rs index b0b97ebd2..284c820bc 100644 --- a/crates/engine/src/cmd/xref.rs +++ b/crates/engine/src/cmd/xref.rs @@ -13,7 +13,7 @@ pub fn specs() -> Vec { "Insert Cross-Reference…", ["Type", "Hyperlinks & Cross-References"], None, - "{anchor? | paragraph?: text to find (first paragraph containing it) | story? + para? (0-based), format?: name (default Full Paragraph & Page Number)} — at the insertion point; the text updates itself as the destination moves", + "{anchor? | paragraph?: text to find (first paragraph containing it) | story?: with para? | para?: 0-based paragraph of `story`, format?: name (default Full Paragraph & Page Number)} — at the insertion point; the text updates itself as the destination moves", in_story_text, insert ), diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index caeeb80ba..f7902ab5f 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -25,6 +25,7 @@ use designcraft_compose::Cache; use designcraft_doc::{Document, LayerId, Selection}; use serde_json::Value; +pub use cmd::params::{check_keys, check_params}; pub use cmd::{CommandInfo, CommandSpec, command_specs, find_command}; pub use designcraft_compose as compose; pub use designcraft_doc as doc; @@ -402,6 +403,10 @@ impl Session { fn execute_unguarded(&mut self, id: &str, params: &Value) -> Result { let spec = find_command(id).ok_or_else(|| EngineError::UnknownCommand(id.into()))?; + // The crate's own tests are the audit of the params descriptions (#776): a key a test + // passes that its command's description doesn't name fails the test. + #[cfg(test)] + check_params(id, params)?; if let Err(e) = (spec.enabled)(self) { // A command that takes `ids` acts on the objects the call names, so it needs no // selection: with a text caret or nothing selected, `{"ids": [5]}` still runs. @@ -561,4 +566,6 @@ mod tests; #[cfg(test)] mod tests_idml; #[cfg(test)] +mod tests_params; +#[cfg(test)] mod tests_table; diff --git a/crates/engine/src/tests_params.rs b/crates/engine/src/tests_params.rs new file mode 100644 index 000000000..19625d90c --- /dev/null +++ b/crates/engine/src/tests_params.rs @@ -0,0 +1,106 @@ +//! The params descriptions name the keys their commands accept (#776). + +use serde_json::json; + +use crate::cmd::params::{UNCHECKED, check_keys, check_params, doc_keys}; +use crate::{EngineError, Session, command_specs, find_command}; + +fn keys(doc: &str) -> Vec<&str> { + doc_keys(doc).unwrap().keys +} + +#[test] +fn a_description_names_its_keys() { + assert_eq!(keys("{} → {id}"), Vec::<&str>::new()); + assert_eq!( + keys("{x, y, width?: pt=612|\"210 mm\", sides=6, name?|title?: a name (default: Untitled-N)} → {index}"), + ["x", "y", "width", "sides", "name", "title"] + ); + // Brackets, quotes and backticks keep their commas and bars; a nested object isn't listed. + assert_eq!( + keys("{at: [x, y] (page, not view), stops: [{position: pt, leader?: \", \"}], csv?: a `|, ` in a label is a break, on?: bool}"), + ["at", "stops", "csv", "on"] + ); + // Alternatives after a note are keys when they have their own `?` or `:`; else values. + assert_eq!( + keys("{steps?: n | distance?: pt | smooth?: bool, spacing?: smooth|steps|distance, path? | data?: text | dataBase64?}"), + ["steps", "distance", "smooth", "spacing", "path", "data", "dataBase64"] + ); + assert_eq!(keys("{latex, size?} or {id, latex}"), ["latex", "size", "id", "latex"]); + let more = doc_keys("{name, …same as style.cell.create, …the format's options}").unwrap(); + assert_eq!((more.keys, more.more), (vec!["name"], vec!["same as style.cell.create", "the format's options"])); + for bad in ["same as paint.setFill", "{a, b", "{opacity? 0..1}", "{path | name+dataBase64}", "{xRelative?/yRelative?: anchor}"] { + assert!(doc_keys(bad).is_err(), "{bad}"); + } +} + +/// Every command's description reads as a params object, so none is left unchecked by accident. +#[test] +fn every_description_parses() { + let bad: Vec = command_specs().iter().filter_map(|c| doc_keys(c.params).err().map(|e| format!("{}: {e}", c.id))).collect(); + assert!(bad.is_empty(), "{} descriptions don't parse:\n{}", bad.len(), bad.join("\n")); + let stale: Vec<_> = UNCHECKED.iter().filter(|id| find_command(id).is_none()).collect(); + assert!(stale.is_empty() && UNCHECKED.is_sorted(), "UNCHECKED: sorted ids of commands; not commands: {stale:?}"); +} + +fn refusal(id: &str, params: serde_json::Value) -> String { + match check_params(id, ¶ms) { + Err(EngineError::BadParams { cmd, msg }) if cmd == id => msg, + other => panic!("{id} {params}: {other:?}"), + } +} + +/// The examples of #776: both ran and did nothing (or the default) without a word. +#[test] +fn the_issue_examples_are_refused() { + // (The keys are checked in order: `end` comes before `start`.) + assert_eq!(refusal("text.select", json!({"story": 1, "start": 0, "end": 5})), "unknown parameter `end`; the command takes: story, anchor, focus"); + assert_eq!(refusal("text.select", json!({"story": 1, "start": 0})), "unknown parameter `start`; the command takes: story, anchor, focus"); + assert_eq!(refusal("style.list", json!({"bogus": 1})), "unknown parameter `bogus`; the command takes no parameters"); +} + +#[test] +fn an_unknown_key_is_named_with_the_nearest_accepted_one() { + assert_eq!(refusal("file.new", json!({"widht": 300})), "unknown parameter `widht`; did you mean `width`?"); + assert_eq!(refusal("file.new", json!({"Width": 300})), "unknown parameter `Width`; did you mean `width`?"); + assert_eq!( + refusal("line.create", json!({"a": [0, 0], "b": [5, 5], "colour": "red"})), + "unknown parameter `colour`; the command takes: spread, a, b" + ); + for (id, ok) in [ + ("file.new", json!({"width": 300, "title": "a", "margins": {"anything": 1}})), + ("file.new", json!({})), + ("file.new", json!(null)), + // A command that acts on `ids` takes one object as `id` too. + ("transform.move", json!({"dx": 1, "dy": 1, "id": 3})), + // Keys listed by reference. + ("style.cell.edit", json!({"name": "a", "fill": "Paper"})), + // On UNCHECKED: the attributes may be the params object itself. + ("type.char", json!({"size": 12})), + // Not a command: running it says so. + ("no.such", json!({"x": 1})), + ] { + assert!(check_params(id, &ok).is_ok(), "{id} {ok}"); + } + assert!(check_keys("x", "{a, …same as file.new}", &json!({"a": 1, "width": 2})).is_ok()); + assert!(check_keys("x", "{a, …same as file.new}", &json!({"b": 1})).is_err()); + // A `…` naming no command stands for any key. + assert!(check_keys("x", "{a, …the format's options}", &json!({"b": 1})).is_ok()); +} + +/// [`Session::execute`] itself takes what it is given (old journals replay); only callers from +/// outside (control channel, MCP, CLI) are checked. In this crate's tests it checks too, as the +/// audit of the descriptions, so this goes through a journal entry instead. +#[test] +fn journal_entries_pass_the_check() { + let mut s = Session::new(); + s.execute("file.new", &json!({})).unwrap(); + let f = s.execute("frame.create", &json!({"rect": [36, 36, 300, 200], "content": "text", "text": "Hello world"})).unwrap(); + s.execute("text.select", &json!({"story": f["story"], "anchor": 0, "focus": 5})).unwrap(); + s.execute("type.char", &json!({"attrs": {"size": 24}})).unwrap(); + s.execute("transform.move", &json!({"dx": 10, "dy": 0, "ids": [f["id"]]})).unwrap(); + assert!(s.journal.len() >= 4); + for (id, params) in &s.journal { + assert!(check_params(id, params).is_ok(), "{id} {params}"); + } +} diff --git a/crates/engine/tests/param_keys.rs b/crates/engine/tests/param_keys.rs new file mode 100644 index 000000000..96f4c8360 --- /dev/null +++ b/crates/engine/tests/param_keys.rs @@ -0,0 +1,14 @@ +//! #776: callers from outside are checked with `check_params`; `Session::execute` itself takes +//! what it is given, so a journal written by an older version still replays. + +use designcraft_engine::{Session, check_params}; +use serde_json::json; + +#[test] +fn execute_replays_what_the_check_refuses() { + let mut s = Session::new(); + s.execute("file.new", &json!({})).unwrap(); + let params = json!({"bogus": 1}); + assert!(check_params("style.list", ¶ms).is_err()); + assert!(s.execute("style.list", ¶ms).is_ok()); +} diff --git a/crates/mcp/src/headless.rs b/crates/mcp/src/headless.rs index de0353575..ccc2a5d2b 100644 --- a/crates/mcp/src/headless.rs +++ b/crates/mcp/src/headless.rs @@ -95,6 +95,8 @@ impl Headless { } fn exec(&mut self, id: &str, params: &Value) -> Result { + // A key the command's description doesn't name is an error, not a default (#776). + designcraft_engine::check_params(id, params).map_err(|e| e.to_string())?; let r = self.session.execute(id, params).map_err(|e| { let ui_only = ["app.", "view.", "window."].iter().any(|p| id.starts_with(p)); if ui_only && designcraft_engine::find_command(id).is_none() { format!("`{id}` is a UI command; {NEEDS_APP}") } else { e.to_string() } diff --git a/crates/mcp/src/server.rs b/crates/mcp/src/server.rs index cd7bffda4..328df76d3 100644 --- a/crates/mcp/src/server.rs +++ b/crates/mcp/src/server.rs @@ -149,6 +149,7 @@ impl Server { "tools/call" => { let name = params.get("name").and_then(Value::as_str).ok_or((INVALID_PARAMS, "missing tool `name`".to_string()))?; let args = params.get("arguments").cloned().unwrap_or(Value::Null); + check_arguments(name, &args).map_err(|m| (INVALID_PARAMS, m))?; Ok(call_tool(self.backend.as_mut(), name, &args).to_value()) } "resources/list" => Ok(json!({"resources": [ @@ -170,3 +171,41 @@ impl Server { } } } + +/// Arguments a tool doesn't take, and command params a command doesn't take, are invalid params +/// (#776): a misspelt key is refused instead of being ignored. `execute` and `batch` are checked +/// against the params descriptions of the commands they run, every step before the first runs. +fn check_arguments(name: &str, args: &Value) -> Result<(), String> { + let Some(arguments) = args.as_object() else { return Ok(()) }; + if let Some(def) = tool_definitions().into_iter().find(|t| t.get("name").and_then(Value::as_str) == Some(name)) + && let Some(properties) = def.get("inputSchema").and_then(|s| s.get("properties")).and_then(Value::as_object) + && let Some(key) = arguments.keys().find(|k| !properties.contains_key(*k)) + { + return Err(format!("unknown argument `{key}` for `{name}`")); + } + let command = |step: &Value| -> Result<(), String> { + match (step.get("command").and_then(Value::as_str), step.get("params")) { + (Some(id), Some(params)) => designcraft_engine::check_params(id, params).map_err(|e| e.to_string()), + _ => Ok(()), + } + }; + match name { + "execute" => command(args), + "batch" => { + let steps = match (arguments.get("commands").and_then(Value::as_array), arguments.get("script").and_then(Value::as_str)) { + (Some(steps), _) => steps.clone(), + // A script that doesn't parse is the tool's to report. + (None, Some(text)) => match designcraft_engine::script::parse(text) { + Ok(steps) => steps.into_iter().map(|s| json!({"command": s.command, "params": s.params})).collect(), + Err(_) => return Ok(()), + }, + (None, None) => return Ok(()), + }; + for (i, step) in steps.iter().enumerate() { + command(step).map_err(|e| format!("step {i}: {e}"))?; + } + Ok(()) + } + _ => Ok(()), + } +} diff --git a/crates/mcp/src/tests.rs b/crates/mcp/src/tests.rs index 3051a5ad4..2ce8088f7 100644 --- a/crates/mcp/src/tests.rs +++ b/crates/mcp/src/tests.rs @@ -500,3 +500,41 @@ fn explicit_typing_rejects_stale_selection_and_closed_document() { b.session.execute("file.close", &json!({})).unwrap(); assert!(b.call("ui.text", json!({"text":"X"})).is_err()); } + +/// #776: a key a command (or a tool) doesn't take is an invalid-params error, not ignored. +#[test] +fn unknown_params_are_invalid_params() { + let mut s = server(); + let code_and_message = |v: &Value| (v["error"]["code"].as_i64(), v["error"]["message"].as_str().unwrap_or("").to_string()); + let f = ok(&mut s, "execute", json!({"command": "frame.create", "params": {"rect": [36, 36, 300, 200], "content": "text", "text": "Hello"}})); + let v = rpc( + &mut s, + 1, + "tools/call", + json!({"name": "execute", "arguments": {"command": "text.select", "params": {"story": f["story"], "start": 0, "end": 5}}}), + ); + let (code, msg) = code_and_message(&v); + assert_eq!(code, Some(-32602), "{v}"); + assert!(msg.contains("unknown parameter `end`; the command takes: story, anchor, focus"), "{msg}"); + let v = rpc(&mut s, 2, "tools/call", json!({"name": "execute", "arguments": {"command": "style.list", "params": {"bogus": 1}}})); + let (code, msg) = code_and_message(&v); + assert_eq!(code, Some(-32602), "{v}"); + assert!(msg.contains("unknown parameter `bogus`; the command takes no parameters"), "{msg}"); + // A batch is checked step by step before any step runs: the frame isn't created. + let before = ok(&mut s, "inspect_document", json!({})); + for args in [ + json!({"commands": [{"command": "frame.create", "params": {"rect": [0, 0, 50, 50]}}, {"command": "file.new", "params": {"widht": 300}}]}), + json!({"script": "frame.create {\"rect\": [0, 0, 50, 50]}\nfile.new {\"widht\": 300}"}), + ] { + let v = rpc(&mut s, 3, "tools/call", json!({"name": "batch", "arguments": args})); + let (code, msg) = code_and_message(&v); + assert_eq!(code, Some(-32602), "{v}"); + assert!(msg.contains("step 1") && msg.contains("did you mean `width`?"), "{msg}"); + } + assert_eq!(ok(&mut s, "inspect_document", json!({})), before); + // An argument the tool doesn't take. + let v = rpc(&mut s, 4, "tools/call", json!({"name": "execute", "arguments": {"command": "style.list", "parms": {}}})); + assert_eq!(code_and_message(&v), (Some(-32602), "unknown argument `parms` for `execute`".to_string())); + // The right keys still run. + ok(&mut s, "execute", json!({"command": "text.select", "params": {"story": f["story"], "anchor": 0, "focus": 5}})); +} diff --git a/crates/tools/src/pen.rs b/crates/tools/src/pen.rs index 21e90f734..354064359 100644 --- a/crates/tools/src/pen.rs +++ b/crates/tools/src/pen.rs @@ -117,7 +117,7 @@ impl PenTool { match self.path { None if self.anchors.len() >= 2 => { let anchors: Vec = self.anchors.iter().map(anchor_json).collect(); - vec![Action::Exec("path.create".into(), json!({"spread": spread_json(sr), "anchors": anchors, "closed": false, "notifyPen": true}))] + vec![Action::Exec("path.create".into(), json!({"spread": spread_json(sr), "anchors": anchors, "closed": false}))] } Some(id) => { let mut p = json!({"id": id, "anchor": anchor_json(last)}); diff --git a/crates/ui-egui/src/control.rs b/crates/ui-egui/src/control.rs index 5a4de0058..76f01e4a2 100644 --- a/crates/ui-egui/src/control.rs +++ b/crates/ui-egui/src/control.rs @@ -114,6 +114,10 @@ pub fn handle(app: &mut DesignApp, ctx: &egui::Context, req: &ControlRequest) -> let Some(id) = s("command").or(s("id")) else { return err("missing `command`") }; let params = p.get("params").cloned().unwrap_or(json!({})); let params = if params.is_null() { json!({}) } else { params }; + // A key the command's description doesn't name is an error, not a default (#776). + if let Err(e) = designcraft_engine::check_params(id, ¶ms) { + return err(e.to_string()); + } wrap(app.run(id, params)) } "engine.commands" => ok(all_commands(app)), diff --git a/crates/ui-egui/src/dialogs.rs b/crates/ui-egui/src/dialogs.rs index 695a66af1..147b071bf 100644 --- a/crates/ui-egui/src/dialogs.rs +++ b/crates/ui-egui/src/dialogs.rs @@ -4312,7 +4312,8 @@ mod tests { assert_eq!(page.margins.top, 10.0); assert_eq!(page.margins.outside, 40.0); } - assert!(doc.spreads.iter().all(|s| s.items.is_empty()), "undocumented fields stay excluded"); + // `primaryTextFrame` is documented since #776, so the dialog passes it on (`status` isn't). + assert!(doc.spreads.iter().any(|s| !s.items.is_empty()), "documented fields reach the command"); assert!(app.ui.dialog.is_none()); } } diff --git a/docs/control-protocol.md b/docs/control-protocol.md index 9e5145616..4b889e351 100644 --- a/docs/control-protocol.md +++ b/docs/control-protocol.md @@ -31,7 +31,7 @@ so only enable it while you use it. Transport: `apps/designcraft/src/control_ser | Method | Params | What it does | |---|---|---| -| `engine.execute` / `ui.menu.invoke` | `{command, params}` | Run any engine or UI command (see `engine.commands`). `ui.menu.invoke` without `params` acts like choosing the menu item: a command labelled "…" that takes parameters opens its dialog | +| `engine.execute` / `ui.menu.invoke` | `{command, params}` | Run any engine or UI command (see `engine.commands`). `ui.menu.invoke` without `params` acts like choosing the menu item: a command labelled "…" that takes parameters opens its dialog. A key the command's params description doesn't name is an error (`unknown parameter …`), not ignored | | `engine.commands` | — | Every command: id, label, menu path, shortcut, params doc, enabled | | `document.inspect` | — | Pages, spreads, items, stories (overset), styles, swatches, selection | | `ui.inspect` | — | Tool, UI state, view (zoom/origin), canvas rect, perf | diff --git a/docs/mcp.md b/docs/mcp.md index f35585746..af05aea59 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -76,7 +76,7 @@ and its second character starts at 3. An offset inside a character snaps back to | Tool | What it does | |---|---| | `list_commands` | Every command (id, label, menu path, shortcut, params, enabled), with `filter` / `enabledOnly` | -| `execute` | Run any command: `{command, params}` | +| `execute` | Run any command: `{command, params}`. A params key the command doesn't take is a JSON-RPC `-32602` (invalid params) error naming it, as is an argument a tool doesn't take; `batch` checks every step before running the first | | `batch` | Run `commands: [{command, params}]` (or `script` text, one `command.id {json}` per line) in order, stopping at the first error (reports `failedIndex`); `"$N.path"` parameters use earlier results ([agents.md](agents.md)) | | `inspect_document` | Pages, spreads, items (ids, bounds, fill, story), stories (overset), layers, styles, swatches, selection | | `get_story` | Text, frames, paragraphs, lines, overset of a story (`story` or text `frame`) |