diff --git a/crates/engine/src/binding_lists_tests.rs b/crates/engine/src/binding_lists_tests.rs index 805b0afe..9db9975a 100644 --- a/crates/engine/src/binding_lists_tests.rs +++ b/crates/engine/src/binding_lists_tests.rs @@ -97,3 +97,77 @@ fn effect_moves_and_removals_reject_junk() { ex(&mut s, "effect.remove", json!({"ids": [r], "index": 7})); assert_eq!(s.doc().unwrap().node(r).unwrap().effects.len(), 1); } + +fn fill_hex(s: &Session, r: NodeId) -> Vec { + s.doc().unwrap().node(r).unwrap().fills.iter().filter_map(|p| p.solid_color()).map(|c| c.to_hex()).collect() +} + +fn brand_id(s: &Session) -> String { + s.doc().unwrap().variables.get("brand").unwrap().id.clone() +} + +#[test] +fn explicit_paint_edits_detach_the_colour_binding() { + // An edit that reported success keeps its colour: the binding it overwrote lets go (as + // selection colours already do), instead of putting the variable's colour back. + let edits = [ + ("fill.set", json!({"color": "#00FF00"})), + ("node.set", json!({"props": {"fill": "#00FF00"}})), + ("node.set", json!({"props": {"fills": ["#00FF00"]}})), + ]; + for (cmd, mut p) in edits { + let (mut s, r) = setup(); + ex(&mut s, "variable.bind", json!({"ids": [r], "field": "fill", "variable": "brand"})); + p["ids"] = json!([r]); + ex(&mut s, cmd, p.clone()); + assert_eq!(fill_hex(&s, r), ["00FF00"], "{cmd} {p}"); + assert!(bound(&s, r).is_empty(), "{cmd} {p}"); + ex(&mut s, "variable.set", json!({"variable": "brand", "value": "#123456"})); + assert_eq!(fill_hex(&s, r), ["00FF00"], "{cmd} {p}: no longer follows the variable"); + } + for (cmd, mut p) in [("stroke.set", json!({"color": "#00FF00"})), ("node.set", json!({"props": {"stroke": "#00FF00"}}))] { + let (mut s, r) = setup(); + ex(&mut s, "node.set", json!({"ids": [r], "props": {"strokes": ["#000000"]}})); + ex(&mut s, "variable.bind", json!({"ids": [r], "field": "stroke", "variable": "brand"})); + p["ids"] = json!([r]); + ex(&mut s, cmd, p); + let n = s.doc().unwrap().node(r).unwrap(); + assert_eq!(n.strokes[0].solid_color().map(|c| c.to_hex()).as_deref(), Some("00FF00"), "{cmd}"); + assert!(n.bound.is_empty(), "{cmd}"); + } +} + +#[test] +fn clearing_bound_fills_adds_no_paint_back() { + let (mut s, r) = setup(); + ex(&mut s, "variable.bind", json!({"ids": [r], "field": "fill", "variable": "brand"})); + ex(&mut s, "node.set", json!({"ids": [r], "props": {"fills": []}})); + assert!(s.doc().unwrap().node(r).unwrap().fills.is_empty()); + assert!(bound(&s, r).is_empty()); + ex(&mut s, "variable.set", json!({"variable": "brand", "value": "#123456"})); + assert!(s.doc().unwrap().node(r).unwrap().fills.is_empty(), "the variable doesn't bring the fill back"); +} + +#[test] +fn paint_opacity_edits_keep_the_colour_binding() { + let (mut s, r) = setup(); + ex(&mut s, "variable.bind", json!({"ids": [r], "field": "fill", "variable": "brand"})); + ex(&mut s, "fill.set", json!({"ids": [r], "opacity": 0.5})); + ex(&mut s, "variable.set", json!({"variable": "brand", "value": "#0000FF"})); + assert_eq!(fill_hex(&s, r), ["0000FF"], "the colour still follows the variable"); + assert_eq!(s.doc().unwrap().node(r).unwrap().fills[0].opacity, 0.5); + // The same colour with another opacity, through node.set, keeps it too. + ex(&mut s, "node.set", json!({"ids": [r], "props": {"fills": [{"type": "solid", "color": "#0000FF", "opacity": 0.25}]}})); + assert_eq!(bound(&s, r).len(), 1); + assert_eq!(s.doc().unwrap().node(r).unwrap().fills[0].opacity, 0.25); +} + +#[test] +fn paints_set_with_their_binding_in_the_same_call_keep_it() { + let (mut s, r) = setup(); + ex(&mut s, "variable.bind", json!({"ids": [r], "field": "fill", "variable": "brand"})); + let brand = brand_id(&s); + ex(&mut s, "node.set", json!({"ids": [r], "props": {"fills": ["#00FF00"], "boundVariables": {"fill": brand}}})); + assert_eq!(bound(&s, r), [("fill".to_string(), brand)]); + assert_eq!(fill_hex(&s, r), ["FF0000"], "the binding given with the paints applies"); +} diff --git a/crates/engine/src/cmd/edit.rs b/crates/engine/src/cmd/edit.rs index a4769a35..ae2f1dc8 100644 --- a/crates/engine/src/cmd/edit.rs +++ b/crates/engine/src/cmd/edit.rs @@ -1342,7 +1342,7 @@ fn paint_set(s: &mut Session, p: &Value, fills: bool) -> Result { if let Some(b) = blend(p, "blend") { slot.blend = b; } - super::lists::remap_stop_bindings(n, fills, at, &old); + super::lists::rebind_edited_paint(n, fills, at, &old); if fills { n.styles.fill = None; } else { diff --git a/crates/engine/src/cmd/lists.rs b/crates/engine/src/cmd/lists.rs index 769d0a29..287c9ddd 100644 --- a/crates/engine/src/cmd/lists.rs +++ b/crates/engine/src/cmd/lists.rs @@ -112,7 +112,8 @@ pub fn stop_binding(field: &str) -> Option { /// Keep the gradient stop bindings of paint `index` of `n`'s fills (or strokes) on their stops after /// the paint changed from `old`: one stop removed shifts the later ones down (the removed one's /// binding goes); a paint that is no longer a gradient drops them. Other edits keep stop indices. -pub(crate) fn remap_stop_bindings(n: &mut Node, fills: bool, index: usize, old: &figcraft_color::Paint) { +/// Returns the removed stop's index. +pub(crate) fn remap_stop_bindings(n: &mut Node, fills: bool, index: usize, old: &figcraft_color::Paint) -> Option { use figcraft_color::PaintKind; let list = if fills { BoundList::Fills } else { BoundList::Strokes }; let new = if fills { n.fills.get(index) } else { n.strokes.get(index) }; @@ -122,11 +123,11 @@ pub(crate) fn remap_stop_bindings(n: &mut Node, fills: bool, index: usize, old: // The first stop that differs is the removed one when the rest moved down unchanged. let k = a.stops.iter().zip(&b.stops).position(|(x, y)| x != y).unwrap_or(b.stops.len()); if a.stops.get(k + 1..) != b.stops.get(k..) { - return; + return None; } Some(k) } - (_, Some(PaintKind::Gradient(_))) => return, + (_, Some(PaintKind::Gradient(_))) => return None, _ => None, }; let keys: Vec<(String, Option)> = n @@ -150,6 +151,42 @@ pub(crate) fn remap_stop_bindings(n: &mut Node, fills: bool, index: usize, old: } } n.bound.extend(kept); + removed +} + +/// After an explicit edit changed paint `index` of `n`'s fills (or strokes) from `old`: its stop +/// bindings follow a removed stop, then each binding whose value the edit changed (the colour, the +/// opacity, a stop's colour) lets go, so the edit stays instead of being put back from the +/// variable. Bindings whose value the edit kept stay; a paint that is gone takes its bindings. +pub(crate) fn rebind_edited_paint(n: &mut Node, fills: bool, index: usize, old: &figcraft_color::Paint) { + let removed = remap_stop_bindings(n, fills, index, old); + let mut old = old.clone(); + if let (Some(k), figcraft_color::PaintKind::Gradient(g)) = (removed, &mut old.kind) + && k < g.stops.len() + { + g.stops.remove(k); + } + let list = if fills { BoundList::Fills } else { BoundList::Strokes }; + let new = if fills { n.fills.get(index) } else { n.strokes.get(index) }.cloned(); + n.bound.retain(|key, _| match list_binding(key) { + Some((l, i, field)) if l == list && i == index => new.as_ref().is_some_and(|new| paint_field_kept(&old, new, field)), + _ => true, + }); +} + +/// Whether a paint's bound `field` (`color`, `opacity`, `stops//color`, …) has the same value in +/// `new` as in `old`. +fn paint_field_kept(old: &figcraft_color::Paint, new: &figcraft_color::Paint, field: &str) -> bool { + use figcraft_color::PaintKind; + match field { + "color" => matches!((&old.kind, &new.kind), (PaintKind::Solid { color: a }, PaintKind::Solid { color: b }) if a == b), + "opacity" => old.opacity == new.opacity, + _ => match (stop_binding(field), &old.kind, &new.kind) { + (Some(j), PaintKind::Gradient(a), PaintKind::Gradient(b)) => a.stops.get(j).map(|s| s.color) == b.stops.get(j).map(|s| s.color), + (Some(_), _, _) => false, + _ => old == new, + }, + } } /// Where item `k` lands when the item at `from` moves to `to`. diff --git a/crates/engine/src/cmd/props.rs b/crates/engine/src/cmd/props.rs index 8a1f4e1c..5df46003 100644 --- a/crates/engine/src/cmd/props.rs +++ b/crates/engine/src/cmd/props.rs @@ -315,18 +315,32 @@ fn layout_mut<'a>(cmd: &str, k: &str, n: &'a mut Node) -> Result<&'a mut AutoLay 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(); - // Geometry keys first so rotation applies to the final size. - let order = |k: &str| match k { +/// Replace a node's fills (or strokes). Bindings on the paints whose bound values the new list +/// changed let go, as an explicit edit does (see `lists::rebind_edited_paint`). +fn set_paints(n: &mut Node, fills: bool, paints: Vec) { + let old = std::mem::replace(if fills { &mut n.fills } else { &mut n.strokes }, paints); + for (i, p) in old.iter().enumerate() { + super::lists::rebind_edited_paint(n, fills, i, p); + } +} + +/// The order [`apply`] takes keys in: geometry first so rotation applies to the final size, variable +/// bindings last so paints set in the same call don't detach them. +fn apply_order(k: &str) -> u8 { + match k { "width" | "height" | "w" | "h" => 0, "rotation" => 2, "x" | "y" => 3, + "boundVariables" => 4, _ => 1, - }; + } +} + +/// 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(); let mut keys: Vec<(&String, &Value)> = props.iter().collect(); - keys.sort_by_key(|(k, _)| order(k)); + keys.sort_by_key(|(k, _)| apply_order(k)); for (k, v) in keys { let k = k.as_str(); match k { @@ -398,15 +412,15 @@ pub fn apply(cmd: &str, doc: &mut Document, id: NodeId, props: &Map doc.get_mut(id)?.fills = paints_of(v).ok_or_else(|| bad(cmd, "`fills` must be paints or colours"))?, + "fills" => set_paints(doc.get_mut(id)?, true, paints_of(v).ok_or_else(|| bad(cmd, "`fills` must be paints or colours"))?), "fill" => { let c = color_of(v).ok_or_else(|| bad(cmd, "`fill` must be a colour"))?; - doc.get_mut(id)?.fills = vec![Paint::solid(c)]; + set_paints(doc.get_mut(id)?, true, vec![Paint::solid(c)]); } - "strokes" => doc.get_mut(id)?.strokes = paints_of(v).ok_or_else(|| bad(cmd, "`strokes` must be paints or colours"))?, + "strokes" => set_paints(doc.get_mut(id)?, false, paints_of(v).ok_or_else(|| bad(cmd, "`strokes` must be paints or colours"))?), "stroke" => { let c = color_of(v).ok_or_else(|| bad(cmd, "`stroke` must be a colour"))?; - doc.get_mut(id)?.strokes = vec![Paint::solid(c)]; + set_paints(doc.get_mut(id)?, false, vec![Paint::solid(c)]); } "strokeWeight" => doc.get_mut(id)?.stroke.weight = f64_of(v).ok_or_else(|| bad(cmd, "`strokeWeight` must be a number"))?.max(0.0), "strokeAlign" => doc.get_mut(id)?.stroke.align = enum_of(v).ok_or_else(|| bad(cmd, "strokeAlign: inside|center|outside"))?, diff --git a/crates/engine/src/components_scale_tests.rs b/crates/engine/src/components_scale_tests.rs index c796dad8..68465932 100644 --- a/crates/engine/src/components_scale_tests.rs +++ b/crates/engine/src/components_scale_tests.rs @@ -747,3 +747,29 @@ fn alt_dropping_a_library_component_swaps_to_it() { assert!(doc.node(glyph).unwrap().overrides.is_empty()); let _ = std::fs::remove_dir_all(dir); } + +#[test] +fn a_fill_set_on_a_bound_instance_layer_detaches_it_as_an_override() { + // Without unbinding first: the explicit colour detaches the variable on the instance layer only, + // and stays through main edits until the override is reset. + let mut s = Session::new(); + let (chip, _) = bound_chip(&mut s); + let i = crate::params::id_of(&ex(&mut s, "instance.create", json!({"component": chip, "x": 200, "y": 0}))["id"]).unwrap(); + let bg = find_path(s.doc().unwrap(), i, &["Bg"]).unwrap(); + ex(&mut s, "node.set", json!({"ids": [bg], "props": {"fill": "#ff00ff"}})); + let n = s.doc().unwrap().node(bg).unwrap().clone(); + assert_eq!(n.fills[0].solid_color().unwrap().to_hex(), "FF00FF"); + assert!(!n.bound.contains_key("fill")); + assert!(n.overrides.contains("fills")); + let main_bg = find_path(s.doc().unwrap(), chip, &["Bg"]).unwrap(); + ex(&mut s, "node.set", json!({"ids": [main_bg], "props": {"height": 28}})); + ex(&mut s, "variable.set", json!({"variable": "Chip", "value": "#445566"})); + let doc = s.doc().unwrap(); + healthy(doc); + assert_eq!(doc.node(bg).unwrap().fills[0].solid_color().unwrap().to_hex(), "FF00FF"); + assert!(doc.node(main_bg).unwrap().bound.contains_key("fill"), "the main keeps its binding"); + ex(&mut s, "component.resetOverride", json!({"id": bg, "group": "fills"})); + let n = s.doc().unwrap().node(bg).unwrap(); + assert!(n.bound.contains_key("fill")); + assert_eq!(n.fills[0].solid_color().unwrap().to_hex(), "445566"); +} diff --git a/crates/engine/src/selection_colors.rs b/crates/engine/src/selection_colors.rs index e604c798..875c6416 100644 --- a/crates/engine/src/selection_colors.rs +++ b/crates/engine/src/selection_colors.rs @@ -9,7 +9,7 @@ use figcraft_doc::{Document, NodeId, NodeKind, StyleKind}; use serde::{Deserialize, Serialize}; use serde_json::{Value, json}; -use crate::cmd::lists::{BoundList, list_binding}; +use crate::cmd::lists::{BoundList, list_binding, rebind_edited_paint}; use crate::params::*; use crate::{CommandSpec, Result, Session, bad, spec}; @@ -153,37 +153,6 @@ fn checked_targets(s: &Session, p: &Value, cmd: &str) -> Result Ok(ts) } -/// Detach variables only where the explicit edit changed their value. In particular, an -/// opacity edit keeps the color binding and a range edit keeps other characters' node bindings. -fn unbind_changed(n: &mut figcraft_doc::Node, t: &PaintTarget, old: &Paint, new: &Paint) { - // Range fills override the node's paints; the node binding still belongs to other chars. - if t.range.is_some() { - return; - } - let list = if t.fills { BoundList::Fills } else { BoundList::Strokes }; - n.bound.retain(|key, _| { - let Some((l, index, field)) = list_binding(key) else { return true }; - if l != list || index != t.index { - return true; - } - match field { - "color" => match (&old.kind, &new.kind) { - (PaintKind::Solid { color: a }, PaintKind::Solid { color: b }) => a == b, - _ => false, - }, - "opacity" => old.opacity == new.opacity, - _ => { - let stop = field.strip_prefix("stops/").and_then(|s| s.strip_suffix("/color")).and_then(|s| s.parse::().ok()); - match (stop, &old.kind, &new.kind) { - (Some(i), PaintKind::Gradient(a), PaintKind::Gradient(b)) => a.stops.get(i).map(|s| s.color) == b.stops.get(i).map(|s| s.color), - (Some(_), _, _) => false, - _ => old == new, - } - } - } - }); -} - fn set_paint(s: &mut Session, p: &Value) -> Result { const CMD: &str = "selection.setPaint"; let targets = checked_targets(s, p, CMD)?; @@ -197,18 +166,22 @@ fn set_paint(s: &mut Session, p: &Value) -> Result { let old = slot.clone(); *slot = paint.clone(); let n = s.doc_mut()?.get_mut(target.id)?; - unbind_changed(n, target, &old, &paint); + // Variables detach only where the edit changed their value; range fills override the node's + // paints, whose bindings still belong to the other characters. if let Some((start, end)) = target.range { if let NodeKind::Text(t) = &mut n.kind { t.restyle(start, end, |style| style.fills = Some(paints.clone())); } - } else if target.fills { + continue; + } + if target.fills { n.fills = paints; n.styles.fill = None; } else { n.strokes = paints; n.styles.stroke = None; } + rebind_edited_paint(n, target.fills, target.index, &old); } Ok(json!({"ok": true})) } diff --git a/crates/engine/src/variables_io_tests.rs b/crates/engine/src/variables_io_tests.rs index 5c7305bb..3a57d9ea 100644 --- a/crates/engine/src/variables_io_tests.rs +++ b/crates/engine/src/variables_io_tests.rs @@ -371,3 +371,29 @@ fn variant_properties_follow_string_variables() { assert_eq!(main(&s), variants[1]); healthy(&s); } + +#[test] +fn paints_replaced_through_node_set_keep_unchanged_stop_bindings() { + let mut s = Session::new(); + let r = gradient_rect(&mut s); + ex(&mut s, "variable.bind", json!({"ids": [r], "field": "fills/0/stops/2/color", "variable": "brand"})); + ex(&mut s, "variable.bind", json!({"ids": [r], "field": "fills/0/stops/1/color", "variable": "ink"})); + let cur = |s: &Session| s.doc().unwrap().node(r).unwrap().fills[0].clone(); + // Removing the first stop moves both bindings down with their stops. + let mut p = cur(&s); + if let PaintKind::Gradient(g) = &mut p.kind { + g.stops.remove(0); + } + ex(&mut s, "node.set", json!({"ids": [r], "props": {"fills": [p]}})); + assert_eq!(bound_keys(&s, r), ["fills/0/stops/0/color", "fills/0/stops/1/color"]); + // Recolouring one bound stop detaches that stop only. + let mut p = cur(&s); + if let PaintKind::Gradient(g) = &mut p.kind { + g.stops[0].color = Color::WHITE; + } + ex(&mut s, "node.set", json!({"ids": [r], "props": {"fills": [p]}})); + assert_eq!(bound_keys(&s, r), ["fills/0/stops/1/color"]); + ex(&mut s, "variable.set", json!({"variable": "brand", "value": "#0000ff"})); + assert_eq!(stops(&s, r), ["FFFFFF", "0000FF"]); + healthy(&s); +}