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 <noreply@anthropic.com>

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);

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!({}));
+        }
     }
 }
 

crates/ui-egui/src/state.rs

@@ -234,7 +234,10 @@ pub struct AppSettings {
     /// [`RECENT_LIBRARIES`]).
     pub recent_libraries: Vec<String>,
     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(),

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();

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));
+}

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` | |