Merge pull request #1958 from MortenHusted/fix/import-variable-id-collisions

M3: importing a file keeps its own variables and styles when their ids are already taken

crates/doc/src/ids.rs

@@ -14,6 +14,8 @@
 //! space 0. The session server accepts a new object only with an id in its creator's space or
 //! space 0, so two people never make the same id.
 
+use std::collections::BTreeSet;
+
 use crate::{CollectionId, Document, ModeId, NodeId, StyleId, VariableId};
 
 /// What style ids start with.
@@ -74,25 +76,42 @@ impl Document {
 
     /// A new style id.
     pub fn new_style_id(&mut self) -> StyleId {
-        let taken: Vec<u64> = self.styles.iter().filter_map(|s| id_number(&s.id, STYLE_ID_PREFIX)).collect();
+        self.new_style_id_avoiding(&BTreeSet::new())
+    }
+
+    /// A new style id that is none of `avoid` either: ids of styles about to come in with a file
+    /// being imported.
+    pub fn new_style_id_avoiding(&mut self, avoid: &BTreeSet<StyleId>) -> StyleId {
+        let taken: Vec<u64> = self.styles.iter().map(|s| &s.id).chain(avoid).filter_map(|id| id_number(id, STYLE_ID_PREFIX)).collect();
         if let Some(n) = self.session_number(taken) {
             return format!("{STYLE_ID_PREFIX}{n}");
         }
         (self.styles.len() + 1..)
             .map(|k| format!("{STYLE_ID_PREFIX}{k}"))
-            .find(|id| self.style(id).is_none())
+            .find(|id| self.style(id).is_none() && !avoid.contains(id))
             .unwrap_or_else(|| format!("{STYLE_ID_PREFIX}0"))
     }
 
     /// A new variable id.
     pub fn new_variable_id(&mut self) -> VariableId {
-        let taken: Vec<u64> = self.variables.variables.iter().filter_map(|v| id_number(&v.id, VARIABLE_ID_PREFIX)).collect();
+        self.new_variable_id_avoiding(&BTreeSet::new())
+    }
+
+    /// A new variable id that is none of `avoid` either (see [`Self::new_style_id_avoiding`]).
+    pub fn new_variable_id_avoiding(&mut self, avoid: &BTreeSet<VariableId>) -> VariableId {
+        let taken = self.variables.variables.iter().map(|v| &v.id).chain(avoid).filter_map(|id| id_number(id, VARIABLE_ID_PREFIX)).collect();
         self.new_numbered_id(VARIABLE_ID_PREFIX, taken)
     }
 
     /// A new variable collection id.
     pub fn new_collection_id(&mut self) -> CollectionId {
-        let taken: Vec<u64> = self.variables.collections.iter().filter_map(|c| id_number(&c.id, COLLECTION_ID_PREFIX)).collect();
+        self.new_collection_id_avoiding(&BTreeSet::new())
+    }
+
+    /// A new variable collection id that is none of `avoid` either (see
+    /// [`Self::new_style_id_avoiding`]).
+    pub fn new_collection_id_avoiding(&mut self, avoid: &BTreeSet<CollectionId>) -> CollectionId {
+        let taken = self.variables.collections.iter().map(|c| &c.id).chain(avoid).filter_map(|id| id_number(id, COLLECTION_ID_PREFIX)).collect();
         self.new_numbered_id(COLLECTION_ID_PREFIX, taken)
     }
 

crates/doc/src/ids_tests.rs

@@ -1,6 +1,8 @@
 //! Ids of styles, variables, collections, modes and comments: unchanged outside a live session,
 //! from the client's own space during one.
 
+use std::collections::BTreeSet;
+
 use crate::ids::{COLLECTION_ID_PREFIX, id_number, id_space, mode_number, space_of};
 use crate::{Collection, Comment, Document, Message, Mode, NodeId, NodeKind, Style, StyleKind};
 
@@ -93,6 +95,20 @@ fn ids_of_the_space_already_in_the_document_stay_taken() {
     assert_eq!(e.new_style_id(), format!("S:{}", C3 | 1));
 }
 
+#[test]
+fn new_ids_can_avoid_ids_about_to_come_in() {
+    let mut d = Document::new("a");
+    d.styles.push(style("S:1"));
+    let avoid: BTreeSet<String> = ["S:2", "S:3", "VariableID:4", "VariableCollectionId:2", "lib:VariableID:9"].map(String::from).into();
+    assert_eq!(d.new_style_id_avoiding(&avoid), "S:4");
+    assert_eq!(d.new_variable_id_avoiding(&avoid), "VariableID:5");
+    assert_eq!(d.new_collection_id_avoiding(&avoid), "VariableCollectionId:3");
+    // In a session the numbers come from the client's space, above those of it to avoid.
+    let avoid: BTreeSet<String> = [format!("VariableID:{}", C3 | 7)].into();
+    d.set_client_id(3);
+    assert_eq!(d.new_variable_id_avoiding(&avoid), format!("VariableID:{}", C3 | 8));
+}
+
 #[test]
 fn the_highest_space_counts_every_kind_of_id() {
     let mut d = Document::new("a");

crates/engine/src/cmd/file.rs

@@ -15,6 +15,7 @@ use crate::params::{bool_of, boolean, get, id, num, num_or, req_index, req_strin
 use crate::{DocState, EngineError, Result, Session, bad, spec};
 
 mod export;
+mod import_ids;
 mod load;
 mod open;
 mod save;

crates/engine/src/cmd/file/import_ids.rs

@@ -0,0 +1,254 @@
+//! Ids a file being imported brings that the document already uses for something else.
+//!
+//! Style, variable and collection ids are numbered per file (`S:1`, `VariableID:1`,
+//! `VariableCollectionId:1`; see [`figcraft_doc::ids`]), so two files made apart use the same ones
+//! for unrelated things. Kept as they were, the imported layers would show the document's
+//! variables and styles instead of their own.
+
+use std::collections::{BTreeMap, BTreeSet};
+
+use figcraft_doc::ids::MODE_ID_INFIX;
+use figcraft_doc::proto::{Action, Expr, MAX_EXPR_DEPTH, walk_mut};
+use figcraft_doc::{Document, ModeId, NodeId, NodeKind, VariableValue, Variables};
+
+/// A style, variable or collection of the file being imported.
+#[derive(Clone, Copy, Debug, PartialEq, Eq, PartialOrd, Ord)]
+enum Asset<'a> {
+    Style(&'a str),
+    Variable(&'a str),
+    Collection(&'a str),
+}
+
+/// New ids for styles, variables and collections of a file being imported (its id → the new one).
+#[derive(Debug, Default)]
+pub(super) struct Renames {
+    styles: BTreeMap<String, String>,
+    variables: BTreeMap<String, String>,
+    collections: BTreeMap<String, String>,
+}
+
+impl Renames {
+    /// The new ids `other`'s styles, variables and collections take when appended to `doc`.
+    ///
+    /// One whose id `doc` doesn't use keeps it. One `doc` already has is shared and keeps its id:
+    /// the same content under the same id, naming only things that are shared too (the same file
+    /// imported again). A collection and its variables are shared only all together, so variables
+    /// of one file never join a collection of another. Library copies keep their ids and are shared
+    /// with the document's copies, as before (`merge_store` records them). Every other id `doc`
+    /// already uses is replaced by a new one from `doc`'s allocators, clear of `other`'s ids too.
+    pub(super) fn new(doc: &mut Document, other: &Document) -> Renames {
+        let (theirs, libs) = (&other.variables, &other.libraries);
+        // Each with whether `doc` has the same under its id (None: nothing there), and whether
+        // it's a library copy.
+        let styles = other.styles.iter().map(|s| {
+            let here = doc.styles.iter().find(|m| m.id == s.id);
+            (Asset::Style(&s.id), here.map(|m| m == s), libs.style_import(&s.id).is_some())
+        });
+        let colls = theirs.collections.iter().map(|c| {
+            let here = doc.variables.collection_by_id(&c.id);
+            (Asset::Collection(&c.id), here.map(|m| m == c), libs.collection_import(&c.id).is_some())
+        });
+        let vars = theirs.variables.iter().map(|v| {
+            let here = doc.variables.get_by_id(&v.id);
+            (Asset::Variable(&v.id), here.map(|m| m == v), libs.variable_import(&v.id).is_some())
+        });
+        // `taken`: ids `doc` uses; `differs`: what `doc` doesn't have the same of.
+        let (mut taken, mut copies, mut differs) = (BTreeSet::new(), BTreeSet::new(), BTreeSet::new());
+        for (a, same, copy) in styles.chain(colls).chain(vars) {
+            match same {
+                None => {
+                    differs.insert(a);
+                }
+                Some(same) => {
+                    taken.insert(a);
+                    if copy {
+                        copies.insert(a);
+                    } else if !same {
+                        differs.insert(a);
+                    }
+                }
+            }
+        }
+        // What differs makes what names it differ (library copies aside), and a collection and its
+        // variables differ together.
+        let mut named_by: BTreeMap<Asset, Vec<Asset>> = BTreeMap::new();
+        for v in &theirs.variables {
+            let (var, coll) = (Asset::Variable(&v.id), Asset::Collection(&v.collection));
+            named_by.entry(coll).or_default().push(var);
+            named_by.entry(var).or_default().push(coll);
+            for r in v.values.values().flat_map(VariableValue::references).chain(&v.extends) {
+                named_by.entry(Asset::Variable(r)).or_default().push(var);
+            }
+        }
+        for c in &theirs.collections {
+            if let Some(e) = &c.extends {
+                named_by.entry(Asset::Collection(e)).or_default().push(Asset::Collection(&c.id));
+            }
+        }
+        for s in &other.styles {
+            for v in s.bound.values() {
+                named_by.entry(Asset::Variable(v)).or_default().push(Asset::Style(&s.id));
+            }
+        }
+        let mut todo: Vec<Asset> = differs.iter().copied().collect();
+        while let Some(a) = todo.pop() {
+            for &n in named_by.get(&a).into_iter().flatten() {
+                if !copies.contains(&n) && differs.insert(n) {
+                    todo.push(n);
+                }
+            }
+        }
+        let renamed = |a: Asset| taken.contains(&a) && differs.contains(&a);
+        Renames {
+            collections: fresh(
+                theirs.collections.iter().map(|c| &c.id),
+                |id| renamed(Asset::Collection(id)),
+                |avoid| doc.new_collection_id_avoiding(avoid),
+            ),
+            variables: fresh(theirs.variables.iter().map(|v| &v.id), |id| renamed(Asset::Variable(id)), |avoid| doc.new_variable_id_avoiding(avoid)),
+            styles: fresh(other.styles.iter().map(|s| &s.id), |id| renamed(Asset::Style(id)), |avoid| doc.new_style_id_avoiding(avoid)),
+        }
+    }
+
+    pub(super) fn is_empty(&self) -> bool {
+        self.styles.is_empty() && self.variables.is_empty() && self.collections.is_empty()
+    }
+
+    /// `other` with the renamed styles, variables and collections under their new ids, and what
+    /// names them following: layers' style links (text ranges too), bindings, explicit modes and
+    /// prototype actions; styles' bindings; variables' collections, aliases and calculations; what
+    /// extended collections and their variables extend; and the ids of the renamed collections'
+    /// modes, which name their collection (`<collection>/mode<k>`).
+    pub(super) fn applied(&self, other: &Document) -> Document {
+        let theirs = &other.variables;
+        let mut doc = other.clone();
+        let (styles, vars, colls) = (&self.styles, &self.variables, &self.collections);
+        for s in &mut doc.styles {
+            rename(styles, &mut s.id);
+            s.bound.values_mut().for_each(|v| rename(vars, v));
+        }
+        for c in &mut doc.variables.collections {
+            for m in &mut c.modes {
+                self.mode(theirs, &c.id, &mut m.id);
+            }
+            for id in std::iter::once(&mut c.id).chain(&mut c.extends) {
+                rename(colls, id);
+            }
+        }
+        for v in &mut doc.variables.variables {
+            let values = std::mem::take(&mut v.values).into_iter().map(|(mut mode, mut val)| {
+                self.mode(theirs, &v.collection, &mut mode);
+                val.references_mut().into_iter().for_each(|r| rename(vars, r));
+                (mode, val)
+            });
+            v.values = values.collect();
+            let overrides = std::mem::take(&mut v.overrides).into_iter().map(|mut mode| {
+                self.mode(theirs, &v.collection, &mut mode);
+                mode
+            });
+            v.overrides = overrides.collect();
+            rename(colls, &mut v.collection);
+            for id in std::iter::once(&mut v.id).chain(&mut v.extends) {
+                rename(vars, id);
+            }
+        }
+        let ids: Vec<NodeId> = doc.ids().collect();
+        for id in ids {
+            let Some(n) = doc.node_mut(id) else { continue };
+            let links = [&mut n.styles.fill, &mut n.styles.stroke, &mut n.styles.effect, &mut n.styles.grid, &mut n.styles.text];
+            for s in links.into_iter().flatten() {
+                rename(styles, s);
+            }
+            if let NodeKind::Text(t) = &mut n.kind {
+                t.map_text_styles(|s| Some(styles.get(s).cloned().unwrap_or_else(|| s.to_string())));
+            }
+            n.bound.values_mut().for_each(|v| rename(vars, v));
+            n.var_modes = std::mem::take(&mut n.var_modes)
+                .into_iter()
+                .map(|(mut c, mut mode)| {
+                    self.mode(theirs, &c, &mut mode);
+                    rename(colls, &mut c);
+                    (c, mode)
+                })
+                .collect();
+            for it in &mut n.interactions {
+                walk_mut(&mut it.actions, &mut |a| self.action(theirs, a));
+            }
+        }
+        doc
+    }
+
+    /// Rename mode `mode` of collection `of` (`theirs`: the imported file's variables as they were).
+    /// A mode id names the collection it was made in: `of`, or one `of` extends (an extended
+    /// collection has the modes of the one it extends). Library copies keep their library's mode
+    /// ids, which may name one of the file's own collections.
+    fn mode(&self, theirs: &Variables, of: &str, mode: &mut ModeId) {
+        let mut at = Some(of);
+        for _ in 0..=theirs.collections.len() {
+            let Some(c) = at else { return };
+            if let Some(k) = mode.strip_prefix(c).and_then(|rest| rest.strip_prefix(MODE_ID_INFIX)) {
+                if let Some(new) = self.collections.get(c) {
+                    *mode = format!("{new}{MODE_ID_INFIX}{k}");
+                }
+                return;
+            }
+            at = theirs.collection_by_id(c).and_then(|c| c.extends.as_deref());
+        }
+    }
+
+    /// Rename what one prototype action names (not the actions nested in it).
+    fn action(&self, theirs: &Variables, a: &mut Action) {
+        match a {
+            Action::SetVariable { variable, expr, .. } => {
+                rename(&self.variables, variable);
+                if let Some(e) = expr {
+                    self.expr(theirs, e, 0);
+                }
+            }
+            Action::SetVariableMode { collection, mode } => {
+                self.mode(theirs, collection, mode);
+                rename(&self.collections, collection);
+            }
+            Action::Conditional { blocks } => blocks.iter_mut().filter_map(|b| b.condition.as_mut()).for_each(|c| self.expr(theirs, c, 0)),
+            _ => {}
+        }
+    }
+
+    fn expr(&self, theirs: &Variables, e: &mut Expr, depth: usize) {
+        match e {
+            Expr::Value(v) => v.references_mut().into_iter().for_each(|r| rename(&self.variables, r)),
+            Expr::Op { args, .. } if depth < MAX_EXPR_DEPTH => args.iter_mut().for_each(|a| self.expr(theirs, a, depth + 1)),
+            Expr::Op { .. } => {}
+            Expr::Mode { collection } => rename(&self.collections, collection),
+            Expr::InMode { variable, mode } => {
+                if let Some(v) = theirs.get_by_id(variable) {
+                    self.mode(theirs, &v.collection, mode);
+                }
+                rename(&self.variables, variable);
+            }
+        }
+    }
+}
+
+/// New ids, from `new_id`, for those of `ids` that `pick` picks; each clear of all of `ids`.
+fn fresh<'a>(
+    ids: impl Iterator<Item = &'a String> + Clone,
+    pick: impl Fn(&str) -> bool,
+    mut new_id: impl FnMut(&BTreeSet<String>) -> String,
+) -> BTreeMap<String, String> {
+    let mut avoid: BTreeSet<String> = ids.clone().cloned().collect();
+    let mut out = BTreeMap::new();
+    for id in ids.filter(|id| pick(id)) {
+        let new = new_id(&avoid);
+        avoid.insert(new.clone());
+        out.insert(id.clone(), new);
+    }
+    out
+}
+
+/// `id` as `map` renames it.
+fn rename(map: &BTreeMap<String, String>, id: &mut String) {
+    if let Some(new) = map.get(id.as_str()) {
+        *id = new.clone();
+    }
+}

crates/engine/src/cmd/file/open.rs

@@ -1,3 +1,4 @@
+use super::import_ids::Renames;
 use super::load::{ext, file_stem, import_summary, load_path, load_with, read, rest_images, sibling_variables, variables_param};
 use super::*;
 
@@ -218,8 +219,18 @@ pub(super) fn import(s: &mut Session, p: &Value) -> Result<Value> {
 /// Append `other`'s pages (with their settings and flows, its images, styles and variables) to
 /// `doc`. Ids are remapped across all pages at once, so instances keep their main, instance layers
 /// their source and interactions and flows their targets wherever those are. Styles, variables and
-/// collections whose id the document already has are kept as they are.
+/// collections the document already has (the same under the same id, as when a file comes in
+/// again) and library copies are shared; others whose id the document uses for something else get
+/// new ids, and what comes in names them by those ([`Renames`]).
 pub(super) fn append_pages(doc: &mut Document, other: &Document) -> Result<Vec<NodeId>> {
+    let renames = Renames::new(doc, other);
+    let renamed;
+    let other = if renames.is_empty() {
+        other
+    } else {
+        renamed = renames.applied(other);
+        &renamed
+    };
     for (k, v) in &other.images {
         doc.images.entry(k.clone()).or_insert_with(|| v.clone());
     }

crates/engine/src/import_ids_tests.rs

@@ -0,0 +1,172 @@
+//! `file.import` of a file whose style, variable and collection ids the document already uses for
+//! other things: ids are numbered per file, so two files made apart share them. The imported
+//! layers keep their own variables, modes and styles, and a file imported again still shares what
+//! it brought the first time.
+
+use base64::Engine as _;
+use figcraft_doc::proto::Action;
+use figcraft_doc::{Document, Node, NodeId, VariableValue};
+use serde_json::{Value, json};
+
+use crate::Session;
+use crate::components::tests::ex;
+use crate::params::id_of;
+
+fn idv(v: &Value) -> NodeId {
+    id_of(&v["id"]).unwrap()
+}
+
+fn var_id(s: &Session, name: &str) -> String {
+    s.doc().unwrap().variables.get(name).unwrap().id.clone()
+}
+
+fn fill(s: &Session, id: NodeId) -> String {
+    s.doc().unwrap().node(id).unwrap().fills.first().and_then(|p| p.solid_color()).map(|c| c.to_hex()).unwrap_or_default()
+}
+
+/// The document's file: collection A (Light, Dark) with colours `a1`…`a4`, paint style `Host`
+/// and a frame filled with `a1`.
+fn host() -> (Session, NodeId) {
+    let mut s = Session::new();
+    ex(&mut s, "variable.createCollection", json!({"name": "A", "modes": ["Light", "Dark"]}));
+    for i in 1..=4 {
+        ex(&mut s, "variable.create", json!({"collection": "A", "name": format!("a{i}"), "type": "color", "value": format!("#A{i}0000")}));
+    }
+    ex(&mut s, "style.create", json!({"type": "paint", "name": "Host", "value": "#FF0000"}));
+    let f = idv(&ex(&mut s, "frame.create", json!({"x": 0, "y": 0, "width": 50, "height": 50, "name": "Host frame"})));
+    ex(&mut s, "variable.bind", json!({"ids": [f], "field": "fill", "variable": "a1"}));
+    (s, f)
+}
+
+/// Another file made apart: collection B (Light, Dark) with `b1` (green in each mode), `b2` (an
+/// alias of `b1`) and `size`; paint style `Brand` bound to `b1`. Frame `Card` shows B in Dark, its
+/// corners bound to `size`; in it `Swatch` is filled with `b2` and on click sets B to Dark and
+/// `size` to `size + 1`, and `Styled` uses `Brand`.
+fn other() -> Session {
+    let mut s = Session::new();
+    ex(&mut s, "variable.createCollection", json!({"name": "B", "modes": ["Light", "Dark"]}));
+    ex(&mut s, "variable.create", json!({"collection": "B", "name": "b1", "type": "color", "values": {"Light": "#00B000", "Dark": "#004000"}}));
+    ex(&mut s, "variable.create", json!({"collection": "B", "name": "b2", "type": "color", "value": {"alias": "b1"}}));
+    ex(&mut s, "variable.create", json!({"collection": "B", "name": "size", "type": "number", "value": 8}));
+    let brand = ex(&mut s, "style.create", json!({"type": "paint", "name": "Brand", "value": "#00B000"}))["id"].clone();
+    ex(&mut s, "style.bind", json!({"style": brand, "field": "fill", "variable": "b1"}));
+    let card = idv(&ex(&mut s, "frame.create", json!({"x": 0, "y": 0, "width": 200, "height": 100, "name": "Card"})));
+    ex(&mut s, "variable.setMode", json!({"ids": [card], "collection": "B", "mode": "Dark"}));
+    ex(&mut s, "variable.bind", json!({"ids": [card], "field": "cornerRadius", "variable": "size"}));
+    let swatch = idv(&ex(&mut s, "shape.rectangle", json!({"x": 10, "y": 10, "width": 40, "height": 40, "parent": card, "name": "Swatch"})));
+    ex(&mut s, "variable.bind", json!({"ids": [swatch], "field": "fill", "variable": "b2"}));
+    let actions = json!([
+        {"type": "setVariableMode", "collection": "B", "mode": "Dark"},
+        {"type": "setVariable", "variable": "size", "expression": "size + 1"}
+    ]);
+    ex(&mut s, "proto.interaction.add", json!({"id": swatch, "actions": actions}));
+    let styled = idv(&ex(&mut s, "shape.rectangle", json!({"x": 60, "y": 10, "width": 40, "height": 40, "parent": card, "name": "Styled"})));
+    ex(&mut s, "style.apply", json!({"ids": [styled], "style": "Brand"}));
+    s
+}
+
+fn import(s: &mut Session, file: &Session) -> Value {
+    let bytes = figcraft_format::save_bytes(file.doc().unwrap(), None).unwrap();
+    let data = base64::engine::general_purpose::STANDARD.encode(bytes);
+    ex(s, "file.import", json!({"data": data, "name": "other.figcraft"}))
+}
+
+/// The layers on the pages after the first `kept` pages.
+fn layers_after(doc: &Document, kept: usize) -> Vec<&Node> {
+    doc.pages[kept..].iter().flat_map(|p| doc.subtree(*p).into_iter().skip(1)).filter_map(|id| doc.node(id)).collect()
+}
+
+fn layer<'a>(doc: &'a Document, kept: usize, name: &str) -> &'a Node {
+    layers_after(doc, kept).into_iter().find(|n| n.name == name).unwrap()
+}
+
+#[test]
+fn imported_layers_keep_their_own_variables_and_styles_when_the_ids_are_taken() {
+    let (mut s, frame) = host();
+    let file = other();
+    assert_eq!(var_id(&file, "b1"), var_id(&s, "a1"), "the ids must collide for this test");
+    assert_eq!(file.doc().unwrap().styles[0].id, s.doc().unwrap().styles[0].id);
+    let before = s.doc().unwrap().clone();
+    import(&mut s, &file);
+    let doc = s.doc().unwrap();
+    assert_eq!(doc.variables.variables.len(), 4 + 3);
+    assert_eq!(doc.variables.collections.iter().map(|c| c.name.as_str()).collect::<Vec<_>>(), ["A", "B"]);
+    assert_eq!(doc.styles.iter().map(|st| st.name.as_str()).collect::<Vec<_>>(), ["Host", "Brand"]);
+    // The document's own are as they were.
+    assert_eq!(doc.variables.variables[..4], before.variables.variables[..]);
+    assert_eq!(doc.variables.collections[0], before.variables.collections[0]);
+    assert_eq!(doc.styles[0], before.styles[0]);
+    // Everything imported names B's variables, B and Brand.
+    let b = doc.variables.collection("B").unwrap().id.clone();
+    let imported = layers_after(doc, before.pages.len());
+    assert!(imported.iter().any(|n| !n.bound.is_empty()));
+    for n in imported {
+        for v in n.bound.values() {
+            assert_eq!(doc.variables.get_by_id(v).unwrap().collection, b, "{}: {v}", n.name);
+        }
+        assert!(n.var_modes.keys().all(|c| *c == b), "{}: {:?}", n.name, n.var_modes);
+        if let Some(st) = &n.styles.fill {
+            assert_eq!(doc.styles.iter().find(|x| x.id == *st).unwrap().name, "Brand");
+        }
+    }
+    let brand = doc.styles.iter().find(|st| st.name == "Brand").unwrap();
+    assert_eq!(brand.bound.values().map(|v| doc.variables.get_by_id(v).unwrap().name.as_str()).collect::<Vec<_>>(), ["b1"]);
+    // Bindings applied again leave the imported layers alone.
+    let swatch = layer(doc, before.pages.len(), "Swatch").id;
+    let a2 = var_id(&s, "a2");
+    ex(&mut s, "variable.set", json!({"variable": a2, "value": "#123456"}));
+    assert_eq!(fill(&s, swatch), "004000");
+    assert_eq!(fill(&s, frame), "A10000");
+    assert!(s.doc().unwrap().validate().is_empty());
+}
+
+#[test]
+fn an_alias_a_frame_mode_and_prototype_actions_follow_the_renamed_collection() {
+    let (mut s, _) = host();
+    let file = other();
+    let kept = s.doc().unwrap().pages.len();
+    import(&mut s, &file);
+    let doc = s.doc().unwrap();
+    let vars = &doc.variables;
+    let b = vars.collection("B").unwrap();
+    let in_b = |name: &str| vars.variables.iter().find(|v| v.name == name && v.collection == b.id).unwrap();
+    let (b1, b2, size) = (in_b("b1"), in_b("b2"), in_b("size"));
+    assert!(b2.values.values().all(|v| *v == VariableValue::Alias(b1.id.clone())), "{:?}", b2.values);
+    // B's mode ids name B's new id, and its variables' values are by those.
+    let modes: Vec<&str> = b.modes.iter().map(|m| m.id.as_str()).collect();
+    assert_eq!(modes, [format!("{}/mode1", b.id), format!("{}/mode2", b.id)]);
+    assert!([b1, b2, size].iter().all(|v| v.values.keys().all(|m| modes.contains(&m.as_str()))));
+    // The frame shows B in Dark: its mode is one of B's.
+    let card = layer(doc, kept, "Card");
+    let dark = b.find_mode("Dark").unwrap().id.clone();
+    assert_eq!(card.var_modes.iter().collect::<Vec<_>>(), [(&b.id, &dark)]);
+    assert_eq!(card.bound.get("cornerRadius"), Some(&size.id));
+    let swatch = layer(doc, kept, "Swatch");
+    assert_eq!(swatch.bound.get("fill"), Some(&b2.id));
+    let shown = vars.resolve(&b2.id, &doc.modes_for(swatch.id));
+    assert!(matches!(shown, Some(VariableValue::Color(c)) if c.to_hex() == "004000"), "{shown:?}");
+    let actions = &swatch.interactions[0].actions;
+    assert_eq!(actions[0], Action::SetVariableMode { collection: b.id.clone(), mode: dark });
+    let Action::SetVariable { variable, expr: Some(e), .. } = &actions[1] else { panic!("{actions:?}") };
+    assert_eq!((variable, e.variables()), (&size.id, vec![size.id.as_str()]));
+}
+
+#[test]
+fn importing_the_same_file_again_shares_what_it_brought() {
+    let file = other();
+    let mut s = other();
+    let before = s.doc().unwrap().clone();
+    import(&mut s, &file);
+    let doc = s.doc().unwrap();
+    assert_eq!(doc.variables, before.variables);
+    assert_eq!(doc.styles, before.styles);
+    let swatch = layer(doc, before.pages.len(), "Swatch");
+    assert_eq!(swatch.bound.get("fill"), Some(&var_id(&s, "b2")));
+    // A file imported into an empty one, then again: one copy of its variables.
+    let mut t = Session::new();
+    import(&mut t, &file);
+    import(&mut t, &file);
+    let doc = t.doc().unwrap();
+    assert_eq!((doc.variables.variables.len(), doc.variables.collections.len(), doc.styles.len()), (3, 1, 1));
+    assert!(layers_after(doc, 1).iter().flat_map(|n| n.bound.values()).all(|v| doc.variables.get_by_id(v).is_some()));
+}

crates/engine/src/lib.rs

@@ -367,6 +367,8 @@ mod images_left_in_file_tests;
 #[cfg(test)]
 mod import_export_tests;
 #[cfg(test)]
+mod import_ids_tests;
+#[cfg(test)]
 mod import_journal_tests;
 #[cfg(test)]
 mod inspect_behaviour_tests;

crates/engine/src/library_variables_tests.rs

@@ -385,6 +385,34 @@ fn library_components_keep_their_variable_bindings() {
     let _ = std::fs::remove_dir_all(&d);
 }
 
+#[test]
+fn a_file_bringing_older_copies_of_the_same_library_variables_shares_them() {
+    let d = dir("older-copies");
+    let lib_path = path_str(&d.join("kit.figcraft"));
+    let mut lib = Session::new();
+    theme(&mut lib);
+    let lid = publish_to(&mut lib, &lib_path);
+    let mut s = Session::new();
+    ex(&mut s, "library.add", json!({"path": lib_path}));
+    let r = idv(&ex(&mut s, "shape.rectangle", json!({"width": 10, "height": 10, "name": "Swatch"})));
+    ex(&mut s, "library.bindVariable", json!({"library": lid, "variable": "primary", "field": "fill", "ids": [r]}));
+    let old_path = path_str(&d.join("old.figcraft"));
+    ex(&mut s, "file.saveAs", json!({"path": old_path}));
+    // The copies take an update; the saved file keeps the older ones under the same ids.
+    ex(&mut lib, "variable.set", json!({"variable": "primary", "mode": "Light", "value": "#444444"}));
+    republish(&mut lib);
+    ex(&mut s, "library.acceptUpdates", json!({}));
+    let before = s.doc().unwrap().clone();
+    ex(&mut s, "file.import", json!({"path": old_path}));
+    let doc = s.doc().unwrap();
+    assert_eq!((&doc.variables, &doc.libraries.variables), (&before.variables, &before.libraries.variables), "shared by id, as before");
+    let swatches: Vec<&Node> = doc.nodes().filter(|n| n.name == "Swatch").collect();
+    assert_eq!(swatches.len(), 2);
+    assert!(swatches.iter().all(|n| n.bound.get("fill") == Some(&copy_of(&s, "primary").id)));
+    healthy(&s);
+    let _ = std::fs::remove_dir_all(&d);
+}
+
 #[test]
 fn library_variable_commands_fail_cleanly() {
     let mut s = Session::new();