diff --git a/crates/engine/src/cmd/missing.rs b/crates/engine/src/cmd/missing.rs index 3c1bd7799..e5a17ab5d 100644 --- a/crates/engine/src/cmd/missing.rs +++ b/crates/engine/src/cmd/missing.rs @@ -52,18 +52,35 @@ 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 { +fn fingerprint_of<'a>(entries: impl Iterator) -> u64 { use std::hash::{DefaultHasher, Hash, Hasher}; - cat.photos().filter_map(|p| checked_path(p).map(|f| (p.id, f))).fold(0u64, |sum, entry| { + entries.fold(0u64, |sum, entry| { let mut h = DefaultHasher::new(); entry.hash(&mut h); sum.wrapping_add(h.finish()) }) } +/// What [`checked`] lists, as one number (the photos in scope and the file each one points at, +/// whatever their order), without building the list. A caller that took `checked` and checked its +/// files on another thread compares it with [`fingerprint_of_checked`] to know whether the photos +/// changed meanwhile. +pub fn fingerprint(cat: &Catalog) -> u64 { + fingerprint_of(cat.photos().filter_map(|p| checked_path(p).map(|f| (p.id, f)))) +} + +/// [`fingerprint`] of the list [`checked`] gave. +pub fn fingerprint_of_checked(files: &[(PhotoId, String)]) -> u64 { + fingerprint_of(files.iter().map(|(id, f)| (*id, f.as_str()))) +} + +/// What of `gone` (a sweep's answer, of the catalog as it was) still holds in `cat` as it is now: the +/// photos still in scope that still point at the file that was not found. A photo removed, moved to +/// Recently Deleted or relinked since is not reported as missing; one added since was not checked. +pub fn still_missing(cat: &Catalog, gone: Vec<(PhotoId, String)>) -> Vec<(PhotoId, String)> { + gone.into_iter().filter(|(id, file)| cat.photo(*id).and_then(|p| checked_path(p)) == Some(file.as_str())).collect() +} + /// How many files [`sweep`] checks between two reports of its progress. const PROGRESS_EVERY: usize = 64; diff --git a/crates/ui-egui/src/missing_sweep.rs b/crates/ui-egui/src/missing_sweep.rs index 6ac45d1e2..1afd0a280 100644 --- a/crates/ui-egui/src/missing_sweep.rs +++ b/crates/ui-egui/src/missing_sweep.rs @@ -84,7 +84,7 @@ pub fn ask(app: &mut LightcraftApp, reply: Sender) -> Outcome { return Outcome::Later; } let files = missing::checked(&app.session.catalog); - let snapshot = missing::fingerprint(&app.session.catalog); + let snapshot = missing::fingerprint_of_checked(&files); let total = files.len() as u64; let exists = app.session.media.availability.probe(); let cancel = Arc::new(AtomicBool::new(false)); @@ -101,13 +101,12 @@ 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), + Ok(Some(gone)) if missing::fingerprint(&app.session.catalog) == snapshot => (control::success(missing::missing_json(gone)), true), + // (photos changed while the files were checked, as an import does: the answer is what of the + // check still holds, and is not kept, for the next request must see the photos as they are then) + Ok(Some(gone)) => (control::success(missing::missing_json(missing::still_missing(&app.session.catalog, gone))), false), }; let received = app.missing.answer(library, &answer); if complete && !received { diff --git a/crates/ui-egui/src/tests/missing_sweep.rs b/crates/ui-egui/src/tests/missing_sweep.rs index 4cde807f0..62b15a9d1 100644 --- a/crates/ui-egui/src/tests/missing_sweep.rs +++ b/crates/ui-egui/src/tests/missing_sweep.rs @@ -172,28 +172,69 @@ fn a_check_that_panics_is_answered_with_an_error() { 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. +fn new_photo(id: u64, path: &str) -> Box { + Box::new(Photo::new(PhotoId(id), Source::File { path: path.into() }, "x.jpg", "JPEG", 60, 40, "2026-01-01T00:00:00")) +} + +/// Photos change while the files are being checked (auto-import adds some every few seconds, a share check +/// takes minutes): the answer is the check's, kept to the photos that are still where it looked, so asking +/// again never has to be the answer. A photo that was missing and has been removed, moved to Recently +/// Deleted or relinked since is not reported; one added since was not checked and is not either. #[test] -fn a_check_of_an_earlier_catalog_is_not_answered_as_current() { +fn a_check_over_changing_photos_answers_what_still_holds() { 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(); + let cat = &mut h.app.session.catalog; + cat.apply(Op::RemovePhoto { id: PhotoId(1) }).unwrap(); + cat.apply(Op::SetDeleted { id: PhotoId(2), deleted: true }).unwrap(); + let relinked = + Op::Relink { id: PhotoId(3), file_name: "y.jpg".into(), source: Source::File { path: "/share/IMG_9998.jpg".into() }, format: None }; + cat.apply(relinked).unwrap(); + cat.apply(Op::AddPhoto { photo: new_photo(PHOTOS + 1, "/share/IMG_0000x.jpg") }).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_eq!(reply["ok"], true, "{reply}"); + let ids: Vec = reply["result"].as_array().unwrap().iter().map(|r| r["id"].as_u64().unwrap()).collect(); + assert_eq!(ids, (4..=10).collect::>(), "{reply}"); +} + +/// A stop is a stop, whatever the photos did meanwhile. +#[test] +fn a_stopped_check_says_stopped_even_if_the_photos_changed() { + 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::AddPhoto { photo: new_photo(PHOTOS + 1, "/share/IMG_9999.jpg") }).unwrap(); + let id = missing_rows(&h)[0].id; + let r = h.request("engine.execute", json!({"command": "activity.cancel", "params": {"id": id}}), T); + assert_eq!(r["ok"], true, "{r}"); + share.open.store(true, Ordering::SeqCst); + let reply = answer(&mut h, &rx); + assert!(reply["error"].as_str().is_some_and(|e| e.contains("stopped")), "{reply}"); +} + +/// A check nobody waited for, over photos that changed while it ran, is not kept: the next request +/// checks afresh (and then sees the photo that came meanwhile). +#[test] +fn a_check_over_changed_photos_is_not_kept_for_a_later_request() { + let (mut h, share) = app_on_a_slow_share(Duration::from_secs(3)); + drop(ask(&mut h)); + for _ in 0..3 { + h.step(); + } + h.app.session.catalog.apply(Op::AddPhoto { photo: new_photo(PHOTOS + 1, "/share/IMG_0000x.jpg") }).unwrap(); + share.open.store(true, Ordering::SeqCst); 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"); + assert_eq!(reply["result"].as_array().map(Vec::len), Some(11), "{reply}"); + assert!(share.calls.load(Ordering::SeqCst) > calls, "checked afresh"); } /// An edit that leaves the photos' files alone (a rating) does not spoil the check. diff --git a/docs/whats-new.md b/docs/whats-new.md index 86d1c190b..279a8585b 100644 --- a/docs/whats-new.md +++ b/docs/whats-new.md @@ -56,9 +56,10 @@ 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. 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. + that check when it asks again within ten minutes, instead of another walk of the share, as long as the photos have + not changed since; otherwise the files are checked again. If photos change while the files are being checked (an + import adds some, others are removed, moved to Recently Deleted or relinked), the answer leaves out the photos that + no longer apply, a photo added meanwhile is not part of that check, and the answer is not kept for a later request. ### Choose where a Lightroom catalog goes - File ▸ Import Lightroom Catalog… now asks first: merge the catalog's photos, collections and edits into the