From ba1ee2329b971bb2dc7dde669846f44618290be4 Mon Sep 17 00:00:00 2001 From: VoiceLessQ Date: Wed, 28 Oct 2026 21:10:50 -0200 Subject: [PATCH] Give anchored objects fresh ids when copying text frames (fixes #1522) Copying a text frame cloned its story but left the objects anchored in it with the original's item ids. Save does not validate, load runs Document::check and rejects duplicate ids, so a duplicated or pasted frame with an inline graphic produced a file that failed to open with "not a DesignCraft document: duplicate item id". renumber (Duplicate, Alt-drag copy, Paste, Paste into text, parent item overrides) and the data merge copy now give every anchored item in the copied story, its table cells, footnotes and endnotes a fresh id. Anchored text frames from IDML get their own story copy. Assets are not adopted again: the story copy already remapped them. The IDML export tests that relied on duplication to create shared ids now set the shared id explicitly, so the export guard stays covered. Files already saved with duplicate ids still do not open. --- ROADMAP.md | 4 + crates/engine/src/cmd/datamerge/fill.rs | 29 ++++++- crates/engine/src/cmd/datamerge/tests.rs | 16 ++++ crates/engine/src/cmd/interchange.rs | 35 +++++--- crates/engine/src/cmd/object.rs | 103 ++++++++++++++++++++++- 5 files changed, 166 insertions(+), 21 deletions(-) diff --git a/ROADMAP.md b/ROADMAP.md index c02176bab..bdd8ec314 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -12,6 +12,10 @@ DesignCraft aims at full Adobe InDesign parity — and to be better: faster, open (documented JSON format + IDML), scriptable by agents (MCP), and available on the web. +- **2026-10-28** · Duplicate, paste, parent overrides and data merge give anchored objects in the + copied story fresh ids (#1522). Copies used to keep the original's ids, and the saved file then + failed to open with "duplicate item id". Files already saved that way still don't open; no + readiness percentage change. - **2026-10-25** · The application bar uses craft-window's shared title-bar code. A drag that starts on a floating panel over the bar moves the panel, not the window, and in full screen the bar no longer zooms or moves the window. No readiness percentage change. diff --git a/crates/engine/src/cmd/datamerge/fill.rs b/crates/engine/src/cmd/datamerge/fill.rs index 629258d6d..f2e60baae 100644 --- a/crates/engine/src/cmd/datamerge/fill.rs +++ b/crates/engine/src/cmd/datamerge/fill.rs @@ -71,11 +71,17 @@ pub fn copy_item( } fn renumber(dst: &mut designcraft_doc::Document, src: &designcraft_doc::Document, it: &mut Item, maps: &mut Maps) { + renumber_in(dst, src, it, maps, true); +} + +/// `adopt` is false inside a story copy, whose asset references the copy has already adopted. +fn renumber_in(dst: &mut designcraft_doc::Document, src: &designcraft_doc::Document, it: &mut Item, maps: &mut Maps, adopt: bool) { let old = it.id; let new_id = ItemId(dst.alloc()); maps.items.insert(old, new_id); it.id = new_id; - if let Some(m) = it.media.as_mut() + if adopt + && let Some(m) = it.media.as_mut() && let Some(p) = m.poster { m.poster = dst.adopt_asset(src, p).or(Some(p)); @@ -98,8 +104,10 @@ fn renumber(dst: &mut designcraft_doc::Document, src: &designcraft_doc::Document } designcraft_doc::remap_story_assets(&mut st, &assets); st.frames.clear(); - dst.stories.insert(n, Arc::new(st)); + // Mapped first: the map is the seen-set that stops story recursion. maps.stories.insert(tf.story, n); + renumber_anchored(dst, src, &mut st, maps); + dst.stories.insert(n, Arc::new(st)); n }; tf.story = new_sid; @@ -108,19 +116,32 @@ fn renumber(dst: &mut designcraft_doc::Document, src: &designcraft_doc::Document } } Content::Graphic(g) => { - if let Some(a) = dst.adopt_asset(src, g.asset) { + if adopt && let Some(a) = dst.adopt_asset(src, g.asset) { g.asset = a; } } Content::Group { items } => { for child in items.iter_mut() { - renumber(dst, src, Arc::make_mut(child), maps); + renumber_in(dst, src, Arc::make_mut(child), maps, adopt); } } Content::Unassigned => {} } } +/// Fresh ids for the objects anchored in story copy `st`, its table cells, footnotes and endnotes (#1522). +fn renumber_anchored(dst: &mut designcraft_doc::Document, src: &designcraft_doc::Document, st: &mut Story, maps: &mut Maps) { + let mut fresh = |t: &mut Story| { + for o in &mut t.objects { + renumber_in(dst, src, &mut Arc::make_mut(o).item, maps, false); + } + }; + st.for_each_text_mut(&mut fresh); + for n in &mut st.endnotes { + Arc::make_mut(n).text.for_each_text_mut(&mut fresh); + } +} + struct Rep { story: StoryId, start: usize, diff --git a/crates/engine/src/cmd/datamerge/tests.rs b/crates/engine/src/cmd/datamerge/tests.rs index 79e16027e..2195bdf62 100644 --- a/crates/engine/src/cmd/datamerge/tests.rs +++ b/crates/engine/src/cmd/datamerge/tests.rs @@ -1198,3 +1198,19 @@ fn facing_pages_merge_tiles_short_blocks_without_stacking() { let unique: std::collections::BTreeSet<_> = origins.iter().collect(); assert_eq!(unique.len(), 3, "no two records may share a page and coordinates: {origins:?}"); } + +// #1522: each record's copy of an anchored object needs its own id, or the merged file won't reopen. +#[test] +fn merged_records_give_anchored_objects_their_own_ids() { + let mut s = Session::new(); + new_doc(&mut s, 1); + select_csv(&mut s, "Name\nAda\nGrace\n"); + let made = text_frame(&mut s, [36.0, 36.0, 400.0, 80.0], "<> here", None); + let b = exec(&mut s, "frame.create", json!({"rect": [450, 450, 474, 490]})); + exec(&mut s, "anchored.insert", json!({"ids": [b["id"]], "story": made["story"], "pos": 2})); + assert_eq!(exec(&mut s, "data.merge", json!({}))["records"], 2); + let doc = &s.documents()[s.active_index().unwrap()].doc; + assert_eq!(doc.anchored_items().len(), 2); + doc.check().unwrap(); + designcraft_format::load(&designcraft_format::save(doc).unwrap()).unwrap(); +} diff --git a/crates/engine/src/cmd/interchange.rs b/crates/engine/src/cmd/interchange.rs index 0ddb6a1aa..152761fb1 100644 --- a/crates/engine/src/cmd/interchange.rs +++ b/crates/engine/src/cmd/interchange.rs @@ -74,7 +74,7 @@ fn with_pdf_layer_variants(doc: &Document, embed: bool) -> Result = doc.placed_items().into_iter().filter(|i| i.has_pdf_layer_override()).map(|i| i.id).collect(); - assert_eq!(with_choice, [chosen, chosen], "both placements share one id"); - assert!(doc.check().unwrap_err().to_string().contains(&format!("duplicate item id {chosen}"))); + assert_eq!(with_choice.len(), 2); + assert!(with_choice.contains(&chosen) && with_choice[0] != with_choice[1], "{with_choice:?}"); + doc.check().unwrap(); let result = s.execute("file.exportIdml", &json!({})).unwrap(); let imported = designcraft_idml::import_idml(&base64_decode(result["base64"].as_str().unwrap())).unwrap(); let mut layers: Vec<_> = @@ -831,8 +832,8 @@ mod nested_layer_tests { assert_eq!(layers, vec![states(false, true), states(true, false)]); } - // Shared anchored ids are a pre-existing duplication gap. Conflicting choices must not - // silently select one variant; this fixture changes only the copied anchor's choice. + // Duplication no longer shares anchored ids (#1522), so this fixture forces the shared id. + // Conflicting choices must not silently select one variant; only the copy's choice changes. #[test] fn shared_id_with_divergent_pdf_choices_is_rejected() { let mut s = Session::new(); @@ -840,23 +841,25 @@ mod nested_layer_tests { let pdf = designcraft_render::pdf_hide_layers(&super::super::pdflayers::tests::two_layer_pdf(), &["Red".into()]).unwrap(); let placed = s.execute("file.place", &json!({"base64": base64_encode(&pdf), "name": "layers.pdf"})).unwrap(); let control = ItemId(placed["id"].as_u64().unwrap()); - let frame = { + let (frame, anchored) = { let doc = Arc::make_mut(&mut s.doc_mut().unwrap().doc); let mut item = doc.item(control).unwrap().clone(); item.id = ItemId(doc.alloc()); item.pdf_hidden_layers = vec!["Blue".into()]; + let anchored = item.id; let layer = doc.default_layer(); let (frame, sid) = doc.add_text_frame(SpreadRef::Doc(0), Rect::new(0.0, 0.0, 200.0, 200.0), layer, "Body", Default::default()).unwrap(); doc.story_mut(sid).unwrap().insert_object(0, AnchoredObject { item, position: Default::default() }); - frame + (frame, anchored) }; s.execute("transform.move", &json!({"ids": [frame.0], "dx": 10, "dy": 10, "copy": true})).unwrap(); - // Change ONLY the copy in the newest story to explicit all-on. + // Give ONLY the copy in the newest story the original's id and explicit all-on. { let doc = Arc::make_mut(&mut s.doc_mut().unwrap().doc); let (_, st) = doc.stories.iter_mut().filter(|(_, st)| !st.objects.is_empty()).last().unwrap(); let o = &mut Arc::make_mut(st).objects[0]; let it = &mut Arc::make_mut(o).item; + it.id = anchored; it.pdf_hidden_layers.clear(); it.pdf_layer_override = true; } @@ -871,8 +874,8 @@ mod nested_layer_tests { assert!(Arc::ptr_eq(&before, &s.doc().unwrap().doc)); } - // Cross-document paste: the pasted frame's anchored placement keeps its source id, - // which can collide with an unrelated placement in the destination. + // Cross-document paste used to keep the anchored placement's source id (#1522), colliding + // with an unrelated placement in the destination. The fixture forces that collision. #[test] fn shared_id_with_different_pdf_assets_is_rejected() { let mut s = Session::new(); @@ -893,7 +896,13 @@ mod nested_layer_tests { .as_u64() .unwrap(); s.execute("edit.paste", &json!({})).unwrap(); - assert_eq!(a, b, "fresh documents allocate the same first placement id"); + { + let doc = Arc::make_mut(&mut s.doc_mut().unwrap().doc); + let (_, st) = doc.stories.iter_mut().find(|(_, st)| !st.objects.is_empty()).unwrap(); + let pasted = &mut Arc::make_mut(&mut Arc::make_mut(st).objects[0]).item; + assert_ne!(pasted.id.0, b, "paste gives the anchored placement a fresh id"); + pasted.id = ItemId(b); + } let before = s.doc().unwrap().doc.clone(); let graphics: Vec<_> = before.placed_items().into_iter().filter(|i| i.graphic().is_some()).collect(); assert_eq!(graphics.len(), 2); diff --git a/crates/engine/src/cmd/object.rs b/crates/engine/src/cmd/object.rs index ad8b25213..9ba95cf7c 100644 --- a/crates/engine/src/cmd/object.rs +++ b/crates/engine/src/cmd/object.rs @@ -1076,9 +1076,15 @@ fn adopt_or_dangle(dst: &mut Document, src: &Document, id: AssetId) -> AssetId { /// Give `it` (an item of `src`) fresh ids from `dst`, with copies of its stories and the assets it uses. pub(crate) fn renumber(dst: &mut Document, src: &Document, it: &mut Item, stories: &mut HashMap) { + renumber_in(dst, src, it, stories, true); +} + +/// `adopt` is false inside a story copy, whose asset references the copy has already adopted. +fn renumber_in(dst: &mut Document, src: &Document, it: &mut Item, stories: &mut HashMap, adopt: bool) { it.id = ItemId(dst.alloc()); let my_id = it.id; - if let Some(m) = it.media.as_mut() + if adopt + && let Some(m) = it.media.as_mut() && let Some(p) = m.poster { m.poster = Some(adopt_or_dangle(dst, src, p)); @@ -1100,8 +1106,10 @@ pub(crate) fn renumber(dst: &mut Document, src: &Document, it: &mut Item, storie } designcraft_doc::remap_story_assets(&mut st, &assets); st.frames.clear(); - dst.stories.insert(n, Arc::new(st)); + // Mapped first: the map is the seen-set that stops story recursion. stories.insert(tf.story, n); + renumber_anchored(dst, src, &mut st, stories); + dst.stories.insert(n, Arc::new(st)); n } }; @@ -1111,17 +1119,32 @@ pub(crate) fn renumber(dst: &mut Document, src: &Document, it: &mut Item, storie } } Content::Graphic(g) => { - g.asset = adopt_or_dangle(dst, src, g.asset); + if adopt { + g.asset = adopt_or_dangle(dst, src, g.asset); + } } Content::Group { items } => { for c in items.iter_mut() { - renumber(dst, src, Arc::make_mut(c), stories); + renumber_in(dst, src, Arc::make_mut(c), stories, adopt); } } Content::Unassigned => {} } } +/// Fresh ids for the objects anchored in story copy `st`, its table cells, footnotes and endnotes (#1522). +fn renumber_anchored(dst: &mut Document, src: &Document, st: &mut Story, stories: &mut HashMap) { + let mut fresh = |t: &mut Story| { + for o in &mut t.objects { + renumber_in(dst, src, &mut Arc::make_mut(o).item, stories, false); + } + }; + st.for_each_text_mut(&mut fresh); + for n in &mut st.endnotes { + Arc::make_mut(n).text.for_each_text_mut(&mut fresh); + } +} + fn transform_move(s: &mut Session, p: &Value) -> Result { let ids = targets(s, p)?; let (dx, dy) = (f64_or(p, "dx", 0.0), f64_or(p, "dy", 0.0)); @@ -5701,3 +5724,75 @@ mod frame_baseline_grid_tests { assert_eq!(grid(&s), None); } } + +#[cfg(test)] +mod anchored_copy_tests { + use super::*; + use designcraft_doc::AnchoredObject; + + /// A text frame with an inline rectangle; returns the frame and the anchored item's id. + fn frame_with_inline_box(s: &mut Session) -> (ItemId, ItemId) { + s.execute("file.new", &json!({})).unwrap(); + let r = s.execute("frame.create", &json!({"rect": [72, 72, 400, 400], "content": "text", "text": "Before after."})).unwrap(); + let frame = ItemId(r["id"].as_u64().unwrap()); + let sid = s.doc().unwrap().doc.item(frame).unwrap().text_frame().unwrap().story; + let b = s.execute("frame.create", &json!({"rect": [450, 450, 474, 490]})).unwrap(); + s.execute("anchored.insert", &json!({"ids": [b["id"]], "story": sid.0, "pos": 7})).unwrap(); + (frame, s.doc().unwrap().doc.story(sid).unwrap().objects[0].item.id) + } + + fn assert_reopens(doc: &Document) { + doc.check().unwrap(); + designcraft_format::load(&designcraft_format::save(doc).unwrap()).unwrap(); + } + + // #1522: the copy kept the anchored object's id, and the saved file no longer opened. + #[test] + fn duplicating_a_frame_with_an_anchored_object_saves_a_file_that_reopens() { + let mut s = Session::new(); + let (frame, anchored) = frame_with_inline_box(&mut s); + s.execute("transform.move", &json!({"ids": [frame.0], "dx": 10, "dy": 10, "copy": true})).unwrap(); + let doc = &s.doc().unwrap().doc; + assert_eq!(doc.anchored_items().iter().filter(|(_, i)| i.id == anchored).count(), 1); + assert_eq!(doc.anchored_items().len(), 2); + assert_reopens(doc); + } + + #[test] + fn pasting_a_frame_with_an_anchored_object_twice_keeps_ids_unique() { + let mut s = Session::new(); + let (frame, _) = frame_with_inline_box(&mut s); + s.execute("selection.set", &json!({"ids": [frame.0]})).unwrap(); + s.execute("edit.copy", &json!({})).unwrap(); + s.execute("edit.paste", &json!({})).unwrap(); + s.execute("edit.paste", &json!({})).unwrap(); + let doc = &s.doc().unwrap().doc; + assert_eq!(doc.anchored_items().len(), 3); + assert_reopens(doc); + } + + // IDML import can anchor a text frame; its story is copied too, with the copy in its thread. + #[test] + fn an_anchored_text_frame_gets_its_own_story_copy() { + let mut d = Document::new(&designcraft_doc::build::NewDocument::default()); + let layer = d.default_layer(); + let (host, host_sid) = d.add_text_frame(SpreadRef::Doc(0), Rect::new(0.0, 0.0, 300.0, 300.0), layer, "Host", ParaFormat::default()).unwrap(); + let (inner, inner_sid) = d.add_text_frame(SpreadRef::Doc(0), Rect::new(0.0, 0.0, 50.0, 50.0), layer, "Inner", ParaFormat::default()).unwrap(); + let item = d.item(inner).unwrap().clone(); + let inner_story = d.stories.get(&inner_sid).unwrap().clone(); + d.remove_item(inner).unwrap(); + d.stories.insert(inner_sid, inner_story); + d.story_mut(host_sid).unwrap().insert_object(0, AnchoredObject { item, position: Default::default() }); + d.check().unwrap(); + let src = d.clone(); + let copy = duplicate_from(&mut d, &src, &[host], SpreadRef::Doc(0), Vec2::new(10.0, 10.0)).unwrap(); + let copy_sid = d.item(copy[0]).unwrap().text_frame().unwrap().story; + let anchored = &d.story(copy_sid).unwrap().objects[0].item; + assert_ne!(anchored.id, inner); + let anchored_sid = anchored.text_frame().unwrap().story; + assert_ne!(anchored_sid, inner_sid); + assert_eq!(d.story(anchored_sid).unwrap().frames, [anchored.id]); + assert_eq!(d.story(inner_sid).unwrap().frames, [inner]); + assert_reopens(&d); + } +}