From 87859340324746cedae57da9c1692b7cb564781e Mon Sep 17 00:00:00 2001 From: NetterSilver <133535268+Silvertree2010@users.noreply.github.com> Date: Wed, 28 Oct 2026 01:50:17 +0000 Subject: [PATCH 1/2] Open documents whose items share an id Fixes #1883 Co-Authored-By: Claude Opus 5.5 --- crates/doc/src/lib.rs | 82 +++++++++++++++++++++++++++++++++++ crates/format/src/lib.rs | 2 + crates/format/src/tests.rs | 88 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 172 insertions(+) diff --git a/crates/doc/src/lib.rs b/crates/doc/src/lib.rs index bf9998209..08ba34ca0 100644 --- a/crates/doc/src/lib.rs +++ b/crates/doc/src/lib.rs @@ -955,6 +955,88 @@ 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> = std::collections::HashMap::new(); + let mut fix = |it: &mut Item| { + let old = it.id; + if !dups.contains(&old) { + return; + } + if !seen.insert(old) { + next += 1; + 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 { diff --git a/crates/format/src/lib.rs b/crates/format/src/lib.rs index 1adc43449..15156f8ad 100644 --- a/crates/format/src/lib.rs +++ b/crates/format/src/lib.rs @@ -251,6 +251,7 @@ fn load_zip(bytes: &[u8]) -> Result { 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 { 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) } diff --git a/crates/format/src/tests.rs b/crates/format/src/tests.rs index 981898a45..702428d14 100644 --- a/crates/format/src/tests.rs +++ b/crates/format/src/tests.rs @@ -734,3 +734,91 @@ 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 { + 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::>().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); +} From 84801932f2897a4b355e634132f58c7d2ae01996 Mon Sep 17 00:00:00 2001 From: Brandon Thomas Date: Thu, 29 Oct 2026 21:39:23 -0400 Subject: [PATCH 2/2] Renumbering duplicate ids stops at the top of the id range instead of overflowing Co-Authored-By: Claude Opus 5.5 --- crates/doc/src/lib.rs | 5 ++++- crates/format/src/tests.rs | 14 ++++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/crates/doc/src/lib.rs b/crates/doc/src/lib.rs index 08ba34ca0..fecb2c3b4 100644 --- a/crates/doc/src/lib.rs +++ b/crates/doc/src/lib.rs @@ -1007,7 +1007,10 @@ impl Document { return; } if !seen.insert(old) { - next += 1; + // 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() { diff --git a/crates/format/src/tests.rs b/crates/format/src/tests.rs index 702428d14..882f5e6b8 100644 --- a/crates/format/src/tests.rs +++ b/crates/format/src/tests.rs @@ -822,3 +822,17 @@ fn duplicate_anchored_item_ids_are_renumbered() { 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()); +}