diff --git a/Cargo.lock b/Cargo.lock index 0b4ab6f4ab..04e8753f72 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3772,6 +3772,7 @@ dependencies = [ "pdfcraft-model", "pdfcraft-render", "png 0.18.1", + "pollster 1.0.1", "rfd", "serde", "serde_json", diff --git a/crates/ui-egui/Cargo.toml b/crates/ui-egui/Cargo.toml index 38c23962e2..35fd3f5c1e 100644 --- a/crates/ui-egui/Cargo.toml +++ b/crates/ui-egui/Cargo.toml @@ -28,6 +28,8 @@ base64 = "0.22" getrandom = "0.4" # Create ▸ Clipboard (images; already in the tree through egui-winit). arboard = { version = "3.6", default-features = false, features = ["image-data"] } +# Waits for native file pickers off the UI thread (already in the tree through wgpu). +pollster = "1.0" [target.'cfg(target_arch = "wasm32")'.dependencies] wasm-bindgen-futures = "0.4" diff --git a/crates/ui-egui/src/files.rs b/crates/ui-egui/src/files.rs index c211019e4b..bb81ea6862 100644 --- a/crates/ui-egui/src/files.rs +++ b/crates/ui-egui/src/files.rs @@ -1,7 +1,7 @@ //! Multi-document page operations: Combine Files, Insert Pages from File, Extract Pages, Split. //! -//! Desktop builds pick files synchronously. Browsers pick them asynchronously: the chosen -//! files land in `requests` with their purpose and are handled on the next frame. +//! Files are picked asynchronously and used on a later frame: desktop builds through `pickers` +//! (a blocking picker crashes the app on macOS), browsers through `requests`. use std::sync::Arc; @@ -117,24 +117,7 @@ impl PdfCraftApp { fn pick_files(&mut self, purpose: FilePurpose, multiple: bool) { #[cfg(not(target_arch = "wasm32"))] - { - let dialog = rfd::FileDialog::new().add_filter("PDF", &["pdf"]); - let paths = if multiple { dialog.pick_files().unwrap_or_default() } else { dialog.pick_file().into_iter().collect() }; - let mut files = Vec::new(); - for p in paths { - let name = p.file_name().map(|n| n.to_string_lossy().into_owned()).unwrap_or_else(|| "file.pdf".into()); - match std::fs::read(&p) { - Ok(b) => files.push((name, b)), - Err(e) => { - self.notify(format!("Couldn't read {name}: {e}")); - return; - } - } - } - if !files.is_empty() { - self.use_files(purpose, files); - } - } + self.pick(crate::pickers::PickFor::Files(purpose), rfd::AsyncFileDialog::new().add_filter("PDF", &["pdf"]), multiple); #[cfg(target_arch = "wasm32")] { let requests = self.requests.clone(); @@ -154,6 +137,25 @@ impl PdfCraftApp { } } + /// Read the picked files and use them. + #[cfg(not(target_arch = "wasm32"))] + pub(crate) fn use_paths(&mut self, purpose: FilePurpose, paths: &[std::path::PathBuf]) { + let mut files = Vec::new(); + for p in paths { + let name = p.file_name().map(|n| n.to_string_lossy().into_owned()).unwrap_or_else(|| "file.pdf".into()); + match std::fs::read(p) { + Ok(b) => files.push((name, b)), + Err(e) => { + self.notify(format!("Couldn't read {name}: {e}")); + return; + } + } + } + if !files.is_empty() { + self.use_files(purpose, files); + } + } + /// Handle files picked asynchronously. pub(crate) fn process_file_requests(&mut self) { let pending: Vec<_> = self.requests.lock().map(|mut q| std::mem::take(&mut *q)).unwrap_or_default(); diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index d3877d624c..0309a54f45 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -53,6 +53,8 @@ pub mod icons; mod pageboxes; mod palette; mod panels; +#[cfg(not(target_arch = "wasm32"))] +mod pickers; pub mod prepare; mod print_ui; pub use print_ui::{Handling as PrintHandling, PrintDraft, Which as PrintWhich}; @@ -345,6 +347,12 @@ pub struct PdfCraftApp { pub view_draft: Option<(DocId, pdfcraft_engine::InitialView)>, /// Files picked asynchronously for combine / insert (web). pub requests: files::Requests, + /// Native file pickers in flight (they never block the frame; see `pickers`). + #[cfg(not(target_arch = "wasm32"))] + pickers: pickers::Pickers, + /// Pick these files instead of showing a picker (tests and automation). + #[cfg(not(target_arch = "wasm32"))] + pub pick_override: Option>, /// Write exported files (split) here instead of asking (tests and automation). pub export_dir_override: Option, /// Split dialog settings. @@ -543,6 +551,10 @@ impl PdfCraftApp { props_draft: None, view_draft: None, requests: Default::default(), + #[cfg(not(target_arch = "wasm32"))] + pickers: Default::default(), + #[cfg(not(target_arch = "wasm32"))] + pick_override: None, export_dir_override: None, split_draft: SplitDraft::default(), extract_draft: ExtractDraft::default(), @@ -814,11 +826,11 @@ impl PdfCraftApp { pub fn open_dialog(&mut self) { #[cfg(not(target_arch = "wasm32"))] - if let Some(p) = - rfd::FileDialog::new().add_filter("PDF", &["pdf"]).add_filter("Images and text (converted to PDF)", &create_ui::CONVERTIBLE).pick_file() - { - self.open_path(&p.to_string_lossy()); - } + self.pick( + pickers::PickFor::Open, + rfd::AsyncFileDialog::new().add_filter("PDF", &["pdf"]).add_filter("Images and text (converted to PDF)", &create_ui::CONVERTIBLE), + false, + ); // Browsers pick files asynchronously; the bytes arrive through `inbox`. #[cfg(target_arch = "wasm32")] { @@ -1310,6 +1322,8 @@ impl eframe::App for PdfCraftApp { self.poll_ocr(); self.poll_action(); self.process_file_requests(); + #[cfg(not(target_arch = "wasm32"))] + self.process_picked(); // Pull finished renders into textures for every open document. for view in &mut self.views { if let Some(doc) = self.session.get(view.id) { diff --git a/crates/ui-egui/src/pickers.rs b/crates/ui-egui/src/pickers.rs new file mode 100644 index 0000000000..43fde6949f --- /dev/null +++ b/crates/ui-egui/src/pickers.rs @@ -0,0 +1,177 @@ +//! Native file pickers that never block the frame. +//! +//! A blocking `rfd::FileDialog` runs `-[NSOpenPanel runModal]` on macOS: a nested run loop inside +//! winit's event handler. An event that arrives while it spins re-enters the handler, winit +//! panics, and because the panic can't unwind out of the AppKit callback the app aborts. +//! `rfd::AsyncFileDialog` shows the panel without a nested run loop and returns at once instead. +//! A worker thread waits for the choice, and the app uses it on a later frame +//! ([`PdfCraftApp::process_picked`]). The panel has no parent window yet, like every other +//! picker in the app, so it floats rather than opening as a sheet. + +use std::future::Future; +use std::path::PathBuf; +use std::sync::atomic::{AtomicBool, Ordering}; +use std::sync::{Arc, Mutex, PoisonError}; + +use pdfcraft_engine::DocId; + +use crate::PdfCraftApp; +use crate::files::FilePurpose; + +/// What the chosen files are for. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum PickFor { + /// File ▸ Open: open the chosen file in a new tab. + Open, + /// Combine, insert, replace or OCR (see [`FilePurpose`]). + Files(FilePurpose), +} + +/// A finished pick, waiting for the next frame. +#[derive(Debug)] +struct Picked { + pick_for: PickFor, + /// The document the pick acts on, for purposes that edit the active document. + target: Option, + /// The chosen files; empty when the picker was cancelled. + paths: Vec, +} + +/// Pickers in flight and the picks they finished. +#[derive(Clone, Debug, Default)] +pub struct Pickers { + /// A picker is showing; a second request is ignored until it closes. + showing: Arc, + done: Arc>>, +} + +/// Clears `showing` when the worker ends, including when it panics. +struct Showing(Arc); + +impl Drop for Showing { + fn drop(&mut self) { + self.0.store(false, Ordering::SeqCst); + } +} + +impl Pickers { + /// Wait for `pick` on a worker thread and queue its result for the next frame. `pick` must + /// already have shown the picker (rfd starts the panel when the future is created, on the + /// main thread). Returns false when the worker couldn't start. + fn spawn(&self, pick_for: PickFor, target: Option, ctx: Option, pick: F) -> bool + where + F: Future> + Send + 'static, + { + let showing = Showing(self.showing.clone()); + let done = self.done.clone(); + let started = std::thread::Builder::new().name("file-picker".into()).spawn(move || { + let _showing = showing; + let paths = pollster::block_on(pick); + done.lock().unwrap_or_else(PoisonError::into_inner).push(Picked { pick_for, target, paths }); + if let Some(ctx) = ctx { + ctx.request_repaint(); + } + }); + started.is_ok() + } + + /// Queue a pick that is already known (tests and automation). + fn deliver(&self, pick_for: PickFor, target: Option, paths: Vec) { + self.done.lock().unwrap_or_else(PoisonError::into_inner).push(Picked { pick_for, target, paths }); + } + + fn take(&self) -> Vec { + std::mem::take(&mut *self.done.lock().unwrap_or_else(PoisonError::into_inner)) + } +} + +impl PdfCraftApp { + /// Show a native picker for `pick_for` without blocking the frame. The choice is used on a + /// later frame by [`Self::process_picked`]. + pub(crate) fn pick(&mut self, pick_for: PickFor, dialog: rfd::AsyncFileDialog, multiple: bool) { + if self.pickers.showing.swap(true, Ordering::SeqCst) { + return; + } + // Insert and Replace edit the active document: remember which one. + let target = match pick_for { + PickFor::Files(FilePurpose::InsertPages | FilePurpose::ReplacePages) => self.active_ids().map(|(_, id)| id), + _ => None, + }; + if let Some(paths) = self.pick_override.clone() { + self.pickers.showing.store(false, Ordering::SeqCst); + self.pickers.deliver(pick_for, target, paths.into_iter().map(PathBuf::from).collect()); + return; + } + // Creating the future shows the panel; it must happen here, on the main thread. + let started = if multiple { + let pick = dialog.pick_files(); + self.pickers.spawn(pick_for, target, self.ctx.clone(), async move { + pick.await.unwrap_or_default().into_iter().map(|h| h.path().to_path_buf()).collect() + }) + } else { + let pick = dialog.pick_file(); + self.pickers.spawn(pick_for, target, self.ctx.clone(), async move { pick.await.map(|h| h.path().to_path_buf()).into_iter().collect() }) + }; + if !started { + self.pickers.showing.store(false, Ordering::SeqCst); + self.notify("Couldn't show the file picker. Please try again."); + } + } + + /// Use the picks that finished since the last frame. Called every frame. + pub(crate) fn process_picked(&mut self) { + for Picked { pick_for, target, paths } in self.pickers.take() { + if paths.is_empty() { + continue; + } + if target.is_some() && self.active_ids().map(|(_, id)| id) != target { + self.notify("The document changed while you were choosing a file, so nothing was added."); + continue; + } + match pick_for { + PickFor::Open => paths.iter().for_each(|p| self.open_path(&p.to_string_lossy())), + PickFor::Files(purpose) => self.use_paths(purpose, &paths), + } + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn worker_queues_the_pick_and_clears_showing() { + let pickers = Pickers::default(); + pickers.showing.store(true, Ordering::SeqCst); + assert!(pickers.spawn(PickFor::Open, None, None, async { vec![PathBuf::from("a.pdf")] })); + for _ in 0..500 { + if !pickers.showing.load(Ordering::SeqCst) { + break; + } + std::thread::sleep(std::time::Duration::from_millis(2)); + } + assert!(!pickers.showing.load(Ordering::SeqCst)); + let picked = pickers.take(); + assert_eq!(picked.len(), 1); + assert_eq!(picked[0].paths, vec![PathBuf::from("a.pdf")]); + } + + #[test] + fn a_panicking_worker_clears_showing_and_queues_nothing() { + let pickers = Pickers::default(); + pickers.showing.store(true, Ordering::SeqCst); + fn fail() -> Vec { + panic!("picker failed") + } + assert!(pickers.spawn(PickFor::Open, None, None, async { fail() })); + for _ in 0..500 { + if !pickers.showing.load(Ordering::SeqCst) { + break; + } + std::thread::sleep(std::time::Duration::from_millis(2)); + } + assert!(!pickers.showing.load(Ordering::SeqCst)); + assert!(pickers.take().is_empty()); + } +} diff --git a/crates/ui-egui/tests/pickers.rs b/crates/ui-egui/tests/pickers.rs new file mode 100644 index 0000000000..e3b8084452 --- /dev/null +++ b/crates/ui-egui/tests/pickers.rs @@ -0,0 +1,106 @@ +//! Native file pickers never block the frame. A blocking picker ran a nested run loop inside +//! winit's event handler on macOS, which crashed the app when the user clicked File ▸ Open. +//! Commands now return at once, and the chosen files are used on a later frame. + +use egui_kittest::Harness; +use pdfcraft_render::PageRenderer; +use pdfcraft_ui_egui::PdfCraftApp; + +/// An `n`-page document with a proper xref table. +fn fixture(n: usize) -> Vec { + 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} /MediaBox [0 0 200 300] >>", kids.join(" "))); + objs.push("<< /Type /Font /Subtype /Type1 /BaseFont /Helvetica >>".into()); + for i in 0..n { + objs.push(format!("<< /Type /Page /Parent 2 0 R /Contents {} 0 R /Resources << /Font << /F1 3 0 R >> >> >>", 5 + 2 * i)); + let body = format!("BT /F1 24 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 offsets = Vec::new(); + for (i, o) in objs.iter().enumerate() { + offsets.push(out.len()); + out.extend_from_slice(format!("{} 0 obj\n{o}\nendobj\n", i + 1).as_bytes()); + } + let xref = out.len(); + out.extend_from_slice(format!("xref\n0 {}\n0000000000 65535 f \n", objs.len() + 1).as_bytes()); + for o in offsets { + 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{xref}\n%%EOF\n", objs.len() + 1).as_bytes()); + out +} + +fn pages(app: &PdfCraftApp, view: usize) -> usize { + let doc = app.session.get(app.views[view].id).unwrap(); + PageRenderer::new(doc.bytes.clone(), Default::default()).page_count() +} + +/// A shell with `docs` open, each the given number of pages. +fn harness(docs: &[usize]) -> Harness<'static, PdfCraftApp> { + let docs = docs.to_vec(); + let mut h = Harness::builder().with_size(egui::vec2(1200.0, 800.0)).build_eframe(move |_cc| { + let mut app = PdfCraftApp::new(); + for (i, n) in docs.iter().enumerate() { + app.open_bytes(&format!("doc{i}.pdf"), None, fixture(*n)).unwrap(); + } + app + }); + h.run_steps(2); + h +} + +#[test] +fn file_open_returns_at_once_and_opens_the_pick_on_the_next_frame() { + let dir = std::env::temp_dir().join(format!("printcraft-pickers-open-{}", std::process::id())); + std::fs::create_dir_all(&dir).unwrap(); + let file = dir.join("picked.pdf"); + std::fs::write(&file, fixture(2)).unwrap(); + + let mut h = harness(&[]); + h.state_mut().pick_override = Some(vec![file.to_string_lossy().into_owned()]); + assert!(h.state_mut().execute("file.open")); + assert!(h.state().views.is_empty(), "the command must not wait for the picker"); + h.run_steps(2); + assert_eq!(h.state().views.len(), 1, "the picked file opens on a later frame"); + assert_eq!(pages(h.state(), 0), 2); + let _ = std::fs::remove_dir_all(&dir); +} + +#[test] +fn insert_from_file_uses_the_pick_on_the_next_frame() { + let dir = std::env::temp_dir().join(format!("printcraft-pickers-insert-{}", std::process::id())); + std::fs::create_dir_all(&dir).unwrap(); + let file = dir.join("more.pdf"); + std::fs::write(&file, fixture(2)).unwrap(); + + let mut h = harness(&[3]); + h.state_mut().pick_override = Some(vec![file.to_string_lossy().into_owned()]); + assert!(h.state_mut().execute("page.insert")); + assert_eq!(pages(h.state(), 0), 3, "nothing is inserted until the pick arrives"); + h.run_steps(2); + assert_eq!(pages(h.state(), 0), 5); + let _ = std::fs::remove_dir_all(&dir); +} + +#[test] +fn a_pick_for_a_document_that_is_no_longer_active_is_dropped() { + let dir = std::env::temp_dir().join(format!("printcraft-pickers-switch-{}", std::process::id())); + std::fs::create_dir_all(&dir).unwrap(); + let file = dir.join("more.pdf"); + std::fs::write(&file, fixture(2)).unwrap(); + + let mut h = harness(&[3, 4]); + h.state_mut().active = Some(0); + h.state_mut().pick_override = Some(vec![file.to_string_lossy().into_owned()]); + assert!(h.state_mut().execute("page.insert")); + // The user switches tabs before the pick arrives. + h.state_mut().active = Some(1); + h.run_steps(2); + assert_eq!(pages(h.state(), 0), 3, "the document the pick was for is unchanged"); + assert_eq!(pages(h.state(), 1), 4, "the newly active document is unchanged"); + let toast = h.state().toast.as_ref().map(|(m, _)| m.clone()).unwrap_or_default(); + assert!(toast.contains("document changed"), "the user is told why nothing happened: {toast:?}"); + let _ = std::fs::remove_dir_all(&dir); +}