mirror of
https://github.com/storytold/pdfcraft.git
synced 2026-11-12 00:46:57 -06:00
fix(automation): comment_list reads a status or checkmark from the annotation it was set on
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 <noreply@anthropic.com>
This commit is contained in:
alehel
co-authored by
Claude Opus 5.5
Claude Opus 5.5
parent
a629835fec
commit
ecb3f6527e
+1
-1
@@ -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.
|
||||
|
||||
@@ -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<Value> = all
|
||||
.iter()
|
||||
|
||||
@@ -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");
|
||||
|
||||
@@ -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| {
|
||||
|
||||
Reference in New Issue
Block a user