From f035218cb47e414ccc649c59229315bab8d89f0b Mon Sep 17 00:00:00 2001 From: Elarionus <29511515+Elarionus@users.noreply.github.com> Date: Sun, 1 Nov 2026 20:49:10 -0800 Subject: [PATCH] fix(ui): don't check recent files on the UI thread at startup (#5682) (#5692) * fix(ui): don't check recent files on the UI thread at startup (#5682) Restoring settings dropped recent files whose files no longer exist by asking the file system about each one on the UI thread, before the first frame. On a network mount that has stopped answering (davfs2 in the report) that call blocks in the kernel, so the window never appears. Settings now keep every recent entry when they load. Once the window is up, a worker checks the restored paths and each frame drops the ones it has found gone. An entry it hasn't reached, or can't tell about (the mount answers with an error), stays listed. A worker stuck on a dead mount stays stuck, but nothing waits for it. This follows the pinned folders' worker. Fixes #5682 Co-Authored-By: Claude Opus 5.5 * Recent files: a file opened while the check is pending stays listed The worker's answer that a restored path is gone is out of date once the user opens that path; keep the new entry (CodeRabbit on #5692). Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Claude Opus 5.5 Co-authored-by: Brandon Thomas --- crates/ui-egui/src/app.rs | 2 + crates/ui-egui/src/app/files.rs | 1 + crates/ui-egui/src/app/prefs.rs | 6 +- crates/ui-egui/src/lib.rs | 2 + crates/ui-egui/src/persist.rs | 1 + crates/ui-egui/src/persist/recent_check.rs | 125 +++++++++++++++++++++ crates/ui-egui/tests/it/recent_files.rs | 33 ++++++ 7 files changed, 167 insertions(+), 3 deletions(-) create mode 100644 crates/ui-egui/src/persist/recent_check.rs diff --git a/crates/ui-egui/src/app.rs b/crates/ui-egui/src/app.rs index 43d0066e63..c326b479fe 100644 --- a/crates/ui-egui/src/app.rs +++ b/crates/ui-egui/src/app.rs @@ -144,6 +144,7 @@ impl PdfCraftApp { #[cfg(all(test, not(target_arch = "wasm32")))] open_prepare_gate: None, pinned: Default::default(), + recent_check: Default::default(), pinned_tools: Vec::new(), pin_tool_search: String::new(), toast: None, @@ -452,6 +453,7 @@ impl eframe::App for PdfCraftApp { } self.poll_pending_opens(); } + self.check_restored_recent(); // Chinese wants its own faces before the Japanese ones (one baseline per line). let cjk = theme::CjkPreference::for_language(lang.code()); if !self.styled { diff --git a/crates/ui-egui/src/app/files.rs b/crates/ui-egui/src/app/files.rs index 29e6978461..9991047077 100644 --- a/crates/ui-egui/src/app/files.rs +++ b/crates/ui-egui/src/app/files.rs @@ -332,6 +332,7 @@ impl PdfCraftApp { #[cfg(target_os = "macos")] macos_association::repair(std::path::Path::new(&p)); self.recent.retain(|r| r.path != p); + self.recent_check.opened(&p); self.recent.insert(0, RecentFile { name: name.to_string(), path: p, pages, size }); self.recent.truncate(self.recent_limit); } diff --git a/crates/ui-egui/src/app/prefs.rs b/crates/ui-egui/src/app/prefs.rs index 7bd8943774..7eddee0af0 100644 --- a/crates/ui-egui/src/app/prefs.rs +++ b/crates/ui-egui/src/app/prefs.rs @@ -182,9 +182,9 @@ impl PdfCraftApp { self.organize_zoom = zoom.clamp(*canvas::GRID_ZOOM_RANGE.start(), *canvas::GRID_ZOOM_RANGE.end()); } if let Ok(r) = serde_json::from_value::>(v["recent"].clone()) { - // Only keep entries whose files still exist. - #[cfg(not(target_arch = "wasm32"))] - let r: Vec = r.into_iter().filter(|f| std::path::Path::new(&f.path).exists()).collect(); + // Entries whose files are gone leave once the window is up: asking here could block + // startup on a mount that has stopped answering (#5682). + self.recent_check.restored(r.iter().map(|f| f.path.clone()).collect()); self.recent = r; } // Nonsense keeps the limit it had; settings from before the limit have none. diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index fe0138f27f..c25dae98e3 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -392,6 +392,8 @@ pub struct PdfCraftApp { open_prepare_gate: Option>, /// Folders pinned to Home, and what they held when last listed. pub pinned: folders_ui::PinnedFolders, + /// The recent files restored from settings, checked off the UI thread (#5682). + recent_check: persist::recent_check::RecentCheck, /// Tools pinned to Home, in user-defined order. pub pinned_tools: Vec, /// Search query for the Pin a tool selection dialog. diff --git a/crates/ui-egui/src/persist.rs b/crates/ui-egui/src/persist.rs index 32a8ec048d..20ccecdd3f 100644 --- a/crates/ui-egui/src/persist.rs +++ b/crates/ui-egui/src/persist.rs @@ -1,6 +1,7 @@ pub mod last_session; pub mod last_view; pub mod paper; +pub(crate) mod recent_check; pub(super) mod recovery; pub mod units; #[cfg(not(target_arch = "wasm32"))] diff --git a/crates/ui-egui/src/persist/recent_check.rs b/crates/ui-egui/src/persist/recent_check.rs new file mode 100644 index 0000000000..8c7db75f8b --- /dev/null +++ b/crates/ui-egui/src/persist/recent_check.rs @@ -0,0 +1,125 @@ +//! Recent files whose files are gone leave the list, without the window waiting to find out +//! (#5682). +//! +//! A recent file can be on a network or cloud mount that has stopped answering, where even +//! asking whether the file exists blocks in the kernel, and no timeout can cancel that. So +//! settings keep every recent entry when they load, and a worker asks about them once the window +//! is up; each frame only takes the answers it has so far. An entry the worker hasn't reached +//! yet, or can't tell about (the mount answers with an error), stays listed and can be opened or +//! removed by hand. A worker stuck on a dead mount stays stuck, but nothing waits for it. + +use std::sync::{Arc, Mutex, PoisonError}; + +use crate::PdfCraftApp; + +/// The recent files restored from settings that haven't been checked yet, and the check under way. +#[derive(Debug, Default)] +pub struct RecentCheck { + /// Paths restored from settings, checked on the next frame. + unchecked: Vec, + /// The paths the worker has found gone so far; it adds to them as it goes. + gone: Option>>>, + /// Paths opened while a check was pending: the file is there now, whatever the worker found. + opened: Vec, +} + +impl RecentCheck { + /// Settings just restored these recent files: check them once the window is up. + pub(crate) fn restored(&mut self, paths: Vec) { + self.unchecked = paths; + } + + /// `path` was just opened: an answer the worker gives about it later is out of date. + pub(crate) fn opened(&mut self, path: &str) { + if self.gone.is_some() || !self.unchecked.is_empty() { + self.opened.push(path.to_string()); + } + } +} + +/// Whether `path` is known to be gone. Any other answer, an error included, keeps the entry. +fn gone(path: &str) -> bool { + matches!(std::fs::exists(path), Ok(false)) +} + +impl PdfCraftApp { + /// Start checking the recent files restored from settings, and drop the ones found gone so + /// far (called each frame). The check runs on a worker thread, inline in tests + /// ([`PdfCraftApp::run_inline`]). + pub(crate) fn check_restored_recent(&mut self) { + let paths = std::mem::take(&mut self.recent_check.unchecked); + // The web build's recent files aren't paths on a file system. + if !paths.is_empty() && !cfg!(target_arch = "wasm32") { + let slot = Arc::new(Mutex::new(Vec::new())); + let out = slot.clone(); + let ctx = self.ctx.clone(); + let work = move || { + for path in paths { + if gone(&path) { + out.lock().unwrap_or_else(PoisonError::into_inner).push(path); + if let Some(ctx) = &ctx { + ctx.request_repaint(); + } + } + } + }; + if self.run_inline { + work(); + } else if std::thread::Builder::new().name("pdfcraft-recent".into()).spawn(work).is_err() { + // No worker: the entries stay listed, as if the check hadn't reached them. + return; + } + self.recent_check.gone = Some(slot); + } + let Some(slot) = &self.recent_check.gone else { return }; + // Asked before taking, so a path the worker adds after this is taken on a later frame. + let finished = Arc::strong_count(slot) == 1; + let gone = std::mem::take(&mut *slot.lock().unwrap_or_else(PoisonError::into_inner)); + if !gone.is_empty() { + // A file opened since the check began is there, whatever the worker found earlier. + let opened = &self.recent_check.opened; + self.recent.retain(|r| !gone.contains(&r.path) || opened.contains(&r.path)); + } + // The worker has finished (or panicked) and everything it found is taken. + if finished { + self.recent_check.gone = None; + self.recent_check.opened.clear(); + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + /// A file opened after the worker found it gone, but before that answer was taken, stays + /// listed (CodeRabbit on #5692). + #[test] + fn a_file_opened_while_the_check_is_pending_stays() { + let mut app = PdfCraftApp::new(); + app.recent = vec![crate::state::RecentFile { name: "a.pdf".into(), path: "/gone/a.pdf".into(), pages: 1, size: 0 }]; + // The worker's answer is waiting: it found the file gone. + app.recent_check.gone = Some(Arc::new(Mutex::new(vec!["/gone/a.pdf".to_string()]))); + app.recent_check.opened("/gone/a.pdf"); + app.check_restored_recent(); + assert_eq!(app.recent.len(), 1, "the reopened file stays"); + assert!(app.recent_check.gone.is_none() && app.recent_check.opened.is_empty()); + // Without the open, the answer removes it. + app.recent_check.gone = Some(Arc::new(Mutex::new(vec!["/gone/a.pdf".to_string()]))); + app.check_restored_recent(); + assert!(app.recent.is_empty()); + } + + #[test] + fn only_a_path_known_to_be_missing_is_gone() { + let dir = std::env::temp_dir().join(format!("pdfcraft-recent-check-{}", std::process::id())); + std::fs::create_dir_all(&dir).unwrap(); + let here = dir.join("here.pdf"); + std::fs::write(&here, b"%PDF-1.7").unwrap(); + assert!(!gone(&here.to_string_lossy())); + assert!(gone(&dir.join("missing.pdf").to_string_lossy())); + // A path that can't be asked about (here: one with a NUL) is not known to be gone. + assert!(!gone("bad\0path.pdf")); + std::fs::remove_dir_all(&dir).unwrap(); + } +} diff --git a/crates/ui-egui/tests/it/recent_files.rs b/crates/ui-egui/tests/it/recent_files.rs index b4eccfb10e..e3f7721d2f 100644 --- a/crates/ui-egui/tests/it/recent_files.rs +++ b/crates/ui-egui/tests/it/recent_files.rs @@ -221,6 +221,39 @@ fn the_limit_is_saved_and_restored_and_cuts_a_longer_list() { std::fs::remove_dir_all(&dir).unwrap(); } +/// #5682: restoring settings never asks the file system about recent files, which can block on a +/// mount that has stopped answering. A gone file leaves the list once frames run, and a file that +/// is there stays. +#[test] +fn restoring_settings_does_not_check_recent_files_and_a_frame_drops_the_gone_ones() { + let dir = std::env::temp_dir().join(format!("pdfcraft-recent-restore-{}", std::process::id())); + std::fs::create_dir_all(&dir).unwrap(); + let here = dir.join("here.pdf"); + std::fs::write(&here, PDF).unwrap(); + let here = here.to_string_lossy().into_owned(); + let gone = dir.join("gone.pdf").to_string_lossy().into_owned(); + let entries: Vec = + [&here, &gone].iter().map(|p| RecentFile { name: "x.pdf".into(), path: (*p).clone(), pages: 1, size: 0 }).collect(); + let json = serde_json::json!({ "recent": entries }).to_string(); + + let mut restored = PdfCraftApp::new(); + restored.restore(&json); + let paths: Vec<&str> = restored.recent.iter().map(|r| r.path.as_str()).collect(); + assert_eq!(paths, [here.as_str(), gone.as_str()], "restoring keeps every entry without asking whether its file exists"); + + let mut h = Harness::builder().with_size(egui::vec2(1400.0, 1600.0)).build_eframe(move |_cc| { + let mut app = PdfCraftApp::new(); + app.set_option("language", "en").unwrap(); + app.run_inline = true; + app.restore(&json); + app + }); + h.run_steps(2); + let paths: Vec<&str> = h.state().recent.iter().map(|r| r.path.as_str()).collect(); + assert_eq!(paths, [here.as_str()], "the gone file leaves once the window is up"); + std::fs::remove_dir_all(&dir).unwrap(); +} + #[test] fn a_settings_file_can_not_ask_for_a_limit_that_is_not_one() { for (saved, want) in [