diff --git a/crates/engine/src/cmd/edit.rs b/crates/engine/src/cmd/edit.rs index 90f69e8b..804c7819 100644 --- a/crates/engine/src/cmd/edit.rs +++ b/crates/engine/src/cmd/edit.rs @@ -1156,9 +1156,34 @@ fn set(s: &mut Session, p: &Value) -> Result { .collect(), None => Vec::new(), }; + let mut warnings = font_style_warnings(&s.ds()?.doc, &ids, &props); + // Auto layout keys go to the layers that can have auto layout; with none among the targets, + // the first refuses them (`props::layout_mut`). + let layout_keys: Vec<&str> = props.keys().map(String::as_str).filter(|k| super::props::LAYOUT_KEYS.contains(k)).collect(); + let mut without_layout = props.clone(); + let mut skipped = BTreeSet::new(); + if !layout_keys.is_empty() { + let doc = &s.ds()?.doc; + let frame_like = |i: &NodeId| doc.node(*i).is_some_and(|n| n.kind.is_frame_like()); + if ids.iter().any(frame_like) { + skipped = ids.iter().copied().filter(|i| !frame_like(i)).collect(); + without_layout.retain(|k, _| !layout_keys.contains(&k.as_str())); + } + if !skipped.is_empty() { + let names: Vec = + skipped.iter().take(10).filter_map(|i| doc.node(*i)).map(|n| format!("{} “{}” ({})", n.id, n.name, n.kind.type_name())).collect(); + let more = if skipped.len() > names.len() { format!(" and {} more", skipped.len() - names.len()) } else { String::new() }; + warnings.push(format!( + "{} skipped on {}{more}: only frames, components and instances have auto layout", + layout_keys.iter().map(|k| format!("`{k}`")).collect::>().join(", "), + names.join(", ") + )); + } + } let doc = s.doc_mut()?; for i in ids { - super::props::apply("node.set", doc, i, &props)?; + let props = if skipped.contains(&i) { &without_layout } else { &props }; + super::props::apply("node.set", doc, i, props)?; } if let Some(o) = pivot { for (i, q) in pinned { @@ -1168,7 +1193,43 @@ fn set(s: &mut Session, p: &Value) -> Result { } } } - ok() + if warnings.is_empty() { ok() } else { Ok(json!({"ok": true, "warnings": warnings})) } +} + +/// A `fontStyle` draws in the family's face nearest to it, as a text layer resolves it. When that +/// face has another weight or slant than the name asks for ("Black" on a family whose heaviest is +/// Bold), say which face it is. Families that aren't installed, and names that match a style or a +/// named instance (spelled any way: "SemiBold", "Semi Bold"), say nothing. +fn font_style_warnings(doc: &figcraft_doc::Document, ids: &[NodeId], props: &serde_json::Map) -> Vec { + let Some(style) = props.get("fontStyle").and_then(Value::as_str) else { return Vec::new() }; + let mut families: Vec = match props.get("fontFamily").and_then(Value::as_str) { + Some(f) => vec![f.to_string()], + None => ids + .iter() + .filter_map(|i| doc.node(*i)) + .filter_map(|n| n.kind.text()) + .flat_map(|t| t.spans().into_iter().map(|(_, _, st)| st.font_family.clone()).collect::>()) + .collect(), + }; + families.sort(); + families.dedup(); + let norm = |s: &str| s.to_lowercase().replace([' ', '-', '_'], ""); + let (want_w, want_i) = (i32::from(figcraft_doc::style_weight(style)), style.to_lowercase().contains("italic")); + families + .into_iter() + .filter_map(|family| { + let styles = figcraft_text::family_styles(&family); + if styles.is_empty() || styles.iter().any(|x| norm(x) == norm(style)) { + return None; + } + let face = figcraft_text::find_face(&family, style).filter(|f| f.family.eq_ignore_ascii_case(&family))?; + let w = face.axis("wght").map_or(i32::from(face.weight), |a| a.clamp(f64::from(want_w)).round() as i32); + if face.instance(style).is_some() || (w == want_w && face.italic == want_i) { + return None; + } + Some(format!("{family} has no `{style}` style: it draws in {}, the nearest (it has: {})", face.style, styles.join(", "))) + }) + .collect() } /// `stroke.widthProfile`: a preset or points (through `node.set`'s `widthProfile`, so instances diff --git a/crates/engine/src/cmd/props.rs b/crates/engine/src/cmd/props.rs index 78b448c0..ca62962d 100644 --- a/crates/engine/src/cmd/props.rs +++ b/crates/engine/src/cmd/props.rs @@ -265,6 +265,56 @@ fn ensure_layout(n: &mut Node) -> &mut AutoLayout { n.layout.get_or_insert_with(|| AutoLayout { padding: [0.0; 4], gap: 0.0, ..Default::default() }) } +/// The keys that set a frame's auto layout (through [`layout_mut`]). +pub const LAYOUT_KEYS: &[&str] = &[ + "gap", + "itemSpacing", + "columnGap", + "crossGap", + "counterAxisSpacing", + "rowGap", + "padding", + "paddingTop", + "paddingRight", + "paddingBottom", + "paddingLeft", + "paddingHorizontal", + "paddingVertical", + "mainAlign", + "primaryAxisAlignItems", + "crossAlign", + "counterAxisAlignItems", + "contentAlign", + "counterAxisAlignContent", + "wrap", + "gridColumns", + "gridRows", + "strokesIncluded", + "reverseZ", + "columnTracks", + "gridColumnTracks", + "rowTracks", + "gridRowTracks", +]; + +/// The auto layout of a frame-like node, created when it has none yet. Other nodes can't hold an +/// auto layout: an auto layout key on them is refused, not stored where nothing reads it +/// (`node.set` on several layers skips them instead, see `edit::set`). +fn layout_mut<'a>(cmd: &str, k: &str, n: &'a mut Node) -> Result<&'a mut AutoLayout> { + if !n.kind.is_frame_like() { + return Err(bad( + cmd, + format!( + "`{k}` is an auto layout property, and layer {} “{}” is a {} (only frames, components and instances have auto layout)", + n.id, + n.name, + n.kind.type_name() + ), + )); + } + Ok(ensure_layout(n)) +} + /// Apply a property map to one node. `x`/`y` are relative to the parent. pub fn apply(cmd: &str, doc: &mut Document, id: NodeId, props: &Map) -> Result<()> { let mut unknown = Vec::new(); @@ -452,56 +502,56 @@ pub fn apply(cmd: &str, doc: &mut Document, id: NodeId, props: &Map { - ensure_layout(doc.get_mut(id)?).gap = f64_of(v).ok_or_else(|| bad(cmd, format!("`{k}` must be a number")))? + layout_mut(cmd, k, doc.get_mut(id)?)?.gap = f64_of(v).ok_or_else(|| bad(cmd, format!("`{k}` must be a number")))? } "crossGap" | "counterAxisSpacing" | "rowGap" => { - ensure_layout(doc.get_mut(id)?).cross_gap = f64_of(v).ok_or_else(|| bad(cmd, format!("`{k}` must be a number")))? + layout_mut(cmd, k, doc.get_mut(id)?)?.cross_gap = f64_of(v).ok_or_else(|| bad(cmd, format!("`{k}` must be a number")))? } "padding" => { let p = four(v).ok_or_else(|| bad(cmd, "padding: n or [t,r,b,l]"))?; - ensure_layout(doc.get_mut(id)?).padding = p.map(|x| x.max(0.0)); + layout_mut(cmd, k, doc.get_mut(id)?)?.padding = p.map(|x| x.max(0.0)); } "paddingTop" | "paddingRight" | "paddingBottom" | "paddingLeft" => { let i = ["paddingTop", "paddingRight", "paddingBottom", "paddingLeft"].iter().position(|x| *x == k).unwrap_or(0); let val = f64_of(v).ok_or_else(|| bad(cmd, format!("`{k}` must be a number")))?.max(0.0); - if let Some(slot) = ensure_layout(doc.get_mut(id)?).padding.get_mut(i) { + if let Some(slot) = layout_mut(cmd, k, doc.get_mut(id)?)?.padding.get_mut(i) { *slot = val; } } "paddingHorizontal" => { let val = f64_of(v).unwrap_or(0.0).max(0.0); - let l = ensure_layout(doc.get_mut(id)?); + let l = layout_mut(cmd, k, doc.get_mut(id)?)?; l.padding[1] = val; l.padding[3] = val; } "paddingVertical" => { let val = f64_of(v).unwrap_or(0.0).max(0.0); - let l = ensure_layout(doc.get_mut(id)?); + let l = layout_mut(cmd, k, doc.get_mut(id)?)?; l.padding[0] = val; l.padding[2] = val; } "mainAlign" | "primaryAxisAlignItems" => { - ensure_layout(doc.get_mut(id)?).main_align = + layout_mut(cmd, k, doc.get_mut(id)?)?.main_align = enum_of(v).ok_or_else(|| bad(cmd, "mainAlign: min|center|max|spaceBetween|spaceAround|spaceEvenly"))? } "crossAlign" | "counterAxisAlignItems" => { - ensure_layout(doc.get_mut(id)?).cross_align = enum_of(v).ok_or_else(|| bad(cmd, "crossAlign: min|center|max|baseline"))? + layout_mut(cmd, k, doc.get_mut(id)?)?.cross_align = enum_of(v).ok_or_else(|| bad(cmd, "crossAlign: min|center|max|baseline"))? } "contentAlign" | "counterAxisAlignContent" => { - ensure_layout(doc.get_mut(id)?).content_align = enum_of(v).ok_or_else(|| bad(cmd, "contentAlign: auto|spaceBetween"))? + layout_mut(cmd, k, doc.get_mut(id)?)?.content_align = enum_of(v).ok_or_else(|| bad(cmd, "contentAlign: auto|spaceBetween"))? } - "wrap" => ensure_layout(doc.get_mut(id)?).wrap = v.as_bool().unwrap_or(false), + "wrap" => layout_mut(cmd, k, doc.get_mut(id)?)?.wrap = v.as_bool().unwrap_or(false), "gridColumns" | "gridRows" => { let count = f64_of(v).ok_or_else(|| bad(cmd, format!("`{k}` must be a number")))?.clamp(1.0, 1000.0) as u32; - let l = ensure_layout(doc.get_mut(id)?); + let l = layout_mut(cmd, k, doc.get_mut(id)?)?; // Tracks past the new count go away (new tracks are 1fr). let (n, tracks) = if k == "gridColumns" { (&mut l.grid_columns, &mut l.column_tracks) } else { (&mut l.grid_rows, &mut l.row_tracks) }; *n = count; tracks.truncate(count as usize); } - "strokesIncluded" => ensure_layout(doc.get_mut(id)?).strokes_included = v.as_bool().unwrap_or(false), - "reverseZ" => ensure_layout(doc.get_mut(id)?).reverse_z = v.as_bool().unwrap_or(false), + "strokesIncluded" => layout_mut(cmd, k, doc.get_mut(id)?)?.strokes_included = v.as_bool().unwrap_or(false), + "reverseZ" => layout_mut(cmd, k, doc.get_mut(id)?)?.reverse_z = v.as_bool().unwrap_or(false), "sizingH" | "layoutSizingHorizontal" => { let s = sizing(v).ok_or_else(|| bad(cmd, "sizing: fixed|hug|fill"))?; set_sizing(cmd, doc, id, s, true)?; @@ -533,7 +583,7 @@ pub fn apply(cmd: &str, doc: &mut Document, id: NodeId, props: &Map { let a = v.as_array().ok_or_else(|| bad(cmd, format!("`{k}` must be an array of tracks")))?; let tracks = a.iter().map(track_of).collect::>>().ok_or_else(|| bad(cmd, TRACK_HELP))?; - let l = ensure_layout(doc.get_mut(id)?); + let l = layout_mut(cmd, k, doc.get_mut(id)?)?; if k.contains("olumn") { l.grid_columns = (tracks.len() as u32).clamp(1, 1000); l.column_tracks = tracks; diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index a0d87f9c..60f99136 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -1657,6 +1657,8 @@ mod menu_gaps_tests; #[cfg(test)] mod nested_override_tests; #[cfg(test)] +mod node_set_refusals_tests; +#[cfg(test)] mod online_tests; #[cfg(test)] mod open_issues_tests; diff --git a/crates/engine/src/node_set_refusals_tests.rs b/crates/engine/src/node_set_refusals_tests.rs new file mode 100644 index 00000000..ce316d30 --- /dev/null +++ b/crates/engine/src/node_set_refusals_tests.rs @@ -0,0 +1,95 @@ +//! `node.set` doesn't store what can't apply: auto layout keys go only to layers that can have auto +//! layout (the others are named in `warnings`, and with none of them the call is refused), and a +//! `fontStyle` the family lacks says which face draws instead. + +use serde_json::{Value, json}; + +use crate::{EngineError, Session}; + +fn ex(s: &mut Session, cmd: &str, p: Value) -> Value { + s.execute(cmd, &p).unwrap_or_else(|e| panic!("{cmd}: {e}")) +} + +fn id(v: &Value) -> crate::NodeId { + crate::params::id_of(v).unwrap() +} + +fn bad_msg(r: crate::Result) -> String { + match r { + Err(EngineError::BadParams { msg, .. }) => msg, + other => panic!("expected BadParams, got {other:?}"), + } +} + +#[test] +fn auto_layout_keys_on_layers_without_auto_layout_are_refused() { + let mut s = Session::new(); + let t = id(&ex(&mut s, "node.create", json!({"type": "text", "text": "Hi"}))["id"]); + let r = id(&ex(&mut s, "node.create", json!({"type": "rectangle"}))["id"]); + for (target, key, value) in [(t, "gap", json!(8)), (r, "padding", json!(4)), (t, "mainAlign", json!("center")), (r, "wrap", json!(true))] { + let msg = bad_msg(s.execute("node.set", &json!({"ids": [target], "props": {key: value}}))); + assert!(msg.contains(key) && msg.contains("auto layout"), "{msg}"); + assert!(s.doc().unwrap().node(target).unwrap().layout.is_none(), "no hidden auto layout on {target}"); + } + // Frames still take them, with or without a layout mode first. + let f = id(&ex(&mut s, "node.create", json!({"type": "frame"}))["id"]); + let out = ex(&mut s, "node.set", json!({"ids": [f], "props": {"gap": 8, "padding": 4}})); + assert_eq!(s.doc().unwrap().node(f).unwrap().layout.as_ref().map(|l| l.gap), Some(8.0)); + assert!(out.get("warnings").is_none(), "{out}"); +} + +#[test] +fn with_several_layers_the_frames_take_auto_layout_keys_and_the_others_are_named() { + let mut s = Session::new(); + let f = id(&ex(&mut s, "node.create", json!({"type": "frame", "name": "Card"}))["id"]); + let t = id(&ex(&mut s, "node.create", json!({"type": "text", "text": "Hi", "name": "Title"}))["id"]); + let out = ex(&mut s, "node.set", json!({"ids": [f, t], "props": {"gap": 8, "opacity": 0.5}})); + let doc = s.doc().unwrap(); + assert_eq!(doc.node(f).unwrap().layout.as_ref().map(|l| l.gap), Some(8.0)); + assert!(doc.node(t).unwrap().layout.is_none(), "no hidden auto layout on the text"); + assert_eq!(doc.node(t).unwrap().opacity, 0.5, "the text still gets the other keys"); + let w = out["warnings"][0].as_str().unwrap(); + assert!(w.contains("`gap`") && w.contains(&format!("{t} “Title”")) && w.contains("TEXT"), "{w}"); + // One undo step for all of it. + ex(&mut s, "edit.undo", json!({})); + assert!(s.doc().unwrap().node(f).unwrap().layout.as_ref().is_none_or(|l| l.gap != 8.0)); +} + +#[test] +fn a_font_style_draws_in_the_nearest_face_and_says_so_when_it_differs() { + figcraft_text::scan_system_fonts(); + // Any installed family with a regular and a bold face and nothing heavier (CI machines differ). + let pick = figcraft_text::family_names().into_iter().find_map(|f| { + let st = figcraft_text::family_styles(&f); + let has = |w: u16| st.iter().any(|x| figcraft_doc::style_weight(x) == w && !x.to_lowercase().contains("italic")); + (has(400) && has(700) && !st.iter().any(|x| figcraft_doc::style_weight(x) > 700)).then_some((f, st)) + }); + let Some((family, styles)) = pick else { return }; + let mut s = Session::new(); + let t = id(&ex(&mut s, "node.create", json!({"type": "text", "text": "Hi", "props": {"fontFamily": family}}))["id"]); + let set = |s: &mut Session, style: &str| ex(s, "node.set", json!({"ids": [t], "props": {"fontStyle": style}})); + // Any spelling of a style it has, and names that resolve to the weight they ask for, say nothing. + let bold = styles.iter().find(|x| figcraft_doc::style_weight(x) == 700).unwrap().clone(); + for style in [bold.clone(), bold.to_uppercase(), bold.replace(' ', ""), "Roman".into(), "Normal".into()] { + let out = set(&mut s, &style); + assert!(out.get("warnings").is_none(), "{style}: {out}"); + } + // A heavier weight than the family has applies (as a text layer resolves it) and says which face draws. + let out = set(&mut s, "Black"); + let w = out["warnings"][0].as_str().unwrap(); + assert!(w.contains(&family) && w.contains("`Black`") && w.contains("nearest"), "{w}"); + assert_eq!(s.doc().unwrap().node(t).unwrap().kind.text().unwrap().spans()[0].2.font_style, "Black"); + // A family that isn't installed keeps any style name without a word (files from elsewhere name theirs). + let out = ex(&mut s, "node.set", json!({"ids": [t], "props": {"fontFamily": "No Such Family 7f3a", "fontStyle": "Hairline"}})); + assert!(out.get("warnings").is_none(), "{out}"); +} + +#[test] +fn a_refused_call_changes_nothing() { + let mut s = Session::new(); + let t = id(&ex(&mut s, "node.create", json!({"type": "text", "text": "Hi"}))["id"]); + let before = s.doc().unwrap().node(t).unwrap().fills.clone(); + // `fill` comes before `gap` (keys apply in order), so this checks the rollback. + assert!(s.execute("node.set", &json!({"ids": [t], "props": {"fill": "#FF0000", "gap": 8}})).is_err()); + assert_eq!(s.doc().unwrap().node(t).unwrap().fills, before); +} diff --git a/crates/mcp/src/tools.rs b/crates/mcp/src/tools.rs index 511d329e..79ef4945 100644 --- a/crates/mcp/src/tools.rs +++ b/crates/mcp/src/tools.rs @@ -163,7 +163,7 @@ pub fn tool_definitions() -> Vec { tool( "set_properties", "Set properties", - "Set properties on one or more layers as one undo step (engine `node.set`). Keys: name, visible, locked, opacity (0-1), blend, x, y (relative to the parent frame; groups don't count), width, height, rotation (degrees), fill (colour) / fills (array of paints: \"#hex\" or {\"type\":\"solid\",\"color\":\"#hex\"} or {\"type\":\"linearGradient\",\"stops\":[\"#hex\",\"#hex\"]} or stops [{\"position\":0..1,\"color\":\"#hex\"}], optional from/to handles in the unit box; radialGradient too), stroke / strokes, strokeWeight, strokeAlign (inside|center|outside), cornerRadius (n or [tl,tr,br,bl]), effects (array, e.g. {\"type\":\"dropShadow\",\"color\":\"#00000040\",\"offset\":[0,4],\"radius\":8}), clip, layout (null|horizontal|vertical|grid), gap, padding (n or [t,r,b,l]), mainAlign (min|center|max|spaceBetween|spaceAround|spaceEvenly), crossAlign, wrap, contentAlign (auto|spaceBetween: wrapped lines), sizingH / sizingV (fixed|hug|fill), constraints {horizontal, vertical}, characters, fontFamily, fontStyle, fontSize, lineHeight, letterSpacing, textAlign, textAutoResize. Run list_commands with filter \"node.set\" for the full list. Unknown keys are an error. Returns the updated layers.", + "Set properties on one or more layers as one undo step (engine `node.set`). Keys: name, visible, locked, opacity (0-1), blend, x, y (relative to the parent frame; groups don't count), width, height, rotation (degrees), fill (colour) / fills (array of paints: \"#hex\" or {\"type\":\"solid\",\"color\":\"#hex\"} or {\"type\":\"linearGradient\",\"stops\":[\"#hex\",\"#hex\"]} or stops [{\"position\":0..1,\"color\":\"#hex\"}], optional from/to handles in the unit box; radialGradient too), stroke / strokes, strokeWeight, strokeAlign (inside|center|outside), cornerRadius (n or [tl,tr,br,bl]), effects (array, e.g. {\"type\":\"dropShadow\",\"color\":\"#00000040\",\"offset\":[0,4],\"radius\":8}), clip, layout (null|horizontal|vertical|grid), gap, padding (n or [t,r,b,l]), mainAlign (min|center|max|spaceBetween|spaceAround|spaceEvenly), crossAlign, wrap, contentAlign (auto|spaceBetween: wrapped lines), sizingH / sizingV (fixed|hug|fill), constraints {horizontal, vertical}, characters, fontFamily, fontStyle, fontSize, lineHeight, letterSpacing, textAlign, textAutoResize. Auto layout keys (gap, padding, mainAlign, crossAlign, wrap, …) apply to frames, components and instances only: other layers among the ids are skipped and named in `warnings`, and with none that can take them the call is an error. A fontStyle draws in the family's nearest face; `warnings` says when that face has another weight or slant than the name asks for. Run list_commands with filter \"node.set\" for the full list. Unknown keys are an error. Returns the updated layers.", obj( json!({"ids": ids_schema("Layer ids (default: the selection)"), "props": {"type": "object", "description": "Property values to set"}}), &["props"], @@ -544,7 +544,7 @@ fn set_properties(b: &mut dyn Backend, a: &Map) -> Result i, None => b @@ -555,7 +555,11 @@ fn set_properties(b: &mut dyn Backend, a: &Map) -> Result = ids.iter().take(20).filter_map(|i| b.call("node.get", json!({"id": i, "depth": 0})).ok()).collect(); - Ok(ToolResult::json(&json!({"ok": true, "nodes": nodes}))) + let mut out = json!({"ok": true, "nodes": nodes}); + if let Some(w) = warnings { + out["warnings"] = w; + } + Ok(ToolResult::json(&out)) } fn inspect_document(b: &mut dyn Backend, a: &Map) -> Result { diff --git a/crates/mcp/tests/protocol.rs b/crates/mcp/tests/protocol.rs index 89f2a649..6e943a1e 100644 --- a/crates/mcp/tests/protocol.rs +++ b/crates/mcp/tests/protocol.rs @@ -208,3 +208,14 @@ fn serve_accepts_a_limit_sized_frame_and_a_final_unterminated_frame() { assert_eq!(lines[0]["result"], json!({})); assert_eq!(lines[1]["id"], 2); } + +#[test] +fn set_properties_passes_on_what_it_skipped() { + let mut c = Client::new(); + let f = c.ok("create_node", json!({"type": "frame"}))["id"].clone(); + let t = c.ok("create_node", json!({"type": "text", "text": "Hi"}))["id"].clone(); + let r = c.ok("set_properties", json!({"ids": [f, t], "props": {"gap": 8}})); + assert!(r["warnings"][0].as_str().is_some_and(|w| w.contains("`gap`") && w.contains("TEXT")), "{r}"); + assert!(c.ok("set_properties", json!({"ids": [f], "props": {"gap": 4}})).get("warnings").is_none()); + assert!(c.err("set_properties", json!({"ids": [t], "props": {"gap": 4}})).contains("auto layout")); +}