Merge pull request #1935 from Silvertree2010/fix/1883-bug-report-designcraft-error-mes

Open documents whose items share an id

crates/doc/src/lib.rs

@@ -955,6 +955,91 @@ impl Document {
         designcraft_color::swatch::resolve(&self.swatches, swatch, tint)
     }
 
+    /// Give items that reuse an earlier item's id (spreads, then parents, then items anchored in
+    /// stories) fresh ids, so a file holding such duplicates opens. The first item keeps the id and
+    /// what refers to it by id; a renumbered text frame takes its place in its story's thread.
+    pub fn renumber_duplicate_items(&mut self) {
+        fn walk_mut(it: &mut Item, f: &mut impl FnMut(&mut Item)) {
+            f(it);
+            for c in it.children_mut().into_iter().flatten() {
+                walk_mut(Arc::make_mut(c), f);
+            }
+        }
+        fn story_items(st: &mut Story, f: &mut impl FnMut(&mut Item)) {
+            for o in &mut st.objects {
+                walk_mut(&mut Arc::make_mut(o).item, f);
+            }
+            for t in st.tables.values_mut() {
+                for c in &mut Arc::make_mut(t).cells {
+                    story_items(&mut c.text, f);
+                }
+            }
+            for n in st.notes.iter_mut().chain(st.endnotes.iter_mut()) {
+                story_items(&mut Arc::make_mut(n).text, f);
+            }
+        }
+        let mut seen = std::collections::HashSet::new();
+        let mut dups = std::collections::HashSet::new();
+        let mut next = self.next_id;
+        let mut note = |id: ItemId| {
+            next = next.max(id.0);
+            if !seen.insert(id) {
+                dups.insert(id);
+            }
+        };
+        for sp in self.spreads.iter().chain(self.parents.iter()) {
+            for it in &sp.items {
+                it.walk(&mut |i| note(i.id));
+            }
+        }
+        for (_, i) in self.anchored_items() {
+            note(i.id);
+        }
+        if dups.is_empty() {
+            return;
+        }
+        let mut seen = std::collections::HashSet::new();
+        // (story, id in the file) → the ids its frames carry now, in walk order.
+        let mut threads: std::collections::HashMap<(StoryId, ItemId), Vec<ItemId>> = std::collections::HashMap::new();
+        let mut fix = |it: &mut Item| {
+            let old = it.id;
+            if !dups.contains(&old) {
+                return;
+            }
+            if !seen.insert(old) {
+                // An id at the top of the range has no successor: leave the duplicate for
+                // `check()` to refuse rather than overflow.
+                let Some(n) = next.checked_add(1) else { return };
+                next = n;
+                it.id = ItemId(next);
+            }
+            if let Some(t) = it.text_frame() {
+                threads.entry((t.story, old)).or_default().push(it.id);
+            }
+        };
+        for sp in self.spreads.iter_mut().chain(self.parents.iter_mut()) {
+            for it in &mut Arc::make_mut(sp).items {
+                walk_mut(Arc::make_mut(it), &mut fix);
+            }
+        }
+        for st in self.stories.values_mut() {
+            story_items(Arc::make_mut(st), &mut fix);
+        }
+        self.next_id = next;
+        for ((sid, old), ids) in threads {
+            let Some(st) = self.stories.get_mut(&sid) else { continue };
+            let st = Arc::make_mut(st);
+            let mut ids = ids.into_iter();
+            for f in st.frames.iter_mut().filter(|f| **f == old) {
+                match ids.next() {
+                    Some(id) => *f = id,
+                    None => break,
+                }
+            }
+            st.frames.extend(ids);
+        }
+    }
+
     /// Validate structural invariants (ids unique, threads consistent, stories well-formed).
     pub fn check(&self) -> Result<()> {
         for f in &self.styles.composite_fonts {

crates/format/src/lib.rs

@@ -251,6 +251,7 @@ fn load_zip(bytes: &[u8]) -> Result<Document, FormatError> {
     upgrade(&mut doc, u32::try_from(version).unwrap_or(u32::MAX));
     doc.merge_duplicate_assets();
     doc.mark_differing_margins_local();
+    doc.renumber_duplicate_items();
     doc.check().map_err(|e| FormatError::NotOurs(e.to_string()))?;
     Ok(doc)
 }
@@ -318,6 +319,7 @@ fn load_legacy_json(bytes: &[u8]) -> Result<Document, FormatError> {
     upgrade(&mut doc, 1);
     doc.merge_duplicate_assets();
     doc.mark_differing_margins_local();
+    doc.renumber_duplicate_items();
     doc.check().map_err(|e| FormatError::NotOurs(e.to_string()))?;
     Ok(doc)
 }

crates/format/src/tests.rs

@@ -734,3 +734,105 @@ fn too_many_entries_are_refused() {
     let Err(err) = load(&z.finish().unwrap().into_inner()) else { panic!("an archive with 100 001 entries loaded") };
     assert!(err.to_string().contains("entries"), "{err}");
 }
+
+/// Every item id in `d` (spreads, parents, anchored), in walk order.
+fn all_item_ids(d: &Document) -> Vec<u64> {
+    let mut ids = Vec::new();
+    for sp in d.spreads.iter().chain(d.parents.iter()) {
+        for it in &sp.items {
+            it.walk(&mut |i| ids.push(i.id.0));
+        }
+    }
+    ids.extend(d.anchored_items().iter().map(|(_, i)| i.id.0));
+    ids
+}
+
+fn set_item_id(d: &mut Document, index: usize, id: designcraft_doc::ItemId) {
+    let sp = Arc::make_mut(&mut d.spreads[0]);
+    Arc::make_mut(&mut sp.items[index]).id = id;
+}
+
+#[test]
+fn files_with_duplicate_item_ids_open_with_unique_ids() {
+    let mut d = Document::new(&NewDocument::default());
+    let one = png_asset(&mut d, &[1]);
+    let two = png_asset(&mut d, &[2]);
+    let a = place(&mut d, one, 0.0);
+    place(&mut d, two, 0.0);
+    set_item_id(&mut d, 1, a);
+    let back = load(&save(&d).unwrap()).unwrap();
+    back.check().unwrap();
+    let ids = all_item_ids(&back);
+    assert_eq!(ids.len(), 2);
+    assert_ne!(ids[0], ids[1]);
+    assert!(back.next_id >= ids[0].max(ids[1]));
+    let assets: Vec<_> = back.spreads[0].items.iter().map(|i| i.graphic().unwrap().asset).collect();
+    assert_eq!(assets, vec![one, two]);
+    assert_eq!(back.item(a).and_then(|i| i.graphic()).map(|g| g.asset), Some(one));
+}
+
+#[test]
+fn duplicate_text_frame_ids_keep_each_story_in_its_own_frame() {
+    let mut d = Document::new(&NewDocument::default());
+    let lid = d.default_layer();
+    let (a, sa) = d.add_text_frame(SpreadRef::Doc(0), Rect::new(10.0, 10.0, 200.0, 200.0), lid, "first", ParaFormat::default()).unwrap();
+    let (b, sb) = d.add_text_frame(SpreadRef::Doc(0), Rect::new(10.0, 210.0, 200.0, 400.0), lid, "second", ParaFormat::default()).unwrap();
+    let (c, _) = d.add_text_frame(SpreadRef::Doc(0), Rect::new(210.0, 210.0, 400.0, 400.0), lid, "", ParaFormat::default()).unwrap();
+    d.thread(b, c).unwrap();
+    // The second story's first frame carries the first frame's id.
+    set_item_id(&mut d, 1, a);
+    let st = d.story_mut(sb).unwrap();
+    assert_eq!(st.frames, vec![b, c]);
+    st.frames[0] = a;
+    let back = load(&save(&d).unwrap()).unwrap();
+    back.check().unwrap();
+    let ids = all_item_ids(&back);
+    let unique: HashSet<_> = ids.iter().collect();
+    assert_eq!(unique.len(), 3);
+    assert_eq!(back.stories[&sa].frames, vec![a]);
+    assert_eq!(back.stories[&sa].text, "first");
+    let frames = &back.stories[&sb].frames;
+    assert_eq!(frames.len(), 2);
+    assert_ne!(frames[0], a);
+    assert_eq!(frames[1], c);
+    assert_eq!(back.item(frames[0]).and_then(|i| i.text_frame()).map(|t| t.story), Some(sb));
+    assert_eq!(back.stories[&sb].text, "second");
+}
+
+#[test]
+fn duplicate_anchored_item_ids_are_renumbered() {
+    use designcraft_doc::anchored::{AnchorPosition, AnchoredObject};
+    use designcraft_doc::geom::shapes;
+    use designcraft_doc::{Item, Shape};
+    let mut d = Document::new(&NewDocument::default());
+    let lid = d.default_layer();
+    let (_, sid) = d.add_text_frame(SpreadRef::Doc(0), Rect::new(10.0, 10.0, 200.0, 200.0), lid, "ab", ParaFormat::default()).unwrap();
+    let asset = png_asset(&mut d, &[1]);
+    let g = place(&mut d, asset, 0.0);
+    let item = Item::new(g, lid, Shape::Oval, shapes::rectangle(Rect::new(0.0, 0.0, 10.0, 10.0)));
+    d.story_mut(sid).unwrap().insert_object(1, AnchoredObject::new(item, AnchorPosition::default()));
+    let back = load(&save(&d).unwrap()).unwrap();
+    back.check().unwrap();
+    let ids = all_item_ids(&back);
+    assert_eq!(ids.len(), 3);
+    assert_eq!(ids.iter().collect::<HashSet<_>>().len(), 3);
+    assert_eq!(back.item(g).and_then(|i| i.graphic()).map(|g| g.asset), Some(asset));
+    let anchored = back.anchored_items();
+    assert_eq!(anchored.len(), 1);
+    assert_eq!(anchored[0].1.shape, Shape::Oval);
+    assert_ne!(anchored[0].1.id, g);
+}
+
+#[test]
+fn duplicate_ids_at_the_top_of_the_range_do_not_overflow() {
+    let mut d = Document::new(&NewDocument::default());
+    let one = png_asset(&mut d, &[1]);
+    let two = png_asset(&mut d, &[2]);
+    place(&mut d, one, 0.0);
+    place(&mut d, two, 0.0);
+    set_item_id(&mut d, 0, designcraft_doc::ItemId(u64::MAX));
+    set_item_id(&mut d, 1, designcraft_doc::ItemId(u64::MAX));
+    d.renumber_duplicate_items();
+    assert_eq!(all_item_ids(&d), vec![u64::MAX, u64::MAX]);
+    assert!(d.check().is_err());
+}