diff --git a/crates/mcp/src/tests_drawopts.rs b/crates/mcp/src/tests_drawopts.rs index 666ff1632..bef4e59b8 100644 --- a/crates/mcp/src/tests_drawopts.rs +++ b/crates/mcp/src/tests_drawopts.rs @@ -138,3 +138,45 @@ fn schemas_list_the_options() { assert_eq!(v["result"]["isError"], false, "{tool}: {v}"); } } + +/// A paint the engine refuses (a fill or stroke it can't read, a swatch the document lacks) fails +/// the whole call: no object, no undo step, nothing in the SVG (#3545). +#[test] +fn a_bad_paint_draws_nothing() { + let mut h = headless(); + let (before, undo, svg) = + (objects(&h), h.session.doc().unwrap().history.undo.len(), vectorcraft_svg::export(&h.session.doc().unwrap().doc, &Default::default())); + for bad in [ + json!({"stroke": "currentColor"}), + json!({"fill": "currentColor"}), + json!({"fill": "#550505", "stroke": "not-a-colour"}), + json!({"fill": {"swatch": "No Such Swatch"}}), + json!({"stroke": "#000000", "strokeWidth": "thick"}), + ] { + for (tool, base) in + [("draw_path", json!({"d": "M0 0 L10 10"})), ("draw_shape", json!({"shape": "rectangle", "x": 0, "y": 0, "width": 10, "height": 10}))] + { + let mut args = base.clone(); + for (k, v) in bad.as_object().unwrap() { + args[k] = v.clone(); + } + let r = call_tool(&mut h, tool, &args); + assert!(r.is_error, "{tool} {args} must fail"); + let st = h.session.doc().unwrap(); + assert_eq!(objects(&h), before, "{tool} {args} drew something"); + assert_eq!(st.history.undo.len(), undo, "{tool} {args} left an undo step"); + assert_eq!(vectorcraft_svg::export(&st.doc, &Default::default()), svg, "{tool} {args} changed the SVG"); + } + } + // A good call is one undo step, named as the drawing command names its own. + for (tool, args, label) in [ + ("draw_path", json!({"d": "M0 0 L10 10", "stroke": "#c19a4b", "strokeWidth": 2, "opacity": 50}), "Pen"), + ("draw_shape", json!({"shape": "rectangle", "x": 0, "y": 0, "width": 10, "height": 10, "fill": "#000000", "stroke": "none"}), "Rectangle"), + ] { + let undo = h.session.doc().unwrap().history.undo.len(); + draw(&mut h, tool, args); + let history = &h.session.doc().unwrap().history.undo; + assert_eq!(history.len(), undo + 1, "{tool}"); + assert_eq!(history.last().unwrap().label, label); + } +} diff --git a/crates/mcp/src/tools.rs b/crates/mcp/src/tools.rs index 7587d67aa..3f37bce16 100644 --- a/crates/mcp/src/tools.rs +++ b/crates/mcp/src/tools.rs @@ -528,20 +528,19 @@ fn paint_params(v: &Value) -> Result { }) } -fn apply_paint(b: &mut dyn Backend, a: &Args, ids: Option) -> Result { - let with_ids = |mut p: Value| { - if let Some(ids) = &ids { - p["ids"] = ids.clone(); - } - p - }; - let mut applied = Map::new(); +/// A command a drawing tool runs and its params. +type Step<'a> = (&'a str, Value); + +/// The commands that apply `a`'s `fill`, `stroke`, `strokeWidth`, `dash`, `cap` and `join`, without +/// targets, and what each applies. +fn paint_steps(a: &Args) -> Result<(Vec>, Map), String> { + let (mut steps, mut applied) = (vec![], Map::new()); if let Some(f) = a.get("fill") { - exec(b, "paint.setFill", with_ids(paint_params(f)?))?; + steps.push(("paint.setFill", paint_params(f)?)); applied.insert("fill".into(), f.clone()); } if let Some(s) = a.get("stroke") { - exec(b, "paint.setStroke", with_ids(paint_params(s)?))?; + steps.push(("paint.setStroke", paint_params(s)?)); applied.insert("stroke".into(), s.clone()); } // Weight, dash, cap and join go to one stroke.set. @@ -558,7 +557,18 @@ fn apply_paint(b: &mut dyn Backend, a: &Args, ids: Option) -> Result) -> Result { + let (steps, applied) = paint_steps(a)?; + for (cmd, mut p) in steps { + if let Some(ids) = &ids { + p["ids"] = ids.clone(); + } + exec(b, cmd, p)?; } Ok(Value::Object(applied)) } @@ -609,9 +619,18 @@ fn check_draw_options(b: &mut dyn Backend, a: &Args) -> Result<(), String> { Ok(()) } -/// Applies the checked options of [`check_draw_options`] to the new object `ids`, adding what was -/// applied to `made`. Paint and stroke options are [`apply_paint`]'s. -fn apply_draw_options(b: &mut dyn Backend, a: &Args, ids: Option, made: &mut Value) -> Result<(), String> { +/// Draws an object with `create` (a creating command and its params) and styles it with `a`'s +/// paint and draw options ([`check_draw_options`], checked first) as ONE `command.batch`, named +/// `label` (the undo step the creating command makes on its own): any step the engine refuses — a +/// colour it can't read, a swatch the document lacks — rolls the whole call back, so it leaves no +/// object and no undo step behind (#3545). The styling steps act on the new object, which the +/// creating command selects, and on its whole appearance (`item: null`) → `made` with the new +/// object's `id`, the `paint` applied and the other options applied. +fn draw(b: &mut dyn Backend, a: &Args, label: &str, create: Step<'_>, mut made: Value) -> Result { + check_draw_options(b, a)?; + let (paint, applied) = paint_steps(a)?; + let mut steps = vec![create]; + steps.extend(paint); let mut props = json!({}); for k in ["name", "opacity"] { if let Some(v) = a.get(k) { @@ -619,34 +638,40 @@ fn apply_draw_options(b: &mut dyn Backend, a: &Args, ids: Option, made: & made[k] = v.clone(); } } - if let Some(ids) = &ids { - props["ids"] = ids.clone(); - } if a.contains_key("name") || a.contains_key("opacity") { - exec(b, "object.setProps", props)?; + steps.push(("object.setProps", props)); } - let effects = a.get("effects").and_then(Value::as_array).filter(|e| !e.is_empty()); - if let Some(effects) = effects { - let mut applied = vec![]; - for e in effects { - // The whole object (item: null), as the Effect menu applies it. - let mut p = json!({"effect": e.get("effect"), "params": e.get("params").cloned().unwrap_or(json!({})), "item": null}); - if let Some(ids) = &ids { - p["ids"] = ids.clone(); + // The fill, stroke and effects go to the new object's whole appearance (item: null), as the + // Fill and Stroke proxies and the Effect menu apply them to a selected object. + let effects = a.get("effects").and_then(Value::as_array).map(Vec::as_slice).unwrap_or_default(); + for e in effects { + steps.push(("effect.apply", json!({"effect": e.get("effect"), "params": e.get("params").cloned().unwrap_or(json!({}))}))); + } + let commands: Vec = steps + .into_iter() + .map(|(cmd, mut p)| { + if matches!(cmd, "paint.setFill" | "paint.setStroke" | "stroke.set" | "effect.apply") { + p["item"] = Value::Null; } - applied.push(exec(b, "effect.apply", p)?); - } - made["effects"] = Value::Array(applied); + json!({"command": cmd, "params": p}) + }) + .collect(); + // The engine names the failed step of its batch; the caller sent no batch, so the reply gives + // the step's own error. + let r = exec(b, "command.batch", json!({"label": label, "commands": commands})).map_err(|e| match e.split_once(" failed: ") { + Some((step, error)) if step.starts_with("batch step ") => error.to_string(), + _ => e, + })?; + let results = r.get("results").and_then(Value::as_array).ok_or("command.batch returned no results")?; + made["id"] = results.first().and_then(|r| r.get("id")).cloned().unwrap_or(Value::Null); + made["paint"] = Value::Object(applied); + if !effects.is_empty() { + made["effects"] = Value::Array(results.iter().skip(results.len().saturating_sub(effects.len())).cloned().collect()); } - Ok(()) -} - -fn created_id(r: &Value) -> Option { - r.get("id").filter(|v| v.is_u64()).map(|id| json!([id])) + Ok(made) } fn draw_path(b: &mut dyn Backend, a: &Args) -> Result { - check_draw_options(b, a)?; let mut p = json!({}); if let Some(d) = a.get("d").and_then(Value::as_str) { p["d"] = json!(d); @@ -667,33 +692,26 @@ fn draw_path(b: &mut dyn Backend, a: &Args) -> Result { p["anchors"] = Value::Array(anchors); p["closed"] = json!(a.get("closed").and_then(Value::as_bool).unwrap_or(false)); } - let r = exec(b, "path.create", p)?; - let paint = apply_paint(b, a, created_id(&r))?; - let mut made = json!({"id": r.get("id"), "paint": paint}); - apply_draw_options(b, a, created_id(&r), &mut made)?; - Ok(made) + draw(b, a, "Pen", ("path.create", p), json!({})) } fn draw_shape(b: &mut dyn Backend, a: &Args) -> Result { let shape = req_str(a, "shape")?; - let cmd = match shape { - "rectangle" | "rect" => "shape.rectangle", - "ellipse" | "circle" => "shape.ellipse", - "polygon" => "shape.polygon", - "star" => "shape.star", - "line" => "shape.line", + // Each command and the name of the undo step it makes. + let (cmd, label) = match shape { + "rectangle" | "rect" if a.get("radius").and_then(Value::as_f64).is_some_and(|r| r > 0.0) => ("shape.rectangle", "Rounded Rectangle"), + "rectangle" | "rect" => ("shape.rectangle", "Rectangle"), + "ellipse" | "circle" => ("shape.ellipse", "Ellipse"), + "polygon" => ("shape.polygon", "Polygon"), + "star" => ("shape.star", "Star"), + "line" => ("shape.line", "Line"), other => return Err(format!("unknown shape `{other}` (rectangle, ellipse, polygon, star, line)")), }; - check_draw_options(b, a)?; let mut p = a.clone(); for k in ["shape", "fill", "stroke", "strokeWidth", "dash", "cap", "join", "opacity", "name", "effects"] { p.remove(k); } - let r = exec(b, cmd, Value::Object(p))?; - let paint = apply_paint(b, a, created_id(&r))?; - let mut made = json!({"id": r.get("id"), "shape": shape, "paint": paint}); - apply_draw_options(b, a, created_id(&r), &mut made)?; - Ok(made) + draw(b, a, label, (cmd, Value::Object(p)), json!({"shape": shape})) } /// Select `ids` when given (tools that act on the selection). diff --git a/docs/mcp.md b/docs/mcp.md index 750e3e11e..95bbcfda7 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -332,7 +332,9 @@ also style the new object in the same call: `dash` (lengths, or null for solid), (butt|round|square) and `join` (miter|round|bevel) go to `stroke.set` with `strokeWidth`; `opacity` (0–100 %) and `name` to `object.setProps`; `effects: [{effect, params?}]` are applied in order as `apply_effect` does, to the whole object. They are checked before anything is drawn (effect ids against `effect.list`), -so a bad one draws nothing. The reply adds `name`, `opacity` and `effects` (each `effect.apply` result) +so a bad one draws nothing. The object and its styling are one `command.batch`, so the call is one undo step +(named as the drawing command names its own, e.g. Pen or Rectangle) and a paint the engine refuses (a colour +it can't read, a swatch the document lacks) fails the call and leaves the document as it was. The reply adds `name`, `opacity` and `effects` (each `effect.apply` result) when given; `paint` lists the stroke options applied. A gradient given to several objects fits each one's own bounds; `paint.setFill` / `paint.setStroke` with `span: "selection"` place a linear or radial gradient once on the union of the targets' bounds instead, so one