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 [