From 0a474cc06e98778f1f7cfeb7d36ab41d1bb66a18 Mon Sep 17 00:00:00 2001 From: Nathaniel Ledford Date: Thu, 8 Oct 2026 11:29:05 -0400 Subject: [PATCH] Permanent delete of several photos is one consistent op list; restored photos join album/selection Review of the onDeleted work found that purging photos one at a time built each op list against the unchanged catalog, so photos sharing an album or a stack made Empty Recently Deleted (and Fresh imports) fail with NoStack or leave dangling album entries. Catalog::delete_photos_permanently_ops builds one list for all of them; Empty, Delete Permanently and the Fresh purge use it. Photos an import restores now take part in album/albumName/selection, and undo labels say Restore. Co-Authored-By: Claude Sonnet 5.5 --- crates/catalog/src/lib.rs | 16 +++++++ crates/engine/src/cmd/library.rs | 19 ++++++--- crates/engine/src/import.rs | 14 +++++- crates/engine/src/tests_import.rs | 71 +++++++++++++++++++++++++++++++ 4 files changed, 111 insertions(+), 9 deletions(-) diff --git a/crates/catalog/src/lib.rs b/crates/catalog/src/lib.rs index 208a86d09..ba9849011 100644 --- a/crates/catalog/src/lib.rs +++ b/crates/catalog/src/lib.rs @@ -612,6 +612,22 @@ impl Catalog { Op::Batch { ops } } + /// [`Self::delete_permanently_ops`] for several photos at once. One op list built against the + /// catalog as it is now: albums lose all of them in one write each, and stacks are shortened + /// (or dissolved) once, however many of their photos go. + pub fn delete_photos_permanently_ops(&self, ids: &[PhotoId]) -> Op { + let gone: std::collections::HashSet = ids.iter().copied().collect(); + let mut ops: Vec = self + .albums + .values() + .filter(|a| a.photos.iter().any(|p| gone.contains(p))) + .map(|a| Op::SetAlbumPhotos { id: a.id, photos: a.photos.iter().copied().filter(|p| !gone.contains(p)).collect() }) + .collect(); + ops.extend(self.remove_from_stacks_ops(ids)); + ops.extend(gone.iter().map(|id| Op::RemovePhoto { id: *id })); + Op::Batch { ops } + } + // ---- persistence /// Full snapshot as JSON. diff --git a/crates/engine/src/cmd/library.rs b/crates/engine/src/cmd/library.rs index c3e6b8248..b71796df9 100644 --- a/crates/engine/src/cmd/library.rs +++ b/crates/engine/src/cmd/library.rs @@ -94,7 +94,9 @@ pub fn import_params(s: &Session, p: &Value) -> Result { /// `album`). pub fn import_batch_done(s: &mut Session, report: crate::import::ImportReport, mut album: Option, album_name: Option<&str>) -> Result { let mut report = serde_json::to_value(report).unwrap_or_default(); - let imported: Vec = report["imported"].as_array().map(|a| a.iter().filter_map(Value::as_u64).collect()).unwrap_or_default(); + // photos an import restored from Recently Deleted take part like new ones (album, selection) + let imported: Vec = + ["imported", "restored"].iter().flat_map(|k| report[*k].as_array().into_iter().flatten().filter_map(Value::as_u64)).collect(); if album.is_none() && let Some(name) = album_name && !imported.is_empty() @@ -579,16 +581,16 @@ pub fn specs() -> Vec { if ids.is_empty() { return Ok(json!({"deleted": 0})); } - let ops = ids.iter().map(|id| s.catalog.delete_permanently_ops(*id)).collect::>(); - s.commit("Empty Recently Deleted", Op::Batch { ops })?; + let op = s.catalog.delete_photos_permanently_ops(&ids); + s.commit("Empty Recently Deleted", op)?; s.selection = Selection::default(); Ok(json!({"deleted": ids.len()})) } ), cmd!("photo.deletePermanently", "Delete Permanently", ["Photo"], None, "{ids?}", has_selection, |s, p| { let t = s.targets(p); - let ops = t.iter().map(|id| s.catalog.delete_permanently_ops(*id)).collect::>(); - s.commit("Delete Permanently", Op::Batch { ops })?; + let op = s.catalog.delete_photos_permanently_ops(&t); + s.commit("Delete Permanently", op)?; s.selection = Selection::default(); Ok(json!({"deleted": t.len()})) }), @@ -992,9 +994,12 @@ pub fn specs() -> Vec { let report = crate::import::import_with(s, &req.paths, &req.opts)?; let report = import_batch_done(s, report, req.album, req.album_name.as_deref())?; // one undo step for the whole import - let imported: Vec = report["imported"].as_array().map(|a| a.iter().filter_map(Value::as_u64).collect()).unwrap_or_default(); + let ids_of = |k: &str| -> Vec { report[k].as_array().map(|a| a.iter().filter_map(Value::as_u64).collect()).unwrap_or_default() }; + let (imported, restored) = (ids_of("imported"), ids_of("restored")); let n = s.undo.len().saturating_sub(undo0); - s.merge_undo(n, &format!("Add {} Photo{}", imported.len(), if imported.len() == 1 { "" } else { "s" })); + let (verb, count) = if imported.is_empty() && !restored.is_empty() { ("Restore", restored.len()) } else { ("Add", imported.len()) }; + s.merge_undo(n, &format!("{verb} {count} Photo{}", if count == 1 { "" } else { "s" })); + let imported = if imported.is_empty() { restored } else { imported }; if let Some(f) = imported.first() { s.selection = Selection::single(PhotoId(*f)); } diff --git a/crates/engine/src/import.rs b/crates/engine/src/import.rs index 1bbbd64fc..5df949887 100644 --- a/crates/engine/src/import.rs +++ b/crates/engine/src/import.rs @@ -887,6 +887,7 @@ pub fn commit_prepared(s: &mut Session, opts: &ImportOptions, now: &str, prepare } } } + let mut purge: Vec = Vec::new(); let mut purged: std::collections::HashSet = Default::default(); for it in prepared.items { match it { @@ -923,7 +924,7 @@ pub fn commit_prepared(s: &mut Session, opts: &ImportOptions, now: &str, prepare } for old in info.content_hash.as_ref().and_then(|h| trash_hash.get(h)).into_iter().chain(trash_path.get(&stored)) { if purged.insert(*old) { - ops.push(s.catalog.delete_permanently_ops(*old)); + purge.push(*old); } } let sidecar = sidecar.map(|sc| sc.resolve_label(&s.catalog)).filter(|sc| *sc != crate::sidecar::SidecarData::default()); @@ -969,9 +970,18 @@ pub fn commit_prepared(s: &mut Session, opts: &ImportOptions, now: &str, prepare } } } + // (built against the catalog as it is: one op list for all of them, ahead of the additions) + if !purge.is_empty() { + ops.insert(0, s.catalog.delete_photos_permanently_ops(&purge)); + } let log0 = s.pending_log.len(); + let (verb, n) = if report.imported.is_empty() && !report.restored.is_empty() { + ("Restore", report.restored.len()) + } else { + ("Add", report.imported.len().max(1)) + }; if !ops.is_empty() - && let Err(e) = s.commit(&format!("Add {} Photo{}", ops.len(), if ops.len() == 1 { "" } else { "s" }), Op::Batch { ops }) + && let Err(e) = s.commit(&format!("{verb} {n} Photo{}", if n == 1 { "" } else { "s" }), Op::Batch { ops }) { // nothing was catalogued: take the moves back (their sources were never touched) placed.iter().for_each(crate::import_move::rollback); diff --git a/crates/engine/src/tests_import.rs b/crates/engine/src/tests_import.rs index 0853a7e9e..1c5edf9ed 100644 --- a/crates/engine/src/tests_import.rs +++ b/crates/engine/src/tests_import.rs @@ -931,3 +931,74 @@ fn empty_recently_deleted_removes_only_trashed_photos() { s.execute("edit.undo", &json!({})).unwrap(); let _ = std::fs::remove_dir_all(&src); } + +/// Two trashed photos that share an album and a two-photo stack: emptying (or permanently +/// deleting) them together must not fail or leave dangling references. +fn two_trashed_in_album_and_stack(tag: &str) -> (Session, std::path::PathBuf, [u64; 2], u64) { + let src = temp_dir(tag); + write_png(&src.join("a.png"), 1); + write_png(&src.join("b.png"), 2); + let mut s = Session::new().with_fs(); + let r = s.execute("library.import", &json!({"paths": [src.to_string_lossy()]})).unwrap(); + let ids = [r["imported"][0].as_u64().unwrap(), r["imported"][1].as_u64().unwrap()]; + let al = s.execute("album.create", &json!({"name": "Trip"})).unwrap()["id"].as_u64().unwrap(); + s.execute("album.addPhotos", &json!({"id": al, "ids": ids})).unwrap(); + s.execute("library.select", &json!({"ids": ids})).unwrap(); + s.execute("stack.group", &json!({"ids": ids})).unwrap(); + s.execute("photo.delete", &json!({"ids": ids})).unwrap(); + (s, src, ids, al) +} + +#[test] +fn emptying_trashed_photos_of_one_album_and_stack_leaves_nothing_dangling() { + let (mut s, src, ids, al) = two_trashed_in_album_and_stack("trash-empty-refs"); + let r = s.execute("library.emptyRecentlyDeleted", &json!({})).unwrap(); + assert_eq!(r["deleted"], 2, "{r}"); + assert_eq!(s.catalog.len(), 0); + assert!(s.catalog.album(lightcraft_catalog::AlbumId(al)).unwrap().photos.is_empty(), "album still lists a removed photo"); + assert!(ids.iter().all(|i| s.catalog.stack_of(lightcraft_catalog::PhotoId(*i)).is_none())); + s.execute("edit.undo", &json!({})).unwrap(); + assert_eq!(s.catalog.len(), 2, "one undo step brings both back"); + assert_eq!(s.catalog.album(lightcraft_catalog::AlbumId(al)).unwrap().photos.len(), 2); + assert!(s.catalog.stack_of(lightcraft_catalog::PhotoId(ids[0])).is_some()); + assert!(s.catalog.photos().all(|p| p.deleted), "back in Recently Deleted"); + let _ = std::fs::remove_dir_all(&src); +} + +#[test] +fn deleting_several_photos_permanently_leaves_nothing_dangling() { + let (mut s, src, ids, al) = two_trashed_in_album_and_stack("trash-perm-refs"); + s.execute("library.select", &json!({"ids": ids})).unwrap(); + s.execute("photo.deletePermanently", &json!({"ids": ids})).unwrap(); + assert!(s.catalog.album(lightcraft_catalog::AlbumId(al)).unwrap().photos.is_empty()); + let _ = std::fs::remove_dir_all(&src); +} + +#[test] +fn fresh_import_of_two_trashed_photos_in_one_album_and_stack_works() { + let (mut s, src, ids, al) = two_trashed_in_album_and_stack("trash-fresh-refs"); + let r = s.execute("library.import", &json!({"paths": [src.to_string_lossy()], "onDeleted": "fresh"})).unwrap(); + assert_eq!(ids_len(&r), 2, "{r}"); + assert!(s.catalog.album(lightcraft_catalog::AlbumId(al)).unwrap().photos.is_empty()); + assert!(ids.iter().all(|i| s.catalog.photo(lightcraft_catalog::PhotoId(*i)).is_none())); + let _ = std::fs::remove_dir_all(&src); +} + +fn ids_len(r: &Value) -> usize { + ids(r, "imported") +} + +#[test] +fn restore_import_applies_the_album_to_restored_photos() { + let (mut s, src, ids, _) = trashed_photo_ids("trash-restore-album"); + let r = s.execute("library.import", &json!({"paths": [src.to_string_lossy()], "onDeleted": "restore", "albumName": "Back"})).unwrap(); + assert_eq!(r["restored"], json!(ids), "{r}"); + let album = r["album"].as_u64().expect("album created for the restored photos"); + assert_eq!(s.catalog.album(lightcraft_catalog::AlbumId(album)).unwrap().photos.len(), 1); + let _ = std::fs::remove_dir_all(&src); +} + +fn trashed_photo_ids(tag: &str) -> (Session, std::path::PathBuf, [u64; 1], u64) { + let (s, src, id) = trashed_photo(tag); + (s, src, [id], 0) +}