From a31a43670bb3a12acb6da0ecc4c19429eaefef1a Mon Sep 17 00:00:00 2001 From: Chad Elliott Date: Tue, 27 Oct 2026 10:20:19 -0500 Subject: [PATCH] fix(lightroom): content duplicates start from import defaults; Show in Library skips Recently Deleted (#2589) A catalog photo whose bytes match another library photo got the generic develop defaults, so a raw with no white balance or lens setting in the catalog lost its as-shot white balance and embedded lens corrections and rendered unlike a file import. It now gets the import defaults (camera defaults plus the default preset), as a file import does, before the catalog's settings go on top. Show in Library selected the lowest mapped photo id, which can be a master left in Recently Deleted whose new virtual copy the import added; All Photos doesn't show it. It now selects the lowest mapped photo not in Recently Deleted, and the button is left out when there is none. Tests: a content-duplicate raw with no catalog settings equals its camera defaults (as-shot WB, lens profile on); Show in Library skips a trashed mapped photo and offers nothing when only trashed or vanished ids are mapped. Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/lightroom_catalog.rs | 3 ++ crates/engine/src/lightroom_catalog/tests.rs | 46 ++++++++++++++++ crates/ui-egui/src/lightroom_import.rs | 55 +++++++++++++++++--- docs/lightroom-catalog-import.md | 3 +- 4 files changed, 98 insertions(+), 9 deletions(-) diff --git a/crates/engine/src/lightroom_catalog.rs b/crates/engine/src/lightroom_catalog.rs index e0dee272a..30691009f 100644 --- a/crates/engine/src/lightroom_catalog.rs +++ b/crates/engine/src/lightroom_catalog.rs @@ -1033,6 +1033,9 @@ pub(crate) fn apply_prepared(s: &mut crate::Session, mut data: CatalogImport, co p.meta.iso = m.iso; p.meta.gps = m.gps; p.meta.focus = m.focus.clone(); + // the settings a file import gives it (as-shot white balance, embedded lens + // corrections, the default preset); the catalog's settings apply on top + crate::import::apply_import_defaults(s, &mut p); p } else { let id = s.catalog.alloc_photo_id(); diff --git a/crates/engine/src/lightroom_catalog/tests.rs b/crates/engine/src/lightroom_catalog/tests.rs index 725350a3e..28d9c615f 100644 --- a/crates/engine/src/lightroom_catalog/tests.rs +++ b/crates/engine/src/lightroom_catalog/tests.rs @@ -245,6 +245,52 @@ fn a_content_duplicate_of_a_trashed_photo_or_virtual_copy_is_a_new_live_photo() assert_eq!(s.catalog.photo(other).unwrap().deleted, case == "trashed", "{case}: the other photo is left as it was"); } } +/// A content duplicate starts from the settings a file import gives a raw (its as-shot white +/// balance and embedded lens corrections), as its file would on its own, before the catalog's +/// settings go on top; not from the generic defaults (#2589). +#[test] +fn a_content_duplicate_raw_starts_from_its_camera_defaults() { + let mut s = crate::Session::new(); + let other = library_photo(&mut s, "/elsewhere/a.DNG", |p| { + p.deleted = true; + p.format = "DNG".into(); + p.kind = lightcraft_catalog::MediaKind::Raw; + p.as_shot_wb = Some((5200.0, 4.0)); + p.embedded_lens = Some(lightcraft_develop::EmbeddedLens::default()); + }); + let mut data = sample(); + data.photos.retain(|p| number(&p.image, "masterImage") == 0); + // a catalog row with no develop settings of its own + data.photos[0].settings.clear(); + let files = crate::import::ImportReport { + duplicates: vec![crate::import::Duplicate { + path: "/missing/test.ARW".into(), + existing: Some(other.0), + reason: "content", + existing_deleted: true, + }], + ..Default::default() + }; + let undo0 = s.undo.len(); + let r = apply_prepared( + &mut s, + data, + ApplyPrepared { + update_existing: false, + files, + index: &mut ImportIndex::default(), + archive_path: None, + missing_sources: &HashSet::new(), + undo0, + }, + ) + .unwrap() + .report; + let new = s.catalog.photo(PhotoId(r["mapping"]["1"].as_u64().unwrap())).unwrap(); + assert_eq!(*new.develop, new.camera_defaults(), "{r}"); + assert_eq!((new.develop.wb.temp, new.develop.wb.tint), (5200.0, 4.0), "the file's as-shot white balance"); + assert!(new.develop.optics.lens_profile, "the file's embedded lens corrections are on"); +} /// A catalog photo matched (by path) to a library photo in Recently Deleted leaves it there, as /// the import was set up to do (a Lightroom import doesn't restore, see `OnDeleted`), and the report /// counts it so the photos don't go missing unexplained (#2589). A new virtual copy of it is diff --git a/crates/ui-egui/src/lightroom_import.rs b/crates/ui-egui/src/lightroom_import.rs index 83be31b7f..1b67f19ca 100644 --- a/crates/ui-egui/src/lightroom_import.rs +++ b/crates/ui-egui/src/lightroom_import.rs @@ -303,7 +303,7 @@ fn terminal(app: &mut LightcraftApp, ctx: &egui::Context, task: &LightroomTask, } }; if matches!(kind, Kind::Import) && value.get("cancelled") != Some(&Value::Bool(true)) { - app.lightroom_result = Some(result_dialog(&value, &message)); + app.lightroom_result = Some(result_dialog(&value, &message, &app.session.catalog)); show_result(app); } app.lightroom_last = Some(value); @@ -318,8 +318,9 @@ fn show_result(app: &mut LightcraftApp) { } /// The lasting result window for an inspect or import: counts, failures, missing originals, the -/// skipped images (one line, in the UI language) and at most eight warnings. -fn result_dialog(value: &Value, summary: &str) -> crate::state::Dialog { +/// skipped images (one line, in the UI language) and at most eight warnings. Show in Library goes +/// to the lowest mapped photo that is not in Recently Deleted, where All Photos can show it. +fn result_dialog(value: &Value, summary: &str, catalog: &lightcraft_catalog::Catalog) -> crate::state::Dialog { let report = value.get("report").unwrap_or(value); let mut details = Vec::new(); if let Some(photos) = report["photos"].as_u64() { @@ -366,7 +367,10 @@ fn result_dialog(value: &Value, summary: &str) -> crate::state::Dialog { details.push(crate::i18n::tr_format!("{count} more warnings.", count = warnings.len() - 8)); } } - let first = value.get("error").is_none().then(|| report["mapping"].as_object()?.values().filter_map(Value::as_u64).min()).flatten(); + // a catalog photo can map to a library photo left in Recently Deleted (#2589), which All + // Photos doesn't show + let live = |id: &u64| catalog.photo(lightcraft_catalog::PhotoId(*id)).is_some_and(|p| !p.deleted); + let first = value.get("error").is_none().then(|| report["mapping"].as_object()?.values().filter_map(Value::as_u64).filter(live).min()).flatten(); crate::state::Dialog::LightroomResult { summary: summary.to_string(), library: report["library"].as_str().or_else(|| value["library"].as_str()).map(str::to_string), @@ -680,6 +684,33 @@ mod tests { assert!(!h.app.session.visible().is_empty()); } + /// Show in Library goes to a photo All Photos shows: not a master left in Recently Deleted + /// whose new virtual copy the import added (#2589), and nowhere when no mapped photo is shown. + #[test] + fn show_in_library_skips_photos_in_recently_deleted() { + use lightcraft_catalog::{Op, Photo, Source}; + let mut s = lightcraft_engine::Session::new(); + let mut add = |deleted: bool| { + let id = s.catalog.alloc_photo_id(); + let mut p = Photo::new(id, Source::File { path: format!("/p/{}.jpg", id.0) }, "a.jpg", "JPEG", 2, 2, "now"); + p.deleted = deleted; + s.commit("Add", Op::AddPhoto { photo: Box::new(p) }).unwrap(); + id.0 + }; + let (trashed, live) = (add(true), add(false)); + assert!(trashed < live); + let first = |mapping: Value| { + let crate::state::Dialog::LightroomResult { first, .. } = result_dialog(&json!({"report": {"mapping": mapping}}), "done", &s.catalog) + else { + panic!("not a Lightroom result") + }; + first + }; + assert_eq!(first(json!({"1": trashed, "2": live})), Some(live)); + assert_eq!(first(json!({"1": trashed})), None, "only a photo in Recently Deleted: no Show in Library"); + assert_eq!(first(json!({"1": live + 100})), None, "no longer in the library"); + } + #[test] fn lightroom_failure_stays_readable_without_offering_a_success_action() { let mut h = demo(); @@ -714,7 +745,9 @@ mod tests { {"image": 9, "reason": "masterSkipped"}, ]}}); crate::i18n::set_language(crate::i18n::Locale::En); - let crate::state::Dialog::LightroomResult { details: en, .. } = result_dialog(&value, "done") else { panic!("not a Lightroom result") }; + let crate::state::Dialog::LightroomResult { details: en, .. } = result_dialog(&value, "done", &Default::default()) else { + panic!("not a Lightroom result") + }; assert_eq!(en.len(), 4, "{en:?}"); assert_eq!(en[1], "3 catalog images were not imported because their records in the catalog are incomplete."); assert_eq!(&en[2..], ["w1", "w2"]); @@ -724,11 +757,15 @@ mod tests { fn photos_left_in_recently_deleted_are_one_line() { let value = json!({"report": {"photos": 3, "collections": 0, "inRecentlyDeleted": 2}}); crate::i18n::set_language(crate::i18n::Locale::En); - let crate::state::Dialog::LightroomResult { details, .. } = result_dialog(&value, "done") else { panic!("not a Lightroom result") }; + let crate::state::Dialog::LightroomResult { details, .. } = result_dialog(&value, "done", &Default::default()) else { + panic!("not a Lightroom result") + }; assert_eq!(details.len(), 2, "{details:?}"); assert!(details[1].starts_with("2 catalog photos are in this library's Recently Deleted"), "{details:?}"); let value = json!({"report": {"photos": 3, "collections": 0, "inRecentlyDeleted": 0}}); - let crate::state::Dialog::LightroomResult { details, .. } = result_dialog(&value, "done") else { panic!("not a Lightroom result") }; + let crate::state::Dialog::LightroomResult { details, .. } = result_dialog(&value, "done", &Default::default()) else { + panic!("not a Lightroom result") + }; assert_eq!(details.len(), 1, "no line when none are: {details:?}"); } @@ -737,7 +774,9 @@ mod tests { let warnings: Vec<_> = (0..10).map(|i| format!("w{i}")).collect(); let skipped: Vec<_> = (0..500).map(|i| json!({"image": i, "reason": "missingFile"})).collect(); let value = json!({"report": {"warnings": warnings, "skipped": skipped}}); - let crate::state::Dialog::LightroomResult { details, .. } = result_dialog(&value, "done") else { panic!("not a Lightroom result") }; + let crate::state::Dialog::LightroomResult { details, .. } = result_dialog(&value, "done", &Default::default()) else { + panic!("not a Lightroom result") + }; // one line for the skipped images, then the usual eight warnings and the rest counted assert_eq!(details.len(), 10, "{details:?}"); assert!(details[0].starts_with("500 catalog images were not imported")); diff --git a/docs/lightroom-catalog-import.md b/docs/lightroom-catalog-import.md index cd5df6b85..04cbfc415 100644 --- a/docs/lightroom-catalog-import.md +++ b/docs/lightroom-catalog-import.md @@ -52,7 +52,8 @@ lightcraft-cli run --library "LightCraft Library" library.importLightroom path=" The import itself (`library.importLightroom`, `lightcraft-cli run`) adds records to the currently open LightCraft library; only the desktop window above can make a new library for it first. The desktop result stays open until dismissed and shows the destination library, the number of records added, missing originals and warnings. -**Show in Library** opens All Photos, clears filters and selects an imported record. Errors stay +**Show in Library** opens All Photos, clears filters and selects an imported record (not one left in Recently +Deleted; the button is left out when every record is there). Errors stay readable in the same result window. If another dialog is open, the result waits until that dialog closes. The result is also available through `library.importLightroom {"status":true}` during that session.