M5: quick-bar colour recolours the selected comment (#1191) (#1255)

* Fix selected comment quick-bar color (#1191)

* M5: skip quick-bar recolour when commenting is forbidden (#1191)

The quick-bar colour swatch now checks allows_annotation() before
restyling the selected comment, matching the canvas and context-menu
gating. Without it, a restricted document showed a "Change comment
properties failed" toast on every swatch pick; the engine already
rejected the edit. Adds a regression test on an encrypted, print-only
fixture that fails without the guard.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01T3DWkcKZcZLbww1oeDbCfb

* M5: quick-bar colour also recolours a selected arrow (#1191)

Arrows are saved as Line annotations with an ending, so tool_for()
reports them as the Line tool and the Arrow tool's swatch never matched
the selected arrow. comments::made_by() treats the Line and Arrow tools
as the same kind. Adds a regression test that fails without the fix.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01T3DWkcKZcZLbww1oeDbCfb

---------

Co-authored-by: Claude <noreply@anthropic.com>
This commit is contained in:
Raj Patel 2026-10-11 23:32:40 -06:00 committed by GitHub
parent f9bec964cb
commit d0975b974d
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
5 changed files with 131 additions and 4 deletions

View File

@ -141,6 +141,7 @@ Ranked; detail and the full milestone table in [docs/roadmap.md](docs/roadmap.md
- **2026-10-12 (M14, gap 13, large-file memory):** Measured on synthetic 1 GiB and 100,000-page files. Edits let go of the previous working file (working bytes, display bytes, renderer) before writing the next, so a 1 GiB file peaks at two copies while editing instead of three (five edits: 3.1 → 2.1 GiB, 16 → 8.5 s). Saving still peaks at three; reading from disk on demand is blocked on the renderer (gap 2). Estimates unchanged. - **2026-10-12 (M14, gap 13, large-file memory):** Measured on synthetic 1 GiB and 100,000-page files. Edits let go of the previous working file (working bytes, display bytes, renderer) before writing the next, so a 1 GiB file peaks at two copies while editing instead of three (five edits: 3.1 → 2.1 GiB, 16 → 8.5 s). Saving still peaks at three; reading from disk on demand is blocked on the renderer (gap 2). Estimates unchanged.
- **2026-10-12 (M2, short text redraws):** Same-line redraws that restart a matching three-glyph prefix after more than one glyph-height of progress split before placement, so short doubled words no longer interleave. The existing redraw-position tolerance is reused; controls cover negative TJ movement, combining marks, RTL, CJK, vertical and turned text. Estimates unchanged. - **2026-10-12 (M2, short text redraws):** Same-line redraws that restart a matching three-glyph prefix after more than one glyph-height of progress split before placement, so short doubled words no longer interleave. The existing redraw-position tolerance is reused; controls cover negative TJ movement, combining marks, RTL, CJK, vertical and turned text. Estimates unchanged.
- **2026-10-11 (M5, #1191):** Quick-bar colour changes now restyle the selected comment when it matches the active comment tool, while continuing to update that tool's default; added an accessible label to the swatch controls and a UI regression proving the saved style and undo step. Estimates unchanged.
- **2026-10-11 (M10.5, focused follow-up to #599):** Carry the architectural/ANSI/large ISO paper presets, current-page matching at 100%, and previewed PDF scale into the current print dialog and `doc_print`. Retains the #847/#1007 spooler and background discovery. Driver substitution, physical calibration, vector Windows printing and managed-endpoint restrictions remain separate gaps. Overall estimates unchanged. - **2026-10-11 (M10.5, focused follow-up to #599):** Carry the architectural/ANSI/large ISO paper presets, current-page matching at 100%, and previewed PDF scale into the current print dialog and `doc_print`. Retains the #847/#1007 spooler and background discovery. Driver substitution, physical calibration, vector Windows printing and managed-endpoint restrictions remain separate gaps. Overall estimates unchanged.
- **2026-10-11 (contributor fork, M7):** Full matching font import completes source-subset glyphs; installed/file font selection, TrueType/OpenType-CFF mapping, disjoint range styles, marked paragraph boundaries and single-face contextual shaping/bidi added. Private Chinese keyboard edits on all four pages preserve source anchors and save/reopen renders. Curved clips exclude holes from hit tests; shared path scopes are isolated for image transforms without moving neighbouring paint. No font binary or private fixture is published; release font inputs and readiness estimates unchanged. - **2026-10-11 (contributor fork, M7):** Full matching font import completes source-subset glyphs; installed/file font selection, TrueType/OpenType-CFF mapping, disjoint range styles, marked paragraph boundaries and single-face contextual shaping/bidi added. Private Chinese keyboard edits on all four pages preserve source anchors and save/reopen renders. Curved clips exclude holes from hit tests; shared path scopes are isolated for image transforms without moving neighbouring paint. No font binary or private fixture is published; release font inputs and readiness estimates unchanged.

View File

@ -3872,7 +3872,27 @@ fn quick_bar(app: &mut PdfCraftApp, area: Rect, ui: &mut egui::Ui) {
if let QuickTool::Comment(tool) = app.quick_tool { if let QuickTool::Comment(tool) = app.quick_tool {
let (r, _) = ui.allocate_exact_size(vec2(32.0, 9.0), Sense::hover()); let (r, _) = ui.allocate_exact_size(vec2(32.0, 9.0), Sense::hover());
ui.painter().hline(r.x_range().shrink(6.0), r.center().y, Stroke::new(1.0, t.divider)); ui.painter().hline(r.x_range().shrink(6.0), r.center().y, Stroke::new(1.0, t.divider));
comments::quick_bar_controls(ui, tool, &mut app.comment_prefs); if let Some(color) = comments::quick_bar_controls(ui, tool, &mut app.comment_prefs)
&& let Some((index, id)) = app.active_ids()
&& let Some((page, annotation)) = app.views.get(index).and_then(|view| view.comments.selected)
&& app.session.get(id).is_some_and(|doc| {
doc.allows_annotation()
&& doc
.info
.annotations
.iter()
.any(|a| a.page == page && a.index == annotation && a.in_reply_to.is_none() && comments::made_by(a, tool))
})
{
app.apply_edit(Edit::StyleAnnotation {
page,
index: annotation,
color: Some(color),
opacity: None,
width: None,
endings: None,
});
}
} }
}); });
}); });

View File

@ -306,6 +306,15 @@ impl CommentPrefs {
} }
} }
/// Whether `tool` makes comments like `a`. Arrows are saved as plain Line annotations with an
/// ending, so the Line and Arrow tools both match a line.
pub(crate) fn made_by(a: &Annotation, tool: CommentTool) -> bool {
match tool_for(a) {
Some(CommentTool::Line) => matches!(tool, CommentTool::Line | CommentTool::Arrow),
made => made == Some(tool),
}
}
/// The tool that makes comments like `a` (for Make Current Properties Default). /// The tool that makes comments like `a` (for Make Current Properties Default).
pub fn tool_for(a: &Annotation) -> Option<CommentTool> { pub fn tool_for(a: &Annotation) -> Option<CommentTool> {
Some(match (a.subtype.as_str(), a.intent.as_deref()) { Some(match (a.subtype.as_str(), a.intent.as_deref()) {
@ -1430,6 +1439,7 @@ pub fn swatch_grid(ui: &mut egui::Ui, current: Option<Rgb>) -> Option<Rgb> {
egui::Grid::new(ui.id().with("swatches")).spacing(vec2(6.0, 6.0)).show(ui, |ui| { egui::Grid::new(ui.id().with("swatches")).spacing(vec2(6.0, 6.0)).show(ui, |ui| {
for (i, (name, c)) in SWATCHES.iter().enumerate() { for (i, (name, c)) in SWATCHES.iter().enumerate() {
let (r, resp) = ui.allocate_exact_size(vec2(22.0, 22.0), Sense::click()); let (r, resp) = ui.allocate_exact_size(vec2(22.0, 22.0), Sense::click());
resp.widget_info(|| egui::WidgetInfo::labeled(egui::WidgetType::Button, true, tl!(name)));
let on = current.is_some_and(|cur| cur.iter().zip(c).all(|(a, b)| (a - b).abs() < 0.02)); let on = current.is_some_and(|cur| cur.iter().zip(c).all(|(a, b)| (a - b).abs() < 0.02));
ui.painter().circle_filled(r.center(), 9.0, color32(*c)); ui.painter().circle_filled(r.center(), 9.0, color32(*c));
ui.painter().circle_stroke(r.center(), 9.0, Stroke::new(1.0, Color32::from_black_alpha(40))); ui.painter().circle_stroke(r.center(), 9.0, Stroke::new(1.0, Color32::from_black_alpha(40)));
@ -1464,18 +1474,21 @@ pub fn fill_picker(ui: &mut egui::Ui, current: Option<Rgb>) -> Option<Option<Rgb
} }
/// The comment tools' extra quick-bar controls: pin, colour, fill, opacity and thickness. /// The comment tools' extra quick-bar controls: pin, colour, fill, opacity and thickness.
pub(crate) fn quick_bar_controls(ui: &mut egui::Ui, tool: CommentTool, prefs: &mut CommentPrefs) { pub(crate) fn quick_bar_controls(ui: &mut egui::Ui, tool: CommentTool, prefs: &mut CommentPrefs) -> Option<Rgb> {
let style = prefs.style(tool); let style = prefs.style(tool);
let mut selected_color = None;
if icons::button(ui, "pin", 32.0, prefs.pinned, if prefs.pinned { tl!("Keep tool selected: on") } else { tl!("Keep tool selected") }).clicked() { if icons::button(ui, "pin", 32.0, prefs.pinned, if prefs.pinned { tl!("Keep tool selected: on") } else { tl!("Keep tool selected") }).clicked() {
prefs.pinned = !prefs.pinned; prefs.pinned = !prefs.pinned;
} }
let (r, resp) = ui.allocate_exact_size(vec2(32.0, 32.0), Sense::click()); let (r, resp) = ui.allocate_exact_size(vec2(32.0, 32.0), Sense::click());
ui.painter().circle_filled(r.center(), 9.0, color32(style.color)); ui.painter().circle_filled(r.center(), 9.0, color32(style.color));
ui.painter().circle_stroke(r.center(), 9.0, Stroke::new(1.0, Color32::from_black_alpha(50))); ui.painter().circle_stroke(r.center(), 9.0, Stroke::new(1.0, Color32::from_black_alpha(50)));
resp.widget_info(|| egui::WidgetInfo::labeled(egui::WidgetType::Button, true, tl!("Colour")));
let resp = resp.on_hover_text(tl!("Colour")); let resp = resp.on_hover_text(tl!("Colour"));
egui::Popup::menu(&resp).align(egui::RectAlign::RIGHT_START).show(|ui| { egui::Popup::menu(&resp).align(egui::RectAlign::RIGHT_START).show(|ui| {
if let Some(c) = swatch_grid(ui, Some(style.color)) { if let Some(c) = swatch_grid(ui, Some(style.color)) {
prefs.set_color(tool, c); prefs.set_color(tool, c);
selected_color = Some(c);
ui.close(); ui.close();
} }
}); });
@ -1509,6 +1522,7 @@ pub(crate) fn quick_bar_controls(ui: &mut egui::Ui, tool: CommentTool, prefs: &m
} }
}); });
} }
selected_color
} }
/// The quick bar's fill button: a filled square in the fill colour, or a crossed-out outline /// The quick bar's fill button: a filled square in the fill colour, or a crossed-out outline

View File

@ -116,6 +116,98 @@ fn dragging_over_text_with_the_highlighter_highlights_it() {
assert_eq!(h.state().session.get(h.state().views[0].id).unwrap().can_undo(), Some("Add highlight")); assert_eq!(h.state().session.get(h.state().views[0].id).unwrap().can_undo(), Some("Add highlight"));
} }
#[test]
fn the_quick_bar_colour_changes_the_selected_comment() {
use pdfcraft_ui_egui::comments::CommentTool;
let mut h = harness(|app| app.set_option("quick", "highlight").unwrap());
let (a, b) = {
let v = &h.state().views[0];
(v.glyph_screen_pos(0, 4).expect("text layer"), v.glyph_screen_pos(0, 14).expect("glyph"))
};
drag(&mut h, a, b);
assert_eq!(comments(&h)[0].color, Some([1.0, 0.94, 0.0]));
h.get_by_label("Colour").click();
h.run_steps(2);
h.get_by_label("Blue").click();
h.run_steps(3);
let state = h.state();
let doc = state.session.get(state.views[0].id).expect("document");
assert_eq!(doc.comment_props(0, 0).and_then(|props| props.color), Some([0.0, 0.47, 0.84]));
assert_eq!(doc.can_undo(), Some("Change comment properties"), "recoloring is one undo step");
assert_eq!(state.comment_prefs.style(CommentTool::Highlight).color, [0.0, 0.47, 0.84]);
}
#[test]
fn the_quick_bar_colour_changes_the_selected_arrow() {
use pdfcraft_ui_egui::comments::CommentTool;
// Arrows are saved as Line annotations; the Arrow tool's colour must still reach them.
let mut h = harness(|app| app.set_option("quick", "arrow").unwrap());
drag_pt(&mut h, (20.0, 20.0), (120.0, 30.0));
assert_eq!(comments(&h)[0].subtype, "Line");
assert_eq!(h.state().quick_tool, QuickTool::Comment(CommentTool::Arrow), "the arrow tool is still active");
assert_eq!(h.state().views[0].comments.selected, Some((0, 0)), "the new arrow is selected");
h.get_by_label("Colour").click();
h.run_steps(2);
h.get_by_label("Blue").click();
h.run_steps(3);
let state = h.state();
let doc = state.session.get(state.views[0].id).expect("document");
assert_eq!(doc.comment_props(0, 0).and_then(|props| props.color), Some([0.0, 0.47, 0.84]));
assert_eq!(doc.can_undo(), Some("Change comment properties"));
}
#[test]
fn the_quick_bar_colour_skips_the_selected_comment_when_commenting_is_forbidden() {
use pdfcraft_ui_egui::comments::CommentTool;
// One page with a yellow highlight, encrypted to allow printing only (no commenting).
let plain = 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] >> endobj
4 0 obj << /Type /Annot /Subtype /Highlight /Rect [20 140 120 165] /QuadPoints [20 165 120 165 20 140 120 140] /C [1 0.94 0] >> endobj
trailer << /Root 1 0 R >>
%%EOF";
let mut doc = pdfcraft_cos::Document::open(std::sync::Arc::new(plain.to_vec())).expect("fixture parses");
doc.set_encryption(&pdfcraft_cos::NewEncryption {
algorithm: pdfcraft_cos::Algorithm::Aes256,
user_password: "",
owner_password: "owner",
permissions: 0b0100,
encrypt_metadata: true,
seed: [5; 32],
})
.expect("encrypts");
let bytes = pdfcraft_cos::write_full(&doc, &Default::default()).expect("writes");
let mut h = harness(move |app| {
app.open_bytes("restricted.pdf", None, bytes).expect("opens restricted");
app.set_option("quick", "highlight").unwrap();
});
let id = h.state().active_ids().expect("active document").1;
assert!(!h.state().session.get(id).expect("document").allows_annotation(), "commenting is forbidden");
let index = h.state().views.iter().position(|v| v.id == id).expect("view");
h.state_mut().views[index].comments.selected = Some((0, 0));
h.run_steps(2);
h.get_by_label("Colour").click();
h.run_steps(2);
h.get_by_label("Blue").click();
h.run_steps(3);
let state = h.state();
let doc = state.session.get(id).expect("document");
assert_eq!(doc.comment_props(0, 0).and_then(|props| props.color), Some([1.0, 0.94, 0.0]), "the comment keeps its colour");
assert_eq!(doc.can_undo(), None, "no edit was made");
assert_eq!(state.toast, None, "picking a colour is not reported as a failed edit");
assert_eq!(state.comment_prefs.style(CommentTool::Highlight).color, [0.0, 0.47, 0.84], "the tool default still changes");
}
#[test] #[test]
fn a_new_highlight_opens_its_note_for_typing() { fn a_new_highlight_opens_its_note_for_typing() {
let mut h = harness(|app| app.set_option("quick", "highlight").unwrap()); let mut h = harness(|app| app.set_option("quick", "highlight").unwrap());

View File

@ -3830,8 +3830,8 @@ milestone = "M5"
status = "shipped" status = "shipped"
commands = ["comment.note"] commands = ["comment.note"]
tools = ["comment_edit"] tools = ["comment_edit"]
evidence = ["crates/annot/src/tests.rs::restyle_and_resize_redraw_the_appearance", "crates/ui-egui/tests/comments.rs::the_opacity_set_for_a_tool_goes_into_its_new_comments"] evidence = ["crates/annot/src/tests.rs::restyle_and_resize_redraw_the_appearance", "crates/ui-egui/tests/comments.rs::the_opacity_set_for_a_tool_goes_into_its_new_comments", "crates/ui-egui/tests/comments.rs::the_quick_bar_colour_changes_the_selected_comment", "crates/ui-egui/tests/comments.rs::the_quick_bar_colour_changes_the_selected_arrow"]
notes = "quick-bar colour swatches, fill colour (rectangle, oval, polygon, cloud), opacity (10–100 %) and line thickness per tool; colour from the context menu" notes = "quick-bar colour swatches immediately recolour a selected comment made with the active tool, and set the tool default; fill colour (rectangle, oval, polygon, cloud), opacity (10–100 %) and line thickness per tool; colour from the context menu"
[[feature]] [[feature]]
id = "comment.lock-comment" id = "comment.lock-comment"