From eb0f2ff2e475375e120630002dc88c5f36b66102 Mon Sep 17 00:00:00 2001 From: Fahri Meral Date: Fri, 9 Oct 2026 20:39:55 +0300 Subject: [PATCH] variables: let one object be driven by several variables MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `bindings` mapped an object id to a single variable name, so binding an object to a second variable silently dropped the first — a binding lost with nothing to show for it. The reference app binds the selected object to the selected variable, one click at a time, and nowhere says a second binding replaces the first (https://helpx.adobe.com/illustrator/desktop/automate-visualize-data/ automate-actions/work-with-variables.html), so this allows several instead. `bindings` is now a set of variable names per object. `dataset.select` walks them in the order the variables are defined, which keeps two text variables on one object deterministic (the later one's value is what is left, and `applied` counts both). `variable.bind` reports `bound` (added) and `already` (was there already); `variable.unbind` takes an optional `variable` to drop one binding instead of all of them. This is done before the field ships rather than after: `variables` has never been in a released file, so the shape can still change for free. Once a version with it is out, the same change would need a format version bump and a migration, since `crates/format` keeps every version from 1 loading. A strict superset of what worked before: every binding a document had is still there and still applied, in the same order. cargo xtask ci: all 7 steps pass. --- crates/doc/src/variables.rs | 35 ++++++++++++-- crates/engine/src/cmd/variablecmds.rs | 53 ++++++++++++++------- crates/engine/src/tests_variables.rs | 66 ++++++++++++++++++++++----- 3 files changed, 122 insertions(+), 32 deletions(-) diff --git a/crates/doc/src/variables.rs b/crates/doc/src/variables.rs index 9ba5de03..5c23c70d 100644 --- a/crates/doc/src/variables.rs +++ b/crates/doc/src/variables.rs @@ -2,7 +2,7 @@ //! them all at once. One template document produces many variants (badges, price //! lists, localized copies) by selecting a dataset instead of editing each object. -use std::collections::BTreeMap; +use std::collections::{BTreeMap, BTreeSet}; use serde::{Deserialize, Serialize}; @@ -71,7 +71,13 @@ pub struct DataSet { } /// The variable state of a document: definitions, datasets, bindings -/// (object id → variable name) and which dataset is active. +/// (object id → the variables bound to it) and which dataset is active. +/// +/// An object may be driven by several variables — the reference app binds the selected object +/// to the selected variable, one click at a time, and never says a second binding replaces the +/// first — so a binding is a set. `dataset.select` applies them in the order the variables are +/// defined, which is what makes two text variables on one object deterministic: the last one +/// wins, and `applied` counts both. #[derive(Clone, Debug, Default, PartialEq, Serialize, Deserialize)] pub struct Variables { #[serde(default, skip_serializing_if = "Vec::is_empty")] @@ -79,7 +85,7 @@ pub struct Variables { #[serde(default, skip_serializing_if = "Vec::is_empty")] pub datasets: Vec, #[serde(default, skip_serializing_if = "BTreeMap::is_empty")] - pub bindings: BTreeMap, + pub bindings: BTreeMap>, #[serde(default, skip_serializing_if = "Option::is_none")] pub active_dataset: Option, } @@ -95,9 +101,25 @@ impl Variables { self.datasets.iter().find(|d| d.name == name) } + /// Bind `id` to the variable `name`. True when it was not bound to it already. + pub fn bind(&mut self, id: NodeId, name: &str) -> bool { + self.bindings.entry(id).or_default().insert(name.to_string()) + } + + /// Drop `id`'s binding to `name` (and the binding itself when that was its last variable). + /// True when there was one. + pub fn unbind(&mut self, id: NodeId, name: &str) -> bool { + let Some(bound) = self.bindings.get_mut(&id) else { return false }; + let dropped = bound.remove(name); + if bound.is_empty() { + self.bindings.remove(&id); + } + dropped + } + /// The objects bound to the variable `name`, in document order. pub fn objects_of(&self, name: &str) -> Vec { - let mut ids: Vec = self.bindings.iter().filter(|(_, v)| *v == name).map(|(id, _)| *id).collect(); + let mut ids: Vec = self.bindings.iter().filter(|(_, v)| v.contains(name)).map(|(id, _)| *id).collect(); ids.sort_unstable(); ids } @@ -111,7 +133,10 @@ impl Variables { for dataset in &mut self.datasets { dataset.values.retain(|k, _| !names.contains(k)); } - self.bindings.retain(|_, v| !names.contains(v)); + for bound in self.bindings.values_mut() { + bound.retain(|v| !names.contains(v)); + } + self.bindings.retain(|_, v| !v.is_empty()); before - self.variables.len() } } diff --git a/crates/engine/src/cmd/variablecmds.rs b/crates/engine/src/cmd/variablecmds.rs index a7b260a5..58438ccd 100644 --- a/crates/engine/src/cmd/variablecmds.rs +++ b/crates/engine/src/cmd/variablecmds.rs @@ -92,7 +92,7 @@ fn list(s: &mut Session, _p: &Value) -> Result { .variables .iter() .map(|v| { - let bindings = st.doc.variables.bindings.values().filter(|b| *b == &v.name).count(); + let bindings = st.doc.variables.objects_of(&v.name).len(); json!({"name": v.name, "kind": v.kind.label(), "bindings": bindings}) }) .collect(); @@ -123,27 +123,35 @@ fn bind(s: &mut Session, p: &Value) -> Result { Some(_) => {} } } - // An object holds one binding, so binding it again replaces what it had; say which. - let replaced: Vec = ids.iter().filter(|id| doc.variables.bindings.get(id).is_some_and(|b| *b != name)).map(|id| id.0).collect(); - s.edit("Bind Variable", |d, _| { - for id in &ids { - d.variables.bindings.insert(*id, name.clone()); - } - Ok(()) - })?; - Ok(json!({"variable": name, "ids": ids.iter().map(|i| i.0).collect::>(), "replaced": replaced})) + // Binding again is not a second binding, and says so rather than counting as a change. + let already: Vec = ids.iter().filter(|id| doc.variables.objects_of(&name).contains(id)).map(|id| id.0).collect(); + let n = s.edit("Bind Variable", |d, _| Ok(ids.iter().filter(|id| d.variables.bind(**id, &name)).count()))?; + Ok(json!({"variable": name, "ids": ids.iter().map(|i| i.0).collect::>(), "bound": n, "already": already})) } fn unbind(s: &mut Session, p: &Value) -> Result { + const C: &str = "variable.unbind"; let ids = match ids_param(p, "ids") { Some(v) => v, None => selected_roots(s)?, }; + let name = match str_param(p, "variable").filter(|v| !v.is_empty()) { + Some(v) => { + let v = v.to_string(); + if s.doc()?.doc.variables.variable(&v).is_none() { + return Err(bad(C, format!("no variable named `{v}`"))); + } + Some(v) + } + None => None, + }; let n = s.edit("Unbind Variable", |d, _| { let mut n = 0; for id in &ids { - if d.variables.bindings.remove(id).is_some() { - n += 1; + // With `variable` only that one binding goes; without it, every one the object has. + match &name { + Some(v) => n += usize::from(d.variables.unbind(*id, v)), + None => n += usize::from(d.variables.bindings.remove(id).is_some()), } } Ok(n) @@ -207,10 +215,15 @@ fn apply_dataset(s: &mut Session, name: &str) -> Result { const C: &str = "dataset.select"; let (applied, skipped) = s.edit("Apply Data Set", |d, _| { let ds = d.variables.dataset(name).cloned().ok_or_else(|| bad(C, format!("no dataset named `{name}`")))?; - let bindings: Vec<(NodeId, String)> = d.variables.bindings.iter().map(|(id, v)| (*id, v.clone())).collect(); + // One object may be driven by several variables; walk them in definition order, so two + // text variables on the same object land the later one's value and both are counted. + let mut pairs: Vec<(NodeId, String)> = Vec::new(); + for v in &d.variables.variables { + pairs.extend(d.variables.objects_of(&v.name).into_iter().map(|id| (id, v.name.clone()))); + } let mut applied = 0usize; let mut skipped = 0usize; - for (id, var) in bindings { + for (id, var) in pairs { // A binding whose variable is gone, whose value this row doesn't carry, or whose // value doesn't fit the variable's kind, is left alone and counted, never // silently reinterpreted. @@ -288,11 +301,19 @@ pub fn specs() -> Vec { "Bind Variable", ["Window", "Variables"], None, - "{variable, ids?} bind objects (default: the selection) to a variable; text needs type, locked art is refused. An object holds one binding, so this replaces what it had → {ids, replaced}", + "{variable, ids?} bind objects (default: the selection) to a variable; text needs type, locked art is refused. An object may hold several variables, and binding it to one it already has changes nothing → {bound, already}", has_doc, bind ), - cmd!("variable.unbind", "Unbind Variable", ["Window", "Variables"], None, "{ids?} drop bindings (default: the selection)", has_doc, unbind), + cmd!( + "variable.unbind", + "Unbind Variable", + ["Window", "Variables"], + None, + "{ids?, variable?} drop bindings (default: the selection): `variable` drops that one, else every variable the object has", + has_doc, + unbind + ), cmd!("dataset.new", "New Data Set…", ["Window", "Variables"], None, "{name, values?: {variable: value}} add a dataset", has_doc, dataset_new), cmd!( "dataset.set", diff --git a/crates/engine/src/tests_variables.rs b/crates/engine/src/tests_variables.rs index 1db01ea8..8e69472f 100644 --- a/crates/engine/src/tests_variables.rs +++ b/crates/engine/src/tests_variables.rs @@ -167,24 +167,68 @@ fn delete_reports_what_it_actually_removed() { assert_eq!(s.execute("dataset.delete", &json!({"names": ["D"]})).unwrap()["deleted"], 0); } -/// An object holds one binding, so binding it again replaces what it had, and the reply says -/// which objects were rebound rather than letting the old variable silently lose them. +/// An object may be driven by several variables, so binding it again adds to what it has +/// instead of quietly dropping the earlier one. #[test] -fn binding_again_replaces_and_says_so() { +fn one_object_can_be_driven_by_several_variables() { let mut s = session(); let t = text(&mut s, "x"); - s.execute("variable.define", &json!({"name": "First", "kind": "text"})).unwrap(); - s.execute("variable.define", &json!({"name": "Second", "kind": "text"})).unwrap(); - s.execute("variable.bind", &json!({"variable": "First", "ids": [t.0]})).unwrap(); - let v = s.execute("variable.bind", &json!({"variable": "Second", "ids": [t.0]})).unwrap(); - assert_eq!(v["replaced"], json!([t.0])); + let r = rect(&mut s); + s.execute("variable.define", &json!({"name": "Title", "kind": "text"})).unwrap(); + s.execute("variable.define", &json!({"name": "Price", "kind": "text"})).unwrap(); + s.execute("variable.define", &json!({"name": "Badge", "kind": "visibility"})).unwrap(); + + // A text object driven by two text variables keeps both. + assert_eq!(s.execute("variable.bind", &json!({"variable": "Title", "ids": [t.0]})).unwrap()["bound"], 1); + let v = s.execute("variable.bind", &json!({"variable": "Price", "ids": [t.0]})).unwrap(); + assert_eq!((v["bound"].clone(), v["already"].clone()), (json!(1), json!([])), "Title is untouched"); + // A rectangle driven by a visibility variable as well as nothing else yet. + s.execute("variable.bind", &json!({"variable": "Badge", "ids": [r.0]})).unwrap(); + s.execute("dataset.new", &json!({"name": "D", "values": {"Title": "Hat", "Price": "9.99", "Badge": false}})).unwrap(); + + let v = s.execute("dataset.select", &json!({"name": "D"})).unwrap(); + assert_eq!((v["applied"].as_u64(), v["skipped"].as_u64()), (Some(3), Some(0)), "every binding applied"); + // Both text variables are counted; the later one in definition order is what is left. + assert_eq!(plain(&s, t), "9.99"); + assert!(!s.doc().unwrap().doc.node(r).unwrap().visible); + let vars = s.execute("variable.list", &json!({})).unwrap(); let of = |n: &str| vars["variables"].as_array().unwrap().iter().find(|v| v["name"] == n).unwrap()["bindings"].as_u64(); - assert_eq!((of("First"), of("Second")), (Some(0), Some(1))); - // Rebinding to the same variable changes nothing and reports no replacement. - assert_eq!(s.execute("variable.bind", &json!({"variable": "Second", "ids": [t.0]})).unwrap()["replaced"], json!([])); + assert_eq!((of("Title"), of("Price"), of("Badge")), (Some(1), Some(1), Some(1))); + + // Binding again to one it already has is not a change, and says so. + let v = s.execute("variable.bind", &json!({"variable": "Price", "ids": [t.0]})).unwrap(); + assert_eq!((v["bound"].clone(), v["already"].clone()), (json!(0), json!([t.0]))); + // Unbinding one variable leaves the object's others alone. + assert_eq!(s.execute("variable.unbind", &json!({"ids": [t.0], "variable": "Price"})).unwrap()["unbound"], 1); + s.execute("edit.undo", &json!({})).unwrap(); + let vars = s.execute("variable.list", &json!({})).unwrap(); + assert_eq!(vars["variables"][1]["bindings"], 1, "the other binding is still there"); + assert!(s.execute("variable.unbind", &json!({"ids": [t.0], "variable": "Ghost"})).is_err()); + + // Deleting a variable drops only its own binding. + s.execute("variable.delete", &json!({"names": ["Price"]})).unwrap(); + let vars = s.execute("variable.list", &json!({})).unwrap(); + assert_eq!(vars["variables"][0]["bindings"], 1, "Title still drives the text"); +} + +/// `unbound` counts what was actually dropped, like `variable.delete` does. +#[test] +fn unbind_reports_what_it_actually_removed() { + let mut s = session(); + let t = text(&mut s, "x"); + s.execute("variable.define", &json!({"name": "Title", "kind": "text"})).unwrap(); + s.execute("variable.define", &json!({"name": "Price", "kind": "text"})).unwrap(); + s.execute("variable.bind", &json!({"variable": "Title", "ids": [t.0]})).unwrap(); + s.execute("variable.bind", &json!({"variable": "Price", "ids": [t.0]})).unwrap(); + assert_eq!(s.execute("variable.unbind", &json!({"ids": [t.0], "variable": "Title"})).unwrap()["unbound"], 1); + assert_eq!(s.execute("variable.unbind", &json!({"ids": [t.0], "variable": "Title"})).unwrap()["unbound"], 0, "already gone"); + // Without `variable` the object's last binding goes and the entry with it. assert_eq!(s.execute("variable.unbind", &json!({"ids": [t.0]})).unwrap()["unbound"], 1); assert_eq!(s.execute("variable.unbind", &json!({"ids": [t.0]})).unwrap()["unbound"], 0); + // An object with no binding left is not kept as an empty set. + let v: Value = serde_json::from_str(&serde_json::to_string(&s.doc().unwrap().doc).unwrap()).unwrap(); + assert!(v["variables"].get("bindings").is_none_or(|v| v == &json!({})), "{v}"); } /// Locked art is the document's to rewrite only while it isn't locked.