Skip to content

Commit 3efdb19

Browse files
committed
M3: look up named destinations off the UI thread, and open at the latest place asked for (storytold#3425)
A named-destination lookup reads the page tree and the whole name tree: about 100 ms for a 3,000-page manual with 30,000 names. In the running app it now runs on its own thread, and the tab moves when the frame loop picks up the answer. Startup and inline test runs still resolve it in place, as opening does. A newer destination for the same document drops a lookup still in flight. A second launch that asks for another place while the file is still opening now replaces the pending open's destination, instead of being dropped. Raised by CodeRabbit in review of storytold#5049.
1 parent 2706dad commit 3efdb19

7 files changed

Lines changed: 92 additions & 13 deletions

File tree

‎ROADMAP.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -200,7 +200,7 @@ Ranked; detail and the full milestone table in [docs/roadmap.md](docs/roadmap.md
200200

201201
## Progress log
202202

203-
- **2026-10-27 (M3, #3425):** A file named with open parameters, as help systems start a PDF reader (`manual.pdf#nameddest=Intro`, `#page=12` or `#Intro`), opens at that destination instead of failing to read `manual.pdf#…`: at launch, when another launch hands it over, and in its tab when it is already open. Other parameters are ignored. The app now also applies a GoToR link's named destination, looked up in the target's /Names /Dests or catalog /Dests, instead of warning that it wasn't applied. Estimates unchanged.
203+
- **2026-10-27 (M3, #3425):** A file named with open parameters, as help systems start a PDF reader (`manual.pdf#nameddest=Intro`, `#page=12` or `#Intro`), opens at that destination instead of failing to read `manual.pdf#…`: at launch, when another launch hands it over, and in its tab when it is already open. Other parameters are ignored. The app and `link_open` now also apply a GoToR link's named destination, looked up in the target's /Names /Dests or catalog /Dests, instead of warning that it wasn't applied; the app looks names up off the UI thread. `doc_open` reports the destination of a path with open parameters. Estimates unchanged.
204204
- **2026-10-27 (M10, OCR models):** Model staging downloads and verifies the ONNX pair after the upstream RTEN URL brownout; runtime discovery prefers complete ONNX pairs per directory, retains complete RTEN fallback and preserves directory precedence.
205205
- **2026-10-24 (M10, #4158 partial):** Word, HTML, RTF and PowerPoint export keep a block that continues another on its line (an italic case name, a link, a superscript) as a run in its own style inside that paragraph, with the source's word space, instead of a paragraph of its own. Style changes that wrap onto the next line still split paragraphs (the reading blocks keep style boundaries). Estimates unchanged.
206206
- **2026-10-24 (M3, window move on Wayland, #3970 follow-up):** After a move or resize by PdfCraft's own title bar or edges, the button release given back to egui goes before the pointer leaving the window, so hover no longer sticks at the old place until the pointer moves. Found by CodeRabbit in review of #3970; a unit test covers it. Estimates unchanged.

‎crates/ui-egui/src/app.rs‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,8 @@ impl PdfCraftApp {
100100
#[cfg(not(target_arch = "wasm32"))]
101101
pending_opens: Vec::new(),
102102
#[cfg(not(target_arch = "wasm32"))]
103+
pending_destinations: Vec::new(),
104+
#[cfg(not(target_arch = "wasm32"))]
103105
next_open_token: 0,
104106
#[cfg(all(test, not(target_arch = "wasm32")))]
105107
open_prepare_gate: None,
@@ -353,6 +355,7 @@ impl eframe::App for PdfCraftApp {
353355
worker.set_context(ctx);
354356
}
355357
self.poll_pending_opens();
358+
self.poll_pending_destinations();
356359
}
357360
// Chinese wants its own faces before the Japanese ones (one baseline per line).
358361
let cjk = theme::CjkPreference::for_language(lang.code());

‎crates/ui-egui/src/app/files.rs‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -553,7 +553,11 @@ impl PdfCraftApp {
553553
}
554554
return OpenOutcome::Focused;
555555
}
556-
if self.pending_opens.iter().any(|p| p.path.as_deref().is_some_and(|p| same_file(p, path))) {
556+
if let Some(pending) = self.pending_opens.iter_mut().find(|p| p.path.as_deref().is_some_and(|p| same_file(p, path))) {
557+
// Asked for another place before it finished opening: it opens at the latest one.
558+
if after_open.is_some() {
559+
pending.after_open = after_open;
560+
}
557561
return OpenOutcome::Pending;
558562
}
559563
let name = std::path::Path::new(path).file_name().map(|n| n.to_string_lossy().into_owned()).unwrap_or_else(|| path.to_string());

‎crates/ui-egui/src/app/links.rs‎

Lines changed: 54 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -72,19 +72,58 @@ impl PdfCraftApp {
7272
}
7373

7474
/// Show `destination` in the tab at `index`: a document just opened from a link or with open
75-
/// parameters (#3425), or one already open that such a link or launch names.
75+
/// parameters (#3425), or one already open that such a link or launch names. The latest request
76+
/// for a document wins over a named destination still being looked up.
7677
pub(super) fn apply_destination_at(&mut self, index: usize, destination: pdfcraft_render::RemoteDestination) {
77-
let Some(view) = self.views.get_mut(index) else { return };
78-
let Some(doc) = self.session.get(view.id) else { return };
78+
let Some(id) = self.views.get(index).map(|view| view.id) else { return };
79+
#[cfg(not(target_arch = "wasm32"))]
80+
self.pending_destinations.retain(|pending| pending.doc != id);
7981
let destination = match destination {
8082
pdfcraft_render::RemoteDestination::Named(name) => {
81-
match pdfcraft_render::named_destination(doc.bytes.clone(), doc.password.as_deref(), &name) {
82-
Some((page, dest)) => pdfcraft_render::RemoteDestination::Page { page, view: dest },
83-
None => pdfcraft_render::RemoteDestination::Named(name),
83+
let Some(doc) = self.session.get(id) else { return };
84+
let (bytes, password) = (doc.bytes.clone(), doc.password.clone());
85+
// Live, the name tree is read off the UI thread; startup and tests stay inline.
86+
#[cfg(not(target_arch = "wasm32"))]
87+
if let Some(ctx) = self.ctx.clone().filter(|_| !self.run_inline) {
88+
let (sender, receiver) = std::sync::mpsc::channel();
89+
let (work_bytes, work_password, work_name) = (bytes.clone(), password.clone(), name.clone());
90+
let work = move || {
91+
let _ = sender.send(pdfcraft_render::named_destination(work_bytes, work_password.as_deref(), &work_name));
92+
ctx.request_repaint();
93+
};
94+
if std::thread::Builder::new().name("pdfcraft-named-destination".into()).spawn(work).is_ok() {
95+
self.pending_destinations.push(PendingDestination { doc: id, name, receiver });
96+
return;
97+
}
8498
}
99+
found_destination(pdfcraft_render::named_destination(bytes, password.as_deref(), &name), name)
85100
}
86101
other => other,
87102
};
103+
self.show_destination_at(index, destination);
104+
}
105+
106+
/// Show each named destination whose lookup has finished, in its document's tab.
107+
#[cfg(not(target_arch = "wasm32"))]
108+
pub(crate) fn poll_pending_destinations(&mut self) {
109+
for pending in std::mem::take(&mut self.pending_destinations) {
110+
let found = match pending.receiver.try_recv() {
111+
Ok(found) => found,
112+
Err(std::sync::mpsc::TryRecvError::Empty) => {
113+
self.pending_destinations.push(pending);
114+
continue;
115+
}
116+
Err(std::sync::mpsc::TryRecvError::Disconnected) => None,
117+
};
118+
if let Some(index) = self.views.iter().position(|view| view.id == pending.doc) {
119+
self.show_destination_at(index, found_destination(found, pending.name));
120+
}
121+
}
122+
}
123+
124+
fn show_destination_at(&mut self, index: usize, destination: pdfcraft_render::RemoteDestination) {
125+
let Some(view) = self.views.get_mut(index) else { return };
126+
let Some(doc) = self.session.get(view.id) else { return };
88127
let warning = match destination {
89128
pdfcraft_render::RemoteDestination::Default => None,
90129
pdfcraft_render::RemoteDestination::Named(_) => Some("The linked PDF opened, but its named destination was not applied."),
@@ -140,3 +179,12 @@ impl PdfCraftApp {
140179
}
141180
}
142181
}
182+
183+
/// A named destination's lookup result as a destination to show: its page, or the name again when
184+
/// the document doesn't define it, which shows a warning.
185+
fn found_destination(found: Option<(usize, pdfcraft_render::DestView)>, name: String) -> pdfcraft_render::RemoteDestination {
186+
match found {
187+
Some((page, view)) => pdfcraft_render::RemoteDestination::Page { page, view },
188+
None => pdfcraft_render::RemoteDestination::Named(name),
189+
}
190+
}

‎crates/ui-egui/src/lib.rs‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -229,6 +229,15 @@ struct PendingOpen {
229229
then: Option<&'static str>,
230230
}
231231

232+
/// A named destination being looked up off the UI thread: reading a large document's name tree
233+
/// takes a noticeable moment.
234+
#[cfg(not(target_arch = "wasm32"))]
235+
struct PendingDestination {
236+
doc: DocId,
237+
name: String,
238+
receiver: std::sync::mpsc::Receiver<Option<(usize, pdfcraft_render::DestView)>>,
239+
}
240+
232241
pub struct PdfCraftApp {
233242
/// Tools side and pane widths; which panes are open is `left_open` and `right`.
234243
pub side_panels: side_panels::SidePanels,
@@ -353,6 +362,8 @@ pub struct PdfCraftApp {
353362
#[cfg(not(target_arch = "wasm32"))]
354363
pending_opens: Vec<PendingOpen>,
355364
#[cfg(not(target_arch = "wasm32"))]
365+
pending_destinations: Vec<PendingDestination>,
366+
#[cfg(not(target_arch = "wasm32"))]
356367
next_open_token: u64,
357368
#[cfg(all(test, not(target_arch = "wasm32")))]
358369
open_prepare_gate: Option<std::sync::Arc<open_worker::PrepareGate>>,

‎crates/ui-egui/tests/it/document_links.rs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -213,14 +213,14 @@ fn a_gotor_link_to_a_named_destination_opens_at_its_page() {
213213
for (destination, page) in [("/there", 2), ("(here)", 1)] {
214214
let mut h = remote_harness(Some(dir.join("source.pdf").to_string_lossy().into_owned()), "target.pdf", destination);
215215
click_remote_link(&mut h);
216-
support::wait_until(&mut h, "remote document", |app| app.views.len() == 2);
217-
assert_eq!(h.state().views[1].current, page, "{destination}");
216+
// The name is looked up off the UI thread, so the tab may open before it moves there.
217+
support::wait_until(&mut h, "remote document at its destination", |app| app.views.len() == 2 && app.views[1].current == page);
218218
assert_eq!(h.state().toast, None, "{destination}: applied, so nothing to warn about");
219219
}
220220
// A name the target doesn't define opens it at its first page, with the warning.
221221
let mut h = remote_harness(Some(dir.join("source.pdf").to_string_lossy().into_owned()), "target.pdf", "/elsewhere");
222222
click_remote_link(&mut h);
223-
support::wait_until(&mut h, "remote document", |app| app.views.len() == 2);
223+
support::wait_until(&mut h, "remote document and the warning", |app| app.views.len() == 2 && app.toast.is_some());
224224
assert_eq!(h.state().views[1].current, 0);
225225
assert!(h.state().toast.as_ref().is_some_and(|(text, _)| text.contains("named destination was not applied")), "{:?}", h.state().toast);
226226
}

‎crates/ui-egui/tests/it/open_action.rs‎

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -243,10 +243,23 @@ fn open_parameters_handed_to_the_running_app_show_their_destination() {
243243
app
244244
});
245245
h.run_steps(4);
246+
// Asked twice before it has opened, it opens at the place asked for last.
247+
assert_eq!(h.state_mut().open_path(&format!("{path}#page=5")), OpenOutcome::Pending);
246248
assert_eq!(h.state_mut().open_path(&format!("{path}#nameddest=Load Flow")), OpenOutcome::Pending);
247-
crate::support::wait_until(&mut h, "the manual", |app| app.views.len() == 1);
248-
assert_eq!(h.state().views[0].current, 3);
249+
crate::support::wait_until(&mut h, "the manual at Load Flow", |app| app.views.len() == 1 && app.views[0].current == 3);
249250
// Asked again for a place in the open file, its tab goes there instead of opening it again.
250251
assert_eq!(h.state_mut().open_path(&format!("{path}#page=2")), OpenOutcome::Focused);
251252
assert_eq!((h.state().views.len(), h.state().views[0].current), (1, 1));
253+
// A name is looked up off the UI thread: the tab moves once the lookup is in.
254+
assert_eq!(h.state_mut().open_path(&format!("{path}#nameddest=Load Flow")), OpenOutcome::Focused);
255+
assert_eq!(h.state().views[0].current, 1, "not yet");
256+
crate::support::wait_until(&mut h, "Load Flow", |app| app.views[0].current == 3);
257+
// A page asked for while a name is still being looked up wins: the lookup's answer is dropped.
258+
assert_eq!(h.state_mut().open_path(&format!("{path}#nameddest=intro")), OpenOutcome::Focused);
259+
assert_eq!(h.state_mut().open_path(&format!("{path}#page=5")), OpenOutcome::Focused);
260+
for _ in 0..25 {
261+
h.run_steps(1);
262+
std::thread::sleep(std::time::Duration::from_millis(20));
263+
}
264+
assert_eq!(h.state().views[0].current, 4);
252265
}

0 commit comments

Comments
 (0)