address review: keep modern-comments precedence for already-owned parts

crates/pptx/src/read.rs

@@ -698,21 +698,20 @@ fn slide_notes(imp: &mut Imp, part: &Part, owned: &mut HashSet<String>) -> 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<String>) {
-    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));

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]