From 3c3d504eb15243a6517d1a1d8b841082a2edef9b Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Tue, 3 Nov 2026 20:12:24 -0500 Subject: [PATCH] PPTX: read a notes or comments part for the first slide that names it (#1741) * pptx: read a notes or comments part for the first slide that names it * address review: keep modern-comments precedence for already-owned parts --- crates/pptx/src/read.rs | 37 +++++++++++++++++-------------- crates/pptx/tests/it/comments.rs | 27 ++++++++++++++++++++++ crates/pptx/tests/it/malformed.rs | 31 ++++++++++++++++++++++++++ 3 files changed, 78 insertions(+), 17 deletions(-) diff --git a/crates/pptx/src/read.rs b/crates/pptx/src/read.rs index 2056d53d7..4ee3c4d32 100644 --- a/crates/pptx/src/read.rs +++ b/crates/pptx/src/read.rs @@ -668,52 +668,55 @@ fn read_slides( let pkg = imp.pkg; let fallback_layout = p.masters.first().and_then(|m| m.layouts.first()).map(|l| l.id).unwrap_or_default(); let mut file_slide_ids: HashMap = HashMap::new(); - let mut notes_by_target: HashMap = HashMap::new(); + // A notes or comments part belongs to one slide; one shared by many must not be read (and + // copied) once per slide that names it. + let mut owned_parts: HashSet = HashSet::new(); for (file_id, path, id) in slide_ids { let Some(doc) = docs.get(&opc::norm(path)) else { continue }; let part = Part::new(pkg, path, doc); let layout = part.rels.first_of("slideLayout").and_then(|r| layout_by_part.get(&opc::norm(&r.target)).copied()).unwrap_or(fallback_layout); let mut slide = read_slide(imp, &part, doc, *id, layout); - if let Some(notes) = slide_notes(imp, &part, &mut notes_by_target) { + if let Some(notes) = slide_notes(imp, &part, &mut owned_parts) { slide.notes = notes; } - read_slide_comments(pkg, pres_part, &part, &mut slide); + read_slide_comments(pkg, pres_part, &part, &mut slide, &mut owned_parts); file_slide_ids.insert(*file_id, *id); p.slides.push(Arc::new(slide)); } file_slide_ids } -/// The slide's notes text, read once per notes part. -fn slide_notes(imp: &mut Imp, part: &Part, cache: &mut HashMap) -> Option { +/// The slide's notes text. A notes part is the notes of the first slide that names it. +fn slide_notes(imp: &mut Imp, part: &Part, owned: &mut HashSet) -> Option { let n = part.rels.first_of("notesSlide")?; - let key = opc::norm(&n.target); - if let Some(cached) = cache.get(&key) { - return Some(cached.clone()); + if !owned.insert(opc::norm(&n.target)) { + log::warn!("pptx: notes part {} is named by more than one slide; kept on the first", n.target); + return None; } let pkg = imp.pkg; let nd = load(pkg, &n.target)?; let np = Part::new(pkg, &n.target, &nd); - let notes = read_notes(imp, &np, &nd.root); - cache.insert(key, notes.clone()); - Some(notes) + Some(read_notes(imp, &np, &nd.root)) } /// Comments. A modern part and the legacy part can describe the same threads (PowerPoint /// writes both): the modern one wins, its flattened legacy copy is skipped. -fn read_slide_comments(pkg: &Package, pres_part: &Part, part: &Part, slide: &mut Slide) { - let mut parts: Vec<(Doc, bool)> = vec![]; +/// 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, bool)> = vec![]; for c in part.rels.all_of("comments") { + // 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] diff --git a/crates/pptx/tests/it/malformed.rs b/crates/pptx/tests/it/malformed.rs index 774a5ee92..a07493486 100644 --- a/crates/pptx/tests/it/malformed.rs +++ b/crates/pptx/tests/it/malformed.rs @@ -287,3 +287,34 @@ fn unreadable_slide_is_reported() { assert_eq!(lost, vec!["ppt/slides/slide2.xml".to_string()]); assert_eq!(deckcraft_pptx::import(&broken).expect("import").slides.len(), 1); } + +/// Many slides naming one notes part: it is read for the first slide only, not copied onto every +/// slide that names it. +#[test] +fn a_shared_notes_part_is_read_once() { + use std::sync::Arc; + let mut p = defaults::new_presentation(None); + let layout = p.slides[0].layout; + for _ in 0..3 { + let s = defaults::new_slide(&mut p, layout).expect("slide"); + p.slides.push(Arc::new(s)); + } + for (i, s) in p.slides.iter_mut().enumerate() { + Arc::make_mut(s).notes = deckcraft_model::text::TextBody::from_text(&format!("notes {i}")); + } + let mut es = entries(&deckcraft_pptx::export(&p).expect("export")); + for (n, d) in es.iter_mut() { + if n.starts_with("ppt/slides/_rels/") { + let x = String::from_utf8(d.clone()).expect("utf8"); + let x = x + .replace("notesSlide2.xml", "notesSlide1.xml") + .replace("notesSlide3.xml", "notesSlide1.xml") + .replace("notesSlide4.xml", "notesSlide1.xml"); + *d = x.into_bytes(); + } + } + let q = deckcraft_pptx::import(&rezip(&es)).expect("import"); + assert_eq!(q.slides.len(), 4); + assert_eq!(q.slides[0].notes.text(), "notes 0"); + assert!(q.slides[1..].iter().all(|s| s.notes.text().is_empty()), "a shared notes part is not copied"); +}