Merge pull request #5860 from mockydevs/fix/variable-library-keeps-bindings

Variables: Load Variable Library keeps a bound text variable the library declares (#4136)

crates/engine/src/cmd/variablecmds.rs

@@ -856,7 +856,7 @@ pub fn specs() -> Vec<CommandSpec> {
             "Load Variable Library…",
             ["Window", "Variables"],
             None,
-            "{path? | xml? | dataBase64?} replace the document's variables and data sets with a variable library's (XML: `variableSets` › `variableSet` › `variables` of `variable` elements with `varName` and `trait`, and `sampleDataSets` of `sampleDataSet` elements with `dataSetName`) in one undo step (none when the document already holds what the library gives). Every variable of the document is deleted with its bindings and values, and every data set is deleted; then variables whose `trait` is `textcontent` are defined as text variables, unbound, and each data set takes a value for a variable from the `p` elements of the element named after it, one paragraph each. Variables of other kinds are left out and named in `skipped` and `warning`. Refused while the variables are locked when it would delete or define a variable → {variables, deleted, deletedDataSets, names, skipped, warning?}",
+            "{path? | xml? | dataBase64?} replace the document's variables and data sets with a variable library's (XML: `variableSets` › `variableSet` › `variables` of `variable` elements with `varName` and `trait`, and `sampleDataSets` of `sampleDataSet` elements with `dataSetName`) in one undo step (none when the document already holds what the library gives). Every data set is deleted, and every variable of the document is deleted with its bindings and values except a text variable the library also declares, which keeps its bindings; then the library's other variables whose `trait` is `textcontent` are defined as text variables, unbound, and each data set takes a value for a variable from the `p` elements of the element named after it, one paragraph each. Variables of other kinds are left out and named in `skipped` and `warning`. Refused while the variables are locked when it would delete or define a variable → {variables, deleted, deletedDataSets, names, skipped, warning?}",
             has_doc,
             load_library
         ),

crates/engine/src/cmd/variablecmds/library.rs

@@ -258,24 +258,33 @@ fn parse_library(xml: &str, cmd: &str) -> Result<Library> {
 
 /// Load Variable Library: replace the document's variables and data sets with a variable library's
 /// ([`parse_library`]) in one undo step, or none when the document already holds what the library
-/// gives. Every variable of the document is deleted with its bindings
-/// and values, and every data set is deleted. The library's text variables are then defined,
-/// unbound, and its data sets are added. The library's variables of other kinds are left out, named
-/// in `skipped` and in a warning.
+/// gives. Every data set is deleted, and every variable of the document is deleted with its
+/// bindings and values, except a text variable the library also declares: it stays bound to its
+/// objects, so the library's data sets change them (#4136; a template's bound variables take a
+/// library of the same names:
+/// <https://community.adobe.com/questions-652/illustrator-error-with-csv-files-812184>). The
+/// library's other text variables are then defined, unbound, and its data sets are added. The
+/// library's variables of other kinds are left out, named in `skipped` and in a warning.
 pub(super) fn load_library(s: &mut Session, p: &Value) -> Result<Value> {
     const C: &str = "variable.loadLibrary";
     let bytes = library_bytes(p, C)?;
     let xml = std::str::from_utf8(bytes.strip_prefix(b"\xEF\xBB\xBF").unwrap_or(&bytes)).map_err(|_| bad(C, "the variable library is not UTF-8"))?;
     let lib = parse_library(xml, C)?;
     let vars = &s.doc()?.doc.variables;
-    let deleted: Vec<String> = vars.variables.iter().map(|v| v.name.clone()).collect();
+    let kept = |v: &Variable| v.kind == VariableKind::Text && lib.text.contains(&v.name);
+    let deleted: Vec<String> = vars.variables.iter().filter(|v| !kept(v)).map(|v| v.name.clone()).collect();
     let deleted_sets: Vec<String> = vars.datasets.iter().map(|d| d.name.clone()).collect();
+    let added: Vec<Variable> = lib
+        .text
+        .iter()
+        .filter(|n| !vars.variables.iter().any(|v| kept(v) && &v.name == *n))
+        .map(|n| Variable { name: n.clone(), kind: VariableKind::Text })
+        .collect();
     // Lock Variables refuses New Variable and Delete Variable, so the load is refused when it would
     // delete or define a variable.
-    if !deleted.is_empty() || !lib.text.is_empty() {
+    if !deleted.is_empty() || !added.is_empty() {
         variables_unlocked(s).map_err(|why| EngineError::Disabled(C.into(), why))?;
     }
-    let added: Vec<Variable> = lib.text.iter().map(|n| Variable { name: n.clone(), kind: VariableKind::Text }).collect();
     let names: Vec<String> = lib.datasets.iter().map(|d| d.name.clone()).collect();
     // A load that leaves the variables and data sets as they were adds no undo step.
     super::edit_variables(s, "Load Variable Library", |vars| {

crates/engine/src/tests/variables.rs

@@ -2060,14 +2060,17 @@ fn a_variable_library_defines_text_variables_and_data_sets() {
     assert_eq!(from_file["names"], json!(["Row 1", "Row 2"]));
     let bom = [b"\xEF\xBB\xBF".as_slice(), xml.as_bytes()].concat();
     let from_base64 = s.execute("variable.loadLibrary", &json!({"dataBase64": vectorcraft_format::base64_encode(&bom)})).unwrap();
-    assert_eq!((from_base64["deleted"].clone(), from_base64["deletedDataSets"].clone()), (json!(["Name", "Note"]), json!(["Row 1", "Row 2"])));
+    // The library declares the document's text variables: they stay, and only the data sets go.
+    assert_eq!((from_base64["deleted"].clone(), from_base64["deletedDataSets"].clone()), (json!([]), json!(["Row 1", "Row 2"])));
     assert_eq!(s.doc().unwrap().doc.variables.datasets.len(), 2);
 }
 
 /// Load Variable Library replaces the document's variables and data sets: every variable goes with
-/// its bindings and values, a text variable of the same name included, and every data set goes,
-/// the active one included. The highlight of a variable that went highlights nothing. Undo brings
-/// back the variables, bindings, data sets and active data set as they were.
+/// its bindings and values, a variable of another kind with a library variable's name included,
+/// and every data set goes, the active one included. A text variable the library also declares
+/// stays bound (#4136), and the library's data sets then change its text. The highlight of a
+/// variable that went highlights nothing. Undo brings back the variables, bindings, data sets and
+/// active data set as they were.
 #[test]
 fn a_variable_library_replaces_every_variable_and_data_set() {
     let mut s = session();
@@ -2085,17 +2088,22 @@ fn a_variable_library_replaces_every_variable_and_data_set() {
 
     let xml = library(&[("Name", "textcontent"), ("Title", "textcontent")], &[("A", &[("Name", &["New"]), ("Title", &["Heading"])])]);
     let loaded = s.execute("variable.loadLibrary", &json!({"xml": xml})).unwrap();
-    assert_eq!(loaded["deleted"], json!(["Name", "Title", "Kept"]));
+    assert_eq!(loaded["deleted"], json!(["Title", "Kept"]));
     assert_eq!((loaded["deletedDataSets"].clone(), loaded["names"].clone()), (json!(["A", "Other"]), json!(["A"])));
     let listed = s.execute("variable.list", &json!({})).unwrap();
-    assert_eq!(listed["variables"], json!([{"name": "Name", "kind": "text", "bindings": 0}, {"name": "Title", "kind": "text", "bindings": 0}]));
+    assert_eq!(listed["variables"], json!([{"name": "Name", "kind": "text", "bindings": 1}, {"name": "Title", "kind": "text", "bindings": 0}]));
     let sets = s.execute("dataset.list", &json!({})).unwrap();
     assert_eq!(sets["datasets"].as_array().map(Vec::len), Some(1));
     assert_eq!(sets["datasets"][0]["values"], json!({"Name": {"text": "New"}, "Title": {"text": "Heading"}}));
     assert_eq!(sets["active"], Value::Null);
-    assert!(s.doc().unwrap().doc.variables.bindings.is_empty(), "the bindings went with the variables");
+    let bindings = &s.doc().unwrap().doc.variables.bindings;
+    assert_eq!(bindings.len(), 1, "the bindings went with the variables, but for Name's");
+    assert_eq!(bindings.get(&t).map(|v| v.len()), Some(1));
     assert!(s.doc().unwrap().variables_highlight.is_none(), "the highlighted variable went");
     assert_eq!(plain(&s, t), "Stays", "loading applies nothing");
+    s.execute("dataset.select", &json!({"name": "A"})).unwrap();
+    assert_eq!(plain(&s, t), "New", "the library's data set changes the text bound to Name");
+    s.execute("edit.undo", &json!({})).unwrap();
 
     s.execute("edit.undo", &json!({})).unwrap();
     assert_eq!(s.doc().unwrap().doc.variables, before, "one undo step brings everything back");
@@ -2133,11 +2141,17 @@ fn an_import_or_library_load_that_changes_nothing_records_no_undo_step() {
     let before = super::history(&s);
     let r = s.execute("variable.loadLibrary", &json!({"xml": xml})).unwrap();
     assert_eq!((super::history(&s), r["names"].clone()), (before, json!(["A", "B"])));
-    // Binding the variable makes the next load a change: it deletes the binding.
+    // A bound variable the library declares keeps its binding: loading it again changes nothing.
     s.execute("variable.bind", &json!({"variable": "Name", "ids": [t.0]})).unwrap();
     super::select_with_a_step_to_redo(&mut s, &[t]);
-    let undo = super::history(&s).0;
+    let before = super::history(&s);
     s.execute("variable.loadLibrary", &json!({"xml": xml})).unwrap();
+    assert_eq!(super::history(&s), before);
+    assert_eq!(s.execute("variable.list", &json!({})).unwrap()["variables"][0]["bindings"], json!(1));
+    // A library of other variables is a change: it deletes Name with its binding.
+    let other = library(&[("Note", "textcontent")], &[("A", &[("Note", &["Hi"])])]);
+    let undo = super::history(&s).0;
+    s.execute("variable.loadLibrary", &json!({"xml": other})).unwrap();
     assert_eq!(super::history(&s), (undo + 1, 0, true));
 
     // While the variables are locked, a library without text variables still loads its data sets,
@@ -2492,9 +2506,10 @@ fn a_saved_variable_library_near_the_size_cap_loads_back() {
 
 /// While the variables are locked, a library that would delete or define a variable is refused, as
 /// Delete Variable and New Variable are, and the document keeps its variables and data sets. That
-/// covers a library whose only variable is a text variable the document already has, and one
-/// without text variables loaded into a document with variables. A library without text variables
-/// loads into a document without variables, replacing its data sets.
+/// covers a library without text variables loaded into a document with variables, and one that
+/// adds a text variable to the document's. A library of the document's own text variables, and
+/// one without text variables loaded into a document without variables, delete and define none:
+/// they load, replacing the data sets.
 #[test]
 fn locked_variables_refuse_a_library_that_deletes_or_defines_variables() {
     let mut s = session();
@@ -2502,11 +2517,14 @@ fn locked_variables_refuse_a_library_that_deletes_or_defines_variables() {
     s.execute("dataset.new", &json!({"name": "A", "values": {"Name": "Old"}})).unwrap();
     s.execute("variable.lock", &json!({"locked": true})).unwrap();
     let before = s.doc().unwrap().doc.variables.clone();
-    for vars in [&[("Name", "textcontent")][..], &[("Shown", "other")][..]] {
+    for vars in [&[("Shown", "other")][..], &[("Name", "textcontent"), ("Note", "textcontent")][..]] {
         let e = s.execute("variable.loadLibrary", &json!({"xml": library(vars, &[("B", &[])])})).unwrap_err();
         assert!(matches!(&e, EngineError::Disabled(cmd, why) if cmd == "variable.loadLibrary" && why == "variables are locked"), "{e}");
         assert_eq!(s.doc().unwrap().doc.variables, before);
     }
+    // A library of the document's own text variable deletes and defines none: it loads its data sets.
+    let r = s.execute("variable.loadLibrary", &json!({"xml": library(&[("Name", "textcontent")], &[("B", &[])])})).unwrap();
+    assert_eq!((r["deleted"].clone(), r["deletedDataSets"].clone()), (json!([]), json!(["A"])));
 
     let mut s = session();
     s.execute("dataset.new", &json!({"name": "A"})).unwrap();

docs/mcp.md

@@ -1790,8 +1790,10 @@ already walks Image Trace. Short agent entry points:
   `dataset.import`, adds no undo step when it leaves the variables and data sets as they were. The library
   is XML whose `variableSets` element holds `variableSet` elements, each with a `variables` element of `variable`
   elements (`varName`, `trait`) and a `sampleDataSets` element of `sampleDataSet` elements (`dataSetName`). Every
-  variable of the document is deleted with its bindings and values, and every data set is deleted. Then the library's
-  variables whose `trait` is `textcontent` are defined as text variables, unbound. In each data set, the element named
+  data set is deleted, and every variable of the document is deleted with its bindings and values, except a text
+  variable the library also declares: it keeps its bindings, so selecting one of the library's data sets changes the
+  objects bound to it. Then the library's other variables whose `trait` is `textcontent` are defined as text
+  variables, unbound. In each data set, the element named
   after a text variable gives its value: the text of its `p` elements, one paragraph each. An element without a `p`
   element gives no value. Variables of other kinds are left out with their values: the reply lists them in `skipped`
   (`name`, `trait`), and its `warning` names each `trait` with its count. The reply's `deleted` and `deletedDataSets`

docs/target-app-parity.md

@@ -634,7 +634,7 @@ Moved from the ROADMAP's "Shipped so far" on 2026-10-10. Update it when a featur
   - Variables (data merge): text, visibility and image variables bound to objects (an object may carry several), dataset rows applied in one undo step (`variable.*`, `dataset.*`) from a Variables panel and Window › Variables, with dialogs to define, bind and edit, Make Text/Visibility/Image Dynamic, Capture/Update Data Set, CSV import/export (`dataset.import` / `dataset.export`), and a row in italics when an edit leaves the art not matching it; an image dataset value is a file path and apply keeps the object's bounds; bindings let go of deleted art and locked art is left alone; every command, like the rest, is drivable over MCP and the CLI.
   - Variables: Import Data Sets… (`dataset.import`) reads tab-separated files as well as comma-separated ones. Every header cell is a variable, and the rows replace the document's data sets as Data Set 1, Data Set 2 and so on. An import error about a row's values gives the file line the row starts on.
   - Variables: Import Data Sets… (`dataset.import`) defines the variables a file's header names that the document lacks, unbound. `#Shown` defines a visibility variable, `@Photo` defines an image variable, and a header without a prefix defines a text variable. A header that is a variable's exact name keeps naming it, and any other `%` header (graph data files) is refused.
-  - Variables: Load Variable Library… in the Variables panel menu (`variable.loadLibrary`) replaces the document's variables and data sets with a variable library's, read from its XML file, in one undo step: every variable is deleted with its bindings and values, and every data set is deleted; then the library's text variables are defined, unbound, and its data sets take the text of each value's `p` elements, one paragraph each. Variables of other kinds are left out with a warning that names their kinds.
+  - Variables: Load Variable Library… in the Variables panel menu (`variable.loadLibrary`) replaces the document's variables and data sets with a variable library's, read from its XML file, in one undo step: every data set is deleted, and every variable is deleted with its bindings and values, except a text variable the library also declares, which stays bound so the library's data sets change its objects (#4136); then the library's other text variables are defined, unbound, and its data sets take the text of each value's `p` elements, one paragraph each. Variables of other kinds are left out with a warning that names their kinds.
   - Variables: Save Variable Library… in the Variables panel menu (`variable.saveLibrary`) writes the document's text variables and data sets as a variable library's XML file, which Load Variable Library reads back with the same text variables, data set names and text values. Variables of other kinds are left out with a warning that names their kinds.
   - JSON-lines control channel with real egui pointer and keyboard injection.
   - Synthetic input validates complete gestures before queue mutation: at most 256 drag steps, 16 clicks and 1,024 pending events; up to 64 control requests per frame, with later requests retained for the next frame (#1709).