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.
This commit is contained in:
skylar taylor-barrick
2026-10-24 17:44:02 -06:00
parent ba8936e061
commit dfba0a3f9d
5 changed files with 25 additions and 281 deletions
+8 -139
View File
@@ -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<ItemId> {
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(&note.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");
}
}
+4 -9
View File
@@ -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<designcraft_doc::Document>, usize)> {
let choices: Vec<_> = d
.placed_items()
@@ -686,12 +687,6 @@ fn rasterize_layered(d: &designcraft_doc::Document) -> Result<(Option<designcraf
})
.collect();
if choices.is_empty() {
// Fail closed, with the same predicate as the walk. As in the IDML pass this is a tripwire
// for a future construct the walk misses, not live behaviour; a stale choice on an item that
// is no longer a placed PDF must not block the export.
if super::interchange::has_applicable_pdf_layer_overrides(d) {
return Err(EngineError::Other("a placed PDF's layer choice sits where this raster export cannot apply it".into()));
}
return Ok((None, 0));
}
let mut out = d.clone();
@@ -721,7 +716,7 @@ fn rasterize_layered(d: &designcraft_doc::Document) -> Result<(Option<designcraf
images.insert(id, aid);
n += 1;
}
out.update_placed_items(
out.update_items(
|it| images.contains_key(&it.id),
|it| {
if let designcraft_doc::Content::Graphic(gg) = &mut it.content
+8 -14
View File
@@ -40,9 +40,9 @@ fn export_idml(s: &mut Session, p: &Value) -> Result<Value> {
}
}
/// 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<Option<Document>> {
let choices: Vec<_> = doc
.placed_items()
@@ -65,14 +67,6 @@ fn with_pdf_layer_variants(doc: &Document, embed: bool) -> Result<Option<Documen
})
.collect();
if choices.is_empty() {
// A tripwire, not live behaviour: the walk and this predicate cover the same placements, so
// they can only disagree if a future nesting construct slips past the walk, and that would
// silently become the file's defaults. Do not narrow the walk to match this check. The
// predicate asks for a graphic whose asset is a PDF, like the walk, so stale fields left on a
// relinked image never block an export.
if has_applicable_pdf_layer_overrides(doc) {
return Err(EngineError::Other("a placed PDF's layer choice sits where this IDML export cannot apply it".into()));
}
return Ok(None);
}
if !embed {
@@ -93,7 +87,7 @@ fn with_pdf_layer_variants(doc: &Document, embed: bool) -> Result<Option<Documen
}
// Anchored objects live in stories, and parent pages outside the document-spread walk, so
// the choice is cleared where the placement actually is.
out.update_placed_items(
out.update_items(
|it| variants.contains_key(&it.id),
|it| {
if let designcraft_doc::Content::Graphic(g) = &mut it.content
+5 -82
View File
@@ -507,93 +507,16 @@ pub(crate) mod tests {
// An external-only copy would keep the original file's defaults: the choice is rejected.
let err = s.execute("file.exportIdml", &json!({"embedImages": false})).unwrap_err().to_string();
assert!(err.contains("embedded images"), "{err}");
// PDF/A-2b rasterises a placement with a choice wherever it sits, warnings included.
// PDF/A-2b rasterises a parent-page placement with a choice, warnings included.
let a2b = s.execute("file.exportPdf", &json!({"standard": "a2b"})).unwrap();
assert!(a2b["warnings"].as_array().unwrap().iter().any(|w| w.as_str().is_some_and(|w| w.contains("exported as images"))), "{a2b}");
}
/// A choice on a frame anchored inside a table cell or a footnote is a placement too: the
/// save asks for the newer reader, the IDML copy gets a variant, an external-only copy is
/// refused and PDF/A-2b rasterises it, rather than each quietly falling back to the file.
#[test]
fn nested_placements_keep_their_layer_choices_in_saves_and_exports() {
use designcraft_doc::{AnchoredObject, Content, Graphic, Item, Shape, SpreadRef, Table};
use designcraft_geom::{Affine, Rect, shapes};
let pdf = designcraft_render::pdf_hide_layers(&two_layer_pdf(), &["Red".into()]).unwrap();
for in_note in [false, true] {
let mut d = designcraft_doc::Document::new(&Default::default());
let asset = designcraft_doc::AssetId(d.alloc());
d.add_asset(designcraft_doc::Asset {
id: asset,
name: "nested.pdf".into(),
mime: "application/pdf".into(),
data: std::sync::Arc::new(pdf.clone()),
..Default::default()
});
let mut item = Item::new(
designcraft_doc::ItemId(d.alloc()),
d.default_layer(),
Shape::Rectangle,
shapes::rectangle(Rect::new(0.0, 0.0, 100.0, 100.0)),
);
item.pdf_layer_override = true;
item.content = Content::Graphic(Graphic {
asset,
size: (100.0, 100.0),
xf: Affine::IDENTITY,
clip: None,
auto_fit: Default::default(),
fit_align: 4,
crop: [0.0; 4],
wrap: Default::default(),
fill: Default::default(),
layer_options: None,
});
let layer = d.default_layer();
let (_, story) = d.add_text_frame(SpreadRef::Doc(0), Rect::new(0.0, 0.0, 80.0, 80.0), layer, "Body", Default::default()).unwrap();
let anchored = AnchoredObject { item, position: Default::default() };
if in_note {
let st = d.story_mut(story).unwrap();
st.insert_note(0, "Note", Default::default()).unwrap();
assert_eq!(st.notes.len(), 1);
std::sync::Arc::make_mut(&mut st.notes[0]).text.insert_object(0, anchored);
} else {
let mut table = Table::new(1, 1, 1, 0, 0, 20.0);
table.cells[0].text.insert_object(0, anchored);
d.story_mut(story).unwrap().insert_table(0, table);
}
assert!(d.has_pdf_layer_overrides(), "the nested placement carries the choice");
let mut s = Session::new();
s.add_document(crate::DocState::new(d, None));
// The save keeps it under the newer reader, as the detection walk says.
let saved = designcraft_format::save(&s.doc().unwrap().doc).unwrap();
let reopened = designcraft_format::load(&saved).unwrap();
assert!(reopened.has_pdf_layer_overrides(), "in_note={in_note}");
s.add_document(crate::DocState::new(reopened, None));
// Embedded IDML carries a variant that shows every layer.
let idml = s.execute("file.exportIdml", &json!({})).unwrap();
let imported = designcraft_idml::import_idml(&super::super::file::base64_decode(idml["base64"].as_str().unwrap())).unwrap();
let copies: Vec<_> = 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();
@@ -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[<</Event/View/Category[/View]/OCGs[5 0 R 6 3 R]>><</Event/Print/Category[/Print]/OCGs[5 0 R 6 3 R]>><</Event/Export/Category[/Export]/OCGs[5 0 R 6 3 R]>>]";
/// 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();