diff --git a/crates/annot/src/info.rs b/crates/annot/src/info.rs index 6a90541437..18a395a848 100644 --- a/crates/annot/src/info.rs +++ b/crates/annot/src/info.rs @@ -70,7 +70,7 @@ pub(crate) fn own_popup(doc: &Document, own: Option, 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, annot: &Dict) -> Option { +pub(crate) 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); diff --git a/crates/annot/src/modify.rs b/crates/annot/src/modify.rs index 8e5a297ab3..7a1071c306 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, 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 = 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 = 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")); diff --git a/crates/annot/src/tests.rs b/crates/annot/src/tests.rs index e20f001a65..4e45d1d777 100644 --- a/crates/annot/src/tests.rs +++ b/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"); +} diff --git a/crates/render/src/inspect/annotations.rs b/crates/render/src/inspect/annotations.rs index 71e95024ab..36e0556f65 100644 --- a/crates/render/src/inspect/annotations.rs +++ b/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 = 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, diff --git a/crates/render/src/inspect/tests.rs b/crates/render/src/inspect/tests.rs index 94794b698b..62bc130290 100644 --- a/crates/render/src/inspect/tests.rs +++ b/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:?}"); + } +} diff --git a/crates/ui-egui/src/canvas.rs b/crates/ui-egui/src/canvas.rs index 7d9187270b..5c5dd611f8 100644 --- a/crates/ui-egui/src/canvas.rs +++ b/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); diff --git a/crates/ui-egui/src/editing.rs b/crates/ui-egui/src/editing.rs index f1c9ba7949..71b6beb030 100644 --- a/crates/ui-egui/src/editing.rs +++ b/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 = 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 { diff --git a/crates/ui-egui/src/tools/comments/state.rs b/crates/ui-egui/src/tools/comments/state.rs index 32ef995f8f..54a723396f 100644 --- a/crates/ui-egui/src/tools/comments/state.rs +++ b/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; + } } } diff --git a/crates/ui-egui/tests/it/comments.rs b/crates/ui-egui/tests/it/comments.rs index e2f6b0412b..64de66a640 100644 --- a/crates/ui-egui/tests/it/comments.rs +++ b/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::>() + ); + 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::>() + ); +} + +/// #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); +}