From dfba0a3f9dae196d2e5ce05fbb026b11016de496 Mon Sep 17 00:00:00 2001 From: skylar taylor-barrick Date: Sat, 24 Oct 2026 17:44:02 -0600 Subject: [PATCH] M6: leave placements nested in cells and notes out of the PDF layer pass Both exports walk Document::placed_items: document and parent spreads, groups, and the objects anchored in a story. That is the walk the parent-page and anchored fix introduced. The nested-story traversal (table cells, notes, endnotes), Document::update_placed_items and the fail-closed refusals that went with it are not part of it: that is the traversal change on its own, not the placements this pass carries. update_items keeps its reach, so the two exports clear the choice where the placement actually is again. A choice anchored inside a table cell's or a note's text therefore stays on the document and is not carried into the embedded IDML copy, as on main; the refusal that turned that silent difference into an export error goes with the traversal it guarded, since asking the walk's own predicate it could never fire. The stale-field half keeps: has_applicable_pdf_layer_overrides still asks for a graphic whose asset is a PDF, so relinking a placed PDF to an image no longer makes the package report name a choice that does not exist. Tests: the nested-placement regression and the relink integration test of the removed refusal go; the parent-page regression, the stale-field predicate test and the format 4 cell-only reader test stay. The comments no longer claim every placement counts. --- crates/doc/src/edit.rs | 147 ++------------------ crates/engine/src/cmd/export.rs | 13 +- crates/engine/src/cmd/interchange.rs | 22 ++- crates/engine/src/cmd/pdflayers.rs | 87 +----------- crates/engine/tests/pdf_optional_content.rs | 37 ----- 5 files changed, 25 insertions(+), 281 deletions(-) diff --git a/crates/doc/src/edit.rs b/crates/doc/src/edit.rs index c64911cc6..4235346c7 100644 --- a/crates/doc/src/edit.rs +++ b/crates/doc/src/edit.rs @@ -642,9 +642,11 @@ impl Document { found } - /// Every item on document and parent spreads, in groups, and among the objects anchored in - /// stories, including the stories in table cells and in notes: walks that must cover every - /// placement use this, not [`Document::all_items`]. + /// Every item on document and parent spreads, in groups, and among the objects anchored in a + /// story: a placement a page inherits from its parent, and one anchored in text, both count. + /// Objects anchored inside a table cell's text or a note's text are not reached, as this walk + /// does not follow the stories nested there. Walks that must cover every placement a page can + /// show use this, not [`Document::all_items`]. pub fn placed_items(&self) -> Vec<&Item> { let mut v = Vec::new(); for sp in self.spreads.iter().chain(&self.parents) { @@ -653,35 +655,13 @@ impl Document { } } for st in self.stories.values() { - placed_story_items(st, 0, &mut v); + for o in &st.objects { + o.item.walk(&mut |i| v.push(i)); + } } v } - /// Applies `f` to every placed item `hit` selects, wherever it sits: on document or parent - /// spreads, in groups, and anchored in a story (also inside table cells and notes). - /// Returns how many items `f` saw. - pub fn update_placed_items(&mut self, hit: impl Fn(&Item) -> bool, mut f: impl FnMut(&mut Item)) -> usize { - let mut n = 0; - for sp in self.spreads.iter_mut().chain(self.parents.iter_mut()) { - if !sp.items.iter().any(|i| has_hit(i, &hit)) { - continue; - } - for it in Arc::make_mut(sp).items.iter_mut() { - if has_hit(it, &hit) { - update_placed_item(Arc::make_mut(it), 0, &hit, &mut f, &mut n); - } - } - } - for st in self.stories.values_mut() { - if !story_has_hit(st, 0, &hit) { - continue; - } - update_story_items(Arc::make_mut(st), 0, &hit, &mut f, &mut n); - } - n - } - /// Every item id (pre-order) on document spreads. pub fn all_items(&self) -> Vec { let mut v = Vec::new(); @@ -776,87 +756,6 @@ impl Document { } } -/// How deep nested stories (a cell's text, a footnote's text, an endnote's text) are followed -/// when every placement is visited. A document nested deeper is left alone. -const MAX_PLACEMENT_DEPTH: usize = 64; - -/// The items anchored in `st`, and in the stories nested in its cells and notes, pre-order. -fn placed_story_items<'a>(st: &'a Story, depth: usize, v: &mut Vec<&'a Item>) { - if depth > MAX_PLACEMENT_DEPTH { - return; - } - for o in &st.objects { - o.item.walk(&mut |i| v.push(i)); - } - for c in st.tables.values().flat_map(|t| &t.cells) { - placed_story_items(&c.text, depth + 1, v); - } - for n in st.notes.iter().chain(&st.endnotes) { - placed_story_items(&n.text, depth + 1, v); - } -} - -/// Whether `it` or anything in it is selected by `hit`. -fn has_hit(it: &Item, hit: &impl Fn(&Item) -> bool) -> bool { - let mut found = false; - it.walk(&mut |i| found |= hit(i)); - found -} - -/// Whether anything anchored in `st`, or in the stories nested in its cells and notes, is selected -/// by `hit`. Stops at the first match, so an untouched story is never copied. -fn story_has_hit(st: &Story, depth: usize, hit: &impl Fn(&Item) -> bool) -> bool { - if depth > MAX_PLACEMENT_DEPTH { - return false; - } - st.objects.iter().any(|o| has_hit(&o.item, hit)) - || st.tables.values().flat_map(|t| &t.cells).any(|c| story_has_hit(&c.text, depth + 1, hit)) - || st.notes.iter().chain(&st.endnotes).any(|n| story_has_hit(&n.text, depth + 1, hit)) -} - -/// [`Document::update_placed_items`] for one item: only the branches holding a hit are copied. -fn update_placed_item(it: &mut Item, depth: usize, hit: &impl Fn(&Item) -> bool, f: &mut impl FnMut(&mut Item), n: &mut usize) { - if hit(it) { - f(it); - *n += 1; - } - if depth >= MAX_PLACEMENT_DEPTH { - return; - } - if let Some(children) = it.children_mut() { - for c in children.iter_mut().filter(|c| has_hit(c, hit)) { - update_placed_item(Arc::make_mut(c), depth + 1, hit, f, n); - } - } -} - -/// [`Document::update_placed_items`] for the placements of one story, and of the stories nested -/// in its cells and notes. -fn update_story_items(st: &mut Story, depth: usize, hit: &impl Fn(&Item) -> bool, f: &mut impl FnMut(&mut Item), n: &mut usize) { - if depth > MAX_PLACEMENT_DEPTH { - return; - } - for o in st.objects.iter_mut() { - if has_hit(&o.item, hit) { - update_placed_item(&mut Arc::make_mut(o).item, 0, hit, f, n); - } - } - for t in st.tables.values_mut() { - if !t.cells.iter().any(|c| story_has_hit(&c.text, depth + 1, hit)) { - continue; - } - for c in &mut Arc::make_mut(t).cells { - update_story_items(&mut c.text, depth + 1, hit, f, n); - } - } - for note in st.notes.iter_mut().chain(st.endnotes.iter_mut()) { - if !story_has_hit(¬e.text, depth + 1, hit) { - continue; - } - update_story_items(&mut Arc::make_mut(note).text, depth + 1, hit, f, n); - } -} - /// Does `p` (in the item's parent space) hit `it`? pub fn item_hit(it: &Item, p: Point, tol: f64) -> bool { if let Content::Group { items } = &it.content { @@ -901,33 +800,3 @@ pub fn hit_descendant(it: &Item, p: Point, tol: f64, want: &dyn Fn(&Item) -> boo /// How far from its path type on a path is hit (points): about a cap height of 12–18 pt type, /// either side (the type may sit on or under the path). const PATH_TYPE_REACH: f64 = 12.0; - -#[cfg(test)] -mod placed_item_tests { - use super::*; - - /// A pass that changes nothing must not copy a spread or a story: only branches holding a - /// match are touched (see `update_placed_items`). - #[test] - fn update_placed_items_leaves_untouched_spreads_and_stories_shared() { - let mut d = Document::new(&crate::build::NewDocument { pages: 2, ..Default::default() }); - let layer = d.default_layer(); - let (_, story) = - d.add_text_frame(crate::SpreadRef::Doc(0), crate::geom::Rect::new(0.0, 0.0, 80.0, 80.0), layer, "Body", Default::default()).unwrap(); - let mut table = crate::Table::new(1, 1, 1, 0, 0, 20.0); - table.cells[0].text.insert_object(0, crate::anchored::AnchoredObject::default()); - d.story_mut(story).unwrap().insert_table(0, table); - let spreads = d.spreads.to_vec(); - let parents = d.parents.to_vec(); - let stories: Vec<_> = d.stories.values().cloned().collect(); - let seen = d.update_placed_items(|_| false, |_| panic!("nothing matches")); - assert_eq!(seen, 0); - assert!(d.spreads.iter().zip(&spreads).all(|(now, before)| std::sync::Arc::ptr_eq(now, before)), "spreads are shared"); - assert!(d.parents.iter().zip(&parents).all(|(now, before)| std::sync::Arc::ptr_eq(now, before)), "parents are shared"); - assert!(d.stories.values().zip(&stories).all(|(now, before)| std::sync::Arc::ptr_eq(now, before)), "stories are shared"); - // A matching pass still reaches the anchored placement inside the cell. - let mut hits = 0; - d.update_placed_items(|_| true, |_| hits += 1); - assert!(hits > 0, "the walk reaches the nested placement"); - } -} diff --git a/crates/engine/src/cmd/export.rs b/crates/engine/src/cmd/export.rs index 9271898a9..005e261e5 100644 --- a/crates/engine/src/cmd/export.rs +++ b/crates/engine/src/cmd/export.rs @@ -672,8 +672,9 @@ fn wrap_stand_in(it: &designcraft_doc::Item, depth: usize) -> designcraft_doc::I /// A copy of `d` where placed PDFs with Object Layer Options hidden layers are 300 ppi images. /// -/// Every placement counts, including parent-page and anchored ones, so a choice is never -/// silently replaced by the file's defaults. +/// Parent-page and anchored placements count too, so a page that inherits the placement is not +/// silently replaced by the file's defaults. A choice anchored inside a table cell's or a note's +/// text keeps the source file's defaults (see `docs/pdf-layer-visibility.md`). fn rasterize_layered(d: &designcraft_doc::Document) -> Result<(Option, usize)> { let choices: Vec<_> = d .placed_items() @@ -686,12 +687,6 @@ fn rasterize_layered(d: &designcraft_doc::Document) -> Result<(Option Result<(Option Result { } } -/// Whether a placement still holds an Object Layer Options choice the exports can apply: a -/// graphic whose asset is a PDF. A choice left on an item that is no longer a placed PDF (its -/// link was replaced with another format) is not one, and must not block an export. +/// Whether a placement holds an Object Layer Options choice an export can still apply: a graphic +/// whose asset is a PDF. A choice left on an item that is no longer a placed PDF (its link was +/// replaced with another format) is not one, so the package report does not name it. pub(crate) fn has_applicable_pdf_layer_overrides(doc: &Document) -> bool { doc.placed_items().into_iter().any(|it| { it.has_pdf_layer_override() && it.graphic().and_then(|g| doc.assets.get(&g.asset)).is_some_and(|a| designcraft_render::is_pdf(&a.data)) @@ -52,8 +52,10 @@ pub(crate) fn has_applicable_pdf_layer_overrides(doc: &Document) -> bool { /// Each placement gets its own embedded variant so two frames sharing a PDF can /// choose different layers. The live document and original linked bytes stay untouched. /// -/// Every placement counts, including parent-page and anchored ones: a choice that the copy -/// cannot carry is rejected rather than exported as the file's defaults. +/// Parent-page and anchored placements count too, so a page that inherits the placement does not +/// quietly fall back to the file's defaults. A choice anchored inside a table cell's or a note's +/// text is not carried yet: this walk does not follow the stories nested there (see +/// `docs/pdf-layer-visibility.md`). fn with_pdf_layer_variants(doc: &Document, embed: bool) -> Result> { let choices: Vec<_> = doc .placed_items() @@ -65,14 +67,6 @@ fn with_pdf_layer_variants(doc: &Document, embed: bool) -> Result Result = imported.assets.values().filter(|a| designcraft_render::is_pdf(&a.data)).collect(); - assert!(!copies.is_empty(), "in_note={in_note}: the anchored placement is exported"); - assert!( - copies.iter().all(|a| designcraft_render::pdf_layers(&a.data).iter().all(|(_, on)| *on)), - "in_note={in_note}: the embedded copy must not fall back to the file's defaults" - ); - // An external-only copy would keep the original file's defaults. - assert!(s.execute("file.exportIdml", &json!({"embedImages": false})).is_err(), "in_note={in_note}"); - // PDF/A-2b rasterises a placement with a choice, wherever it sits. - let a2b = s.execute("file.exportPdf", &json!({"standard": "a2b"})); - assert!(a2b.is_ok(), "in_note={in_note}: {a2b:?}"); - } - } - /// Relinking a placed PDF to an image leaves the item's layer fields behind. They are no longer a - /// placed-PDF choice, so they must not block an export: the refusal guard asks for a graphic whose - /// asset is a PDF, the same thing the export walks ask for. The user path (a real relink to a PNG) - /// is covered in `tests/pdf_optional_content.rs`, where the file guard allows a test fixture on disk. + /// placed-PDF choice: the package report asks for a graphic whose asset is a PDF, the same thing + /// the export walks ask for, so it does not name the stale field, and exports run unaffected. #[test] - fn a_stale_choice_on_a_non_pdf_placement_does_not_block_exports() { + fn a_stale_choice_on_a_non_pdf_placement_is_not_a_placed_pdf_choice() { let png = designcraft_render::Rendered { width: 4, height: 3, pixels: [30, 90, 180, 255].repeat(12) }.to_png(); let mut s = Session::new(); s.execute("file.new", &json!({"width": 200, "height": 200})).unwrap(); @@ -608,7 +531,7 @@ pub(crate) mod tests { assert!(d.item(item).unwrap().has_pdf_layer_override(), "the stale field is on the item"); assert!(d.has_pdf_layer_overrides(), "the field is there"); assert!(!super::super::interchange::has_applicable_pdf_layer_overrides(d), "but it is not a placed-PDF choice"); - // Both exports still run: nothing is dropped, and there is no choice to refuse. + // Nothing is dropped: the field is not a choice any export applies. s.execute("file.exportIdml", &json!({})).unwrap(); s.execute("file.exportPdf", &json!({"standard": "a2b"})).unwrap(); s.execute("file.exportPdf", &json!({})).unwrap(); diff --git a/crates/engine/tests/pdf_optional_content.rs b/crates/engine/tests/pdf_optional_content.rs index eac1a0cab..e96e34915 100644 --- a/crates/engine/tests/pdf_optional_content.rs +++ b/crates/engine/tests/pdf_optional_content.rs @@ -14,43 +14,6 @@ use support::{catalog, document, fixture, painted}; const DEFAULT: &str = "/BaseState/ON/OFF[6 3 R]"; const USAGE_APPLICATIONS: &str = "/BaseState/ON/ON[5 0 R]/OFF[6 3 R]/AS[<><><>]"; -/// The user path behind the stale-choice guard: place a PDF, hide a layer, relink that asset to a -/// PNG (links.relink replaces the asset in place, so the item keeps its layer fields), then export. -/// The fields are no longer a placed-PDF choice, so nothing is refused. -#[test] -fn relinking_a_layered_pdf_to_an_image_still_exports() { - let source = fixture(DEFAULT, "Hidden"); - let pdf_path = std::env::temp_dir().join(format!( - "dc-layered-{}-{}.pdf", - std::process::id(), - std::time::SystemTime::now().duration_since(std::time::UNIX_EPOCH).unwrap().as_nanos() - )); - std::fs::write(&pdf_path, &source).unwrap(); - let mut session = designcraft_engine::Session::new(); - session.execute("file.new", &serde_json::json!({"width": 200, "height": 200})).unwrap(); - let placed = session.execute("file.place", &serde_json::json!({"path": pdf_path.to_string_lossy(), "x": 0, "y": 0, "width": 100})).unwrap(); - let id = designcraft_doc::ItemId(placed["id"].as_u64().unwrap()); - session.execute("object.layerOptions", &serde_json::json!({"ids": [placed["id"]], "hidden": ["Hidden"]})).unwrap(); - let asset = session.doc().unwrap().doc.item(id).unwrap().graphic().unwrap().asset; - let png = designcraft_render::Rendered { width: 4, height: 3, pixels: [30, 90, 180, 255].repeat(12) }.to_png(); - let path = std::env::temp_dir().join(format!( - "dc-relinked-{}-{}.png", - std::process::id(), - std::time::SystemTime::now().duration_since(std::time::UNIX_EPOCH).unwrap().as_nanos() - )); - std::fs::write(&path, &png).unwrap(); - session.execute("links.relink", &serde_json::json!({"asset": asset.0, "path": path.to_string_lossy()})).unwrap(); - let doc = session.doc().unwrap().doc.clone(); - assert!(!designcraft_render::is_pdf(&doc.assets[&asset].data), "the placement is an image now"); - assert!(doc.item(id).unwrap().has_pdf_layer_override(), "the stale fields are still on the item"); - // All three exports still work: the field is not a placed-PDF choice any more. - session.execute("file.exportIdml", &serde_json::json!({})).unwrap(); - session.execute("file.exportPdf", &serde_json::json!({"standard": "a2b"})).unwrap(); - session.execute("file.exportPdf", &serde_json::json!({})).unwrap(); - std::fs::remove_file(&path).ok(); - std::fs::remove_file(&pdf_path).ok(); -} - #[test] fn source_layer_dictionaries_are_readable() { let pdf = hayro_syntax::Pdf::new(fixture(DEFAULT, "Hidden")).unwrap();