M4: a pop-up counts as a comment's only when it is a pop-up of that comment (#2137)

Review on #2137. set_popup accepted a pop-up whose /Parent was present but
not a reference, and the comment list (pdfcraft_annot::summaries and the
inspector) reported whatever /Popup named: another annotation's rectangle
and open state. One check, own_popup, now decides for both: /Subtype
/Popup, and /Parent absent or a reference to the comment (ISO 32000-2:2020
12.5.6.14, Table 186: "Optional; shall be an indirect reference").

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Gv2WwdgjmBz33GWkwgNn3Q

crates/annot/src/lib.rs

@@ -1872,10 +1872,7 @@ pub fn set_popup(doc: &mut Document, page: usize, index: usize, rect: Option<[f6
     let _ = meta;
     // Only a pop-up that is this comment's: `/Popup` naming another annotation (or a pop-up
     // whose `/Parent` is another comment) would move or open that one, so it gets its own.
-    let own_popup = |pr: &ObjRef| {
-        doc.get(*pr).as_dict().is_some_and(|pd| pd.name(b"Subtype") == Some(b"Popup") && pd.reference(b"Parent").is_none_or(|parent| parent == r))
-    };
-    match d.reference(b"Popup").filter(own_popup) {
+    match own_popup(doc, Some(r), &d) {
         Some(pr) => doc.update_dict(pr, |p| {
             if let Some(rect) = rect {
                 p.set(b"Rect".to_vec(), num_array(&rect));
@@ -2396,9 +2393,24 @@ pub struct PopupInfo {
     pub open: bool,
 }
 
-/// The pop-up `annot` names, as [`PopupInfo`]: `None` when it has none or it is unreadable.
-fn popup_info(doc: &Document, annot: &Dict) -> Option<PopupInfo> {
-    let r = annot.reference(b"Popup")?;
+/// The pop-up the comment `annot` (the object `own`, when it is one of its own) names in
+/// `/Popup`, when it is the comment's: a pop-up annotation whose `/Parent`, if it has one, is a
+/// reference to the comment. A `/Popup` naming another annotation, or a pop-up of another
+/// comment, isn't. ISO 32000-2:2020 §12.5.6.14, Table 186: `/Parent` is "(Optional; shall be
+/// an indirect reference)".
+fn own_popup(doc: &Document, own: Option<ObjRef>, annot: &Dict) -> Option<ObjRef> {
+    let pr = annot.reference(b"Popup")?;
+    let is_own = doc.get(pr).as_dict().is_some_and(|pd| {
+        pd.name(b"Subtype") == Some(b"Popup") && pd.get(b"Parent").is_none_or(|parent| parent.as_ref().is_some_and(|parent| Some(parent) == own))
+    });
+    is_own.then_some(pr)
+}
+
+/// The comment's own pop-up (see [`own_popup`]), as [`PopupInfo`]: `None` when it has none or it
+/// is unreadable.
+fn popup_info(doc: &Document, own: Option<ObjRef>, annot: &Dict) -> Option<PopupInfo> {
+    // Another annotation's rectangle and open state aren't this comment's (review on #2137).
+    let r = own_popup(doc, own, annot)?;
     let p = doc.get(r);
     let p = p.as_dict()?;
     let rect: Vec<f32> = doc.resolve(p.get(b"Rect")?).as_array()?.iter().filter_map(|v| doc.resolve(v).as_f64().map(|x| x as f32)).collect();
@@ -2512,7 +2524,7 @@ pub fn summaries(doc: &Document) -> Vec<Summary> {
                 fill_sign: is_fill_sign(doc, d),
                 no_rotate: drawn_upright(doc, d),
                 points,
-                popup: popup_info(doc, d),
+                popup: popup_info(doc, entry.as_ref(), d),
             });
         }
     }

crates/annot/src/tests.rs

@@ -2181,6 +2181,40 @@ fn pop_ups_open_close_and_move_and_are_reported() {
     let own = annot_dict(&doc, mr).reference(b"Popup").expect("a pop-up of its own");
     assert_ne!(own, r);
     assert_eq!(doc.get(own).as_dict().and_then(|p| p.reference(b"Parent")), Some(mr));
+    // Its own pop-up, with /Parent naming it, is the one moved next time.
+    set_popup(&mut doc, 0, mark, Some([12.0, 10.0, 112.0, 60.0]), None, &meta("h1")).unwrap();
+    assert_eq!(annot_dict(&doc, mr).reference(b"Popup"), Some(own), "the same pop-up");
+
+    // #2137 review: nor is it reported as the comment's. A /Popup naming the note, or the note's
+    // own pop-up (its /Parent is the note), gives the mark no pop-up in the summaries.
+    let popup_of_mark = |doc: &Document| summaries(doc).into_iter().find(|s| s.index == mark).unwrap().popup;
+    let notes_popup = annot_dict(&doc, r).reference(b"Popup").expect("the note's pop-up");
+    for foreign in [r, notes_popup] {
+        doc.update_dict(mr, |m| m.set(b"Popup".to_vec(), Object::Ref(foreign))).unwrap();
+        assert_eq!(popup_of_mark(&doc), None, "{foreign:?} isn't the mark's");
+    }
+    // A pop-up whose /Parent isn't a reference (it must be one) isn't the mark's either: it is
+    // neither reported nor changed, and the mark gets one of its own.
+    let mut bad = Dict::new();
+    bad.set(b"Type".to_vec(), Object::name("Annot"));
+    bad.set(b"Subtype".to_vec(), Object::name("Popup"));
+    bad.set(b"Rect".to_vec(), Object::Array([0.0, 0.0, 50.0, 50.0].iter().map(|v| Object::Real(*v)).collect()));
+    bad.set(b"Parent".to_vec(), Object::Dict(Dict::new()));
+    let bad = doc.add(Object::Dict(bad));
+    doc.update_dict(mr, |m| m.set(b"Popup".to_vec(), Object::Ref(bad))).unwrap();
+    assert_eq!(popup_of_mark(&doc), None, "a direct /Parent");
+    let bad_before = doc.get(bad).as_dict().cloned();
+    set_popup(&mut doc, 0, mark, None, Some(true), &meta("h1")).unwrap();
+    assert_eq!(doc.get(bad).as_dict().cloned(), bad_before, "left alone");
+    assert_ne!(annot_dict(&doc, mr).reference(b"Popup"), Some(bad));
+    // A pop-up with no /Parent at all is taken as the comment's.
+    let mut orphan = Dict::new();
+    orphan.set(b"Type".to_vec(), Object::name("Annot"));
+    orphan.set(b"Subtype".to_vec(), Object::name("Popup"));
+    orphan.set(b"Rect".to_vec(), Object::Array([0.0, 0.0, 50.0, 50.0].iter().map(|v| Object::Real(*v)).collect()));
+    let orphan = doc.add(Object::Dict(orphan));
+    doc.update_dict(mr, |m| m.set(b"Popup".to_vec(), Object::Ref(orphan))).unwrap();
+    assert_eq!(popup_of_mark(&doc), Some(PopupInfo { rect: [0.0, 0.0, 50.0, 50.0], open: false }), "no /Parent");
 
     // None of their own.
     let text_box = add_annotation(&mut doc, &new(0, Shape::TextBox { rect: [10.0, 10.0, 110.0, 40.0], font_size: 12.0 }), &meta("t1")).unwrap();

crates/render/src/inspect.rs

@@ -1512,7 +1512,14 @@ impl<'a> Inspector<'a> {
                 }
                 _ => Vec::new(),
             };
-            let popup = d.get(b"Popup").ok().and_then(|o| self.dict(o)).map(|p| {
+            // Only the comment's own pop-up: a pop-up annotation whose /Parent, if any, is a
+            // reference to this comment (keep in step with `pdfcraft_annot::summaries`).
+            let own = a.as_reference().ok();
+            let is_own = |p: &&Dictionary| {
+                self.name(p, b"Subtype").as_deref() == Some("Popup")
+                    && p.get(b"Parent").map_or(true, |parent| parent.as_reference().ok().is_some_and(|id| Some(id) == own))
+            };
+            let popup = d.get(b"Popup").ok().and_then(|o| self.dict(o)).filter(is_own).map(|p| {
                 let [x0, y0, x1, y1] = rect4(self.resolve(p.get(b"Rect").unwrap_or(&Object::Null)));
                 Popup {
                     rect: [x0.min(x1), y0.min(y1), x0.max(x1), y0.max(y1)],
@@ -4107,6 +4114,21 @@ trailer << /Root 1 0 R >>
         let square = info.annotations.iter().find(|a| a.subtype == "Square").expect("the square");
         assert_eq!(square.popup, None);
         assert!(info.annotations.iter().all(|a| a.subtype != "Popup"), "the pop-up is not a comment of its own");
+
+        // #2137 review: a /Popup naming another annotation, a pop-up of another comment, or one
+        // whose /Parent isn't a reference isn't this comment's pop-up; one without /Parent is.
+        for (target, extra, own) in [
+            ("6 0 R", "", false),
+            ("7 0 R", "7 0 obj << /Type /Annot /Subtype /Popup /Rect [0 0 50 50] /Parent 6 0 R >> endobj\n", false),
+            ("7 0 R", "7 0 obj << /Type /Annot /Subtype /Popup /Rect [0 0 50 50] /Parent << >> >> endobj\n", false),
+            ("7 0 R", "7 0 obj << /Type /Annot /Subtype /Popup /Rect [0 0 50 50] >> endobj\n", true),
+        ] {
+            let text =
+                String::from_utf8_lossy(bytes).replace("/Popup 5 0 R", &format!("/Popup {target}")).replace("trailer", &format!("{extra}trailer"));
+            let info = inspect(Arc::new(text.into_bytes()), None).expect("opens");
+            let note = info.annotations.iter().find(|a| a.subtype == "Text").expect("the note");
+            assert_eq!(note.popup.is_some(), own, "/Popup {target} {extra}");
+        }
     }
 
     #[test]