mirror of
https://github.com/storytold/pdfcraft.git
synced 2026-11-03 15:05:02 -06:00
* 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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> Co-authored-by: Brandon Thomas <echelon@gmail.com>
This commit is contained in:
co-authored by
Claude Opus 5.5
Brandon Thomas
parent
0cba87ae6f
commit
f035218cb4
@@ -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 {
|
||||
|
||||
@@ -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);
|
||||
}
|
||||
|
||||
@@ -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::<Vec<RecentFile>>(v["recent"].clone()) {
|
||||
// Only keep entries whose files still exist.
|
||||
#[cfg(not(target_arch = "wasm32"))]
|
||||
let r: Vec<RecentFile> = 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.
|
||||
|
||||
@@ -392,6 +392,8 @@ pub struct PdfCraftApp {
|
||||
open_prepare_gate: Option<std::sync::Arc<open_worker::PrepareGate>>,
|
||||
/// 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<String>,
|
||||
/// Search query for the Pin a tool selection dialog.
|
||||
|
||||
@@ -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"))]
|
||||
|
||||
@@ -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<String>,
|
||||
/// The paths the worker has found gone so far; it adds to them as it goes.
|
||||
gone: Option<Arc<Mutex<Vec<String>>>>,
|
||||
/// Paths opened while a check was pending: the file is there now, whatever the worker found.
|
||||
opened: Vec<String>,
|
||||
}
|
||||
|
||||
impl RecentCheck {
|
||||
/// Settings just restored these recent files: check them once the window is up.
|
||||
pub(crate) fn restored(&mut self, paths: Vec<String>) {
|
||||
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();
|
||||
}
|
||||
}
|
||||
@@ -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<RecentFile> =
|
||||
[&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 [
|
||||
|
||||
Reference in New Issue
Block a user