From d9476bc16dd0c301da05a0defced9701059d1426 Mon Sep 17 00:00:00 2001 From: Chad Elliott Date: Thu, 29 Oct 2026 23:57:55 -0500 Subject: [PATCH] fix(lightroom): a content duplicate takes its file's capture time (#2589) A content duplicate whose catalog row has no capture time took the capture time of the library photo with the same bytes, which Edit Capture Time may have changed. The import job now resolves the file's capture time (the file's, else its XMP sidecar's, else its modification time) before it sets the file aside as a duplicate, and hands it over with the file's metadata (`Duplicate::file`, a `FileFacts`). The Lightroom import uses it; the catalog's capture time still applies on top when the row has one. Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/import.rs | 4 +- crates/engine/src/import/commit.rs | 12 ++---- crates/engine/src/import/job.rs | 39 ++++++++++---------- crates/engine/src/import/types.rs | 19 +++++++--- crates/engine/src/lightroom_catalog.rs | 18 +++++---- crates/engine/src/lightroom_catalog/tests.rs | 19 +++++++--- 6 files changed, 63 insertions(+), 48 deletions(-) diff --git a/crates/engine/src/import.rs b/crates/engine/src/import.rs index 6fe665896..e42057c2b 100644 --- a/crates/engine/src/import.rs +++ b/crates/engine/src/import.rs @@ -53,8 +53,8 @@ pub use scan::{ScanInput, ScanOutput, ScanProgress, scan, scan_with}; #[cfg(test)] use scan::{library_files, library_hashes}; pub use types::{ - Duplicate, ImportCandidate, ImportMode, ImportOptions, ImportReport, ImportUprightMode, ImportWarning, ImportWarningCode, Kept, Moved, OnDeleted, - Organize, + Duplicate, FileFacts, ImportCandidate, ImportMode, ImportOptions, ImportReport, ImportUprightMode, ImportWarning, ImportWarningCode, Kept, Moved, + OnDeleted, Organize, }; #[cfg(test)] diff --git a/crates/engine/src/import/commit.rs b/crates/engine/src/import/commit.rs index 02f587a43..a731488a0 100644 --- a/crates/engine/src/import/commit.rs +++ b/crates/engine/src/import/commit.rs @@ -213,7 +213,7 @@ pub fn commit_prepared(s: &mut Session, opts: &ImportOptions, now: &str, prepare } report.imported.push(id.0); } - PreparedItem::Duplicate { path, existing, reason, hash, file_meta } => { + PreparedItem::Duplicate { path, existing, reason, hash, file } => { let existing = existing.or_else(|| { let h = hash?; new_hash.get(&h).copied().or_else(|| s.catalog.photos().find(|p| p.content_hash.as_deref() == Some(h.as_str())).map(|p| p.id)) @@ -232,13 +232,9 @@ pub fn commit_prepared(s: &mut Session, opts: &ImportOptions, now: &str, prepare report.restored.push(id.0); } } - _ => report.duplicates.push(Duplicate { - path, - existing: existing.map(|i| i.0), - reason, - existing_deleted: trashed.is_some(), - file_meta, - }), + _ => { + report.duplicates.push(Duplicate { path, existing: existing.map(|i| i.0), reason, existing_deleted: trashed.is_some(), file }) + } } } PreparedItem::Failed(path, e) => report.failed.push((path, e)), diff --git a/crates/engine/src/import/job.rs b/crates/engine/src/import/job.rs index d4468d128..ffc098c50 100644 --- a/crates/engine/src/import/job.rs +++ b/crates/engine/src/import/job.rs @@ -1,6 +1,6 @@ use super::commit::{How, Plan, Slot, Transfer, commit_prepared, name_family, run_transfers, system_clock}; use super::files::{expand, probe_all}; -use super::types::{ImportCandidate, ImportMode, ImportOptions, ImportReport, Kept, OnDeleted}; +use super::types::{FileFacts, ImportCandidate, ImportMode, ImportOptions, ImportReport, Kept, OnDeleted}; use super::{HashMap, MediaKind, Path, PathBuf, Photo, PhotoId, ProbeInfo, Session, Source}; /// Import files/folders. See the module docs. @@ -73,8 +73,8 @@ pub(super) enum PreparedItem { existing: Option, reason: &'static str, hash: Option, - /// The metadata the file itself carries, when it was read ([`Duplicate::file_meta`]). - file_meta: Option>, + /// What the file itself says, when it was read ([`Duplicate::file`]). + file: Option>, }, Failed(String, String), Kept(Kept), @@ -143,7 +143,7 @@ impl Prepared { existing: Some(existing), reason: if existing_path.is_some() { "path" } else { "content" }, hash: info.content_hash, - file_meta: Some(Box::new(info.meta)), + file: Some(Box::new(FileFacts { meta: info.meta, captured: info.captured })), }); } else { items.push(PreparedItem::Ready(Box::new(ReadyFile { path, stored, info, sidecar, placed }))); @@ -264,7 +264,7 @@ impl ImportJob { self.done.fetch_add(1, Relaxed); } Some((id, _)) => { - out.items.push(PreparedItem::Duplicate { path: f, existing: Some(id), reason: "path", hash: None, file_meta: None }); + out.items.push(PreparedItem::Duplicate { path: f, existing: Some(id), reason: "path", hash: None, file: None }); self.done.fetch_add(1, Relaxed); } None => todo.push(f), @@ -324,20 +324,6 @@ impl ImportJob { break; } } - if let Some(h) = &info.content_hash - && !opts.local - && let Some(existing) = self.by_hash.get(h) - { - self.done.fetch_add(1, Relaxed); - plan.slots.push(Slot::Item(PreparedItem::Duplicate { - path, - existing: *existing, - reason: "content", - hash: Some(h.clone()), - file_meta: Some(Box::new(info.meta)), - })); - continue; - } // the XMP sidecar (or a raw's embedded XMP), read before copying: its capture time files // and names the copy when the file itself has none let raw = info.kind == MediaKind::Raw; @@ -359,6 +345,21 @@ impl ImportJob { if info.captured.is_none() { info.captured = info.file_modified.clone(); } + // same bytes as a library photo: skipped, with what its file says (its capture time too) + if let Some(h) = &info.content_hash + && !opts.local + && let Some(existing) = self.by_hash.get(h) + { + self.done.fetch_add(1, Relaxed); + plan.slots.push(Slot::Item(PreparedItem::Duplicate { + path, + existing: *existing, + reason: "content", + hash: Some(h.clone()), + file: Some(Box::new(FileFacts { meta: info.meta, captured: info.captured })), + })); + continue; + } let stored = match (mode, &self.copy_root) { (ImportMode::Copy | ImportMode::Move, Some(root)) if !crate::import_move::inside(Path::new(&path), root) diff --git a/crates/engine/src/import/types.rs b/crates/engine/src/import/types.rs index 779c5f27c..f342bf4c9 100644 --- a/crates/engine/src/import/types.rs +++ b/crates/engine/src/import/types.rs @@ -146,12 +146,21 @@ pub struct Duplicate { /// That photo is in Recently Deleted (see [`OnDeleted`]). #[serde(rename = "existingDeleted", skip_serializing_if = "std::ops::Not::not")] pub existing_deleted: bool, - /// The metadata the file itself carries (EXIF, IPTC, embedded XMP), as a file import reads it; - /// `None` when the file wasn't read (a path match). A Lightroom import makes a record of its - /// own for a duplicate by content and starts it from these, not from the other photo's record, - /// whose metadata may have been edited (#2589). + /// What the file itself says, as a file import reads it; `None` when the file wasn't read (a + /// path match). A Lightroom import makes a record of its own for a duplicate by content and + /// starts it from these, not from the other photo's record, which may have been edited (#2589). #[serde(skip)] - pub file_meta: Option>, + pub file: Option>, +} + +/// What a file import reads from a file itself, before anything in the library is edited. +#[derive(Clone, Debug, Default)] +pub struct FileFacts { + /// The metadata the file carries (EXIF, IPTC, embedded XMP). + pub meta: lightcraft_catalog::Meta, + /// The capture time a file import gives it: the file's, else its XMP sidecar's, else the + /// file's modification time. + pub captured: Option, } #[derive(Clone, Debug, Default, Serialize)] diff --git a/crates/engine/src/lightroom_catalog.rs b/crates/engine/src/lightroom_catalog.rs index e77192b78..0dfef543b 100644 --- a/crates/engine/src/lightroom_catalog.rs +++ b/crates/engine/src/lightroom_catalog.rs @@ -944,7 +944,7 @@ pub(crate) fn apply_prepared(s: &mut crate::Session, mut data: CatalogImport, co validate(&data).map_err(error)?; let mut warnings = data.warnings.clone(); let duplicates: HashMap<_, _> = - files.duplicates.iter().filter_map(|d| d.existing.map(|id| (key(&d.path), (PhotoId(id), d.file_meta.as_deref())))).collect(); + files.duplicates.iter().filter_map(|d| d.existing.map(|id| (key(&d.path), (PhotoId(id), d.file.as_deref())))).collect(); let mut by_path: HashMap<_, _> = s.catalog.photos().filter(|p| p.copy_of.is_none()).filter_map(|p| source_path(p).map(|path| (key(path), p.id))).collect(); let mut ids = HashMap::new(); @@ -1001,14 +1001,15 @@ pub(crate) fn apply_prepared(s: &mut crate::Session, mut data: CatalogImport, co // a new copy of a master in Recently Deleted is imported, not trashed with it p.deleted = false; p - } else if let Some((duplicate, file_meta)) = + } else if let Some((duplicate, file)) = duplicates.get(&key(&src.path)).and_then(|(id, meta)| s.catalog.photo(*id).cloned().map(|p| (p, *meta))) { // Catalogs can contain distinct originals with identical bytes and independent edits. // The new record takes only what the bytes determine from the photo with the same // bytes, never that photo's own state: not its Recently Deleted (#2589), virtual copy, - // edits, history, versions, ratings or the metadata a person writes. Its metadata is - // what its own file carries, as a file import reads it (copyright, creator…). + // edits, history, versions, ratings, capture time or the metadata a person writes. Its + // metadata and capture time are what its own file gives it, as a file import reads them + // (copyright, creator…); the catalog's capture time, when it has one, applies on top. let name = Path::new(&src.path).file_name().map(|n| n.to_string_lossy().to_string()).unwrap_or_else(|| src.path.clone()); let mut p = Photo::new( s.catalog.alloc_photo_id(), @@ -1021,18 +1022,19 @@ pub(crate) fn apply_prepared(s: &mut crate::Session, mut data: CatalogImport, co ); p.kind = duplicate.kind; p.file_size = duplicate.file_size; - p.captured = duplicate.captured.clone(); p.duration = duplicate.duration; p.as_shot_wb = duplicate.as_shot_wb; p.content_hash = duplicate.content_hash.clone(); p.embedded_lens = duplicate.embedded_lens; p.preview_only = duplicate.preview_only.clone(); p.preview_checked = duplicate.preview_checked.clone(); - if let Some(meta) = file_meta { - p.meta = meta.clone(); + if let Some(file) = file { + p.meta = file.meta.clone(); + p.captured = file.captured.clone(); } else { // (the file wasn't read: only the exposure description, which no command edits; - // not GPS, which placing a photo on the map does) + // not GPS, which placing a photo on the map does, nor the capture time, which + // Edit Capture Time does) let m = &duplicate.meta; p.meta.camera = m.camera.clone(); p.meta.lens = m.lens.clone(); diff --git a/crates/engine/src/lightroom_catalog/tests.rs b/crates/engine/src/lightroom_catalog/tests.rs index 34270621c..aa380774c 100644 --- a/crates/engine/src/lightroom_catalog/tests.rs +++ b/crates/engine/src/lightroom_catalog/tests.rs @@ -209,7 +209,10 @@ fn a_content_duplicate_of_a_trashed_photo_or_virtual_copy_is_a_new_live_photo() reason: "content", existing_deleted: case == "trashed", // what the file itself carries - file_meta: Some(Box::new(lightcraft_catalog::Meta { camera: "Same Camera".into(), ..Default::default() })), + file: Some(Box::new(crate::import::FileFacts { + meta: lightcraft_catalog::Meta { camera: "Same Camera".into(), ..Default::default() }, + captured: None, + })), }], ..Default::default() }; @@ -270,7 +273,7 @@ fn a_content_duplicate_raw_starts_from_its_camera_defaults() { existing: Some(other.0), reason: "content", existing_deleted: true, - file_meta: None, + file: None, }], ..Default::default() }; @@ -294,11 +297,11 @@ fn a_content_duplicate_raw_starts_from_its_camera_defaults() { 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 content duplicate keeps the metadata its file carries (copyright, rights, creator), as a file -/// import of it would, when the catalog's XMP doesn't restate it; it reads the file's, not the -/// other photo's, which a person may have edited (#2589). +/// A content duplicate keeps the metadata its file carries (copyright, rights, creator) and the +/// capture time its file gives it, as a file import of it would, when the catalog doesn't restate +/// them; it reads the file's, not the other photo's, which a person may have edited (#2589). #[test] -fn a_content_duplicate_keeps_the_rights_metadata_in_its_file() { +fn a_content_duplicate_keeps_the_rights_metadata_and_capture_time_in_its_file() { let dir = std::env::temp_dir().join(format!("lightcraft-lrcat-duplicate-rights-{}", std::process::id())); let _ = std::fs::remove_dir_all(&dir); std::fs::create_dir_all(dir.join("Photos")).unwrap(); @@ -312,6 +315,7 @@ fn a_content_duplicate_keeps_the_rights_metadata_in_its_file() { p.meta.copyright = "Other photo's notice".into(); p.meta.creator = "Other photo's creator".into(); p.meta.gps = Some((1.0, 2.0)); + p.captured = Some("1999-12-31T23:00:00".into()); }); let file_meta = lightcraft_catalog::Meta { camera: "Same Camera".into(), @@ -329,6 +333,7 @@ fn a_content_duplicate_keeps_the_rights_metadata_in_its_file() { height: 10, content_hash: Some("same-bytes".into()), meta: probed.clone(), + captured: Some("2024-05-06T07:08:09".into()), ..Default::default() }) })); @@ -345,6 +350,8 @@ fn a_content_duplicate_keeps_the_rights_metadata_in_its_file() { "the file's own metadata: {r}" ); assert_eq!(m.gps, None, "nothing from the other photo's record"); + // the catalog row has no capture time: the file's, not the other photo's edited one + assert_eq!(new.captured.as_deref(), Some("2024-05-06T07:08:09"), "{r}"); std::fs::remove_dir_all(dir).unwrap(); } /// A catalog photo matched (by path) to a library photo in Recently Deleted leaves it there, as