From d57f7400037eb1345fef2dfa6efd9fec6d7e7369 Mon Sep 17 00:00:00 2001 From: Omar Elders <118695915+omarelders@users.noreply.github.com> Date: Mon, 12 Oct 2026 12:52:36 +0300 Subject: [PATCH] Refine search match selection and text failure jitter handling (#1291) * Fix search match selection order and text reload failure recovery 1. rerun_find: Accumulate and sort matches across all loaded pages and pick the first match on or after the currently viewed page before flashing once, avoiding arbitrary viewport jumps to page 0 when viewing subsequent pages. 2. receive_result: Gracefully fall back to the next match at or after the viewed page without flashing when page text extraction fails, avoiding latent None state that caused viewport jitter on subsequent page reloads. 3. Add tests verifying match selection on viewed page, continuous multi-page drawing with active search, and text failure recovery. * rustfmt Co-Authored-By: Claude Opus 5.5 * Clippy: RenderRequest is Copy Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Brandon Thomas Co-authored-by: Claude Opus 5.5 --- crates/ui-egui/src/canvas.rs | 58 ++++++++++++++++++++++++++++++++-- crates/ui-egui/tests/viewer.rs | 32 +++++++++++++++++++ 2 files changed, 87 insertions(+), 3 deletions(-) diff --git a/crates/ui-egui/src/canvas.rs b/crates/ui-egui/src/canvas.rs index fe8a915a..6fe4a9bf 100644 --- a/crates/ui-egui/src/canvas.rs +++ b/crates/ui-egui/src/canvas.rs @@ -898,9 +898,24 @@ impl DocView { f.case_query = f.query.clone(); f.matches.clear(); f.current = None; - let pages: Vec = self.texts.keys().copied().collect(); - for p in pages { - self.refresh_find_page(p); + if f.case_query.trim().is_empty() { + return; + } + let mut pages: Vec = self.texts.keys().copied().collect(); + pages.sort(); + for &p in &pages { + if let Some(text) = self.texts.get(&p) { + f.matches.extend(text.find_opts(&f.case_query, f.case_sensitive, f.whole_words).into_iter().map(|r| (p, r))); + } + } + f.matches.sort_by_key(|(p, r)| (*p, r.start)); + if !f.matches.is_empty() { + // First match at or after the page being viewed. + let cur = self.current; + let i = f.matches.iter().position(|(p, _)| *p >= cur).unwrap_or(0); + f.current = Some(i); + let (p, _) = f.matches[i]; + self.flash_match(p); } } @@ -1548,6 +1563,10 @@ impl DocView { let current_key = f.current.and_then(|c| f.matches.get(c).cloned()); f.matches.retain(|(p, _)| *p != r.request.page); f.current = current_key.and_then(|k| f.matches.iter().position(|m| *m == k)); + if f.current.is_none() && !f.matches.is_empty() { + let cur = self.current; + f.current = Some(f.matches.iter().position(|(p, _)| *p >= cur).unwrap_or(0)); + } } } } @@ -5184,4 +5203,37 @@ trailer << /Root 1 0 R >> v.go_to_dest(1, DestView::FitH { top: Some(100.0) }, &empty); assert_eq!((v.current, v.goto_point), (1, None)); } + + #[test] + fn page_text_failure_preserves_valid_search_match_without_jitter() { + let mut v = view(3, PageLayout::Single, true); + v.current = 1; + v.find = Some(Find { + query: "test".into(), + case_query: "test".into(), + matches: vec![(0, 0..4), (1, 5..9)], + current: Some(0), + ..Default::default() + }); + let req = RenderRequest { page: 0, kind: RequestKind::Text, ..Default::default() }; + v.last_queue = vec![req]; + let ctx = egui::Context::default(); + v.receive_result( + &ctx, + pdfcraft_render::RenderedPage { + request: req, + width: 0, + height: 0, + rgba: Default::default(), + error: Some("extraction failed".into()), + text: None, + warnings: Vec::new(), + millis: 0, + }, + ); + // Active match was on page 0 which failed: current should fall back to match on page 1 without jitter + assert_eq!(v.find.as_ref().unwrap().current, Some(0)); + assert_eq!(v.find.as_ref().unwrap().matches, vec![(1, 5..9)]); + assert!(v.goto.is_none()); + } } diff --git a/crates/ui-egui/tests/viewer.rs b/crates/ui-egui/tests/viewer.rs index 4f0c6dbd..a8f15793 100644 --- a/crates/ui-egui/tests/viewer.rs +++ b/crates/ui-egui/tests/viewer.rs @@ -220,6 +220,38 @@ fn marking_while_searching_does_not_jitter_or_reset_search_state() { } assert_eq!(h.state().views[0].find.as_ref().unwrap().current, current_before); assert_eq!(h.state().views[0].find.as_ref().unwrap().matches.len(), 15); + + // If user is viewing page 3 and reruns find, the selected match must be on page 3 (index 9), not page 0 + h.state_mut().views[0].current = 3; + h.state_mut().views[0].rerun_find(); + let current_p3 = h.state().views[0].find.as_ref().unwrap().current; + assert_eq!(current_p3, Some(9), "rerun_find must select the first match at or after the viewed page (page 3), but selected {:?}", current_p3); + + // Continuous drawing across multiple pages must never trigger viewport jumps or wipe search state + h.run_steps(2); + h.state_mut().views[0].goto = None; + for p in 1..=4 { + let shape = pdfcraft_engine::Shape::Ink { strokes: vec![vec![[15.0 * p as f64, 20.0], [30.0 * p as f64, 40.0]]] }; + let stroke = pdfcraft_engine::Edit::AddAnnotation(pdfcraft_engine::NewAnnotation { + page: p, + style: pdfcraft_engine::Style::default_for(&shape), + shape, + contents: String::new(), + author: "tester".into(), + }); + h.state_mut().apply_edit(stroke); + h.run_steps(2); + assert!(h.state().views[0].goto.is_none(), "drawing on page {p} must not set goto"); + assert_eq!(h.state().views[0].find.as_ref().unwrap().current, Some(9)); + } + let multi_deadline = std::time::Instant::now() + std::time::Duration::from_secs(5); + while std::time::Instant::now() < multi_deadline { + h.run_steps(2); + std::thread::sleep(std::time::Duration::from_millis(5)); + assert!(h.state().views[0].goto.is_none(), "multi-page reload after annotations must not set goto"); + } + assert_eq!(h.state().views[0].find.as_ref().unwrap().current, Some(9)); + assert_eq!(h.state().views[0].find.as_ref().unwrap().matches.len(), 15); } #[test]