From ecb3f6527e830520e93e763f6184bc34d7a70ad7 Mon Sep 17 00:00:00 2001 From: alehel Date: Thu, 22 Oct 2026 14:45:23 +0200 Subject: [PATCH] fix(automation): comment_list reads a status or checkmark from the annotation it was set on MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A status or checkmark is a state reply whose /IRT names the annotation it applies to (ISO 32000-2 §12.5.6.2). comment_list gathered state replies at any depth of a thread (since #839 listed nested replies), so a checkmark or status set on a reply was reported as the comment's own, while the Comments panel, comment properties and the page's context menu showed the comment unmarked. comment_list now files them under the annotation they were set on, as the panel does. A UI regression pins the card and the page's menu (it also fails if the menu counts checkmarks at any depth), and an automation one pins comment_list. Co-Authored-By: Claude Opus 5.5 --- ROADMAP.md | 2 +- crates/automation/src/comments.rs | 13 ++++++--- crates/automation/tests/it/automation.rs | 29 +++++++++++++++++++ crates/ui-egui/tests/it/comments.rs | 36 ++++++++++++++++++++++++ 4 files changed, 75 insertions(+), 5 deletions(-) diff --git a/ROADMAP.md b/ROADMAP.md index 20613dc466..f7b6fb3ff3 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -199,7 +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) with the replies in small text under it ("16 replies", in every catalogue), and a reply's thread line runs from the avatar above it to its own. A reply to a reply shows in its thread in the panel too (it was listed nowhere), an `/IRT` that leads to no comment (itself, a loop, a pop-up, an unlisted object) leaves its annotation a comment of its own instead of hiding it, and a Replace Text caret isn't counted as a reply. The comment tools refuse an `id` several comments share instead of acting on the first (page 1's), and take `page` + `index` for those. UI, engine and automation regressions cover it. 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) with the replies in small text under it ("16 replies", in every catalogue), and a reply's thread line runs from the avatar above it to its own. A reply to a reply shows in its thread in the panel too (it was listed nowhere), an `/IRT` that leads to no comment (itself, a loop, a pop-up, an unlisted object) leaves its annotation a comment of its own instead of hiding it, and a Replace Text caret isn't counted as a reply. The comment tools refuse an `id` several comments share instead of acting on the first (page 1's), and take `page` + `index` for those; `comment_list` reads a status or checkmark from the annotation it was set on, as the panel does. 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/automation/src/comments.rs b/crates/automation/src/comments.rs index 87e6ae4f9b..05e2b361ee 100644 --- a/crates/automation/src/comments.rs +++ b/crates/automation/src/comments.rs @@ -207,13 +207,18 @@ impl Automation { }; // Every reply under the comment its thread starts from, 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. Parents are found by reference, as `/NM` need not be unique. + // drop it. Parents are found by reference, as `/NM` need not be unique. A status or + // checkmark belongs to the annotation its `/IRT` names (ISO 32000-2 §12.5.6.2), as in the + // Comments panel: one set on a reply is not the comment's. let tops = pdfcraft_render::thread_tops(&all); let mut threads: HashMap<(usize, usize), Vec<&Annotation>> = HashMap::new(); for r in &all { - if let Some(&top) = tops.get(&(r.page, r.index)) { - threads.entry(top).or_default().push(r); - } + let Some(&top) = tops.get(&(r.page, r.index)) else { continue }; + let key = match r.parent { + Some(parent) if r.state.is_some() => parent, + _ => top, + }; + threads.entry(key).or_default().push(r); } let roots: Vec = all .iter() diff --git a/crates/automation/tests/it/automation.rs b/crates/automation/tests/it/automation.rs index d9c004b130..3c725de9df 100644 --- a/crates/automation/tests/it/automation.rs +++ b/crates/automation/tests/it/automation.rs @@ -2121,6 +2121,35 @@ trailer << /Root 1 0 R >> assert_eq!(replies(&mut a), [("COVER".into(), vec![]), ("SECOND".into(), vec!["ANSWER".to_string(), "LATER".to_string()])]); } +/// A status or checkmark belongs to the annotation its `/IRT` names: one set on a reply is not +/// its comment's, as the Comments panel shows it. +#[test] +fn a_checkmark_on_a_reply_is_not_its_comments() { + let dir = workdir("reply-checkmark"); + // Contributor-original PDF: a comment, a reply, and a checkmark and a status on the reply. + std::fs::write( + dir.join("checked.pdf"), + b"%PDF-1.7 +1 0 obj << /Type /Catalog /Pages 2 0 R >> endobj +2 0 obj << /Type /Pages /Kids [3 0 R] /Count 1 >> endobj +3 0 obj << /Type /Page /Parent 2 0 R /MediaBox [0 0 300 200] /Annots [4 0 R 5 0 R 6 0 R 7 0 R] >> endobj +4 0 obj << /Type /Annot /Subtype /Text /Rect [100 100 120 120] /T (Alpha) /Contents (COMMENT) >> endobj +5 0 obj << /Type /Annot /Subtype /Text /Rect [100 60 120 80] /IRT 4 0 R /T (Beta) /Contents (REPLY) >> endobj +6 0 obj << /Type /Annot /Subtype /Text /Rect [100 60 120 80] /IRT 5 0 R /T (Beta) /StateModel /Marked /State /Marked >> endobj +7 0 obj << /Type /Annot /Subtype /Text /Rect [100 60 120 80] /IRT 5 0 R /T (Beta) /StateModel /Review /State /Accepted >> endobj +trailer << /Root 1 0 R >> +%%EOF", + ) + .unwrap(); + let mut a = auto(&dir); + let doc = ok(&mut a, "doc_open", json!({ "path": "checked.pdf" }))["doc"].as_u64().unwrap(); + let list = ok(&mut a, "comment_list", json!({ "doc": doc })); + assert_eq!(list["count"], 1, "{list}"); + let c = &list["comments"][0]; + assert_eq!((&c["marked"], &c["status"]), (&json!(false), &Value::Null), "{c}"); + assert_eq!(c["replies"].as_array().map(Vec::len), Some(1), "the reply, not its states: {c}"); +} + #[test] fn protecting_through_tools() { let dir = workdir("protect"); diff --git a/crates/ui-egui/tests/it/comments.rs b/crates/ui-egui/tests/it/comments.rs index 13e728c5ed..f7b8159b9b 100644 --- a/crates/ui-egui/tests/it/comments.rs +++ b/crates/ui-egui/tests/it/comments.rs @@ -1245,6 +1245,42 @@ fn replies_that_lead_nowhere_stay_comments_and_deeper_replies_join_their_thread( h.get_by_label("2 replies"); } +// Contributor-original PDF: a comment, a reply, and a checkmark and a status set on the reply. +const CHECKED_REPLY: &[u8] = b"%PDF-1.7 +1 0 obj << /Type /Catalog /Pages 2 0 R >> endobj +2 0 obj << /Type /Pages /Kids [3 0 R] /Count 1 >> endobj +3 0 obj << /Type /Page /Parent 2 0 R /MediaBox [0 0 300 200] /Annots [4 0 R 5 0 R 6 0 R 7 0 R] >> endobj +4 0 obj << /Type /Annot /Subtype /Text /Rect [100 100 120 120] /T (Alpha) /Contents (Unchecked comment) >> endobj +5 0 obj << /Type /Annot /Subtype /Text /Rect [100 60 120 80] /IRT 4 0 R /T (Beta) /Contents (Checked reply) >> endobj +6 0 obj << /Type /Annot /Subtype /Text /Rect [100 60 120 80] /IRT 5 0 R /T (Beta) /StateModel /Marked /State /Marked >> endobj +7 0 obj << /Type /Annot /Subtype /Text /Rect [100 60 120 80] /IRT 5 0 R /T (Beta) /StateModel /Review /State /Accepted >> endobj +trailer << /Root 1 0 R >> +%%EOF"; + +/// A checkmark or status belongs to the annotation its `/IRT` names (ISO 32000-2 §12.5.6.2): one +/// set on a reply doesn't mark the comment, on its card or in the page's menu. +#[test] +fn a_checkmark_on_a_reply_does_not_mark_its_comment() { + let mut h = harness_with_bytes(CHECKED_REPLY.to_vec(), |app| { + app.set_option("panel", "comments").unwrap(); + app.set_option("quick", "select").unwrap(); + }); + click_pt(&mut h, (110.0, 110.0)); + assert_eq!(h.state().views[0].comments.selected, Some((0, 0)), "the comment is selected"); + h.get_by_label("Checked reply"); + h.get_by_label("Mark with checkmark"); // The card's checkmark. + assert_eq!(h.query_all_by_label("Accepted").count(), 0, "no status badge"); + let p = at(&h, 110.0, 110.0); + h.hover_at(p); + h.run_steps(1); + h.event(egui::Event::PointerButton { pos: p, button: egui::PointerButton::Secondary, pressed: true, modifiers: Default::default() }); + h.event(egui::Event::PointerButton { pos: p, button: egui::PointerButton::Secondary, pressed: false, modifiers: Default::default() }); + h.run_steps(3); + h.get_by_label("Copy text"); // The page's menu is open. + assert_eq!(h.query_all_by_label("Remove checkmark").count(), 0); + assert_eq!(h.query_all_by_label("Mark with checkmark").count(), 2, "the card and the page's menu"); +} + #[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| {