From f1c023ed3deceb85ff0342a254ac815538ba3fba Mon Sep 17 00:00:00 2001 From: Alex Chambers Date: Mon, 12 Oct 2026 09:24:29 +0200 Subject: [PATCH] LR-META-XMP: Read Metadata from File doesn't auto-write the sidecar it just read (#934) With XMP auto-write on, photo.readMetadataFromFile committed what it read and the command handler then wrote the sidecar back, adding lc:settings to a sidecar that had only another app's crs: settings. Later reads prefer lc:settings, so LRTimelapse's next crs: edits were ignored. The read now sets the existing one-command skip_auto_write switch (as photo.removeRegion does); edits made afterwards are still auto-written. Tests: a read leaves the sidecar byte for byte and a later develop edit is still written; the issue's crs:Exposure2012 0.5 -> 1.2 round trip is read. Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/cmd/xmp.rs | 6 +++- crates/engine/src/tests_xmp.rs | 65 ++++++++++++++++++++++++++++++++++ docs/parity-checklist.md | 5 +-- docs/xmp-interop.md | 2 +- 4 files changed, 74 insertions(+), 4 deletions(-) diff --git a/crates/engine/src/cmd/xmp.rs b/crates/engine/src/cmd/xmp.rs index 317a87aca..6af62637f 100644 --- a/crates/engine/src/cmd/xmp.rs +++ b/crates/engine/src/cmd/xmp.rs @@ -49,6 +49,10 @@ fn read(s: &mut Session, p: &Value) -> Result { } if !ops.is_empty() { s.commit("Read Metadata from File", Op::Batch { ops })?; + // Don't auto-write the sidecars just read: that would add our `lc:settings` to one that had + // only another app's `crs:` settings, and later reads prefer `lc:settings`, so that app's + // next edits would be ignored (issue #934). The next edit, or Save Metadata, writes them. + s.skip_auto_write = true; } let mut result = json!({"read": read, "failed": failed}); if !warnings.is_empty() { @@ -90,7 +94,7 @@ pub fn specs() -> Vec { "Read Metadata from File", ["Photo"], None, - "{ids?} — reads each photo's XMP sidecar (or a raw/DNG's embedded XMP): metadata and develop settings, one undo step → {read, failed, warnings?: [{path, code, profile?, mode?, fields?}]}", + "{ids?} — reads each photo's XMP sidecar (or a raw/DNG's embedded XMP): metadata and develop settings, one undo step; never writes the sidecar, even with auto-write on → {read, failed, warnings?: [{path, code, profile?, mode?, fields?}]}", has_selection, read ), diff --git a/crates/engine/src/tests_xmp.rs b/crates/engine/src/tests_xmp.rs index c768078ce..50223435e 100644 --- a/crates/engine/src/tests_xmp.rs +++ b/crates/engine/src/tests_xmp.rs @@ -175,6 +175,71 @@ fn foreign_crs_sidecar_on_import_and_read_from_file() { let _ = std::fs::remove_dir_all(&src); } +/// A sidecar with only `crs:` develop settings, as LRTimelapse writes them (issue #934). +fn crs_exposure_sidecar(exposure: &str) -> String { + format!( + r#""# + ) +} + +/// Issue #934: with auto-write on, Read Metadata from File wrote what it had just read straight back +/// into the sidecar, adding our `lc:settings`. Reading leaves the file byte for byte; an edit made +/// after it is still written. +#[test] +fn reading_metadata_leaves_the_sidecar_alone_with_auto_write_on() { + let src = temp_dir("read-no-autowrite"); + write_png(&src.join("frame.png"), 6); + let sidecar = src.join("frame.xmp"); + std::fs::write(&sidecar, crs_exposure_sidecar("0.5")).unwrap(); + let mut s = Session::new().with_fs(); + s.execute("library.import", &json!({"paths": [src.to_string_lossy()]})).unwrap(); + let id = only(&s); + assert_eq!(s.catalog.photo(id).unwrap().develop.light.exposure, 0.5); + // something for the read to change back (auto-write is still off, so the file stays as it is) + s.execute("develop.reset", &json!({"ids": [id.0]})).unwrap(); + s.execute("library.xmpPreferences", &json!({"autoWrite": true})).unwrap(); + let before = std::fs::read(&sidecar).unwrap(); + let r = s.execute("photo.readMetadataFromFile", &json!({"ids": [id.0]})).unwrap(); + assert_eq!(r["read"].as_array().unwrap().len(), 1, "{r}"); + assert_eq!(s.catalog.photo(id).unwrap().develop.light.exposure, 0.5, "the sidecar was read"); + let now = std::fs::read(&sidecar).unwrap(); + assert!(now == before, "reading leaves the sidecar byte for byte, but it is now:\n{}", String::from_utf8_lossy(&now)); + // a develop edit afterwards is still written (only the read is exempt) + s.execute("develop.set", &json!({"ids": [id.0], "control": "light.exposure", "value": 0.9})).unwrap(); + let after = std::fs::read_to_string(&sidecar).unwrap(); + match crate::sidecar::parse_sidecar(&after, crate::crs::Target::Rendered).unwrap().develop { + Some(crate::sidecar::DevelopPatch::Full(d)) => assert_eq!(d.light.exposure, 0.9), + other => panic!("the edit wasn't auto-written: {other:?}\n{after}"), + } + assert!(after.contains(r#"crs:Exposure2012="0.5""#), "the other app's settings are kept: {after}"); + let _ = std::fs::remove_dir_all(&src); +} + +/// Issue #934, the round trip the write-back broke: after a read with auto-write on, another app +/// changes `crs:Exposure2012` from 0.5 to 1.2 (keeping the rest of the file), and the next read +/// takes 1.2. +#[test] +fn a_crs_change_after_a_read_is_taken_with_auto_write_on() { + let src = temp_dir("read-then-crs-change"); + write_png(&src.join("frame.png"), 7); + let sidecar = src.join("frame.xmp"); + std::fs::write(&sidecar, crs_exposure_sidecar("0.5")).unwrap(); + let mut s = Session::new().with_fs(); + s.execute("library.import", &json!({"paths": [src.to_string_lossy()]})).unwrap(); + let id = only(&s); + s.execute("library.xmpPreferences", &json!({"autoWrite": true})).unwrap(); + s.execute("photo.readMetadataFromFile", &json!({"ids": [id.0]})).unwrap(); + assert_eq!(s.catalog.photo(id).unwrap().develop.light.exposure, 0.5); + // the other app edits the file as it is now + let text = std::fs::read_to_string(&sidecar).unwrap(); + let changed = text.replace(r#"crs:Exposure2012="0.5""#, r#"crs:Exposure2012="1.2""#); + assert_ne!(changed, text); + std::fs::write(&sidecar, changed).unwrap(); + s.execute("photo.readMetadataFromFile", &json!({"ids": [id.0]})).unwrap(); + assert_eq!(s.catalog.photo(id).unwrap().develop.light.exposure, 1.2, "the second read takes the other app's exposure"); + let _ = std::fs::remove_dir_all(&src); +} + /// Lightroom's default profiles (`Adobe Standard`, `Adobe Color`) are named by nearly every sidecar /// and our own starting look stands in for them: importing one gives no camera-profile warning. #[test] diff --git a/docs/parity-checklist.md b/docs/parity-checklist.md index 77ae26e4c..c43686b1a 100644 --- a/docs/parity-checklist.md +++ b/docs/parity-checklist.md @@ -1,6 +1,6 @@ # Lightroom parity checklist -> **Last reviewed:** 2026-10-10 · **Last updated:** 2026-10-11 · **Change:** minor (rapid slider entry; LR-PROF-CAMERACOLOR: tone curve from black-and-white camera JPEGs; LR-VIEW-ZOOM, LR-VIEW-COMPARE: Compare and Reference zoom windows; LR-IMP-FORMATS, LR-IMP-CAMERA-COVERAGE: EOS R5 Mark II CR3 decodes; renamed from `parity.md`; Top gaps moved to gaps.md, shortcut notes to ui-parity.md; filesystem history retry/Undo/Redo evidence; renamed from `parity.md`; Top gaps moved to gaps.md, shortcut notes to ui-parity.md) · **Target:** Adobe Lightroom Classic 15.5 / Lightroom 9.6 +> **Last reviewed:** 2026-10-10 · **Last updated:** 2026-10-12 · **Change:** minor (LR-META-XMP: Read Metadata from File no longer writes the sidecar back (#934); rapid slider entry; LR-PROF-CAMERACOLOR: tone curve from black-and-white camera JPEGs; LR-VIEW-ZOOM, LR-VIEW-COMPARE: Compare and Reference zoom windows; LR-IMP-FORMATS, LR-IMP-CAMERA-COVERAGE: EOS R5 Mark II CR3 decodes; renamed from `parity.md`; Top gaps moved to gaps.md, shortcut notes to ui-parity.md; filesystem history retry/Undo/Redo evidence; renamed from `parity.md`; Top gaps moved to gaps.md, shortcut notes to ui-parity.md) · **Target:** Adobe Lightroom Classic 15.5 / Lightroom 9.6 The measured, row-by-row checklist: one row per Lightroom feature, menu item and shortcut, what we have and what is missing. It answers *does the feature exist?* (feature breadth). Whether it is good @@ -167,7 +167,7 @@ differences and missing bindings are in [ui-parity.md](ui-parity.md#shortcuts). | LR-META-COPYRIGHT-DEFAULT | Default copyright on import | P1 | ✅ | `cmd:library.preferences` (`import.copyright`, `import.creator`), `crates/ui-egui/src/panels/settings.rs` | Settings → Import → Metadata; fills only photos without their own | | LR-META-LOCATION | Location editing | P2 | 🟡 | `cmd:photo.setMeta` (`location`, `city`, `state`, `country`, `gps`), `cmd:metadata.reverseGeocode` | place fields and GPS (decimal or degrees / minutes / seconds) edited in Info, read/written as IPTC Core / Photoshop / EXIF XMP; Show on Map opens OpenStreetMap; Fill Location fills empty City / State / Country from GPS offline (GeoNames, downloaded on request: `cmd:metadata.downloadPlaceNames`); no embedded map | | LR-META-COPYPASTE | Copy / paste metadata | P2 | ✅ | `cmd:photo.copyMetadata`, `cmd:photo.pasteMetadata` | title, caption, alt text, extended description, copyright (notice, status, usage terms, info URL), creator, place fields, keywords; `fields` picks a subset | -| LR-META-XMP | XMP read/write | P0 | ✅ | `cmd:photo.saveMetadataToFile`, `cmd:photo.readMetadataFromFile`, `cmd:library.xmpPreferences`, `crates/engine/src/sidecar.rs`, `docs/xmp-interop.md` | sidecar wins for metadata; a sidecar capture time (`exif:DateTimeOriginal` / `photoshop:DateCreated` / `xmp:CreateDate`) fills in only when the file has none; saving merges into an existing sidecar (`crates/meta/src/xmp_merge.rs`): another app's `crs:`, `xmpMM:History` and unknown namespaces are kept byte for byte, an unreadable sidecar is backed up first; with stem naming, files sharing a stem (raw + JPEG) get separate sidecars (`.xmp` for all but the raw); a `crs:` custom white balance on a relative-WB raw is shifted from `crs:AsShotTemperature` rather than read as Kelvin, or kept As Shot and reported when the packet has no as-shot white (issue #510); heal and clone spots (`crs:RetouchAreas`, legacy `crs:RetouchInfo`) are read into our spots (`crates/engine/src/crs_masks.rs`, see `docs/xmp-interop.md` → Spot removal) | +| LR-META-XMP | XMP read/write | P0 | ✅ | `cmd:photo.saveMetadataToFile`, `cmd:photo.readMetadataFromFile`, `cmd:library.xmpPreferences`, `crates/engine/src/sidecar.rs`, `docs/xmp-interop.md` | sidecar wins for metadata; a sidecar capture time (`exif:DateTimeOriginal` / `photoshop:DateCreated` / `xmp:CreateDate`) fills in only when the file has none; saving merges into an existing sidecar (`crates/meta/src/xmp_merge.rs`): another app's `crs:`, `xmpMM:History` and unknown namespaces are kept byte for byte, an unreadable sidecar is backed up first; with stem naming, files sharing a stem (raw + JPEG) get separate sidecars (`.xmp` for all but the raw); a `crs:` custom white balance on a relative-WB raw is shifted from `crs:AsShotTemperature` rather than read as Kelvin, or kept As Shot and reported when the packet has no as-shot white (issue #510); heal and clone spots (`crs:RetouchAreas`, legacy `crs:RetouchInfo`) are read into our spots (`crates/engine/src/crs_masks.rs`, see `docs/xmp-interop.md` → Spot removal); Read Metadata from File never writes the sidecar back, even with auto-write on, so another app's later `crs:` edits are still read (issue #934) | | LR-META-EXIF-FULL | Full EXIF/IPTC [Classic] | P1 | ✅ | `cmd:photo.allMetadata`, `cmd:dialog.allMetadata`, `crates/meta/src/tags.rs`, `crates/meta/src/exif.rs`, `crates/meta/src/iptc.rs` | read and written on export; Info ▸ All Metadata…: every TIFF / EXIF / GPS / interop tag (named, common values spelled out) and the XMP fields, searchable; metadata presets | ## F. Edit panel — global adjustments (EDIT) @@ -787,6 +787,7 @@ Latin stays upright. Advanced Japanese composition remains open. Regression cove | Date | Change | Summary | |---|---|---| +| 2026-10-12 | minor | LR-META-XMP: Read Metadata from File no longer auto-writes the sidecar it has just read, which added `lc:settings` and hid another app's later `crs:` edits (#934); status unchanged (✅) | | 2026-10-11 | minor | KEY-SLIDER-TYPE: honor immediate Return/Escape and keep typed keys in the value editor; add regression references | | 2026-10-11 | minor | LR-PROF-CAMERACOLOR: raws with a black-and-white camera JPEG take its tone curve (issue #577) | | 2026-10-11 | minor | LR-VIEW-ZOOM and LR-VIEW-COMPARE: Compare and Reference are sharp at 1:1 (zoom windows per pane, two originals kept decoded while they are open); statuses unchanged | diff --git a/docs/xmp-interop.md b/docs/xmp-interop.md index 3669d1199..5e47b9157 100644 --- a/docs/xmp-interop.md +++ b/docs/xmp-interop.md @@ -10,7 +10,7 @@ other tools), and let LightCraft pick up edits made elsewhere. |---|---| | Name | `.xmp` by default (`IMG_0001.CR3` → `IMG_0001.xmp`); `.xmp` (`IMG_0001.CR3.xmp`) with `naming: "full"`. Reading accepts either (preferred first) and `.XMP`. With stem naming, files sharing a stem don't share a sidecar: see *Shared names* below. | | Write | `photo.saveMetadataToFile {ids?}` (Photo ▸ Save Metadata to File, ⌘S), or automatically after every change with `library.xmpPreferences {autoWrite: true}` (File ▸ Automatically Write Changes into XMP). Slider drags are written once, when the drag ends; undo/redo rewrite the sidecar. An existing sidecar is **merged into, never replaced** (see *Saving into an existing sidecar*). Written atomically (temp file, fsync, rename). The result lists `written`, `merged` and `backups`. | -| Read | On import (`library.import`, the report counts `sidecars`), and `photo.readMetadataFromFile {ids?}` (one undo step). For raw/DNG files without a sidecar, the XMP embedded in the file is used. | +| Read | On import (`library.import`, the report counts `sidecars`), and `photo.readMetadataFromFile {ids?}` (one undo step). For raw/DNG files without a sidecar, the XMP embedded in the file is used. `photo.readMetadataFromFile` never writes the sidecar, even with auto-write on: writing back what it has just read would add `lc:settings`, which later reads prefer over another app's `crs:` edits (issue #934). | | Preferences | `library.xmpPreferences {autoWrite?, naming?: stem\|full}`, stored in the library's `prefs.json`. | What we write (standard namespaces, so other tools can read the metadata):