aleleba/vectorcraft
1
0
Fork 0
mirror of https://github.com/storytold/vectorcraft.git synced 2026-10-24 09:46:59 -06:00
Code Issues Packages Projects Releases Wiki Activity

Fix #3545: draw_path and draw_shape with a paint the engine refuses leave the document unchanged

Browse Source 
The MCP drawing tools ran the creating command (path.create, shape.*)
and only then paint.setFill, paint.setStroke, stroke.set,
object.setProps and effect.apply, each as a command of its own.
check_draw_options checked dash, cap, join, opacity, name and effects up
front, but not the paints: a fill or stroke the engine can't read
(stroke: "currentColor"), or a swatch the document lacks, failed after
the object was made, so the call returned an error and left the path,
its undo step and its SVG node behind. Each styling command was also an
undo step of its own.

The tools now send the creating command and the styling commands as one
command.batch (crates/mcp/src/tools.rs, draw): the engine rolls the
whole batch back when a step fails, so a refused paint leaves no object,
no undo step and nothing in the SVG, and a good call is one undo step
named as the creating command names its own (Pen, Rectangle, …). The
styling steps act on the new object, which the creating command selects,
with item: null (its whole appearance), as the ids did before. The
reply gives the failed step's own error, without the batch's prefix.
docs/mcp.md says so.

Test: tests_drawopts::a_bad_paint_draws_nothing (invalid fill, stroke,
a missing swatch and a non-numeric strokeWidth on draw_path and
draw_shape: error, node count, undo history and SVG export unchanged;
a good call adds one undo step named Pen / Rectangle). It fails without
the change (draw_path with stroke "currentColor" drew a path) and passes
with it; the other draw option and command_batch tests still pass.
This commit is contained in:
Taha-Mahmoodi
2026-10-22 18:56:32 +07:00
committed by bflatastic
parent 2f27759bcc
commit 648a8f5104
3 changed files with 115 additions and 53 deletions
Download Patch File Download Diff File Expand all files Collapse all files
crates/mcp/src/tests_drawopts.rs
+42
View File
@@ -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);
}
}
crates/mcp/src/tools.rs
+70 -52
View File
@@ -528,20 +528,19 @@ fn paint_params(v: &Value) -> Result<Value, String> {
})
}
fn apply_paint(b: &mut dyn Backend, a: &Args, ids: Option<Value>) -> Result<Value, String> {
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<Step<'static>>, Map<String, Value>), 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<Value>) -> Result<Valu
}
}
if !stroke.is_empty() {
exec(b, "stroke.set", with_ids(Value::Object(stroke)))?;
steps.push(("stroke.set", Value::Object(stroke)));
}
Ok((steps, applied))
}
fn apply_paint(b: &mut dyn Backend, a: &Args, ids: Option<Value>) -> Result<Value, String> {
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<Value>, 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<Value, String> {
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<Value>, 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<Value> = 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<Value> {
r.get("id").filter(|v| v.is_u64()).map(|id| json!([id]))
Ok(made)
}
fn draw_path(b: &mut dyn Backend, a: &Args) -> Result<Value, String> {
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<Value, String> {
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<Value, String> {
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).
docs/mcp.md
+3 -1
View File
@@ -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