From 745976e95c2e8343d1c842a387e78f362a566e24 Mon Sep 17 00:00:00 2001 From: ronibalthazar <61232335+ronibalthazar@users.noreply.github.com> Date: Sat, 24 Oct 2026 09:38:23 +0100 Subject: [PATCH] LR-LIB-VIRTUALCOPY: check numbering before copy creation --- crates/engine/src/cmd/organize.rs | 51 ++++++++++-- crates/engine/src/tests_organize.rs | 122 +++++++++++++++++++++++++++- docs/gaps.md | 3 +- docs/parity-checklist.md | 5 +- docs/whats-new.md | 6 ++ 5 files changed, 177 insertions(+), 10 deletions(-) diff --git a/crates/engine/src/cmd/organize.rs b/crates/engine/src/cmd/organize.rs index dbd0aa842..84d82c7c9 100644 --- a/crates/engine/src/cmd/organize.rs +++ b/crates/engine/src/cmd/organize.rs @@ -3,6 +3,7 @@ use lightcraft_catalog::{HistoryStep, Op, PhotoId, Stack, StackId}; use serde_json::{Value, json}; +use std::collections::HashMap; use super::{CommandSpec, always, bad, bool_or, cmd, f64_or, has_selection, str_param}; use crate::{Result, Selection, Session}; @@ -63,18 +64,53 @@ fn copy_number(name: Option<&str>) -> u32 { name.and_then(|n| n.strip_prefix("Copy ")).and_then(|n| n.trim().parse().ok()).unwrap_or(0) } +/// Resolve every requested name before creating copies, so an exhausted counter cannot leave a +/// partially completed batch. Explicit names do not depend on the numeric sequence. +fn virtual_copy_names(s: &Session, targets: &[PhotoId], name: Option<&str>) -> Result>> { + if let Some(name) = name { + return Ok(targets.iter().map(|id| s.catalog.photo(*id).map(|_| name.to_string())).collect()); + } + + let mut last_by_master = HashMap::new(); + for id in targets { + if let Some(photo) = s.catalog.photo(*id) { + last_by_master.entry(photo.copy_of.unwrap_or(*id)).or_insert(0_u32); + } + } + for photo in s.catalog.photos() { + if let Some(master) = photo.copy_of + && let Some(last) = last_by_master.get_mut(&master) + { + *last = (*last).max(copy_number(photo.copy_name.as_deref())); + } + } + + targets + .iter() + .map(|id| { + let Some(photo) = s.catalog.photo(*id) else { return Ok(None) }; + let master = photo.copy_of.unwrap_or(*id); + let last = last_by_master.get_mut(&master).ok_or_else(|| bad("photo.virtualCopy", "could not resolve the virtual-copy master"))?; + let number = last.checked_add(1).ok_or_else(|| { + bad("photo.virtualCopy", format!("default virtual-copy numbering is exhausted for master {}; provide an explicit name", master.0)) + })?; + *last = number; + Ok(Some(format!("Copy {number}"))) + }) + .collect() +} + /// Create one virtual copy of `src`: a new photo sharing its file, with the same settings and /// metadata, added to the same albums and stacked right after it (the stack is expanded so the -/// copy is visible). `name` replaces the default "Copy N". -fn virtual_copy(s: &mut Session, src: PhotoId, name: Option<&str>) -> Result> { +/// copy is visible). `name` is resolved by the command before any copies are created. +fn virtual_copy(s: &mut Session, src: PhotoId, name: &str) -> Result> { let Some(orig) = s.catalog.photo(src).cloned() else { return Ok(None) }; let master = orig.copy_of.unwrap_or(src); - let n = 1 + s.catalog.photos().filter(|p| p.copy_of == Some(master)).map(|p| copy_number(p.copy_name.as_deref())).max().unwrap_or(0); let id = s.catalog.alloc_photo_id(); let mut c = (*orig).clone(); c.id = id; c.copy_of = Some(master); - c.copy_name = Some(name.map_or_else(|| format!("Copy {n}"), str::to_string)); + c.copy_name = Some(name.to_string()); c.versions.clear(); c.history = vec![HistoryStep { label: "Virtual Copy".into(), settings: c.develop.clone() }]; c.deleted = false; @@ -113,9 +149,12 @@ pub fn specs() -> Vec { |s, p| { let targets = s.targets(p)?; let name = str_param(p, "name").filter(|n| !n.trim().is_empty()).map(str::to_string); + let names = virtual_copy_names(s, &targets, name.as_deref())?; let mut made = Vec::new(); - for t in targets { - if let Some(id) = virtual_copy(s, t, name.as_deref())? { + for (t, name) in targets.into_iter().zip(names) { + if let Some(name) = name + && let Some(id) = virtual_copy(s, t, &name)? + { made.push(id); } } diff --git a/crates/engine/src/tests_organize.rs b/crates/engine/src/tests_organize.rs index ea0414256..efd345cc6 100644 --- a/crates/engine/src/tests_organize.rs +++ b/crates/engine/src/tests_organize.rs @@ -4,7 +4,7 @@ use lightcraft_catalog::Flag; use serde_json::json; -use crate::{LibrarySource, Session}; +use crate::{EngineError, LibrarySource, Session}; fn temp_dir(tag: &str) -> std::path::PathBuf { let d = std::env::temp_dir().join(format!("lc-organize-{tag}-{}", std::process::id())); @@ -18,6 +18,13 @@ fn open(dir: &std::path::Path) -> Session { s } +fn leave_redo_entry(s: &mut Session, id: lightcraft_catalog::PhotoId) { + let rating = if s.catalog.photo(id).unwrap().rating == 5 { 4 } else { 5 }; + s.execute("photo.rate", &json!({"ids": [id.0], "rating": rating})).unwrap(); + s.execute("edit.undo", &json!({})).unwrap(); + assert!(!s.redo.is_empty()); +} + #[test] fn smart_album_from_view_updates_live_and_persists() { let dir = temp_dir("smart"); @@ -141,6 +148,119 @@ fn virtual_copies_are_independent_and_persist() { let _ = std::fs::remove_dir_all(&dir); } +#[test] +fn virtual_copy_explicit_name_bypasses_exhausted_counter() { + let mut s = Session::with_demo(); + let visible = s.visible_cloned(); + let src = visible[0]; + s.execute("photo.virtualCopy", &json!({"ids": [src.0], "name": "Copy 4294967295"})).unwrap(); + + let result = s.execute("photo.virtualCopy", &json!({"ids": [src.0], "name": "Proof copy"})); + assert!(result.is_ok(), "an explicit name does not need a numeric suffix: {result:?}"); + let id = lightcraft_catalog::PhotoId(result.unwrap()["ids"][0].as_u64().unwrap()); + let copy = s.catalog.photo(id).unwrap(); + assert_eq!(copy.copy_of, Some(src)); + assert_eq!(copy.copy_name.as_deref(), Some("Proof copy")); +} + +#[test] +fn virtual_copy_exhausted_counter_returns_bad_params() { + let mut s = Session::with_demo(); + let visible = s.visible_cloned(); + let src = visible[0]; + s.execute("photo.virtualCopy", &json!({"ids": [src.0], "name": "Copy 4294967295"})).unwrap(); + leave_redo_entry(&mut s, visible[1]); + + let before = s.catalog.to_snapshot(); + let revision = s.catalog.revision; + let selection = (s.selection.ids.clone(), s.selection.active); + let commits = s.commits(); + let undo = s.undo.iter().map(|entry| entry.label.clone()).collect::>(); + let redo = s.redo.iter().map(|entry| entry.label.clone()).collect::>(); + + let result = s.execute("photo.virtualCopy", &json!({"ids": [src.0]})); + let actionable = + matches!(&result, Err(EngineError::BadParams { cmd, msg }) if cmd == "photo.virtualCopy" && msg.contains("provide an explicit name")); + assert_eq!(s.catalog.to_snapshot(), before, "exhaustion must not allocate IDs or mutate the catalog"); + assert_eq!(s.catalog.revision, revision); + assert_eq!((s.selection.ids.clone(), s.selection.active), selection); + assert_eq!(s.commits(), commits); + assert_eq!(s.undo.iter().map(|entry| entry.label.clone()).collect::>(), undo); + assert_eq!(s.redo.iter().map(|entry| entry.label.clone()).collect::>(), redo); + assert!(actionable, "expected actionable parameter error, got {result:?}"); +} + +#[test] +fn virtual_copy_numbering_failure_does_not_partially_change_batch() { + let mut s = Session::with_demo(); + let visible = s.visible_cloned(); + let (first, second) = (visible[0], visible[1]); + s.execute("photo.virtualCopy", &json!({"ids": [second.0], "name": "Copy 4294967295"})).unwrap(); + leave_redo_entry(&mut s, visible[2]); + s.execute("library.select", &json!({"ids": [first.0, second.0], "active": first.0})).unwrap(); + + let before = s.catalog.to_snapshot(); + let revision = s.catalog.revision; + let selection = (s.selection.ids.clone(), s.selection.active); + let commits = s.commits(); + let undo = s.undo.iter().map(|entry| entry.label.clone()).collect::>(); + let redo = s.redo.iter().map(|entry| entry.label.clone()).collect::>(); + + let result = s.execute("photo.virtualCopy", &json!({"ids": [first.0, second.0]})); + let bad_params = matches!(&result, Err(EngineError::BadParams { cmd, .. }) if cmd == "photo.virtualCopy"); + assert_eq!(s.catalog.to_snapshot(), before, "a later exhausted master must not leave earlier copies behind"); + assert_eq!(s.catalog.revision, revision); + assert_eq!((s.selection.ids.clone(), s.selection.active), selection); + assert_eq!(s.commits(), commits); + assert_eq!(s.undo.iter().map(|entry| entry.label.clone()).collect::>(), undo); + assert_eq!(s.redo.iter().map(|entry| entry.label.clone()).collect::>(), redo); + assert!(bad_params, "expected batch preflight error, got {result:?}"); +} + +#[test] +fn virtual_copy_batch_reserves_numbers_for_the_same_master() { + let mut s = Session::with_demo(); + let visible = s.visible_cloned(); + let src = visible[0]; + let near_limit = lightcraft_catalog::PhotoId( + s.execute("photo.virtualCopy", &json!({"ids": [src.0], "name": "Copy 4294967294"})).unwrap()["ids"][0].as_u64().unwrap(), + ); + leave_redo_entry(&mut s, visible[1]); + s.execute("library.select", &json!({"ids": [src.0, near_limit.0], "active": src.0})).unwrap(); + + let before = s.catalog.to_snapshot(); + let revision = s.catalog.revision; + let selection = (s.selection.ids.clone(), s.selection.active); + let commits = s.commits(); + let undo = s.undo.iter().map(|entry| entry.label.clone()).collect::>(); + let redo = s.redo.iter().map(|entry| entry.label.clone()).collect::>(); + + let result = s.execute("photo.virtualCopy", &json!({"ids": [src.0, near_limit.0]})); + let bad_params = matches!(&result, Err(EngineError::BadParams { cmd, .. }) if cmd == "photo.virtualCopy"); + assert_eq!(s.catalog.to_snapshot(), before, "same-master reservations must be checked before either copy is added"); + assert_eq!(s.catalog.revision, revision); + assert_eq!((s.selection.ids.clone(), s.selection.active), selection); + assert_eq!(s.commits(), commits); + assert_eq!(s.undo.iter().map(|entry| entry.label.clone()).collect::>(), undo); + assert_eq!(s.redo.iter().map(|entry| entry.label.clone()).collect::>(), redo); + assert!(bad_params, "a shared master has only one number left: {result:?}"); +} + +#[test] +fn virtual_copy_last_valid_number_can_be_created_and_undone() { + let mut s = Session::with_demo(); + let src = s.visible_cloned()[0]; + s.execute("photo.virtualCopy", &json!({"ids": [src.0], "name": "Copy 4294967294"})).unwrap(); + + let result = s.execute("photo.virtualCopy", &json!({"ids": [src.0]})).unwrap(); + let id = lightcraft_catalog::PhotoId(result["ids"][0].as_u64().unwrap()); + assert_eq!(s.catalog.photo(id).unwrap().copy_name.as_deref(), Some("Copy 4294967295")); + assert_eq!(s.catalog.photo(id).unwrap().copy_of, Some(src)); + + s.execute("edit.undo", &json!({})).unwrap(); + assert!(s.catalog.photo(id).is_none(), "one undo removes the last valid copy"); +} + /// Issue #1493: soft-delete must drop the copy from the stack so `1/2` does not linger, and /// deleting `2/3` renumbers the rest to `1/2` `2/2`. Undo restores the stack. #[test] diff --git a/docs/gaps.md b/docs/gaps.md index 329dcf531..b032d5dff 100644 --- a/docs/gaps.md +++ b/docs/gaps.md @@ -1,6 +1,6 @@ # Gaps: where LightCraft falls short of Lightroom -> **Last reviewed:** 2026-10-11 · **Last updated:** 2026-10-23 · **Change:** minor (gap 28: ⌘L Enable Filters switches the filter off and on; gap 28: F8 shows / hides the right panel; import review album hierarchy, #2362; gap 20: Minolta MRW decodes; gap 10: 551 models verified; gap 15: Print Page Options — page numbers, page info, crop marks; gap 15: Print Page Options — page numbers, page info, crop marks; gap 28: `\` shows / hides the Filter Bar in the grids; gap 21: Indonesian interface shipped, every message but What's New; gap 27: the Windows x64 installer verified on Windows 11; the Settings → Apps uninstall confirmation remains) +> **Last reviewed:** 2026-10-11 · **Last updated:** 2026-10-24 · **Change:** patch (LR-LIB-VIRTUALCOPY: default number exhaustion is validated before copy creation; explicit names bypass suffix allocation; status and counts unchanged; gap 28: ⌘L Enable Filters switches the filter off and on; gap 28: F8 shows / hides the right panel; import review album hierarchy, #2362; gap 20: Minolta MRW decodes; gap 10: 551 models verified; gap 15: Print Page Options — page numbers, page info, crop marks; gap 15: Print Page Options — page numbers, page info, crop marks; gap 28: `\` shows / hides the Filter Bar in the grids; gap 21: Indonesian interface shipped, every message but What's New; gap 27: the Windows x64 installer verified on Windows 11; the Settings → Apps uninstall confirmation remains) Every known shortfall, one entry each, ranked by user impact (how many Lightroom users hit it, and how hard it blocks them), then by effort. **Agents pick work from the top** (the first entry nobody is @@ -84,6 +84,7 @@ Beta-blocking gaps (B) total **~230–430 h**; with the stability and platform w | Date | Change | Summary | |---|---|---| +| 2026-10-24 | patch | LR-LIB-VIRTUALCOPY: default numbering exhaustion is an actionable validation error before batch mutation; explicit names bypass suffix allocation; ranked gaps and readiness counts unchanged | | 2026-10-22 | patch | XMP namespace-prefix collisions no longer overwrite foreign metadata; ranked gaps and estimates unchanged | | 2026-10-22 | patch | Issue #2519: PSD/PSB EXIF resource 1058 and XMP resource 1060 now use the common metadata extractor, and `.psb` files are accepted by import; no ranked gap or readiness count changes | | 2026-10-22 | minor | Gap 30 added: multi-select metadata editing — Info panel and All Metadata dialog show one photo's values with no mixed-value indication | diff --git a/docs/parity-checklist.md b/docs/parity-checklist.md index 9790523e1..430d5c296 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-22 · **Change:** minor (MENU-BAR: on macOS a menu item that yields its key to a focused text field or to shortcut recording really lets it go; LR-MERGE-HDR / LR-MERGE-PANO: Lanczos-3 warps; LR-IMP-LOCAL: browsing from the sidebar records the folder; LR-VIEW-ZOOM: only a full-size drag within 20 ms clears a slow-drag verdict; LR-IMP-ADD-DIALOG: hierarchical import album destination reconciled with main, #2362; KEY-SLIDER-NUDGE: vertical wheel nudges need a click or drag in the row; leaving it disarms them; LR-EDIT-OPTICS-PROFILE: Fujifilm nine-knot lens tables read at the plain radius; LR-VIEW-ZOOM: only a full-size drag within 20 ms clears a slow-drag verdict; LRC-LIB-LIBFOLDERS, LR-BEHAV-BG-TASKS: Synchronize Folder's scan shows its folder listing and stops it on ✕; LRC-LIB-LIBFOLDERS: Synchronize Folder says a folder is up to date; LR-LIB-STACK: filmstrip stack badge click; LRC-LIB-REFVIEW / CATALOG-SWITCH: Escape preserves edits; catalog-local pins; deleted and malformed references refused; LRC-LIB-COMPARE: the Info panel labels the active photo while culling, #2398; KEYC-RATING: `\` shows / hides the Filter Bar in the grids; LR-VIEW-PHOTOGRID: the grid reads only the photos an import batch or an edit touched, not the whole list; earlier: LRC-PRINT-PAGE: Print Page Options; LR-META-XMP: fewer false unimported-XMP warnings) · **Target:** Adobe Lightroom Classic 15.5 / Lightroom 9.6 +> **Last reviewed:** 2026-10-10 · **Last updated:** 2026-10-24 · **Change:** patch (LR-LIB-VIRTUALCOPY: default numbering is preflighted across the requested batch and explicit names bypass suffix allocation; status and counts unchanged; MENU-BAR: on macOS a menu item that yields its key to a focused text field or to shortcut recording really lets it go; LR-MERGE-HDR / LR-MERGE-PANO: Lanczos-3 warps; LR-IMP-LOCAL: browsing from the sidebar records the folder; LR-VIEW-ZOOM: only a full-size drag within 20 ms clears a slow-drag verdict; LR-IMP-ADD-DIALOG: hierarchical import album destination reconciled with main, #2362; KEY-SLIDER-NUDGE: vertical wheel nudges need a click or drag in the row; leaving it disarms them; LR-EDIT-OPTICS-PROFILE: Fujifilm nine-knot lens tables read at the plain radius; LR-VIEW-ZOOM: only a full-size drag within 20 ms clears a slow-drag verdict; LRC-LIB-LIBFOLDERS, LR-BEHAV-BG-TASKS: Synchronize Folder's scan shows its folder listing and stops it on ✕; LRC-LIB-LIBFOLDERS: Synchronize Folder says a folder is up to date; LR-LIB-STACK: filmstrip stack badge click; LRC-LIB-REFVIEW / CATALOG-SWITCH: Escape preserves edits; catalog-local pins; deleted and malformed references refused; LRC-LIB-COMPARE: the Info panel labels the active photo while culling, #2398; KEYC-RATING: `\` shows / hides the Filter Bar in the grids; LR-VIEW-PHOTOGRID: the grid reads only the photos an import batch or an edit touched, not the whole list; earlier: LRC-PRINT-PAGE: Print Page Options; LR-META-XMP: fewer false unimported-XMP warnings) · **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 @@ -117,7 +117,7 @@ differences and missing bindings are in [ui-parity.md](ui-parity.md#shortcuts). | LR-LIB-CULL | Assisted culling | P2 | 🟡 | `cmd:photo.analyze`, `cmd:dialog.cull`, `crates/pipeline/src/cull.rs` | Photo ▸ Assisted Culling…: focus score (0–100), clipping, burst grouping (look-alike shots within 10 s) with the sharpest marked; reject below a focus score, pick each burst's best; classical measures — no eyes-closed / expression detection | | LR-LIB-ACTIVITY | Comments & likes | OOS | 🚫 | | | | LR-LIB-QUICKCOLL | Quick collection [Classic] | P2 | ✅ | `cmd:album.toggleTarget`, `cmd:album.clearQuick` | B in the grids | -| LR-LIB-VIRTUALCOPY | Virtual copies [Classic] | P1 | ✅ | `cmd:photo.virtualCopy`, `cmd:photo.delete`, `crates/engine/src/cmd/organize.rs` | “Copy N” badge, stacked with the original, same albums, no XMP writes; deleting a copy dissolves or renumbers the stack (#1493); no “Set Copy as Master” | +| LR-LIB-VIRTUALCOPY | Virtual copies [Classic] | P1 | ✅ | `cmd:photo.virtualCopy`, `cmd:photo.delete`, `crates/engine/src/cmd/organize.rs` | “Copy N” badge, stacked with the original, same albums, no XMP writes; default numbers are checked across the batch before copies are created, while explicit names bypass the counter; deleting a copy dissolves or renumbers the stack (#1493); no “Set Copy as Master” | ## C. Views & navigation (VIEW) @@ -798,6 +798,7 @@ Latin stays upright. Advanced Japanese composition remains open. Regression cove | Date | Change | Summary | |---|---|---| +| 2026-10-24 | patch | LR-LIB-VIRTUALCOPY: copy names are resolved before batch mutation, numeric exhaustion returns BadParams, and explicit names bypass the counter; status and feature counts unchanged | | 2026-10-22 | patch | LR-META-XMP: namespace URI determines ownership; foreign namespaces using recognized prefixes survive saves and cannot supply native settings or ratings; status and counts unchanged | | 2026-10-22 | patch | LR-EXP-PSD, LR-META-EXIF-FULL: PSD/PSB imports and image resources 1058/1060 now supply EXIF and XMP to metadata readers and All Metadata; Original with Remove Location Info refuses detected PSD/PSB GPS; statuses and feature counts unchanged | | 2026-10-21 | patch | LR-IMP-ADD-DIALOG: reconcile #2362 with current main; deleted-album guard and hierarchical picker retained; counts unchanged | diff --git a/docs/whats-new.md b/docs/whats-new.md index c09dddc09..110c9cc59 100644 --- a/docs/whats-new.md +++ b/docs/whats-new.md @@ -1,5 +1,11 @@ # What's new in LightCraft +## 2026-10-24 + +### Virtual-copy numbering stops cleanly at its limit +- Creating a numbered virtual copy now reports when the `Copy N` sequence is exhausted, before any copies in a + batch are created. An explicit name remains available even when the numeric sequence is full. + ## 2026-10-23 ### Tab leaves the name field in New Album, New Smart Album and the name prompts