From afa31c83fed769f8a1622015d75c2f889dc22383 Mon Sep 17 00:00:00 2001 From: mtlcl410 <2368091+mtlcl410@users.noreply.github.com> Date: Mon, 19 Oct 2026 12:08:49 +0800 Subject: [PATCH] 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 Claude-Session: https://claude.ai/code/session_01Gv2WwdgjmBz33GWkwgNn3Q --- crates/annot/src/lib.rs | 28 ++++++++++++++++++++-------- crates/annot/src/tests.rs | 34 ++++++++++++++++++++++++++++++++++ crates/render/src/inspect.rs | 24 +++++++++++++++++++++++- 3 files changed, 77 insertions(+), 9 deletions(-) diff --git a/crates/annot/src/lib.rs b/crates/annot/src/lib.rs index d0ed8e5ee9..8e5901710c 100644 --- a/crates/annot/src/lib.rs +++ b/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 { - 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, annot: &Dict) -> Option { + 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, annot: &Dict) -> Option { + // 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 = 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 { 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), }); } } diff --git a/crates/annot/src/tests.rs b/crates/annot/src/tests.rs index b40110d993..5cfbf3abb3 100644 --- a/crates/annot/src/tests.rs +++ b/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(); diff --git a/crates/render/src/inspect.rs b/crates/render/src/inspect.rs index 21ba33972d..434c37436b 100644 --- a/crates/render/src/inspect.rs +++ b/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]