From 03cb96f86a5dc359082142f13d0934de2c72645b Mon Sep 17 00:00:00 2001 From: Elliot Graham Date: Fri, 9 Oct 2026 13:33:22 +0100 Subject: [PATCH] LR-LIB-FOLDER: delete populated album folders with undo (#501) --- crates/engine/src/cmd/library.rs | 45 ++++++++++++++++++++++++----- crates/engine/src/tests.rs | 5 ++-- crates/engine/src/tests_organize.rs | 41 ++++++++++++++++++++++++++ crates/ui-egui/src/panels/left.rs | 11 +++++-- crates/ui-egui/src/tests_panels.rs | 20 +++++++++++++ docs/parity.md | 2 +- 6 files changed, 112 insertions(+), 12 deletions(-) diff --git a/crates/engine/src/cmd/library.rs b/crates/engine/src/cmd/library.rs index d900eacff..70ddfc421 100644 --- a/crates/engine/src/cmd/library.rs +++ b/crates/engine/src/cmd/library.rs @@ -874,14 +874,45 @@ pub fn specs() -> Vec { s.commit("Rename Album", Op::RenameAlbum { id, name })?; ok() }), - cmd!("album.delete", "Delete Album", [], None, "{id}", always, |s, p| { - let id = album_param(p, "id", "album.delete")?; - s.commit("Delete Album", Op::RemoveAlbum { id })?; - if s.source == LibrarySource::Album(id) { - s.source = LibrarySource::All; + cmd!( + "album.delete", + "Delete Album", + [], + None, + "{id} — deletes the album or folder and its descendants, keeping photos in the library; one undo step", + always, + |s, p| { + let id = album_param(p, "id", "album.delete")?; + let folder = s.catalog.album(id).ok_or_else(|| bad("album.delete", "no such album"))?.folder; + let mut children = std::collections::BTreeMap::>::new(); + for album in s.catalog.albums() { + if let Some(parent) = album.parent { + children.entry(parent).or_default().push(album.id); + } + } + // Iterative traversal: even a malformed cycle is rejected before any change. + // Reverse parent-first order removes children first; the batch inverse restores + // parents first, including memberships, smart rules, covers and manual ordering. + let mut pending = vec![id]; + let mut order = Vec::new(); + let mut removed = std::collections::BTreeSet::new(); + while let Some(next) = pending.pop() { + if !removed.insert(next) { + return Err(bad("album.delete", "cycle in album folders")); + } + order.push(next); + if let Some(nested) = children.remove(&next) { + pending.extend(nested); + } + } + let ops = order.into_iter().rev().map(|id| Op::RemoveAlbum { id }).collect(); + s.commit(if folder { "Delete Folder" } else { "Delete Album" }, Op::Batch { ops })?; + if matches!(s.source, LibrarySource::Album(current) if removed.contains(¤t)) { + s.source = LibrarySource::All; + } + ok() } - ok() - }), + ), cmd!("album.move", "Move Album", [], None, "{id, parent?: folderId|null}", always, |s, p| { let id = album_param(p, "id", "album.move")?; let parent = p.get("parent").and_then(Value::as_u64).map(AlbumId); diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 9806a81ee..ad5739cb4 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -113,9 +113,10 @@ fn albums_crud() { s.execute("library.selectAll", &json!({})).unwrap(); s.execute("album.addPhotos", &json!({"id": a})).unwrap(); assert_eq!(s.catalog.album(lightcraft_catalog::AlbumId(a)).unwrap().photos.len(), 24); - assert!(s.execute("album.delete", &json!({"id": f})).is_err(), "non-empty folder"); - s.execute("album.delete", &json!({"id": a})).unwrap(); s.execute("album.delete", &json!({"id": f})).unwrap(); + assert!(s.catalog.album(lightcraft_catalog::AlbumId(a)).is_none()); + assert!(s.catalog.album(lightcraft_catalog::AlbumId(f)).is_none()); + assert_eq!(s.catalog.photos().count(), 24, "album deletion keeps the photos"); } /// Given an album and a folder, when something is created inside each, then only the folder diff --git a/crates/engine/src/tests_organize.rs b/crates/engine/src/tests_organize.rs index 7fb24806b..fc00ce7ea 100644 --- a/crates/engine/src/tests_organize.rs +++ b/crates/engine/src/tests_organize.rs @@ -256,3 +256,44 @@ fn random_sort_persists_with_the_view() { assert_eq!(s2.sort.seed, 123_456_789); let _ = std::fs::remove_dir_all(&dir); } + +/// Issue #501: deleting a populated folder is one reversible, durable catalog edit. +#[test] +fn deleting_an_album_folder_preserves_photos_and_restores_its_subtree() { + use lightcraft_catalog::AlbumId; + let dir = temp_dir("delete-folder"); + let mut s = open(&dir); + let make = |s: &mut Session, p| s.execute("album.create", &p).unwrap()["id"].as_u64().unwrap(); + let outer = make(&mut s, json!({"name": "Trips", "folder": true})); + let nested = make(&mut s, json!({"name": "Europe", "folder": true, "parent": outer})); + let album = make(&mut s, json!({"name": "Best", "parent": nested, "addSelected": true})); + let sibling = make(&mut s, json!({"name": "Kept", "addSelected": true})); + let smart = s.execute("album.createSmart", &json!({"name": "Rated", "parent": outer, "rules": {"rating": 4}})).unwrap()["id"].as_u64().unwrap(); + s.execute("album.reorder", &json!({"id": album, "parent": nested})).unwrap(); + s.execute("library.source", &json!({"kind": "album", "id": album})).unwrap(); + let before = s.catalog.to_snapshot(); + let photos = serde_json::to_value(s.catalog.photos().collect::>()).unwrap(); + let undo_before = s.undo.len(); + s.execute("album.delete", &json!({"id": outer})).unwrap(); + for id in [outer, nested, album, smart] { + assert!(s.catalog.album(AlbumId(id)).is_none()); + } + assert!(s.catalog.album(AlbumId(sibling)).is_some()); + assert_eq!(serde_json::to_value(s.catalog.photos().collect::>()).unwrap(), photos); + assert_eq!(s.source, LibrarySource::All, "a deleted descendant cannot remain the source"); + assert_eq!(s.undo.len(), undo_before + 1); + let deleted = s.catalog.to_snapshot(); + s.execute("edit.undo", &json!({})).unwrap(); + assert_eq!(s.catalog.to_snapshot(), before, "all album fields and memberships restored"); + s.execute("edit.redo", &json!({})).unwrap(); + assert_eq!(s.catalog.to_snapshot(), deleted); + let undo_after = s.undo.len(); + assert!(s.execute("album.delete", &json!({"id": outer})).is_err()); + assert_eq!(s.catalog.to_snapshot(), deleted, "missing id leaves the catalog unchanged"); + assert_eq!(s.undo.len(), undo_after); + drop(s); + let reopened = open(&dir); + assert_eq!(reopened.catalog.to_snapshot(), deleted, "journal replay preserves the deletion"); + drop(reopened); + let _ = std::fs::remove_dir_all(dir); +} diff --git a/crates/ui-egui/src/panels/left.rs b/crates/ui-egui/src/panels/left.rs index 8863e5d18..13a0042f0 100644 --- a/crates/ui-egui/src/panels/left.rs +++ b/crates/ui-egui/src/panels/left.rs @@ -1087,8 +1087,15 @@ fn folder_menu(app: &mut LightcraftApp, resp: &egui::Response, a: &Album) { if ui.button(crate::i18n::tr("Rename…")).clicked() { app.ui.dialog = Some(crate::state::Dialog::RenameAlbum { id: a.id.0, name: a.name.clone() }); } - if ui.button(crate::i18n::tr("Delete")).clicked() { - let _ = app.run("album.delete", json!({"id": a.id.0})); + let delete = ui.button(crate::i18n::tr("Delete")); + register(ui.ctx(), format!("albumDelete:{}", a.id.0), delete.rect); + let delete = + delete.on_hover_text(crate::i18n::tr("Photos stay in All Photos. Deleting a folder also deletes its albums. You can undo this.")); + if delete.clicked() { + if let Err(e) = app.run("album.delete", json!({"id": a.id.0})) { + app.toast(ui.ctx(), e); + } + ui.close(); } }); } diff --git a/crates/ui-egui/src/tests_panels.rs b/crates/ui-egui/src/tests_panels.rs index 66eb360e1..70ac3a3a2 100644 --- a/crates/ui-egui/src/tests_panels.rs +++ b/crates/ui-egui/src/tests_panels.rs @@ -1096,3 +1096,23 @@ fn date_and_keyword_rows_show_their_photos_from_any_source() { h.step(); assert_eq!(h.app.session.filter, lightcraft_catalog::Filter::default(), "{key}"); } + +/// Issue #501: use the actual context-menu button on the populated demo folder. +#[test] +fn deleting_a_populated_album_folder_from_its_menu_is_undoable() { + let mut h = demo([1400.0, 900.0], json!({"view": "photoGrid", "leftPanel": true})); + let folder = h.app.session.catalog.albums().find(|a| a.folder && a.name == "Travel 2026").unwrap().id; + assert!(!h.app.session.catalog.album_children(Some(folder)).is_empty()); + let before = h.app.session.catalog.to_snapshot(); + let photo_count = h.app.session.catalog.photos().count(); + right_click(&mut h, &format!("source:folder:{}", folder.0)); + click(&mut h, &format!("albumDelete:{}", folder.0)); + assert!(h.app.session.catalog.album(folder).is_none()); + assert_eq!(h.app.session.catalog.photos().count(), photo_count); + assert!(!popup_open(&h)); + h.app.run("edit.undo", json!({})).unwrap(); + h.step(); + h.step(); + assert_eq!(h.app.session.catalog.to_snapshot(), before); + assert!(has(&h, &format!("source:folder:{}", folder.0))); +} diff --git a/docs/parity.md b/docs/parity.md index c06dad1b0..1c9e67a40 100644 --- a/docs/parity.md +++ b/docs/parity.md @@ -156,7 +156,7 @@ under the UI command that wraps it (a no-op error); the UI command now wins. | LR-LIB-RECENT-ADDED | Recently added | P1 | ✅ | `cmd:library.source` (`recentlyAdded`), `crates/engine/src/view.rs` (`RECENT_DAYS`) | photos imported in the 30 days up to the latest import, newest import first, grouped by import day | | LR-LIB-BYDATE | Browse by date | P1 | ✅ | `cmd:library.filter` (`date`), `crates/ui-egui/src/panels/left.rs` (`date_row`), `crates/catalog/src/query.rs` (`date_groups`) | year → month → day tree with library-wide counts; a row filters the current source by that date. Active filters show as removable chips under the grid header (`crates/ui-egui/src/panels/chips.rs`, with “+N more” and Clear all), the header counts “N of M photos” (matching of the source's total), a badge on the top-bar filter icon counts them, and an empty result says no photos match the active filters | | LR-LIB-ALBUM | Albums | P0 | ✅ | `cmd:album.create`, `cmd:album.rename`, `cmd:album.delete`, `cmd:album.addPhotos`, `cmd:album.removePhotos`, `cmd:dialog.newAlbum` | photos dragged from the grid onto an album row are added to it (LR-BEHAV-DRAGDROP) | -| LR-LIB-FOLDER | Folders of albums | P0 | ✅ | `cmd:album.create` (`folder`, `parent`), `cmd:album.move`, `cmd:album.reorder`, `cmd:album.sort`, `cmd:dialog.newAlbum` / `cmd:dialog.newFolder` / `cmd:dialog.smartAlbum` / `cmd:dialog.newSmartAlbum` (`parent`), `crates/ui-egui/src/panels/left.rs` (`albums_tree`, `folder_menu`, `disclosure_triangle`), `crates/ui-egui/src/tests_panels.rs` | folders nest and fold with the same disclosure triangle as Local / By Date / Folders / Keywords (`albumToggle:`; the row folds them too; open by default); right-click a folder ▸ New ▸ Create Album… / Create Smart Album… / Create Smart Album from Filter… / Create Folder… creates inside it (the `dialog.new*` commands take `parent`), opening the folders down to the new row; creating is only allowed inside folders; moving: drag an album or folder row onto a folder (outlined when allowed; a closed folder opens after 0.6 s under the pointer; the "Albums" header takes an item that sits in a folder back to the top level while it is dragged), the Move to menu or `album.move` — one undo step each; not onto a plain album, itself or something inside it; drop on the top / bottom half of an album or the edge of a folder to place it there by hand (`cmd:album.reorder`, a line shows the place; ordered per folder, folders stay first, new albums go after the ordered ones; right-click ▸ Sort Contents A–Z or ＋ ▸ Sort Albums A–Z = `cmd:album.sort`) | +| LR-LIB-FOLDER | Folders of albums | P0 | ✅ | `cmd:album.create` (`folder`, `parent`), `cmd:album.move`, `cmd:album.reorder`, `cmd:album.sort`, `cmd:dialog.newAlbum` / `cmd:dialog.newFolder` / `cmd:dialog.smartAlbum` / `cmd:dialog.newSmartAlbum` (`parent`), `crates/ui-egui/src/panels/left.rs` (`albums_tree`, `folder_menu`, `disclosure_triangle`), `crates/ui-egui/src/tests_panels.rs` | Delete removes a folder and its nested albums as one undoable edit, keeps photos in the library and returns a deleted album view to All Photos (issue #501); folders nest and fold with the same disclosure triangle as Local / By Date / Folders / Keywords (`albumToggle:`; the row folds them too; open by default); right-click a folder ▸ New ▸ Create Album… / Create Smart Album… / Create Smart Album from Filter… / Create Folder… creates inside it (the `dialog.new*` commands take `parent`), opening the folders down to the new row; creating is only allowed inside folders; moving: drag an album or folder row onto a folder (outlined when allowed; a closed folder opens after 0.6 s under the pointer; the "Albums" header takes an item that sits in a folder back to the top level while it is dragged), the Move to menu or `album.move` — one undo step each; not onto a plain album, itself or something inside it; drop on the top / bottom half of an album or the edge of a folder to place it there by hand (`cmd:album.reorder`, a line shows the place; ordered per folder, folders stay first, new albums go after the ordered ones; right-click ▸ Sort Contents A–Z or ＋ ▸ Sort Albums A–Z = `cmd:album.sort`) | | LR-LIB-SMARTALBUM | Smart albums | P1 | ✅ | `cmd:album.createSmart`, `cmd:album.setRules`, `crates/catalog/src/query.rs`, `crates/ui-egui/src/panels/filterbar.rs`, `crates/ui-egui/src/lib.rs` (`album_counts`) | saved filters (rating/flag/label/kind/edited/keyword/camera/lens/date range/text/album), live; match-all only (no any/none rule groups, no rule editor dialog — rules come from the filter bar or `album.setRules`); sidebar counts are cached per catalog revision (and per minute while an “in the last…” rule is used), counted without building id lists | | LR-LIB-SHARED-ALBUM | Shared albums | P2 | ⬜ | | needs a sharing service | | LR-LIB-OFFLINE | Keep album offline | P2 | 🚫 | | not applicable: local-first library |