From ad9e5d2ba99db6139b8f540f0e4945c636546736 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Cie=C5=9Blik?= <132496025+Hathor875@users.noreply.github.com> Date: Fri, 16 Oct 2026 02:49:20 +0200 Subject: [PATCH 01/20] fix(edit): keep a one-row centred title centred when it grows (#2581) - Detect a one-row centred title through a new `is_centred` helper in `text_regions.rs`, shared by both rewrite paths, instead of the multi-row ragged-centres rule that can never fire on a single row. - A lone row counts as centred when its centre sits within one font size of the page centre and its left edge is not the page's dominant left edge, so a full-width left-aligned line at the margin is not mistaken for a title. - In the single-style path, a centred block with no explicit width or alignment no longer has its width stretched to the page margin; the line is centred in the box it already sits in, so a grown row keeps its centre and every wrapped row shares it. - Gate the new rule behind main's `upright` flag: `is_centred` measures page-space rects, which are not meaningful across a turned baseline; frame coordinates (#1443) are the tool for that, not this change. - Add `a_one_row_title_keeps_its_centre_when_it_grows_and_when_it_wraps`, built from the issue's fixture; red before the fix (centre 398.025 vs expected 306), green after. --- crates/edit/src/tests.rs | 37 +++++++++++++++++++++++++++ crates/edit/src/text.rs | 14 +++++++--- crates/edit/src/text_regions.rs | 45 ++++++++++++++++++++++++++++----- parity/acrobat-features.toml | 4 +-- 4 files changed, 88 insertions(+), 12 deletions(-) diff --git a/crates/edit/src/tests.rs b/crates/edit/src/tests.rs index b542142df8..dc86c947be 100644 --- a/crates/edit/src/tests.rs +++ b/crates/edit/src/tests.rs @@ -2046,6 +2046,43 @@ fn paragraphs_take_new_formatting() { } } +#[test] +fn a_one_row_title_keeps_its_centre_when_it_grows_and_when_it_wraps() { + // A centred one-row title (its centre is at x ≈ 306 on the 600 pt page) above a body + // left-aligned at x = 72. Growing it must not drag it to the right. + let src = "BT /F1 18 Tf 1 0 0 1 252.477 700 Tm (Centered title) Tj ET\n\ + BT /F1 12 Tf 1 0 0 1 72 640 Tm (Body text below the heading starts here and runs) Tj ET\n\ + BT /F1 12 Tf 1 0 0 1 72 625.6 Tm (on for a second row of ordinary words.) Tj ET"; + let mut doc = text_page(src); + let before = text::text_blocks(&doc, 0).unwrap()[0].clone(); + let centre = (before.rect[0] + before.rect[2]) / 2.0; + text::rewrite_block(&mut doc, 0, 0, Some("Centered title with a few more words"), &text::BlockStyle::default()).unwrap(); + let doc = reopen(&doc); + let blocks = text::text_blocks(&doc, 0).unwrap(); + let lines = text::text_lines(&doc, 0).unwrap(); + assert_eq!(blocks[0].text, "Centered title with a few more words"); + let r = lines[blocks[0].lines[0]].rect; + assert!(((r[0] + r[2]) / 2.0 - centre).abs() < 1.0, "the grown title stays centred on {centre}, its centre is {}", (r[0] + r[2]) / 2.0); + // Long enough to have to wrap: every row stays centred on the same centre too. + let mut doc = text_page(src); + text::rewrite_block( + &mut doc, + 0, + 0, + Some("Centered title with many more words added so that the single row has to become much longer"), + &text::BlockStyle::default(), + ) + .unwrap(); + let doc = reopen(&doc); + let blocks = text::text_blocks(&doc, 0).unwrap(); + let lines = text::text_lines(&doc, 0).unwrap(); + assert!(blocks[0].lines.len() > 1, "the long title wraps into rows: {:?}", blocks[0].text); + for i in &blocks[0].lines { + let r = lines[*i].rect; + assert!(((r[0] + r[2]) / 2.0 - centre).abs() < 2.0, "row {:?} stays centred on {centre}", lines[*i].text); + } +} + #[test] fn a_new_paragraph_colour_does_not_spill_into_the_text_after_it() { let mut doc = text_page("BT /F1 10 Tf 72 700 Td (First paragraph) Tj 0 -40 Td (Second paragraph) Tj ET"); diff --git a/crates/edit/src/text.rs b/crates/edit/src/text.rs index 9c83cf9a39..a24a0f4773 100644 --- a/crates/edit/src/text.rs +++ b/crates/edit/src/text.rs @@ -22,7 +22,7 @@ use crate::EditError; #[path = "text_regions.rs"] mod regions; -use regions::{group_blocks, rewrite_styled_block}; +use regions::{group_blocks, is_centred, rewrite_styled_block}; /// One line of existing text. #[derive(Clone, Debug, PartialEq)] @@ -2098,6 +2098,11 @@ fn rewrite_block_from( } else { members.iter().map(|l| l.frame_rect()).reduce(|a, f| [a[0].min(f[0]), a[1].min(f[1]), a[2].max(f[2]), a[3].max(f[3])]).unwrap_or(b.rect) }; + // A centred title is laid out in the box it already sits in, so it keeps its centre as it + // grows or wraps, rather than being grown to the right up to the page's margin (#2581). + // Turned text is left alone: its centre lives along its own baseline, not in page space. + let crop = p.crop(doc); + let centred = upright && !rtl && style.width.is_none() && style.align.is_none() && is_centred(&b, &lines, (crop[0] + crop[2]) / 2.0, first.size); // A paragraph keeps its width (or takes the one asked for); a single line grows to the right // (to the left when right-to-left), up to the page's margin. let width = match style.width { @@ -2106,14 +2111,15 @@ fn rewrite_block_from( Some(_) => return Err(EditError::Invalid("the paragraph's width must be a number".into())), // Turned text: the page's margins are along another axis. A single line grows (below). None if !upright => (frame[2] - frame[0]).max(size * k), - None if anchored_right => (b.rect[2] - b.rect[0]).max(size * k).max(b.rect[2] + dx - (p.crop(doc)[0] + 36.0)), - None if members.len() == 1 => (b.rect[2] - b.rect[0]).max(size * k).max(p.crop(doc)[2] - 36.0 - (b.rect[0] + dx)), + None if anchored_right => (b.rect[2] - b.rect[0]).max(size * k).max(b.rect[2] + dx - (crop[0] + 36.0)), + // A centred line is not stretched to the margin: its own box is the frame it is centred in. + None if members.len() == 1 && !centred => (b.rect[2] - b.rect[0]).max(size * k).max(crop[2] - 36.0 - (b.rect[0] + dx)), None => (b.rect[2] - b.rect[0]).max(size * k), }; if !width.is_finite() { return Err(EditError::Invalid("the paragraph's width must be a number".into())); } - let align = style.align.or(rtl.then_some(crate::added::Align::Right)); + let align = if centred { Some(crate::added::Align::Center) } else { style.align.or(rtl.then_some(crate::added::Align::Right)) }; // Where a right-aligned line ends, from the paragraph's left edge (user space). let right_edge = if anchored_right { (b.rect[2] - b.rect[0]).max(0.0) } else { width }; // The move and the width are checked here, before the fallback fonts are written: a refused diff --git a/crates/edit/src/text_regions.rs b/crates/edit/src/text_regions.rs index 2d7baf519f..75e35b723b 100644 --- a/crates/edit/src/text_regions.rs +++ b/crates/edit/src/text_regions.rs @@ -31,6 +31,43 @@ fn frame_row_rect(row: &[usize], lines: &[TextLine]) -> Option<[f64; 4]> { row.iter().filter_map(|i| lines.get(*i)).map(TextLine::frame_rect).reduce(union) } +/// The left edge the rest of the page's text sits on, ignoring the rows of `block`. A single row +/// that does not share it is not merely an indented or full-width left-aligned line, so its own +/// centre is the only remaining explanation for where it sits. +fn neighbour_left(lines: &[TextLine], block: &TextBlock) -> Option { + let mut edges: HashMap = HashMap::new(); + for (i, line) in lines.iter().enumerate() { + if block.lines.contains(&i) { + continue; + } + // Quarter-point buckets: equal edges found through different runs still agree. + *edges.entry((line.rect[0] * 4.0).round() as i64).or_insert(0) += 1; + } + let (best, _) = edges.into_iter().max_by_key(|(_, count)| *count)?; + Some(best as f64 / 4.0) +} + +/// Whether a block reads as centred, and so must keep its centre when it grows or wraps. +/// +/// Several ragged rows whose left edges disagree while their centres agree are centred. A one row +/// block cannot say that: all of its left edges are the same, so the ragged test never fires and a +/// centred title would be taken for left-aligned and grow to the right (#2581). Fall back to the +/// page instead — a lone row sitting on the page's own centre, and not on the edge the rest of the +/// text uses, is a centred title. A full-width left-aligned line still lines up with that edge, so +/// it is not mistaken for one. +pub(super) fn is_centred(block: &TextBlock, lines: &[TextLine], page_centre: f64, size: f64) -> bool { + let centre = (block.rect[0] + block.rect[2]) / 2.0; + let lefts: Vec<_> = block.rows.iter().filter_map(|r| row_rect(r, lines).map(|r| r[0])).collect(); + if block.rows.len() > 1 { + let left_span = lefts.iter().copied().fold(f64::NEG_INFINITY, f64::max) - lefts.iter().copied().fold(f64::INFINITY, f64::min); + return left_span > size * 0.5 + && block.rows.iter().all(|row| row_rect(row, lines).is_some_and(|r| ((r[0] + r[2]) / 2.0 - centre).abs() < size)); + } + lefts + .first() + .is_some_and(|left| (centre - page_centre).abs() < size && neighbour_left(lines, block).is_none_or(|edge| (*left - edge).abs() > size * 0.5)) +} + pub(super) fn group_blocks(lines: &[TextLine]) -> Vec { group_layout(lines, true) } @@ -674,12 +711,8 @@ pub(super) fn rewrite_styled_block( .ok_or_else(|| EditError::Invalid("the paragraph is drawn in a space it cannot be edited in".into()))?; let (tx, ty) = back.apply(dx, dy); block_ops.push(Op::new("cm", vec![n(1.0), n(0.0), n(0.0), n(1.0), n(tx), n(ty)])); - let centre = (block.rect[0] + block.rect[2]) / 2.0; - let lefts: Vec<_> = block.rows.iter().filter_map(|r| row_rect(r, lines).map(|r| r[0])).collect(); - let left_span = lefts.iter().copied().fold(f64::NEG_INFINITY, f64::max) - lefts.iter().copied().fold(f64::INFINITY, f64::min); - let centered = block.rows.len() > 1 - && left_span > first.size * 0.5 - && block.rows.iter().all(|row| row_rect(row, lines).is_some_and(|r| ((r[0] + r[2]) / 2.0 - centre).abs() < first.size)); + let crop = p.crop(doc); + let centered = is_centred(block, lines, (crop[0] + crop[2]) / 2.0, first.size); let source_leading = block .rows .windows(2) diff --git a/parity/acrobat-features.toml b/parity/acrobat-features.toml index 533013b547..e47bd28986 100644 --- a/parity/acrobat-features.toml +++ b/parity/acrobat-features.toml @@ -1962,8 +1962,8 @@ milestone = "M7" status = "shipped" commands = ["edit.edit_text", "edit.text"] tools = ["text_edit", "content_update"] -evidence = ["crates/edit/src/tests.rs::justify_underline_and_spacing", "crates/edit/src/tests.rs::paragraphs_take_new_formatting", "crates/ui-egui/tests/editing.rs::the_format_panel_restyles_the_paragraph_being_edited"] -notes = "left, centre, right and justify (word spacing per line, last line left) for existing paragraphs and added text" +evidence = ["crates/edit/src/tests.rs::justify_underline_and_spacing", "crates/edit/src/tests.rs::paragraphs_take_new_formatting", "crates/edit/src/tests.rs::a_one_row_title_keeps_its_centre_when_it_grows_and_when_it_wraps", "crates/ui-egui/tests/editing.rs::the_format_panel_restyles_the_paragraph_being_edited"] +notes = "left, centre, right and justify (word spacing per line, last line left) for existing paragraphs and added text. A centred paragraph is found either from its ragged rows or, for a one row title, from its centre sitting on the page's own centre away from the edge the rest of the text uses, so growing or wrapping it keeps that centre (#2581)" [[feature]] id = "edit.line-paragraph-spacing" From 437b691eb7862541d49ec3a406d6741293b94317 Mon Sep 17 00:00:00 2001 From: Guillem Hermida Date: Fri, 16 Oct 2026 18:25:57 +0200 Subject: [PATCH 02/20] fix(ui): the Read mode command leaves Edit as the Edit tab does #2241 made leaving the Edit tab put Edit text & images down and apply a paragraph still open in the editor, but only in select_mode, which only the mode bar reaches. The Read mode command (View > Read mode, Ctrl+H, the command search) went through switch_mode's other branch, which only set the mode: Read mode then kept Edit text & images on, every paragraph boxed, and an open paragraph editor floating over the page with its edit not applied. enter_mode now sets the workspace together with its tool, and both routes go through it, so they can't drift apart again. The command still leaves the panels as they were. Test: editing.rs::the_read_mode_command_applies_an_open_paragraph_and_puts_edit_text_down --- crates/ui-egui/src/lib.rs | 18 ++++++++++++----- crates/ui-egui/tests/editing.rs | 34 +++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 5 deletions(-) diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index 79464624c6..27db0e98cc 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -2132,11 +2132,10 @@ impl PdfCraftApp { } } - /// Select the workspace and its matching tool panel, just like the mode bar. Edit turns on - /// Edit text & images at once, as Acrobat does: every paragraph and image on the page gets its - /// box and is editable with one click. Leaving Edit puts the editing tools down again. + /// Select the workspace and its matching tool panel, just like the mode bar. The tool goes + /// with the workspace as [`Self::enter_mode`] sets it. pub(crate) fn select_mode(&mut self, mode: Mode) { - self.mode = mode; + self.enter_mode(mode); self.reveal_tools(); if let Some(view) = self.active.and_then(|i| self.views.get_mut(i)) { view.organize = false; @@ -2147,6 +2146,15 @@ impl PdfCraftApp { Mode::Sign => LeftPanel::Tool("fill_sign"), _ => LeftPanel::AllTools, }; + } + + /// Changes the workspace and the tool that goes with it. The mode bar (through + /// [`Self::select_mode`]) and the Read mode command both come here, so they leave Edit alike. + /// Edit turns on Edit text & images at once, as Acrobat does: every paragraph and image on the + /// page gets its box and is editable with one click. Leaving Edit puts the editing tools down + /// again and applies a paragraph still open in the editor. + fn enter_mode(&mut self, mode: Mode) { + self.mode = mode; if mode == Mode::Edit { self.quick_tool = QuickTool::EditText; } else if matches!(self.quick_tool, QuickTool::EditText | QuickTool::AddText) { @@ -2174,7 +2182,7 @@ impl PdfCraftApp { if select_tools { self.select_mode(mode); } else { - self.mode = mode; + self.enter_mode(mode); } if mode == Mode::Read && !was_read { self.right_before_read = self.right.take(); diff --git a/crates/ui-egui/tests/editing.rs b/crates/ui-egui/tests/editing.rs index f8db525751..fb0e750282 100644 --- a/crates/ui-egui/tests/editing.rs +++ b/crates/ui-egui/tests/editing.rs @@ -2629,6 +2629,40 @@ fn editing_existing_text_in_place() { assert_eq!(texts_of(s, 0), ["Chapter One"], "the page shows it"); } +/// The Read mode command (View ▸ Read mode, its shortcut, the palette) leaves Edit as the Edit tab +/// does when another tab is chosen (#2241): Edit text & images is put down, and a paragraph still +/// open in the editor is applied instead of staying open, editable, over the page in Read mode. +#[test] +fn the_read_mode_command_applies_an_open_paragraph_and_puts_edit_text_down() { + use pdfcraft_ui_egui::{Mode, QuickTool}; + let mut h = harness(1, |_| {}); + h.get_by_label("Edit").click(); + h.run_steps(3); + assert_eq!((h.state().mode, h.state().quick_tool), (Mode::Edit, QuickTool::EditText)); + // Open the line "Page 1" (at (20, 150) on a 200 × 300 page, 24 pt) and retype it. + let r = h.state().views[0].page_screen_rect(0).expect("on screen"); + let at = egui::pos2(r.left() + 40.0 / 200.0 * r.width(), r.top() + (300.0 - 158.0) / 300.0 * r.height()); + h.hover_at(at); + h.run_steps(1); + h.drag_at(at); + h.run_steps(1); + h.drop_at(at); + h.run_steps(3); + assert_eq!(h.state().views[0].line_editor.as_ref().expect("the line opens for editing").text, "Page 1"); + h.state_mut().views[0].line_editor.as_mut().unwrap().text = "Chapter One".into(); + h.run_steps(1); + assert!(h.state_mut().execute("view.read_mode")); + h.run_steps(3); + let s = h.state(); + assert_eq!(s.mode, Mode::Read); + assert_eq!(s.quick_tool, QuickTool::Select, "Edit text & images is put down"); + assert!(s.views[0].line_editor.is_none(), "no paragraph editor is left open in Read mode"); + let doc = s.session.get(s.views[0].id).unwrap(); + assert_eq!(doc.can_undo(), Some("Edit text"), "the open paragraph's edit is applied"); + assert_eq!(doc.text_lines(0)[0].text, "Chapter One"); + assert_eq!(texts_of(s, 0), ["Chapter One"], "the page shows it"); +} + #[test] fn text_tool_clicking_source_text_edits_instead_of_adding_a_copy() { let mut h = harness(1, |_| {}); From b15fe91627246bc54aac4b28a8ba3f708d10cfcf Mon Sep 17 00:00:00 2001 From: mtlcl410 <2368091+mtlcl410@users.noreply.github.com> Date: Sun, 18 Oct 2026 01:19:28 +0800 Subject: [PATCH 03/20] test: the image-drag pixel test waits for the page raster image_pixels_follow_the_frame_while_a_corner_is_dragged drags before the asynchronous page raster has arrived on a slow runner, and the preview (drawn from that raster) then draws no picture: 'not only at []' on windows-latest for #3070 and #3071 (a deny.toml-only change). With a 300 ms render delay added locally, main's test fails the same way and this one passes. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01RUNQSPVXx3dDBabRZHAnjz --- crates/ui-egui/tests/editing.rs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/crates/ui-egui/tests/editing.rs b/crates/ui-egui/tests/editing.rs index f8db525751..570b847507 100644 --- a/crates/ui-egui/tests/editing.rs +++ b/crates/ui-egui/tests/editing.rs @@ -4197,6 +4197,16 @@ fn image_pixels_follow_the_frame_while_a_corner_is_dragged() { assert!(h.state_mut().execute("edit.edit_text")); h.state_mut().views[0].set_zoom(1.2); h.run_steps(4); + // The dragged pixels are the page raster's: wait for it, as a slow runner may not have it yet + // (the drag then drew no picture at all). + for _ in 0..400 { + h.run_steps(1); + if !h.state().render_pending() { + break; + } + std::thread::sleep(std::time::Duration::from_millis(5)); + } + assert!(!h.state().render_pending(), "the page raster arrives"); let page = h.state().views[0].page_screen_rect(0).unwrap(); let at = |x: f32, y: f32| egui::pos2(page.left() + x / 200.0 * page.width(), page.top() + (300.0 - y) / 300.0 * page.height()); let p = at(95.0, 195.0); From 66981f1e47f0b52d0686e4b918a53f7fa2a9a331 Mon Sep 17 00:00:00 2001 From: PerchAnton <101743951+PerchAnton@users.noreply.github.com> Date: Sun, 18 Oct 2026 09:38:29 +0300 Subject: [PATCH 04/20] M7: normalize added-image rotations before addition --- ROADMAP.md | 3 + crates/automation/README.md | 6 ++ crates/automation/src/content.rs | 3 +- crates/automation/tests/automation.rs | 97 +++++++++++++++++++++++++++ docs/roadmap.md | 4 +- parity/acrobat-features.toml | 6 +- 6 files changed, 114 insertions(+), 5 deletions(-) diff --git a/ROADMAP.md b/ROADMAP.md index ee84a11110..ccf6269e32 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -2,6 +2,7 @@ **Stage: alpha** · next: beta, ~25% (ready for real work 50% → 75%) and ~400–700 h away +> **Last reviewed:** 2026-10-18 · **Last updated:** 2026-10-18 · **Change:** minor (M7: added-image automation normalizes large rotation angles before addition; estimates unchanged) > **Last reviewed:** 2026-10-18 · **Last updated:** 2026-10-18 · **Change:** minor (M8, #1350: named replacement descriptions retired during Sanitize and across shared resources; estimates unchanged) > **Last reviewed:** 2026-10-15 · **Last updated:** 2026-10-15 · **Change:** minor (M6: AcroForm XML text; estimates unchanged) > **Last reviewed:** 2026-10-15 · **Last updated:** 2026-10-15 · **Change:** minor (M2/gap 12: the Annex D.2 alias codes read as the ink they draw in both tables; the text oracle scores reading order; estimates unchanged) · **Target:** Adobe Acrobat Pro (Acrobat DC, continuous track 26.002.21931, macOS) @@ -161,6 +162,7 @@ Ranked; detail and the full milestone table in [docs/roadmap.md](docs/roadmap.md | 8 | 1.0 polish: performance, right-to-left, missing languages, keyboard-only (M14) | 55–95 | ## Progress log +- **2026-10-18 (M7, added-image rotation):** Added-image content_update reduces signed rotation angles before adding the current quarter turns. Extreme valid angles no longer panic or wrap to a wrong orientation. Synthetic automation and MCP regressions cover all starting orientations, Undo/Redo, invalid-angle preservation, rendered pixels and save/reopen. Estimates unchanged. - **2026-10-18 (M8, #1350):** Integrated current main while preserving content-stream comment boundaries. Hidden-text cleanup retires unused named replacement descriptions on rewritten pages and forms, including inherited/shared resource holders, without removing descriptions still used by surviving content. Synthetic saved-byte and preservation regressions cover the cleanup. Estimates unchanged. - **2026-10-15 (M6, AcroForm XML text):** Plain AcroForm XML imports retain all direct text around comments and processing instructions, including CDATA and character references. Synthetic regressions verify exact values and unknown catalog data after incremental and full save/reopen. Nested-element and XFA interpretation are unchanged. Estimates unchanged. - **2026-10-15 (M6, #1917 refresh):** Integrated upstream `99941daa` while retaining the XML/XFDF attribute whitespace correction and all upstream progress. Only independent documentation conflicts required resolution; focused production and test files are unchanged from the previously green CI head. Estimates unchanged. @@ -506,6 +508,7 @@ Newest first: the date, what moved, and the new overall estimate. | Date | Change | Summary | |---|---|---| +| 2026-10-18 | minor | M7: Added-image content_update reduces signed rotation angles before adding the current quarter turns. Extreme valid angles no longer panic or wrap to a wrong orientation. Synthetic automation and MCP regressions cover all starting orientations, Undo/Redo, invalid-angle preservation, rendered pixels and save/reopen. Estimates unchanged. | | 2026-10-15 | minor | M6: Plain AcroForm XML imports retain all direct text around comments and processing instructions, including CDATA and character references. Estimates unchanged. | | 2026-10-13 | minor | M6: XML/XFDF attribute whitespace survives form and comment export/import; estimates unchanged | | 2026-10-14 | minor | M1: raw-deflate fallback preserves decoding-limit errors in strict and tolerant decoding; synthetic filter and COS stream regressions. Estimates unchanged. | diff --git a/crates/automation/README.md b/crates/automation/README.md index 4e27702a32..7593290d4d 100644 --- a/crates/automation/README.md +++ b/crates/automation/README.md @@ -15,6 +15,12 @@ tools() -> Vec // name, title, descrip mcp::McpServer::new(a).serve(stdin, stdout)? // newline-delimited JSON-RPC 2.0 ``` +`content_update.rotate` rotates an added image counter-clockwise by an integer multiple of +90 degrees. Large positive and negative signed 64-bit angles are normalized before combining +them with the image's current orientation. Full turns retain its appearance, rectangle, crop +and flips. Angles that are not multiples of 90 are refused without changing the document or +its undo/redo history. + `Content` is `Json(Value)` or `Png { data, width, height }`. Errors are `UnknownTool`, `InvalidArgs` (the call didn't match the schema) or `Failed` (a readable message for the agent). ## Conventions diff --git a/crates/automation/src/content.rs b/crates/automation/src/content.rs index 710952d7c2..3be99d846a 100644 --- a/crates/automation/src/content.rs +++ b/crates/automation/src/content.rs @@ -171,7 +171,8 @@ impl Automation { if deg.rem_euclid(90) != 0 { return Err(bad("rotate must be a multiple of 90")); } - i.rotation = ((i64::from(i.rotation) * 90 + deg).rem_euclid(360) / 90) as u8; + // Reduce untrusted degrees before adding the current quarter turns. + i.rotation = (i64::from(i.rotation) + deg.rem_euclid(360) / 90).rem_euclid(4) as u8; } if a.opt_bool("flip_h")? == Some(true) { i.flip_h = !i.flip_h; diff --git a/crates/automation/tests/automation.rs b/crates/automation/tests/automation.rs index 1e662d8f38..ff64136153 100644 --- a/crates/automation/tests/automation.rs +++ b/crates/automation/tests/automation.rs @@ -3582,6 +3582,103 @@ fn adding_content_through_tools() { assert!(matches!(a.call("content_delete", &json!({ "doc": doc, "page": 1, "index": 5 })), Err(ToolError::Failed(_)))); } +fn added_rotation_image(dir: &Path) { + let mut png = Vec::new(); + let mut enc = png::Encoder::new(&mut png, 2, 1); + enc.set_color(png::ColorType::Rgb); + enc.write_header().unwrap().write_image_data(&[255, 0, 0, 0, 0, 255]).unwrap(); + std::fs::write(dir.join("rotation.png"), png).unwrap(); +} + +#[test] +fn large_added_image_rotations_keep_orientation_and_undo() { + let dir = workdir("large-added-rotation"); + added_rotation_image(&dir); + let mut a = auto(&dir); + let image = |a: &Automation, doc| { + let d = a.session().get(pdfcraft_engine::DocId(doc)).unwrap(); + let pdfcraft_engine::AddedContent::Image(image) = &d.added()[0].content else { panic!("added image") }; + image.clone() + }; + let largest = i64::MAX / 90 * 90; + let smallest = i64::MIN / 90 * 90; + for initial in [1, 0, 2, 3] { + let doc = ok(&mut a, "doc_create", json!({ "from": "blank", "width": 200, "height": 300 }))["doc"].as_u64().unwrap(); + ok(&mut a, "page_add_image", json!({ "doc": doc, "page": 1, "path": "rotation.png", "rect": [20, 20, 120, 70] })); + ok( + &mut a, + "content_update", + json!({ "doc": doc, "page": 1, "index": 1, "rotate": initial * 90, + "flip_h": true, "flip_v": true, "crop": [0.1, 0.05, 0.2, 0.1] }), + ); + let before = image(&a, doc); + for degrees in [largest, largest - 90, largest - 180, smallest, smallest + 90, smallest + 180, -450, -90, 0, 90, 450] { + ok(&mut a, "content_update", json!({ "doc": doc, "page": 1, "index": 1, "rotate": degrees })); + let expected = pdfcraft_engine::AddedImage { rotation: (initial + (degrees / 90).rem_euclid(4)).rem_euclid(4) as u8, ..before.clone() }; + assert_eq!(image(&a, doc), expected, "initial quarter turn {initial}, degrees {degrees}"); + ok(&mut a, "edit_undo", json!({ "doc": doc })); + assert_eq!(image(&a, doc), before, "Undo restores the image and its properties"); + ok(&mut a, "edit_redo", json!({ "doc": doc })); + assert_eq!(image(&a, doc), expected, "Redo restores the normalized orientation"); + ok(&mut a, "edit_undo", json!({ "doc": doc })); + } + ok(&mut a, "doc_save", json!({ "doc": doc, "path": format!("initial-{initial}.pdf") })); + let state = |a: &Automation| { + let d = a.session().get(pdfcraft_engine::DocId(doc)).unwrap(); + (d.bytes.clone(), d.dirty, d.edit_generation(), d.can_undo().map(str::to_owned), d.can_redo().map(str::to_owned)) + }; + let saved = state(&a); + assert!(!saved.1 && saved.4.is_some(), "a clean document with a redo step"); + for degrees in [i64::MIN, i64::MAX, -1, 1, 45] { + assert!(matches!( + a.call("content_update", &json!({ "doc": doc, "page": 1, "index": 1, "rotate": degrees })), + Err(ToolError::InvalidArgs(_)) + )); + assert_eq!(state(&a), saved, "invalid degrees {degrees} leave bytes and history alone"); + } + } +} + +#[cfg(feature = "mcp")] +#[test] +fn large_added_image_rotations_round_trip_through_mcp() { + let dir = workdir("large-added-rotation-mcp"); + added_rotation_image(&dir); + let mut a = auto(&dir); + let doc = ok(&mut a, "doc_create", json!({ "from": "blank", "width": 200, "height": 300 }))["doc"].as_u64().unwrap(); + ok(&mut a, "page_add_image", json!({ "doc": doc, "page": 1, "path": "rotation.png", "rect": [20, 20, 120, 70] })); + ok( + &mut a, + "content_update", + json!({ "doc": doc, "page": 1, "index": 1, "rotate": 90, + "flip_h": true, "flip_v": true, "crop": [0.1, 0.05, 0.2, 0.1] }), + ); + let mut server = McpServer::new(a); + let render = json!({ "name": "page_render", "arguments": { "doc": doc, "page": 1, "dpi": 72 } }); + let before = rpc(&mut server, 1, "tools/call", render.clone()); + let update = rpc( + &mut server, + 2, + "tools/call", + json!({ "name": "content_update", "arguments": { + "doc": doc, "page": 1, "index": 1, "rotate": i64::MAX / 360 * 360 + } }), + ); + assert_eq!(update["result"]["isError"], false, "{update}"); + let after = rpc(&mut server, 3, "tools/call", render.clone()); + assert_eq!(after["result"]["content"], before["result"]["content"], "full turns retain the rendered orientation"); + let saved = rpc(&mut server, 4, "tools/call", json!({ "name": "doc_save", "arguments": { "doc": doc, "path": "rotated.pdf" } })); + assert_eq!(saved["result"]["isError"], false, "{saved}"); + let mut reopened = auto(&dir); + let doc = ok(&mut reopened, "doc_open", json!({ "path": "rotated.pdf" }))["doc"].as_u64().unwrap(); + let d = reopened.session().get(pdfcraft_engine::DocId(doc)).unwrap(); + let pdfcraft_engine::AddedContent::Image(image) = &d.added()[0].content else { panic!("saved image") }; + assert_eq!(image.rotation, 1); + let png = reopened.call("page_render", &json!({ "doc": doc, "page": 1, "dpi": 72 })).unwrap(); + let Content::Png { data, .. } = &png[0] else { panic!("rendered PNG") }; + assert_eq!(after["result"]["content"][0]["data"], base64::Engine::encode(&base64::engine::general_purpose::STANDARD, data)); +} + /// Text styling on an added image is refused (#1331), as image transforms are on text, and the /// refusal leaves the content, the dirty flag and the undo and redo steps as they were. #[test] diff --git a/docs/roadmap.md b/docs/roadmap.md index 55e898b283..dfcc6e7763 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -1,5 +1,6 @@ # PdfCraft roadmap: milestones and what's next +> **Last reviewed:** 2026-10-18 · **Last updated:** 2026-10-18 · **Change:** minor (M7: added-image automation normalizes large rotation angles before addition; estimates unchanged) > **Last reviewed:** 2026-10-18 · **Last updated:** 2026-10-18 · **Change:** minor (M8, #1350: named replacement descriptions retired during Sanitize and across shared resources; estimates unchanged) > **Last reviewed:** 2026-10-15 · **Last updated:** 2026-10-15 · **Change:** minor (M6: AcroForm XML text; estimates unchanged) > **Last reviewed:** 2026-10-15 · **Last updated:** 2026-10-15 · **Change:** minor (M6: preserve XML/XFDF attribute whitespace; refreshed upstream; estimates unchanged) · **Target:** Adobe Acrobat Pro (Acrobat DC, continuous track 26.002.21931, macOS) @@ -123,7 +124,7 @@ total matches the parity estimate. | M5 | Comments (all annotation types, XFDF) | 120–200 | 88% | 12–25 | Done: notes, highlight/underline/strikeout/squiggly, text boxes, ink, lines, arrows, rectangles, ovals, stamps (dynamic, Sign Here, Standard Business), with appearance streams; replies, status, checkmarks, locking, move/resize/restyle/delete; properties dialog; panel filter and sort; hide all; summaries (comments only); XFDF/FDF import and export (partial imports preserve existing reply and review-status links); flatten; Fill & Sign with adjustable text/date size; agent tools.
Done since: polygons, connected lines, clouds, callouts, inserted text, colour and checkmark filters; image signature pointer previews, selection handles and live image movement/resizing with proportional corners and independent edge handles. A refused restyle or resize of a text box another program drew leaves that comment unchanged (#1327). Make Current Properties Default on an arrow updates arrows, not plain lines (#1302). Flattening keeps a visible comment it cannot draw (#1317). Missing: custom stamps, replace-text proposals, summary layouts with the page | | M6 | Forms + JavaScript | 160–320 | 50% | 40–75 | Done: filling all field types with regenerated appearances; Prepare a form (every field type, multi-select/shared properties, move/resize/delete, Field Properties General/Appearance/Position/Options/Format/Validate/Calculate); Acrobat's AF format/keystroke/validate/calculate functions and simplified field notation run natively in Acrobat's event order; tab order; push-button actions; flatten; agent tools. Form data exchange (FDF, XFDF, XML, CSV, text) done; XFDF values retain text across XML comments and processing instructions. Plain AcroForm XML imports retain all direct text around comments and processing instructions, including CDATA and character references.
Done since: JavaScript engine (boa, sandboxed) running custom keystroke/validate/calculate/format and button scripts, document JavaScripts, console. Missing: Actions tab, wider object model (annotations, layers, dialogs), XFA
Done since: Multi-select choice fields write ascending `/I` option indices while preserving `/V` selection order; synthetic forms/engine regressions cover reverse selection and save/reopen. CSV export preserves carriage returns in names and values. | | M6 | Forms + JavaScript | 160–320 | 50% | 40–75 | Done: filling all field types with regenerated appearances; Prepare a form (every field type, multi-select/shared properties, move/resize/delete, Field Properties General/Appearance/Position/Options/Format/Validate/Calculate); Acrobat's AF format/keystroke/validate/calculate functions and simplified field notation run natively in Acrobat's event order; tab order; push-button actions; flatten; agent tools. Form data exchange (FDF, XFDF, XML, CSV, text) done; XFDF values retain text across XML comments and processing instructions. XML/XFDF exports preserve tabs and line breaks in names and comment metadata. Refreshed against upstream `99941daa` (2026-10-15).
Done since: JavaScript engine (boa, sandboxed) running custom keystroke/validate/calculate/format and button scripts, document JavaScripts, console. Missing: Actions tab, wider object model (annotations, layers, dialogs), XFA
Done since: Multi-select choice fields write ascending `/I` option indices while preserving `/V` selection order; synthetic forms/engine regressions cover reverse selection and save/reopen. CSV export preserves carriage returns in names and values. | -| M7 | Content editing (text, images, header/footer, watermark) | 250–500 | 20% | 100–180 | Done: header & footer, watermarks, backgrounds, Bates; added text and images that stay editable (move, resize, format, rotate, flip, crop, replace); links (Link tool, Link Properties, create from URLs, remove all). Done since: Edit text preserves Arabic/Hebrew `/ActualText` typing order, with font-independent regressions; single-glyph RTL ligature spans (including lam-alef) join their surrounding word. Done since (#1116): editable text regions grouped across compatible words and subset fonts, one rich-text editor over a background with the source text removed, embedded sfnt faces reused as editor faces, and visible (clipped) bounds for cropped-image selection and resize. Exclusive crops now resize across nested states and content streams, with shared clips fixed.
Done since: #594 ordinary paragraph drags preserve overlapping source text; UI/engine regressions, native save/reopen, independent-reader checks and full CI passed in the Linux test environment. #1200 candidate: local GoToR links open in desktop tabs and through root-confined automation; appearance-only edits preserve existing actions. Named destinations remain unapplied and `/NewWindow` is a tab hint. Missing: editing existing text and images in place (the longest pole), image/PDF watermarks | +| M7 | Content editing (text, images, header/footer, watermark) | 250–500 | 20% | 100–180 | Done: header & footer, watermarks, backgrounds, Bates; added text and images that stay editable (move, resize, format, rotate, flip, crop, replace); links (Link tool, Link Properties, create from URLs, remove all). Done since: Edit text preserves Arabic/Hebrew `/ActualText` typing order, with font-independent regressions; single-glyph RTL ligature spans (including lam-alef) join their surrounding word. Done since (#1116): editable text regions grouped across compatible words and subset fonts, one rich-text editor over a background with the source text removed, embedded sfnt faces reused as editor faces, and visible (clipped) bounds for cropped-image selection and resize. Exclusive crops now resize across nested states and content streams, with shared clips fixed.
Done since: #594 ordinary paragraph drags preserve overlapping source text; UI/engine regressions, native save/reopen, independent-reader checks and full CI passed in the Linux test environment. #1200 candidate: local GoToR links open in desktop tabs and through root-confined automation; appearance-only edits preserve existing actions. Named destinations remain unapplied and `/NewWindow` is a tab hint. Missing: editing existing text and images in place (the longest pole), image/PDF watermarks Added-image automation reduces extreme signed rotation angles before combining them with the current orientation; synthetic automation/MCP tests cover integer bounds, history, pixels and save/reopen. | | M7 | Content editing (text, images, header/footer, watermark) | 250–500 | 20% | 100–180 | Done: header & footer, watermarks, backgrounds, Bates; added text and images that stay editable (move, resize, format, rotate, flip, crop, replace); links (Link tool, Link Properties, create from URLs, remove all). Done since: Edit text preserves Arabic/Hebrew `/ActualText` typing order, with font-independent regressions. Done since (#1116): editable text regions grouped across compatible words and subset fonts, one rich-text editor over a background with the source text removed, embedded sfnt faces reused as editor faces, and visible (clipped) bounds for cropped-image selection and resize. Exclusive crops now resize across nested states and content streams, with shared clips fixed.
Done since: #594 ordinary paragraph drags preserve overlapping source text; UI/engine regressions, native save/reopen, independent-reader checks and full CI passed in the Linux test environment. #1200 candidate: local GoToR links open in desktop tabs and through root-confined automation; appearance-only edits preserve existing actions. Named destinations remain unapplied and `/NewWindow` is a tab hint. #1316: replacing a line or paragraph keeps a neighbouring line whose glyph box overlaps when the baselines differ; a second copy on the same baseline is still replaced. Missing: editing existing text and images in place (the longest pole), image/PDF watermarks | | M7 | Content editing (text, images, header/footer, watermark) | 250–500 | 20% | 100–180 | Done: header & footer, watermarks, backgrounds, Bates; added text and images that stay editable (move, resize, format, rotate, flip, crop, replace); links (Link tool, Link Properties, create from URLs, remove all). Missing: editing existing text and images in place (the longest pole), watermark print/screen visibility and repeat | | M6 | Forms + JavaScript | 160–320 | 50% | 40–75 | Done: filling all field types with regenerated appearances; Prepare a form (every field type, multi-select/shared properties, move/resize/delete, Field Properties General/Appearance/Position/Options/Format/Validate/Calculate); Acrobat's AF format/keystroke/validate/calculate functions and simplified field notation run natively in Acrobat's event order; tab order; push-button actions; flatten; agent tools. Form data exchange (FDF, XFDF, XML, CSV, text) done; XFDF values retain text across XML comments and processing instructions.
Done since: JavaScript engine (boa, sandboxed) running custom keystroke/validate/calculate/format and button scripts, document JavaScripts, console. Missing: Actions tab, wider object model (annotations, layers, dialogs), XFA
Done since: Existing simple field-font encodings and Differences are respected when generating appearances; an unencodable value uses a separate Helvetica appearance font without changing the source font. Synthetic reopen regressions cover punctuation, subset fallback and multiline text. Exact font metrics/layout and broader Unicode fallback remain incomplete. | @@ -184,6 +185,7 @@ M2 (renderer), M7 (content editing), M10 (OCR and Office), M11 (Preflight and st | Date | Change | Summary | |---|---|---| +| 2026-10-18 | minor | M7: Added-image content_update reduces signed rotation angles before adding the current quarter turns. Extreme valid angles no longer panic or wrap to a wrong orientation. Synthetic automation and MCP regressions cover all starting orientations, Undo/Redo, invalid-angle preservation, rendered pixels and save/reopen. Estimates unchanged. | | 2026-10-15 | minor | M6: Plain AcroForm XML imports retain all direct text around comments and processing instructions, including CDATA and character references. Estimates unchanged. | | 2026-10-13 | minor | M6: XML/XFDF attribute whitespace survives form and comment export/import; estimates unchanged | | 2026-10-14 | minor | M1: raw-deflate fallback preserves decoding-limit errors in strict and tolerant decoding; synthetic filter and COS stream regressions. Estimates unchanged. | diff --git a/parity/acrobat-features.toml b/parity/acrobat-features.toml index 157e9e368c..e24c3e8a73 100644 --- a/parity/acrobat-features.toml +++ b/parity/acrobat-features.toml @@ -2105,9 +2105,9 @@ tier = "P0" milestone = "M7" status = "shipped" commands = ["edit.edit_text"] -tools = ["image_edit"] -evidence = ["crates/edit/src/tests.rs::page_images_move_turn_replace_and_delete", "crates/ui-egui/tests/editing.rs::editing_existing_images_on_the_page", "crates/automation/tests/automation.rs::editing_page_images_through_tools"] -notes = "existing raster images and grouped Form artwork: rotate either way about the centre, flip horizontally or vertically (a q/cm/Q wrapper); Forms stay intact as groups; added images too" +tools = ["image_edit", "content_update"] +evidence = ["crates/edit/src/tests.rs::page_images_move_turn_replace_and_delete", "crates/ui-egui/tests/editing.rs::editing_existing_images_on_the_page", "crates/automation/tests/automation.rs::editing_page_images_through_tools", "crates/automation/tests/automation.rs::large_added_image_rotations_keep_orientation_and_undo", "crates/automation/tests/automation.rs::large_added_image_rotations_round_trip_through_mcp"] +notes = "Existing raster images and grouped Form artwork: rotate either way about the centre, flip horizontally or vertically (a q/cm/Q wrapper); Forms stay intact as groups; added images too. Added-image content_update rotation accepts positive or negative integer multiples of 90 degrees; reducing the angle before addition prevents overflow at signed 64-bit limits and preserves the current orientation through full turns, Undo/Redo and save/reopen. Added-image rotations remain limited to 90-degree steps." [[feature]] id = "edit.image-move-resize" From 2d97486757ffab740ee4c68c08cf165236a847ff Mon Sep 17 00:00:00 2001 From: PerchAnton <101743951+PerchAnton@users.noreply.github.com> Date: Sun, 18 Oct 2026 16:42:17 +0300 Subject: [PATCH 05/20] M7: keep roadmap status blockquotes valid --- docs/roadmap.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/roadmap.md b/docs/roadmap.md index 1690d5ffb0..88f6e11dab 100644 --- a/docs/roadmap.md +++ b/docs/roadmap.md @@ -1,7 +1,7 @@ # PdfCraft roadmap: milestones and what's next > **Last reviewed:** 2026-10-18 · **Last updated:** 2026-10-18 · **Change:** minor (M7: added-image automation normalizes large rotation angles before addition; estimates unchanged) - +> > **Last reviewed:** 2026-10-18 · **Last updated:** 2026-10-18 · **Change:** minor (M3, #3149: discard render demand after page geometry changes and check the GPU texture limit before upload; estimates unchanged) > **Last reviewed:** 2026-10-18 · **Last updated:** 2026-10-18 · **Change:** minor (M8, #1350: named replacement descriptions retired during Sanitize and across shared resources; estimates unchanged) > **Last reviewed:** 2026-10-15 · **Last updated:** 2026-10-15 · **Change:** minor (M6: AcroForm XML text; estimates unchanged) From 8477810738d4829526e8ad1a1720aa6285762798 Mon Sep 17 00:00:00 2001 From: Michael Date: Mon, 19 Oct 2026 21:31:49 +0800 Subject: [PATCH 06/20] fix(edit): keep a superscript footnote marker in its paragraph's row (#3252) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A footnote marker drawn with Ts (text rise) and a smaller Tf size sits on the same visual row as the body text around it, but the row-joining baseline tolerance was scaled by the smaller of the two runs' sizes (20% of it) — far tighter than a typical superscript's rise, which runs up to about half the body size. The marker split off into its own line, and then its own block, fragmenting one paragraph into three separate editable zones in Edit PDF > Edit text and images. Scale the tolerance by the larger of the two sizes instead, which still leaves ordinary next-line leading (about 1.2x the font size) well outside it. Added a regression test in crates/edit/src/tests.rs that fails on main (reverted the fix locally and confirmed it goes red: 3 blocks instead of 1) and passes with the fix. cargo fmt --check, clippy -D warnings, cargo test -p pdfcraft-edit (186 passed), cargo xtask layers and cargo xtask assets all clean. Co-Authored-By: Claude Sonnet 5 --- crates/edit/src/tests.rs | 17 +++++++++++++++++ crates/edit/src/text_regions.rs | 6 +++++- 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/crates/edit/src/tests.rs b/crates/edit/src/tests.rs index f2def2fd6a..6c6390f63f 100644 --- a/crates/edit/src/tests.rs +++ b/crates/edit/src/tests.rs @@ -4975,3 +4975,20 @@ fn flattening_comments_keeps_annotations_that_are_not_comments() { let without_widget: Vec<&str> = all.iter().copied().filter(|s| *s != "Widget").collect(); assert_eq!(left(&reopen(&doc)), without_widget, "flattening fields touches no annotation that isn't a widget"); } + +/// A footnote marker (`Ts` text rise, a smaller `Tf` size) does not split the paragraph around it +/// into separate editable zones (#3252): on `main` this read as three blocks — the text up to the +/// marker, the marker alone, and the text after it — because the row-joining baseline tolerance +/// was scaled by the smaller of the two runs' sizes, far tighter than a typical superscript's rise. +#[test] +fn a_superscript_footnote_marker_stays_in_its_paragraphs_row() { + let doc = text_page( + "BT /F1 12 Tf 72 700 Td (and A.) Tj 0 -14 Td (Edward Siecienski) Tj 4 Ts /F1 7.2 Tf (3) Tj 0 Ts /F1 12 Tf (. More words continue the paragraph right after the marker.) Tj ET", + ); + let lines = text::text_lines(&doc, 0).unwrap(); + assert_eq!(lines.len(), 4, "the marker is its own line, raised and smaller: {lines:?}"); + let blocks = text::text_blocks(&doc, 0).unwrap(); + assert_eq!(blocks.len(), 1, "one paragraph, not split at the marker: {blocks:?}"); + assert_eq!(blocks[0].rows.len(), 2, "two visual rows, the second carrying the marker"); + assert_eq!(blocks[0].text, "and A. Edward Siecienski3. More words continue the paragraph right after the marker.", "{:?}", blocks[0]); +} diff --git a/crates/edit/src/text_regions.rs b/crates/edit/src/text_regions.rs index e08068d9f5..cb0af64d3f 100644 --- a/crates/edit/src/text_regions.rs +++ b/crates/edit/src/text_regions.rs @@ -71,7 +71,11 @@ fn group_layout(lines: &[TextLine], mixed: bool) -> Vec { scanned && (mixed || same_style(prev, line)) && same_plane(prev, line) - && (prev.frame_origin().1 - line.frame_origin().1).abs() < prev.size.min(line.size) * 0.2 + // A superscript or subscript run (a footnote marker, say) sits on the same visual + // row as the text around it, raised or lowered by `Ts` from the body baseline + // (#3252). That shift is typically up to half the body size, not the marker's own + // smaller size, so the tolerance is scaled by the larger of the two. + && (prev.frame_origin().1 - line.frame_origin().1).abs() < prev.size.max(line.size) * 0.5 && gap >= -prev.size.min(line.size) * 0.2 && gap < prev.size.max(line.size) * 1.5 // Separate same-style text objects that merely touch or overlap, with no space From fe2ece46757cfcb35bc5078d4ac3d8aed9860de2 Mon Sep 17 00:00:00 2001 From: Michael Date: Tue, 20 Oct 2026 01:48:31 +0800 Subject: [PATCH 07/20] fix(edit): keep the overlap guard for the wider footnote-marker tolerance MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit review on #3471: the widened baseline tolerance (half the larger run's size, for a same-row footnote marker) could also let two genuinely stacked rows of ordinary body text join into one row, if horizontal compression (Tz) narrows the first row enough to put the second row's start within it — a negative `gap` inside the existing overlap allowance, with a baseline shift inside the new 50% band but outside the original 20% one. Only take the wider tolerance when the two runs don't overlap horizontally (`gap >= 0.0`), which is true for a footnote marker drawn right after the text it follows. Overlapping runs keep the original, tighter 20% tolerance. Added stacked_rows_that_merely_overlap_do_not_join_into_one_row, confirmed it fails without this change (merges into one row/block) and passes with it. cargo test -p pdfcraft-edit (197 passed), fmt, clippy -D warnings, xtask layers and xtask assets all clean. Co-Authored-By: Claude Sonnet 5 --- crates/edit/src/tests.rs | 18 ++++++++++++++++++ crates/edit/src/text_regions.rs | 8 ++++++-- 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/crates/edit/src/tests.rs b/crates/edit/src/tests.rs index 9bc2ee365b..39627c08ce 100644 --- a/crates/edit/src/tests.rs +++ b/crates/edit/src/tests.rs @@ -5235,3 +5235,21 @@ fn a_clipping_copy_on_the_same_baseline_is_refused() { assert!(matches!(&block, Err(EditError::Invalid(m)) if m.contains("clipping path")), "{block:?}"); assert_eq!(text::text_lines(&doc, 0).unwrap().len(), 2, "refused means untouched"); } + +/// Two genuinely stacked rows of ordinary body text must not join into one row just because the +/// first is narrow enough to put the second's start within the #3252 tolerance band: here the +/// second row starts 1.4pt to the left of where the first ends (so the two overlap a little, +/// as compressed text or a tight column can) and 5pt below its baseline — inside the widened 50% +/// tolerance but outside the original 20% one. They stay two rows (and two paragraphs). +#[test] +fn stacked_rows_that_merely_overlap_do_not_join_into_one_row() { + let doc = text_page("BT /F1 12 Tf 72 700 Td (A compressed line of body text here) Tj 190 -5 Td (Second stacked line of body text) Tj ET"); + let lines = text::text_lines(&doc, 0).unwrap(); + assert_eq!(lines.len(), 2); + let gap = lines[1].rect[0] - lines[0].rect[2]; + assert!((-2.4..0.0).contains(&gap), "the rows overlap a little, within the old tolerance band: {gap}"); + let baseline_shift = lines[0].rect[3] - lines[1].rect[3]; + assert!((2.4..6.0).contains(&baseline_shift), "between the old and new vertical tolerance: {baseline_shift}"); + let blocks = text::text_blocks(&doc, 0).unwrap(); + assert_eq!(blocks.len(), 2, "two separate rows stay two paragraphs, not one: {blocks:?}"); +} diff --git a/crates/edit/src/text_regions.rs b/crates/edit/src/text_regions.rs index cb0af64d3f..0baccc4082 100644 --- a/crates/edit/src/text_regions.rs +++ b/crates/edit/src/text_regions.rs @@ -74,8 +74,12 @@ fn group_layout(lines: &[TextLine], mixed: bool) -> Vec { // A superscript or subscript run (a footnote marker, say) sits on the same visual // row as the text around it, raised or lowered by `Ts` from the body baseline // (#3252). That shift is typically up to half the body size, not the marker's own - // smaller size, so the tolerance is scaled by the larger of the two. - && (prev.frame_origin().1 - line.frame_origin().1).abs() < prev.size.max(line.size) * 0.5 + // smaller size — but only between runs with no horizontal overlap (`gap >= 0.0`): + // two stacked rows of ordinary body text can land within that wider tolerance too + // when horizontal compression (`Tz`) narrows the first row enough to overlap the + // second's start, and the tighter 20% tolerance is what keeps those apart. + && ((prev.frame_origin().1 - line.frame_origin().1).abs() < prev.size.min(line.size) * 0.2 + || (gap >= 0.0 && (prev.frame_origin().1 - line.frame_origin().1).abs() < prev.size.max(line.size) * 0.5)) && gap >= -prev.size.min(line.size) * 0.2 && gap < prev.size.max(line.size) * 1.5 // Separate same-style text objects that merely touch or overlap, with no space From c38cdbf0be84153ec2f6272becb8ed38666e7fd3 Mon Sep 17 00:00:00 2001 From: Michael Date: Tue, 20 Oct 2026 02:28:34 +0800 Subject: [PATCH 08/20] fix(edit): require a meaningful size difference for the wider tolerance CodeRabbit review on #3471: even with the gap >= 0.0 overlap guard, two equal-size rows with no horizontal overlap (not just overlapping ones) could still land inside the widened 50% baseline tolerance at a 5pt shift that the original 20% tolerance would have rejected, merging distinct body-text lines into one row. The wider tolerance is meant for a run distinctly smaller or larger than its neighbour (a super/subscript), not two ordinary same-size rows. Require prev.size.min(line.size) < prev.size.max(line.size) * 0.9 before taking it; equal-size runs keep the original 20% tolerance regardless of overlap. Added equal_size_rows_need_the_tighter_tolerance_even_without_overlap, confirmed it fails without this change (merges into one row/block) and passes with it, alongside the two existing regression tests from the earlier rounds of this fix. cargo test -p pdfcraft-edit (198 passed), fmt, clippy -D warnings, xtask layers and xtask assets all clean. Co-Authored-By: Claude Sonnet 5 --- crates/edit/src/tests.rs | 19 +++++++++++++++++++ crates/edit/src/text_regions.rs | 13 ++++++++----- 2 files changed, 27 insertions(+), 5 deletions(-) diff --git a/crates/edit/src/tests.rs b/crates/edit/src/tests.rs index 39627c08ce..602d0d83b0 100644 --- a/crates/edit/src/tests.rs +++ b/crates/edit/src/tests.rs @@ -5253,3 +5253,22 @@ fn stacked_rows_that_merely_overlap_do_not_join_into_one_row() { let blocks = text::text_blocks(&doc, 0).unwrap(); assert_eq!(blocks.len(), 2, "two separate rows stay two paragraphs, not one: {blocks:?}"); } + +/// Two equal-size rows with no horizontal overlap must not join into one row either (#3252): +/// the wider tolerance is for a run distinctly smaller or larger than its neighbour (a +/// super/subscript), not for two ordinary body-text rows that happen to sit close enough +/// vertically — here a 5pt baseline shift, inside the widened 50% tolerance but outside the +/// original 20% one, with the second row starting just past where the first ends. +#[test] +fn equal_size_rows_need_the_tighter_tolerance_even_without_overlap() { + let doc = text_page("BT /F1 12 Tf 72 700 Td (Short line one) Tj 75 -5 Td (Second line two here) Tj ET"); + let lines = text::text_lines(&doc, 0).unwrap(); + assert_eq!(lines.len(), 2); + assert_eq!(lines[0].size, lines[1].size, "same size: no super/subscript here"); + let gap = lines[1].rect[0] - lines[0].rect[2]; + assert!(gap >= 0.0, "no horizontal overlap: {gap}"); + let baseline_shift = lines[0].rect[3] - lines[1].rect[3]; + assert!((2.4..6.0).contains(&baseline_shift), "between the old and new vertical tolerance: {baseline_shift}"); + let blocks = text::text_blocks(&doc, 0).unwrap(); + assert_eq!(blocks.len(), 2, "two separate rows stay two paragraphs, not one: {blocks:?}"); +} diff --git a/crates/edit/src/text_regions.rs b/crates/edit/src/text_regions.rs index 0baccc4082..66c80fb06e 100644 --- a/crates/edit/src/text_regions.rs +++ b/crates/edit/src/text_regions.rs @@ -74,12 +74,15 @@ fn group_layout(lines: &[TextLine], mixed: bool) -> Vec { // A superscript or subscript run (a footnote marker, say) sits on the same visual // row as the text around it, raised or lowered by `Ts` from the body baseline // (#3252). That shift is typically up to half the body size, not the marker's own - // smaller size — but only between runs with no horizontal overlap (`gap >= 0.0`): - // two stacked rows of ordinary body text can land within that wider tolerance too - // when horizontal compression (`Tz`) narrows the first row enough to overlap the - // second's start, and the tighter 20% tolerance is what keeps those apart. + // smaller size — but the wider tolerance only applies between a run and a + // distinctly smaller or larger one, with no horizontal overlap (`gap >= 0.0`): + // two equal-size runs can land within it too, at a baseline shift tight columns + // or compressed text (`Tz`) can produce between genuinely separate rows, and the + // original 20% tolerance is what keeps those apart. && ((prev.frame_origin().1 - line.frame_origin().1).abs() < prev.size.min(line.size) * 0.2 - || (gap >= 0.0 && (prev.frame_origin().1 - line.frame_origin().1).abs() < prev.size.max(line.size) * 0.5)) + || (gap >= 0.0 + && prev.size.min(line.size) < prev.size.max(line.size) * 0.9 + && (prev.frame_origin().1 - line.frame_origin().1).abs() < prev.size.max(line.size) * 0.5)) && gap >= -prev.size.min(line.size) * 0.2 && gap < prev.size.max(line.size) * 1.5 // Separate same-style text objects that merely touch or overlap, with no space From 5beaf88a2aa64f66a9e8aba982fea377cdce5d90 Mon Sep 17 00:00:00 2001 From: Hazel Granados Date: Mon, 19 Oct 2026 23:42:25 -0500 Subject: [PATCH 09/20] M7: paint a new background over older ones (#3591) Marks behind the page content were all inserted at the start of /Contents, so a second background went under the first and the old colour stayed. Backgrounds now come first, then behind-content watermarks, each in the order added. A watermark added after a background no longer disappears under it. --- ROADMAP.md | 2 ++ crates/edit/src/lib.rs | 22 ++++++++++++++-------- crates/edit/src/tests.rs | 27 +++++++++++++++++++++++++++ 3 files changed, 43 insertions(+), 8 deletions(-) diff --git a/ROADMAP.md b/ROADMAP.md index b1283e7751..ee1dc5823c 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -172,6 +172,8 @@ Ranked; detail and the full milestone table in [docs/roadmap.md](docs/roadmap.md ## Progress log +- **2026-10-19 (M7, #3591):** A background added to a page that already has one is painted over the old one instead of going under it and staying hidden. Behind the content, backgrounds come first and then behind-content watermarks, each in the order added, so a watermark added after a background no longer disappears. Synthetic regressions in `crates/edit` check the order. Estimates unchanged. + - **2026-10-19 (M14, #3373):** The main menu and its long submenus scroll in short windows, keeping their commands reachable. Estimates unchanged. - **2026-10-19 (M3, #3387):** Save-prompt shortcuts yield to later consent and utility dialogs; a synthetic UI regression checks that an edited document stays open. Estimates unchanged. diff --git a/crates/edit/src/lib.rs b/crates/edit/src/lib.rs index c1062b4105..0bd0ab5691 100644 --- a/crates/edit/src/lib.rs +++ b/crates/edit/src/lib.rs @@ -505,8 +505,8 @@ fn isolation(doc: &Document, list: &[Object]) -> Isolation { Isolation::Clean { suffix: (dirty || comment_open || marked != 0).then_some(split), wrappers, marked } } -/// Put a mark's content on a page: behind (prepended) or on top (appended, after wrapping the -/// original content in q/Q). +/// Put a mark's content on a page: behind (with the behind-marks at the start) or on top +/// (appended, after wrapping the original content in q/Q). fn place(doc: &mut Document, page: &pdfcraft_model::Page, kind: MarkKind, content: Vec, behind: bool) -> Result<(), EditError> { place_tagged(doc, page, kind.tag(), content, behind) } @@ -518,6 +518,9 @@ fn place(doc: &mut Document, page: &pdfcraft_model::Page, kind: MarkKind, conten /// sequences are closed at the end too, so hidden optional content cannot hide the new mark. fn place_tagged(doc: &mut Document, page: &pdfcraft_model::Page, tag: &str, content: Vec, behind: bool) -> Result<(), EditError> { let mut list = contents(doc, page)?; + // The behind-marks at the start: backgrounds, then watermarks, each in the order added. + let behind_kinds = [MarkKind::Background.tag(), MarkKind::Watermark.tag()]; + let behind_marks = list.iter().take_while(|o| stream_tag(doc, o).is_some_and(|t| behind_kinds.contains(&t.as_str()))).count(); let wrap = if behind { None } else { @@ -526,16 +529,19 @@ fn place_tagged(doc: &mut Document, page: &pdfcraft_model::Page, tag: &str, cont // Content that can't be read precisely is wrapped whole, as before: after the // behind-marks at the start. A `q` it leaves open is closed at its end, where // closing it changes nothing already drawn. - Isolation::Unknown { unclosed, marked } => { - let behind_kinds = [MarkKind::Background.tag(), MarkKind::Watermark.tag()]; - let split = list.iter().position(|o| !stream_tag(doc, o).is_some_and(|t| behind_kinds.contains(&t.as_str()))).unwrap_or(list.len()); - (split < list.len()).then_some((split, unclosed, marked)) - } + Isolation::Unknown { unclosed, marked } => (behind_marks < list.len()).then_some((behind_marks, unclosed, marked)), } }; let mark = Object::Ref(doc.add(Object::Stream(Stream::flate(tagged(tag), &content)))); if behind { - list.insert(0, mark); + // A new background goes after the backgrounds already there and below any watermark; + // a new watermark after all of them. Each is painted over the older ones (#3591). + let at = if tag == MarkKind::Background.tag() { + list.iter().take(behind_marks).rposition(|o| stream_tag(doc, o).as_deref() == Some(tag)).map_or(0, |i| i + 1) + } else { + behind_marks + }; + list.insert(at, mark); } else { if let Some((split, unclosed, marked)) = wrap { let mut close_bytes = b"\nEMC".repeat(marked); diff --git a/crates/edit/src/tests.rs b/crates/edit/src/tests.rs index 879ebc6c66..bd506d3b97 100644 --- a/crates/edit/src/tests.rs +++ b/crates/edit/src/tests.rs @@ -168,6 +168,33 @@ fn watermarks_rotate_fade_and_can_go_behind() { assert_eq!(s.len(), 2, "no wrapper needed for content behind"); } +#[test] +fn a_second_background_is_drawn_over_the_first() { + // #3591: the new background went in front of the old one in /Contents, so the old colour + // was painted over it and stayed. + let mut doc = fixture(); + add_background(&mut doc, &[0], &Background { color: [0.0, 0.0, 1.0], opacity: 1.0, ..Background::default() }, false).unwrap(); + add_background(&mut doc, &[0], &Background { color: [0.0, 1.0, 0.0], opacity: 1.0, ..Background::default() }, false).unwrap(); + let s = streams(&doc, 0); + assert_eq!(s.len(), 3, "{s:?}"); + assert!(s[0].contains("0 0 1 rg") && s[1].contains("0 1 0 rg"), "the newer background is painted last: {s:?}"); + assert!(s[2].contains("(Hi) Tj"), "both stay behind the content: {s:?}"); +} + +#[test] +fn backgrounds_stay_below_watermarks_behind_the_content() { + // Behind the content, backgrounds come first and then watermarks, each in the order added, + // so a watermark added after a background shows and a later background doesn't cover it. + let mut doc = fixture(); + add_background(&mut doc, &[0], &Background { color: [0.0, 0.0, 1.0], opacity: 1.0, ..Background::default() }, false).unwrap(); + add_watermark(&mut doc, &[0], &Watermark { text: "DRAFT".into(), behind: true, ..Watermark::default() }, false).unwrap(); + add_background(&mut doc, &[0], &Background { color: [0.0, 1.0, 0.0], opacity: 1.0, ..Background::default() }, false).unwrap(); + let s = streams(&doc, 0); + assert_eq!(s.len(), 4, "{s:?}"); + assert!(s[0].contains("0 0 1 rg") && s[1].contains("0 1 0 rg"), "backgrounds first, oldest first: {s:?}"); + assert!(s[2].contains("/PCMark /Watermark") && s[3].contains("(Hi) Tj"), "then the watermark, then the content: {s:?}"); +} + #[test] fn watermark_offset_moves_the_centre_right_and_up() { let mut doc = fixture(); From e9c53ea71883b260794a4bcebc05c6a471b9a4d7 Mon Sep 17 00:00:00 2001 From: Hazel Granados Date: Tue, 20 Oct 2026 00:09:41 -0500 Subject: [PATCH 10/20] M7: keep a behind watermark under one on top on an empty page (#3591) On a page with no content of its own, an on-top watermark is the first stream in /Contents, so it was counted with the behind-content marks and a watermark added behind later was drawn over it. Nothing records whether a watermark is behind, so a new behind mark now goes right after the last background, which is always below any on-top watermark. A newer behind watermark goes under older ones again, as before #3591. --- ROADMAP.md | 2 +- crates/edit/src/lib.rs | 19 +++++++++---------- crates/edit/src/tests.rs | 16 ++++++++++++++-- 3 files changed, 24 insertions(+), 13 deletions(-) diff --git a/ROADMAP.md b/ROADMAP.md index ee1dc5823c..6b6607df88 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -172,7 +172,7 @@ Ranked; detail and the full milestone table in [docs/roadmap.md](docs/roadmap.md ## Progress log -- **2026-10-19 (M7, #3591):** A background added to a page that already has one is painted over the old one instead of going under it and staying hidden. Behind the content, backgrounds come first and then behind-content watermarks, each in the order added, so a watermark added after a background no longer disappears. Synthetic regressions in `crates/edit` check the order. Estimates unchanged. +- **2026-10-19 (M7, #3591):** A background added to a page that already has one is painted over the old one instead of going under it and staying hidden. Behind the content, backgrounds come first, in the order added, and then behind-content watermarks, so a watermark added after a background no longer disappears. Synthetic regressions in `crates/edit` check the order. Estimates unchanged. - **2026-10-19 (M14, #3373):** The main menu and its long submenus scroll in short windows, keeping their commands reachable. Estimates unchanged. diff --git a/crates/edit/src/lib.rs b/crates/edit/src/lib.rs index 0bd0ab5691..850838cc24 100644 --- a/crates/edit/src/lib.rs +++ b/crates/edit/src/lib.rs @@ -2,8 +2,9 @@ //! Watermark, Background and Bates numbering (execution plan M7.6). //! //! **How marks are stored.** Each mark is one content stream appended to (or, for content behind -//! the page, prepended to) the page's `/Contents`, drawn in display space (after `/Rotate`) and -//! wrapped in a marked-content artifact (§14.8.2.2) so readers and screen readers skip it: +//! the page, inserted after the backgrounds at the start of) the page's `/Contents`, drawn in +//! display space (after `/Rotate`) and wrapped in a marked-content artifact (§14.8.2.2) so +//! readers and screen readers skip it: //! `/Artifact <> BDC … EMC`. The //! `/PCMark` entry (also in the stream dictionary) names the kind of mark, which is how Update and //! Remove find them again without decompressing page content. When @@ -518,7 +519,7 @@ fn place(doc: &mut Document, page: &pdfcraft_model::Page, kind: MarkKind, conten /// sequences are closed at the end too, so hidden optional content cannot hide the new mark. fn place_tagged(doc: &mut Document, page: &pdfcraft_model::Page, tag: &str, content: Vec, behind: bool) -> Result<(), EditError> { let mut list = contents(doc, page)?; - // The behind-marks at the start: backgrounds, then watermarks, each in the order added. + // The behind-marks at the start: backgrounds in the order added, then watermarks. let behind_kinds = [MarkKind::Background.tag(), MarkKind::Watermark.tag()]; let behind_marks = list.iter().take_while(|o| stream_tag(doc, o).is_some_and(|t| behind_kinds.contains(&t.as_str()))).count(); let wrap = if behind { @@ -534,13 +535,11 @@ fn place_tagged(doc: &mut Document, page: &pdfcraft_model::Page, tag: &str, cont }; let mark = Object::Ref(doc.add(Object::Stream(Stream::flate(tagged(tag), &content)))); if behind { - // A new background goes after the backgrounds already there and below any watermark; - // a new watermark after all of them. Each is painted over the older ones (#3591). - let at = if tag == MarkKind::Background.tag() { - list.iter().take(behind_marks).rposition(|o| stream_tag(doc, o).as_deref() == Some(tag)).map_or(0, |i| i + 1) - } else { - behind_marks - }; + // Right after the backgrounds already there, so a new background is painted over the + // older ones (#3591) and a watermark over all of them. Not after the watermarks: on a + // page with no content, one on top is at the start too and can't be told apart, so a + // newer watermark behind goes under the older ones. + let at = list.iter().take(behind_marks).rposition(|o| stream_tag(doc, o).as_deref() == Some(MarkKind::Background.tag())).map_or(0, |i| i + 1); list.insert(at, mark); } else { if let Some((split, unclosed, marked)) = wrap { diff --git a/crates/edit/src/tests.rs b/crates/edit/src/tests.rs index bd506d3b97..a95d7bfe0a 100644 --- a/crates/edit/src/tests.rs +++ b/crates/edit/src/tests.rs @@ -183,8 +183,8 @@ fn a_second_background_is_drawn_over_the_first() { #[test] fn backgrounds_stay_below_watermarks_behind_the_content() { - // Behind the content, backgrounds come first and then watermarks, each in the order added, - // so a watermark added after a background shows and a later background doesn't cover it. + // Behind the content, backgrounds come first, in the order added, and then watermarks, so a + // watermark added after a background shows and a later background doesn't cover it. let mut doc = fixture(); add_background(&mut doc, &[0], &Background { color: [0.0, 0.0, 1.0], opacity: 1.0, ..Background::default() }, false).unwrap(); add_watermark(&mut doc, &[0], &Watermark { text: "DRAFT".into(), behind: true, ..Watermark::default() }, false).unwrap(); @@ -195,6 +195,18 @@ fn backgrounds_stay_below_watermarks_behind_the_content() { assert!(s[2].contains("/PCMark /Watermark") && s[3].contains("(Hi) Tj"), "then the watermark, then the content: {s:?}"); } +#[test] +fn a_watermark_behind_goes_under_one_on_top_of_a_page_without_content() { + // With no content of its own, the page's watermark on top is its first stream, which looks + // like a behind-mark; a watermark added behind later must still be painted under it. + let mut doc = fixture(); + add_watermark(&mut doc, &[2], &Watermark { text: "TOP".into(), ..Watermark::default() }, false).unwrap(); + add_watermark(&mut doc, &[2], &Watermark { text: "BEHIND".into(), behind: true, ..Watermark::default() }, false).unwrap(); + let s = streams(&doc, 2); + assert_eq!(s.len(), 2, "{s:?}"); + assert!(s[0].contains("(BEHIND) Tj") && s[1].contains("(TOP) Tj"), "the watermark behind is painted first: {s:?}"); +} + #[test] fn watermark_offset_moves_the_centre_right_and_up() { let mut doc = fixture(); From 631ca00bdd4ad890f7ab8f1e2a115a6b9d8aa3ee Mon Sep 17 00:00:00 2001 From: Rahul Ranjan <11793455+rahul05ranjan@users.noreply.github.com> Date: Tue, 20 Oct 2026 14:50:30 +0530 Subject: [PATCH 11/20] The image scan skips a path segment before any m instead of panicking (#3581) A content stream that uses l, c, v, y or h before any m or re has no subpath to extend. The edit-side image scan (crates/edit/src/images.rs) handed the operators straight to BezPath's line_to/curve_to/close_path, which panic on an empty path by contract, so object_list, page_images, image_hit and the Office export's image walk all crashed on such a page, while the renderer, text extraction, MuPDF and pdftotext skip the malformed segment and render the rest. The scan now skips the segment when the path has no elements, mirroring the v arm's existing guard and kurbo's own condition (debug_assert on !is_empty). A path started with m or re extends and closes exactly as before. A clip set by a stray segment (W n on an empty path) takes a zero bounding box, which kurbo defines as Rect::default, not a panic. Regression test: each of the five segment operators before any m, then a well-formed m l h and a re W n clip, all before an image Do - the image is still listed in every case. Verified against kurbo 0.13.1's source (line_to/curve_to/close_path debug_assert on empty; bounding_box unwrap_or_default) and the issue's repro; no Rust toolchain on this machine, so cargo test was not run here - saying so plainly for the reviewer. Co-authored-by: rahul05ranjan <11793455+rahul05ranjan@users.noreply.github.com> --- crates/edit/src/images.rs | 22 ++++++++++++++++++---- crates/edit/src/tests.rs | 19 +++++++++++++++++++ 2 files changed, 37 insertions(+), 4 deletions(-) diff --git a/crates/edit/src/images.rs b/crates/edit/src/images.rs index e90d8e4f15..6c67bf917c 100644 --- a/crates/edit/src/images.rs +++ b/crates/edit/src/images.rs @@ -165,12 +165,19 @@ impl ImageState { } } b"l" => { - if let Some([x, y]) = op.nums::<2>() { + // A segment before any m (or re) has no subpath to extend: BezPath's line_to and + // friends panic on an empty path by contract, so the segment is skipped, as the + // renderer, MuPDF and pdftotext do (#3581). + if let Some([x, y]) = op.nums::<2>() + && self.path.elements().last().is_some() + { self.path.line_to(self.point(x, y)); } } b"c" => { - if let Some([x1, y1, x2, y2, x3, y3]) = op.nums::<6>() { + if let Some([x1, y1, x2, y2, x3, y3]) = op.nums::<6>() + && self.path.elements().last().is_some() + { self.path.curve_to(self.point(x1, y1), self.point(x2, y2), self.point(x3, y3)); } } @@ -182,12 +189,19 @@ impl ImageState { } } b"y" => { - if let Some([x1, y1, x2, y2]) = op.nums::<4>() { + if let Some([x1, y1, x2, y2]) = op.nums::<4>() + && self.path.elements().last().is_some() + { let end = self.point(x2, y2); self.path.curve_to(self.point(x1, y1), end, end); } } - b"h" => self.path.close_path(), + b"h" => { + // ClosePath with no subpath panics by the same contract (#3581). + if self.path.elements().last().is_some() { + self.path.close_path(); + } + } b"re" => { if let Some([x, y, w, h]) = op.nums::<4>() { self.path.move_to(self.point(x, y)); diff --git a/crates/edit/src/tests.rs b/crates/edit/src/tests.rs index 879ebc6c66..9bd3b86113 100644 --- a/crates/edit/src/tests.rs +++ b/crates/edit/src/tests.rs @@ -1964,6 +1964,25 @@ fn image_page_with(content: &str) -> Document { Document::open(Arc::new(out)).unwrap() } +/// A path segment operator before any `m` (or `re`) has no subpath to extend: the image scan used +/// to call `BezPath`'s `line_to`/`curve_to`/`close_path` on an empty path, which panics by contract, +/// taking `object_list`, `page_images` and the Office export's image walk down with it (#3581). +/// The segment is now skipped, as the renderer, MuPDF and pdftotext do, and the page's images are +/// still listed. +#[test] +fn image_scan_skips_a_path_segment_before_any_m() { + for bad in ["1 1 l S", "1 1 2 2 3 3 c S", "1 1 2 2 4 4 y S", "1 1 2 2 3 3 v S", "h S"] { + let content = format!("q {bad} Q 100 100 200 100 cm /Im0 Do"); + let doc = image_page_with(&content); + assert_eq!(images::page_images(&doc, 0).unwrap().len(), 1, "{bad}: the image is still listed"); + } + // A path started with m (or re) still extends and closes as before. + let doc = image_page_with("q 0 0 m 1 1 l h S Q 100 100 200 100 cm /Im0 Do"); + assert_eq!(images::page_images(&doc, 0).unwrap().len(), 1); + let doc = image_page_with("q 10 10 20 20 re W n Q 100 100 200 100 cm /Im0 Do"); + assert_eq!(images::page_images(&doc, 0).unwrap().len(), 1); +} + /// The box of the first `re` on the page in user space, after the `cm`s before it. fn first_rect_in_user_space(doc: &Document) -> [f64; 4] { let mut ctm = pdfcraft_content::Matrix::IDENTITY; From fecfc774f256ede59f33f722b112b54f3f503ab9 Mon Sep 17 00:00:00 2001 From: Lombardi2003 Date: Wed, 21 Oct 2026 00:48:48 +0200 Subject: [PATCH 12/20] feat(edit): Header & Footer can be drawn in Times or Courier (#3754) Headers and footers were Helvetica only. HeaderFooter takes the same standard-face Family as Add text: the content selects /PCHelv, /PCTimes or /PCCour, the page gains that font, and centred and right-aligned text is placed by the chosen face's standard-14 widths. doc_header_footer takes font, and the dialog's fixed "Font: Helvetica" label becomes a Helvetica / Times / Courier menu. Co-Authored-By: Claude Opus 5.5 --- crates/automation/src/lib.rs | 8 ++++++ crates/automation/src/tools.rs | 3 ++- crates/automation/tests/automation.rs | 14 +++++++++++ crates/edit/src/lib.rs | 29 ++++++++++++++++++---- crates/edit/src/tests.rs | 34 ++++++++++++++++++++++++++ crates/ui-egui/src/marks_ui.rs | 15 +++++++++--- crates/ui-egui/tests/ui_group/marks.rs | 27 ++++++++++++++++++++ parity/acrobat-features.toml | 2 +- 8 files changed, 122 insertions(+), 10 deletions(-) diff --git a/crates/automation/src/lib.rs b/crates/automation/src/lib.rs index 0e2e0dd698..3d52d0ff24 100644 --- a/crates/automation/src/lib.rs +++ b/crates/automation/src/lib.rs @@ -1199,6 +1199,14 @@ impl Automation { if let Some(s) = a.opt_num("font_size")? { hf.font_size = s; } + hf.font = match a.opt_str("font")? { + None | Some("Helvetica") => pdfcraft_engine::FontFamily::Helvetica, + Some("Times") => pdfcraft_engine::FontFamily::Times, + Some("Courier") => pdfcraft_engine::FontFamily::Courier, + Some(other) => { + return Err(ToolError::InvalidArgs(format!("font must be \"Helvetica\", \"Times\" or \"Courier\", not {other:?}"))); + } + }; hf.color = color("color", hf.color)?; if let Some(m) = a.get("margins").and_then(Value::as_array) { let m: Vec = m.iter().filter_map(Value::as_f64).collect(); diff --git a/crates/automation/src/tools.rs b/crates/automation/src/tools.rs index e154e1eebc..f5130437c9 100644 --- a/crates/automation/src/tools.rs +++ b/crates/automation/src/tools.rs @@ -869,7 +869,7 @@ pub fn tools() -> Vec { t( "doc_header_footer", "Add header and footer", - "Add a header and/or footer to pages. Text boxes: header_left/center/right, footer_left/center/right. Tokens: <<1>> page number, <> page count, <<1 of n>>, <>, <<1/n>>, dates <> <> <> (and more), Bates <>. replace: true swaps out existing ones (Update). Undoable.", + "Add a header and/or footer to pages. Text boxes: header_left/center/right, footer_left/center/right. Tokens: <<1>> page number, <> page count, <<1 of n>>, <>, <<1/n>>, dates <> <> <> (and more), Bates <>. font: Helvetica (default), Times or Courier. replace: true swaps out existing ones (Update). Undoable.", ) .cmd("edit.header_footer") .with(schema( @@ -878,6 +878,7 @@ pub fn tools() -> Vec { "pages": pages("to mark (default: all)"), "header_left": { "type": "string" }, "header_center": { "type": "string" }, "header_right": { "type": "string" }, "footer_left": { "type": "string" }, "footer_center": { "type": "string" }, "footer_right": { "type": "string" }, + "font": { "type": "string", "enum": ["Helvetica", "Times", "Courier"] }, "font_size": { "type": "number", "exclusiveMinimum": 0 }, "color": { "type": "string", "description": "#RRGGBB or a colour name." }, "margins": { "type": "array", "items": { "type": "number", "minimum": 0 }, "minItems": 4, "maxItems": 4, "description": "Top, bottom, left, right in points (default 36, 36, 72, 72)." }, diff --git a/crates/automation/tests/automation.rs b/crates/automation/tests/automation.rs index cbf40f1962..9bbb5543c3 100644 --- a/crates/automation/tests/automation.rs +++ b/crates/automation/tests/automation.rs @@ -2239,6 +2239,20 @@ fn headers_watermarks_and_backgrounds_through_tools() { assert!(matches!(a.call("doc_remove_marks", &json!({ "doc": doc, "kind": "watermark" })), Err(ToolError::Failed(_)))); assert!(matches!(a.call("doc_header_footer", &json!({ "doc": doc })), Err(ToolError::Failed(_))), "no text"); ok(&mut a, "doc_save", json!({ "doc": doc, "path": "marked.pdf" })); + // A standard face other than Helvetica (#3754), kept through save and reopen; one it doesn't + // offer is refused with the faces it does. + let mono = ok(&mut a, "doc_open", json!({ "path": "b.pdf" }))["doc"].as_u64().unwrap(); + ok(&mut a, "doc_header_footer", json!({ "doc": mono, "header_left": "MONO", "font": "Courier" })); + assert!(page_text(&mut a, mono)[0].contains("MONO")); + ok(&mut a, "doc_save", json!({ "doc": mono, "path": "courier.pdf" })); + let saved = pdfcraft_cos::Document::open(std::sync::Arc::new(std::fs::read(dir.join("courier.pdf")).unwrap())).unwrap(); + let page = &pdfcraft_model::pages(&saved)[0]; + let res = saved.resolve(page.dict.get(b"Resources").unwrap()); + let fonts = saved.resolve(res.as_dict().unwrap().get(b"Font").unwrap()).as_dict().cloned().unwrap(); + let face = saved.resolve(fonts.get(b"PCCour").expect("Courier is added")).as_dict().cloned().unwrap(); + assert_eq!(face.name(b"BaseFont"), Some(&b"Courier"[..])); + let refused = a.call("doc_header_footer", &json!({ "doc": mono, "header_left": "X", "font": "Arial" })); + assert!(matches!(refused, Err(ToolError::InvalidArgs(e)) if e.contains("Courier"))); } #[test] diff --git a/crates/edit/src/lib.rs b/crates/edit/src/lib.rs index c1062b4105..90d8f24cdd 100644 --- a/crates/edit/src/lib.rs +++ b/crates/edit/src/lib.rs @@ -11,7 +11,8 @@ //! so its graphics state cannot reach the mark. The closing stream also ends known unfinished //! marked-content sequences before the new mark. //! -//! Text uses standard Helvetica (WinAnsi) added to the page resources as `/PCHelv`. +//! Text uses standard Helvetica (WinAnsi) added to the page resources as `/PCHelv`; headers and +//! footers may use standard Times (`/PCTimes`) or Courier (`/PCCour`) instead (#3754). #![deny(clippy::unwrap_used, clippy::expect_used, clippy::panic, clippy::unimplemented, clippy::todo, clippy::unreachable)] @@ -58,6 +59,8 @@ pub struct HeaderFooter { /// `<>`, `<>`, and Bates numbers `<>` /// (digits, start, prefix, suffix). pub text: [String; 6], + /// The standard face the text is drawn in (#3754): Helvetica (the default), Times or Courier. + pub font: added::Family, pub font_size: f64, pub color: Rgb, pub underline: bool, @@ -70,7 +73,15 @@ pub struct HeaderFooter { impl Default for HeaderFooter { /// Acrobat's defaults: 8 pt black, margins 0.5 in top and bottom, 1 in left and right. fn default() -> Self { - Self { text: Default::default(), font_size: 8.0, color: [0.0; 3], underline: false, margins: [36.0, 36.0, 72.0, 72.0], start_number: 1 } + Self { + text: Default::default(), + font: added::Family::Helvetica, + font_size: 8.0, + color: [0.0; 3], + underline: false, + margins: [36.0, 36.0, 72.0, 72.0], + start_number: 1, + } } } @@ -645,7 +656,13 @@ pub fn add_header_footer(doc: &mut Document, pages: &[usize], hf: &HeaderFooter, let bates = bates0.checked_add(k as u64).ok_or_else(|| EditError::Invalid("the Bates numbers are too large".into()))?; let size = hf.font_size; let [top, bottom, left, right] = hf.margins; - let mut body: Vec = format!("BT\n/PCHelv {} Tf\n{}\n", n(size), rgb(hf.color)).into_bytes(); + // The page resource each face is added under (see `add_resources`). + let resource = match hf.font { + added::Family::Helvetica => "PCHelv", + added::Family::Times => "PCTimes", + added::Family::Courier => "PCCour", + }; + let mut body: Vec = format!("BT\n/{resource} {} Tf\n{}\n", n(size), rgb(hf.color)).into_bytes(); let mut underlines = String::new(); for (slot, template) in hf.text.iter().enumerate() { if template.trim().is_empty() { @@ -655,7 +672,8 @@ pub fn add_header_footer(doc: &mut Document, pages: &[usize], hf: &HeaderFooter, let header = slot < 3; let line_h = size * 1.2; for (li, line) in lines.iter().enumerate() { - let tw = helvetica_width(line, size); + // Centred and right-aligned text is placed by its width in the chosen face. + let tw = hf.font.width(line, size, false, false); let x = match slot % 3 { 0 => left, 1 => (w - tw) / 2.0, @@ -676,7 +694,8 @@ pub fn add_header_footer(doc: &mut Document, pages: &[usize], hf: &HeaderFooter, let mut content = begin(MarkKind::HeaderFooter, "Header", page.view_matrix(doc)).into_bytes(); content.extend(body); content.extend_from_slice(END.as_bytes()); - add_resources(doc, page, None, None)?; + // The content names the face, so Times or Courier is added beside Helvetica. + add_resources(doc, page, None, Some(&content))?; let page = &page_list(doc)[i]; place(doc, page, MarkKind::HeaderFooter, content, false)?; } diff --git a/crates/edit/src/tests.rs b/crates/edit/src/tests.rs index 879ebc6c66..7e16f22ce0 100644 --- a/crates/edit/src/tests.rs +++ b/crates/edit/src/tests.rs @@ -97,6 +97,40 @@ fn header_and_footer_are_drawn_in_display_space_and_wrap_the_original_content() assert_eq!(marks_present(&doc), [MarkKind::HeaderFooter]); } +#[test] +fn header_and_footer_are_drawn_in_the_chosen_standard_face() { + // #3754: Times or Courier instead of Helvetica. The content names the face, the page's fonts + // gain it, and right-aligned text is placed by that face's widths (not Helvetica's). + let text = "Wide words"; + for (font, resource, base) in [(added::Family::Times, "PCTimes", "Times-Roman"), (added::Family::Courier, "PCCour", "Courier")] { + let mut doc = fixture(); + let hf = HeaderFooter { + text: [String::new(), String::new(), text.into(), String::new(), String::new(), String::new()], + font, + ..HeaderFooter::default() + }; + add_header_footer(&mut doc, &[0], &hf, false, &cx()).unwrap(); + let doc = reopen(&doc); + let mark = streams(&doc, 0).last().cloned().unwrap(); + assert!(mark.contains(&format!("/{resource} 8 Tf")), "{base}: {mark}"); + let x = 600.0 - 72.0 - font.width(text, 8.0, false, false); + assert!((x - (600.0 - 72.0 - pdfcraft_fonts::helvetica_width(text, 8.0))).abs() > 1.0, "{base}: the faces differ in width"); + assert!(mark.contains(&format!("1 0 0 1 {} ", n(x))), "{base}: right-aligned by its own widths: {mark}"); + let page = &pdfcraft_model::pages(&doc)[0]; + let res = doc.resolve(page.dict.get(b"Resources").unwrap()); + let fonts = doc.resolve(res.as_dict().unwrap().get(b"Font").unwrap()).as_dict().cloned().unwrap(); + let face = doc.resolve(fonts.get(resource.as_bytes()).unwrap()).as_dict().cloned().unwrap(); + assert_eq!(face.name(b"BaseFont"), Some(base.as_bytes()), "{base}"); + } + // The default is still Helvetica, and only Helvetica is added. + let mut doc = fixture(); + let hf = + HeaderFooter { text: [text.into(), String::new(), String::new(), String::new(), String::new(), String::new()], ..HeaderFooter::default() }; + add_header_footer(&mut doc, &[0], &hf, false, &cx()).unwrap(); + let mark = streams(&reopen(&doc), 0).last().cloned().unwrap(); + assert!(mark.contains("/PCHelv 8 Tf") && !mark.contains("/PCTimes") && !mark.contains("/PCCour"), "{mark}"); +} + #[test] fn header_start_number_that_would_wrap_is_refused_without_drawing() { // u32::MAX is a valid start_number on its own, but there is no room for a second page diff --git a/crates/ui-egui/src/marks_ui.rs b/crates/ui-egui/src/marks_ui.rs index d91a69f937..bc84aa1588 100644 --- a/crates/ui-egui/src/marks_ui.rs +++ b/crates/ui-egui/src/marks_ui.rs @@ -3,7 +3,7 @@ //! preview of the current page. "Update" opens the same dialog in replace mode. use egui::{Align, Color32, CornerRadius, Layout, Rect, Stroke, pos2, vec2}; -use pdfcraft_engine::{Background, Edit, HeaderFooter, MarkKind, Watermark}; +use pdfcraft_engine::{Background, Edit, FontFamily, HeaderFooter, MarkKind, Watermark}; use crate::theme::{self, Tokens}; use crate::{PdfCraftApp, widgets}; @@ -160,7 +160,13 @@ pub(crate) fn body(ui: &mut egui::Ui, app: &mut PdfCraftApp, t: &Tokens, kind: M match kind { MarkKind::HeaderFooter => { ui.horizontal(|ui| { - ui.label(tl!("Font: Helvetica")); + // The standard faces a header can be drawn in (#3754). + ui.label(tl!("Font:")); + egui::ComboBox::from_id_salt("hf-font").selected_text(d.hf.font.label()).width(100.0).show_ui(ui, |ui| { + for f in [FontFamily::Helvetica, FontFamily::Times, FontFamily::Courier] { + ui.selectable_value(&mut d.hf.font, f, f.label()); + } + }); ui.label(tl!("Size")); ui.add(egui::DragValue::new(&mut d.hf.font_size).range(4.0..=72.0).speed(0.5)); ui.add(widgets::checkbox(&mut d.hf.underline, tl!("Underline"))); @@ -322,7 +328,10 @@ pub(crate) fn body(ui: &mut egui::Ui, app: &mut PdfCraftApp, t: &Tokens, kind: M 1 => (r.center().x, egui::Align2::CENTER_CENTER), _ => (r.right() - hf.margins[3] as f32 * s, egui::Align2::RIGHT_CENTER), }; - p.text(pos2(x, y), align, shown, egui::FontId::proportional((hf.font_size as f32 * s).max(5.0)), rgb32(hf.color)); + // Courier previews in the monospaced UI face, the others proportional. + let size = (hf.font_size as f32 * s).max(5.0); + let font = if hf.font == FontFamily::Courier { egui::FontId::monospace(size) } else { egui::FontId::proportional(size) }; + p.text(pos2(x, y), align, shown, font, rgb32(hf.color)); } } MarkKind::Watermark if file_name.is_some() => { diff --git a/crates/ui-egui/tests/ui_group/marks.rs b/crates/ui-egui/tests/ui_group/marks.rs index 1f1515ad60..e0d86c4ab3 100644 --- a/crates/ui-egui/tests/ui_group/marks.rs +++ b/crates/ui-egui/tests/ui_group/marks.rs @@ -58,6 +58,33 @@ fn header_and_footer_dialog_inserts_tokens_and_applies_to_a_subset() { assert!(marks(&h).is_empty()); } +#[test] +fn header_and_footer_dialog_draws_in_the_chosen_face() { + // #3754: the dialog offers Helvetica, Times and Courier (it only said "Font: Helvetica"), + // and the face chosen is the one the page's header is drawn in. + let mut h = harness(); + assert!(h.state_mut().execute("edit.header_footer")); + h.run_steps(3); + h.get_by_label("Font:"); + h.get_by_value("Helvetica").click(); + h.run_steps(2); + h.get_by_label("Times").click(); + h.run_steps(2); + assert_eq!(h.state().marks_draft.hf.font, pdfcraft_engine::FontFamily::Times); + h.state_mut().marks_draft.hf.text[0] = "Serif".into(); + h.run_steps(2); + h.get_by_label("OK").click(); + h.run_steps(4); + assert_eq!(marks(&h), [MarkKind::HeaderFooter]); + let s = h.state(); + let doc = pdfcraft_cos::Document::open(s.session.get(s.views[0].id).unwrap().bytes.clone()).unwrap(); + let page = &pdfcraft_model::pages(&doc)[0]; + let res = doc.resolve(page.dict.get(b"Resources").unwrap()); + let fonts = doc.resolve(res.as_dict().unwrap().get(b"Font").unwrap()).as_dict().cloned().unwrap(); + let face = doc.resolve(fonts.get(b"PCTimes").expect("Times is added")).as_dict().cloned().unwrap(); + assert_eq!(face.name(b"BaseFont"), Some(&b"Times-Roman"[..])); +} + #[test] fn watermark_and_background_dialogs() { let mut h = harness(); diff --git a/parity/acrobat-features.toml b/parity/acrobat-features.toml index 7aca7d5c8b..13aeeb5826 100644 --- a/parity/acrobat-features.toml +++ b/parity/acrobat-features.toml @@ -2389,7 +2389,7 @@ status = "shipped" commands = ["edit.header_footer"] tools = ["doc_header_footer"] evidence = ["crates/ui-egui/tests/ui_group/marks.rs::header_and_footer_dialog_inserts_tokens_and_applies_to_a_subset", "crates/edit/src/tests.rs::header_and_footer_are_drawn_in_display_space_and_wrap_the_original_content", "crates/engine/src/tests.rs::headers_footers_watermarks_and_backgrounds_show_update_and_remove"] -notes = "Helvetica only (no font choice yet); drawn in display space on rotated pages; artifact-tagged" +notes = "Standard faces only: Helvetica, Times or Courier (#3754; no bold, italic or embedded fonts yet); drawn in display space on rotated pages; artifact-tagged" [[feature]] id = "edit.header-footer-update-remove" From 4e8e17c7e63336d6d708c936a0f8c22dd76928cf Mon Sep 17 00:00:00 2001 From: Lombardi2003 Date: Wed, 21 Oct 2026 01:13:00 +0200 Subject: [PATCH 13/20] fix(edit): a header never takes over a font name the page already draws with MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From review of #3756: add_resources set /PCTimes (or /PCCour, /PCHelv) in the page's fonts whatever was there, so a page that already drew another font by that name would have its own text change face. Headers and footers now pick the usual name when it is free or already names the same standard face, otherwise the first free name_1, name_2…, and add only that face, never replacing an entry. An update finds its own name again. Co-Authored-By: Claude Opus 5.5 --- crates/edit/src/lib.rs | 48 ++++++++++++++++++++++++++++++++++------ crates/edit/src/tests.rs | 39 ++++++++++++++++++++++++++++++++ 2 files changed, 80 insertions(+), 7 deletions(-) diff --git a/crates/edit/src/lib.rs b/crates/edit/src/lib.rs index 90d8f24cdd..a37ed4d1e0 100644 --- a/crates/edit/src/lib.rs +++ b/crates/edit/src/lib.rs @@ -308,19 +308,51 @@ fn contents(doc: &mut Document, page: &pdfcraft_model::Page) -> Result, content: Option<&[u8]>) -> Result<(), EditError> { - let mut res = page.dict.get(b"Resources").map(|r| doc.resolve(r)).and_then(|r| r.as_dict().cloned()).unwrap_or_default(); - let mut fonts = res.get(b"Font").map(|f| doc.resolve(f)).and_then(|f| f.as_dict().cloned()).unwrap_or_default(); + let mut faces: Vec<(Vec, &str)> = Vec::new(); for (name, base) in [(&b"PCHelv"[..], "Helvetica"), (b"PCTimes", "Times-Roman"), (b"PCCour", "Courier")] { // Helvetica always (marks use it); the others only when the content names them. if name != b"PCHelv" && !content.is_some_and(|c| c.windows(name.len() + 1).any(|w| w[0] == b'/' && &w[1..] == name)) { continue; } + faces.push((name.to_vec(), base)); + } + add_page_resources(doc, page, opacity, &faces) +} + +/// The fonts a page draws with: its own resources' or the ones it inherits. +fn page_fonts(doc: &Document, page: &pdfcraft_model::Page) -> Dict { + let res = page.dict.get(b"Resources").map(|r| doc.resolve(r)).and_then(|r| r.as_dict().cloned()).unwrap_or_default(); + res.get(b"Font").map(|f| doc.resolve(f)).and_then(|f| f.as_dict().cloned()).unwrap_or_default() +} + +/// A name in `fonts` for the standard face `base` that can't take over a font the page already +/// draws with: `preferred` when it is free or already names that face, otherwise the first such +/// `preferred_1`, `preferred_2`… (one of the first `fonts.len() + 1` always is). +fn free_font_name(doc: &Document, fonts: &Dict, preferred: &str, base: &str) -> String { + let usable = |name: &str| match fonts.get(name.as_bytes()) { + None => true, + Some(font) => doc.resolve(font).as_dict().is_some_and(|d| { + d.name(b"Subtype") == Some(b"Type1") && d.name(b"BaseFont") == Some(base.as_bytes()) && d.name(b"Encoding") == Some(b"WinAnsiEncoding") + }), + }; + if usable(preferred) { + return preferred.to_string(); + } + (1..=fonts.len().saturating_add(1)).map(|k| format!("{preferred}_{k}")).find(|name| usable(name)).unwrap_or_else(|| preferred.to_string()) +} + +/// Add the standard `faces` (resource name, base font) and an opacity state to the page's own +/// resources. +fn add_page_resources(doc: &mut Document, page: &pdfcraft_model::Page, opacity: Option, faces: &[(Vec, &str)]) -> Result<(), EditError> { + let mut res = page.dict.get(b"Resources").map(|r| doc.resolve(r)).and_then(|r| r.as_dict().cloned()).unwrap_or_default(); + let mut fonts = res.get(b"Font").map(|f| doc.resolve(f)).and_then(|f| f.as_dict().cloned()).unwrap_or_default(); + for (name, base) in faces { let mut font = Dict::new(); font.set(b"Type".to_vec(), Object::name("Font")); font.set(b"Subtype".to_vec(), Object::name("Type1")); font.set(b"BaseFont".to_vec(), Object::name(base)); font.set(b"Encoding".to_vec(), Object::name("WinAnsiEncoding")); - fonts.set(name.to_vec(), Object::Dict(font)); + fonts.set(name.clone(), Object::Dict(font)); } res.set(b"Font".to_vec(), Object::Dict(fonts)); if let Some(o) = opacity { @@ -656,12 +688,15 @@ pub fn add_header_footer(doc: &mut Document, pages: &[usize], hf: &HeaderFooter, let bates = bates0.checked_add(k as u64).ok_or_else(|| EditError::Invalid("the Bates numbers are too large".into()))?; let size = hf.font_size; let [top, bottom, left, right] = hf.margins; - // The page resource each face is added under (see `add_resources`). - let resource = match hf.font { + // The page resource the face is added under: the usual name, unless the page already + // draws another font by it, which must not change under the header. + let preferred = match hf.font { added::Family::Helvetica => "PCHelv", added::Family::Times => "PCTimes", added::Family::Courier => "PCCour", }; + let base = hf.font.base_font(false, false); + let resource = free_font_name(doc, &page_fonts(doc, page), preferred, base); let mut body: Vec = format!("BT\n/{resource} {} Tf\n{}\n", n(size), rgb(hf.color)).into_bytes(); let mut underlines = String::new(); for (slot, template) in hf.text.iter().enumerate() { @@ -694,8 +729,7 @@ pub fn add_header_footer(doc: &mut Document, pages: &[usize], hf: &HeaderFooter, let mut content = begin(MarkKind::HeaderFooter, "Header", page.view_matrix(doc)).into_bytes(); content.extend(body); content.extend_from_slice(END.as_bytes()); - // The content names the face, so Times or Courier is added beside Helvetica. - add_resources(doc, page, None, Some(&content))?; + add_page_resources(doc, page, None, &[(resource.into_bytes(), base)])?; let page = &page_list(doc)[i]; place(doc, page, MarkKind::HeaderFooter, content, false)?; } diff --git a/crates/edit/src/tests.rs b/crates/edit/src/tests.rs index 7e16f22ce0..a0f4272ecd 100644 --- a/crates/edit/src/tests.rs +++ b/crates/edit/src/tests.rs @@ -131,6 +131,45 @@ fn header_and_footer_are_drawn_in_the_chosen_standard_face() { assert!(mark.contains("/PCHelv 8 Tf") && !mark.contains("/PCTimes") && !mark.contains("/PCCour"), "{mark}"); } +#[test] +fn a_header_never_takes_over_a_font_name_the_page_already_draws_with() { + // From review of #3756: a page that already draws a different font as /PCTimes keeps it; the + // header's Times goes under a free name, and an update reuses that name, not a third one. + let mut doc = fixture(); + let page = pdfcraft_model::pages(&doc)[0].obj; + let mut fonts = Dict::new(); + let mut theirs = Dict::new(); + theirs.set(b"Type".to_vec(), Object::name("Font")); + theirs.set(b"Subtype".to_vec(), Object::name("Type1")); + theirs.set(b"BaseFont".to_vec(), Object::name("Helvetica-Bold")); + fonts.set(b"PCTimes".to_vec(), Object::Dict(theirs)); + let mut res = Dict::new(); + res.set(b"Font".to_vec(), Object::Dict(fonts)); + doc.update_dict(page, |d| d.set(b"Resources".to_vec(), Object::Dict(res))).unwrap(); + let hf = HeaderFooter { + text: ["Serif".into(), String::new(), String::new(), String::new(), String::new(), String::new()], + font: added::Family::Times, + ..HeaderFooter::default() + }; + let base_of = |doc: &Document, name: &[u8]| { + let fonts = page_fonts(doc, &pdfcraft_model::pages(doc)[0]); + fonts.get(name).map(|f| doc.resolve(f)).and_then(|f| f.as_dict().and_then(|d| d.name(b"BaseFont")).map(<[u8]>::to_vec)) + }; + add_header_footer(&mut doc, &[0], &hf, false, &cx()).unwrap(); + let doc = reopen(&doc); + assert_eq!(base_of(&doc, b"PCTimes").as_deref(), Some(&b"Helvetica-Bold"[..]), "the page's own font is untouched"); + assert_eq!(base_of(&doc, b"PCTimes_1").as_deref(), Some(&b"Times-Roman"[..])); + let mark = streams(&doc, 0).last().cloned().unwrap(); + assert!(mark.contains("/PCTimes_1 8 Tf"), "{mark}"); + // Updating the header finds its own name again. + let mut doc = doc; + add_header_footer(&mut doc, &[0], &hf, true, &cx()).unwrap(); + let doc = reopen(&doc); + assert!(streams(&doc, 0).last().unwrap().contains("/PCTimes_1 8 Tf")); + assert_eq!(base_of(&doc, b"PCTimes_2"), None, "no new name on update"); + assert_eq!(base_of(&doc, b"PCTimes").as_deref(), Some(&b"Helvetica-Bold"[..])); +} + #[test] fn header_start_number_that_would_wrap_is_refused_without_drawing() { // u32::MAX is a valid start_number on its own, but there is no room for a second page From 14b3978344007130d5395c74e0ce49425c2d9ffd Mon Sep 17 00:00:00 2001 From: Lombardi2003 Date: Wed, 21 Oct 2026 07:56:53 +0200 Subject: [PATCH 14/20] fix(edit): a header reuses a font name only for its very own dictionary From review of #3756: a /PCTimes naming Times-Roman with more to it (a /ToUnicode map, /Widths) passed as the same face and was replaced, so the page's own text could lose its map. A name is reused only when it holds exactly the dictionary a header writes, and then it is left as it is; anything else gets a free suffixed name. Co-Authored-By: Claude Opus 5.5 --- crates/edit/src/lib.rs | 17 +++++++++++++---- crates/edit/src/tests.rs | 20 ++++++++++++++++++++ 2 files changed, 33 insertions(+), 4 deletions(-) diff --git a/crates/edit/src/lib.rs b/crates/edit/src/lib.rs index a37ed4d1e0..7b64106194 100644 --- a/crates/edit/src/lib.rs +++ b/crates/edit/src/lib.rs @@ -326,13 +326,19 @@ fn page_fonts(doc: &Document, page: &pdfcraft_model::Page) -> Dict { } /// A name in `fonts` for the standard face `base` that can't take over a font the page already -/// draws with: `preferred` when it is free or already names that face, otherwise the first such -/// `preferred_1`, `preferred_2`… (one of the first `fonts.len() + 1` always is). +/// draws with: `preferred` when it is free or already names exactly the dictionary +/// [`add_page_resources`] writes for that face, otherwise the first such `preferred_1`, +/// `preferred_2`… (one of the first `fonts.len() + 1` always is). A font with anything more +/// (`/ToUnicode`, `/Widths`…) is someone else's, even in the same face. fn free_font_name(doc: &Document, fonts: &Dict, preferred: &str, base: &str) -> String { let usable = |name: &str| match fonts.get(name.as_bytes()) { None => true, Some(font) => doc.resolve(font).as_dict().is_some_and(|d| { - d.name(b"Subtype") == Some(b"Type1") && d.name(b"BaseFont") == Some(base.as_bytes()) && d.name(b"Encoding") == Some(b"WinAnsiEncoding") + d.len() == 4 + && d.name(b"Type") == Some(b"Font") + && d.name(b"Subtype") == Some(b"Type1") + && d.name(b"BaseFont") == Some(base.as_bytes()) + && d.name(b"Encoding") == Some(b"WinAnsiEncoding") }), }; if usable(preferred) { @@ -729,7 +735,10 @@ pub fn add_header_footer(doc: &mut Document, pages: &[usize], hf: &HeaderFooter, let mut content = begin(MarkKind::HeaderFooter, "Header", page.view_matrix(doc)).into_bytes(); content.extend(body); content.extend_from_slice(END.as_bytes()); - add_page_resources(doc, page, None, &[(resource.into_bytes(), base)])?; + // A name already there is that very font (see `free_font_name`): left as it is. + if !page_fonts(doc, page).contains(resource.as_bytes()) { + add_page_resources(doc, page, None, &[(resource.into_bytes(), base)])?; + } let page = &page_list(doc)[i]; place(doc, page, MarkKind::HeaderFooter, content, false)?; } diff --git a/crates/edit/src/tests.rs b/crates/edit/src/tests.rs index a0f4272ecd..bc6b84e19e 100644 --- a/crates/edit/src/tests.rs +++ b/crates/edit/src/tests.rs @@ -168,6 +168,26 @@ fn a_header_never_takes_over_a_font_name_the_page_already_draws_with() { assert!(streams(&doc, 0).last().unwrap().contains("/PCTimes_1 8 Tf")); assert_eq!(base_of(&doc, b"PCTimes_2"), None, "no new name on update"); assert_eq!(base_of(&doc, b"PCTimes").as_deref(), Some(&b"Helvetica-Bold"[..])); + // The same face with more to it (here a /ToUnicode map) is the page's own too: kept whole. + let mut doc = fixture(); + let page = pdfcraft_model::pages(&doc)[0].obj; + let map = doc.add(Object::Stream(pdfcraft_cos::Stream::flate(Dict::new(), b"/CIDInit /ProcSet findresource begin"))); + let mut theirs = Dict::new(); + for (k, v) in [("Type", "Font"), ("Subtype", "Type1"), ("BaseFont", "Times-Roman"), ("Encoding", "WinAnsiEncoding")] { + theirs.set(k.as_bytes().to_vec(), Object::name(v)); + } + theirs.set(b"ToUnicode".to_vec(), Object::Ref(map)); + let mut fonts = Dict::new(); + fonts.set(b"PCTimes".to_vec(), Object::Dict(theirs)); + let mut res = Dict::new(); + res.set(b"Font".to_vec(), Object::Dict(fonts)); + doc.update_dict(page, |d| d.set(b"Resources".to_vec(), Object::Dict(res))).unwrap(); + add_header_footer(&mut doc, &[0], &hf, false, &cx()).unwrap(); + let doc = reopen(&doc); + let fonts = page_fonts(&doc, &pdfcraft_model::pages(&doc)[0]); + let kept = doc.resolve(fonts.get(b"PCTimes").unwrap()).as_dict().cloned().unwrap(); + assert!(kept.contains(b"ToUnicode"), "the page's map is kept"); + assert!(streams(&doc, 0).last().unwrap().contains("/PCTimes_1 8 Tf")); } #[test] From 7a7d39f01f93333d3f42365b66d093bd04969835 Mon Sep 17 00:00:00 2001 From: komo1iddin <45395275+komo1iddin@users.noreply.github.com> Date: Wed, 21 Oct 2026 23:15:28 +0800 Subject: [PATCH 15/20] M7: keep a moved or resized page image in place until its page is drawn again A dragged page image is drawn from the page raster, blanked where it was and stretched into its new frame. On release the edit applied and the drag state cleared in the same frame, but the page raster on screen still had the picture at its old size and place until its replacement arrived. For those frames the old picture showed again around the new frame, then jumped. Keep the old and new frames on the selection after a release, and draw the picture the way the drag did while the page is out of date: its raster, or on a tiled page any tile, still waits for a replacement. A failed edit never makes the page stale, so nothing is kept. Co-Authored-By: Claude Opus 5.5 --- crates/ui-egui/src/canvas.rs | 10 ++++ crates/ui-egui/src/edit_text_ui.rs | 57 +++++++++++++++------ crates/ui-egui/tests/editing.rs | 79 ++++++++++++++++++++++++++++++ 3 files changed, 130 insertions(+), 16 deletions(-) diff --git a/crates/ui-egui/src/canvas.rs b/crates/ui-egui/src/canvas.rs index eb9244a284..20b7a0a2ea 100644 --- a/crates/ui-egui/src/canvas.rs +++ b/crates/ui-egui/src/canvas.rs @@ -1046,6 +1046,16 @@ impl DocView { self.pages.get(&page).map(|p| p.tex.id()) } + /// Whether `page` is still drawn as it was before its last change: its raster, or on a tiled + /// page any of its tiles, is waiting for a replacement. + pub(crate) fn page_out_of_date(&self, page: usize) -> bool { + if self.tiles.keys().any(|k| k.0 == page) { + self.stale_tiles.iter().any(|k| k.0 == page) + } else { + self.pages.get(&page).is_some_and(|p| p.tag == STALE_TAG) + } + } + /// The print preview's picture of `page`: the sheet raster when it has arrived, otherwise the /// thumbnail. pub(crate) fn page_preview(&self, page: usize) -> Option { diff --git a/crates/ui-egui/src/edit_text_ui.rs b/crates/ui-egui/src/edit_text_ui.rs index 66ad98ffe6..5b5a6445f8 100644 --- a/crates/ui-egui/src/edit_text_ui.rs +++ b/crates/ui-egui/src/edit_text_ui.rs @@ -358,6 +358,9 @@ pub struct ImageSelection { drag: Option<(Pos2, Option)>, /// Crop mode (Acrobat's crop handles): the box kept so far. pub crop: Option, + /// A move or resize applied on release: the frame it had and the one it got, in the page's + /// 0..1 coordinates, while the page raster still has the picture where it was. + settled: Option<(Rect, Rect)>, } /// Cropping a page image: the handles trim instead of resizing, the part that will go is @@ -533,6 +536,25 @@ impl ImageSelection { } } +/// The page raster's picture of the box `from`, stretched into `to` over a blank where it was. +fn stretched_picture(painter: &egui::Painter, xf: &PageXform, tex: egui::TextureId, from: Rect, to: Rect) { + let clipped = painter.with_clip_rect(painter.clip_rect().intersect(xf.rect)); + clipped.rect_filled(from, CornerRadius::ZERO, Color32::WHITE); + let mut mesh = egui::Mesh::with_texture(tex); + for (a, b) in [ + (from.left_top(), to.left_top()), + (from.right_top(), to.right_top()), + (from.right_bottom(), to.right_bottom()), + (from.left_bottom(), to.left_bottom()), + ] { + let (u, v) = xf.screen_to_norm(a); + mesh.vertices.push(egui::epaint::Vertex { pos: b, uv: Pos2::new(u, v), color: Color32::WHITE }); + } + mesh.add_triangle(0, 1, 2); + mesh.add_triangle(0, 2, 3); + clipped.add(egui::Shape::mesh(mesh)); +} + /// Once a selected corner is grabbed, text under it cannot take over the gesture. The same /// applies after the pointer leaves the image/page while dragging. pub(crate) fn image_owns_pointer(ui: &egui::Ui, xf: &PageXform, page: usize, info: &DocInfo, view: &DocView) -> bool { @@ -697,6 +719,18 @@ pub(crate) fn image_input( ); } }; + // Until the page is rendered again its raster has an applied move or resize's picture + // where it was: it stays blanked there and drawn where it went. + let (out_of_date, raster) = (view.page_out_of_date(page), view.page_raster(page)); + if let Some(s) = view.image_selection.as_mut().filter(|s| s.drag.is_none()) { + match (s.settled, raster) { + (Some((from, to)), Some(tex)) if out_of_date => { + let screen = |r: Rect| Rect::from_two_pos(xf.norm_to_screen(r.min.x, r.min.y), xf.norm_to_screen(r.max.x, r.max.y)); + stretched_picture(painter, xf, tex, screen(from), screen(to)); + } + _ => s.settled = None, + } + } if view.image_selection.as_ref().is_some_and(|s| s.drag.is_none()) { painter.rect_stroke(b, CornerRadius::ZERO, Stroke::new(1.5, ACCENT), egui::StrokeKind::Outside); handles(b); @@ -754,21 +788,7 @@ pub(crate) fn image_input( if preview != b && let Some(tex) = view.page_raster(page) { - let clipped = painter.with_clip_rect(painter.clip_rect().intersect(xf.rect)); - clipped.rect_filled(b, CornerRadius::ZERO, Color32::WHITE); - let mut mesh = egui::Mesh::with_texture(tex); - for (from, to) in [ - (b.left_top(), preview.left_top()), - (b.right_top(), preview.right_top()), - (b.right_bottom(), preview.right_bottom()), - (b.left_bottom(), preview.left_bottom()), - ] { - let (u, v) = xf.screen_to_norm(from); - mesh.vertices.push(egui::epaint::Vertex { pos: to, uv: Pos2::new(u, v), color: Color32::WHITE }); - } - mesh.add_triangle(0, 1, 2); - mesh.add_triangle(0, 2, 3); - clipped.add(egui::Shape::mesh(mesh)); + stretched_picture(painter, xf, tex, b, preview); } painter.rect_filled(visible, CornerRadius::ZERO, ACCENT.gamma_multiply(0.04)); painter.rect_stroke(visible, CornerRadius::ZERO, Stroke::new(1.5, ACCENT), egui::StrokeKind::Outside); @@ -777,6 +797,11 @@ pub(crate) fn image_input( if resp.drag_stopped() || !ui.input(|i| i.pointer.primary_down()) { if let Some(s) = view.image_selection.as_mut() { s.drag = None; + let norm = |r: Rect| { + let ((u0, v0), (u1, v1)) = (xf.screen_to_norm(r.min), xf.screen_to_norm(r.max)); + Rect::from_two_pos(Pos2::new(u0, v0), Pos2::new(u1, v1)) + }; + s.settled = (preview != b).then(|| (norm(b), norm(preview))); } if preview != b { view.pending_edit = @@ -832,7 +857,7 @@ pub(crate) fn image_input( _ => egui::CursorIcon::PointingHand, }); if resp.clicked() || resp.secondary_clicked() { - view.image_selection = Some(ImageSelection { page, index: hit, drag: None, crop: None }); + view.image_selection = Some(ImageSelection { page, index: hit, drag: None, crop: None, settled: None }); } if resp.secondary_clicked() { ui.ctx().data_mut(|d| d.insert_temp(menu_key, true)); diff --git a/crates/ui-egui/tests/editing.rs b/crates/ui-egui/tests/editing.rs index d41041e52e..4a386372ec 100644 --- a/crates/ui-egui/tests/editing.rs +++ b/crates/ui-egui/tests/editing.rs @@ -4454,6 +4454,85 @@ fn image_pixels_follow_the_frame_while_a_corner_is_dragged() { } } +/// Red pixels in `r`: the test picture. +fn red(pixels: &image::RgbaImage, r: egui::Rect) -> usize { + let (x0, y0, x1, y1) = (r.left().max(0.0) as u32, r.top().max(0.0) as u32, r.right() as u32, r.bottom() as u32); + (y0..y1.min(pixels.height())) + .flat_map(|y| (x0..x1.min(pixels.width())).map(move |x| (x, y))) + .filter(|(x, y)| matches!(pixels.get_pixel(*x, *y).0, [r, g, b, _] if r > 150 && g < 90 && b < 90)) + .count() +} + +/// A resized picture is applied when the corner is let go, but the page raster still has it at +/// its old size until the page is rendered again. Until then it stays drawn at its new size and +/// blanked at its old one; the old one used to show again for those frames. +#[test] +fn a_resized_image_keeps_its_new_size_until_the_page_is_redrawn() { + use pdfcraft_cos::{Dict, Document, Object, Stream, write_incremental}; + let mut doc = Document::open(std::sync::Arc::new(fixture(1))).unwrap(); + let mut image = Dict::new(); + image.set(b"Type".to_vec(), Object::name("XObject")); + image.set(b"Subtype".to_vec(), Object::name("Image")); + image.set(b"Width".to_vec(), Object::Int(1)); + image.set(b"Height".to_vec(), Object::Int(1)); + image.set(b"BitsPerComponent".to_vec(), Object::Int(8)); + image.set(b"ColorSpace".to_vec(), Object::name("DeviceRGB")); + let image = doc.add(Object::Stream(Stream::flate(image, &[200, 30, 30]))); + let page = pdfcraft_model::pages(&doc).remove(0); + let contents = doc.add(Object::Stream(Stream::flate(Dict::new(), b"q 80 0 0 80 60 40 cm /Im0 Do Q"))); + let mut resources = doc.resolve(page.dict.get(b"Resources").unwrap()).as_dict().unwrap().clone(); + let mut xo = Dict::new(); + xo.set(b"Im0".to_vec(), Object::Ref(image)); + resources.set(b"XObject".to_vec(), Object::Dict(xo)); + doc.update_dict(page.obj, |d| { + d.set(b"Resources".to_vec(), Object::Dict(resources)); + d.set(b"Contents".to_vec(), Object::Ref(contents)); + }) + .unwrap(); + let bytes = write_incremental(&doc, &Default::default()).unwrap(); + let mut h = Harness::builder().with_size(egui::vec2(1400.0, 1100.0)).build_eframe(move |_cc| { + let mut app = PdfCraftApp::new(); + app.set_option("language", "en").unwrap(); + app.open_bytes("picture.pdf", None, bytes).unwrap(); + app + }); + h.run_steps(4); + assert!(h.state_mut().execute("edit.edit_text")); + h.state_mut().views[0].set_zoom(1.2); + h.run_steps(4); + support::wait_until(&mut h, "the page raster", |app| !app.render_pending()); + let page = h.state().views[0].page_screen_rect(0).unwrap(); + let at = |x: f32, y: f32| egui::pos2(page.left() + x / 200.0 * page.width(), page.top() + (300.0 - y) / 300.0 * page.height()); + click_at(&mut h, at(100.0, 80.0)); + assert!(h.state().views[0].image_selection.is_some(), "the picture is selected"); + support::wait_until(&mut h, "the page raster", |app| !app.render_pending()); + // From 60..140 × 40..120 to half its size, keeping its lower left corner. + let (grip, to) = (at(140.0, 120.0), at(100.0, 80.0)); + let kept = egui::Rect::from_two_pos(at(62.0, 42.0), at(98.0, 78.0)); + let gone = egui::Rect::from_two_pos(at(104.0, 84.0), at(138.0, 118.0)); + h.hover_at(grip); + h.run_steps(1); + h.drag_at(grip); + h.run_steps(1); + for k in 1..=4 { + h.hover_at(grip + (to - grip) * (k as f32 / 4.0)); + h.run_steps(1); + } + h.state_mut().views[0].hold_rasters = true; + h.drop_at(to); + for frame in 0..4 { + h.step(); + let pixels = h.render().unwrap(); + assert!(red(&pixels, kept) > 100, "frame {frame} after the release: the picture is not at its new size"); + assert_eq!(red(&pixels, gone), 0, "frame {frame} after the release: the picture shows at its old size"); + } + h.state_mut().views[0].hold_rasters = false; + support::wait_until(&mut h, "the page drawn with the resized picture", |app| !app.render_pending()); + h.run_steps(2); + let pixels = h.render().unwrap(); + assert!(red(&pixels, kept) > 100 && red(&pixels, gone) == 0, "the page raster has the picture at its new size"); +} + /// Exercise actual pointer gestures on each cropped placement, without capturing private pixels. #[test] #[ignore = "requires an explicitly supplied local PDF with cropped artwork"] From f632c1b0ca5b172811b4b515083806201fddca5b Mon Sep 17 00:00:00 2001 From: komo1iddin <45395275+komo1iddin@users.noreply.github.com> Date: Thu, 22 Oct 2026 00:26:39 +0800 Subject: [PATCH 16/20] M7: a resized page image stays blanked where the raster still has it Two cases kept the old picture on screen after a move or resize: - A page that had no tiles when the edit applied was taken as drawn again as soon as a first tile arrived, while its backdrop still had the picture. Out of date now means its whole-page raster or any tile still waits for a replacement. - A second drag before the page was drawn again took the picture's pixels from its new frame, where the raster doesn't have it, and blanked only that. It now takes them from where the raster still has the picture and blanks that, and keeps it as the source on release. Co-Authored-By: Claude Opus 5.5 --- crates/ui-egui/src/canvas.rs | 10 +++------- crates/ui-egui/src/edit_text_ui.rs | 13 +++++++------ crates/ui-egui/tests/editing.rs | 21 ++++++++++++++++++++- 3 files changed, 30 insertions(+), 14 deletions(-) diff --git a/crates/ui-egui/src/canvas.rs b/crates/ui-egui/src/canvas.rs index 20b7a0a2ea..13ff04ec4d 100644 --- a/crates/ui-egui/src/canvas.rs +++ b/crates/ui-egui/src/canvas.rs @@ -1046,14 +1046,10 @@ impl DocView { self.pages.get(&page).map(|p| p.tex.id()) } - /// Whether `page` is still drawn as it was before its last change: its raster, or on a tiled - /// page any of its tiles, is waiting for a replacement. + /// Whether `page` is still drawn, somewhere, as it was before its last change: its whole-page + /// raster (a tiled page's backdrop too) or any of its tiles waits for a replacement. pub(crate) fn page_out_of_date(&self, page: usize) -> bool { - if self.tiles.keys().any(|k| k.0 == page) { - self.stale_tiles.iter().any(|k| k.0 == page) - } else { - self.pages.get(&page).is_some_and(|p| p.tag == STALE_TAG) - } + self.pages.get(&page).is_some_and(|p| p.tag == STALE_TAG) || self.stale_tiles.iter().any(|k| k.0 == page) } /// The print preview's picture of `page`: the sheet raster when it has arrived, otherwise the diff --git a/crates/ui-egui/src/edit_text_ui.rs b/crates/ui-egui/src/edit_text_ui.rs index 5b5a6445f8..adfbc675ae 100644 --- a/crates/ui-egui/src/edit_text_ui.rs +++ b/crates/ui-egui/src/edit_text_ui.rs @@ -722,12 +722,13 @@ pub(crate) fn image_input( // Until the page is rendered again its raster has an applied move or resize's picture // where it was: it stays blanked there and drawn where it went. let (out_of_date, raster) = (view.page_out_of_date(page), view.page_raster(page)); + let screen = |r: Rect| Rect::from_two_pos(xf.norm_to_screen(r.min.x, r.min.y), xf.norm_to_screen(r.max.x, r.max.y)); + // Where the raster on screen has the picture: where it was before an applied move that + // hasn't been rendered yet, else its frame. Another drag starts from there too. + let drawn_at = view.image_selection.as_ref().and_then(|s| s.settled).filter(|_| out_of_date).map(|(from, _)| from); if let Some(s) = view.image_selection.as_mut().filter(|s| s.drag.is_none()) { match (s.settled, raster) { - (Some((from, to)), Some(tex)) if out_of_date => { - let screen = |r: Rect| Rect::from_two_pos(xf.norm_to_screen(r.min.x, r.min.y), xf.norm_to_screen(r.max.x, r.max.y)); - stretched_picture(painter, xf, tex, screen(from), screen(to)); - } + (Some((from, to)), Some(tex)) if out_of_date => stretched_picture(painter, xf, tex, screen(from), screen(to)), _ => s.settled = None, } } @@ -788,7 +789,7 @@ pub(crate) fn image_input( if preview != b && let Some(tex) = view.page_raster(page) { - stretched_picture(painter, xf, tex, b, preview); + stretched_picture(painter, xf, tex, drawn_at.map_or(b, screen), preview); } painter.rect_filled(visible, CornerRadius::ZERO, ACCENT.gamma_multiply(0.04)); painter.rect_stroke(visible, CornerRadius::ZERO, Stroke::new(1.5, ACCENT), egui::StrokeKind::Outside); @@ -801,7 +802,7 @@ pub(crate) fn image_input( let ((u0, v0), (u1, v1)) = (xf.screen_to_norm(r.min), xf.screen_to_norm(r.max)); Rect::from_two_pos(Pos2::new(u0, v0), Pos2::new(u1, v1)) }; - s.settled = (preview != b).then(|| (norm(b), norm(preview))); + s.settled = (preview != b).then(|| (drawn_at.unwrap_or_else(|| norm(b)), norm(preview))); } if preview != b { view.pending_edit = diff --git a/crates/ui-egui/tests/editing.rs b/crates/ui-egui/tests/editing.rs index 4a386372ec..67d22f77ae 100644 --- a/crates/ui-egui/tests/editing.rs +++ b/crates/ui-egui/tests/editing.rs @@ -4526,11 +4526,30 @@ fn a_resized_image_keeps_its_new_size_until_the_page_is_redrawn() { assert!(red(&pixels, kept) > 100, "frame {frame} after the release: the picture is not at its new size"); assert_eq!(red(&pixels, gone), 0, "frame {frame} after the release: the picture shows at its old size"); } + // Resized again before the page is drawn: the raster still has the first size, so that is + // what stays blanked while the corner moves. + let (grip, to) = (to, at(80.0, 60.0)); + let again = egui::Rect::from_two_pos(at(62.0, 42.0), at(78.0, 58.0)); + h.hover_at(grip); + h.run_steps(1); + h.drag_at(grip); + h.run_steps(1); + for k in 1..=4 { + h.hover_at(grip + (to - grip) * (k as f32 / 4.0)); + h.run_steps(1); + } + let pixels = h.render().unwrap(); + assert!(red(&pixels, again) > 20, "the second drag draws the picture at its new size"); + assert_eq!(red(&pixels, gone), 0, "the second drag shows the picture at its first size"); + h.drop_at(to); + h.run_steps(2); + let pixels = h.render().unwrap(); + assert_eq!(red(&pixels, gone), 0, "after the second release"); h.state_mut().views[0].hold_rasters = false; support::wait_until(&mut h, "the page drawn with the resized picture", |app| !app.render_pending()); h.run_steps(2); let pixels = h.render().unwrap(); - assert!(red(&pixels, kept) > 100 && red(&pixels, gone) == 0, "the page raster has the picture at its new size"); + assert!(red(&pixels, again) > 20 && red(&pixels, gone) == 0, "the page raster has the picture at its new size"); } /// Exercise actual pointer gestures on each cropped placement, without capturing private pixels. From bef4e28a4d713e9c47bde074b8c5180035aaa3e3 Mon Sep 17 00:00:00 2001 From: komo1iddin <45395275+komo1iddin@users.noreply.github.com> Date: Thu, 22 Oct 2026 06:52:56 +0800 Subject: [PATCH 17/20] M7: a dragged page image leaves no outline where it was Edit text & images outlines every picture from the page's inventory, which still has the dragged one where it was until the release; that outline trailed behind the picture and caught up after it was let go. Leave the dragged picture's own outline out while it moves. Co-Authored-By: Claude Opus 5.5 --- crates/ui-egui/src/edit_text_ui.rs | 7 +++++-- crates/ui-egui/tests/editing.rs | 6 ++++++ 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/crates/ui-egui/src/edit_text_ui.rs b/crates/ui-egui/src/edit_text_ui.rs index adfbc675ae..d6ddab2a94 100644 --- a/crates/ui-egui/src/edit_text_ui.rs +++ b/crates/ui-egui/src/edit_text_ui.rs @@ -677,14 +677,17 @@ pub(crate) fn image_input( // hovered boxes, so a ~1000-image CAD page stays interactive in Edit text & images. // Fully clipped placements (empty visible frames) get no outline. let faint_all = boxes.len() <= MAX_OUTLINES; + // The picture being dragged has its frame at the pointer; its outline where it was would + // trail behind until the release. + let moving = selected.filter(|_| dragging); if faint_all { - for b in boxes.iter().filter(|b| b.is_positive()) { + for (_, b) in boxes.iter().enumerate().filter(|(i, b)| b.is_positive() && moving != Some(*i)) { painter.rect_stroke(*b, CornerRadius::ZERO, Stroke::new(0.75, ACCENT.gamma_multiply(0.35)), egui::StrokeKind::Outside); } } let hovered_box = hover_pos.and_then(|p| boxes.iter().enumerate().rposition(|(i, b)| b.is_positive() && b.contains(p) && image_contains(i, p))); if !faint_all { - for i in selected.into_iter().chain(hovered_box.filter(|h| Some(*h) != selected)) { + for i in selected.filter(|_| moving.is_none()).into_iter().chain(hovered_box.filter(|h| Some(*h) != selected)) { if let Some(b) = boxes.get(i).filter(|b| b.is_positive()) { painter.rect_stroke(*b, CornerRadius::ZERO, Stroke::new(0.75, ACCENT.gamma_multiply(0.35)), egui::StrokeKind::Outside); } diff --git a/crates/ui-egui/tests/editing.rs b/crates/ui-egui/tests/editing.rs index 67d22f77ae..e1f542bde5 100644 --- a/crates/ui-egui/tests/editing.rs +++ b/crates/ui-egui/tests/editing.rs @@ -4518,6 +4518,12 @@ fn a_resized_image_keeps_its_new_size_until_the_page_is_redrawn() { h.hover_at(grip + (to - grip) * (k as f32 / 4.0)); h.run_steps(1); } + // While dragged, no outline stays where the picture was. + let old = egui::Rect::from_two_pos(at(60.0, 40.0), at(140.0, 120.0)); + let trailing = h.output().shapes.iter().any(|shape| { + matches!(&shape.shape, egui::Shape::Rect(r) if r.stroke.width == 0.75 && (r.rect.min - old.min).length() < 2.0 && (r.rect.max - old.max).length() < 2.0) + }); + assert!(!trailing, "the dragged picture's outline trails behind it"); h.state_mut().views[0].hold_rasters = true; h.drop_at(to); for frame in 0..4 { From af333266bf51c9d97f878f4595d408524f871e6e Mon Sep 17 00:00:00 2001 From: komo1iddin <45395275+komo1iddin@users.noreply.github.com> Date: Thu, 22 Oct 2026 07:28:34 +0800 Subject: [PATCH 18/20] M7: sample a moved picture from where the fresh raster has it The whole-page raster and the tiles of a page come back separately. Once the whole-page raster is fresh it has the picture where it went, but the settled preview still took its pixels from where it was, now empty, and drew them over it until the last stale tile arrived; a drag begun in that interval did the same. Keep blanking the old frame while anything is still stale, but take the pixels from the old frame only while the whole-page raster itself is stale (page_raster_out_of_date), else from where the picture went; a drag in that interval blanks the first edit's frame and samples its own. Co-Authored-By: Claude Opus 5.5 --- crates/ui-egui/src/canvas.rs | 122 +++++++++++++++++++++++++++++ crates/ui-egui/src/edit_text_ui.rs | 33 +++++--- 2 files changed, 145 insertions(+), 10 deletions(-) diff --git a/crates/ui-egui/src/canvas.rs b/crates/ui-egui/src/canvas.rs index 13ff04ec4d..5dfb0b3fdf 100644 --- a/crates/ui-egui/src/canvas.rs +++ b/crates/ui-egui/src/canvas.rs @@ -1048,6 +1048,12 @@ impl DocView { /// Whether `page` is still drawn, somewhere, as it was before its last change: its whole-page /// raster (a tiled page's backdrop too) or any of its tiles waits for a replacement. + /// Whether `page`'s whole-page raster (a tiled page's backdrop too) is still the one from + /// before its last change, whatever its tiles are. + pub(crate) fn page_raster_out_of_date(&self, page: usize) -> bool { + self.pages.get(&page).is_some_and(|p| p.tag == STALE_TAG) + } + pub(crate) fn page_out_of_date(&self, page: usize) -> bool { self.pages.get(&page).is_some_and(|p| p.tag == STALE_TAG) || self.stale_tiles.iter().any(|k| k.0 == page) } @@ -6484,4 +6490,120 @@ trailer << /Root 1 0 R >> assert_eq!(v.find.as_ref().unwrap().matches, vec![(1, 5..9)]); assert!(v.goto.is_none()); } + + /// A page whose picture moved can get its whole-page raster back before its last stale tile. + /// From then on the raster has the picture where it went: it is drawn from there, not from + /// where it was (which the raster now shows empty), and another drag starts from there too. + #[test] + fn a_moved_picture_is_drawn_from_the_fresh_raster_while_a_tile_is_still_stale() { + let content = b"q 60 0 0 60 40 260 cm /Im0 Do Q"; + let objs: Vec> = vec![ + b"<< /Type /Catalog /Pages 2 0 R >>".to_vec(), + b"<< /Type /Pages /Kids [3 0 R] /Count 1 /MediaBox [0 0 300 400] >>".to_vec(), + b"<< /Type /Page /Parent 2 0 R /Contents 4 0 R /Resources << /XObject << /Im0 5 0 R >> >> >>".to_vec(), + [format!("<< /Length {} >>\nstream\n", content.len()).as_bytes(), content.as_slice(), b"\nendstream"].concat(), + [ + b"<< /Type /XObject /Subtype /Image /Width 1 /Height 1 /BitsPerComponent 8 /ColorSpace /DeviceRGB /Length 3 >>\nstream\n".as_slice(), + &[200, 30, 30], + b"\nendstream", + ] + .concat(), + ]; + let mut bytes = b"%PDF-1.7\n".to_vec(); + let mut offsets = Vec::new(); + for (i, o) in objs.iter().enumerate() { + offsets.push(bytes.len()); + bytes.extend_from_slice(format!("{} 0 obj\n", i + 1).as_bytes()); + bytes.extend_from_slice(o); + bytes.extend_from_slice(b"\nendobj\n"); + } + let xref = bytes.len(); + bytes.extend_from_slice(format!("xref\n0 {}\n0000000000 65535 f \n", objs.len() + 1).as_bytes()); + for o in offsets { + bytes.extend_from_slice(format!("{o:010} 00000 n \n").as_bytes()); + } + bytes.extend_from_slice(format!("trailer\n<< /Size {} /Root 1 0 R >>\nstartxref\n{xref}\n%%EOF\n", objs.len() + 1).as_bytes()); + let mut h = egui_kittest::Harness::builder().with_size(vec2(1400.0, 1100.0)).build_eframe(move |_cc| { + let mut app = PdfCraftApp::new(); + app.set_option("language", "en").unwrap(); + app.open_bytes("picture.pdf", None, bytes.clone()).unwrap(); + app.set_option("zoom", "100").unwrap(); + app.quick_tool = QuickTool::EditText; + app + }); + let settle = |h: &mut egui_kittest::Harness<'static, PdfCraftApp>| { + for _ in 0..400 { + h.step(); + if !h.state().render_pending() { + return; + } + std::thread::sleep(std::time::Duration::from_millis(5)); + } + }; + h.run_steps(4); + settle(&mut h); + let page = h.state().views[0].page_screen_rect(0).unwrap(); + let at = |x: f32, y: f32| egui::pos2(page.left() + x / 300.0 * page.width(), page.top() + (400.0 - y) / 400.0 * page.height()); + let red = |pixels: &image::RgbaImage, r: Rect| { + let (w, hh) = (pixels.width(), pixels.height()); + (r.top().max(0.0) as u32..(r.bottom() as u32).min(hh)) + .flat_map(|y| (r.left().max(0.0) as u32..(r.right() as u32).min(w)).map(move |x| (x, y))) + .filter(|(x, y)| matches!(pixels.get_pixel(*x, *y).0, [r, g, b, _] if r > 150 && g < 90 && b < 90)) + .count() + }; + let (old, new) = (Rect::from_two_pos(at(42.0, 262.0), at(98.0, 318.0)), Rect::from_two_pos(at(182.0, 82.0), at(238.0, 138.0))); + // Select the picture and drag it, without overlap, from (40, 260) to (180, 80). + let drag = |h: &mut egui_kittest::Harness<'static, PdfCraftApp>, from: Pos2, to: Pos2, release: bool| { + h.hover_at(from); + h.run_steps(1); + h.drag_at(from); + h.run_steps(1); + for k in 1..=4 { + h.hover_at(from + (to - from) * (k as f32 / 4.0)); + h.run_steps(1); + } + if release { + h.drop_at(to); + h.step(); + } + }; + let (grab, put) = (at(70.0, 290.0), at(210.0, 110.0)); + h.hover_at(grab); + h.run_steps(1); + h.drag_at(grab); + h.run_steps(1); + h.drop_at(grab); + h.run_steps(3); + assert!(h.state().views[0].image_selection.is_some(), "the picture is selected"); + h.state_mut().views[0].hold_rasters = true; + drag(&mut h, grab, put, true); + // One tile stays stale, as a tile result arriving after the whole-page raster would; then + // the whole-page raster is let through. + let tile = h.ctx.load_texture("stale-tile", egui::ColorImage::new([1, 1], vec![Color32::TRANSPARENT]), TextureOptions::LINEAR); + h.state_mut().views[0].tiles.insert((0, 1, 0, 0), tile); + h.state_mut().views[0].stale_tiles.insert((0, 1, 0, 0)); + h.state_mut().views[0].hold_rasters = false; + for _ in 0..400 { + h.step(); + if !h.state().views[0].page_raster_out_of_date(0) { + break; + } + std::thread::sleep(std::time::Duration::from_millis(5)); + } + assert!(!h.state().views[0].page_raster_out_of_date(0), "the whole-page raster is fresh"); + assert!(h.state().views[0].page_out_of_date(0), "a tile is still stale"); + h.run_steps(2); + let pixels = h.render().unwrap(); + assert!(red(&pixels, new) > 100, "the picture shows where it went"); + assert_eq!(red(&pixels, old), 0, "and not where it was"); + // Another drag in that interval takes its pixels from where the raster has them. + let further = at(210.0, 200.0); + drag(&mut h, put, further, false); + let pixels = h.render().unwrap(); + let moving = Rect::from_two_pos(at(182.0, 172.0), at(238.0, 228.0)); + assert!(red(&pixels, moving) > 100, "the dragged picture is drawn from the fresh raster"); + assert_eq!(red(&pixels, old), 0, "nothing shows where it first was"); + h.state_mut().views[0].stale_tiles.clear(); + h.state_mut().views[0].tiles.clear(); + } } diff --git a/crates/ui-egui/src/edit_text_ui.rs b/crates/ui-egui/src/edit_text_ui.rs index d6ddab2a94..a0dacebb58 100644 --- a/crates/ui-egui/src/edit_text_ui.rs +++ b/crates/ui-egui/src/edit_text_ui.rs @@ -536,10 +536,13 @@ impl ImageSelection { } } -/// The page raster's picture of the box `from`, stretched into `to` over a blank where it was. -fn stretched_picture(painter: &egui::Painter, xf: &PageXform, tex: egui::TextureId, from: Rect, to: Rect) { +/// The page raster's picture of the box `from`, stretched into `to`, over blanks at `blank` (where +/// the screen may still show the picture). +fn stretched_picture(painter: &egui::Painter, xf: &PageXform, tex: egui::TextureId, blank: &[Rect], from: Rect, to: Rect) { let clipped = painter.with_clip_rect(painter.clip_rect().intersect(xf.rect)); - clipped.rect_filled(from, CornerRadius::ZERO, Color32::WHITE); + for r in blank { + clipped.rect_filled(*r, CornerRadius::ZERO, Color32::WHITE); + } let mut mesh = egui::Mesh::with_texture(tex); for (a, b) in [ (from.left_top(), to.left_top()), @@ -724,14 +727,21 @@ pub(crate) fn image_input( }; // Until the page is rendered again its raster has an applied move or resize's picture // where it was: it stays blanked there and drawn where it went. - let (out_of_date, raster) = (view.page_out_of_date(page), view.page_raster(page)); + // The whole-page raster and its tiles are replaced separately: the old frame stays blanked + // while anything still shows the picture there, but its pixels come from the old frame + // only while the whole-page raster itself is the old one. Once that is fresh it has the + // picture where it went, and that is where they come from. + let (out_of_date, raster_old, raster) = (view.page_out_of_date(page), view.page_raster_out_of_date(page), view.page_raster(page)); let screen = |r: Rect| Rect::from_two_pos(xf.norm_to_screen(r.min.x, r.min.y), xf.norm_to_screen(r.max.x, r.max.y)); - // Where the raster on screen has the picture: where it was before an applied move that - // hasn't been rendered yet, else its frame. Another drag starts from there too. - let drawn_at = view.image_selection.as_ref().and_then(|s| s.settled).filter(|_| out_of_date).map(|(from, _)| from); + // Where the screen may still show the picture from before an applied move, while the page + // isn't drawn again yet. Another drag keeps it blanked too. + let left_at = view.image_selection.as_ref().and_then(|s| s.settled).filter(|_| out_of_date).map(|(from, _)| from); if let Some(s) = view.image_selection.as_mut().filter(|s| s.drag.is_none()) { match (s.settled, raster) { - (Some((from, to)), Some(tex)) if out_of_date => stretched_picture(painter, xf, tex, screen(from), screen(to)), + (Some((from, to)), Some(tex)) if out_of_date => { + let source = if raster_old { from } else { to }; + stretched_picture(painter, xf, tex, &[screen(from)], screen(source), screen(to)); + } _ => s.settled = None, } } @@ -792,7 +802,10 @@ pub(crate) fn image_input( if preview != b && let Some(tex) = view.page_raster(page) { - stretched_picture(painter, xf, tex, drawn_at.map_or(b, screen), preview); + // Its pixels: where the whole-page raster has it. + let source = left_at.filter(|_| raster_old).map_or(b, screen); + let blank: Vec = left_at.map(screen).into_iter().chain(std::iter::once(b)).collect(); + stretched_picture(painter, xf, tex, &blank, source, preview); } painter.rect_filled(visible, CornerRadius::ZERO, ACCENT.gamma_multiply(0.04)); painter.rect_stroke(visible, CornerRadius::ZERO, Stroke::new(1.5, ACCENT), egui::StrokeKind::Outside); @@ -805,7 +818,7 @@ pub(crate) fn image_input( let ((u0, v0), (u1, v1)) = (xf.screen_to_norm(r.min), xf.screen_to_norm(r.max)); Rect::from_two_pos(Pos2::new(u0, v0), Pos2::new(u1, v1)) }; - s.settled = (preview != b).then(|| (drawn_at.unwrap_or_else(|| norm(b)), norm(preview))); + s.settled = (preview != b).then(|| (left_at.unwrap_or_else(|| norm(b)), norm(preview))); } if preview != b { view.pending_edit = From 895ccb90c3b54b522844f5f7f18a721229fbe312 Mon Sep 17 00:00:00 2001 From: jonathanwxh-cell Date: Thu, 22 Oct 2026 08:28:37 +0800 Subject: [PATCH 19/20] fix(engine): guard undo and redo refresh and roll back on failure (#3925) --- crates/engine/src/lib.rs | 57 ++++++++++++++++++-- crates/engine/src/tests.rs | 107 +++++++++++++++++++++++++++++++++++++ 2 files changed, 160 insertions(+), 4 deletions(-) diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 2125a5b9d8..f28b751e1d 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -171,6 +171,23 @@ use std::sync::{Arc, OnceLock}; #[cfg(test)] thread_local! { static SECONDARY_SCAN_COUNTS: std::cell::Cell<(usize, usize)> = const { std::cell::Cell::new((0, 0)) }; + static REFRESH_PANICS: std::cell::Cell = const { std::cell::Cell::new(0) }; +} + +#[cfg(test)] +fn panic_on_refresh(count: usize) { + REFRESH_PANICS.with(|n| n.set(count)); +} + +#[cfg(test)] +fn check_refresh_panic() { + REFRESH_PANICS.with(|n| { + let rem = n.get(); + if rem > 0 { + n.set(rem - 1); + panic!("injected refresh panic"); + } + }); } #[cfg(test)] @@ -3853,14 +3870,28 @@ impl Session { pub fn undo(&mut self, id: DocId) -> Result { let doc = self.doc_mut(id)?; - doc.edit_notices.clear(); let editor = doc.editor.as_mut().ok_or(EditError::NothingToUndo)?; let (label, prev, keys, scope) = editor.undo.pop().ok_or(EditError::NothingToUndo)?; let current = std::mem::replace(&mut editor.cos, prev); let current_keys = std::mem::replace(&mut editor.keys, keys); editor.redo.push((label.clone(), current, current_keys, scope)); Self::adopt_keys(doc); - Self::refresh_scoped(doc, scope).inspect_err(|_| Self::show_released(doc))?; + let refreshed = guard(|| Self::refresh_scoped(doc, scope)).unwrap_or_else(|m| Err(EditError::Reopen(m))); + if let Err(e) = refreshed { + if let Some(ed) = doc.editor.as_mut() + && let Some((label, current, current_keys, scope)) = ed.redo.pop() + { + let prev = std::mem::replace(&mut ed.cos, current); + let keys = std::mem::replace(&mut ed.keys, current_keys); + ed.undo.push((label, prev, keys, scope)); + } + Self::adopt_keys(doc); + if !matches!(guard(|| Self::refresh(doc)), Ok(Ok(()))) { + Self::show_released(doc); + } + return Err(e); + } + doc.edit_notices.clear(); Self::sync_dirty(doc); doc.generation += 1; Ok(label) @@ -3868,14 +3899,28 @@ impl Session { pub fn redo(&mut self, id: DocId) -> Result { let doc = self.doc_mut(id)?; - doc.edit_notices.clear(); let editor = doc.editor.as_mut().ok_or(EditError::NothingToRedo)?; let (label, next, keys, scope) = editor.redo.pop().ok_or(EditError::NothingToRedo)?; let current = std::mem::replace(&mut editor.cos, next); let current_keys = std::mem::replace(&mut editor.keys, keys); editor.undo.push((label.clone(), current, current_keys, scope)); Self::adopt_keys(doc); - Self::refresh_scoped(doc, scope).inspect_err(|_| Self::show_released(doc))?; + let refreshed = guard(|| Self::refresh_scoped(doc, scope)).unwrap_or_else(|m| Err(EditError::Reopen(m))); + if let Err(e) = refreshed { + if let Some(ed) = doc.editor.as_mut() + && let Some((label, current, current_keys, scope)) = ed.undo.pop() + { + let next = std::mem::replace(&mut ed.cos, current); + let keys = std::mem::replace(&mut ed.keys, current_keys); + ed.redo.push((label, next, keys, scope)); + } + Self::adopt_keys(doc); + if !matches!(guard(|| Self::refresh(doc)), Ok(Ok(()))) { + Self::show_released(doc); + } + return Err(e); + } + doc.edit_notices.clear(); Self::sync_dirty(doc); doc.generation += 1; Ok(label) @@ -3937,6 +3982,8 @@ impl Session { doc.info.producer = info("Producer"); doc.info.custom = pdfcraft_organize::custom_info(&editor.cos); doc.info.file_size = bytes.len(); + #[cfg(test)] + check_refresh_panic(); if !doc.signatures.is_empty() { let signatures = signatures_of(&editor.cos, &bytes, &doc.trust, &doc.sig_cache); doc.signatures = Arc::new(signatures); @@ -4050,6 +4097,8 @@ impl Session { doc.renderer = RenderPool::new_inline(Arc::new(Vec::new()), doc.config.clone()); doc.display = original.clone(); doc.bytes = original; + #[cfg(test)] + check_refresh_panic(); } /// After a refresh failed: draw the display bytes it left (the original, once released), diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 080dc32dd3..808e8aaa8e 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -6147,3 +6147,110 @@ fn undo_never_makes_an_unsaved_document_clean() { s.undo(new).unwrap(); assert!(!s.get(new).unwrap().dirty, "once saved, undo back to the saved state is clean"); } + +#[test] +fn undo_and_redo_roll_back_on_a_refresh_panic_or_error() { + let (mut s, id) = session_with(1); + s.apply(id, Edit::RotatePages { pages: vec![0], degrees: 90 }).unwrap(); + let (bytes_90, gen_90) = { + let d = s.get(id).unwrap(); + (d.bytes.clone(), d.generation) + }; + + // 1. A panic during undo's refresh catches the unwind and rolls back cos, undo/redo, info, and working bytes. + panic_on_refresh(1); + let err = s.undo(id).unwrap_err(); + assert_eq!(err, EditError::Reopen("injected refresh panic".into())); + { + let d = s.get(id).unwrap(); + let ed = d.editor.as_ref().unwrap(); + assert_eq!(d.info.pages[0].rotation, 90); + assert_eq!(d.bytes, bytes_90); + assert_eq!(d.generation, gen_90); + assert!(d.dirty); + assert!(ed.cos.is_modified()); + assert_eq!(ed.undo.len(), 1); + assert!(ed.redo.is_empty()); + } + let _ = shown(&s, id, 0); + + // 2. If the rollback refresh also panics, `show_released` restores the renderer so the page still draws. + panic_on_refresh(2); + assert_eq!(s.undo(id), Err(EditError::Reopen("injected refresh panic".into()))); + assert_eq!(s.get(id).unwrap().editor.as_ref().unwrap().undo.len(), 1); + let _ = shown(&s, id, 0); + + // Undo now succeeds cleanly. + assert_eq!(s.undo(id).unwrap().as_str(), "Rotate page"); + let (bytes_0, gen_0) = { + let d = s.get(id).unwrap(); + assert_eq!(d.info.pages[0].rotation, 0); + assert!(!d.dirty); + (d.bytes.clone(), d.generation) + }; + + // 3. A panic during redo's refresh catches the unwind and rolls back to the undone state. + panic_on_refresh(1); + let err = s.redo(id).unwrap_err(); + assert_eq!(err, EditError::Reopen("injected refresh panic".into())); + { + let d = s.get(id).unwrap(); + let ed = d.editor.as_ref().unwrap(); + assert_eq!(d.info.pages[0].rotation, 0); + assert_eq!(d.bytes, bytes_0); + assert_eq!(d.generation, gen_0); + assert!(!d.dirty); + assert!(!ed.cos.is_modified()); + assert!(ed.undo.is_empty()); + assert_eq!(ed.redo.len(), 1); + } + let _ = shown(&s, id, 0); + + // 4. If redo's rollback refresh also panics, `show_released` restores the renderer. + panic_on_refresh(2); + assert_eq!(s.redo(id), Err(EditError::Reopen("injected refresh panic".into()))); + assert_eq!(s.get(id).unwrap().editor.as_ref().unwrap().redo.len(), 1); + let _ = shown(&s, id, 0); + + // Redo now succeeds cleanly. + assert_eq!(s.redo(id).unwrap().as_str(), "Rotate page"); + assert_eq!(s.get(id).unwrap().info.pages[0].rotation, 90); + + // 5. Metadata scope: a panic after `doc.info.title` was written rolls back `info.title` and keeps `edit_notices`. + s.apply(id, Edit::SetInfo { key: "Title".into(), value: "Draft".into() }).unwrap(); + let notice = TextNotice { field: None, page: 0, ch: '☺', redrawn_elsewhere: false }; + s.doc_mut(id).unwrap().edit_notices.push(notice.clone()); + panic_on_refresh(1); + assert_eq!(s.undo(id), Err(EditError::Reopen("injected refresh panic".into()))); + assert_eq!(s.get(id).unwrap().info.title.as_deref(), Some("Draft")); + assert_eq!(s.get(id).unwrap().edit_notices, [notice]); + assert_eq!(s.undo(id).unwrap().as_str(), "Change Title"); + assert_eq!(s.get(id).unwrap().info.title, None); + assert!(s.get(id).unwrap().edit_notices.is_empty()); + + panic_on_refresh(1); + assert_eq!(s.redo(id), Err(EditError::Reopen("injected refresh panic".into()))); + assert_eq!(s.get(id).unwrap().info.title, None); + assert_eq!(s.redo(id).unwrap().as_str(), "Change Title"); + assert_eq!(s.get(id).unwrap().info.title.as_deref(), Some("Draft")); + + // 6. Protection keys: undo/redo rollback restores `editor.keys` and `doc.password` before re-inspecting. + let protect = Protection { open_password: Some("u".into()), permissions_password: Some("o".into()), ..Protection::default() }; + s.apply(id, Edit::Protect(protect)).unwrap(); + assert!(s.get(id).unwrap().info.encrypted); + panic_on_refresh(1); + assert_eq!(s.undo(id), Err(EditError::Reopen("injected refresh panic".into()))); + assert!(s.get(id).unwrap().info.encrypted); + assert_eq!(s.get(id).unwrap().password.as_deref(), Some("u")); + assert_eq!(s.undo(id).unwrap().as_str(), "Protect with password"); + assert!(!s.get(id).unwrap().info.encrypted); + assert_eq!(s.get(id).unwrap().password, None); + + panic_on_refresh(1); + assert_eq!(s.redo(id), Err(EditError::Reopen("injected refresh panic".into()))); + assert!(!s.get(id).unwrap().info.encrypted); + assert_eq!(s.get(id).unwrap().password, None); + assert_eq!(s.redo(id).unwrap().as_str(), "Protect with password"); + assert!(s.get(id).unwrap().info.encrypted); + assert_eq!(s.get(id).unwrap().password.as_deref(), Some("u")); +} From c9aff05965c0c527bbc60138873658ce9602bb72 Mon Sep 17 00:00:00 2001 From: aiongg Date: Thu, 22 Oct 2026 01:45:03 +0000 Subject: [PATCH 20/20] Integration: #3864 one doc comment each for page_raster_out_of_date and page_out_of_date --- crates/ui-egui/src/canvas.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/ui-egui/src/canvas.rs b/crates/ui-egui/src/canvas.rs index 3688816098..b4164c0fd8 100644 --- a/crates/ui-egui/src/canvas.rs +++ b/crates/ui-egui/src/canvas.rs @@ -1049,14 +1049,14 @@ impl DocView { self.pages.get(&page).map(|p| p.tex.id()) } - /// Whether `page` is still drawn, somewhere, as it was before its last change: its whole-page - /// raster (a tiled page's backdrop too) or any of its tiles waits for a replacement. /// Whether `page`'s whole-page raster (a tiled page's backdrop too) is still the one from /// before its last change, whatever its tiles are. pub(crate) fn page_raster_out_of_date(&self, page: usize) -> bool { self.pages.get(&page).is_some_and(|p| p.tag == STALE_TAG) } + /// Whether `page` is still drawn, somewhere, as it was before its last change: its whole-page + /// raster (a tiled page's backdrop too) or any of its tiles waits for a replacement. pub(crate) fn page_out_of_date(&self, page: usize) -> bool { self.pages.get(&page).is_some_and(|p| p.tag == STALE_TAG) || self.stale_tiles.iter().any(|k| k.0 == page) }