diff --git a/ROADMAP.md b/ROADMAP.md index 5634d6bf28..245eafca64 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -199,6 +199,7 @@ Ranked; detail and the full milestone table in [docs/roadmap.md](docs/roadmap.md - **2026-10-20 (M3, #3535):** Escape with a menu open closes the menu and keeps the text selection; the next Escape clears it, as before. One keypress used to do both. A synthetic UI regression checks it. Estimates unchanged. - **2026-10-16 (M3, close prompt keyboard):** The "Save changes?" prompt opens with Save chosen (Enter saves, as before) and the arrow keys and Tab / Shift+Tab move the choice to Cancel or Don't save, shown with a focus ring; Enter or Space press the chosen button and Escape still cancels. Before, Enter always saved whatever was Tab-focused. Estimates unchanged. - **2026-10-21 (M3, #3808):** Page thumbnails in the Inspector can be resized independently from the document and Organize, with a slider, buttons and `thumbnail-zoom` control option. Selection, page following and reload are covered by regressions. Estimates unchanged. +- **2026-10-22 (M5, comment threads):** Replies join the comment their `/IRT` references, not every comment sharing its `/NM`; PyMuPDF names each page's first comment `fitz-A0`, so all replies used to land under page 1. The Comments header counts comments without their replies (28, as Acrobat says, for a file it used to call 44), and a reply's thread line runs from the avatar above it to its own. UI, engine and automation regressions cover it. Estimates unchanged. - **2026-10-22 (M2, #3107 / PR #3438):** Inline-image parsing falls back to the existing EI heuristic when missing declared samples, headers or terminators exceed the content-window budget, instead of waiting for an impossible allocation. Exact-length parsing for available samples and ordinary large streamed images is retained. Estimates unchanged. - **2026-10-21 (M14, Brazilian Portuguese):** Completed the `pt-br` catalog against the current 2,674-key union, including signature validation and accessibility messages; no build or compilation run. Localization remains partial because help and unpredictable engine/OS error details are not translated. - **2026-10-21 (M14, #3557):** Preferences: the OK button stays below the scrolling settings instead of being the last thing in them, so it is in reach in a short window. diff --git a/crates/annot/src/lib.rs b/crates/annot/src/lib.rs index 720d39cd80..7b3446cfcf 100644 --- a/crates/annot/src/lib.rs +++ b/crates/annot/src/lib.rs @@ -2254,6 +2254,8 @@ pub struct Summary { pub modified: Option, pub name: Option, pub in_reply_to: Option, + /// The page and `/Annots` position `/IRT` points at, by reference (`/NM` need not be unique). + pub parent: Option<(usize, usize)>, pub rect: [f32; 4], pub color: Option<[f32; 3]>, pub state: Option, @@ -2289,8 +2291,18 @@ pub fn is_comment_subtype(subtype: &str) -> bool { pub fn summaries(doc: &Document) -> Vec { let mut out = Vec::new(); let Ok(pages) = page_refs(doc) else { return out }; - for (page, p) in pages.iter().enumerate() { - for (index, entry) in annots(doc, *p).iter().enumerate() { + let lists: Vec> = pages.iter().map(|p| annots(doc, *p)).collect(); + // Where each annotation is first listed, so a reply's `/IRT` finds its parent by reference. + let mut slots: std::collections::HashMap = std::collections::HashMap::new(); + for (page, list) in lists.iter().enumerate() { + for (index, entry) in list.iter().enumerate() { + if let Some(r) = entry.as_ref() { + slots.entry(r).or_insert((page, index)); + } + } + } + for (page, list) in lists.iter().enumerate() { + for (index, entry) in list.iter().enumerate() { let obj = doc.resolve(entry); let Some(d) = obj.as_dict() else { continue }; let Some(subtype) = d.name(b"Subtype").map(|s| String::from_utf8_lossy(s).into_owned()) else { continue }; @@ -2326,6 +2338,7 @@ pub fn summaries(doc: &Document) -> Vec { _ => Vec::new(), }; let in_reply_to = d.get(b"IRT").map(|o| doc.resolve(o)).and_then(|o| o.as_dict().cloned()).and_then(|p| text_value(doc, &p, b"NM")); + let parent = d.get(b"IRT").and_then(Object::as_ref).and_then(|r| slots.get(&r).copied()); out.push(Summary { page, index, @@ -2337,6 +2350,7 @@ pub fn summaries(doc: &Document) -> Vec { modified: text_value(doc, d, b"M"), name: text_value(doc, d, b"NM"), in_reply_to, + parent, rect, color, state: text_value(doc, d, b"State"), diff --git a/crates/automation/src/comments.rs b/crates/automation/src/comments.rs index 3d1b8d82dd..bd583aeaed 100644 --- a/crates/automation/src/comments.rs +++ b/crates/automation/src/comments.rs @@ -200,26 +200,24 @@ impl Automation { }; let roots: Vec = all .iter() - .filter(|c| c.in_reply_to.is_none() && only.is_none_or(|p| c.page == p)) + .filter(|c| !c.is_reply() && only.is_none_or(|p| c.page == p)) .map(|c| { let mut v = view(c); // Every reply under this comment, at any depth, in document order. A reply to a // reply keeps its `/IRT` on the direct parent, so following one level would drop it. - let descends_from = |r: &Annotation, root: &str| -> bool { - let mut parent = r.in_reply_to.as_deref(); + // Parents are found by reference: `/NM` need not be unique. + let descends_from = |r: &Annotation, root: &Annotation| -> bool { + let mut parent = r.parent; for _ in 0..64 { - let Some(nm) = parent else { return false }; - if nm == root { + let Some(at) = parent else { return false }; + if at == (root.page, root.index) { return true; } - parent = all.iter().find(|x| x.name.as_deref() == Some(nm)).and_then(|x| x.in_reply_to.as_deref()); + parent = all.iter().find(|x| (x.page, x.index) == at).and_then(|x| x.parent); } false }; - let thread: Vec<&Annotation> = match c.name.as_deref() { - Some(nm) => all.iter().filter(|r| r.in_reply_to.is_some() && descends_from(r, nm)).collect(), - None => Vec::new(), - }; + let thread: Vec<&Annotation> = all.iter().filter(|r| r.is_reply() && descends_from(r, c)).collect(); let status = thread.iter().rev().filter(|r| !r.is_mark()).find_map(|r| r.state.clone()); v["status"] = json!(status); v["marked"] = json!(thread.iter().rev().find(|r| r.is_mark()).is_some_and(|r| r.state.as_deref() == Some("Marked"))); diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 68fc076b97..cd9ec5b855 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -2900,6 +2900,7 @@ fn comment_list(doc: &pdfcraft_cos::Document) -> Vec &str { /// pop-ups; the number is the comment's position on its page. pub fn comment_summary(name: &str, all: &[pdfcraft_render::Annotation], sort: SummarySort) -> String { use std::fmt::Write; - let top: Vec<&pdfcraft_render::Annotation> = all.iter().filter(|a| a.in_reply_to.is_none() && a.state.is_none()).collect(); + let top: Vec<&pdfcraft_render::Annotation> = all.iter().filter(|a| !a.is_reply() && a.state.is_none()).collect(); // Write a comment's replies, indenting each level. A reply to a reply keeps its `/IRT` on the // direct parent, so following one level would leave the deeper replies out of the summary. - fn write_replies(out: &mut String, all: &[pdfcraft_render::Annotation], parent: &str, depth: usize) { + fn write_replies(out: &mut String, all: &[pdfcraft_render::Annotation], parent: &pdfcraft_render::Annotation, depth: usize) { use std::fmt::Write; if depth > 64 { return; } - for r in all.iter().filter(|r| r.state.is_none() && r.in_reply_to.as_deref() == Some(parent)) { + for r in all.iter().filter(|r| r.state.is_none() && r.replies_to(parent)) { let indent = " ".repeat(depth + 1); let _ = writeln!(out, "{indent}Author: {} Subject: Reply Date: {}", r.author.as_deref().unwrap_or(""), r.modified.as_deref().unwrap_or("")); for line in r.contents.as_deref().unwrap_or("").lines() { let _ = writeln!(out, "{indent}{line}"); } - if let Some(nm) = r.name.as_deref() { - write_replies(out, all, nm, depth + 1); - } + write_replies(out, all, r, depth + 1); } } let mut numbered: Vec<(usize, &pdfcraft_render::Annotation)> = Vec::new(); @@ -5383,9 +5382,7 @@ pub fn comment_summary(name: &str, all: &[pdfcraft_render::Annotation], sort: Su if let Some(c) = a.contents.as_deref().filter(|c| !c.is_empty()) { let _ = writeln!(out, "{c}"); } - if let Some(nm) = a.name.as_deref() { - write_replies(&mut out, all, nm, 0); - } + write_replies(&mut out, all, a, 0); out.push('\n'); } out diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index ed4e6efa1c..44ac16c345 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -3419,6 +3419,37 @@ fn comments_lock_take_checkmarks_hide_and_summarize() { assert_eq!(comment_summary("x", &[], SummarySort::Page), "Summary of Comments on x\n\nThis document has no comments.\n"); } +/// Replies thread by their `/IRT` reference, not `/NM`: PyMuPDF names the first comment on every +/// page `fitz-A0`, which used to file every reply under page 1's comment. Both readers (inspect on +/// open, and the comment list the engine refreshes after an edit) must agree. +#[test] +fn replies_thread_by_reference_when_names_repeat_across_pages() { + let pdf = b"%PDF-1.7 +1 0 obj << /Type /Catalog /Pages 2 0 R >> endobj +2 0 obj << /Type /Pages /Kids [3 0 R 4 0 R] /Count 2 >> endobj +3 0 obj << /Type /Page /Parent 2 0 R /MediaBox [0 0 300 200] /Annots [5 0 R] >> endobj +4 0 obj << /Type /Page /Parent 2 0 R /MediaBox [0 0 300 200] /Annots [6 0 R 7 0 R] >> endobj +5 0 obj << /Type /Annot /Subtype /Text /Rect [20 140 40 160] /NM (fitz-A0) /T (Alpha) /Contents (COVER) >> endobj +6 0 obj << /Type /Annot /Subtype /Text /Rect [20 140 40 160] /NM (fitz-A0) /T (Beta) /Contents (SECOND) >> endobj +7 0 obj << /Type /Annot /Subtype /Text /Rect [20 100 40 120] /NM (fitz-A1) /IRT 6 0 R /T (Gamma) /Contents (ANSWER) >> endobj +trailer << /Root 1 0 R >> +%%EOF"; + let mut s = Session::new().with_clock(|| 1_700_000_000); + let id = s.open("fitz.pdf", None, Arc::new(pdf.to_vec()), None).expect("opens"); + let check = |s: &Session, replies: usize| { + let all = &s.get(id).unwrap().info.annotations; + let answer = all.iter().find(|a| a.contents.as_deref() == Some("ANSWER")).unwrap(); + assert_eq!(answer.parent, Some((1, 0)), "{answer:?}"); + let text = comment_summary("fitz.pdf", all, SummarySort::Page); + let (cover, second) = text.split_once("SECOND").unwrap(); + assert!(!cover.contains("ANSWER"), "page 1's comment has no replies: {text}"); + assert_eq!(second.matches(" Author:").count(), replies, "{text}"); + }; + check(&s, 1); + s.apply(id, Edit::ReplyToAnnotation { page: 1, index: 0, text: "LATER".into(), author: "Delta".into() }).unwrap(); + check(&s, 2); +} + /// #820: the generated summary includes replies to replies, indented one level deeper. #[test] fn comment_summary_includes_replies_to_replies() { diff --git a/crates/render/src/inspect.rs b/crates/render/src/inspect.rs index 81fad6b0b2..0490390163 100644 --- a/crates/render/src/inspect.rs +++ b/crates/render/src/inspect.rs @@ -6,6 +6,7 @@ //! are empty and `warnings` says why. use std::cell::OnceCell; +use std::collections::hash_map::Entry; use std::collections::{BTreeMap, HashMap, HashSet}; use std::sync::Arc; @@ -417,9 +418,13 @@ pub struct Annotation { pub author: Option, pub contents: Option, pub modified: Option, - /// `/NM` of this annotation, and of the one it replies to (`/IRT`), for threading. + /// `/NM` of this annotation, and of the one it replies to (`/IRT`). pub name: Option, pub in_reply_to: Option, + /// The page and `/Annots` position of the annotation `/IRT` points at, found by object + /// reference. Threads follow this rather than `/NM`, which need not be unique: PyMuPDF + /// names the first comment on every page `fitz-A0`. + pub parent: Option<(usize, usize)>, /// Rect in PDF user space: [x0, y0, x1, y1]. pub rect: [f32; 4], pub color: Option<[f32; 3]>, @@ -446,6 +451,16 @@ impl Annotation { pub fn is_mark(&self) -> bool { matches!(self.state.as_deref(), Some("Marked" | "Unmarked")) } + + /// A reply (or status reply) to another annotation rather than a comment of its own. + pub fn is_reply(&self) -> bool { + self.in_reply_to.is_some() || self.parent.is_some() + } + + /// Whether this annotation's `/IRT` points at `other`. + pub fn replies_to(&self, other: &Annotation) -> bool { + self.parent == Some((other.page, other.index)) + } } #[derive(Clone, Copy, Debug, PartialEq, Eq)] @@ -1250,13 +1265,12 @@ impl<'a> Inspector<'a> { pages.sort_unstable_by_key(|&(_, page)| page); // An annotation, or a whole `/Annots` array, that several entries or pages list is on the // first of them (§12.5.2 allows one page per annotation). Both are told apart by the object - // a reference resolves to, since an indirect object may itself hold a reference. + // a reference resolves to, since an indirect object may itself hold a reference. Where each + // annotation is listed is collected before any is read, so a reply's `/IRT` finds its + // parent by reference even when the parent comes later. let mut seen_arrays = HashSet::new(); - let mut seen_annotations = HashSet::new(); - let mut kept = 0usize; - // Once the comment budget is spent, comments are no longer built, but links and attachments - // are still listed. - let mut comments_full = false; + let mut slots: HashMap = HashMap::new(); + let mut entries = Vec::new(); for (id, page) in pages { let Ok(page_dict) = self.doc.get_dictionary(id) else { continue }; if let Ok(annots) = page_dict.get(b"Annots") @@ -1267,105 +1281,112 @@ impl<'a> Inspector<'a> { } let Ok(Object::Array(annots)) = page_dict.get(b"Annots").map(|o| self.resolve(o)) else { continue }; for (index, a) in annots.iter().enumerate() { - if let Some(r) = self.resolved_id(a) - && !seen_annotations.insert(r) - { - continue; + if let Some(r) = self.resolved_id(a) { + let Entry::Vacant(slot) = slots.entry(r) else { continue }; + slot.insert((page, index)); } - let Some(d) = self.dict(a) else { continue }; - let Some(subtype) = self.name(d, b"Subtype") else { continue }; - if subtype == "FileAttachment" - && let Ok(fs) = d.get(b"FS") - { - let fallback = |budget: &mut usize| self.budgeted_text(d, b"Contents", budget); - info.attachments.push(self.attachment(fs, AttachmentSource::Annotation { page, index }, text_budget, fallback)); - } - if subtype == "Link" { - if let Some(link) = self.link(page, index, d, text_budget) { - if matches!(&link.target, LinkTarget::Other(s) if s == "JavaScript") { - info.has_javascript = true; - } - info.links.push(link); - } - continue; - } - // Not comments: form widgets, pop-ups and non-markup annotations such as the - // Screen annotation that drives a LaTeX `animate` player (keep in step with - // `pdfcraft_annot::is_comment_subtype`). - if matches!(subtype.as_str(), "Widget" | "Popup" | "Screen" | "Movie" | "RichMedia" | "3D" | "PrinterMark" | "TrapNet" | "Watermark") - { - continue; - } - if comments_full { - continue; - } - let rect = rect4(self.resolve(d.get(b"Rect").unwrap_or(&Object::Null))); - let mut color = match d.get(b"C").map(|o| self.resolve(o)) { - Ok(Object::Array(c)) if c.len() == 3 => { - let f = |i: usize| c[i].as_float().unwrap_or(0.0); - Some([f(0), f(1), f(2)]) - } - _ => None, - }; - // A text box's /C is its background; its text colour is in /DA. - if subtype == "FreeText" - && let Some(da) = self.text(d, b"DA") - { - let t: Vec<&str> = da.split_whitespace().collect(); - if let Some(i) = t.iter().position(|x| *x == "rg") - && i >= 3 - { - let f = |k: usize| t[k].parse::().unwrap_or(0.0); - color = Some([f(i - 3), f(i - 2), f(i - 1)]); - } - } - let quads = match d.get(b"QuadPoints").map(|o| self.resolve(o)) { - Ok(Object::Array(q)) => { - let v: Vec = q.iter().map(|o| o.as_float().unwrap_or(0.0)).collect(); - v.as_chunks::<8>().0.to_vec() - } - _ => Vec::new(), - }; - let in_reply_to = d.get(b"IRT").ok().and_then(|o| self.dict(o)).and_then(|p| self.text(p, b"NM")); - let fill_sign = self.is_fill_sign(d, &subtype); - // Keep in step with `pdfcraft_annot::summaries`. - let key: &[u8] = if subtype == "Line" { b"L" } else { b"Vertices" }; - let points = match d.get(key).map(|o| self.resolve(o)) { - Ok(Object::Array(v)) if matches!(subtype.as_str(), "Line" | "PolyLine" | "Polygon") => { - let v: Vec = v.iter().map(|o| o.as_float().unwrap_or(0.0)).collect(); - let pts: Vec<[f32; 2]> = v.as_chunks::<2>().0.to_vec(); - if subtype == "Line" && pts.len() != 2 { Vec::new() } else { pts } - } - _ => Vec::new(), - }; - let annotation = Annotation { - page, - subtype, - author: self.text(d, b"T"), - // Acrobat ends lines with CR, which the interface would draw as nothing (keep - // in step with `pdfcraft_annot::summaries`). - contents: self.text(d, b"Contents").map(|s| s.replace("\r\n", "\n").replace('\r', "\n")), - modified: self.text(d, b"M").map(|m| pretty_date(&m)), - name: self.text(d, b"NM"), - in_reply_to, - rect, - color, - index, - state: self.text(d, b"State"), - quads, - locked: d.get(b"F").ok().and_then(|f| self.resolve(f).as_i64().ok()).unwrap_or(0) & 128 != 0, - intent: self.name(d, b"IT"), - fill_sign, - points, - }; - kept = kept.saturating_add(retained(&annotation)); - if kept > MAX_ANNOTATION_BYTES { - comments_full = true; - continue; - } - info.annotations.push(annotation); + entries.push((page, index, a)); } } + let mut kept = 0usize; + // Once the comment budget is spent, comments are no longer built, but links and attachments + // are still listed. + let mut comments_full = false; + for (page, index, a) in entries { + let Some(d) = self.dict(a) else { continue }; + let Some(subtype) = self.name(d, b"Subtype") else { continue }; + if subtype == "FileAttachment" + && let Ok(fs) = d.get(b"FS") + { + let fallback = |budget: &mut usize| self.budgeted_text(d, b"Contents", budget); + info.attachments.push(self.attachment(fs, AttachmentSource::Annotation { page, index }, text_budget, fallback)); + } + if subtype == "Link" { + if let Some(link) = self.link(page, index, d, text_budget) { + if matches!(&link.target, LinkTarget::Other(s) if s == "JavaScript") { + info.has_javascript = true; + } + info.links.push(link); + } + continue; + } + // Not comments: form widgets, pop-ups and non-markup annotations such as the + // Screen annotation that drives a LaTeX `animate` player (keep in step with + // `pdfcraft_annot::is_comment_subtype`). + if matches!(subtype.as_str(), "Widget" | "Popup" | "Screen" | "Movie" | "RichMedia" | "3D" | "PrinterMark" | "TrapNet" | "Watermark") { + continue; + } + if comments_full { + continue; + } + let rect = rect4(self.resolve(d.get(b"Rect").unwrap_or(&Object::Null))); + let mut color = match d.get(b"C").map(|o| self.resolve(o)) { + Ok(Object::Array(c)) if c.len() == 3 => { + let f = |i: usize| c[i].as_float().unwrap_or(0.0); + Some([f(0), f(1), f(2)]) + } + _ => None, + }; + // A text box's /C is its background; its text colour is in /DA. + if subtype == "FreeText" + && let Some(da) = self.text(d, b"DA") + { + let t: Vec<&str> = da.split_whitespace().collect(); + if let Some(i) = t.iter().position(|x| *x == "rg") + && i >= 3 + { + let f = |k: usize| t[k].parse::().unwrap_or(0.0); + color = Some([f(i - 3), f(i - 2), f(i - 1)]); + } + } + let quads = match d.get(b"QuadPoints").map(|o| self.resolve(o)) { + Ok(Object::Array(q)) => { + let v: Vec = q.iter().map(|o| o.as_float().unwrap_or(0.0)).collect(); + v.as_chunks::<8>().0.to_vec() + } + _ => Vec::new(), + }; + let in_reply_to = d.get(b"IRT").ok().and_then(|o| self.dict(o)).and_then(|p| self.text(p, b"NM")); + let parent = d.get(b"IRT").ok().and_then(|o| self.resolved_id(o)).and_then(|r| slots.get(&r).copied()); + let fill_sign = self.is_fill_sign(d, &subtype); + // Keep in step with `pdfcraft_annot::summaries`. + let key: &[u8] = if subtype == "Line" { b"L" } else { b"Vertices" }; + let points = match d.get(key).map(|o| self.resolve(o)) { + Ok(Object::Array(v)) if matches!(subtype.as_str(), "Line" | "PolyLine" | "Polygon") => { + let v: Vec = v.iter().map(|o| o.as_float().unwrap_or(0.0)).collect(); + let pts: Vec<[f32; 2]> = v.as_chunks::<2>().0.to_vec(); + if subtype == "Line" && pts.len() != 2 { Vec::new() } else { pts } + } + _ => Vec::new(), + }; + let annotation = Annotation { + page, + subtype, + author: self.text(d, b"T"), + // Acrobat ends lines with CR, which the interface would draw as nothing (keep + // in step with `pdfcraft_annot::summaries`). + contents: self.text(d, b"Contents").map(|s| s.replace("\r\n", "\n").replace('\r', "\n")), + modified: self.text(d, b"M").map(|m| pretty_date(&m)), + name: self.text(d, b"NM"), + in_reply_to, + parent, + rect, + color, + index, + state: self.text(d, b"State"), + quads, + locked: d.get(b"F").ok().and_then(|f| self.resolve(f).as_i64().ok()).unwrap_or(0) & 128 != 0, + intent: self.name(d, b"IT"), + fill_sign, + points, + }; + kept = kept.saturating_add(retained(&annotation)); + if kept > MAX_ANNOTATION_BYTES { + comments_full = true; + continue; + } + info.annotations.push(annotation); + } info.annotations.sort_by(|a, b| a.page.cmp(&b.page).then(b.rect[3].total_cmp(&a.rect[3]))); } diff --git a/crates/ui-egui/src/canvas.rs b/crates/ui-egui/src/canvas.rs index 1af2a6a4f3..0c4104fe99 100644 --- a/crates/ui-egui/src/canvas.rs +++ b/crates/ui-egui/src/canvas.rs @@ -2271,7 +2271,7 @@ impl DocView { /// The hover popup for an annotation. Replies and Fill & Sign marks (typed text, checks, signatures) /// show none: they are page content being edited, not comments. fn annotation_hover_text(a: &pdfcraft_render::Annotation) -> Option { - if a.in_reply_to.is_some() || a.fill_sign { + if a.is_reply() || a.fill_sign { return None; } let who = a.author.clone().unwrap_or_else(|| a.subtype.clone()); @@ -5155,7 +5155,7 @@ fn quick_bar(app: &mut PdfCraftApp, area: Rect, ui: &mut egui::Ui) { .info .annotations .iter() - .any(|a| a.page == page && a.index == annotation && a.in_reply_to.is_none() && comments::made_by(a, tool)) + .any(|a| a.page == page && a.index == annotation && !a.is_reply() && comments::made_by(a, tool)) }) { app.apply_edit(Edit::StyleAnnotation { @@ -5737,6 +5737,7 @@ mod tests { modified: None, name: None, in_reply_to: None, + parent: None, rect: [0.0; 4], color: None, index: 0, diff --git a/crates/ui-egui/src/comment_props.rs b/crates/ui-egui/src/comment_props.rs index f66885d361..38fd2c4117 100644 --- a/crates/ui-egui/src/comment_props.rs +++ b/crates/ui-egui/src/comment_props.rs @@ -98,7 +98,7 @@ impl PdfCraftApp { pub fn make_props_default(&mut self, page: usize, index: usize, p: &CommentProps) { let Some((_, id)) = self.active_ids() else { return }; let Some(doc) = self.session.get(id) else { return }; - let Some(a) = doc.info.annotations.iter().find(|a| a.page == page && a.index == index && a.in_reply_to.is_none()) else { return }; + let Some(a) = doc.info.annotations.iter().find(|a| a.page == page && a.index == index && !a.is_reply()) else { return }; let Some(tool) = crate::comments::tool_for_default(a, p.endings.as_deref()) else { return }; let mut style = self.comment_prefs.style(tool); if let Some(c) = p.color { @@ -167,12 +167,11 @@ pub(crate) fn body(ui: &mut egui::Ui, app: &mut PdfCraftApp, t: &Tokens) -> (boo .and_then(|(_, id)| app.session.get(id)) .zip(app.comment_props.as_ref()) .map(|(doc, d)| { - let me = doc.info.annotations.iter().find(|a| a.page == d.page && a.index == d.index && a.in_reply_to.is_none()); - let nm = me.and_then(|a| a.name.clone()); + let me = doc.info.annotations.iter().find(|a| a.page == d.page && a.index == d.index && !a.is_reply()); doc.info .annotations .iter() - .filter(|r| r.state.is_some() && nm.is_some() && r.in_reply_to == nm) + .filter(|r| r.state.is_some() && me.is_some_and(|a| r.replies_to(a))) .map(|r| (r.state.clone().unwrap_or_default(), r.author.clone().unwrap_or_default(), r.modified.clone().unwrap_or_default())) .collect() }) @@ -180,7 +179,7 @@ pub(crate) fn body(ui: &mut egui::Ui, app: &mut PdfCraftApp, t: &Tokens) -> (boo let default_tool = app.comment_props.as_ref().and_then(|d| { let (_, id) = app.active_ids()?; let doc = app.session.get(id)?; - let a = doc.info.annotations.iter().find(|a| a.page == d.page && a.index == d.index && a.in_reply_to.is_none())?; + let a = doc.info.annotations.iter().find(|a| a.page == d.page && a.index == d.index && !a.is_reply())?; crate::comments::tool_for_default(a, d.edited.endings.as_deref()) }); let Some(d) = app.comment_props.as_mut() else { return (false, true) }; diff --git a/crates/ui-egui/src/comments.rs b/crates/ui-egui/src/comments.rs index c832c2c1a5..0ff3219788 100644 --- a/crates/ui-egui/src/comments.rs +++ b/crates/ui-egui/src/comments.rs @@ -624,7 +624,7 @@ impl PageCx<'_> { /// Top-level comments on this page, in paint order. fn comments(&self) -> impl Iterator { - self.info.annotations.iter().filter(move |a| !self.hidden && a.page == self.page && a.in_reply_to.is_none() && a.subtype != "Popup") + self.info.annotations.iter().filter(move |a| !self.hidden && a.page == self.page && !a.is_reply() && a.subtype != "Popup") } /// Where a comment is on screen: one rectangle, or one per marked line for text markup. @@ -1843,7 +1843,7 @@ pub(crate) fn attachment_items(ui: &mut egui::Ui, view: &mut DocView, info: &Doc /// Items of the canvas context menu for the selected comment (or the page). pub(crate) fn context_menu(ui: &mut egui::Ui, view: &mut DocView, info: &DocInfo, prefs: &CommentPrefs, allowed: bool) -> Option { let mut action = None; - let selected = view.comments.selected.and_then(|(p, i)| info.annotations.iter().find(|a| a.page == p && a.index == i && a.in_reply_to.is_none())); + let selected = view.comments.selected.and_then(|(p, i)| info.annotations.iter().find(|a| a.page == p && a.index == i && !a.is_reply())); match selected { Some(a) => { let (page, index) = (a.page, a.index); @@ -1888,7 +1888,7 @@ pub(crate) fn context_menu(ui: &mut egui::Ui, view: &mut DocView, info: &DocInfo } }); }); - let thread: Vec<&pdfcraft_render::Annotation> = info.annotations.iter().filter(|r| a.name.is_some() && r.in_reply_to == a.name).collect(); + let thread: Vec<&pdfcraft_render::Annotation> = info.annotations.iter().filter(|r| r.replies_to(a)).collect(); let marked = crate::comments_panel::is_marked(&thread); if ui.add_enabled(allowed, egui::Button::new(tl!(if marked { "Remove checkmark" } else { "Mark with checkmark" }))).clicked() { action = Some(CanvasAction::Edit(Box::new(Edit::MarkAnnotation { page, index, marked: !marked, author: prefs.author.clone() }))); @@ -2397,6 +2397,8 @@ mod tests { modified: None, name: None, in_reply_to: None, + parent: None, + grouped: false, rect: [10.0, 10.0, 50.0, 50.0], color: None, index: 0, diff --git a/crates/ui-egui/src/comments_panel.rs b/crates/ui-egui/src/comments_panel.rs index ff85704568..70f51db407 100644 --- a/crates/ui-egui/src/comments_panel.rs +++ b/crates/ui-egui/src/comments_panel.rs @@ -87,9 +87,10 @@ pub fn is_marked(thread: &[&Annotation]) -> bool { thread.iter().rev().find(|r| r.is_mark()).is_some_and(|r| r.state.as_deref() == Some("Marked")) } -/// The number shown in the header: comments and their replies (not status changes). +/// The number shown in the header: comments, not their replies or status changes (as Acrobat +/// counts them). pub fn count(info: &DocInfo) -> usize { - info.annotations.iter().filter(|a| a.state.is_none()).count() + info.annotations.iter().filter(|a| !a.is_reply() && a.state.is_none()).count() } fn matches(a: &Annotation, q: &str) -> bool { @@ -157,25 +158,28 @@ pub(crate) fn show( return edit; } let query = view.comments.search.clone().filter(|q| !q.trim().is_empty()); - let mut replies: HashMap<&str, Vec<&Annotation>> = HashMap::new(); + let mut replies: HashMap<(usize, usize), Vec<&Annotation>> = HashMap::new(); let mut page_counts: HashMap = HashMap::new(); - // Keep global name matching and source reply order. Page badges count roots before filtering. + // Replies find their parent by reference (`/NM` need not be unique) and keep source order. + // Page badges count roots before filtering. for a in &info.annotations { - if let Some(parent) = a.in_reply_to.as_deref() { - replies.entry(parent).or_default().push(a); + if a.is_reply() { + if let Some(parent) = a.parent { + replies.entry(parent).or_default().push(a); + } } else { let count = page_counts.entry(a.page).or_default(); *count = count.saturating_add(1); } } - let replies_of = |a: &Annotation| -> &[&Annotation] { a.name.as_deref().and_then(|name| replies.get(name)).map(Vec::as_slice).unwrap_or(&[]) }; + let replies_of = |a: &Annotation| -> &[&Annotation] { replies.get(&(a.page, a.index)).map(Vec::as_slice).unwrap_or(&[]) }; let cv = &view.comments; let status_of = |a: &Annotation| replies_of(a).iter().rev().filter(|r| !r.is_mark()).find_map(|r| r.state.clone()).unwrap_or_else(|| "None".into()); let mut roots: Vec<&Annotation> = info .annotations .iter() - .filter(|a| a.in_reply_to.is_none()) + .filter(|a| !a.is_reply()) .filter(|a| query.as_deref().is_none_or(|q| matches(a, q) || replies_of(a).iter().any(|r| matches(r, q)))) .filter(|a| !cv.hidden_types.iter().any(|t| t == kind_label(a))) .filter(|a| !cv.hidden_authors.contains(&a.author.clone().unwrap_or_default())) @@ -301,8 +305,9 @@ fn card( .show(ui, |ui| { ui.set_width(ui.available_width()); let mut buttons_left = f32::INFINITY; + let mut above = egui::Rect::NOTHING; let head = ui.horizontal(|ui| { - avatar(ui, t, Some(subtype_icon(&a.subtype)), None, color); + above = avatar(ui, t, Some(subtype_icon(&a.subtype)), None, color); let author = a.author.as_deref().map_or_else(|| tl!("Unknown author").into(), crate::bidi::visual_auto); ui.label(egui::RichText::new(author).font(theme::semibold(12.5)).color(t.text)); if let Some(m) = &a.modified { @@ -373,11 +378,13 @@ fn card( }); } }); - // Replies, joined to the parent by a thread line. + // Replies, each joined to the avatar above it (the comment's or the previous reply's) + // by a thread line that runs the whole gap. for r in &replies { ui.add_space(8.0); - let resp = ui.horizontal(|ui| { - avatar(ui, t, None, Some(&initials(r.author.as_deref().unwrap_or("?"))), t.text_faint); + let mut me = egui::Rect::NOTHING; + ui.horizontal(|ui| { + me = avatar(ui, t, None, Some(&initials(r.author.as_deref().unwrap_or("?"))), t.text_faint); ui.vertical(|ui| { ui.spacing_mut().item_spacing.y = 2.0; ui.horizontal(|ui| { @@ -390,8 +397,12 @@ fn card( widgets::wrapped_text(ui, r.contents.as_deref().unwrap_or(""), theme::regular(12.5), t.text, false); }); }); - let x = resp.response.rect.left() + 12.0; - ui.painter().vline(x, (resp.response.rect.top() - 14.0)..=(resp.response.rect.top() - 2.0), Stroke::new(1.5, t.border)); + // The circles are 12 px in radius; leave a 1 px gap at each end. + let (top, bottom) = (above.center().y + 13.0, me.center().y - 13.0); + if above.is_positive() && top < bottom { + ui.painter().vline(me.center().x, top..=bottom, Stroke::new(1.5, t.border)); + } + above = me; } if selected && allowed { ui.add_space(8.0); @@ -499,7 +510,8 @@ fn card( } /// A 26 pt round badge: a type glyph (comments) or initials (replies). -fn avatar(ui: &mut egui::Ui, t: &Tokens, icon: Option<&str>, text: Option<&str>, color: Color32) { +/// A round avatar; returns where it was drawn. +fn avatar(ui: &mut egui::Ui, t: &Tokens, icon: Option<&str>, text: Option<&str>, color: Color32) -> egui::Rect { let (r, _) = ui.allocate_exact_size(vec2(26.0, 26.0), Sense::hover()); let bg = if t.dark() { Color32::from_gray(0x55) } else { Color32::from_gray(0xD8) }; ui.painter().circle_filled(r.center(), 12.0, bg); @@ -509,6 +521,7 @@ fn avatar(ui: &mut egui::Ui, t: &Tokens, icon: Option<&str>, text: Option<&str>, if let Some(s) = text { ui.painter().text(r.center(), egui::Align2::CENTER_CENTER, s, theme::semibold(10.5), t.text); } + r } /// Darken very light annotation colours (e.g. note yellow) so their glyph stays legible. @@ -527,15 +540,13 @@ fn legible(c: Color32, t: &Tokens) -> Color32 { pub(crate) fn header_controls(ui: &mut egui::Ui, info: &DocInfo, view: &mut DocView, hidden: bool) -> Option<&'static str> { let mut command = None; let cv = &mut view.comments; - let mut types: Vec = info.annotations.iter().filter(|a| a.in_reply_to.is_none()).map(|a| kind_label(a).to_string()).collect(); + let mut types: Vec = info.annotations.iter().filter(|a| !a.is_reply()).map(|a| kind_label(a).to_string()).collect(); types.sort(); types.dedup(); - let mut authors: Vec = - info.annotations.iter().filter(|a| a.in_reply_to.is_none()).map(|a| a.author.clone().unwrap_or_default()).collect(); + let mut authors: Vec = info.annotations.iter().filter(|a| !a.is_reply()).map(|a| a.author.clone().unwrap_or_default()).collect(); authors.sort(); authors.dedup(); - let mut colors: Vec<(String, Option<[f32; 3]>)> = - info.annotations.iter().filter(|a| a.in_reply_to.is_none()).map(|a| (color_key(a), a.color)).collect(); + let mut colors: Vec<(String, Option<[f32; 3]>)> = info.annotations.iter().filter(|a| !a.is_reply()).map(|a| (color_key(a), a.color)).collect(); colors.sort_by(|a, b| a.0.cmp(&b.0)); colors.dedup_by(|a, b| a.0 == b.0); let active = !(cv.hidden_types.is_empty() diff --git a/crates/ui-egui/src/editing.rs b/crates/ui-egui/src/editing.rs index 2fdc2e9d02..1b036ba33e 100644 --- a/crates/ui-egui/src/editing.rs +++ b/crates/ui-egui/src/editing.rs @@ -155,7 +155,7 @@ impl PdfCraftApp { } Edit::AddAnnotation(a) => { // Select the new comment (appended last among the page's comments). - let newest = info.annotations.iter().filter(|x| x.page == a.page && x.in_reply_to.is_none()).map(|x| x.index).max(); + let newest = info.annotations.iter().filter(|x| x.page == a.page && !x.is_reply()).map(|x| x.index).max(); view.comments.selected = newest.map(|n| (a.page, n)); view.comments.reveal = true; // A new highlight opens its note for typing straight away, as in Acrobat, @@ -500,7 +500,7 @@ impl PdfCraftApp { let newest = self .session .get(view.id) - .and_then(|d| d.info.annotations.iter().filter(|a| a.page == page && a.in_reply_to.is_none()).map(|a| a.index).max()); + .and_then(|d| d.info.annotations.iter().filter(|a| a.page == page && !a.is_reply()).map(|a| a.index).max()); view.comments.selected = newest.map(|index| (page, index)); view.comments.reveal = true; self.quick_tool = crate::QuickTool::Select; diff --git a/crates/ui-egui/tests/it/comments.rs b/crates/ui-egui/tests/it/comments.rs index 6194c75163..ea22c98a04 100644 --- a/crates/ui-egui/tests/it/comments.rs +++ b/crates/ui-egui/tests/it/comments.rs @@ -1085,8 +1085,9 @@ fn the_panel_filters_and_sorts() { assert_eq!(h.query_all_by_label("Oval").count(), ovals + 1, "a group header was added"); } -// Contributor-original PDF. Two roots deliberately share /NM. Their replies are on -// another page, with dates opposite to source order. An unnamed root has no thread. +// Contributor-original PDF. Two roots deliberately share /NM; the replies (on another page, +// with dates opposite to source order) point at the first by reference. An unnamed root has +// no thread. const REVIEW_FIXTURE: &[u8] = b"%PDF-1.7 1 0 obj << /Type /Catalog /Pages 2 0 R >> endobj 2 0 obj << /Type /Pages /Kids [3 0 R 4 0 R] /Count 2 >> endobj @@ -1107,7 +1108,7 @@ trailer << /Root 1 0 R >> %%EOF"; #[test] -fn the_panel_preserves_global_threads_source_order_and_unfiltered_page_counts() { +fn the_panel_preserves_threads_source_order_and_unfiltered_page_counts() { let mut h = harness_with_bytes(REVIEW_FIXTURE.to_vec(), |app| { app.set_option("panel", "comments").unwrap(); app.views[0].comments.search = Some("needle".into()); @@ -1129,14 +1130,16 @@ fn the_panel_preserves_global_threads_source_order_and_unfiltered_page_counts() h.state_mut().views[0].comments.hidden_authors.clear(); h.run_steps(2); - h.get_by_label("Duplicate root"); - assert_eq!(h.query_all_by_label("Needle first reply").count(), 2, "duplicate names share the same global thread"); + // The replies point at the first root: sharing its /NM doesn't give the other its thread + // (or a search match on that thread). + assert_eq!(h.query_all_by_label("Duplicate root").count(), 0); + assert_eq!(h.query_all_by_label("Needle first reply").count(), 1); h.state_mut().views[0].comments.search = Some("status-only token".into()); h.state_mut().views[0].comments.hidden_statuses.push("Rejected".into()); h.state_mut().views[0].comments.hidden_checks.push("Checked".into()); h.run_steps(2); h.get_by_label("First root"); - h.get_by_label("Duplicate root"); + assert_eq!(h.query_all_by_label("Duplicate root").count(), 0); h.state_mut().views[0].comments.hidden_statuses.push("Accepted".into()); h.run_steps(2); @@ -1153,6 +1156,31 @@ fn the_panel_preserves_global_threads_source_order_and_unfiltered_page_counts() assert_eq!(h.query_all_by_label("Needle first reply").count(), 0, "an unnamed root has no replies"); } +// Contributor-original PDF, shaped like PyMuPDF output: every page's first comment is named +// `fitz-A0`, so only the `/IRT` reference tells which comment a reply belongs to. +const PER_PAGE_NAMES: &[u8] = b"%PDF-1.7 +1 0 obj << /Type /Catalog /Pages 2 0 R >> endobj +2 0 obj << /Type /Pages /Kids [3 0 R 4 0 R] /Count 2 >> endobj +3 0 obj << /Type /Page /Parent 2 0 R /MediaBox [0 0 300 200] /Annots [5 0 R] >> endobj +4 0 obj << /Type /Page /Parent 2 0 R /MediaBox [0 0 300 200] /Annots [6 0 R 7 0 R 8 0 R] >> endobj +5 0 obj << /Type /Annot /Subtype /Text /Rect [20 140 40 160] /NM (fitz-A0) /T (Alpha) /Contents (Cover note) >> endobj +6 0 obj << /Type /Annot /Subtype /Text /Rect [20 140 40 160] /NM (fitz-A0) /T (Beta) /Contents (Second page note) >> endobj +7 0 obj << /Type /Annot /Subtype /Text /Rect [20 140 40 160] /NM (fitz-A1) /IRT 6 0 R /T (Gamma) /Contents (Reply on page two) >> endobj +8 0 obj << /Type /Annot /Subtype /Text /Rect [20 140 40 160] /NM (fitz-A2) /IRT 6 0 R /T (Beta) /Contents (Answer on page two) >> endobj +trailer << /Root 1 0 R >> +%%EOF"; + +#[test] +fn replies_follow_irt_references_when_names_repeat_on_every_page() { + let h = harness_with_bytes(PER_PAGE_NAMES.to_vec(), |app| app.set_option("panel", "comments").unwrap()); + let reply = h.get_by_label("Reply on page two"); + assert!(reply.rect().top() > h.get_by_label("Second page note").rect().top(), "the reply sits under page 2's comment"); + assert!(h.get_by_label("Page 2").rect().top() < reply.rect().top(), "not under page 1's comment, which shares the /NM"); + h.get_by_label("Answer on page two"); + // The header counts the two comments, not their replies (Acrobat says 2, not 4). + assert_eq!(h.query_all_by_label("4").count(), 0); +} + #[test] fn the_panel_reveals_edits_and_reopens_a_late_thread_without_changing_its_replies() { let mut h = harness_with_bytes(REVIEW_FIXTURE.to_vec(), |app| { diff --git a/parity/acrobat-features.toml b/parity/acrobat-features.toml index 4af0cd98c2..69c02f04e4 100644 --- a/parity/acrobat-features.toml +++ b/parity/acrobat-features.toml @@ -3560,8 +3560,8 @@ milestone = "M3" status = "shipped" commands = ["comment.list"] tools = ["comment_list"] -evidence = ["crates/ui-egui/tests/it/ui.rs::opening_a_pdf_shows_comments_and_bookmarks", "crates/ui-egui/tests/it/comments.rs::the_panel_posts_comments_and_replies", "crates/ui-egui/tests/it/comments.rs::page_groups_collapse_and_expand"] -notes = "cards grouped by page with replies, status badges, selection, reply box and an Add a comment box; page groups fold and unfold from their chevron or with Collapse/Expand all pages in the panel's \"…\" menu (a comment selected on a folded page opens its group). Groups by author, type or colour don't fold yet" +evidence = ["crates/ui-egui/tests/it/ui.rs::opening_a_pdf_shows_comments_and_bookmarks", "crates/ui-egui/tests/it/comments.rs::the_panel_posts_comments_and_replies", "crates/ui-egui/tests/it/comments.rs::page_groups_collapse_and_expand", "crates/ui-egui/tests/it/comments.rs::replies_follow_irt_references_when_names_repeat_on_every_page", "crates/engine/src/tests.rs::replies_thread_by_reference_when_names_repeat_across_pages"] +notes = "cards grouped by page with replies (threaded by the `/IRT` reference, so files whose `/NM` repeats per page, as PyMuPDF writes them, thread correctly; the header counts comments without replies, as Acrobat does), status badges, selection, reply box and an Add a comment box; page groups fold and unfold from their chevron or with Collapse/Expand all pages in the panel's \"…\" menu (a comment selected on a folded page opens its group). Groups by author, type or colour don't fold yet" [[feature]] id = "comment.appearance-generation"