From a7fd7f27ae790c0342ec45127d3184ba7cfad087 Mon Sep 17 00:00:00 2001 From: mtlcl410 <2368091+mtlcl410@users.noreply.github.com> Date: Sat, 31 Oct 2026 09:09:04 +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/info.rs | 23 ++++++++++--- crates/annot/src/modify.rs | 7 ++-- crates/annot/src/tests.rs | 41 ++++++++++++++++++++++++ crates/render/src/inspect/annotations.rs | 9 +++++- crates/render/src/inspect/tests.rs | 14 ++++++++ 5 files changed, 84 insertions(+), 10 deletions(-) diff --git a/crates/annot/src/info.rs b/crates/annot/src/info.rs index bb4cc238a0..c4b385935e 100644 --- a/crates/annot/src/info.rs +++ b/crates/annot/src/info.rs @@ -52,9 +52,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)". +pub(crate) 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(); @@ -168,7 +183,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/modify.rs b/crates/annot/src/modify.rs index d594689e7e..8e5a297ab3 100644 --- a/crates/annot/src/modify.rs +++ b/crates/annot/src/modify.rs @@ -2,7 +2,7 @@ use super::create::{ add_annotation, base_dict, bounds, callout_attach, ending_draws, ending_pair, finite, grow, normalize, num_array, rect_for, rgb, }; use super::draw::{draw_appearance, redraw_or_restore, set_appearance}; -use super::info::{is_comment_subtype, line_endings_of, text_value}; +use super::info::{is_comment_subtype, line_endings_of, own_popup, text_value}; use super::lookup::{annot_dict, annot_ref, annotation_flags, annots, is_fill_sign, page_ref, set_annots}; use super::types::{ FLAG_HIDDEN, FLAG_LOCKED, FLAG_NO_ROTATE, FLAG_NO_ZOOM, FLAG_PRINT, LineEnding, Markup, Meta, NewAnnotation, ReviewState, Rgb, Shape, Style, @@ -312,10 +312,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)); diff --git a/crates/annot/src/tests.rs b/crates/annot/src/tests.rs index 10e8069681..c8e71c68d6 100644 --- a/crates/annot/src/tests.rs +++ b/crates/annot/src/tests.rs @@ -2240,6 +2240,12 @@ 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 (see the helper). + foreign_pop_ups_are_not_the_marks(&mut doc, r, mark, mr); // 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(); @@ -2250,3 +2256,38 @@ fn pop_ups_open_close_and_move_and_are_reported() { assert!(matches!(set_popup(&mut doc, 0, note, Some([f64::NAN, 0.0, 10.0, 10.0]), None, &meta("n1")), Err(AnnotError::Invalid(_)))); assert!(matches!(set_popup(&mut doc, 0, 99, None, Some(true), &meta("n1")), Err(AnnotError::NoSuchAnnotation { .. }))); } + +/// #2137 review: a pop-up that is not the mark's own is neither reported as its pop-up nor +/// changed through it (part of `pop_ups_open_close_and_move_and_are_reported`). +fn foreign_pop_ups_are_not_the_marks(doc: &mut Document, note: ObjRef, mark: usize, mr: ObjRef) { + // #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, note).reference(b"Popup").expect("the note's pop-up"); + for foreign in [note, 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(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"); +} diff --git a/crates/render/src/inspect/annotations.rs b/crates/render/src/inspect/annotations.rs index fcb9dd77bb..e16680119a 100644 --- a/crates/render/src/inspect/annotations.rs +++ b/crates/render/src/inspect/annotations.rs @@ -158,7 +158,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)], diff --git a/crates/render/src/inspect/tests.rs b/crates/render/src/inspect/tests.rs index 79b938719f..5258c4f682 100644 --- a/crates/render/src/inspect/tests.rs +++ b/crates/render/src/inspect/tests.rs @@ -1861,6 +1861,20 @@ fn a_comments_pop_up_is_read_with_its_place_and_state() { 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]