M5: pop-up rectangles read alike, edits in one frame kept, pop-up drafts follow their comment (#2137 review)

- The render path reads a pop-up as none unless its /Rect is four numbers,
  as pdfcraft_annot::summaries does; set_popup gives a comment's own pop-up
  without one the place a new pop-up gets, so opening it shows it again.
- The composer's edit and a pop-up's, landing in one frame, are queued
  together in one batch instead of the pop-up's replacing the composer's.
- A pop-up's unsaved text, reply draft, drag and resize follow their comment
  when a deletion moves it, are dropped on undo/redo, and are dropped after
  any other edit that leaves their (page, index) naming another comment
  (pages moved, inserted or deleted).

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

crates/annot/src/info.rs

@@ -70,7 +70,7 @@ pub(crate) fn own_popup(doc: &Document, own: Option<ObjRef>, annot: &Dict) -> Op
 
 /// 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> {
+pub(crate) 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);

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, own_popup, text_value};
+use super::info::{is_comment_subtype, line_endings_of, own_popup, popup_info, 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,
@@ -310,30 +310,36 @@ pub fn set_popup(doc: &mut Document, page: usize, index: usize, rect: Option<[f6
         None => None,
     };
     let _ = meta;
+    // Where a new pop-up goes: to the right of the comment, as new notes' do.
+    let own: Vec<f64> = d
+        .get(b"Rect")
+        .map(|o| doc.resolve(o))
+        .and_then(|o| o.as_array().cloned())
+        .unwrap_or_default()
+        .iter()
+        .filter_map(|v| doc.resolve(v).as_f64())
+        .collect();
+    let beside = match own[..] {
+        [_, _, x1, y1] if finite(&own) => [x1 + 10.0, y1 - 120.0, x1 + 210.0, y1],
+        _ => [0.0, 0.0, 200.0, 120.0],
+    };
     // 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.
     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));
-            }
-            if let Some(open) = open {
-                p.set(b"Open".to_vec(), Object::Bool(open));
-            }
-        })?,
+        Some(pr) => {
+            // One without a `/Rect` of four numbers reads as no pop-up (`summaries`), so it gets
+            // the new pop-up's place: opening it then shows it.
+            let rect = rect.or_else(|| popup_info(doc, Some(r), &d).is_none().then_some(beside));
+            doc.update_dict(pr, |p| {
+                if let Some(rect) = rect {
+                    p.set(b"Rect".to_vec(), num_array(&rect));
+                }
+                if let Some(open) = open {
+                    p.set(b"Open".to_vec(), Object::Bool(open));
+                }
+            })?
+        }
         None => {
-            let own: Vec<f64> = d
-                .get(b"Rect")
-                .map(|o| doc.resolve(o))
-                .and_then(|o| o.as_array().cloned())
-                .unwrap_or_default()
-                .iter()
-                .filter_map(|v| doc.resolve(v).as_f64())
-                .collect();
-            let beside = match own[..] {
-                [_, _, x1, y1] if finite(&own) => [x1 + 10.0, y1 - 120.0, x1 + 210.0, y1],
-                _ => [0.0, 0.0, 200.0, 120.0],
-            };
             let mut pd = Dict::new();
             pd.set(b"Type".to_vec(), Object::name("Annot"));
             pd.set(b"Subtype".to_vec(), Object::name("Popup"));

crates/annot/src/tests.rs

@@ -2300,3 +2300,23 @@ fn foreign_pop_ups_are_not_the_marks(doc: &mut Document, note: ObjRef, mark: usi
     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");
 }
+
+/// #2137 review: a comment's own pop-up without a `/Rect` of four numbers reads as none; opening
+/// it gives it the place a new pop-up gets, so the window shows again.
+#[test]
+fn opening_a_pop_up_without_a_rect_gives_it_one() {
+    let mut doc = fixture();
+    let note = add_annotation(&mut doc, &new(0, Shape::Note { at: [20.0, 280.0], icon: NoteIcon::Comment }), &meta("n1")).unwrap();
+    let (_, r) = annot_ref(&mut doc, 0, note).unwrap();
+    let pr = annot_dict(&doc, r).reference(b"Popup").expect("new notes have a pop-up");
+    doc.update_dict(pr, |p| {
+        p.remove(b"Rect");
+    })
+    .unwrap();
+    assert_eq!(summaries(&doc).into_iter().find(|s| s.index == note).unwrap().popup, None, "no /Rect reads as no pop-up");
+    set_popup(&mut doc, 0, note, None, Some(true), &meta("n1")).unwrap();
+    let popup = summaries(&doc).into_iter().find(|s| s.index == note).unwrap().popup.expect("the pop-up reads again");
+    assert!(popup.open);
+    assert!(popup.rect[2] - popup.rect[0] >= 100.0 && popup.rect[3] - popup.rect[1] >= 100.0, "a window's size: {:?}", popup.rect);
+    assert_eq!(annot_dict(&doc, r).reference(b"Popup"), Some(pr), "the same pop-up, not a new one");
+}

crates/render/src/inspect/annotations.rs

@@ -166,12 +166,16 @@ impl<'a> Inspector<'a> {
                     && p.get(b"Parent")
                         .map_or(true, |parent| parent.as_reference().is_ok() && self.resolved_id(parent).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 {
+            // A pop-up without a `/Rect` of four numbers is none, as `pdfcraft_annot::summaries`
+            // reads it: both lists of the comments agree.
+            let popup = d.get(b"Popup").ok().and_then(|o| self.dict(o)).filter(is_own).and_then(|p| {
+                let rect = self.resolve(p.get(b"Rect").ok()?).as_array().ok()?;
+                let nums: Vec<f32> = rect.iter().filter_map(|v| self.resolve(v).as_float().ok()).collect();
+                let [x0, y0, x1, y1] = nums[..] else { return None };
+                Some(Popup {
                     rect: [x0.min(x1), y0.min(y1), x0.max(x1), y0.max(y1)],
                     open: p.get(b"Open").ok().and_then(|o| self.resolve(o).as_bool().ok()).unwrap_or(false),
-                }
+                })
             });
             let annotation = Annotation {
                 page,

crates/render/src/inspect/tests.rs

@@ -1904,3 +1904,15 @@ trailer << /Root 1 0 R >>
     assert!((p.width - 3874.96).abs() < 0.1, "width expected ~3874.96, got {}", p.width);
     assert!((p.height - 5493.54).abs() < 0.1, "height expected ~5493.54, got {}", p.height);
 }
+
+/// #2137 review: a pop-up without a `/Rect` of four numbers reads as no pop-up, as
+/// `pdfcraft_annot::summaries` reads it, so the two lists of comments agree.
+#[test]
+fn a_pop_up_without_a_four_number_rect_is_none() {
+    let bytes = "%PDF-1.7\n1 0 obj << /Type /Catalog /Pages 2 0 R >> endobj\n2 0 obj << /Type /Pages /Kids [3 0 R] /Count 1 >> endobj\n3 0 obj << /Type /Page /Parent 2 0 R /MediaBox [0 0 200 300] /Annots [4 0 R 5 0 R] >> endobj\n4 0 obj << /Type /Annot /Subtype /Text /Rect [20 260 40 280] /Contents (Hi) /Popup 5 0 R >> endobj\n5 0 obj << /Type /Annot /Subtype /Popup RECT /Parent 4 0 R /Open true >> endobj\ntrailer << /Root 1 0 R >>\n%%EOF\n";
+    for (rect, read) in [("", false), ("/Rect [0 0 50]", false), ("/Rect (box)", false), ("/Rect [0 0 50 /x]", false), ("/Rect [0 0 50 60]", true)] {
+        let info = inspect(Arc::new(bytes.replace("RECT", rect).into_bytes()), None).expect("opens");
+        let note = info.annotations.iter().find(|a| a.subtype == "Text").expect("the note");
+        assert_eq!(note.popup.is_some(), read, "{rect:?}");
+    }
+}

crates/ui-egui/src/canvas.rs

@@ -2733,7 +2733,12 @@ fn frame_overlays(ui: &mut egui::Ui, view: &mut DocView, env: &PageEnv<'_>, prim
         view.pending_edit = Some(e);
     }
     if let Some(e) = comments::popups(ui.ctx(), view, info, prefs, comments_hidden, allowed) {
-        view.pending_edit = Some(e);
+        // A click on the composer's Post also takes the focus from a pop-up's box, saving what
+        // was typed there: both edits land in this frame, so both are kept, in one batch.
+        view.pending_edit = Some(match view.pending_edit.take() {
+            Some(first) => Edit::Batch { label: "Edit comment".into(), edits: vec![first, e] },
+            None => e,
+        });
     }
     if let Some(e) = crate::edit_text_ui::overlay(ui.ctx(), view, info) {
         view.pending_edit = Some(e);

crates/ui-egui/src/editing.rs

@@ -123,6 +123,15 @@ impl PdfCraftApp {
             (Edit::DeleteAnnotation { page, .. }, Some(d)) => d.info.annotations.iter().filter(|a| a.page == *page).cloned().collect(),
             _ => Vec::new(),
         };
+        // The comments the pop-ups hold unsaved text or a drag for, to see afterwards whether
+        // their places still name them (pages may move, come or go).
+        let popups_before: Vec<pdfcraft_render::Annotation> = match (self.views.get(i), self.session.get(id)) {
+            (Some(v), Some(d)) => {
+                let keys = v.comments.popup_keys();
+                d.info.annotations.iter().filter(|a| keys.contains(&(a.page, a.index))).cloned().collect()
+            }
+            _ => Vec::new(),
+        };
         match self.session.apply(id, edit.clone()) {
             Ok(()) => {
                 let Some(doc) = self.session.get(id) else { return true };
@@ -169,6 +178,10 @@ impl PdfCraftApp {
                 if matches!(edit, Edit::AddAnnotation(_)) {
                     view.comments.comment_added();
                 }
+                // A deletion moves the pop-ups' state with its comments (`annotation_deleted`).
+                if !matches!(edit, Edit::DeleteAnnotation { .. }) {
+                    view.comments.popups_after_edit(&popups_before, &info.annotations);
+                }
                 // Keep the pages the user acted on selected, where they now are.
                 select_after_edit(view, edit, info, &annots_before, self.quick_tool);
                 if std::mem::take(&mut view.comments.tool_done) && !self.comment_prefs.pinned {

crates/ui-egui/src/tools/comments/state.rs

@@ -242,16 +242,59 @@ impl CommentView {
         if self.editing.is_none() {
             self.editor_focus = None;
         }
+        // What is typed in a pop-up, and a window being dragged or resized, follow theirs too.
+        let moved = |k: (usize, usize)| after_delete(before, after, gone, k);
+        self.popup_text = self.popup_text.take().and_then(|(p, i, text)| moved((p, i)).map(|(p, i)| (p, i, text)));
+        self.popup_reply = self.popup_reply.take().and_then(|(p, i, text)| moved((p, i)).map(|(p, i)| (p, i, text)));
+        self.popup_drag = self.popup_drag.and_then(|(p, i, at)| moved((p, i)).map(|(p, i)| (p, i, at)));
+        self.popup_resize = self.popup_resize.and_then(|(p, i, size)| moved((p, i)).map(|(p, i)| (p, i, size)));
     }
 
     /// Undo or Redo just ran. Either may have put annotations back or taken them away, on any
     /// page, so a (page, index) held here may now name another one. The selection and the inline
     /// editor close, its unsaved text with it, rather than Save writing that text over whatever
-    /// took its place.
+    /// took its place; so do the pop-ups' unsaved text and drags.
     pub(crate) fn history_stepped(&mut self) {
         self.selected = None;
         self.editing = None;
         self.editor_focus = None;
+        self.popup_text = None;
+        self.popup_reply = None;
+        self.popup_drag = None;
+        self.popup_resize = None;
+    }
+
+    /// The comments the pop-ups hold unsaved text, a reply or a drag for.
+    pub(crate) fn popup_keys(&self) -> Vec<(usize, usize)> {
+        let text = self.popup_text.as_ref().map(|(p, i, _)| (*p, *i));
+        let reply = self.popup_reply.as_ref().map(|(p, i, _)| (*p, *i));
+        let drag = self.popup_drag.map(|(p, i, _)| (p, i));
+        let resize = self.popup_resize.map(|(p, i, _)| (p, i));
+        [text, reply, drag, resize].into_iter().flatten().collect()
+    }
+
+    /// An edit other than a deletion just ran: `before` are the comments at [`Self::popup_keys`]
+    /// until then, `after` the document's annotations now. When pages moved, came or went, a pop-up's (page, index)
+    /// may now name another comment: what was typed in that pop-up and not saved, and a window
+    /// being dragged or resized, are dropped rather than written to whatever took its place.
+    pub(crate) fn popups_after_edit(&mut self, before: &[Annotation], after: &[Annotation]) {
+        // The same comment: its kind, author and `/NM`, at that place before and after.
+        let who = |list: &[Annotation], k: (usize, usize)| {
+            list.iter().find(|a| (a.page, a.index) == k).map(|a| (a.subtype.clone(), a.author.clone(), a.name.clone()))
+        };
+        let kept = |p: usize, i: usize| who(before, (p, i)).is_some_and(|w| who(after, (p, i)) == Some(w));
+        if self.popup_text.as_ref().is_some_and(|(p, i, _)| !kept(*p, *i)) {
+            self.popup_text = None;
+        }
+        if self.popup_reply.as_ref().is_some_and(|(p, i, _)| !kept(*p, *i)) {
+            self.popup_reply = None;
+        }
+        if self.popup_drag.is_some_and(|(p, i, _)| !kept(p, i)) {
+            self.popup_drag = None;
+        }
+        if self.popup_resize.is_some_and(|(p, i, _)| !kept(p, i)) {
+            self.popup_resize = None;
+        }
     }
 }
 

crates/ui-egui/tests/it/comments.rs

@@ -3439,3 +3439,73 @@ fn a_pop_up_resized_on_a_turned_page_stays_in_place() {
     // Taller by the drag, less what its text had already stretched it.
     assert!(resized.height() > window.height() + 50.0, "taller: {window:?} -> {resized:?}");
 }
+
+/// #2137 review: the composer's Post, clicked while a pop-up's box holds unsaved text, also
+/// saves that text as the box loses the focus. Both land in one frame and both are kept.
+#[test]
+fn posting_from_the_composer_keeps_a_pop_ups_unsaved_text() {
+    let mut h = harness_with_bytes(BLUE_NOTE_WITH_OPEN_POP_UP.to_vec(), |app| app.set_option("quick", "note").unwrap());
+    // Low on the left, so the composer stays clear of the window.
+    click_pt(&mut h, (30.0, 40.0));
+    // The composer opens with its editor focused.
+    h.run_steps(2);
+    h.event(egui::Event::Text("New note".into()));
+    h.run_steps(2);
+    let box_at = h.query_all_by(|n| shows_text(n, "A note")).next().expect("the pop-up's box").rect().right_center() - egui::vec2(4.0, 0.0);
+    click(&mut h, box_at);
+    h.event(egui::Event::Text(" edited".into()));
+    h.run_steps(2);
+    assert!(h.state().views[0].comments.composer.is_some(), "the composer is still open");
+    let post = h.get_by_label("Post").rect().center();
+    click(&mut h, post);
+    h.run_steps(3);
+    let all = comments(&h);
+    assert!(
+        all.iter().any(|a| a.contents.as_deref() == Some("New note")),
+        "the composer's note: {:?}",
+        all.iter().map(|a| &a.contents).collect::<Vec<_>>()
+    );
+    assert!(
+        all.iter().any(|a| a.contents.as_deref() == Some("A note edited")),
+        "the pop-up's text: {:?}",
+        all.iter().map(|a| &a.contents).collect::<Vec<_>>()
+    );
+}
+
+/// #2137 review: what a pop-up holds for a comment (a reply being typed, unsaved text) follows
+/// that comment when a deletion moves it, and is dropped when an undo or a page edit leaves its
+/// (page, index) naming another; other edits keep it.
+#[test]
+fn a_pop_ups_unsaved_work_follows_its_comment_or_is_dropped() {
+    use pdfcraft_engine::Edit;
+    let mut h = harness_with_bytes(NOTE_WITH_A_THREAD.to_vec(), |_| {});
+    let key = |h: &Harness<'static, PdfCraftApp>, text: &str| {
+        comments(h).into_iter().find(|a| a.contents.as_deref() == Some(text)).map(|a| (a.page, a.index)).expect(text)
+    };
+    let (second, third) = (key(&h, "Second"), key(&h, "Third"));
+    h.state_mut().views[0].comments.popup_reply = Some((third.0, third.1, "draft".into()));
+
+    // Another edit, on another comment: the draft stays.
+    assert!(h.state_mut().apply_edit(Edit::SetAnnotationContents { page: second.0, index: second.1, text: "Second!".into() }));
+    h.run_steps(2);
+    assert_eq!(h.state().views[0].comments.popup_reply, Some((third.0, third.1, "draft".into())));
+
+    // Deleting "Second" (and its answer) moves "Third" up: the draft goes with it.
+    assert!(h.state_mut().apply_edit(Edit::DeleteAnnotation { page: second.0, index: second.1 }));
+    h.run_steps(2);
+    let moved = key(&h, "Third");
+    assert_ne!(moved, third, "Third moved up");
+    assert_eq!(h.state().views[0].comments.popup_reply, Some((moved.0, moved.1, "draft".into())), "the draft is still Third's");
+
+    // Undo puts the thread back: the draft is dropped rather than left on another reply.
+    h.state_mut().undo();
+    h.run_steps(2);
+    assert_eq!(h.state().views[0].comments.popup_reply, None);
+
+    // A page inserted before it: the comment's (page, index) now names nothing of it.
+    let note = key(&h, "First");
+    h.state_mut().views[0].comments.popup_text = Some((note.0, note.1, "unsaved".into()));
+    assert!(h.state_mut().apply_edit(Edit::InsertBlankPage { at: 0, width: 300.0, height: 200.0 }));
+    h.run_steps(2);
+    assert_eq!(h.state().views[0].comments.popup_text, None);
+}