From c341a5c929ec40bdd919747fcf2bc82397f2ad77 Mon Sep 17 00:00:00 2001 From: bflatastic Date: Wed, 7 Oct 2026 01:21:42 +0700 Subject: [PATCH] View: next page in two-page view moves a whole spread (#70) Next page went to the right-hand page of the spread already on screen, so the view didn't scroll and the current page snapped back to the left-hand one. Next and previous page (Ctrl/Cmd+arrows, the rail's buttons and the NextPage/PrevPage actions) now step a spread at a time in two-page view, with or without a cover page. Co-Authored-By: Claude Opus 5.5 --- crates/ui-egui/src/canvas.rs | 30 +++++++++++++++++++++++++----- crates/ui-egui/src/chrome.rs | 4 ++-- crates/ui-egui/tests/viewer.rs | 24 ++++++++++++++++++++++++ parity/acrobat-features.toml | 8 ++++---- 4 files changed, 55 insertions(+), 11 deletions(-) diff --git a/crates/ui-egui/src/canvas.rs b/crates/ui-egui/src/canvas.rs index ad76eb2a..d6631fc5 100644 --- a/crates/ui-egui/src/canvas.rs +++ b/crates/ui-egui/src/canvas.rs @@ -498,6 +498,27 @@ impl DocView { self.page_input = (page + 1).to_string(); } + /// Next page (`true`) or previous page. In two-page view this moves a whole spread: the other + /// page of the spread is already on screen, so stepping to it wouldn't move the view (#70). + pub fn step_page(&mut self, forward: bool) { + let c = self.current; + let target = match self.layout { + PageLayout::TwoUp => { + // Spreads are [0, 1], [2, 3], … or, with a cover page, [0], [1, 2], [3, 4], …. + let first = if self.cover { c.saturating_sub((c + 1) % 2) } else { c - c % 2 }; + match (forward, self.cover && c == 0) { + (true, true) => 1, + (true, false) => first.saturating_add(2), + (false, _) if self.cover && first <= 1 => 0, + (false, _) => first.saturating_sub(2), + } + } + PageLayout::Continuous | PageLayout::Single if forward => c + 1, + PageLayout::Continuous | PageLayout::Single => c.saturating_sub(1), + }; + self.go_to_page(target); + } + /// Go to the page typed in the page box: a page label (logical page numbers, as Acrobat /// does), else a page number. `false` when it names no page. pub fn go_to_typed(&mut self, typed: &str, labels: &[String]) -> bool { @@ -869,10 +890,10 @@ pub fn shortcuts(view: &mut DocView, ctx: &egui::Context) { } if view.layout == PageLayout::Single || ctx.input(|i| i.modifiers.command) { if key(Key::ArrowRight) || key(Key::PageDown) { - view.go_to_page(view.current + 1); + view.step_page(true); } if key(Key::ArrowLeft) || key(Key::PageUp) { - view.go_to_page(view.current.saturating_sub(1)); + view.step_page(false); } } } @@ -1678,7 +1699,6 @@ pub fn document_area(app: &mut PrintCraftApp, index: usize, ui: &mut egui::Ui) { fn run_button(app: &mut PrintCraftApp, index: usize, ctx: &egui::Context, name: &str, action: printcraft_engine::form_scripts::ButtonAction) { use printcraft_engine::form_scripts::ButtonAction as B; let pages = app.session.get(app.views[index].id).map_or(0, |d| d.info.pages.len()); - let current = app.views[index].current; match action { B::Reset { fields, exclude } => { let all: Vec = app.session.get(app.views[index].id).map(|d| d.form.iter().map(|f| f.name.clone()).collect()).unwrap_or_default(); @@ -1693,8 +1713,8 @@ fn run_button(app: &mut PrintCraftApp, index: usize, ctx: &egui::Context, name: } B::Named(n) => match n.as_str() { "Print" => app.open_print(), - "NextPage" => app.views[index].go_to_page((current + 1).min(pages.saturating_sub(1))), - "PrevPage" => app.views[index].go_to_page(current.saturating_sub(1)), + "NextPage" => app.views[index].step_page(true), + "PrevPage" => app.views[index].step_page(false), "FirstPage" => app.views[index].go_to_page(0), "LastPage" => app.views[index].go_to_page(pages.saturating_sub(1)), other => app.notify(format!("{name}: the {other} action isn't supported yet")), diff --git a/crates/ui-egui/src/chrome.rs b/crates/ui-egui/src/chrome.rs index ef7b5c4a..cb737600 100644 --- a/crates/ui-egui/src/chrome.rs +++ b/crates/ui-egui/src/chrome.rs @@ -320,10 +320,10 @@ pub fn right_rail(app: &mut PrintCraftApp, ui: &mut egui::Ui) { ui.label(egui::RichText::new(format!("{:.0}%", view.zoom * 100.0)).font(theme::regular(10.5)).color(t.text_faint)); ui.add_space(6.0); if icons::button(ui, "chevron-down", 30.0, false, "Next page").clicked() { - view.go_to_page(view.current + 1); + view.step_page(true); } if icons::button(ui, "chevron-up", 30.0, false, "Previous page").clicked() { - view.go_to_page(view.current.saturating_sub(1)); + view.step_page(false); } ui.label(egui::RichText::new(page_count.to_string()).font(theme::regular(11.0)).color(t.text_muted)); let edit = egui::TextEdit::singleline(&mut view.page_input) diff --git a/crates/ui-egui/tests/viewer.rs b/crates/ui-egui/tests/viewer.rs index 25cbfcc7..e93a763e 100644 --- a/crates/ui-egui/tests/viewer.rs +++ b/crates/ui-egui/tests/viewer.rs @@ -172,6 +172,30 @@ fn layouts_fit_height_labels_and_system_theme() { assert!(h.state().follow_system_theme); } +#[test] +fn two_page_view_steps_a_spread_at_a_time() { + // #70: ⌘→ went to the right-hand page of the same spread, so the view never moved. + use egui::{Key, Modifiers}; + use printcraft_ui_egui::canvas::{Fit, PageLayout}; + let mut h = harness(); + h.state_mut().active = Some(0); + h.state_mut().views[0].fit = Fit::Width; + h.state_mut().views[0].layout = PageLayout::TwoUp; + h.run_steps(4); + let step = |h: &mut Harness<'static, PrintCraftApp>, key| { + h.key_press_modifiers(Modifiers::COMMAND, key); + h.run_steps(4); + h.state().views[0].current + }; + let pages: Vec = [Key::ArrowRight, Key::ArrowRight, Key::ArrowRight, Key::ArrowLeft, Key::ArrowLeft].map(|k| step(&mut h, k)).into(); + assert_eq!(pages, [2, 4, 4, 2, 0], "spreads 1–2, 3–4, 5"); + // With a cover page the spreads are 1, 2–3, 4–5. + h.state_mut().views[0].cover = true; + h.run_steps(4); + let pages: Vec = [Key::ArrowRight, Key::ArrowRight, Key::ArrowLeft, Key::ArrowLeft].map(|k| step(&mut h, k)).into(); + assert_eq!(pages, [1, 3, 1, 0]); +} + #[test] fn damaged_files_say_they_were_repaired() { // No cross-reference table: the file is reconstructed. diff --git a/parity/acrobat-features.toml b/parity/acrobat-features.toml index 6b5c936f..07ba3095 100644 --- a/parity/acrobat-features.toml +++ b/parity/acrobat-features.toml @@ -842,8 +842,8 @@ milestone = "M3" status = "shipped" commands = [] tools = [] -evidence = ["crates/ui-egui/tests/view.rs::two_up_layout_places_pages_side_by_side"] -notes = "" +evidence = ["crates/ui-egui/tests/view.rs::two_up_layout_places_pages_side_by_side", "crates/ui-egui/tests/viewer.rs::two_page_view_steps_a_spread_at_a_time"] +notes = "next/previous page step a spread at a time" [[feature]] id = "view.layout-two-up-scrolling" @@ -1106,8 +1106,8 @@ milestone = "M3" status = "shipped" commands = [] tools = [] -evidence = ["crates/ui-egui/tests/control.rs::state_and_view_options", "crates/ui-egui/tests/ui.rs::theme_and_view_options_apply"] -notes = "" +evidence = ["crates/ui-egui/tests/control.rs::state_and_view_options", "crates/ui-egui/tests/ui.rs::theme_and_view_options_apply", "crates/ui-egui/tests/viewer.rs::two_page_view_steps_a_spread_at_a_time"] +notes = "in two-page view, next and previous page move a whole spread (with or without a cover page)" [[feature]] id = "view.goto-page-label"