From f35ec52fda1f4d9fcef391f613ecb2683a6b2542 Mon Sep 17 00:00:00 2001 From: alehel Date: Thu, 22 Oct 2026 14:58:19 +0200 Subject: [PATCH] fix(automation): comment_list reports a reply's own status and checkmark The previous commit filed a status or checkmark under the annotation it was set on, but comment_list only reported states for comments, so one set on a reply dropped out of the listing. Each reply in `replies` now carries its own `status` and `marked`, read the same way as a comment's, and the tool description says so. Co-Authored-By: Claude Opus 5.5 --- ROADMAP.md | 2 +- crates/automation/src/comments.rs | 22 +++++++++++++++++----- crates/automation/src/tools.rs | 2 +- crates/automation/tests/it/automation.rs | 4 +++- 4 files changed, 22 insertions(+), 8 deletions(-) diff --git a/ROADMAP.md b/ROADMAP.md index f7b6fb3ff3..47f3351b06 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; `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 (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, and reports a reply's own on the reply. 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 05e2b361ee..0ebf622e9d 100644 --- a/crates/automation/src/comments.rs +++ b/crates/automation/src/comments.rs @@ -220,17 +220,29 @@ impl Automation { }; threads.entry(key).or_default().push(r); } + // An annotation's review status and checkmark: the latest of each set on it. + let states = |c: &Annotation, v: &mut Value| { + let set = threads.get(&(c.page, c.index)).map(Vec::as_slice).unwrap_or(&[]); + v["status"] = json!(set.iter().rev().filter(|r| !r.is_mark()).find_map(|r| r.state.clone())); + v["marked"] = json!(set.iter().rev().find(|r| r.is_mark()).is_some_and(|r| r.state.as_deref() == Some("Marked"))); + }; let roots: Vec = all .iter() .filter(|c| !c.is_reply() && only.is_none_or(|p| c.page == p)) .map(|c| { let mut v = view(c); - let thread = threads.get(&(c.page, c.index)).map(Vec::as_slice).unwrap_or(&[]); - 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"))); + states(c, &mut v); v["locked"] = json!(c.locked); - v["replies"] = thread.iter().filter(|r| r.state.is_none()).map(|r| view(r)).collect(); + let thread = threads.get(&(c.page, c.index)).map(Vec::as_slice).unwrap_or(&[]); + v["replies"] = thread + .iter() + .filter(|r| r.state.is_none()) + .map(|r| { + let mut reply = view(r); + states(r, &mut reply); + reply + }) + .collect(); v }) .collect(); diff --git a/crates/automation/src/tools.rs b/crates/automation/src/tools.rs index 9af4eb5b7e..7844d3a4dd 100644 --- a/crates/automation/src/tools.rs +++ b/crates/automation/src/tools.rs @@ -681,7 +681,7 @@ pub fn tools() -> Vec { .ro().cmd("measure.snap").with(schema(json!({"doc":doc(),"page":{"type":"integer","minimum":1},"at":point(),"tolerance":{"type":"number","minimum":0,"maximum":10000},"endpoints":{"type":"boolean"},"midpoints":{"type":"boolean"},"intersections":{"type":"boolean"},"paths":{"type":"boolean"}}), &["doc","page","at"])), t("measure_export", "Export measurements as CSV", "Atomically write saved measurement values, labels, authors and scale ratios as spreadsheet-safe CSV. Returns how many unsupported measurements were left out.") .cmd("measure.export").with(schema(json!({"doc":doc(),"out":path_arg()}), &["doc","out"])), - t("comment_list", "List comments", "Every comment (annotation other than links, form widgets and pop-ups) with its page, index, id, type, author, text, date, rectangle, colour, review status and replies.") + t("comment_list", "List comments", "Every comment (annotation other than links, form widgets and pop-ups) with its page, index, id, type, author, text, date, rectangle, colour, review status, checkmark and replies (each with its own status and checkmark).") .ro() .with(schema(json!({ "doc": doc(), "page": { "type": "integer", "minimum": 1, "description": "Only this page." } }), &["doc"])), t( diff --git a/crates/automation/tests/it/automation.rs b/crates/automation/tests/it/automation.rs index 3c725de9df..d5a06d3506 100644 --- a/crates/automation/tests/it/automation.rs +++ b/crates/automation/tests/it/automation.rs @@ -2122,7 +2122,7 @@ trailer << /Root 1 0 R >> } /// 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. +/// its comment's, as the Comments panel shows it, and is reported on the reply. #[test] fn a_checkmark_on_a_reply_is_not_its_comments() { let dir = workdir("reply-checkmark"); @@ -2148,6 +2148,8 @@ trailer << /Root 1 0 R >> 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}"); + let reply = &c["replies"][0]; + assert_eq!((&reply["marked"], &reply["status"]), (&json!(true), &json!("Accepted")), "{reply}"); } #[test]