From acbd08eb7355f6fe9dc6d3c99a9478f79478ea8e Mon Sep 17 00:00:00 2001 From: Marco Arnulfo Date: Mon, 12 Oct 2026 18:14:05 +0200 Subject: [PATCH] LR-LIB-DELETE: a file in Recently Deleted can be checked by hand in the import review (fixes #1285) With "Leave them", the review refused every click on a file whose photo is in Recently Deleted, and its "In Recently Deleted" badge was drawn over the checkbox. Leave them now only keeps such files out of the default selection (Check All and the file-type toggles skip them); a click checks one and it is restored, as the import does by default. The badge goes under the checkbox when it doesn't fit beside it. --- crates/ui-egui/src/headless.rs | 64 +++++++++++++++++++++++++++++++ crates/ui-egui/src/import.rs | 69 ++++++++++++++++++++++++++++++---- docs/parity-checklist.md | 2 +- 3 files changed, 127 insertions(+), 8 deletions(-) diff --git a/crates/ui-egui/src/headless.rs b/crates/ui-egui/src/headless.rs index 8cb91b9b3..6083c4575 100644 --- a/crates/ui-egui/src/headless.rs +++ b/crates/ui-egui/src/headless.rs @@ -3076,6 +3076,70 @@ mod tests { } } + /// With "Leave them", a file in Recently Deleted is left unchecked but a click on its checkbox + /// (or on its "In Recently Deleted" badge) still checks it, and importing it restores the + /// photo (issue #1285). + #[test] + fn import_review_checks_a_recently_deleted_file_by_hand() { + let dir = std::env::temp_dir().join(format!("lc-ui-import-trash-click-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + for i in 0..2u8 { + let data: Vec<[u8; 4]> = (0..40 * 30).map(|k| [(k % 40 * 6) as u8, i * 90, 7, 255]).collect(); + let img = lightcraft_raster::Rgba8 { width: 40, height: 30, data }; + let png = lightcraft_codecs::encode_png(&lightcraft_codecs::EncodeImage::rgba8(&img), &lightcraft_codecs::EncodeMeta::default()).unwrap(); + std::fs::write(dir.join(format!("img{i}.png")), png).unwrap(); + } + let services = crate::Services { png: None, ..Default::default() }; + let mut app = LightcraftApp::new(lightcraft_engine::Session::new().with_fs(), services); + app.ui.view = crate::state::ViewMode::PhotoGrid; + let mut h = Headless::new(app, [1300.0, 900.0], 1.0); + let t = Duration::from_secs(10); + let r = h.request("engine.execute", json!({"command": "library.import", "params": {"paths": [dir.join("img0.png").to_string_lossy()]}}), t); + let id = r["result"]["imported"][0].as_u64().unwrap(); + h.request("engine.execute", json!({"command": "photo.rate", "params": {"ids": [id], "rating": 3}}), t); + h.request("engine.execute", json!({"command": "photo.delete", "params": {"ids": [id]}}), t); + h.request("engine.execute", json!({"command": "file.addPhotos", "params": {"paths": [dir.to_string_lossy()]}}), t); + h.step_until(SETTLE, |h| matches!(h.app.ui.dialog, Some(crate::state::Dialog::Import { .. }))); + let Some(crate::state::Dialog::Import { opts }) = &mut h.app.ui.dialog else { panic!("no import review") }; + let trashed = (0..opts.candidates.len()).find(|i| opts.is_trashed(*i)).expect("a file in Recently Deleted"); + opts.set_on_deleted(""); + h.settle(SETTLE); + let checked = |h: &Headless| matches!(&h.app.ui.dialog, Some(crate::state::Dialog::Import { opts }) if opts.checked[trashed]); + assert!(!checked(&h), "Leave them: unchecked"); + let cell = |h: &mut Headless| { + let r = h.request("ui.widgets", json!({"filter": format!("import:{trashed}")}), t); + let w = r["result"].as_array().and_then(|a| a.iter().find(|w| w["id"] == format!("import:{trashed}"))).cloned().expect("cell on screen"); + [0usize, 1, 2, 3].map(|i| w["rect"][i].as_f64().unwrap()) + }; + // the checkbox: 16 pt, 6 pt in from the cell's top-left corner + let [x, y, ..] = cell(&mut h); + h.request("ui.click", json!({"x": x + 14.0, "y": y + 14.0}), t); + h.settle(SETTLE); + assert!(checked(&h), "a click on the checkbox checks the file"); + // the badges ("Will be restored" at the top right, "In Recently Deleted" under the + // checkbox) don't swallow the click either + let [x, y, w, _] = cell(&mut h); + h.request("ui.click", json!({"x": x + w - 14.0, "y": y + 12.0}), t); + h.settle(SETTLE); + assert!(!checked(&h), "a click on the badge unchecks it again"); + h.request("ui.click", json!({"x": x + w - 14.0, "y": y + 34.0}), t); + h.settle(SETTLE); + assert!(checked(&h)); + let r = h.request("ui.dialog.confirm", json!({}), t); + assert_eq!(r["ok"], true, "{r}"); + let t0 = std::time::Instant::now(); + while h.app.import.is_some() && t0.elapsed() < Duration::from_secs(30) { + h.step(); + } + assert!(h.app.import.is_none(), "finished"); + let photos = &h.app.session.catalog; + assert!(photos.photo(lightcraft_catalog::PhotoId(id)).is_some_and(|p| !p.deleted && p.rating == 3), "restored with its edits"); + assert_eq!(photos.photos().filter(|p| !p.deleted).count(), 2); + assert_eq!(photos.len(), 2, "restored, not imported a second time"); + let _ = std::fs::remove_dir_all(&dir); + } + /// The import review opens bigger than the old fixed 6 × 2.5 grid and can be resized by its /// corner: the photo grid takes the new height, and the window then keeps its size (issue #337). #[test] diff --git a/crates/ui-egui/src/import.rs b/crates/ui-egui/src/import.rs index a18357eca..89fe2efe7 100644 --- a/crates/ui-egui/src/import.rs +++ b/crates/ui-egui/src/import.rs @@ -8,7 +8,7 @@ //! catalog between frames, with a row and ✕ in the activity stack (issue #345); the whole import is //! one undo step. -use egui::{Align2, Color32, Rect, Sense, Stroke, StrokeKind, pos2, vec2}; +use egui::{Align2, Color32, Rect, Sense, Stroke, StrokeKind, Vec2, pos2, vec2}; use lightcraft_engine::import::{ ImportCandidate, ImportUprightMode, ImportWarning, ImportWarningCode, ScanInput, ScanOutput, ScanProgress, scan_with, }; @@ -123,14 +123,21 @@ impl ImportDialog { if !self.is_trashed(i) { continue; } - let on = self.importable(i); + let on = self.checked_by_default(i); if let Some(c) = self.checked.get_mut(i) { *c = on; } } } + /// Candidate `i` can be checked: a readable file the library doesn't have, or one whose photo + /// is in Recently Deleted — even with "Leave them", a click picks it (issue #1285). pub fn importable(&self, i: usize) -> bool { - self.candidates.get(i).is_some_and(|c| c.error.is_none() && (c.duplicate.is_none() || self.is_trashed(i) && !self.on_deleted.is_empty())) + self.candidates.get(i).is_some_and(|c| c.error.is_none() && (c.duplicate.is_none() || self.is_trashed(i))) + } + /// Candidate `i` is checked by Check All and the file type toggles: with "Leave them", a file in + /// Recently Deleted is only checked by hand. + pub fn checked_by_default(&self, i: usize) -> bool { + self.importable(i) && !(self.is_trashed(i) && self.on_deleted.is_empty()) } /// The file types among the candidates (`ARW`, `JPG`…, upper case, sorted) and how many of each. pub fn file_types(&self) -> Vec<(String, usize)> { @@ -149,7 +156,7 @@ impl ImportDialog { pub fn check_type(&mut self, ty: &str, on: bool) { for i in 0..self.candidates.len() { let same = self.candidates.get(i).is_some_and(|c| file_type(c) == ty); - let importable = self.importable(i); + let importable = self.checked_by_default(i); if same && let Some(c) = self.checked.get_mut(i) { *c = on && importable; } @@ -933,7 +940,7 @@ pub fn body(app: &mut LightcraftApp, ui: &mut egui::Ui, d: &mut ImportDialog) { } if crate::widgets::text_button(ui, "importAll", crate::i18n::tr("Check All"), false).clicked() { for i in 0..n { - d.checked[i] = d.importable(i); + d.checked[i] = d.checked_by_default(i); } } }); @@ -1282,7 +1289,8 @@ fn candidate_cell(app: &mut LightcraftApp, ui: &mut egui::Ui, d: &mut ImportDial } let in_trash = c.existing.is_some_and(|id| app.session.catalog.photo(lightcraft_catalog::PhotoId(id)).is_some_and(|p| p.deleted)); let badge = match (&c.duplicate, &c.error) { - (Some(_), _) if in_trash && ok => Some(if d.on_deleted == "restore" { "Will be restored" } else { "Will be replaced" }), + // checked with "Leave them", the file goes without `onDeleted`: the engine restores it + (Some(_), _) if in_trash && on => Some(if d.on_deleted == "fresh" { "Will be replaced" } else { "Will be restored" }), (Some(_), _) if in_trash => Some("In Recently Deleted"), (Some(r), _) if r == "path" => Some("In library"), (Some(_), _) => Some("Duplicate"), @@ -1291,7 +1299,7 @@ fn candidate_cell(app: &mut LightcraftApp, ui: &mut egui::Ui, d: &mut ImportDial }; if let Some(b) = badge { let g = p.layout_no_wrap(crate::i18n::tr(b).to_string(), t.semibold(10.0), Color32::WHITE); - let br = Rect::from_min_size(pos2(img.right() - g.size().x - 12.0, img.top() + 6.0), g.size() + vec2(8.0, 4.0)); + let br = badge_rect(img, cb, g.size()); p.rect_filled(br, 3.0, Color32::from_rgba_unmultiplied(170, 60, 50, 220)); p.galley(br.min + vec2(4.0, 2.0), g, Color32::WHITE); } @@ -1313,6 +1321,15 @@ fn candidate_cell(app: &mut LightcraftApp, ui: &mut egui::Ui, d: &mut ImportDial } } +/// Where a cell's badge goes: top right, or under the checkbox when the text is too wide to sit +/// beside it — drawn over the checkbox it hid it (issue #1285). +fn badge_rect(img: Rect, cb: Rect, text: Vec2) -> Rect { + let size = text + vec2(8.0, 4.0); + let left = img.right() - size.x - 4.0; + let top = if left < cb.right() + 4.0 { cb.bottom() + 4.0 } else { img.top() + 6.0 }; + Rect::from_min_size(pos2(left, top), size) +} + /// Reserve the actual translated tag button width beside a template field. pub(crate) fn tag_field_width(ui: &egui::Ui) -> f32 { let t = Tokens::get(ui.ctx()); @@ -1580,6 +1597,44 @@ mod tests { assert_eq!(d.checked, [true, true, false, false, true, true]); } + /// With "Leave them", a file in Recently Deleted stays out of Check All and the type toggles, + /// but a click still checks it: it is imported (restored) on its own (issue #1285). + #[test] + fn leave_them_keeps_a_trashed_file_out_until_it_is_clicked() { + let mut d = dialog(); + d.set_trashed(vec![false, false, false, true, false, false]); + assert!(d.checked[3], "restored by default"); + d.set_on_deleted(""); + assert!(!d.checked[3]); + d.check_type("JPG", true); + assert!(!d.checked[3], "the type toggle leaves it out"); + d.click(3, false); + assert!(d.checked[3], "a click checks it"); + assert!(d.selected_paths().contains(&"img3.jpg".to_string())); + d.click(3, false); + assert!(!d.checked[3]); + // a plain duplicate (not in Recently Deleted) still can't be checked + d.set_trashed(vec![false; 6]); + d.click(3, false); + assert!(!d.checked[3]); + } + + /// A badge too wide to fit beside the checkbox goes under it instead of covering it (issue #1285). + #[test] + fn a_badge_never_covers_the_checkbox() { + let img = Rect::from_min_size(pos2(100.0, 50.0), vec2(116.0, 116.0)); + let cb = Rect::from_min_size(img.min + vec2(6.0, 6.0), vec2(16.0, 16.0)); + // "Will be restored" fits beside it, top right + let short = badge_rect(img, cb, vec2(76.0, 12.0)); + assert_eq!(short.right(), img.right() - 4.0); + assert_eq!(short.top(), img.top() + 6.0); + // "In Recently Deleted" doesn't + let long = badge_rect(img, cb, vec2(96.0, 12.0)); + assert!(!long.intersects(cb), "{long:?} covers {cb:?}"); + assert_eq!(long.right(), img.right() - 4.0); + assert!(img.contains_rect(long)); + } + #[test] fn shift_click_with_a_stale_anchor_toggles_one() { let mut d = dialog(); diff --git a/docs/parity-checklist.md b/docs/parity-checklist.md index 77ae26e4c..22833854c 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; with Leave them such a file starts unchecked and Check All and the file-type toggles leave it out, but a click still checks it and it is restored (issue #1285: before, the click did nothing and the "In Recently Deleted" badge was drawn over the checkbox, which now sits above it); 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-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 |