From 142e496f5b6792df1449cd18b9c69e1fd26df7c4 Mon Sep 17 00:00:00 2001 From: francoisgn <32835330+francoisgn@users.noreply.github.com> Date: Sat, 24 Oct 2026 05:25:34 +0300 Subject: [PATCH] #1290: a failed lookup isn't "not found"; recent libraries aren't checked on a timer CodeRabbit on #2689: - `unavailable` used `exists()` / `is_dir()`, which are false for any error: a parent folder that can't be read or a share that timed out came out as "Not found" and greyed the entry out. Only an absent path is "not found" now; other lookup errors are "This library couldn't be checked: " (LookupFailed), and a folder that refuses to be listed is no longer counted as missing either (it is there: permissions, macOS privacy), so neither is greyed out and opening says why. - Every recent library was checked again every 30 s while the app ran, waking external drives and network shares. A library not found is looked for again every 30 s (a drive plugged back in; an absent path wakes nothing); the others only when the window comes back to the front, for the 8 entries shown; paths never checked are checked once. Tests a_failed_lookup_doesnt_mark_a_recent_library_missing and recent_libraries_are_checked_again_only_when_the_window_comes_back fail with the old lookup / the old timer / no timer for libraries not found. Co-Authored-By: Claude Opus 5.5 --- crates/ui-egui/locales/fr.json | 3 +- crates/ui-egui/src/panels/library_problem.rs | 68 +++++++++++++++---- crates/ui-egui/src/tests_launch_library.rs | 71 ++++++++++++++++++++ docs/control-protocol.md | 7 +- 4 files changed, 133 insertions(+), 16 deletions(-) diff --git a/crates/ui-egui/locales/fr.json b/crates/ui-egui/locales/fr.json index b4fadd741..19f33b140 100644 --- a/crates/ui-egui/locales/fr.json +++ b/crates/ui-egui/locales/fr.json @@ -2363,5 +2363,6 @@ "This library couldn't be checked": "Impossible de vérifier cette bibliothèque", "Checking recent libraries": "Vérification des bibliothèques récentes", "The photos are imported once a library is open": "Les photos seront importées dès qu'une bibliothèque sera ouverte", - "Open a library first, then drop the presets again": "Ouvrez d'abord une bibliothèque, puis déposez à nouveau les préréglages" + "Open a library first, then drop the presets again": "Ouvrez d'abord une bibliothèque, puis déposez à nouveau les préréglages", + "This library couldn't be checked:": "Impossible de vérifier cette bibliothèque :" } diff --git a/crates/ui-egui/src/panels/library_problem.rs b/crates/ui-egui/src/panels/library_problem.rs index 1b4efd523..e3e4ee205 100644 --- a/crates/ui-egui/src/panels/library_problem.rs +++ b/crates/ui-egui/src/panels/library_problem.rs @@ -195,6 +195,9 @@ pub enum Unavailable { CatalogUnreadable(String), /// The check itself failed (its worker stopped, or couldn't start). CheckFailed(String), + /// Looking at the path failed without saying it is gone (a parent folder it may not read, a + /// share that timed out; the system's reason). + LookupFailed(String), } impl Unavailable { @@ -202,7 +205,8 @@ impl Unavailable { /// unlistable): what greys out File ▸ Open Recent Library and makes `app.recentLibraries` say /// `exists: false`. An unreadable catalog or a check that failed doesn't say the library is gone. pub fn is_missing(&self) -> bool { - matches!(self, Unavailable::NotFound | Unavailable::NotAFolder | Unavailable::NoLibrary | Unavailable::Unreadable(_)) + // a folder that refuses to list is there (permissions, macOS privacy): opening it says why + matches!(self, Unavailable::NotFound | Unavailable::NotAFolder | Unavailable::NoLibrary) } /// In the interface language. @@ -214,12 +218,15 @@ impl Unavailable { Unavailable::NoLibrary => crate::i18n::tr("There is no LightCraft library in this folder any more").to_string(), Unavailable::CatalogUnreadable(e) => format!("{} {e}", crate::i18n::tr("The library's catalog can't be read:")), Unavailable::CheckFailed(e) => e.clone(), + Unavailable::LookupFailed(e) => format!("{} {e}", crate::i18n::tr("This library couldn't be checked:")), } } } -/// How long an answer stands before File ▸ Open Recent Library asks again (a drive plugged back -/// in, a library moved away meanwhile). +/// How old an answer must be to be asked again: a library not found, on its own (a drive plugged +/// back in); any other, when the window comes back to the front (a library moved away meanwhile). +/// Only "not found" goes on a timer: looking again at the others would wake every external drive +/// and network share in the list over and over. pub const RECHECK_AFTER: std::time::Duration = std::time::Duration::from_secs(30); /// How long a row stays "Checking…" before it says the drive isn't answering (it can still be /// opened: opening says why if it can't). @@ -244,12 +251,22 @@ pub struct RecentChecks { pending: std::collections::HashMap, /// How one path is checked ([`unavailable`]; tests put a slow one here). check: std::sync::Arc Option + Send + Sync>, + /// [`RECHECK_AFTER`] (shorter in tests). + recheck_after: std::time::Duration, + /// Whether the window had the focus on the last frame: answers are refreshed when it comes back. + pub(crate) focused: bool, } impl Default for RecentChecks { /// Nothing known yet, checking with [`unavailable`]. fn default() -> Self { - RecentChecks { known: Default::default(), pending: Default::default(), check: std::sync::Arc::new(unavailable) } + RecentChecks { + known: Default::default(), + pending: Default::default(), + check: std::sync::Arc::new(unavailable), + recheck_after: RECHECK_AFTER, + focused: false, + } } } @@ -294,6 +311,13 @@ impl RecentChecks { RecentChecks { check: std::sync::Arc::new(check), ..Default::default() } } + /// Ask again about answers older than `after` when the window comes back (tests). + #[cfg(test)] + pub fn recheck_after(mut self, after: std::time::Duration) -> RecentChecks { + self.recheck_after = after; + self + } + /// `path` was just opened (or made): it can be opened. pub fn opened(&mut self, path: &str) { self.known.insert(path.to_string(), Answer { why: None, at: web_time::Instant::now() }); @@ -387,14 +411,18 @@ fn apply_answers(app: &mut LightcraftApp) { /// who has it. Runs on a worker: the reason is put into words on the UI thread. fn unavailable(path: &str) -> Option { let dir = Path::new(path); - if !dir.exists() { - return Some(Unavailable::NotFound); + // only a definite answer marks it missing: a lookup that fails otherwise (a parent folder it may + // not read, a share that timed out) is "couldn't check", and the library can still be opened + match std::fs::metadata(dir) { + Err(e) if e.kind() == std::io::ErrorKind::NotFound => return Some(Unavailable::NotFound), + Err(e) => return Some(Unavailable::LookupFailed(e.to_string())), + Ok(m) if !m.is_dir() => return Some(Unavailable::NotAFolder), + Ok(_) => {} } - if !dir.is_dir() { - return Some(Unavailable::NotAFolder); - } - if let Err(e) = std::fs::read_dir(dir) { - return Some(Unavailable::Unreadable(e.to_string())); + match std::fs::read_dir(dir) { + Err(e) if e.kind() == std::io::ErrorKind::PermissionDenied => return Some(Unavailable::Unreadable(e.to_string())), + Err(e) => return Some(Unavailable::LookupFailed(e.to_string())), + Ok(_) => {} } // opening would make a new, empty library in it if !lightcraft_engine::library::is_library(dir) { @@ -524,13 +552,27 @@ pub fn logic(app: &mut LightcraftApp, ctx: &egui::Context) { start_checks(app, listed); apply_answers(app); } - // File ▸ Open Recent Library: paths never checked, or not for a while + // File ▸ Open Recent Library: paths never checked; libraries not found, every RECHECK_AFTER (a + // drive plugged back in; looking at a path that isn't there wakes nothing); the others when the + // window comes back to the front and their answer is that old. The list as shown (at most + // RECENT_LIBRARIES). + let focused = ctx.input(|i| i.focused); + let regained = focused && !std::mem::replace(&mut app.recent_checks.focused, focused); + let after = app.recent_checks.recheck_after; let stale: Vec = app .ui .settings .recent_libraries .iter() - .filter(|p| !app.recent_checks.is_checking(p) && app.recent_checks.known.get(*p).is_none_or(|a| a.at.elapsed() > RECHECK_AFTER)) + .take(crate::state::RECENT_LIBRARIES) + .filter(|p| { + !app.recent_checks.is_checking(p) + && app + .recent_checks + .known + .get(*p) + .is_none_or(|a| (regained || matches!(a.why, Some(Unavailable::NotFound))) && a.at.elapsed() > after) + }) .cloned() .collect(); start_checks(app, stale); diff --git a/crates/ui-egui/src/tests_launch_library.rs b/crates/ui-egui/src/tests_launch_library.rs index 0f0abbf78..39f18d76e 100644 --- a/crates/ui-egui/src/tests_launch_library.rs +++ b/crates/ui-egui/src/tests_launch_library.rs @@ -866,3 +866,74 @@ fn photos_dropped_on_the_launch_list_go_into_the_library_chosen() { drop(h); let _ = std::fs::remove_dir_all(&dir); } + +/// A lookup that fails without saying the library is gone (here a parent folder it may not read; +/// a network share that timed out is the same) is "couldn't check", not "not found": the entry +/// isn't greyed out in File ▸ Open Recent Library (CodeRabbit on #2689). +#[cfg(unix)] +#[test] +fn a_failed_lookup_doesnt_mark_a_recent_library_missing() { + use std::os::unix::fs::PermissionsExt; + let dir = temp("lookup"); + let locked = dir.join("Locked"); + let inside = make_library(&locked.join("Inside")); + let gone = dir.join("Gone").to_string_lossy().into_owned(); + std::fs::set_permissions(&locked, std::fs::Permissions::from_mode(0o000)).unwrap(); + let mut app = LightcraftApp::new(lightcraft_engine::Session::new().with_fs(), Services::default()); + app.ui.settings.recent_libraries = vec![inside.clone(), gone.clone()]; + let mut h = Headless::new(app, [900.0, 600.0], 1.0); + let answered = h.step_until(T, |h| h.app.recent_checks.is_known(&inside) && h.app.recent_checks.is_known(&gone)); + std::fs::set_permissions(&locked, std::fs::Permissions::from_mode(0o755)).unwrap(); + assert!(answered); + assert_eq!(h.app.recent_checks.exists(&gone), Some(false), "a library that isn't there is missing"); + assert_eq!(h.app.recent_checks.exists(&inside), None, "one that couldn't be looked at isn't"); + let why = h.app.recent_checks.reason(&inside).map(|w| w.text()).unwrap_or_default(); + assert!(why.starts_with("This library couldn't be checked:"), "{why}"); + // nor is a folder that is there but refuses to be listed (permissions, macOS privacy) + use crate::panels::library_problem::Unavailable; + assert!(!Unavailable::Unreadable("denied".into()).is_missing() && !Unavailable::LookupFailed("timed out".into()).is_missing()); + assert!(Unavailable::NotFound.is_missing()); + drop(h); + let _ = std::fs::remove_dir_all(&dir); +} + +/// A library found is asked about again when the window comes back to the front, not on a timer: a +/// timer would wake every external drive and network share in the list over and over. One not +/// found is looked for again on its own (a drive plugged back in with the app in front). +#[test] +fn recent_libraries_are_checked_again_only_when_the_window_comes_back() { + use std::sync::atomic::{AtomicUsize, Ordering}; + let dir = temp("recheck"); + let a = make_library(&dir.join("A")); + let gone = dir.join("Gone").to_string_lossy().into_owned(); + let (calls, gone_calls) = (std::sync::Arc::new(AtomicUsize::new(0)), std::sync::Arc::new(AtomicUsize::new(0))); + let (c, g, gone_path) = (calls.clone(), gone_calls.clone(), gone.clone()); + let mut app = LightcraftApp::new(lightcraft_engine::Session::new().with_fs(), Services::default()); + app.recent_checks = crate::panels::library_problem::RecentChecks::with_check(move |p| { + if p == gone_path { + g.fetch_add(1, Ordering::SeqCst); + return Some(crate::panels::library_problem::Unavailable::NotFound); + } + c.fetch_add(1, Ordering::SeqCst); + None + }) + .recheck_after(Duration::ZERO); + app.ui.settings.recent_libraries = vec![a.clone(), gone.clone()]; + let mut h = Headless::new(app, [900.0, 600.0], 1.0); + assert!(h.step_until(T, |h| h.app.recent_checks.exists(&a) == Some(true))); + assert!(h.step_until(T, |_| gone_calls.load(Ordering::SeqCst) >= 3), "the one not found is looked for again on its own"); + // focused all along, every answer "old": nothing is asked again + for _ in 0..30 { + h.step(); + } + assert_eq!(calls.load(Ordering::SeqCst), 1, "no check on a timer"); + // the window went to the back and comes to the front again + h.app.recent_checks.focused = false; + assert!(h.step_until(T, |_| calls.load(Ordering::SeqCst) == 2), "asked again when the window comes back"); + for _ in 0..10 { + h.step(); + } + assert_eq!(calls.load(Ordering::SeqCst), 2, "once"); + drop(h); + let _ = std::fs::remove_dir_all(&dir); +} diff --git a/docs/control-protocol.md b/docs/control-protocol.md index f2e84ed87..ebfa6fb55 100644 --- a/docs/control-protocol.md +++ b/docs/control-protocol.md @@ -193,8 +193,11 @@ background check found (`false`: moved, on a disconnected drive, a folder that c library; `null`: not checked yet, or the check couldn't tell, e.g. an unreadable catalog) and `checking` whether one is running: the command never looks at a folder itself. A failed open never marks a library missing (another program may have it). Recent libraries are checked in the -background when the app starts, again after 30 s, and when the launch window opens; File ▸ Open Recent Library -greys out only an entry a check found missing (an unreadable catalog or a failed open doesn't grey it out). `lastOpened`: ISO 8601, `null` when not known; kept in `ui.json` → +background when the app starts (the first 8) and when the launch window opens; one not found is looked for again every +30 s (a drive plugged back in), the others only when the window comes back to the front (a timer would wake external +drives and shares). File ▸ Open Recent Library greys out only an entry a check found missing (not there, not a folder, +no library in it); an unreadable catalog, a folder that refuses to be listed, a failed open or a lookup that failed +(a share that timed out) doesn't grey it out. `lastOpened`: ISO 8601, `null` when not known; kept in `ui.json` → `settings.recentLibrariesOpened`. `{remove: path}` forgets one entry (an error if the list doesn't hold it) and `{clear: true}` (or `app.clearRecentLibraries`) all of them; nothing on disk is touched.