From ff526a78961204c6853a29b1eb2601f5cdb88a1f Mon Sep 17 00:00:00 2001 From: Verix <77591428+jcnnik@users.noreply.github.com> Date: Thu, 15 Oct 2026 17:14:52 +0200 Subject: [PATCH 01/12] M3: single-page view turns pages without a blank frame At Fit Page in single-page view, every wheel turn forward drew one blank frame before the new page appeared. - Lay out the shown row again after the wheel is read. The jump to the new page was measured from the old row's place and scrolled both out of view; going back it came out negative and was clamped, so only forward turns flickered. The edge-turn deferral that worked around this goes. - Prefetch the rows either side of the shown one, so the next page is rendered before the turn instead of showing blank paper. --- crates/ui-egui/src/canvas.rs | 34 ++++++++++++++++++++++++++++------ crates/ui-egui/tests/view.rs | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+), 6 deletions(-) diff --git a/crates/ui-egui/src/canvas.rs b/crates/ui-egui/src/canvas.rs index 989b731b7..27091ba68 100644 --- a/crates/ui-egui/src/canvas.rs +++ b/crates/ui-egui/src/canvas.rs @@ -2308,9 +2308,10 @@ pub fn document_area(app: &mut PdfCraftApp, index: usize, ui: &mut egui::Ui) { // When not scrolling continuously, only one row is shown at a time: the current page, or the // current two-page spread (with the cover rule). These indices drive paging, which pages are // drawn, and the content height (#752). Empty for a document with no pages. - let row: Vec = if rects.is_empty() { - Vec::new() - } else { + let shown_row = |view: &DocView| -> Vec { + if rects.is_empty() { + return Vec::new(); + } let last = rects.len() - 1; let cur = view.current.min(last); match view.layout { @@ -2322,6 +2323,7 @@ pub fn document_area(app: &mut PdfCraftApp, index: usize, ui: &mut egui::Ui) { } } }; + let row = shown_row(view); let middle_gesture = view.auto_scroll.blocks_input(); // Not scrolling: when the whole page or spread fits, the wheel would do nothing, so it turns // pages instead; zoomed in far enough to pan, it pans, and at the edge it turns the page on @@ -2422,6 +2424,9 @@ pub fn document_area(app: &mut PdfCraftApp, index: usize, ui: &mut egui::Ui) { if view.fling.gliding() { ui.ctx().request_repaint(); } + // The wheel may have turned the page above: draw the row it turned to. Measured from the old + // row, the jump to the new page scrolled both out of view, a blank frame on every turn forward. + let row = shown_row(view); let visible_pages: Vec = if view.continuous { (0..rects.len()).collect() } else { row.clone() }; let (y_shift, content_h) = if view.continuous { (0.0, rects.iter().map(|r| r.bottom()).reduce(f32::max).map(|bottom| bottom + MARGIN).unwrap_or(0.0)) @@ -2461,9 +2466,6 @@ pub fn document_area(app: &mut PdfCraftApp, index: usize, ui: &mut egui::Ui) { let r = rects[page.min(rects.len() - 1)]; let point = pos2(r.left() + fx * r.width(), r.top() - y_shift + fy * r.height()); scroll = scroll.scroll_offset(vec2((point.x - rel.x).max(0.0), (point.y - rel.y).max(0.0))); - } else if edge_turned { - // The row above was laid out for the page before the turn: place the new one next frame. - ui.ctx().request_repaint(); } else if let Some((page, frac)) = view.goto.take() { let page = page.min(rects.len() - 1); let r = rects[page]; @@ -3481,6 +3483,26 @@ pub fn document_area(app: &mut PdfCraftApp, index: usize, ui: &mut egui::Ui) { view.last_scroll = scroll_offset; let pan_held = now_settle < view.pan_hold_until; let (mut wanted, visible_now) = out.inner; + // Single-page and two-page views draw only the current row, so the next one was first asked + // for on the turn and showed blank paper until it rendered. Prefetch the rows either side, + // as continuous scrolling does for the pages just off screen. + if !view.continuous + && let (Some(&first), Some(&last)) = (visible_pages.first(), visible_pages.last()) + { + let n = if view.layout == PageLayout::Single { 1 } else { 2 }; + let max_side = ui.ctx().input(|inp| inp.max_texture_side) as f32; + for i in (last.saturating_add(1)..=last.saturating_add(n)).chain(first.saturating_sub(n)..first) { + let Some(p) = info.pages.get(i).filter(|_| !view.errors.contains_key(&i)) else { continue }; + // As drawn: the whole page at this scale, or the backdrop of a tiled page. + let side = p.width.max(p.height).max(1.0); + wanted.push(if side * scale > TILE_THRESHOLD.min(max_side) { + let bs = BASE_SIDE.min(max_side) / side; + (i, bs, scale_tag(bs), None) + } else { + (i, scale, tag, None) + }); + } + } let cur = view.current; wanted.sort_by_key(|w| (!visible_now.contains(&w.0), w.0.abs_diff(cur), w.3.is_some())); // Tiles of far-away pages are useless: free them. Tiles of an earlier zoom stay for a page diff --git a/crates/ui-egui/tests/view.rs b/crates/ui-egui/tests/view.rs index 3fdeff637..42cc9cde8 100644 --- a/crates/ui-egui/tests/view.rs +++ b/crates/ui-egui/tests/view.rs @@ -185,6 +185,38 @@ fn single_page_layout_shows_one_page_at_a_time() { assert!(rect(&h, 0).is_none() && rect(&h, 1).is_some(), "only page 2"); } +#[test] +fn single_page_view_prefetches_the_next_page() { + // Only the shown page is drawn, but the next one renders ahead of the turn: otherwise + // turning forward shows blank paper until it arrives. + let mut h = harness(&[("layout", "single"), ("zoom", "50")]); + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(10); + while h.state().render_pending() { + assert!(std::time::Instant::now() < deadline, "renders settle"); + h.run_steps(2); + std::thread::sleep(std::time::Duration::from_millis(5)); + } + assert!(rect(&h, 1).is_none(), "page 2 is not shown"); + assert!(h.state().views[0].raster_memory().page_count >= 2, "page 2 is rendered ahead of the turn"); +} + +#[test] +fn single_page_wheel_turn_draws_the_new_page_at_once() { + // The frame that turns the page draws the new one. Its jump used to be measured from the + // old page's place, which scrolled both out of view: a blank frame on every turn forward. + let mut h = harness_stepping(1.0 / 60.0, &[("layout", "single")]); + h.key_press_modifiers(Modifiers::COMMAND, Key::Num0); // fit page + h.run_steps(4); + let vp = h.state().views[0].viewport_rect(); + h.hover_at(vp.center()); + h.run_steps(2); + h.event(wheel(MouseWheelUnit::Line, -1.0, TouchPhase::Move)); + h.step(); + assert_eq!(h.state().views[0].current, 1, "one notch turns one page"); + let r = rect(&h, 1).expect("page 2 drawn in the frame that turns to it"); + assert!(r.min.y >= vp.min.y && r.max.y <= vp.max.y, "page 2 fits on screen: {r:?} in {vp:?}"); +} + #[test] fn single_page_wheel_pans_instead_when_zoomed_in() { // The zoomed-in guard: with room to pan, the wheel pans and must not turn pages. From bdb943dc3d4871ba4d980efbb0583ea60d71562e Mon Sep 17 00:00:00 2001 From: Verix <77591428+jcnnik@users.noreply.github.com> Date: Thu, 15 Oct 2026 17:28:46 +0200 Subject: [PATCH 02/12] M3: a page jumped to in single-page view no longer hops into place The jump scrolls to MARGIN - GAP above the page. A page that fits has nothing to scroll, and egui clamps the offset only after drawing the frame, so the page showed 14 pt too high for a frame and then hopped down. Clamp the jump to the scroll range before handing it to egui. --- crates/ui-egui/src/canvas.rs | 6 +++++- crates/ui-egui/tests/view.rs | 15 +++++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/crates/ui-egui/src/canvas.rs b/crates/ui-egui/src/canvas.rs index 989b731b7..4fb6afb94 100644 --- a/crates/ui-egui/src/canvas.rs +++ b/crates/ui-egui/src/canvas.rs @@ -2467,7 +2467,11 @@ pub fn document_area(app: &mut PdfCraftApp, index: usize, ui: &mut egui::Ui) { } else if let Some((page, frac)) = view.goto.take() { let page = page.min(rects.len() - 1); let r = rects[page]; - scroll = scroll.vertical_scroll_offset((r.top() - y_shift - GAP + frac * r.height()).max(0.0)); + // Clamped here: egui clamps only after drawing, so a page that fits was drawn MARGIN - GAP + // too high for a frame and then hopped into place. (`avail` is the viewport, as the + // scroll bars float.) + let max_y = (content_h - avail.height()).max(0.0); + scroll = scroll.vertical_scroll_offset((r.top() - y_shift - GAP + frac * r.height()).max(0.0).min(max_y)); } let ppp = ui.ctx().pixels_per_point(); let scale = view.render_scale(ppp); diff --git a/crates/ui-egui/tests/view.rs b/crates/ui-egui/tests/view.rs index 3fdeff637..b492d09b0 100644 --- a/crates/ui-egui/tests/view.rs +++ b/crates/ui-egui/tests/view.rs @@ -185,6 +185,21 @@ fn single_page_layout_shows_one_page_at_a_time() { assert!(rect(&h, 0).is_none() && rect(&h, 1).is_some(), "only page 2"); } +#[test] +fn single_page_jump_draws_a_fitting_page_where_it_stays() { + // A page that fits has nothing to scroll, but the jump to it asked for the gap above it: + // drawn that much too high for a frame, it then hopped into place. + let mut h = harness_stepping(1.0 / 60.0, &[("layout", "single")]); + h.key_press_modifiers(Modifiers::COMMAND, Key::Num0); // fit page + h.run_steps(4); + h.state_mut().set_option("page", "2").unwrap(); + h.step(); + let first = rect(&h, 1).expect("page 2 drawn in the frame after the jump"); + h.run_steps(4); + let settled = rect(&h, 1).expect("page 2"); + assert!((first.min.y - settled.min.y).abs() < 0.5, "page 2 drawn where it settles: {first:?} then {settled:?}"); +} + #[test] fn single_page_wheel_pans_instead_when_zoomed_in() { // The zoomed-in guard: with room to pan, the wheel pans and must not turn pages. From 64ab6b67ecc4a5fbabffb4e4930733d9fba46136 Mon Sep 17 00:00:00 2001 From: Mateus Bessa Date: Thu, 15 Oct 2026 15:14:30 -0300 Subject: [PATCH 03/12] fix(ui): Copy in the selection action bar closes the bar MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The bar beside selected text closed after Highlight, Underline, Strikethrough, Add comment and Replace text because those consume the selection. Copy kept the selection, so the bar stayed open. Remember the copied selection and hide the bar until the selection changes; the words stay selected for ⌘C or another action. Co-Authored-By: Claude Opus 5.5 --- crates/ui-egui/src/canvas.rs | 15 ++++++++++++++- crates/ui-egui/tests/comments.rs | 15 +++++++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/crates/ui-egui/src/canvas.rs b/crates/ui-egui/src/canvas.rs index 989b731b7..5c0bdbfc5 100644 --- a/crates/ui-egui/src/canvas.rs +++ b/crates/ui-egui/src/canvas.rs @@ -371,6 +371,9 @@ pub struct DocView { pub(crate) text_failed: HashSet, pub find: Option, selection: Option, + /// The selection whose action bar was closed by its Copy button: the bar stays hidden + /// until the selection changes, as it does once a markup button consumes the selection. + actions_dismissed: Option, /// The drag in progress started a text selection: the wheel scrolls the page during it /// (#721), which egui's scroll area refuses while anything is dragged. selection_drag: bool, @@ -649,6 +652,7 @@ impl DocView { text_failed: HashSet::new(), find: None, selection: None, + actions_dismissed: None, selection_drag: false, clicks: 0, last_press_time: 0.0, @@ -955,6 +959,7 @@ impl DocView { pub(crate) fn clear_selection(&mut self) { self.selection = None; + self.actions_dismissed = None; } /// Select text on `page` from glyph `from` to glyph `to` (tests and automation). @@ -3794,7 +3799,13 @@ fn selection_actions( if ui.input(|input| input.pointer.is_decidedly_dragging()) { return false; } - let Some(selection) = view.selection else { return false }; + let Some(selection) = view.selection else { + view.actions_dismissed = None; + return false; + }; + if view.actions_dismissed == Some(selection) { + return false; + } let Some(text) = view.texts.get(&selection.page) else { return false }; let Some(xf) = view.page_xform(selection.page) else { return false }; let Some(last) = selection.rects(text).last().copied() else { return false }; @@ -3831,6 +3842,8 @@ fn selection_actions( }); let Some(action) = chosen else { return false }; if let SelectionAction::Copy = action { + // Copying closes the bar but keeps the words selected, for ⌘C or a second action. + view.actions_dismissed = Some(selection); return true; } let Some((page, quads)) = view.selection_quads(info) else { return false }; diff --git a/crates/ui-egui/tests/comments.rs b/crates/ui-egui/tests/comments.rs index 914bc281b..aaea7990f 100644 --- a/crates/ui-egui/tests/comments.rs +++ b/crates/ui-egui/tests/comments.rs @@ -364,6 +364,21 @@ fn selected_text_shows_actions_and_underlines_without_changing_tools() { assert_eq!(h.state().quick_tool, QuickTool::Select); } +#[test] +fn selected_text_copy_action_closes_the_bar_and_keeps_the_selection() { + let mut h = harness(|_| {}); + h.state_mut().views[0].select_text(0, 4, 8); + h.run_steps(3); + h.get_by_label("Copy").click(); + h.run_steps(3); + assert!(h.query_by_label("Add comment").is_none(), "the action bar closes after Copy"); + assert!(h.state().views[0].selected_text().is_some_and(|t| !t.is_empty()), "the words stay selected"); + // A new selection brings the bar back. + h.state_mut().views[0].select_text(0, 2, 6); + h.run_steps(3); + h.get_by_label("Add comment"); +} + #[test] fn selected_text_replace_action_opens_replacement_composer() { let mut h = harness(|_| {}); From 4a991b03b5ad2b755ac89e4fefb3f64ff5a34df9 Mon Sep 17 00:00:00 2001 From: outragedevs <243349561+outragedevs@users.noreply.github.com> Date: Thu, 15 Oct 2026 20:21:57 +0200 Subject: [PATCH 04/12] M7: Let the page range select page 1 alone MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Seitenbereich „1 bis 1“ springt nicht mehr auf die letzte Seite zurück. --- crates/ui-egui/src/marks_ui.rs | 12 ++++++--- crates/ui-egui/tests/ui_group/marks.rs | 34 ++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 4 deletions(-) diff --git a/crates/ui-egui/src/marks_ui.rs b/crates/ui-egui/src/marks_ui.rs index 4a60c40a6..d91a69f93 100644 --- a/crates/ui-egui/src/marks_ui.rs +++ b/crates/ui-egui/src/marks_ui.rs @@ -28,8 +28,10 @@ pub struct PageRange { } impl Default for PageRange { + /// From the first page to the last: `to` starts past any page count, and the range field + /// shows (and clamps it to) the document's last page. fn default() -> Self { - Self { all: true, from: 1, to: 1, subset: Subset::All } + Self { all: true, from: 1, to: usize::MAX, subset: Subset::All } } } @@ -53,9 +55,6 @@ impl PageRange { widgets::radio_value(ui, &mut self.all, false, tl!("Pages from")); ui.add_enabled(!self.all, egui::DragValue::new(&mut self.from).range(1..=count.max(1))); ui.label(tl!("to")); - if self.to == 1 && self.from == 1 { - self.to = count.max(1); - } ui.add_enabled(!self.all, egui::DragValue::new(&mut self.to).range(1..=count.max(1))); ui.label(tl!("Subset")); let label = match self.subset { @@ -519,6 +518,11 @@ mod tests { assert_eq!(r.pages(6), [1, 3, 5]); let r = PageRange { all: true, subset: Subset::Odd, ..PageRange::default() }; assert_eq!(r.pages(5), [0, 2, 4]); + // Not all pages: by default up to the last one; a single page is a range too. + let r = PageRange { all: false, ..PageRange::default() }; + assert_eq!(r.pages(4), [0, 1, 2, 3]); + let r = PageRange { all: false, from: 1, to: 1, subset: Subset::All }; + assert_eq!(r.pages(38), [0]); } #[test] diff --git a/crates/ui-egui/tests/ui_group/marks.rs b/crates/ui-egui/tests/ui_group/marks.rs index 0a0e45b15..1f1515ad6 100644 --- a/crates/ui-egui/tests/ui_group/marks.rs +++ b/crates/ui-egui/tests/ui_group/marks.rs @@ -96,3 +96,37 @@ fn backgrounds_and_watermarks_from_a_file() { let s = h.state(); assert_eq!(s.session.get(s.views[0].id).unwrap().can_undo(), Some("Add background")); } + +/// Pages from 1 to 1 stays one page: the dialog used to turn "to" back into the last page on +/// every frame while both were 1, so only page 1 could not be chosen alone. The field starts on +/// the last page. +#[test] +fn page_range_from_one_to_one_selects_only_the_first_page() { + let mut h = harness(); + h.state_mut().execute("edit.background"); + h.run_steps(2); + assert_eq!(h.state().marks_draft.range.pages(3), [0, 1, 2]); + h.get_by_label("Pages from").click(); + h.run_steps(2); + let range = h.state().marks_draft.range; + assert_eq!((range.from, range.to), (1, 3), "from the first page to the last"); + h.state_mut().marks_draft.range.to = 1; + h.run_steps(3); + let range = h.state().marks_draft.range; + assert_eq!((range.from, range.to), (1, 1), "the range stays as typed"); + h.get_by_label("Applies to 1 page"); + h.get_by_label("OK").click(); + h.run_steps(3); + let s = h.state(); + let doc = s.session.get(s.views[0].id).unwrap(); + assert_eq!(doc.marks(), [MarkKind::Background]); + // Rendered: page 1 has the (default cream) background, page 2 is still blank. + let mut r = pdfcraft_render::PageRenderer::new(doc.bytes.clone(), pdfcraft_render::RenderConfig::default()); + let corner = |r: &mut pdfcraft_render::PageRenderer, page: usize| { + let p = r.render(pdfcraft_render::RenderRequest { page, scale: 1.0, ..Default::default() }); + assert!(p.error.is_none(), "{:?}", p.error); + p.rgba[..4].to_vec() + }; + assert_eq!(corner(&mut r, 0), vec![255, 255, 217, 255], "page 1 has the background"); + assert_eq!(corner(&mut r, 1), vec![255, 255, 255, 255], "page 2 does not"); +} From 2dd976b90187e8e676da2aa0bce60c6cf3fb4809 Mon Sep 17 00:00:00 2001 From: Marco Arnulfo Date: Fri, 16 Oct 2026 00:36:24 +0200 Subject: [PATCH 05/12] Go to page brings the page into view horizontally too (part of #2665) --- crates/ui-egui/src/canvas.rs | 13 +++++++++++++ crates/ui-egui/tests/viewer.rs | 24 ++++++++++++++++++++++++ 2 files changed, 37 insertions(+) diff --git a/crates/ui-egui/src/canvas.rs b/crates/ui-egui/src/canvas.rs index 989b731b7..136619a35 100644 --- a/crates/ui-egui/src/canvas.rs +++ b/crates/ui-egui/src/canvas.rs @@ -2468,6 +2468,19 @@ pub fn document_area(app: &mut PdfCraftApp, index: usize, ui: &mut egui::Ui) { let page = page.min(rects.len() - 1); let r = rects[page]; scroll = scroll.vertical_scroll_offset((r.top() - y_shift - GAP + frac * r.height()).max(0.0)); + // Horizontally too, as Acrobat does (#2665): beside a much wider page the others sit off + // to one side. A page already shown whole, or one filling the window's width, keeps the + // scroll, so paging through a zoomed-in document stays on the same column. Otherwise a + // page that fits is centred, and a wider one scrolled just far enough to fill the window. + let (x, w) = (view.last_scroll.x, avail.width()); + let target = if r.width() <= w { + if r.left() >= x && r.right() <= x + w { x } else { r.center().x - w / 2.0 } + } else { + x.clamp(r.left(), r.right() - w) + }; + if (target - x).abs() > 0.5 { + scroll = scroll.horizontal_scroll_offset(target.clamp(0.0, (content_w - w).max(0.0))); + } } let ppp = ui.ctx().pixels_per_point(); let scale = view.render_scale(ppp); diff --git a/crates/ui-egui/tests/viewer.rs b/crates/ui-egui/tests/viewer.rs index 077553c71..973352ade 100644 --- a/crates/ui-egui/tests/viewer.rs +++ b/crates/ui-egui/tests/viewer.rs @@ -1738,3 +1738,27 @@ fn arrow_keys_follow_the_scroll_speed_preference() { let (slow, normal, fast) = (lines_to_page_2("slow"), lines_to_page_2("normal"), lines_to_page_2("fast")); assert!(fast * 3 < normal * 2 && normal * 3 < slow * 2, "presses to page 2: slow {slow}, normal {normal}, fast {fast}"); } + +/// #2665: next to a very wide page the normal pages sit far to its right, outside the window. +/// Going to one changed the page counter but kept the horizontal scroll, so it stayed out of +/// sight; as in Acrobat, it is brought into view. +#[test] +fn go_to_page_brings_the_page_into_view_horizontally() { + use pdfcraft_ui_egui::canvas::Fit; + let mut h = Harness::builder().with_size(egui::vec2(1400.0, 900.0)).build_eframe(|_cc| { + let mut app = PdfCraftApp::new(); + app.set_option("language", "en").unwrap(); + app.open_bytes("wide.pdf", None, sized_fixture(&[(595, 842), (595, 842), (595, 842), (6000, 2000)])).unwrap(); + app.views[0].fit = Fit::None; + app.views[0].zoom = 1.0; + app + }); + h.run_steps(4); + assert!(h.state().views[0].page_screen_rect(0).is_none(), "page 1 starts off-screen, right of the window"); + h.state_mut().views[0].go_to_page(2); + h.run_steps(4); + let v = &h.state().views[0]; + assert_eq!(v.current, 2); + let (r, vp) = (v.page_screen_rect(2).expect("page 3 on screen"), v.viewport_rect()); + assert!(r.left() >= vp.left() && r.right() <= vp.right(), "page 3 at {r:?}, viewport {vp:?}"); +} From b78724be05ea321a32c03a4811b5d2047333329b Mon Sep 17 00:00:00 2001 From: Marco Arnulfo Date: Fri, 16 Oct 2026 00:57:01 +0200 Subject: [PATCH 06/12] Go to page keeps the side gutter clear of the quick-action bar (part of #2665) --- crates/ui-egui/src/canvas.rs | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/crates/ui-egui/src/canvas.rs b/crates/ui-egui/src/canvas.rs index 136619a35..7747fdf00 100644 --- a/crates/ui-egui/src/canvas.rs +++ b/crates/ui-egui/src/canvas.rs @@ -2472,12 +2472,11 @@ pub fn document_area(app: &mut PdfCraftApp, index: usize, ui: &mut egui::Ui) { // to one side. A page already shown whole, or one filling the window's width, keeps the // scroll, so paging through a zoomed-in document stays on the same column. Otherwise a // page that fits is centred, and a wider one scrolled just far enough to fill the window. + // The page counts with its side gutters, which keep its edge clear of the floating + // quick-action bar as the layout does. let (x, w) = (view.last_scroll.x, avail.width()); - let target = if r.width() <= w { - if r.left() >= x && r.right() <= x + w { x } else { r.center().x - w / 2.0 } - } else { - x.clamp(r.left(), r.right() - w) - }; + let (left, right) = (r.left() - SIDE, r.right() + SIDE); + let target = if right - left <= w { if left >= x && right <= x + w { x } else { r.center().x - w / 2.0 } } else { x.clamp(left, right - w) }; if (target - x).abs() > 0.5 { scroll = scroll.horizontal_scroll_offset(target.clamp(0.0, (content_w - w).max(0.0))); } From bf1ce2a07cf78d0df29a7b39b5607c574839ab0b Mon Sep 17 00:00:00 2001 From: mtlcl410 Date: Fri, 16 Oct 2026 07:18:35 +0800 Subject: [PATCH 07/12] =?UTF-8?q?M3:=20=E2=8C=98G=20and=20=E2=87=A7?= =?UTF-8?q?=E2=8C=98G=20step=20through=20matches=20from=20the=20find=20fie?= =?UTF-8?q?ld=20(#2669)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The find-step shortcuts sat after the canvas's "egui wants keyboard input" check, so they did nothing while the find field had the focus (Enter worked). They now run before it, while a search is open, as in Acrobat and Preview. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01RUNQSPVXx3dDBabRZHAnjz --- crates/ui-egui/src/canvas.rs | 18 +++++++++++------- crates/ui-egui/tests/viewer.rs | 30 ++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 7 deletions(-) diff --git a/crates/ui-egui/src/canvas.rs b/crates/ui-egui/src/canvas.rs index 989b731b7..360d317bd 100644 --- a/crates/ui-egui/src/canvas.rs +++ b/crates/ui-egui/src/canvas.rs @@ -2072,11 +2072,20 @@ pub fn shortcuts(view: &mut DocView, ctx: &egui::Context, speed: crate::scroll_s if ctx.input_mut(|i| i.consume_shortcut(&KeyboardShortcut::new(Modifiers::COMMAND, Key::F))) { view.open_find(); } + let cmd = |k| KeyboardShortcut::new(Modifiers::COMMAND, k); + let pressed = |s: KeyboardShortcut| ctx.input_mut(|i| i.consume_shortcut(&s)); + // ⌘G / ⇧⌘G step through the matches from the find field too, as in Acrobat and Preview + // (#2669): ahead of the text-input check, but only while there is a search to step. + if view.find.is_some() { + if pressed(KeyboardShortcut::new(Modifiers::COMMAND | Modifiers::SHIFT, Key::G)) { + view.find_step(false); + } else if pressed(cmd(Key::G)) { + view.find_step(true); + } + } if ctx.egui_wants_keyboard_input() { return; } - let cmd = |k| KeyboardShortcut::new(Modifiers::COMMAND, k); - let pressed = |s: KeyboardShortcut| ctx.input_mut(|i| i.consume_shortcut(&s)); // Rotation first: egui matches ⌘+ loosely with respect to Shift. if pressed(KeyboardShortcut::new(Modifiers::COMMAND | Modifiers::SHIFT, Key::Plus)) || pressed(KeyboardShortcut::new(Modifiers::COMMAND | Modifiers::SHIFT, Key::Equals)) @@ -2117,11 +2126,6 @@ pub fn shortcuts(view: &mut DocView, ctx: &egui::Context, speed: crate::scroll_s view.fit = Fit::Width; view.goto = Some((view.current, 0.0)); } - if pressed(KeyboardShortcut::new(Modifiers::COMMAND | Modifiers::SHIFT, Key::G)) { - view.find_step(false); - } else if pressed(cmd(Key::G)) { - view.find_step(true); - } if pressed(cmd(Key::OpenBracket)) { view.view_history(false); } diff --git a/crates/ui-egui/tests/viewer.rs b/crates/ui-egui/tests/viewer.rs index 077553c71..36c1f07db 100644 --- a/crates/ui-egui/tests/viewer.rs +++ b/crates/ui-egui/tests/viewer.rs @@ -175,6 +175,36 @@ fn find_shortcuts_advance_and_retreat_matches() { assert_eq!(h.state().views[0].find.as_ref().unwrap().current, Some((start + n - 1) % n)); } +/// #2669: ⌘G and ⇧⌘G step through the matches while the find field still has the focus. +#[test] +fn find_shortcuts_work_from_the_find_field() { + let mut h = harness(); + h.state_mut().active = Some(0); + h.run_steps(2); + h.state_mut().views[0].open_find(); + h.state_mut().views[0].find.as_mut().unwrap().query = "page".into(); + h.state_mut().views[0].rerun_find(); + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(20); + while std::time::Instant::now() < deadline { + h.run_steps(2); + std::thread::sleep(std::time::Duration::from_millis(5)); + if h.state().views[0].find.as_ref().unwrap().matches.len() >= 15 { + break; + } + } + let n = h.state().views[0].find.as_ref().unwrap().matches.len(); + assert_eq!(n, 15); + assert!(h.ctx.egui_wants_keyboard_input(), "the find field has the focus"); + let start = h.state().views[0].find.as_ref().unwrap().current.unwrap(); + h.key_press_modifiers(egui::Modifiers::COMMAND, egui::Key::G); + h.run_steps(2); + assert_eq!(h.state().views[0].find.as_ref().unwrap().current, Some((start + 1) % n)); + h.key_press_modifiers(egui::Modifiers::COMMAND | egui::Modifiers::SHIFT, egui::Key::G); + h.run_steps(2); + assert_eq!(h.state().views[0].find.as_ref().unwrap().current, Some(start)); + assert!(h.ctx.egui_wants_keyboard_input(), "and keeps it"); +} + #[test] fn marking_while_searching_does_not_jitter_or_reset_search_state() { let mut h = harness(); From c66384f92ba0042a20196b42c5d0a50f2c94d3aa Mon Sep 17 00:00:00 2001 From: Enrico Sita Date: Fri, 16 Oct 2026 10:27:59 +0200 Subject: [PATCH 08/12] fix(ui): Read mode hides the quick toolbar (#2646) Read mode hides the tools, not the navigation panes, as Acrobat's does and as switch_mode already says, but the floating quick toolbar was still drawn over the page there. It is now hidden in Read mode, and entering Read mode puts the current tool down (back to Select), so no markup tool stays active without a toolbar to show it. The right rail, the page navigation and the V/H shortcuts are unchanged. Co-Authored-By: Claude Opus 5.5 --- crates/ui-egui/src/canvas.rs | 5 ++++- crates/ui-egui/src/lib.rs | 2 ++ crates/ui-egui/tests/ui.rs | 18 ++++++++++++++++++ 3 files changed, 24 insertions(+), 1 deletion(-) diff --git a/crates/ui-egui/src/canvas.rs b/crates/ui-egui/src/canvas.rs index 0acab64a0..b5db6b462 100644 --- a/crates/ui-egui/src/canvas.rs +++ b/crates/ui-egui/src/canvas.rs @@ -3893,7 +3893,10 @@ pub fn document_area(app: &mut PdfCraftApp, index: usize, ui: &mut egui::Ui) { if copy_selected { app.execute("edit.copy"); } - quick_bar(app, avail, ui); + // Read mode hides the tools (#2646). + if app.mode != crate::Mode::Read { + quick_bar(app, avail, ui); + } if let Some((source, file, destination)) = remote_open { app.open_remote_link(source, file, destination); } diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index 79464624c..3ce3f76df 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -2169,6 +2169,7 @@ impl PdfCraftApp { /// Switches workspace because the user chose to (mode bar, Read mode command). Entering Read /// mode closes the right panel, as Acrobat's Read mode hides its panes, and leaving it reopens /// that panel. The rail stays in Read mode, and its buttons open panels there too (#932). + /// Read mode hides the quick toolbar, so entering it also puts the current tool down. pub(crate) fn switch_mode(&mut self, mode: Mode, select_tools: bool) { let was_read = self.mode == Mode::Read; if select_tools { @@ -2178,6 +2179,7 @@ impl PdfCraftApp { } if mode == Mode::Read && !was_read { self.right_before_read = self.right.take(); + self.quick_tool = QuickTool::Select; } else if mode != Mode::Read && was_read { let before = self.right_before_read.take(); if self.right.is_none() { diff --git a/crates/ui-egui/tests/ui.rs b/crates/ui-egui/tests/ui.rs index 1b6276d7c..cc52b01a0 100644 --- a/crates/ui-egui/tests/ui.rs +++ b/crates/ui-egui/tests/ui.rs @@ -246,6 +246,24 @@ fn read_mode_closes_the_panes_and_leaving_it_brings_them_back() { assert_eq!(h.state().right, Some(RightPanel::Pages)); } +/// Read mode hides the quick toolbar too, and puts down the tool that was in use (#2646). +#[test] +fn read_mode_hides_the_quick_toolbar() { + use pdfcraft_ui_egui::{Mode, QuickTool}; + let mut h = harness(|app| app.open_bytes("fixture.pdf", None, FIXTURE.to_vec()).expect("fixture opens")); + h.get_by_label("Hand (H)").click(); + h.run_steps(2); + assert_eq!(h.state().quick_tool, QuickTool::Hand); + h.get_by_label("Read").click(); + h.run_steps(3); + assert!(h.query_by_label("Select (V)").is_none(), "no quick toolbar in Read mode"); + assert_eq!(h.state().quick_tool, QuickTool::Select); + h.state_mut().run_command("view.read_mode"); + h.run_steps(3); + assert_eq!(h.state().mode, Mode::AllTools); + h.get_by_label("Select (V)"); +} + /// A pane closed in Read mode stays closed on the way out: leaving Read mode doesn't bring back the /// pane saved on the way in, and closing Comments keeps its close preference. #[test] From da48b84b8cb00de3cb2bd045c8e41694db1fb649 Mon Sep 17 00:00:00 2001 From: hade0ntema <296480886+hade0ntema@users.noreply.github.com> Date: Fri, 16 Oct 2026 10:33:24 +0200 Subject: [PATCH 09/12] fix(select): a picked picture that is edited away no longer leaves its highlight behind With the Select tool a click picks a picture and draws a blue highlight over it. The pick was only cleared by Escape, a drag or another click. Picking a picture, then entering Edit text & images and deleting it left the highlight on the empty spot, with nothing left to click to remove it. The pick now remembers the document's edit generation and is dropped once the document has been edited since (a delete, a move, Undo), the same condition under which the copy shortcut already refused it as stale. Co-Authored-By: Claude Sonnet 5.5 --- crates/ui-egui/src/canvas.rs | 1 + crates/ui-egui/src/image_pick.rs | 16 ++++++++++++++-- crates/ui-egui/tests/viewer.rs | 29 +++++++++++++++++++++++++++++ 3 files changed, 44 insertions(+), 2 deletions(-) diff --git a/crates/ui-egui/src/canvas.rs b/crates/ui-egui/src/canvas.rs index 0acab64a0..7759accf4 100644 --- a/crates/ui-egui/src/canvas.rs +++ b/crates/ui-egui/src/canvas.rs @@ -3275,6 +3275,7 @@ pub fn document_area(app: &mut PdfCraftApp, index: usize, ui: &mut egui::Ui) { if tool == QuickTool::Select && !consumed { crate::image_pick::page_input(&resp, &xf, i, info, doc, pointer, view); } + crate::image_pick::drop_stale(doc, view); crate::image_pick::paint(painter, &xf, i, info, view); comments::page_after_text(&resp, &pcx, view); if tool == QuickTool::Redact && can_modify { diff --git a/crates/ui-egui/src/image_pick.rs b/crates/ui-egui/src/image_pick.rs index a05aefe2d..53e4bdf15 100644 --- a/crates/ui-egui/src/image_pick.rs +++ b/crates/ui-egui/src/image_pick.rs @@ -9,12 +9,14 @@ use crate::PdfCraftApp; use crate::canvas::{DocView, PageXform}; use pdfcraft_render::DocInfo; -/// The selected picture: page, index in `page_images` and its box in user space. +/// The selected picture: page, index in `page_images` and its box in user space, and the +/// document's edit generation when it was picked (any later edit may have removed or renumbered it). #[derive(Clone, Copy, Debug, PartialEq)] pub struct PickedImage { pub page: usize, pub index: usize, pub rect: [f64; 4], + pub generation: u64, } const BLUE: Color32 = Color32::from_rgb(0x3A, 0x7B, 0xF0); @@ -43,8 +45,18 @@ pub(crate) fn page_input( let (vx, vy) = xf.screen_to_view(p); let u = info_page.view_to_user(vx, vy); let at = [f64::from(u[0]), f64::from(u[1])]; + let generation = doc.edit_generation(); view.picked_image = - doc.page_image_at(page, at).and_then(|index| doc.page_images(page).get(index).map(|i| PickedImage { page, index, rect: i.rect })); + doc.page_image_at(page, at).and_then(|index| doc.page_images(page).get(index).map(|i| PickedImage { page, index, rect: i.rect, generation })); +} + +/// Forget the selected picture once the document has been edited since it was picked: Edit text & +/// images (or Undo) may have deleted or moved it, and its highlight would stay behind on an empty +/// spot of the page. +pub(crate) fn drop_stale(doc: &pdfcraft_engine::Document, view: &mut DocView) { + if view.picked_image.is_some_and(|p| p.generation != doc.edit_generation()) { + view.picked_image = None; + } } /// The selected picture's highlight on page `page`. diff --git a/crates/ui-egui/tests/viewer.rs b/crates/ui-egui/tests/viewer.rs index 474360865..aadf6c56d 100644 --- a/crates/ui-egui/tests/viewer.rs +++ b/crates/ui-egui/tests/viewer.rs @@ -1384,6 +1384,35 @@ fn a_clicked_picture_is_selected_and_copied_in_its_original_format() { assert_eq!(h.state().views[i].picked_image, None); } +/// A picture picked with the Select tool, then deleted in Edit text & images, leaves no highlight +/// behind on the empty spot where it was. +#[test] +fn a_picked_picture_that_is_deleted_in_edit_mode_loses_its_highlight() { + let (_, mut h) = picture_harness(); + let i = h.state().active.unwrap(); + let id = h.state().views[i].id; + let at = h.state().views[i].page_screen_rect(0).expect("on screen").center(); + h.hover_at(at); + h.run_steps(1); + h.event(egui::Event::PointerButton { pos: at, button: egui::PointerButton::Primary, pressed: true, modifiers: Default::default() }); + h.event(egui::Event::PointerButton { pos: at, button: egui::PointerButton::Primary, pressed: false, modifiers: Default::default() }); + h.run_steps(2); + assert!(h.state().views[i].picked_image.is_some(), "picked with the Select tool"); + // Edit text & images: click the picture, delete it with the Delete key. + assert!(h.state_mut().execute("edit.edit_text")); + h.run_steps(2); + h.hover_at(at); + h.run_steps(1); + h.drag_at(at); + h.run_steps(1); + h.drop_at(at); + h.run_steps(2); + h.key_press(egui::Key::Delete); + h.run_steps(4); + assert!(h.state().session.get(id).unwrap().page_images(0).is_empty(), "the picture is gone"); + assert_eq!(h.state().views[i].picked_image, None, "and so is its highlight"); +} + #[test] fn right_click_on_a_picture_copies_it_at_full_size() { let (px, mut h) = picture_harness(); From 37a934662928a289c4f96506118f9d3253e92609 Mon Sep 17 00:00:00 2001 From: hade0ntema <296480886+hade0ntema@users.noreply.github.com> Date: Fri, 16 Oct 2026 11:49:06 +0200 Subject: [PATCH 10/12] fix(select): clear the picked picture in the edit itself, not on the next frame The first fix dropped a stale pick while painting the next frame. The window only redraws on the next input, so after deleting a picture in Edit text & images its highlight stayed on screen until the user clicked. Clear the pick in apply_edit, in the same step as the edit, and ask for a repaint. drop_stale stays for the paths that change the document without apply_edit (Undo, Redo). An edit-level test checks the pick right after apply_edit with no frame in between; it fails without the change. The earlier one-frame test could not tell, the test harness redraws more often than the app does. Co-Authored-By: Claude Sonnet 5.5 --- crates/ui-egui/src/editing.rs | 6 ++++++ crates/ui-egui/tests/viewer.rs | 22 +++++++++++++++++++++- 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/crates/ui-egui/src/editing.rs b/crates/ui-egui/src/editing.rs index be1347d68..6f7819949 100644 --- a/crates/ui-egui/src/editing.rs +++ b/crates/ui-egui/src/editing.rs @@ -52,6 +52,12 @@ impl PdfCraftApp { let Some(doc) = self.session.get(id) else { return true }; let info = &doc.info; let view = &mut self.views[i]; + // A picture picked with the Select tool may be the one this edit removed or moved: + // drop its highlight now, not on the next frame (which only comes with the next input). + view.picked_image = None; + if let Some(ctx) = &self.ctx { + ctx.request_repaint(); + } if !matches!(edit, Edit::AddAnnotation(_)) { // Only a new drawing waits on its raster; any other edit may remove or move it. view.comments.settling.clear(); diff --git a/crates/ui-egui/tests/viewer.rs b/crates/ui-egui/tests/viewer.rs index aadf6c56d..96defce32 100644 --- a/crates/ui-egui/tests/viewer.rs +++ b/crates/ui-egui/tests/viewer.rs @@ -1408,11 +1408,31 @@ fn a_picked_picture_that_is_deleted_in_edit_mode_loses_its_highlight() { h.drop_at(at); h.run_steps(2); h.key_press(egui::Key::Delete); - h.run_steps(4); + // One frame, no more input: the highlight goes at once, it does not wait for a click. + h.run_steps(1); assert!(h.state().session.get(id).unwrap().page_images(0).is_empty(), "the picture is gone"); assert_eq!(h.state().views[i].picked_image, None, "and so is its highlight"); } +/// The highlight goes in the very edit that removes the picture, not on a later frame: the window +/// only redraws on the next input, so waiting for one leaves the highlight on screen until a click. +#[test] +fn an_edit_clears_the_picked_picture_at_once() { + let (_, mut h) = picture_harness(); + let i = h.state().active.unwrap(); + let at = h.state().views[i].page_screen_rect(0).expect("on screen").center(); + h.hover_at(at); + h.run_steps(1); + h.event(egui::Event::PointerButton { pos: at, button: egui::PointerButton::Primary, pressed: true, modifiers: Default::default() }); + h.event(egui::Event::PointerButton { pos: at, button: egui::PointerButton::Primary, pressed: false, modifiers: Default::default() }); + h.run_steps(2); + assert!(h.state().views[i].picked_image.is_some(), "picked with the Select tool"); + let delete = pdfcraft_engine::Edit::EditPageImage { page: 0, index: 0, change: pdfcraft_engine::ImageEdit::Delete }; + assert!(h.state_mut().apply_edit(delete)); + // No frame has run since the edit. + assert_eq!(h.state().views[i].picked_image, None); +} + #[test] fn right_click_on_a_picture_copies_it_at_full_size() { let (px, mut h) = picture_harness(); From e1a1d4211ef41114618eebe8d4f33306c0f2a20f Mon Sep 17 00:00:00 2001 From: aiongg Date: Fri, 16 Oct 2026 14:03:20 +0000 Subject: [PATCH 11/12] Integration: #2638 a new drag over copied text shows the action bar again --- crates/ui-egui/src/canvas.rs | 3 +++ crates/ui-egui/tests/comments.rs | 19 +++++++++++++++++++ 2 files changed, 22 insertions(+) diff --git a/crates/ui-egui/src/canvas.rs b/crates/ui-egui/src/canvas.rs index 2faa05a3f..622cf2481 100644 --- a/crates/ui-egui/src/canvas.rs +++ b/crates/ui-egui/src/canvas.rs @@ -3218,6 +3218,9 @@ pub fn document_area(app: &mut PdfCraftApp, index: usize, ui: &mut egui::Ui) { } } if resp.drag_started() && press_here && !over_link { + // A drag replaces the selection without passing through none, so a + // re-drag over text whose bar Copy closed would keep the bar hidden. + view.actions_dismissed = None; view.selection_drag = true; let origin = ui.input(|inp| inp.pointer.press_origin()).unwrap_or(p); let (ox, oy) = xf.screen_to_view(origin); diff --git a/crates/ui-egui/tests/comments.rs b/crates/ui-egui/tests/comments.rs index aaea7990f..c127e2c99 100644 --- a/crates/ui-egui/tests/comments.rs +++ b/crates/ui-egui/tests/comments.rs @@ -379,6 +379,25 @@ fn selected_text_copy_action_closes_the_bar_and_keeps_the_selection() { h.get_by_label("Add comment"); } +#[test] +fn dragging_over_copied_text_again_brings_the_action_bar_back() { + let mut h = harness(|_| {}); + let (a, b) = { + let v = &h.state().views[0]; + (v.glyph_screen_pos(0, 4).expect("text layer"), v.glyph_screen_pos(0, 8).expect("glyph")) + }; + drag(&mut h, a, b); + let copied = h.state().views[0].selected_text(); + assert!(copied.is_some(), "the drag selects text"); + h.get_by_label("Copy").click(); + h.run_steps(3); + assert!(h.query_by_label("Add comment").is_none(), "the action bar closes after Copy"); + // The same drag again selects the same words, which is a new selection: the bar is back. + drag(&mut h, a, b); + assert_eq!(h.state().views[0].selected_text(), copied, "the drag selects the same words"); + assert!(h.query_by_label("Add comment").is_some(), "the action bar is back after a new drag"); +} + #[test] fn selected_text_replace_action_opens_replacement_composer() { let mut h = harness(|_| {}); From 2d5809b7a6afc0bdc540365922e19ed4bb723fcf Mon Sep 17 00:00:00 2001 From: aiongg Date: Fri, 16 Oct 2026 14:03:56 +0000 Subject: [PATCH 12/12] Integration: #2809 opening a document into Read mode puts the tool down --- crates/ui-egui/src/lib.rs | 5 +++++ crates/ui-egui/tests/ui.rs | 15 +++++++++++++++ 2 files changed, 20 insertions(+) diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index 3ce3f76df..bfe5179a6 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -2152,6 +2152,11 @@ impl PdfCraftApp { } else if matches!(self.quick_tool, QuickTool::EditText | QuickTool::AddText) { self.put_down_edit_text(); } + // Read mode hides the quick toolbar, so no other tool may stay active there. Opening a + // document with Read as the default mode comes here without `switch_mode`. + if mode == Mode::Read { + self.quick_tool = QuickTool::Select; + } } /// Back to the Select tool from an editing tool. A paragraph still open in the editor is diff --git a/crates/ui-egui/tests/ui.rs b/crates/ui-egui/tests/ui.rs index cc52b01a0..7fedd2bcc 100644 --- a/crates/ui-egui/tests/ui.rs +++ b/crates/ui-egui/tests/ui.rs @@ -264,6 +264,21 @@ fn read_mode_hides_the_quick_toolbar() { h.get_by_label("Select (V)"); } +/// Opening a document with Read as the default mode puts down the tool in use, as entering Read +/// mode from the mode bar does: the hidden toolbar would leave no way to see or change it. +#[test] +fn opening_into_read_mode_puts_the_tool_down() { + use pdfcraft_ui_egui::{Mode, QuickTool}; + let mut h = harness(|app| app.open_bytes("fixture.pdf", None, FIXTURE.to_vec()).expect("fixture opens")); + assert_eq!(h.state().mode, Mode::AllTools); + h.state_mut().set_option("quick", "hand").unwrap(); + h.state_mut().set_option("default-mode", "read").unwrap(); + h.state_mut().open_bytes("again.pdf", None, FIXTURE.to_vec()).unwrap(); + h.run_steps(3); + assert_eq!(h.state().mode, Mode::Read, "the new document opens in the default mode"); + assert_eq!(h.state().quick_tool, QuickTool::Select, "no tool stays active behind the hidden toolbar"); +} + /// A pane closed in Read mode stays closed on the way out: leaving Read mode doesn't bring back the /// pane saved on the way in, and closing Comments keeps its close preference. #[test]