diff --git a/crates/pptx/src/read.rs b/crates/pptx/src/read.rs index ce05577e4..3a45b3576 100644 --- a/crates/pptx/src/read.rs +++ b/crates/pptx/src/read.rs @@ -698,21 +698,20 @@ fn slide_notes(imp: &mut Imp, part: &Part, owned: &mut HashSet) -> Optio /// writes both): the modern one wins, its flattened legacy copy is skipped. /// Each comments part is read once, for the first slide that names it. fn read_slide_comments(pkg: &Package, pres_part: &Part, part: &Part, slide: &mut Slide, owned: &mut HashSet) { - let mut parts: Vec<(Doc, bool)> = vec![]; + let mut parts: Vec<(Doc, bool, bool)> = vec![]; for c in part.rels.all_of("comments") { - if !owned.insert(opc::norm(&c.target)) { - continue; - } + // An already-owned part is not re-read, but its kind still decides modern precedence. + let fresh = owned.insert(opc::norm(&c.target)); if let Some(cd) = pkg.xml(&c.target) { // The content type names the modern part; a package that types it only through a // Default entry instead of an Override is still modern when its root is p188. let modern = pkg.content_type(&c.target).as_deref() == Some(crate::write::CT_COMMENTS_MODERN) || cd.root.prefix() == "p188"; - parts.push((cd, modern)); + parts.push((cd, modern, fresh)); } } - let has_modern = parts.iter().any(|(_, m)| *m); - for (cd, modern) in parts { - if has_modern && !modern { + let has_modern = parts.iter().any(|(_, m, _)| *m); + for (cd, modern, fresh) in parts { + if !fresh || (has_modern && !modern) { continue; } slide.comments.extend(read_comments(pkg, pres_part, &cd.root, modern)); diff --git a/crates/pptx/tests/it/comments.rs b/crates/pptx/tests/it/comments.rs index 9713b0f42..9b0e85bde 100644 --- a/crates/pptx/tests/it/comments.rs +++ b/crates/pptx/tests/it/comments.rs @@ -146,6 +146,33 @@ fn comments_on_several_slides_round_trip() { assert_eq!(q.slides[2].comments[0].text, "Slide three"); } +/// A slide that names a modern part another slide already owns, plus its own legacy part, must +/// not import the legacy copy: modern precedence holds even when the modern part is not re-read. +#[test] +fn owned_modern_part_still_suppresses_later_legacy() { + let mut p = deck(); + let layout = p.slides[0].layout; + let s2 = defaults::new_slide(&mut p, layout).expect("slide"); + p.slides.push(Arc::new(s2)); + Arc::make_mut(&mut p.slides[1]).comments = vec![Comment { author: "Cara".into(), text: "Slide two".into(), ..Default::default() }]; + let bytes = deckcraft_pptx::export(&p).expect("export"); + let rels2 = part(&bytes, "ppt/slides/_rels/slide2.xml.rels").replace("modernComment2.xml", "modernComment1.xml"); + let mut z = zip::ZipArchive::new(std::io::Cursor::new(&bytes)).expect("zip"); + let mut out = zip::ZipWriter::new(std::io::Cursor::new(Vec::new())); + for i in 0..z.len() { + let mut f = z.by_index(i).expect("entry"); + out.start_file(f.name(), zip::write::SimpleFileOptions::default()).expect("start"); + if f.name() == "ppt/slides/_rels/slide2.xml.rels" { + std::io::Write::write_all(&mut out, rels2.as_bytes()).expect("write"); + } else { + std::io::copy(&mut f, &mut out).expect("copy"); + } + } + let q = deckcraft_pptx::import(&out.finish().expect("finish").into_inner()).expect("import"); + assert_eq!(q.slides[0].comments.len(), 2); + assert!(q.slides[1].comments.is_empty(), "{:?}", q.slides[1].comments); +} + /// A legacy-only package (what older tools read and write) still imports: 1/8 pt positions, /// flattened replies. #[test]