diff --git a/crates/engine/src/cmd/props.rs b/crates/engine/src/cmd/props.rs index dd2daa82..1b8c45c8 100644 --- a/crates/engine/src/cmd/props.rs +++ b/crates/engine/src/cmd/props.rs @@ -260,6 +260,23 @@ 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 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. +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(); @@ -447,56 +464,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)?; @@ -528,7 +545,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; @@ -852,6 +869,15 @@ pub fn apply_text_style(cmd: &str, t: &mut TextData, start: usize, end: usize, k } "fontStyle" => { let st = string_val(v).ok_or_else(|| bad(cmd, "`fontStyle` must be a string"))?; + // A style the family doesn't have would draw in another face without a word; refuse it + // and list the family's styles. A family that isn't installed keeps any name (files + // from elsewhere name their fonts' styles). + for (_, _, span) in t.spans().into_iter().filter(|(a, b, _)| *b > start && *a < end.max(start + 1)) { + let styles = figcraft_text::family_styles(&span.font_family); + if !styles.is_empty() && !styles.iter().any(|x| x.eq_ignore_ascii_case(&st)) { + return Err(bad(cmd, format!("{} has no `{st}` style (it has: {})", span.font_family, styles.join(", ")))); + } + } t.restyle(start, end, |s| s.set_font_style(st.clone())); } "fontWeight" => { diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 959a6b21..9ee6ee7f 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -1580,6 +1580,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..f465c519 --- /dev/null +++ b/crates/engine/src/node_set_refusals_tests.rs @@ -0,0 +1,55 @@ +//! `node.set` refuses what can't apply instead of storing it where nothing reads it: auto layout +//! keys on layers that can't have auto layout, and a `fontStyle` the font family doesn't have. + +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"]); + 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)); +} + +#[test] +fn a_font_style_the_family_lacks_is_refused_with_the_styles_it_has() { + figcraft_text::scan_system_fonts(); + // Any installed family with a known list of styles will do (CI machines differ). + let Some((family, styles)) = + figcraft_text::family_names().into_iter().map(|f| (f.clone(), figcraft_text::family_styles(&f))).find(|(_, st)| !st.is_empty()) + 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 msg = bad_msg(s.execute("node.set", &json!({"ids": [t], "props": {"fontStyle": "Definitely Not A Style"}}))); + assert!(msg.contains(&family) && msg.contains(&styles[0]), "{msg}"); + ex(&mut s, "node.set", json!({"ids": [t], "props": {"fontStyle": styles[0]}})); + // A family that isn't installed keeps any style name (files from elsewhere name theirs). + ex(&mut s, "node.set", json!({"ids": [t], "props": {"fontFamily": "No Such Family 7f3a", "fontStyle": "Hairline"}})); +} diff --git a/crates/mcp/src/tools.rs b/crates/mcp/src/tools.rs index 9704aab1..a5a2c062 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; on other layers they are an error, as is a fontStyle the font family doesn't have (the error lists its styles). 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"],