From cf085041ed9d5ad10d5d0b5440aeec3873e14cee Mon Sep 17 00:00:00 2001 From: Matthew James Sivak Date: Mon, 12 Oct 2026 15:39:58 +0200 Subject: [PATCH] fix: deleting a photo asks first, also from the right-click menu (#1239) Cause: Settings > General > "Confirm before moving photos to Recently Deleted" defaulted to off, so Delete never asked unless the person found the setting; and the grid/filmstrip photo menu's Delete Photo ran photo.delete directly, skipping menus::confirm_delete even with the setting on. Fix: the setting is on by default; the photo menu's Delete Photo goes through menus::confirm_delete like the key and the menu bar. The setting is now saved as `askBeforeDelete`: every earlier version wrote `confirmDelete: false` for people who never opened Settings, so that key is no longer read and those installs ask too. Turning it off still works and survives a restart. ui.menu.invoke / engine.execute (agents) still run the command directly. Test: tests_panels::deleting_a_photo_asks_first_from_the_key_the_menu_and_ the_right_click_menu (failed: off by default, then the right-click menu deleted without asking) and tests_appearance::delete_asks_first_by_default_ also_for_settings_saved_by_earlier_versions (failed: default off). Co-Authored-By: Claude Opus 5.5 --- crates/ui-egui/src/headless.rs | 5 ++- crates/ui-egui/src/panels/grid.rs | 9 ++++- crates/ui-egui/src/state.rs | 7 +++- crates/ui-egui/src/tests_appearance.rs | 18 ++++++++- crates/ui-egui/src/tests_panels.rs | 52 ++++++++++++++++++++++++++ docs/parity-checklist.md | 4 +- 6 files changed, 86 insertions(+), 9 deletions(-) diff --git a/crates/ui-egui/src/headless.rs b/crates/ui-egui/src/headless.rs index 8cb91b9b3..2bb980c4e 100644 --- a/crates/ui-egui/src/headless.rs +++ b/crates/ui-egui/src/headless.rs @@ -3604,7 +3604,10 @@ mod tests { assert_eq!(r["ok"], true, "{tab}: {r}"); assert_eq!(h.app.ui.dialog, Some(crate::state::Dialog::Settings { tab: tab.into() })); } - // General: confirm before delete, startup view + // General: confirm before delete (on by default), startup view + assert!(h.app.ui.settings.confirm_delete); + h.request("ui.clickWidget", json!({"id": "check:settings.confirmDelete"}), t); + assert!(!h.app.ui.settings.confirm_delete); h.request("ui.clickWidget", json!({"id": "check:settings.confirmDelete"}), t); assert!(h.app.ui.settings.confirm_delete); h.request("ui.clickWidget", json!({"id": "button:settingsStartup-2"}), t); diff --git a/crates/ui-egui/src/panels/grid.rs b/crates/ui-egui/src/panels/grid.rs index 7b6d5cf61..e008e043a 100644 --- a/crates/ui-egui/src/panels/grid.rs +++ b/crates/ui-egui/src/panels/grid.rs @@ -1137,8 +1137,13 @@ pub fn context_menu(app: &mut LightcraftApp, ui: &mut egui::Ui, id: PhotoId) { if ui.button(crate::i18n::tr("Delete Permanently")).clicked() { let _ = app.run("photo.deletePermanently", json!({})); } - } else if ui.button(crate::i18n::tr("Delete Photo")).clicked() { - let _ = app.run("photo.delete", json!({})); + } else { + let delete = ui.button(crate::i18n::tr("Delete Photo")); + register(ui.ctx(), "button:contextDeletePhoto", delete.rect); + // asks first like the key and the menu bar (Settings ▸ General) + if delete.clicked() && !crate::menus::confirm_delete(app) { + let _ = app.run("photo.delete", json!({})); + } } } diff --git a/crates/ui-egui/src/state.rs b/crates/ui-egui/src/state.rs index b76066d55..b1d51b7cb 100644 --- a/crates/ui-egui/src/state.rs +++ b/crates/ui-egui/src/state.rs @@ -234,7 +234,10 @@ pub struct AppSettings { /// [`RECENT_LIBRARIES`]). pub recent_libraries: Vec, pub startup_view: StartupView, - /// Ask before moving photos to Recently Deleted (keyboard and menu). + /// Ask before moving photos to Recently Deleted (keyboard, menu bar and a photo's menu); on by + /// default. Saved as `askBeforeDelete`: earlier versions saved `confirmDelete: false` for + /// everyone who never opened Settings, so that key is no longer read (#1239). + #[serde(rename = "askBeforeDelete")] pub confirm_delete: bool, /// GPU rendering allowed (`app.gpu`). pub gpu: bool, @@ -281,7 +284,7 @@ impl Default for AppSettings { library_path: String::new(), recent_libraries: Vec::new(), startup_view: StartupView::Last, - confirm_delete: false, + confirm_delete: true, gpu: true, preview_limit: 0, external_editor: String::new(), diff --git a/crates/ui-egui/src/tests_appearance.rs b/crates/ui-egui/src/tests_appearance.rs index 947e6cf9f..78a48194f 100644 --- a/crates/ui-egui/src/tests_appearance.rs +++ b/crates/ui-egui/src/tests_appearance.rs @@ -27,15 +27,29 @@ fn new_users_keep_the_dark_look_and_old_settings_load_unchanged() { assert_eq!((s.appearance_mode, s.dark_theme, s.light_theme), (AppearanceMode::Dark, DarkTheme::Charcoal, LightTheme::Silver)); assert_eq!(s.theme(Some(egui::Theme::Light)), ThemeKind::Charcoal, "Auto is opt-in"); // a ui.json saved before appearance modes existed - let old: UiState = serde_json::from_str(r#"{"settings": {"gridBadges": "always", "confirmDelete": true}}"#).unwrap(); + let old: UiState = serde_json::from_str(r#"{"settings": {"gridBadges": "always", "startupView": "detail"}}"#).unwrap(); assert_eq!(old.settings.appearance_mode, AppearanceMode::Dark); assert_eq!(old.settings.dark_theme, DarkTheme::Charcoal); assert_eq!(old.settings.light_theme, LightTheme::Silver); - assert!(old.settings.confirm_delete); + assert_eq!(old.settings.startup_view, crate::state::StartupView::Detail); // and the Charcoal tokens are the ones LightCraft always had assert_eq!(crate::theme::Tokens::for_kind(ThemeKind::Charcoal).chrome, crate::theme::Tokens::default().chrome); } +/// Delete asks first by default (#1239). Earlier versions saved `"confirmDelete": false` for +/// everyone who never opened Settings, so that key is not read any more: those installs ask too. +/// A choice made from now on survives a restart. +#[test] +fn delete_asks_first_by_default_also_for_settings_saved_by_earlier_versions() { + assert!(AppSettings::default().confirm_delete); + let old: UiState = serde_json::from_str(r#"{"settings": {"confirmDelete": false}}"#).unwrap(); + assert!(old.settings.confirm_delete, "the old default does not turn the question off"); + let mut ui = UiState::default(); + ui.settings.confirm_delete = false; + let back: UiState = serde_json::from_str(&serde_json::to_string(&ui).unwrap()).unwrap(); + assert!(!back.settings.confirm_delete, "a choice made now is kept"); +} + #[test] fn choices_validate_and_survive_a_restart() { let mut a = app(); diff --git a/crates/ui-egui/src/tests_panels.rs b/crates/ui-egui/src/tests_panels.rs index 25edf3bfa..dd6e509e6 100644 --- a/crates/ui-egui/src/tests_panels.rs +++ b/crates/ui-egui/src/tests_panels.rs @@ -2141,3 +2141,55 @@ fn right_click_add_to_album_offers_a_new_album_when_there_is_none() { assert!(trip.photos.contains(&photo), "the right-clicked photo is in it: {:?}", trip.photos); assert!(h.app.ui.dialog.is_none(), "{:?}", h.app.ui.dialog); } + +/// Delete asks before photos go to Recently Deleted, by default and from every way in: the key, the +/// Photo menu and a photo's right-click menu (#1239). With the setting off, it deletes at once. +#[test] +fn deleting_a_photo_asks_first_from_the_key_the_menu_and_the_right_click_menu() { + use crate::state::Dialog; + let mut h = demo([1400.0, 900.0], json!({"view": "photoGrid"})); + assert!(h.app.ui.settings.confirm_delete, "on by default"); + let photo = h.app.session.visible_cloned()[0]; + let r = h.request("engine.execute", json!({"command": "library.select", "params": {"ids": [photo.0]}}), T); + assert_eq!(r["ok"], true, "{r}"); + h.settle(SETTLE); + let deleted = |h: &Headless| h.app.session.catalog.photo(photo).unwrap().deleted; + + // ⌫ and Photo ▸ Delete Photo ask; Cancel keeps the photo + h.request("ui.key", json!({"key": "delete"}), T); + assert_eq!(h.app.ui.dialog, Some(Dialog::ConfirmDelete { count: 1 }), "the key asks"); + h.request("ui.dialog.cancel", json!({}), T); + assert!(!deleted(&h)); + // (what a click on the menu bar's item runs; `ui.menu.invoke` is the agents' engine.execute) + crate::menubar::run_item(&mut h.app, "photo.delete", json!({})).unwrap(); + assert_eq!(h.app.ui.dialog, Some(Dialog::ConfirmDelete { count: 1 }), "the menu asks"); + h.request("ui.dialog.cancel", json!({}), T); + assert!(!deleted(&h)); + + // the right-click menu's Delete Photo asks too; OK deletes + let right_click_delete = |h: &mut Headless| { + let thumb = widget(h, &format!("thumb:{}", photo.0)); + let r = h.request("ui.click", json!({"x": thumb.center().x, "y": thumb.center().y, "button": "right"}), T); + assert_eq!(r["ok"], true, "{r}"); + h.step(); + h.step(); + let r = h.request("ui.clickWidget", json!({"id": "button:contextDeletePhoto"}), T); + assert_eq!(r["ok"], true, "{r}"); + h.step(); + }; + right_click_delete(&mut h); + assert_eq!(h.app.ui.dialog, Some(Dialog::ConfirmDelete { count: 1 }), "the right-click menu asks"); + assert!(!deleted(&h), "nothing deleted before the answer"); + let r = h.request("ui.dialog.confirm", json!({}), T); + assert_eq!(r["ok"], true, "{r}"); + assert!(deleted(&h)); + + // turned off, the right-click menu deletes at once + h.request("engine.execute", json!({"command": "edit.undo"}), T); + assert!(!deleted(&h)); + h.app.ui.settings.confirm_delete = false; + h.settle(SETTLE); + right_click_delete(&mut h); + assert_eq!(h.app.ui.dialog, None); + assert!(deleted(&h)); +} diff --git a/docs/parity-checklist.md b/docs/parity-checklist.md index 77ae26e4c..3de0655db 100644 --- a/docs/parity-checklist.md +++ b/docs/parity-checklist.md @@ -107,7 +107,7 @@ differences and missing bindings are in [ui-parity.md](ui-parity.md#shortcuts). | LR-LIB-PEOPLE | People / faces | P2 | 🟡 | `cmd:faces.detect`, `cmd:view.people`, `cmd:view.faceBoxes`, `cmd:photo.removeRegion`, `cmd:photo.setRegion`, `cmd:library.filter`, `crates/ui-egui/src/panels/people.rs`, `crates/catalog/src/query.rs` (`people`), `crates/engine/src/media.rs` (`face_job`), `crates/meta/src/xmp.rs` | MVP: named face regions read from XMP (MWG-RS, read-only, see `docs/xmp-interop.md`; clipped to the photo; Lightroom's `Rotation` of ±π/2 and π handled, mirrored orientations taken as written; boxes follow Rotate Left/Right and flips; a sidecar stating `mwg-rs:Regions`, even empty, replaces the photo's on re-read, one without leaves them) become cards in the People view (View ▸ People, or the People button in the toolbar): a close-up of the person's largest face, name and photo count; a click filters the grid (`person` in `library.filter`, `person:` search token, filter chip). Face boxes in the loupe can be switched off (View ▸ Face Boxes, toolbar) resized with eight drag handles and removed with the × shown on hover (catalog-only and undoable, one undo step per drag; the sidecar is never rewritten for either); no moving a box by dragging it yet. Photo ▸ Detect Faces (`cmd:faces.detect`) finds faces with the YuNet detector (pure Rust, about 200 ms a photo, matches OpenCV's own YuNet on a 45-photo test set; the 232 KB model is a one-time download offered in the same step, never part of LightCraft) and adds them as unnamed boxes; names are suggested from the faces already named (recognition models are opt-in downloads, run on the CPU in a background thread; a face is named only when you confirm it); a person's page shows their cropped faces and the unnamed faces that look like them (LRC-LIB-PEOPLE); with recognition on, a background scan finds faces in photos that have none and embeds every face (one to three worker threads, raws through their embedded preview); an Unnamed faces section orders faces by likeness and names a selected group at once; no separated clusters, no manual tagging, no writing regions back | | LR-LIB-STACK | Stacks | P1 | ✅ | `cmd:stack.group`, `cmd:stack.ungroup`, `cmd:stack.toggle`, `cmd:stack.setTop`, `cmd:stack.remove`, `cmd:stack.auto`, `crates/catalog/src/stacks.rs` | grid/filmstrip count badges, expand/collapse, auto-stack by capture time; no visual-similarity auto-stack | | LR-LIB-VERSIONS | Versions | P1 | ✅ | `cmd:version.create` | see section M | -| LR-LIB-DELETE | Delete / Recently Deleted | P0 | ✅ | `cmd:photo.delete`, `cmd:photo.restore`, `cmd:photo.deletePermanently` | Delete and Delete Permanently retain surviving selected photos; if none remain, select the next visible survivor (previous at the end) in the current source/filter/sort; permanent deletion removes catalog records, not files on disk; Restore and Delete Permanently in the Photo menu and the photo context menus for photos in Recently Deleted (before, the UI had no way to restore); importing a file that is in Recently Deleted again restores the photo with its edits, rating and albums (issues #298, #706: before, the import skipped it as "already in the library" while the grid showed it deleted) — the review dialog checks it, badges it "Will be restored", counts it as "in Recently Deleted" rather than "already in the library", and its choice Restore them / Import as new / Leave them starts on Restore; dropped files restore too ("Restored 1 photo from Recently Deleted"); a deleted photo matched by content whose file is gone is relinked to the imported file; Photo ▸ Empty Recently Deleted (`cmd:library.emptyRecentlyDeleted`, one undo step, shown while the trash is the source); import takes `onDeleted` restore (default) / skip / fresh for files in the trash (`cmd:library.import`; the report lists `restored`, and with skip flags the duplicate `existingDeleted`); browsing a folder in Local (`cmd:library.browse`), Auto Import and a Lightroom catalog import never restore a deleted photo; no confirmation dialog, no auto-purge after N days (needs a deleted-at date in the catalog) | +| LR-LIB-DELETE | Delete / Recently Deleted | P0 | ✅ | `cmd:photo.delete`, `cmd:photo.restore`, `cmd:photo.deletePermanently` | Delete and Delete Permanently retain surviving selected photos; if none remain, select the next visible survivor (previous at the end) in the current source/filter/sort; permanent deletion removes catalog records, not files on disk; Restore and Delete Permanently in the Photo menu and the photo context menus for photos in Recently Deleted (before, the UI had no way to restore); importing a file that is in Recently Deleted again restores the photo with its edits, rating and albums (issues #298, #706: before, the import skipped it as "already in the library" while the grid showed it deleted) — the review dialog checks it, badges it "Will be restored", counts it as "in Recently Deleted" rather than "already in the library", and its choice Restore them / Import as new / Leave them starts on Restore; dropped files restore too ("Restored 1 photo from Recently Deleted"); a deleted photo matched by content whose file is gone is relinked to the imported file; Photo ▸ Empty Recently Deleted (`cmd:library.emptyRecentlyDeleted`, one undo step, shown while the trash is the source); import takes `onDeleted` restore (default) / skip / fresh for files in the trash (`cmd:library.import`; the report lists `restored`, and with skip flags the duplicate `existingDeleted`); browsing a folder in Local (`cmd:library.browse`), Auto Import and a Lightroom catalog import never restore a deleted photo; Delete asks first ("Move this photo to Recently Deleted?") from the key, the Photo menu and the photo context menu, on by default and switched off in Settings ▸ General (#1239: before, it was off by default and the context menu never asked); no auto-purge after N days (needs a deleted-at date in the catalog) | | LR-LIB-REMOVE-ALBUM | Remove from album | P0 | ✅ | `cmd:album.removePhotos` | | | LR-LIB-DUPLICATE | Duplicate a photo | P2 | ✅ | `cmd:photo.duplicate` | Photo ▸ Duplicate: a real `-copy` file with the same settings, metadata and albums (virtual copies share the file) | | LR-LIB-RENAME | Batch rename | P1 | ✅ | `cmd:photo.rename`, `cmd:photo.renamePreview`, `cmd:photo.renameTokens`, `cmd:dialog.rename`, `crates/engine/src/rename.rs` (`TOKENS`) | template tokens {name} {num} {seq:N} {date:%Y%m%d} {folder} {camera} {lens} {iso} {rating} {title} {creator} {ext} (shared with export naming and import rename); a Tags picker beside every template field (Rename Photos, import Rename, export File name) lists each tag with meaning + live example, the date directives and the rules, and inserts the clicked tag at the text cursor; unknown tags are flagged; preview; renames files on disk with their XMP sidecars (a stem sidecar another file still shares, e.g. raw + JPEG, is copied rather than taken), never overwriting (-1, -2… suffixes; a change of letter case only is checked by file identity, so on case-sensitive volumes `img_1.jpg` next to `IMG_1.JPG` is a collision), rolls back on failure (a file that can't be moved back is reported old → new and the library follows it as an undoable partial rename; the same for undo/redo); undo/redo move the files; virtual copies follow | @@ -563,7 +563,7 @@ visible menu bar yet: the menu model is only exposed through the control channel | MENU-PHOTO-RENAME | Rename N photos… | P1 | ✅ | `cmd:dialog.rename` | live “Rename N Photos…” label | | MENU-PHOTO-CAPTURETIME | Edit capture time… | P1 | ✅ | `cmd:dialog.captureTime` | | | MENU-PHOTO-COVER | Set as album cover | P2 | ✅ | `cmd:album.setCover` | | -| MENU-PHOTO-DELETE | Delete N photos… | P0 | ✅ | `cmd:photo.delete` | no confirmation; static label | +| MENU-PHOTO-DELETE | Delete N photos… | P0 | ✅ | `cmd:photo.delete` | asks first (Settings ▸ General, on by default, #1239); static label | | MENU-PHOTO-MOVETOCLOUD | Move/copy to cloud | OOS | 🚫 | | | | MENU-WINDOW-MINIMIZE | Minimize / zoom | P1 | ✅ | `apps/lightcraft/src/native_menu.rs` | the system's Minimize (⌘M) and Zoom items at the top of Window | | MENU-WINDOW-PANELS | Panel switches | P0 | ✅ | `cmd:panel.edit`, `cmd:panel.crop`, `cmd:panel.remove`, `cmd:panel.masking`, `cmd:panel.presets`, `cmd:panel.versions` | |