From d2a59121fd95f07e8dcf51b0118d9d8688ae014b Mon Sep 17 00:00:00 2001 From: wstock Date: Thu, 8 Oct 2026 13:31:38 +0100 Subject: [PATCH] Issue #188: page steps in two-page view take every spread when several fit on screen (#265) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The current page is the page with the largest visible height. When more than one row of pages is wholly on screen (a book with a portrait cover and landscape spreads, at Fit Width or Fit Page), those heights differ only by sub-point rounding after the scroll, so the "current" page was whichever wholly visible page rounding favoured. ⌘Page Down then stepped from the wrong spread: from the cover it went to 2–3 and then to 6–7, skipping 4–5 (or, the other way round, stayed put after the cover). Visible heights within half a point are now equal (the topmost wins), and the page navigated to stays current while no page shows more of itself, so the next step starts from it. Test: viewer.rs::page_down_steps_every_spread_when_several_fit_on_screen (a 12-page book with a cover page at Fit Width, Fit Page and Fit Height: ⌘Page Down visits 1, 3, 5, 7, 9, 11 and ⌘Page Up 9, 7, 5, 3, 1, 0). Before the fix it went 1, 5, 7, 11, 11, 11 at Fit Width. Cited on the view.cover-page parity entry. Co-authored-by: wstock <1394687+wstock@users.noreply.github.com> Co-authored-by: Claude Opus 5.5 Co-authored-by: Brandon Thomas --- crates/ui-egui/src/canvas.rs | 16 ++++++++- crates/ui-egui/tests/viewer.rs | 65 ++++++++++++++++++++++++++++++++++ parity/acrobat-features.toml | 4 +-- 3 files changed, 82 insertions(+), 3 deletions(-) diff --git a/crates/ui-egui/src/canvas.rs b/crates/ui-egui/src/canvas.rs index bd175e64..2b38e76b 100644 --- a/crates/ui-egui/src/canvas.rs +++ b/crates/ui-egui/src/canvas.rs @@ -1303,8 +1303,12 @@ pub fn document_area(app: &mut PdfCraftApp, index: usize, ui: &mut egui::Ui) { let mut visible_now = Vec::new(); view.screen_rects.clear(); view.screen_xforms.clear(); + // Visible heights (points) closer than this are equal: one page size can meet the + // viewport a fraction of a point differently once the scroll offset is applied. + const TIE: f32 = 0.5; let mut current = view.current; let mut best_overlap = -1.0f32; + let mut current_overlap = -1.0f32; let pointer = ui.input(|i| i.pointer.hover_pos()); for &i in &visible_pages { let r = rects[i].translate(origin.to_vec2()); @@ -1318,7 +1322,12 @@ pub fn document_area(app: &mut PdfCraftApp, index: usize, ui: &mut egui::Ui) { .push((i, PageXform { rect: r, rot: view.rotation, pw: info.pages[i].width.max(1.0), ph: info.pages[i].height.max(1.0) })); } let overlap = r.intersect(visible).height(); - if overlap > best_overlap { + if i == view.current { + current_overlap = overlap; + } + // Pages shown equally (two rows wholly on screen) differ only by rounding: the + // topmost one counts. + if overlap > best_overlap + TIE { best_overlap = overlap; current = i; } @@ -1678,6 +1687,11 @@ pub fn document_area(app: &mut PdfCraftApp, index: usize, ui: &mut egui::Ui) { } } } + // The page navigated to stays current while no page shows more of itself, so the next + // page step starts from it rather than from a page further down the screen (#188). + if current_overlap >= best_overlap - TIE { + current = view.current; + } if view.layout != PageLayout::Single { view.current = current; if !ui.memory(|m| m.has_focus(egui::Id::new("page-input"))) { diff --git a/crates/ui-egui/tests/viewer.rs b/crates/ui-egui/tests/viewer.rs index ca809010..6a143c23 100644 --- a/crates/ui-egui/tests/viewer.rs +++ b/crates/ui-egui/tests/viewer.rs @@ -471,3 +471,68 @@ fn arrow_and_page_keys_move_through_a_scrolling_document() { h.run_steps(4); assert_eq!(press(&mut h, Key::PageDown, 1), 3); } + +/// Like [`fixture`], with each page's size given. +fn sized_fixture(sizes: &[(u32, u32)]) -> Vec { + let n = sizes.len(); + let mut objs: Vec = vec!["<< /Type /Catalog /Pages 2 0 R >>".into()]; + let kids: Vec = (0..n).map(|i| format!("{} 0 R", 4 + 2 * i)).collect(); + objs.push(format!("<< /Type /Pages /Kids [{}] /Count {n} >>", kids.join(" "))); + objs.push("<< /Type /Font /Subtype /Type1 /BaseFont /Helvetica >>".into()); + for (i, (w, h)) in sizes.iter().enumerate() { + objs.push(format!( + "<< /Type /Page /Parent 2 0 R /MediaBox [0 0 {w} {h}] /Contents {} 0 R /Resources << /Font << /F1 3 0 R >> >> >>", + 5 + 2 * i + )); + let body = format!("BT /F1 18 Tf 20 150 Td (Page {}) Tj ET", i + 1); + objs.push(format!("<< /Length {} >>\nstream\n{body}\nendstream", body.len())); + } + let mut out = b"%PDF-1.7\n".to_vec(); + let mut offs = Vec::new(); + for (i, o) in objs.iter().enumerate() { + offs.push(out.len()); + out.extend_from_slice(format!("{} 0 obj\n{o}\nendobj\n", i + 1).as_bytes()); + } + let x = out.len(); + out.extend_from_slice(format!("xref\n0 {}\n0000000000 65535 f \n", objs.len() + 1).as_bytes()); + for o in offs { + out.extend_from_slice(format!("{o:010} 00000 n \n").as_bytes()); + } + out.extend_from_slice(format!("trailer\n<< /Size {} /Root 1 0 R >>\nstartxref\n{x}\n%%EOF\n", objs.len() + 1).as_bytes()); + out +} + +#[test] +fn page_down_steps_every_spread_when_several_fit_on_screen() { + // #188: in two-page view with a cover page, ⌘Page Down skipped spreads (or stuck) once more + // than one spread fit on screen: the current page was whichever wholly visible page rounding + // made "most visible", so the next step started from the wrong spread. + use egui::{Key, Modifiers}; + use pdfcraft_ui_egui::canvas::{Fit, PageLayout}; + // a printed book: portrait cover and back, landscape two-page spreads between + let mut sizes = vec![(200, 300)]; + sizes.extend([(400, 300); 10]); + sizes.push((200, 300)); + for fit in [Fit::Width, Fit::Page, Fit::Height] { + let bytes = sized_fixture(&sizes); + let mut h = Harness::builder().with_size(egui::vec2(1400.0, 900.0)).build_eframe(move |_cc| { + let mut app = PdfCraftApp::new(); + app.open_bytes("book.pdf", None, bytes).unwrap(); + app + }); + h.run_steps(6); + h.state_mut().active = Some(0); + let v = &mut h.state_mut().views[0]; + (v.fit, v.layout, v.cover) = (fit, PageLayout::TwoUp, true); + h.run_steps(6); + let mut step = |key| { + h.key_press_modifiers(Modifiers::COMMAND, key); + h.run_steps(4); + h.state().views[0].current + }; + let down: Vec = (0..6).map(|_| step(Key::PageDown)).collect(); + assert_eq!(down, [1, 3, 5, 7, 9, 11], "{fit:?}: cover, five spreads, back cover"); + let up: Vec = (0..6).map(|_| step(Key::PageUp)).collect(); + assert_eq!(up, [9, 7, 5, 3, 1, 0], "{fit:?}"); + } +} diff --git a/parity/acrobat-features.toml b/parity/acrobat-features.toml index c0eb3259..2fb2a3d7 100644 --- a/parity/acrobat-features.toml +++ b/parity/acrobat-features.toml @@ -866,8 +866,8 @@ milestone = "M3" status = "shipped" commands = ["view.layout.cover"] tools = [] -evidence = ["crates/ui-egui/tests/viewer.rs::layouts_fit_height_labels_and_system_theme", "crates/ui-egui/tests/view.rs::page_display_commands_switch_layouts"] -notes = "View ▸ Page display, the rail's Page display button or the palette; disabled outside two-page view" +evidence = ["crates/ui-egui/tests/viewer.rs::layouts_fit_height_labels_and_system_theme", "crates/ui-egui/tests/view.rs::page_display_commands_switch_layouts", "crates/ui-egui/tests/viewer.rs::page_down_steps_every_spread_when_several_fit_on_screen"] +notes = "View ▸ Page display, the rail's Page display button or the palette; disabled outside two-page view; next / previous page step one spread at a time even when several spreads fit on screen" [[feature]] id = "view.page-gaps"