From d0991a8cd527089fb36c89967a6aac075f4a7ddd Mon Sep 17 00:00:00 2001 From: "HG.Studio" Date: Thu, 8 Oct 2026 02:37:07 +0100 Subject: [PATCH] =?UTF-8?q?macOS:=20File=20=E2=96=B8=20Open=20and=20file?= =?UTF-8?q?=20pickers=20no=20longer=20crash=20the=20app=20(#145)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * macOS: File ▸ Open and file pickers no longer crash the app A blocking rfd::FileDialog runs -[NSOpenPanel runModal], a nested run loop inside winit's event handler. A mouse or gesture event that arrives while it spins re-enters the handler, winit panics ("tried to handle event while another event is currently being handled"), and because the panic can't unwind out of the AppKit block callback the app aborts. Open, Combine, Insert pages, Replace pages and OCR ▸ multiple files now use rfd::AsyncFileDialog, which starts the panel as a sheet and returns at once. A worker thread waits for the choice and the app uses it on a later frame. Insert and Replace remember their document and drop a pick that arrives after the user switched tabs. pick_override lets tests and automation drive these commands. Co-Authored-By: Claude Opus 5.5 * Cargo.lock: pollster for pdfcraft-ui-egui Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Claude Opus 5.5 Co-authored-by: Brandon Thomas --- Cargo.lock | 1 + crates/ui-egui/Cargo.toml | 2 + crates/ui-egui/src/files.rs | 42 ++++---- crates/ui-egui/src/lib.rs | 24 ++++- crates/ui-egui/src/pickers.rs | 177 ++++++++++++++++++++++++++++++++ crates/ui-egui/tests/pickers.rs | 106 +++++++++++++++++++ 6 files changed, 327 insertions(+), 25 deletions(-) create mode 100644 crates/ui-egui/src/pickers.rs create mode 100644 crates/ui-egui/tests/pickers.rs 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); +}