Merge pull request #811 from MortenHusted/fix/bound-paint-edits-detach

M3: an explicit paint edit detaches the variable bound to what it changed

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<String> {
+    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");
+}

crates/engine/src/cmd/edit.rs

@@ -1342,7 +1342,7 @@ fn paint_set(s: &mut Session, p: &Value, fills: bool) -> Result<Value> {
         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 {

crates/engine/src/cmd/lists.rs

@@ -112,7 +112,8 @@ pub fn stop_binding(field: &str) -> Option<usize> {
 /// 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<usize> {
     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<String>)> = 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/<j>/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`.

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<String, Value>) -> 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<Paint>) {
+    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<String, Value>) -> 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<String, Valu
                 let b = v.as_str().and_then(figcraft_color::BlendMode::from_name).ok_or_else(|| bad(cmd, "unknown blend mode"))?;
                 doc.get_mut(id)?.blend = b;
             }
-            "fills" => 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"))?,

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");
+}

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<Vec<PaintTarget>
     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::<usize>().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<Value> {
     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<Value> {
         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}))
 }

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);
+}