diff --git a/crates/engine/src/cmd/missing.rs b/crates/engine/src/cmd/missing.rs index 09e2131ef..3c1bd7799 100644 --- a/crates/engine/src/cmd/missing.rs +++ b/crates/engine/src/cmd/missing.rs @@ -52,6 +52,18 @@ pub fn checked(cat: &Catalog) -> Vec<(PhotoId, String)> { cat.photos().filter_map(|p| checked_path(p).map(|f| (p.id, f.to_string()))).collect() } +/// What [`checked`] would list, as one number, without building the list: the photos in scope and +/// the file each one points at, whatever their order. A caller that checked on its own thread +/// compares it before and after to know the answer is still of the catalog it is given to. +pub fn fingerprint(cat: &Catalog) -> u64 { + use std::hash::{DefaultHasher, Hash, Hasher}; + cat.photos().filter_map(|p| checked_path(p).map(|f| (p.id, f))).fold(0u64, |sum, entry| { + let mut h = DefaultHasher::new(); + entry.hash(&mut h); + sum.wrapping_add(h.finish()) + }) +} + /// How many files [`sweep`] checks between two reports of its progress. const PROGRESS_EVERY: usize = 64; @@ -76,7 +88,8 @@ pub fn sweep( gone.push((id, file)); } } - Some(gone) + // (a stop that came during the last check is honoured too) + (!stop()).then_some(gone) } /// `library.missing`'s answer: `[{id, path}]`. diff --git a/crates/engine/src/tests/import.rs b/crates/engine/src/tests/import.rs index 89b2dcafa..c8e2ce2da 100644 --- a/crates/engine/src/tests/import.rs +++ b/crates/engine/src/tests/import.rs @@ -1304,6 +1304,27 @@ fn missing_sweep_answers_what_the_command_does_and_can_stop() { assert_eq!((stopped, looked.load(Ordering::Relaxed)), (None, 10)); } +/// A stop that arrives while the last file is being checked is honoured: no answer. +#[test] +fn missing_sweep_honours_a_stop_during_the_last_check() { + use lightcraft_catalog::PhotoId; + use std::sync::atomic::{AtomicBool, Ordering}; + let stop = AtomicBool::new(false); + let files = vec![(PhotoId(1), "/a".to_string()), (PhotoId(2), "/b".to_string())]; + let swept = crate::cmd::missing::sweep( + files, + |f| { + if f == "/b" { + stop.store(true, Ordering::Relaxed); + } + false + }, + || stop.load(Ordering::Relaxed), + |_| {}, + ); + assert_eq!(swept, None); +} + /// `library.browse` follows `library.source`'s rule for the active photo (issue #1447): a library /// photo the folder doesn't show gives way to the folder's first photo; one it shows stays. #[test] diff --git a/crates/ui-egui/src/missing_sweep.rs b/crates/ui-egui/src/missing_sweep.rs index dd743b8ef..6ac45d1e2 100644 --- a/crates/ui-egui/src/missing_sweep.rs +++ b/crates/ui-egui/src/missing_sweep.rs @@ -38,9 +38,33 @@ const KEEP: Duration = Duration::from_secs(10 * 60); #[derive(Default)] pub struct State { /// Each request with the library ([`lightcraft_engine::Session::library_generation`]) it asked about. - waiting: Vec<(u64, Sender)>, - /// A finished check's answer that no request received: its library and when it was kept. - kept: Option<(u64, Instant, Value)>, + pub(crate) waiting: Vec<(u64, Sender)>, + /// A finished check's answer that no request received. + kept: Option, +} + +/// An answer nobody received: its library, the photos it was of ([`missing::fingerprint`]), when it was kept. +struct Kept { + library: u64, + snapshot: u64, + at: Instant, + answer: Value, +} + +impl State { + /// Send `answer` to every request waiting on `library`; `true` if any caller was still there. + /// (a request whose caller gave up has no one to send to) + fn answer(&mut self, library: u64, answer: &Value) -> bool { + let (theirs, others): (Vec<_>, Vec<_>) = std::mem::take(&mut self.waiting).into_iter().partition(|(l, _)| *l == library); + self.waiting = others; + theirs.into_iter().fold(false, |received, (_, waiting)| waiting.send(answer.clone()).is_ok() || received) + } + + /// A check for `library` could not start: fail its requests, and only those (another library's + /// check may be running for the others). + pub(crate) fn fail(&mut self, library: u64, error: &str) { + self.answer(library, &control::failure(error)); + } } /// Answer `library.missing` from the control channel: give the answer kept from a check nobody @@ -48,17 +72,19 @@ pub struct State { /// `reply` when the check ends. pub fn ask(app: &mut LightcraftApp, reply: Sender) -> Outcome { let library = app.session.library_generation(); - if let Some((kept_for, at, answer)) = app.missing.kept.take() - && kept_for == library - && at.elapsed() < KEEP + if let Some(kept) = app.missing.kept.take() + && kept.library == library + && kept.at.elapsed() < KEEP + && kept.snapshot == missing::fingerprint(&app.session.catalog) { - return Outcome::Done(answer); + return Outcome::Done(kept.answer); } app.missing.waiting.push((library, reply)); if app.tasks.is_running(LABEL) && app.missing.waiting.iter().filter(|(l, _)| *l == library).count() > 1 { return Outcome::Later; } let files = missing::checked(&app.session.catalog); + let snapshot = missing::fingerprint(&app.session.catalog); let total = files.len() as u64; let exists = app.session.media.availability.probe(); let cancel = Arc::new(AtomicBool::new(false)); @@ -75,22 +101,21 @@ pub fn ask(app: &mut LightcraftApp, reply: Sender) -> Outcome { _ if app.session.library_generation() != library => { (control::failure("Missing Photos: another library was opened before the check ended"), false) } + // (photos added, removed or relinked while the files were checked: this is not the catalog's answer) + Ok(_) if missing::fingerprint(&app.session.catalog) != snapshot => { + (control::failure("Missing Photos: the photos changed while the files were checked, ask again"), false) + } Err(e) => (control::failure(e), false), Ok(None) => (control::failure("Missing Photos: the check was stopped"), false), Ok(Some(gone)) => (control::success(missing::missing_json(gone)), true), }; - let (theirs, others): (Vec<_>, Vec<_>) = std::mem::take(&mut app.missing.waiting).into_iter().partition(|(l, _)| *l == library); - app.missing.waiting = others; - // (a request whose caller gave up has no one to send to) - let received = theirs.into_iter().fold(false, |received, (_, waiting)| waiting.send(answer.clone()).is_ok() || received); + let received = app.missing.answer(library, &answer); if complete && !received { - app.missing.kept = Some((library, Instant::now(), answer)); + app.missing.kept = Some(Kept { library, snapshot, at: Instant::now(), answer }); } }; if let Err(e) = crate::tasks::spawn_tracked(app, LABEL, KIND, cancel, work, done) { - for (_, waiting) in std::mem::take(&mut app.missing.waiting) { - let _ = waiting.send(control::failure(&e)); - } + app.missing.fail(library, &e); } Outcome::Later } diff --git a/crates/ui-egui/src/tests/missing_sweep.rs b/crates/ui-egui/src/tests/missing_sweep.rs index eea6fc0a5..4cde807f0 100644 --- a/crates/ui-egui/src/tests/missing_sweep.rs +++ b/crates/ui-egui/src/tests/missing_sweep.rs @@ -171,3 +171,74 @@ fn a_check_that_panics_is_answered_with_an_error() { let rx = ask(&mut h); assert_eq!(answer(&mut h, &rx)["result"], json!([])); } + +/// Photos added while the files are being checked were not in that check: its answer is not given as +/// the library's current one, and is not kept for a later request either. +#[test] +fn a_check_of_an_earlier_catalog_is_not_answered_as_current() { + let (mut h, share) = app_on_a_slow_share(Duration::from_secs(3)); + let rx = ask(&mut h); + for _ in 0..3 { + h.step(); + } + let p = + Photo::new(PhotoId(PHOTOS + 1), Source::File { path: "/share/IMG_9999.jpg".into() }, "IMG_9999.jpg", "JPEG", 60, 40, "2026-01-01T00:00:00"); + h.app.session.catalog.apply(Op::AddPhoto { photo: Box::new(p) }).unwrap(); + share.open.store(true, Ordering::SeqCst); + let reply = answer(&mut h, &rx); + assert_eq!(reply["ok"], false, "{reply}"); + assert!(reply["error"].as_str().is_some_and(|e| e.contains("changed")), "{reply}"); + assert!(h.step_until(T, |h| h.app.tasks.is_empty())); + let calls = share.calls.load(Ordering::SeqCst); + let again = ask(&mut h); + let reply = answer(&mut h, &again); + assert_eq!(reply["ok"], true, "{reply}"); + assert!(share.calls.load(Ordering::SeqCst) > calls, "checked afresh, not given the stale answer"); +} + +/// An edit that leaves the photos' files alone (a rating) does not spoil the check. +#[test] +fn an_edit_that_leaves_the_files_alone_does_not_spoil_the_check() { + let (mut h, share) = app_on_a_slow_share(Duration::from_secs(3)); + let rx = ask(&mut h); + for _ in 0..3 { + h.step(); + } + h.app.session.catalog.apply(Op::SetRating { id: PhotoId(1), rating: 3 }).unwrap(); + share.open.store(true, Ordering::SeqCst); + assert_eq!(answer(&mut h, &rx)["ok"], true); +} + +/// A check that can't start for one library fails only the requests for that library. +#[test] +fn a_failed_start_fails_only_that_librarys_requests() { + let mut state = crate::missing_sweep::State::default(); + let (a_tx, a_rx) = std::sync::mpsc::channel(); + let (b_tx, b_rx) = std::sync::mpsc::channel(); + state.waiting.push((1, a_tx)); + state.waiting.push((2, b_tx)); + state.fail(2, "Missing Photos: could not start"); + assert!(b_rx.try_recv().is_ok_and(|v| v["ok"] == false)); + assert!(a_rx.try_recv().is_err(), "the other library's request keeps waiting"); + assert_eq!(state.waiting.len(), 1); +} + +/// An answer kept for a request that never came is not given once the photos have changed. +#[test] +fn a_kept_answer_is_not_given_after_the_photos_changed() { + let (mut h, share) = app_on_a_slow_share(Duration::from_secs(3)); + drop(ask(&mut h)); + for _ in 0..3 { + h.step(); + } + share.open.store(true, Ordering::SeqCst); + assert!(h.step_until(T, |h| h.app.tasks.is_empty())); + let p = + Photo::new(PhotoId(PHOTOS + 1), Source::File { path: "/share/IMG_0000x.jpg".into() }, "IMG_0000x.jpg", "JPEG", 60, 40, "2026-01-01T00:00:00"); + h.app.session.catalog.apply(Op::AddPhoto { photo: Box::new(p) }).unwrap(); + let calls = share.calls.load(Ordering::SeqCst); + let again = ask(&mut h); + let reply = answer(&mut h, &again); + assert_eq!(reply["result"].as_array().map(Vec::len), Some(11), "{reply}"); + assert!(share.calls.load(Ordering::SeqCst) > calls); +} diff --git a/docs/whats-new.md b/docs/whats-new.md index 100f1e741..86d1c190b 100644 --- a/docs/whats-new.md +++ b/docs/whats-new.md @@ -56,7 +56,9 @@ minutes. The files are now checked in the background: a "Missing Photos" row in the activity stack counts them and its ✕ stops the check, and the request is answered with the same list when the check ends. A second request meanwhile shares the running check. A caller that stops waiting (the control server gives a request 60 s) gets the answer of - that check when it asks again within ten minutes, instead of another walk of the share. + that check when it asks again within ten minutes, instead of another walk of the share. If photos are added, removed + or relinked while the files are being checked, that answer is not given as current: the request is told to ask + again. ### Choose where a Lightroom catalog goes - File ▸ Import Lightroom Catalog… now asks first: merge the catalog's photos, collections and edits into the