diff --git a/crates/engine/src/sync.rs b/crates/engine/src/sync.rs index 0a00a59ab..b049fe70d 100644 --- a/crates/engine/src/sync.rs +++ b/crates/engine/src/sync.rs @@ -243,9 +243,18 @@ pub fn scan_with(input: SyncInput, progress: &SyncProgress) -> FolderChanges { changes } -/// Whether a photo is the same as far as synchronizing goes: its file, its place in the library -/// and what a sidecar is compared with. Face and pet regions found in the background meanwhile -/// don't make a scan stale. +/// Whether a photo is the same file in the same place in the library: what decides whether it +/// is missing or moved, and which files beside it are new. Its rating, keywords and edits are +/// the person's work, not the folder's state. +fn same_file(a: &Arc, b: &Arc) -> bool { + Arc::ptr_eq(a, b) + || (&a.source, a.deleted, a.local, a.copy_of, &a.content_hash, a.file_size) + == (&b.source, b.deleted, b.local, b.copy_of, &b.content_hash, b.file_size) +} + +/// Whether a photo is the same as far as reading its sidecar goes: its file, its place in the +/// library and what a sidecar is compared with. Face and pet regions found in the background +/// meanwhile don't make a scan stale. fn same_for_sync(a: &Arc, b: &Arc) -> bool { if Arc::ptr_eq(a, b) { return true; @@ -394,19 +403,31 @@ fn not_there(path: &str) -> EngineError { impl Session { /// The last scan ([`Session::folder_changes`]) if it is of folder `path` and still current: - /// the folder holds the same photos, none of them changed. What the person was shown is what - /// gets done. Used once. (Changes elsewhere in the library don't matter.) + /// the folder holds the same photos (virtual copies aside), each the same file in the same + /// place ([`same_file`]). + /// What the person was shown is what gets done. Used once. + /// + /// Work on the folder's photos since (ratings, labels, keywords, edits) leaves the scan + /// current: none of it changes which files are new, missing or moved, and a scan of a big + /// folder on a slow disk takes minutes in which the person is told to go on working. The one + /// thing such work does matter to is a sidecar the scan found news in: reading it would go + /// over that work, so for those photos nothing may have changed ([`same_for_sync`]). + /// (Changes elsewhere in the library don't matter.) pub fn take_folder_changes(&mut self, path: &str) -> Option { let c = self.folder_changes.take()?; if lightcraft_catalog::query::folder_key(&c.path) != lightcraft_catalog::query::folder_key(path) { return None; } let f = Filter { library_folder: Some(path.to_string()), ..Default::default() }; - let mut now: Vec = self.catalog.query(&f, &Sort::default()); - let mut then: Vec = c.seen.iter().map(|p| p.id).collect(); + // (virtual copies made or deleted meanwhile are not files: the folder holds what it held) + let mut now: Vec = + self.catalog.query(&f, &Sort::default()).into_iter().filter(|id| self.catalog.photo(*id).is_some_and(|p| p.copy_of.is_none())).collect(); + let mut then: Vec = c.seen.iter().filter(|p| p.copy_of.is_none()).map(|p| p.id).collect(); now.sort(); then.sort(); - let unchanged = now == then && c.seen.iter().all(|p| self.catalog.photo(p.id).is_some_and(|q| same_for_sync(p, q))); + let read: std::collections::HashSet = c.metadata.iter().map(|m| m.id).collect(); + let same = |p: &Arc, q: &Arc| if read.contains(&p.id.0) { same_for_sync(p, q) } else { same_file(p, q) }; + let unchanged = now == then && c.seen.iter().filter(|p| p.copy_of.is_none()).all(|p| self.catalog.photo(p.id).is_some_and(|q| same(p, q))); unchanged.then_some(c) } } diff --git a/crates/engine/src/tests_sync.rs b/crates/engine/src/tests_sync.rs index 39378b100..01e2aa39b 100644 --- a/crates/engine/src/tests_sync.rs +++ b/crates/engine/src/tests_sync.rs @@ -441,23 +441,77 @@ fn a_scan_stays_current_while_only_other_folders_change() { assert_eq!(r["imported"], 1, "{r}"); } +/// Work on the folder's photos while it is scanned (a rating, keywords, an edit) changes nothing +/// about what the scan found: a scan of a big folder on a slow disk takes minutes, and the +/// dialog says to go on working. Synchronize does what it showed. #[test] -fn a_change_to_the_folders_photos_makes_the_scan_stale() { +fn working_on_the_folders_photos_meanwhile_leaves_the_scan_current() { + let dir = Scratch::new("worked-on"); + let mut s = library(&dir); + write_png(&dir.path("trip/c.png"), 3); + scan(&mut s, &dir.path("trip")); + let a = s.catalog.photos().find(|p| p.file_name == "a.png").unwrap().id; + s.selection = crate::Selection::single(a); + s.execute("photo.rate", &json!({"ids": [a.0], "rating": 2})).unwrap(); + s.execute("photo.setMeta", &json!({"ids": [a.0], "addKeywords": ["Trips|Alps", "snow"], "title": "A"})).unwrap(); + s.execute("photo.label", &json!({"ids": [a.0], "label": "red"})).unwrap(); + s.execute("photo.pick", &json!({"ids": [a.0]})).unwrap(); + s.execute("develop.set", &json!({"control": "light.exposure", "value": 0.5})).unwrap(); + // into several albums, a virtual copy made of it, and one of its edits undone + s.execute("photo.virtualCopy", &json!({"ids": [a.0]})).unwrap(); + s.selection = crate::Selection::single(a); + for name in ["One", "Two"] { + let album = s.execute("album.create", &json!({"name": name})).unwrap()["id"].as_u64().unwrap(); + s.execute("album.addPhotos", &json!({"id": album, "ids": [a.0]})).unwrap(); + } + s.execute("edit.undo", &json!({})).unwrap(); + s.execute("edit.redo", &json!({})).unwrap(); + let r = s.execute("folder.synchronize", &json!({"path": dir.path("trip"), "scanned": true})).unwrap(); + assert_eq!(r["imported"], 1, "what the scan showed is done: {r}"); + assert_eq!(s.catalog.photo(a).unwrap().rating, 2, "and the work stays"); +} + +/// What the scan found is no longer true once the folder holds other photos, or one of them is +/// another file now: then it is refused, never redone on the spot. +#[test] +fn a_change_to_which_photos_the_folder_holds_makes_the_scan_stale() { let dir = Scratch::new("stale-folder"); let mut s = library(&dir); write_png(&dir.path("trip/c.png"), 3); scan(&mut s, &dir.path("trip")); let a = s.catalog.photos().find(|p| p.file_name == "a.png").unwrap().id; - s.execute("photo.rate", &json!({"ids": [a.0], "rating": 2})).unwrap(); - // the dialog asks for exactly what it showed: refused, never redone on the spot + s.selection = crate::Selection::single(a); + // one of its photos goes to Recently Deleted + s.execute("photo.delete", &json!({"ids": [a.0]})).unwrap(); let e = s.execute("folder.synchronize", &json!({"path": dir.path("trip"), "scanned": true})).unwrap_err().to_string(); assert!(e.contains("scan again"), "{e}"); - assert_eq!(s.catalog.photos().filter(|p| p.in_library()).count(), 2, "nothing was done"); + assert_eq!(s.catalog.photos().filter(|p| p.in_library()).count(), 1, "nothing was done"); // an agent that just asks to synchronize gets a fresh scan let r = s.execute("folder.synchronize", &json!({"path": dir.path("trip")})).unwrap(); assert_eq!(r["imported"], 1, "{r}"); } +/// A photo whose sidecar the scan found news in, changed here since: what the sidecar says would +/// go over that work, which the person was not shown. That scan is refused. +#[test] +fn a_change_to_a_photo_whose_sidecar_has_news_makes_the_scan_stale() { + let dir = Scratch::new("stale-sidecar"); + let mut s = library(&dir); + write_sidecar(&dir.path("trip/a.png"), 5, Duration::ZERO); + let found = scan(&mut s, &dir.path("trip")); + assert_eq!(found["metadata"].as_array().map(Vec::len), Some(1), "{found}"); + // work on another photo of the folder does not matter… + let (a, b) = (s.catalog.photos().find(|p| p.file_name == "a.png").unwrap().id, s.catalog.photos().find(|p| p.file_name == "b.png").unwrap().id); + s.selection = crate::Selection::single(b); + s.execute("photo.rate", &json!({"ids": [b.0], "rating": 1})).unwrap(); + assert!(s.folder_changes.is_some()); + // …work on the photo whose sidecar is to be read does + s.execute("photo.rate", &json!({"ids": [a.0], "rating": 2})).unwrap(); + let e = s.execute("folder.synchronize", &json!({"path": dir.path("trip"), "scanned": true, "readMetadata": true})).unwrap_err().to_string(); + assert!(e.contains("scan again"), "{e}"); + assert_eq!(s.catalog.photo(a).unwrap().rating, 2, "nothing was read over it"); +} + #[test] fn the_progress_counts_the_photos_checked_too() { use std::sync::atomic::Ordering::Relaxed; diff --git a/crates/ui-egui/src/tests_sync.rs b/crates/ui-egui/src/tests_sync.rs index 1b8626d19..42e2ba6d5 100644 --- a/crates/ui-egui/src/tests_sync.rs +++ b/crates/ui-egui/src/tests_sync.rs @@ -195,19 +195,42 @@ fn a_scan_gone_stale_in_the_dialog_is_made_again_in_the_background() { let mut h = changed_folder(&dir); open_and_scan(&mut h, &dir.path("trip")); tick_remove_missing(&mut h); - // a photo of the folder changes while the dialog is open - let a = h.app.session.catalog.photos().find(|p| p.file_name == "a.png").unwrap().id; - let r = h.request("engine.execute", json!({"command": "photo.rate", "params": {"ids": [a.0], "rating": 3}}), T); + // the folder gets another photo some other way while the dialog is open + write_png(&dir.path("trip/d.png"), 4); + let r = h.request("engine.execute", json!({"command": "library.import", "params": {"paths": [dir.path("trip/d.png")]}}), T); assert_eq!(r["ok"], true, "{r}"); let r = h.request("ui.dialog.confirm", json!({}), T); assert_eq!(r["ok"], false, "not done on a stale scan: {r}"); - assert_eq!(in_library(&h), vec!["a.png", "b.png"]); + assert_eq!(in_library(&h), vec!["a.png", "b.png", "d.png"]); assert!(matches!(h.app.ui.dialog, Some(Dialog::SynchronizeFolder { .. })), "the dialog stays open"); let rescanned = h.step_until(T, |h| matches!(&h.app.ui.dialog, Some(Dialog::SynchronizeFolder { counts: Some(_), .. }))); assert!(rescanned, "and scans again"); assert!(matches!(h.app.ui.dialog, Some(Dialog::SynchronizeFolder { remove_missing: true, .. })), "keeping what was ticked"); synchronize(&mut h); - assert_eq!(in_library(&h), vec!["a.png", "c.png"], "c came in, the missing b went"); + assert_eq!(in_library(&h), vec!["a.png", "c.png", "d.png"], "c came in, the missing b went"); +} + +/// Tagging and rating the folder's photos while the dialog is open (the scan of a big folder +/// takes minutes, and the dialog says to go on working) does not send Synchronize back to +/// scanning: it does what the dialog shows. +#[test] +fn working_on_the_folders_photos_while_the_dialog_is_open_does_not_restart_the_scan() { + let dir = Scratch::new("worked-on"); + let mut h = changed_folder(&dir); + open_and_scan(&mut h, &dir.path("trip")); + let a = h.app.session.catalog.photos().find(|p| p.file_name == "a.png").unwrap().id; + for (command, params) in [ + ("library.select", json!({"ids": [a.0]})), + ("photo.rate", json!({"ids": [a.0], "rating": 3})), + ("photo.setMeta", json!({"ids": [a.0], "addKeywords": ["Places|Italy|Rome", "Transport|Ferry"]})), + ] { + let r = h.request("engine.execute", json!({"command": command, "params": params}), T); + assert_eq!(r["ok"], true, "{command}: {r}"); + } + synchronize(&mut h); + assert_eq!(in_library(&h), vec!["a.png", "b.png", "c.png"], "the new photo came in"); + assert!(h.app.sync.is_none(), "no second scan"); + assert_eq!(h.app.session.catalog.photo(a).unwrap().meta.keywords.len(), 2, "and the keywords stay"); } #[test] @@ -483,10 +506,10 @@ fn a_result_looked_at_after_the_folder_changed_is_scanned_again_not_acted_on() { scan_in_background(&mut h, &dir); gate.store(true, std::sync::atomic::Ordering::Relaxed); assert!(h.step_until(T, |h| h.app.sync_ready.is_some())); - // the person went on working in the folder before looking + // one of the folder's photos left the library before the person looked let id = h.app.session.catalog.photos().find(|p| p.file_name == "a.png").map(|p| p.id.0).unwrap(); h.app.run("library.select", json!({"ids": [id]})).unwrap(); - h.app.run("photo.rate", json!({"rating": 3})).unwrap(); + h.app.run("photo.delete", json!({})).unwrap(); crate::sync::review(&mut h.app); let dlg = h.app.ui.dialog.take().expect("the dialog is open"); let before = in_library(&h); @@ -496,6 +519,32 @@ fn a_result_looked_at_after_the_folder_changed_is_scanned_again_not_acted_on() { assert!(h.app.sync.is_some(), "it is scanned again"); } +/// The person went on working in the folder while it was scanned in the background (ratings, +/// keywords, a colour label, albums): what the scan found still holds, and Synchronize does it. +#[test] +fn a_result_looked_at_after_work_on_the_folders_photos_is_acted_on() { + let dir = Scratch::new("background-worked-on"); + let mut h = changed_folder(&dir); + let gate = gated(&mut h); + scan_in_background(&mut h, &dir); + gate.store(true, std::sync::atomic::Ordering::Relaxed); + assert!(h.step_until(T, |h| h.app.sync_ready.is_some())); + let id = h.app.session.catalog.photos().find(|p| p.file_name == "a.png").map(|p| p.id.0).unwrap(); + h.app.run("library.select", json!({"ids": [id]})).unwrap(); + h.app.run("photo.rate", json!({"rating": 3})).unwrap(); + h.app.run("photo.label", json!({"label": "red"})).unwrap(); + h.app.run("photo.pick", json!({})).unwrap(); + h.app.run("photo.setMeta", json!({"addKeywords": ["Places|Italy|Rome", "Transport|Ferry"], "title": "A"})).unwrap(); + for name in ["One", "Two"] { + let album = h.app.run("album.create", json!({"name": name})).unwrap()["id"].as_u64().unwrap(); + h.app.run("album.addPhotos", json!({"id": album})).unwrap(); + } + crate::sync::review(&mut h.app); + synchronize(&mut h); + assert_eq!(in_library(&h), vec!["a.png", "b.png", "c.png"], "the new photo came in"); + assert!(h.app.sync.is_none(), "and nothing was scanned again"); +} + #[test] fn another_folder_is_not_scanned_over_a_background_scan_or_its_notice() { let dir = Scratch::new("background-other"); diff --git a/docs/whats-new.md b/docs/whats-new.md index 65ecd4055..c4d4bf3a0 100644 --- a/docs/whats-new.md +++ b/docs/whats-new.md @@ -8,6 +8,10 @@ go on working. When it is done nothing pops up: a notice in the activity stack says how many changes were found. Click it to open the dialog with the result, or ✕ to let go of it. A scan that found nothing to do only says so. Cancel and the row's ✕ stop the scan as before. One folder is scanned at a time. +- Working on the folder's photos while it is scanned (ratings, flags, colour labels, keywords, edits, albums, + virtual copies) no longer sends Synchronize back to scanning. What the scan found still holds, and Synchronize + does it. It scans again only when the folder holds other photos than it did, one of them is another file now, or + a photo whose sidecar is about to be read was changed here since. ### JPEG XL export - Export as JPEG XL (issue #247): Format ▸ JPEG XL in the Export dialog, `format: "jxl"` for agents and the CLI