From 3d59dd5d9757d248b8e5a531b633e8df09c20e11 Mon Sep 17 00:00:00 2001 From: Marco Arnulfo Date: Tue, 13 Oct 2026 00:38:39 +0200 Subject: [PATCH] fix(cmd): reject unknown spread values instead of falling back to page 1 (part of #651) --- crates/engine/src/cmd/file.rs | 8 +++---- crates/engine/src/cmd/guides.rs | 27 +++++------------------ crates/engine/src/cmd/linked.rs | 2 +- crates/engine/src/cmd/liquid.rs | 2 +- crates/engine/src/cmd/mathexpr.rs | 2 +- crates/engine/src/cmd/media.rs | 2 +- crates/engine/src/cmd/mod.rs | 23 ++++++++++++++------ crates/engine/src/cmd/object.rs | 36 ++++++++++++++++++++++++++----- crates/engine/src/cmd/qr.rs | 2 +- docs/mcp.md | 4 +++- 10 files changed, 65 insertions(+), 43 deletions(-) diff --git a/crates/engine/src/cmd/file.rs b/crates/engine/src/cmd/file.rs index 692c363e..a1919b89 100644 --- a/crates/engine/src/cmd/file.rs +++ b/crates/engine/src/cmd/file.rs @@ -335,7 +335,7 @@ fn file_place(s: &mut Session, p: &Value) -> Result { let st = s.active()?; st.selection.items.iter().copied().find(|i| st.doc.item(*i).is_some_and(|it| matches!(it.content, Content::Unassigned | Content::Graphic(_)))) }); - let spread = super::spread_param(p, "spread"); + let spread = super::spread_param(p, "file.place")?; let lid = s.doc()?.active_layer; let px = p.get("x").and_then(Value::as_f64); let py = p.get("y").and_then(Value::as_f64); @@ -458,7 +458,7 @@ fn place_load(s: &mut Session, p: &Value) -> Result { fn place_drop(s: &mut Session, p: &Value) -> Result { let (aid, (nw, nh)) = s.loaded.ok_or_else(|| bad("place.drop", "nothing loaded in the place cursor"))?; - let sr = super::spread_param(p, "spread"); + let sr = super::spread_param(p, "place.drop")?; let lid = s.doc()?.active_layer; let frame = super::id_param(p, "frame"); let rect = super::rect_param(p, "rect"); @@ -550,7 +550,7 @@ fn place_layout_page(s: &mut Session, p: &Value, name: &str, bytes: &[u8]) -> Re if ids.is_empty() { return Err(bad(ID, format!("page {page} has no objects"))); } - let to = super::spread_param(p, "spread"); + let to = super::spread_param(p, ID)?; let off = match (p.get("x").and_then(Value::as_f64), p.get("y").and_then(Value::as_f64)) { (Some(x), Some(y)) => designcraft_geom::Vec2::new(x - pr.x0, y - pr.y0), _ => designcraft_geom::Vec2::new(-pr.x0, -pr.y0), @@ -586,7 +586,7 @@ pub(crate) fn snippet_place(s: &mut Session, p: &Value) -> Result { if ids.is_empty() { return Err(bad("snippet.place", "the snippet is empty")); } - let sr = super::spread_param(p, "spread"); + let sr = super::spread_param(p, "snippet.place")?; let bounds = ids.iter().filter_map(|i| src.item(*i).map(|it| it.bounds())).reduce(|a, b| a.union(b)).unwrap_or(Rect::ZERO); let off = match (p.get("x").and_then(Value::as_f64), p.get("y").and_then(Value::as_f64)) { (Some(x), Some(y)) => designcraft_geom::Vec2::new(x - bounds.x0, y - bounds.y0), diff --git a/crates/engine/src/cmd/guides.rs b/crates/engine/src/cmd/guides.rs index 47aacdc8..1111cb3b 100644 --- a/crates/engine/src/cmd/guides.rs +++ b/crates/engine/src/cmd/guides.rs @@ -28,7 +28,7 @@ pub fn specs() -> Vec { cmd!("guide.move", "Move Guide", [], None, "{spread?, page, index, position}", has_doc, move_guide), cmd!("guide.delete", "Delete Guide", [], None, "{spread?, page, index}", has_doc, delete), cmd!("guide.deleteAll", "Delete All Guides on Spread", ["View", "Grids & Guides"], None, "{spread?}", has_doc, |s, p| { - let r = guide_spread(p, "guide.deleteAll")?; + let r = super::spread_param(p, "guide.deleteAll")?; s.edit(|d, _| { let sp = d.spread_mut(r).ok_or_else(|| bad("guide.deleteAll", "no such spread"))?; let mut n = 0; @@ -41,7 +41,7 @@ pub fn specs() -> Vec { }), cmd!(query "guide.list", "Guides", [], None, "{spread?} → [{page, index, orientation, position, spread}]", has_doc, |s, p| { let d = &s.doc()?.doc; - let sp = d.spread(guide_spread(p, "guide.list")?).ok_or_else(|| bad("guide.list", "no such spread"))?; + let sp = d.spread(super::spread_param(p, "guide.list")?).ok_or_else(|| bad("guide.list", "no such spread"))?; let mut out = Vec::new(); for (pi, pg) in sp.pages.iter().enumerate() { for (i, g) in pg.guides.iter().enumerate() { @@ -71,23 +71,6 @@ fn optional_usize(p: &Value, key: &str, command: &str) -> Result> p.get(key).map(|value| checked_usize(value, key, command)).transpose() } -/// Numeric spread shorthand or the serialized `SpreadRef` form, without lossy/defaulting casts. -fn guide_spread(p: &Value, command: &str) -> Result { - let Some(value) = p.get("spread") else { return Ok(SpreadRef::Doc(0)) }; - if value.is_number() { - return Ok(SpreadRef::Doc(checked_usize(value, "spread", command)?)); - } - let object = value.as_object().ok_or_else(|| bad(command, "`spread` must be a non-negative integer or a spread reference"))?; - let kind = object.get("kind").and_then(Value::as_str).ok_or_else(|| bad(command, "`spread.kind` must be `doc` or `parent`"))?; - let index = object.get("index").ok_or_else(|| bad(command, "missing `spread.index`"))?; - let index = checked_usize(index, "spread.index", command)?; - match kind { - "doc" => Ok(SpreadRef::Doc(index)), - "parent" => Ok(SpreadRef::Parent(index)), - _ => Err(bad(command, "`spread.kind` must be `doc` or `parent`")), - } -} - fn finite_number(p: &Value, key: &str, command: &str) -> Result> { let Some(value) = p.get(key) else { return Ok(None) }; let number = value.as_f64().ok_or_else(|| bad(command, format!("`{key}` must be a finite number")))?; @@ -113,7 +96,7 @@ fn page_at(d: &Document, r: SpreadRef, x: f64) -> usize { fn add(s: &mut Session, p: &Value) -> Result { let o = orientation(p)?; let pos = finite_number(p, "position", "guide.add")?.ok_or_else(|| bad("guide.add", "missing position"))?; - let r = guide_spread(p, "guide.add")?; + let r = super::spread_param(p, "guide.add")?; let spread_guide = p.get("spreadGuide").and_then(Value::as_bool).unwrap_or(false); let d = &s.doc()?.doc; let pi = match optional_usize(p, "page", "guide.add")? { @@ -139,7 +122,7 @@ fn add(s: &mut Session, p: &Value) -> Result { fn guide_ref(p: &Value, cmd: &str) -> Result<(SpreadRef, usize, usize)> { let pi = optional_usize(p, "page", cmd)?.ok_or_else(|| bad(cmd, "missing page"))?; let i = optional_usize(p, "index", cmd)?.ok_or_else(|| bad(cmd, "missing index"))?; - Ok((guide_spread(p, cmd)?, pi, i)) + Ok((super::spread_param(p, cmd)?, pi, i)) } /// Commands must use the same guide/layer protection as pointer editing. @@ -237,7 +220,7 @@ fn create_guides(s: &mut Session, p: &Value) -> Result { let cg = guide_gutter(p, "columnGutter")?; let to_page = str_param(p, "fitTo") == Some("page"); let remove = p.get("removeExisting").and_then(Value::as_bool).unwrap_or(false); - let r = guide_spread(p, "layout.createGuides")?; + let r = super::spread_param(p, "layout.createGuides")?; let only = optional_usize(p, "page", "layout.createGuides")?; let page_count = { let sp = s.doc()?.doc.spread(r).ok_or_else(|| bad("layout.createGuides", "no such spread"))?; diff --git a/crates/engine/src/cmd/linked.rs b/crates/engine/src/cmd/linked.rs index ec07a24b..f504fe66 100644 --- a/crates/engine/src/cmd/linked.rs +++ b/crates/engine/src/cmd/linked.rs @@ -28,7 +28,7 @@ pub fn specs() -> Vec { |s, p| { let parent = StoryId(p.get("story").and_then(Value::as_u64).ok_or_else(|| bad("story.placeAndLink", "missing story"))?); let rect = rect_param(p, "rect").ok_or_else(|| bad("story.placeAndLink", "missing rect"))?; - let sr = spread_param(p, "spread"); + let sr = spread_param(p, "story.placeAndLink")?; let layer = s.doc()?.active_layer; s.edit(|d, sel| { if d.story(parent).is_none() { diff --git a/crates/engine/src/cmd/liquid.rs b/crates/engine/src/cmd/liquid.rs index 604369df..879fdb36 100644 --- a/crates/engine/src/cmd/liquid.rs +++ b/crates/engine/src/cmd/liquid.rs @@ -168,7 +168,7 @@ pub fn specs() -> Vec { } ), cmd!("guide.liquid", "Convert to Liquid Guide", [], None, "{spread?, page, index, on?: bool (default true)}", has_doc, |s, p| { - let sr = super::spread_param(p, "spread"); + let sr = super::spread_param(p, "guide.liquid")?; let pi = p.get("page").and_then(Value::as_u64).unwrap_or(0) as usize; let gi = p.get("index").and_then(Value::as_u64).ok_or_else(|| bad("guide.liquid", "`index` required"))? as usize; let on = p.get("on").and_then(Value::as_bool).unwrap_or(true); diff --git a/crates/engine/src/cmd/mathexpr.rs b/crates/engine/src/cmd/mathexpr.rs index ec90d52e..d9ce5114 100644 --- a/crates/engine/src/cmd/mathexpr.rs +++ b/crates/engine/src/cmd/mathexpr.rs @@ -70,7 +70,7 @@ fn insert(s: &mut Session, p: &Value) -> Result { }); } let text = s.doc()?.selection.text.filter(|t| t.cell.is_none()); - let spread = super::spread_param(p, "spread"); + let spread = super::spread_param(p, "math.insert")?; let (px, py) = (p.get("x").and_then(Value::as_f64), p.get("y").and_then(Value::as_f64)); s.edit(|d, sel| { if let Some(t) = text { diff --git a/crates/engine/src/cmd/media.rs b/crates/engine/src/cmd/media.rs index b6e4cd4b..f6d13ec0 100644 --- a/crates/engine/src/cmd/media.rs +++ b/crates/engine/src/cmd/media.rs @@ -20,7 +20,7 @@ pub(crate) fn place_media(s: &mut Session, p: &Value, name: String, mime: &str, let st = s.active()?; st.selection.items.iter().copied().find(|i| st.doc.item(*i).is_some_and(|it| matches!(it.content, Content::Unassigned | Content::Graphic(_)))) }); - let spread = super::spread_param(p, "spread"); + let spread = super::spread_param(p, "file.place")?; let lid = s.doc()?.active_layer; let (px, py) = (p.get("x").and_then(Value::as_f64), p.get("y").and_then(Value::as_f64)); let mime = mime.to_string(); diff --git a/crates/engine/src/cmd/mod.rs b/crates/engine/src/cmd/mod.rs index 68061af0..31e4544f 100644 --- a/crates/engine/src/cmd/mod.rs +++ b/crates/engine/src/cmd/mod.rs @@ -254,12 +254,23 @@ pub(crate) fn rect_param(p: &Value, key: &str) -> Option { let f = |i: usize| a.get(i).and_then(Value::as_f64); Some(Rect::new(f(0)?, f(1)?, f(2)?, f(3)?)) } -/// `{"kind":"doc","index":0}`, `0` or absent (= spread 0). -pub(crate) fn spread_param(p: &Value, key: &str) -> SpreadRef { - match p.get(key) { - Some(Value::Number(n)) => SpreadRef::Doc(n.as_u64().unwrap_or(0) as usize), - Some(v) => serde_json::from_value(v.clone()).unwrap_or(SpreadRef::Doc(0)), - None => SpreadRef::Doc(0), +/// The `spread` param: a body spread index (`0`), a spread reference (`{"kind":"doc"|"parent","index":0}`) +/// or absent (= spread 0). Anything else is an error: read as spread 0, a script aiming at a parent +/// spread got its items on the first page without a word. +pub(crate) fn spread_param(p: &Value, command: &str) -> Result { + let index = |v: &Value, key: &str| { + v.as_u64().and_then(|n| usize::try_from(n).ok()).ok_or_else(|| bad(command, format!("`{key}` must be a non-negative integer"))) + }; + let Some(value) = p.get("spread") else { return Ok(SpreadRef::Doc(0)) }; + if value.is_number() { + return Ok(SpreadRef::Doc(index(value, "spread")?)); + } + let object = value.as_object().ok_or_else(|| bad(command, "`spread` must be a non-negative integer or {kind: doc|parent, index}"))?; + let i = index(object.get("index").ok_or_else(|| bad(command, "missing `spread.index`"))?, "spread.index")?; + match object.get("kind").and_then(Value::as_str) { + Some("doc") => Ok(SpreadRef::Doc(i)), + Some("parent") => Ok(SpreadRef::Parent(i)), + _ => Err(bad(command, "`spread.kind` must be `doc` or `parent`")), } } diff --git a/crates/engine/src/cmd/object.rs b/crates/engine/src/cmd/object.rs index cd13be11..21362ef9 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?, 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, text?: string, caret?: bool, vertical?: bool (text: a new vertical story)}", has_doc, frame_create ), @@ -828,7 +828,7 @@ fn frame_create(s: &mut Session, p: &Value) -> Result { let rect = rect_param(p, "rect").ok_or_else(|| bad("frame.create", "missing rect"))?; let shape = str_param(p, "shape").unwrap_or("rectangle"); let content = str_param(p, "content").unwrap_or("graphic"); - let sr = spread_param(p, "spread"); + let sr = spread_param(p, "frame.create")?; let lid = s.doc()?.active_layer; let text = super::text_param(p, "text"); let caret = bool_or(p, "caret", content == "text"); @@ -891,7 +891,7 @@ pub(crate) fn polygon_in(r: Rect, sides: u32, inset: f64) -> designcraft_geom::P fn line_create(s: &mut Session, p: &Value) -> Result { let a = point_param(p, "a").ok_or_else(|| bad("line.create", "missing a"))?; let b = point_param(p, "b").ok_or_else(|| bad("line.create", "missing b"))?; - let sr = spread_param(p, "spread"); + let sr = spread_param(p, "line.create")?; let lid = s.doc()?.active_layer; s.edit(|d, sel| { let id = ItemId(d.alloc()); @@ -1592,7 +1592,7 @@ fn gap_move(s: &mut Session, p: &Value) -> Result { .and_then(Value::as_array) .and_then(|a| Some(Point::new(a.first()?.as_f64()?, a.get(1)?.as_f64()?))) .ok_or_else(|| bad(ID, "missing at"))?; - let sr = spread_param(p, "spread"); + let sr = spread_param(p, ID)?; let (vertical, a, z, before, after) = find_gap(&s.doc()?.doc, sr, at).ok_or_else(|| bad(ID, "no gap there"))?; // `delta` along the gap's axis, or a drag (dx, dy) of which the axis part counts. let delta = @@ -1919,7 +1919,7 @@ fn path_create(s: &mut Session, p: &Value) -> Result { return Err(bad("path.create", "a path needs at least two anchors")); } let closed = bool_or(p, "closed", false); - let sr = spread_param(p, "spread"); + let sr = spread_param(p, "path.create")?; let lid = s.doc()?.active_layer; s.edit(|d, sel| { let id = ItemId(d.alloc()); @@ -2813,3 +2813,29 @@ mod swatch_name_tests { } } } + +#[cfg(test)] +mod spread_target_tests { + use super::*; + + #[test] + fn creating_on_a_parent_spread_and_rejecting_unknown_spreads() { + let mut s = Session::new(); + s.execute("file.new", &json!({"pages": 1, "facingPages": false})).unwrap(); + let parent = json!({"kind": "parent", "index": 0}); + let r = s.execute("frame.create", &json!({"spread": parent, "rect": [0, 0, 100, 100]})).unwrap(); + let id = ItemId(r["id"].as_u64().unwrap()); + assert_eq!(s.doc().unwrap().doc.find(id).unwrap().spread, SpreadRef::Parent(0)); + s.execute("line.create", &json!({"spread": parent, "a": [0, 0], "b": [50, 50]})).unwrap(); + assert_eq!(s.doc().unwrap().doc.parents[0].items.len(), 2); + + // Each of these used to land on the first body page. + for bad in + [json!("A-Parent"), json!("0"), json!(-1), json!(1.5), json!({"kind": "master", "index": 0}), json!({"kind": "parent"}), json!([0])] + { + assert!(s.execute("frame.create", &json!({"spread": bad, "rect": [0, 0, 100, 100]})).is_err(), "frame.create spread {bad}"); + assert!(s.execute("line.create", &json!({"spread": bad, "a": [0, 0], "b": [50, 50]})).is_err(), "line.create spread {bad}"); + } + assert!(s.doc().unwrap().doc.spreads[0].items.is_empty()); + } +} diff --git a/crates/engine/src/cmd/qr.rs b/crates/engine/src/cmd/qr.rs index c201de4e..d68f5e69 100644 --- a/crates/engine/src/cmd/qr.rs +++ b/crates/engine/src/cmd/qr.rs @@ -58,7 +58,7 @@ fn qr_code(s: &mut Session, p: &Value) -> Result { let text = encode(p).map_err(|e| bad(ID, e))?; let color = str_param(p, "color").unwrap_or(designcraft_color::swatch::BLACK).to_string(); let rect = rect_param(p, "rect"); - let sr = spread_param(p, "spread"); + let sr = spread_param(p, ID)?; let st = s.doc()?; let lid = st.active_layer; let sel = st.selection.items.clone(); diff --git a/docs/mcp.md b/docs/mcp.md index 17c03a3d..f3558574 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -58,7 +58,9 @@ so inspect the document before retrying. The next request re-establishes the con Points (1/72 in), y down, in **spread** space: on a single-page spread the page's top-left is (0, 0); on a facing spread the right page starts at x = page width. `inspect_document` lists page bounds per spread. Page indices in -`render_page` / `export_png` are 0-based. `pointer` events in connected mode may use `"space": "screen"` (egui points, +`render_page` / `export_png` are 0-based. Commands that create items take `spread`: a body spread index, or +`{"kind": "parent", "index": 0}` for a parent spread (its index in `parents` of `inspect_document`); any other +value is an error. `pointer` events in connected mode may use `"space": "screen"` (egui points, see `ui_inspect` → `canvasRect`). ## Text offsets