diff --git a/apps/lightcraft-cli/src/main.rs b/apps/lightcraft-cli/src/main.rs index c8371cbb9..a27e97beb 100644 --- a/apps/lightcraft-cli/src/main.rs +++ b/apps/lightcraft-cli/src/main.rs @@ -935,7 +935,11 @@ fn snapshot(args: &[String]) -> Result<(), String> { fn commands(args: &[String]) -> Result<(), String> { let mut h = Headless::demo(); - let cmds = h.call("engine.commands", json!({}))?; + let mut cmds = h.call("engine.commands", json!({}))?; + // Listed by id so each family (`photo.*`, `mask.*`, …) reads together; the registry keeps its own order. + if let Some(list) = cmds.as_array_mut() { + list.sort_by(|a, b| a["id"].as_str().unwrap_or("").cmp(b["id"].as_str().unwrap_or(""))); + } let mut out = std::io::stdout().lock(); if args.iter().any(|a| a == "--json") { writeln!(out, "{}", serde_json::to_string_pretty(&cmds).unwrap_or_default()).map_err(|e| e.to_string())?; diff --git a/apps/lightcraft-cli/tests/it/cli.rs b/apps/lightcraft-cli/tests/it/cli.rs index 8f56bafe6..1c8910d9d 100644 --- a/apps/lightcraft-cli/tests/it/cli.rs +++ b/apps/lightcraft-cli/tests/it/cli.rs @@ -286,6 +286,38 @@ fn commands_subcommand_lists_registry() { assert!(String::from_utf8_lossy(&o.stdout).contains("light.exposure")); } +#[test] +fn commands_subcommand_lists_commands_sorted_by_id() { + let o = Command::new(BIN).args(["commands", "--json"]).output().unwrap(); + assert!(o.status.success()); + let v: Value = serde_json::from_slice(&o.stdout).unwrap(); + let cmds = v.as_array().unwrap(); + for c in cmds { + for field in ["id", "label", "menu", "shortcut", "params", "enabled"] { + assert!(c.get(field).is_some(), "every command should keep its {field}: {c}"); + } + } + let ids: Vec<&str> = cmds.iter().filter_map(|c| c["id"].as_str()).collect(); + assert_eq!(ids.len(), cmds.len(), "every command should have a string id"); + let unsorted = ids.windows(2).find(|w| w[0] >= w[1]); + assert!(unsorted.is_none(), "--json should list commands sorted by id, but {unsorted:?} are out of order"); + + let o = Command::new(BIN).arg("commands").output().unwrap(); + assert!(o.status.success()); + let text = String::from_utf8_lossy(&o.stdout); + let lines: Vec<&str> = text.lines().collect(); + assert_eq!(lines.len(), 2 * ids.len(), "each command should take an id line and a params line"); + let listed: Vec<&str> = lines.iter().step_by(2).filter_map(|l| l.split_whitespace().next()).collect(); + assert_eq!(listed, ids, "the text output should list the same commands in the same order as --json"); + assert!(lines.iter().skip(1).step_by(2).all(|l| l.trim_start().starts_with("params:")), "each id line should be followed by its params line"); + for (line, c) in lines.iter().step_by(2).zip(cmds) { + assert!(line.contains(c["label"].as_str().unwrap()), "{line:?} should show the label of {c}"); + if let Some(sc) = c["shortcut"].as_str() { + assert!(line.contains(&format!("[{sc}]")), "{line:?} should show the shortcut of {c}"); + } + } +} + #[test] fn snapshot_subcommand_renders_the_ui_headlessly() { let script = tmp("snap.jsonl"); diff --git a/apps/lightcraft/src/native_menu.rs b/apps/lightcraft/src/native_menu.rs index 7f8bd6006..14debc498 100644 --- a/apps/lightcraft/src/native_menu.rs +++ b/apps/lightcraft/src/native_menu.rs @@ -39,18 +39,138 @@ enum Handle { struct Item { handle: Handle, + /// The submenu the item sits in, to swap the item when its accelerator has to go (macOS). + parent: Submenu, cmd: String, params: Value, shortcut: Option, + /// The accelerator the item's shortcut maps to. accel: Option, + /// The accelerator the native item has now: `accel`, or `None` while it yields. + current: Option, label: String, enabled: bool, checked: Option, + /// Whether the item may be replaced to drop its key. Not in the Help menu: a swap there left + /// the old row and added a second one (muda 0.21, macOS 27), so those keys stay. + swappable: bool, +} + +impl Item { + fn native_label(&self) -> String { + lightcraft_ui_egui::menubar::display_item_label(&self.cmd, &self.params, &self.label).replace('&', "&&") + } + + /// Puts `want` on the native item. muda's `set_accelerator(None)` does nothing on macOS (the + /// old key equivalent stays and the menu still takes the key), so there the item is replaced + /// by an identical one without an accelerator, at the same position and with the same id, + /// text, enabled and checked state. Only done when the accelerator changes (and again while a + /// native call keeps failing), never per frame otherwise. + fn set_accelerator(&mut self, want: Option) { + if self.current == want { + return; + } + if cfg!(target_os = "macos") && want.is_none() && self.swappable { + // `current` follows the native item: it changes only when the swap went through, so a + // failed swap (the old item, key and all, is still there) is tried again + let done = self.replace_without_accelerator(); + self.current = settled(self.current, want, done); + return; + } + let done = match &self.handle { + Handle::Plain(h) => h.set_accelerator(want), + Handle::Check(h) => h.set_accelerator(want), + } + .is_ok(); + self.current = settled(self.current, want, done); + } + + /// Replaces the native item by one without an accelerator. `self.handle` changes only when the + /// new item is attached; on failure the old item is still the one in the submenu. + fn replace_without_accelerator(&mut self) -> bool { + let (id, old): (_, &dyn muda::IsMenuItem) = match &self.handle { + Handle::Plain(h) => (h.id().clone(), h), + Handle::Check(h) => (h.id().clone(), h), + }; + let Some(position) = self.parent.items().iter().position(|k| *k.id() == id) else { return false }; + let new = match &self.handle { + Handle::Plain(_) => Handle::Plain(MenuItem::with_id(id, self.native_label(), self.enabled, None)), + Handle::Check(h) => Handle::Check(CheckMenuItem::with_id(id, self.native_label(), self.enabled, h.is_checked(), None)), + }; + let new_ref: &dyn muda::IsMenuItem = match &new { + Handle::Plain(h) => h, + Handle::Check(h) => h, + }; + if !swap_item(&self.parent, old, new_ref, position) { + return false; + } + self.handle = new; + true + } +} + +/// The accelerator the native item has after trying to put `want` on it: `want` when the native +/// call worked, else what it had, so the caller sees the mismatch and tries again. +fn settled(had: Option, want: Option, native_call_worked: bool) -> Option { + if native_call_worked { want } else { had } +} + +/// Whether the per-frame accelerator sync has to run: the focus or recording state changed, or an +/// earlier sync left an item whose native accelerator is not the wanted one (a failed swap). +fn sync_needed(focus: bool, capturing: bool, synced_focus: bool, synced_capturing: bool, pending: bool) -> bool { + focus != synced_focus || capturing != synced_capturing || pending +} + +/// The three submenu calls a swap needs, so the failure paths can be tested without a main-thread +/// `muda::Menu`. +trait SwapTarget { + type Row: ?Sized; + fn remove(&self, row: &Self::Row) -> bool; + fn insert(&self, row: &Self::Row, position: usize) -> bool; + fn append(&self, row: &Self::Row) -> bool; +} + +impl SwapTarget for Submenu { + type Row = dyn muda::IsMenuItem; + fn remove(&self, row: &Self::Row) -> bool { + Submenu::remove(self, row).is_ok() + } + fn insert(&self, row: &Self::Row, position: usize) -> bool { + Submenu::insert(self, row, position).is_ok() + } + fn append(&self, row: &Self::Row) -> bool { + Submenu::append(self, row).is_ok() + } +} + +/// Puts `new` where `old` is. Returns whether `new` is attached; when it is not, `old` is back in +/// the menu (at its position, else at the end), so the caller keeps `old` as its handle. +fn swap_item(target: &T, old: &T::Row, new: &T::Row, position: usize) -> bool { + if !target.remove(old) { + return false; + } + // (an item at the end beats a missing one) + if target.insert(new, position) || target.append(new) { + return true; + } + let _ = target.insert(old, position) || target.append(old); + false +} + +/// The accelerator a native item has: none while a shortcut is being recorded, and none for the +/// keys a focused text field needs. +fn wanted_accelerator(accel: Option, sc: &str, text_focus: bool, capturing: bool) -> Option { + if capturing || (text_focus && yields_to_text(sc)) { None } else { accel } } pub struct NativeMenu { menu: Menu, items: HashMap, + /// Settings… and Quit in the application submenu: ordinary items, so they give up ⌘, and ⌘Q + /// to the shortcut recorder like the rest. (The system's own items there and in File/Window, + /// such as Hide ⌘H, Close Window ⌘W and Minimize ⌘M, are muda predefined items whose key + /// cannot be changed or swapped.) + app_items: Vec, rx: Receiver, /// Structure of the installed menu (ids, kinds, submenu names); a change rebuilds it. structure: String, @@ -58,6 +178,8 @@ pub struct NativeMenu { /// The keyboard shortcuts editor is recording a key: every accelerator is off so the key /// press reaches it. capturing: bool, + /// An item's native accelerator is not the wanted one (its swap or accelerator call failed): sync again next frame. + accelerators_pending: bool, } /// `Cmd+Shift+Z` → a muda accelerator (`None` for keys we leave to egui, e.g. Escape). @@ -216,7 +338,16 @@ impl NativeMenu { let _ = tx.send(e.id.0); repaint.request_repaint(); })); - let mut m = NativeMenu { menu: Menu::new(), items: HashMap::new(), rx, structure: String::new(), text_focus: false, capturing: false }; + let mut m = NativeMenu { + menu: Menu::new(), + items: HashMap::new(), + app_items: Vec::new(), + rx, + structure: String::new(), + text_focus: false, + capturing: false, + accelerators_pending: false, + }; m.rebuild(app); app.native_menu = true; m @@ -227,6 +358,7 @@ impl NativeMenu { self.structure = structure_of(&bar); self.menu = Menu::new(); self.items.clear(); + self.app_items.clear(); // the application menu let app_menu = Submenu::new("LightCraft", true); @@ -246,6 +378,22 @@ impl NativeMenu { &PredefinedMenuItem::separator(), &quit, ]); + for (handle, cmd, shortcut, text) in [(&settings, SETTINGS, SETTINGS_KEY, "Settings…"), (&quit, QUIT, "Cmd+Q", "Quit LightCraft")] { + let accel = accelerator(shortcut); + self.app_items.push(Item { + handle: Handle::Plain(handle.clone()), + parent: app_menu.clone(), + swappable: true, + cmd: cmd.to_string(), + params: Value::Null, + shortcut: Some(shortcut.to_string()), + accel, + current: accel, + label: text.to_string(), + enabled: true, + checked: None, + }); + } let _ = self.menu.append(&app_menu); for (title, nodes) in &bar { @@ -264,7 +412,9 @@ impl NativeMenu { .cloned() .collect(); let nodes = tidy_separators(nodes); - self.append_nodes(&sub, &nodes); + // Swapping an item in the Help menu leaves the old row in place and adds a second one + // (measured on macOS 27 with muda 0.21): the Help menu is not addressed row for row. + self.append_nodes(&sub, &nodes, title != "Help"); if title == "File" { // ⌘W, the system's own item let _ = sub.append_items(&[ @@ -287,10 +437,12 @@ impl NativeMenu { self.menu.init_for_nsapp(); self.text_focus = false; self.capturing = false; + self.accelerators_pending = false; self.publish_shortcuts(app); } - fn append_nodes(&mut self, sub: &Submenu, nodes: &[MenuNode]) { + /// `swappable`: whether its items may be replaced to drop their key (see `Item::swappable`). + fn append_nodes(&mut self, sub: &Submenu, nodes: &[MenuNode], swappable: bool) { for n in nodes { match n { MenuNode::Separator => { @@ -298,7 +450,7 @@ impl NativeMenu { } MenuNode::Submenu { label, children } => { let s = Submenu::new(label_text(label), true); - self.append_nodes(&s, children); + self.append_nodes(&s, children, swappable); let _ = sub.append(&s); } MenuNode::Item { id, params, label, shortcut, enabled, checked } => { @@ -334,10 +486,13 @@ impl NativeMenu { key, Item { handle, + parent: sub.clone(), + swappable, cmd: id.clone(), params: params.clone(), shortcut: shortcut.clone(), accel, + current: accel, label: label.clone(), enabled: *enabled, checked: *checked, @@ -379,8 +534,9 @@ impl NativeMenu { } let bar = menu_bar(app); if structure_of(&bar) != self.structure { + // (no early return: a rebuild installs every accelerator and resets the state they were synced + // to, so the sync below runs in this same frame and no key reaches the menu in between) self.rebuild(app); - return quit; } fn walk(nodes: &[MenuNode], items: &mut HashMap) { for n in nodes { @@ -418,20 +574,20 @@ impl NativeMenu { let focus = ctx.egui_wants_keyboard_input(); let capturing = app.recording_shortcut.is_some(); - if focus != self.text_focus || capturing != self.capturing { + if sync_needed(focus, capturing, self.text_focus, self.capturing, self.accelerators_pending) { self.text_focus = focus; self.capturing = capturing; - for it in self.items.values() { + let mut pending = false; + for it in self.items.values_mut().chain(self.app_items.iter_mut()) { let Some(sc) = it.shortcut.as_deref() else { continue }; if it.accel.is_none() { continue; } - let accel = if capturing || (focus && yields_to_text(sc)) { None } else { it.accel }; - let _ = match &it.handle { - Handle::Plain(h) => h.set_accelerator(accel), - Handle::Check(h) => h.set_accelerator(accel), - }; + let want = wanted_accelerator(it.accel, sc, focus, capturing); + it.set_accelerator(want); + pending |= it.current != want; } + self.accelerators_pending = pending; self.publish_shortcuts(app); } quit @@ -459,6 +615,97 @@ mod tests { set_language(Locale::En); } + #[test] + fn accelerators_yield_to_text_focus_and_shortcut_recording() { + let g = accelerator("G"); + let cmd_s = accelerator("Cmd+S"); + assert_eq!(wanted_accelerator(g, "G", false, false), g); + assert_eq!(wanted_accelerator(g, "G", true, false), None, "typing a G in a field"); + assert_eq!(wanted_accelerator(cmd_s, "Cmd+S", true, false), cmd_s, "⌘S still saves while typing"); + assert_eq!(wanted_accelerator(cmd_s, "Cmd+S", false, true), None, "the editor records the key"); + assert_eq!(wanted_accelerator(cmd_s, "Cmd+S", true, true), None, "recording wins over typing"); + // the application menu's Settings… and Quit are tracked like the other items + for sc in [SETTINGS_KEY, "Cmd+Q"] { + let a = accelerator(sc); + assert!(a.is_some()); + assert_eq!(wanted_accelerator(a, sc, true, false), a, "{sc} stays while typing"); + assert_eq!(wanted_accelerator(a, sc, false, true), None, "{sc} goes while recording"); + } + assert_eq!(wanted_accelerator(None, "Escape", false, false), None, "no accelerator stays none"); + } + + /// A submenu of numbers whose calls can be made to fail, to run `swap_item`'s failure paths. + /// Row 2 is the old item and row 9 its replacement. + #[derive(Default)] + struct Fake { + rows: std::cell::RefCell>, + fail_remove: bool, + /// The new item (9) cannot be inserted / appended. + reject_new_insert: bool, + reject_new_append: bool, + /// The old item (2) cannot be inserted back. + reject_old_insert: bool, + } + impl SwapTarget for Fake { + type Row = u32; + fn remove(&self, row: &u32) -> bool { + if self.fail_remove { + return false; + } + self.rows.borrow_mut().retain(|x| x != row); + true + } + fn insert(&self, row: &u32, position: usize) -> bool { + if (self.reject_new_insert && *row == 9) || (self.reject_old_insert && *row == 2) { + return false; + } + self.rows.borrow_mut().insert(position, *row); + true + } + fn append(&self, row: &u32) -> bool { + if self.reject_new_append && *row == 9 { + return false; + } + self.rows.borrow_mut().push(*row); + true + } + } + + fn swap(fake: Fake) -> (bool, Vec) { + *fake.rows.borrow_mut() = vec![1, 2, 3]; + let done = swap_item(&fake, &2, &9, 1); + (done, fake.rows.into_inner()) + } + + #[test] + fn a_swap_changes_the_item_only_when_the_new_one_is_attached() { + assert_eq!(swap(Fake::default()), (true, vec![1, 9, 3]), "in place"); + assert_eq!(swap(Fake { fail_remove: true, ..Default::default() }), (false, vec![1, 2, 3]), "remove fails: the old item stays"); + assert_eq!(swap(Fake { reject_new_insert: true, ..Default::default() }), (true, vec![1, 3, 9]), "insert fails: the new item goes last"); + assert_eq!( + swap(Fake { reject_new_insert: true, reject_new_append: true, ..Default::default() }), + (false, vec![1, 2, 3]), + "neither works: the old item is back where it was" + ); + assert_eq!( + swap(Fake { reject_new_insert: true, reject_new_append: true, reject_old_insert: true, ..Default::default() }), + (false, vec![1, 3, 2]), + "the old item cannot go back in place either: it is appended" + ); + } + + #[test] + fn a_failed_native_call_is_retried_while_the_state_holds() { + let g = accelerator("G"); + assert_eq!(settled(g, None, true), None, "the swap worked"); + assert_eq!(settled(g, None, false), g, "it failed: the native item still has its key"); + assert_eq!(settled(None, g, false), None, "restoring a key failed"); + assert!(!sync_needed(true, false, true, false, false), "nothing changed"); + assert!(sync_needed(true, false, false, false, false), "focus changed"); + assert!(sync_needed(false, true, false, false, false), "recording changed"); + assert!(sync_needed(true, false, true, false, true), "a swap failed last time: try again with the state unchanged"); + } + #[test] fn shortcuts_map_to_accelerators() { assert_eq!(accelerator("Cmd+Shift+Z"), Some(Accelerator::new(Modifiers::META | Modifiers::SHIFT, Code::KeyZ))); diff --git a/contributors/people.toml b/contributors/people.toml index 4eb002cef..cc71e4cc2 100644 --- a/contributors/people.toml +++ b/contributors/people.toml @@ -38,6 +38,9 @@ sync_github_name = true [people.Losminek] sync_github_name = true +[people.nicolasfalesy] +sync_github_name = true + [people.FooVoo] real_name = "Aleksandr Batyrev" sync_github_name = true diff --git a/crates/catalog/src/lock.rs b/crates/catalog/src/lock.rs index d699da049..f701087a6 100644 --- a/crates/catalog/src/lock.rs +++ b/crates/catalog/src/lock.rs @@ -25,7 +25,7 @@ //! - Browser storage (OPFS) has no file locks: see the web host for its own guard. use std::fs::{File, OpenOptions, TryLockError}; -use std::io; +use std::io::{self, Write}; use std::path::{Path, PathBuf}; /// The lock file in the library directory. @@ -116,9 +116,11 @@ impl LibraryLock { } Err(TryLockError::Error(e)) => return Ok(unlocked(e)), } + // A new file in place of whatever is at the name: a link planted there in a library folder + // someone else prepared is removed, never written through (as for the staging files, #1502). let owner = LockOwner::this_process(program); if let Ok(json) = serde_json::to_vec_pretty(&owner) - && let Err(e) = std::fs::write(dir.join(OWNER), json) + && let Err(e) = crate::store::create_staging(&dir.join(OWNER)).and_then(|mut f| f.write_all(&json)) { log::warn!("library {}: can't write {OWNER}: {e}", dir.display()); } diff --git a/crates/catalog/src/query.rs b/crates/catalog/src/query.rs index a94421369..830857386 100644 --- a/crates/catalog/src/query.rs +++ b/crates/catalog/src/query.rs @@ -29,6 +29,10 @@ pub struct Filter { /// Only these photos (Find Similar results…); empty = no constraint. #[serde(skip_serializing_if = "Vec::is_empty")] pub only: Vec, + /// Show nothing: an explicit empty result (Find Similar with no matches), since an empty + /// `only` means "no constraint". + #[serde(skip_serializing_if = "std::ops::Not::not")] + pub only_none: bool, /// Any of these labels (the filter bar's multi-select); empty = no constraint. #[serde(skip_serializing_if = "Vec::is_empty")] pub labels: Vec, @@ -374,6 +378,9 @@ impl Filter { if !self.labels.is_empty() && !p.label.is_some_and(|l| self.labels.contains(&l)) { return false; } + if self.only_none { + return false; + } if !self.only.is_empty() && !prepared.only.as_ref().map_or_else(|| self.only.contains(&p.id), |only| only.contains(&p.id)) { return false; } diff --git a/crates/catalog/src/rules.rs b/crates/catalog/src/rules.rs index 667509d6f..bdf4b8c88 100644 --- a/crates/catalog/src/rules.rs +++ b/crates/catalog/src/rules.rs @@ -670,6 +670,11 @@ fn number(v: &Value) -> Option { } } +/// Widens an `f32` to the `f64` nearest its shortest decimal form (1.4f32 -> 1.4), so it compares equal to the value a user types. +fn f32_as_decimal(v: f32) -> f64 { + v.to_string().parse().unwrap_or(v as f64) +} + fn num_op(op: &str, have: Option, value: &Value) -> bool { if op == "between" { let (a, b) = match value { @@ -953,8 +958,8 @@ impl Rule { "editDate" => date_op(op, p.edited.as_deref(), value), "shutterSpeed" => num_op(op, crate::parse_shutter_seconds(&m.shutter), &shutter_value(value)), "iso" => num_op(op, m.iso.map(|v| v as f64), value), - "aperture" => num_op(op, m.aperture.map(|v| v as f64), value), - "focalLength" => num_op(op, m.focal_mm.map(|v| v as f64), value), + "aperture" => num_op(op, m.aperture.map(f32_as_decimal), value), + "focalLength" => num_op(op, m.focal_mm.map(f32_as_decimal), value), "longEdge" | "shortEdge" => { let (w, h) = p.shown_size(); num_op(op, Some(if field == "longEdge" { w.max(h) } else { w.min(h) }.round()), value) @@ -1129,6 +1134,25 @@ mod tests { serde_json::from_value(v).unwrap() } + #[test] + fn f32_boundary_values_match_gte_lte() { + let cat = Catalog::new(); + let matches = + |p: &Photo, field: &str, op: &str, value: f64| rs(json!({"rules": [{"field": field, "op": op, "value": value}]})).matches(p, &cat); + for v in [1.4f32, 1.8, 2.8, 5.6, 0.95] { + let mut p = photo(1); + p.meta.aperture = Some(v); + p.meta.focal_mm = Some(v); + let w = v.to_string().parse::().unwrap(); + for field in ["aperture", "focalLength"] { + assert!(matches(&p, field, "gte", w), "{field} gte {w}"); + assert!(matches(&p, field, "lte", w), "{field} lte {w}"); + assert!(!matches(&p, field, "gt", w), "{field} gt {w}"); + assert!(!matches(&p, field, "lt", w), "{field} lt {w}"); + } + } + } + #[test] fn fields_ops_and_groups() { let cat = Catalog::new(); diff --git a/crates/catalog/src/store.rs b/crates/catalog/src/store.rs index 553eb241f..ad008cf0c 100644 --- a/crates/catalog/src/store.rs +++ b/crates/catalog/src/store.rs @@ -75,8 +75,8 @@ impl FsStore { /// A new staging file at `tmp`: whatever is already there (a leftover from a crashed save, or a link) is removed first /// (a link itself, never what it points to), then the file is created exclusively, so a write never follows a link -/// planted at that name into another file (issue #1502). -fn create_staging(tmp: &std::path::Path) -> io::Result { +/// planted at that name into another file (issue #1502). Also writes the lock's owner note. +pub(crate) fn create_staging(tmp: &std::path::Path) -> io::Result { if std::fs::symlink_metadata(tmp).is_ok() { std::fs::remove_file(tmp)?; } diff --git a/crates/catalog/src/tests_lock.rs b/crates/catalog/src/tests_lock.rs index baa8da659..2472ba92a 100644 --- a/crates/catalog/src/tests_lock.rs +++ b/crates/catalog/src/tests_lock.rs @@ -62,6 +62,28 @@ fn leftover_lock_files_are_not_a_lock() { let _ = std::fs::remove_dir_all(&dir); } +/// A link planted at the owner note's name (`catalog.lock.owner`) in a library folder someone else +/// prepared is removed, never written through: the file it points to keeps its bytes, and the note +/// still lands in the library (as for the catalog's staging files, issue #1502). +#[cfg(unix)] +#[test] +fn a_link_at_the_owner_note_is_never_written_through() { + let _serial = serial(); + let dir = temp_dir("owner-link"); + let victim = dir.join("victim.txt"); + std::fs::write(&victim, b"keep").unwrap(); + std::os::unix::fs::symlink(&victim, dir.join(OWNER)).unwrap(); + let l = LibraryLock::acquire(&dir, "LightCraft").unwrap(); + assert!(l.held()); + assert_eq!(std::fs::read(&victim).unwrap(), b"keep"); + assert!(!std::fs::symlink_metadata(dir.join(OWNER)).unwrap().file_type().is_symlink(), "the link was replaced by the note"); + let owner: LockOwner = serde_json::from_slice(&std::fs::read(dir.join(OWNER)).unwrap()).unwrap(); + assert_eq!(owner.pid, std::process::id()); + drop(l); + assert_eq!(std::fs::read(&victim).unwrap(), b"keep"); + let _ = std::fs::remove_dir_all(&dir); +} + /// Another process holds the lock; when it is killed (a crash), the OS releases it. #[test] fn lock_held_by_another_process_is_released_when_it_dies() { diff --git a/crates/engine/src/camera_preview.rs b/crates/engine/src/camera_preview.rs index d074567a7..1d1d7468e 100644 --- a/crates/engine/src/camera_preview.rs +++ b/crates/engine/src/camera_preview.rs @@ -3066,10 +3066,14 @@ mod tests { let bundled: crate::camera_profiles::CameraProfile = serde_json::from_str(json).unwrap(); let used = crate::camera_profiles::get(model).expect("a profile for the model"); assert_eq!(*used, bundled, "a local {model} profile overrides the built-in one; remove it to run this test"); - let raw = lightcraft_raw::decode(&bytes).unwrap(); + let mut raw = lightcraft_raw::decode(&bytes).unwrap(); assert_eq!(raw.metadata.model.as_deref(), Some(model)); + // as the app does (`files.rs`): the camera's own lens warp pairs the sensor with its JPEG (Nikon's in-file + // distortion for the Z 6II and Z 8), and the lens corrections leave the opcodes before the proxy is binned + let lens = lens_geometry(&raw.opcodes.list3); + raw.opcodes.list3.retain(|op| !op.is_lens_correction()); let transform = lightcraft_raw::color::camera_transform(&raw, lightcraft_raw::color::as_shot_white_xy(&raw)); - let look = fit_preview(&raw, &bytes, &transform, None).0.expect("fit accepted"); + let look = fit_preview(&raw, &bytes, &transform, lens.as_ref()).0.expect("fit accepted"); let expected = bundled.matrix().mul(&transform.matrix.inverse().unwrap()); for (row, want) in look.matrix.0.iter().zip(expected.0) { for (got, want) in row.iter().zip(want) { diff --git a/crates/engine/src/cmd/browse.rs b/crates/engine/src/cmd/browse.rs index 01c67c267..6f3f88963 100644 --- a/crates/engine/src/cmd/browse.rs +++ b/crates/engine/src/cmd/browse.rs @@ -482,6 +482,9 @@ pub(crate) fn follow_folder(s: &mut Session, from: &str, to: &str) { // a library folder chosen at or below it follows too, as a source or as a filter follow_library_folder(&mut s.library_folder, from, to); follow_library_folder(&mut s.filter.library_folder, from, to); + if let Some(f) = s.shelved_filter.as_mut() { + follow_library_folder(&mut f.library_folder, from, to); + } if let Some(b) = &mut s.browse && let Ok(rel) = Path::new(&b.path).strip_prefix(from) { diff --git a/crates/engine/src/cmd/cull.rs b/crates/engine/src/cmd/cull.rs index 142accd5a..f6600c6b1 100644 --- a/crates/engine/src/cmd/cull.rs +++ b/crates/engine/src/cmd/cull.rs @@ -109,13 +109,16 @@ fn find_similar(s: &mut Session, p: &Value) -> Result { if let Some(v) = sig(s, q) { let sim = lightcraft_pipeline::cull::similarity(&me, &v); if sim >= min { - hits.push((q, sim)); + hits.push((q, sim.clamp(0.0, 1.0))); } } } hits.sort_by(|a, b| b.1.total_cmp(&a.1)); if p.get("filter").and_then(Value::as_bool).unwrap_or(true) { + s.shelved_filter = None; s.filter.only = hits.iter().map(|h| h.0).collect(); + // an empty `only` means "no constraint": no hits is its own state that shows no photos + s.filter.only_none = s.filter.only.is_empty(); } Ok(json!({"photos": hits.iter().map(|(i, v)| json!({"id": i.0, "similarity": (v * 1000.0).round() / 1000.0})).collect::>()})) } diff --git a/crates/engine/src/cmd/face_recognize.rs b/crates/engine/src/cmd/face_recognize.rs index e66bb56f3..e3b385e2c 100644 --- a/crates/engine/src/cmd/face_recognize.rs +++ b/crates/engine/src/cmd/face_recognize.rs @@ -176,6 +176,11 @@ fn rename_person(s: &mut Session, p: &Value) -> Result { if s.filter.person.as_deref().is_some_and(|n| n.trim().to_lowercase() == from_key) { s.filter.person = Some(to.clone()); } + if let Some(f) = s.shelved_filter.as_mut() + && f.person.as_deref().is_some_and(|n| n.trim().to_lowercase() == from_key) + { + f.person = Some(to.clone()); + } Ok(json!({"faces": faces, "photos": photos, "from": from, "to": to, "merged": other})) } diff --git a/crates/engine/src/cmd/filters.rs b/crates/engine/src/cmd/filters.rs index 73bae384f..afe2011ac 100644 --- a/crates/engine/src/cmd/filters.rs +++ b/crates/engine/src/cmd/filters.rs @@ -38,6 +38,7 @@ fn apply(s: &mut Session, p: &Value) -> Result { let name = str_param(p, "name").ok_or_else(|| bad(C, "missing `name`"))?; let f = s.filter_presets.iter().find(|f| f.name.eq_ignore_ascii_case(name)).ok_or_else(|| bad(C, format!("no filter preset `{name}`")))?; s.filter = f.filter.clone(); + s.shelved_filter = None; Ok(json!({"photos": s.visible().len()})) } diff --git a/crates/engine/src/cmd/geocode.rs b/crates/engine/src/cmd/geocode.rs index c9ffb6b7e..feb9b033e 100644 --- a/crates/engine/src/cmd/geocode.rs +++ b/crates/engine/src/cmd/geocode.rs @@ -63,6 +63,13 @@ impl PlaceNames { self.lock().running } + /// Whether a place names download has been started in this session (running, or ended). + #[cfg(test)] + pub(crate) fn download_started(&self) -> bool { + let sh = self.lock(); + sh.running || sh.last.is_some() + } + /// The gazetteer in `dir`, parsed once and re-read only when the cities file's size or time changes. fn load(&mut self, dir: &Path) -> std::result::Result, lightcraft_geocode::GeocodeError> { let file = dir.join(GEONAMES_FILES[0]); diff --git a/crates/engine/src/cmd/keywords.rs b/crates/engine/src/cmd/keywords.rs index fe09a92f9..0f3b8827a 100644 --- a/crates/engine/src/cmd/keywords.rs +++ b/crates/engine/src/cmd/keywords.rs @@ -32,6 +32,12 @@ fn commit_keywords(s: &mut Session, label: &str, op: lightcraft_catalog::Op, fol if let Some(k) = s.filter.keyword.clone() { s.filter.keyword = follow(&k); } + // a filter put aside by Enable Filters follows the keyword too + if let Some(f) = s.shelved_filter.as_mut() + && let Some(k) = f.keyword.clone() + { + f.keyword = follow(&k); + } Ok(json!({"changed": photos, "listed": listed})) } diff --git a/crates/engine/src/cmd/library.rs b/crates/engine/src/cmd/library.rs index e437dccc2..334d9c92d 100644 --- a/crates/engine/src/cmd/library.rs +++ b/crates/engine/src/cmd/library.rs @@ -426,6 +426,7 @@ pub fn specs() -> Vec { return Err(bad("library.filter", problems.iter().map(ToString::to_string).collect::>().join("; "))); } s.filter = f; + s.shelved_filter = None; Ok(json!({"count": s.visible().len()})) } ), @@ -469,6 +470,15 @@ pub fn specs() -> Vec { if s.filter.library_folder.as_deref().is_some_and(|c| lightcraft_catalog::query::folder_within(c, path)) { s.filter.library_folder = None; } + // a filter put aside by Enable Filters must not bring the removed folder back + if let Some(f) = s.shelved_filter.as_mut() + && f.library_folder.as_deref().is_some_and(|c| lightcraft_catalog::query::folder_within(c, path)) + { + f.library_folder = None; + } + if s.shelved_filter.as_ref() == Some(&Filter::default()) { + s.shelved_filter = None; + } // the folder being shown is gone: back to everything, as for a deleted album if s.library_folder.as_deref().is_some_and(|c| lightcraft_catalog::query::folder_within(c, path)) { s.library_folder = None; @@ -510,8 +520,30 @@ pub fn specs() -> Vec { ), cmd!("library.clearFilter", "Clear Filters", ["View"], None, "{}", always, |s, _| { s.filter = Default::default(); + s.shelved_filter = None; Ok(json!({"count": s.visible().len()})) }), + // Classic: Library > Enable Filters (Cmd+L) switches the filter off and on again as it was + cmd!( + "library.toggleFilters", + "Enable Filters", + ["View"], + Some("Cmd+L"), + "{} — with a filter set, puts it aside and shows everything; with one put aside, brings it back as it was (stars, flag, text… together). Setting or clearing a filter while it is off forgets the one put aside → {enabled, count}", + always, + |s, _| { + if s.filter != Filter::default() { + s.shelved_filter = Some(std::mem::take(&mut s.filter)); + } else if let Some(mut f) = s.shelved_filter.take() { + // one folder at a time: the folder shown now wins over the one the filter had + if s.source == LibrarySource::LibraryFolder { + f.library_folder = None; + } + s.filter = f; + } + Ok(json!({"enabled": s.shelved_filter.is_none(), "count": s.visible().len()})) + } + ), cmd!( "library.sort", "Sort", @@ -1771,3 +1803,7 @@ mod relative_rating_tests; #[cfg(test)] #[path = "library/tests_flag_cycle.rs"] mod flag_cycle_tests; + +#[cfg(test)] +#[path = "library/tests_enable_filters.rs"] +mod enable_filters_tests; diff --git a/crates/engine/src/cmd/library/tests_enable_filters.rs b/crates/engine/src/cmd/library/tests_enable_filters.rs new file mode 100644 index 000000000..9eb3c1ad0 --- /dev/null +++ b/crates/engine/src/cmd/library/tests_enable_filters.rs @@ -0,0 +1,154 @@ +//! `library.toggleFilters` (Library > Enable Filters, Cmd+L) through the real engine. +use lightcraft_catalog::{Filter, Flag, Op, Photo, PhotoId, Source}; +use serde_json::json; + +use crate::{LibrarySource, Result, Session}; + +fn session() -> Result { + let mut s = Session::new(); + for (id, rating, flag) in [(1, 5, Flag::Pick), (2, 5, Flag::None), (3, 2, Flag::Pick), (4, 1, Flag::None)] { + let mut photo = Photo::new( + PhotoId(id), + Source::File { path: format!("/lightcraft-enable-filters/{id}.jpg") }, + &format!("{id}.jpg"), + "JPEG", + 40, + 30, + "2026-10-23", + ); + photo.rating = rating; + photo.flag = flag; + s.catalog.apply(Op::AddPhoto { photo: Box::new(photo) })?; + } + Ok(s) +} + +fn stars_and_pick(s: &mut Session) { + s.execute("library.filter", &json!({"rating": 4, "flag": "pick"})).unwrap(); +} + +#[test] +fn off_shows_everything_and_on_brings_back_the_same_filter() { + let mut s = session().unwrap(); + stars_and_pick(&mut s); + let set = s.filter.clone(); + assert_eq!(s.visible().len(), 1, "the filter hides photos, or off and on would look alike"); + let r = s.execute("library.toggleFilters", &json!({})).unwrap(); + assert_eq!((r["enabled"].clone(), r["count"].clone()), (json!(false), json!(4))); + assert_eq!(s.filter, Filter::default()); + assert_eq!(s.visible().len(), 4); + let r = s.execute("library.toggleFilters", &json!({})).unwrap(); + assert_eq!((r["enabled"].clone(), r["count"].clone()), (json!(true), json!(1))); + assert_eq!(s.filter, set, "stars and flag come back together"); + assert_eq!(s.visible().len(), 1); +} + +#[test] +fn with_no_filter_it_does_nothing_and_leaves_nothing_to_bring_back() { + let mut s = session().unwrap(); + let r = s.execute("library.toggleFilters", &json!({})).unwrap(); + assert_eq!((r["enabled"].clone(), r["count"].clone()), (json!(true), json!(4))); + let r = s.execute("library.toggleFilters", &json!({})).unwrap(); + assert_eq!(r["enabled"], json!(true)); + assert_eq!(s.filter, Filter::default()); + assert!(s.shelved_filter.is_none()); +} + +#[test] +fn setting_a_filter_while_off_turns_filtering_on_and_forgets_the_old_one() { + let mut s = session().unwrap(); + stars_and_pick(&mut s); + s.execute("library.toggleFilters", &json!({})).unwrap(); + s.execute("library.filter", &json!({"rating": 2})).unwrap(); + assert_eq!(s.visible().len(), 3); + // off again, then on: the filter that was set last, not the first one + s.execute("library.toggleFilters", &json!({})).unwrap(); + s.execute("library.toggleFilters", &json!({})).unwrap(); + assert_eq!(s.filter.rating, 2); + assert_eq!(s.filter.flag, None); +} + +#[test] +fn emptying_the_filter_by_hand_while_off_leaves_nothing_to_bring_back() { + let mut s = session().unwrap(); + stars_and_pick(&mut s); + s.execute("library.toggleFilters", &json!({})).unwrap(); + s.execute("library.filter", &json!({"rating": 2})).unwrap(); + s.execute("library.filter", &json!({"rating": 0})).unwrap(); + assert_eq!(s.filter, Filter::default()); + s.execute("library.toggleFilters", &json!({})).unwrap(); + assert_eq!(s.filter, Filter::default(), "the filter from before it was switched off is not back"); + assert_eq!(s.visible().len(), 4); +} + +#[test] +fn clearing_the_filter_while_off_leaves_nothing_to_bring_back() { + let mut s = session().unwrap(); + stars_and_pick(&mut s); + s.execute("library.toggleFilters", &json!({})).unwrap(); + s.execute("library.clearFilter", &json!({})).unwrap(); + s.execute("library.toggleFilters", &json!({})).unwrap(); + assert_eq!(s.filter, Filter::default()); + assert_eq!(s.visible().len(), 4); +} + +#[test] +fn applying_a_preset_while_off_turns_filtering_on_with_the_preset() { + let mut s = session().unwrap(); + s.filter_presets.push(super::super::filters::FilterPreset { name: "Fives".into(), filter: Filter { rating: 5, ..Default::default() } }); + stars_and_pick(&mut s); + s.execute("library.toggleFilters", &json!({})).unwrap(); + s.execute("filter.applyPreset", &json!({"name": "Fives"})).unwrap(); + s.execute("library.toggleFilters", &json!({})).unwrap(); + s.execute("library.toggleFilters", &json!({})).unwrap(); + assert_eq!(s.filter.rating, 5); + assert_eq!(s.filter.flag, None); +} + +#[test] +fn a_folder_filter_does_not_come_back_over_the_folder_now_shown() { + let mut s = session().unwrap(); + s.execute("library.filter", &json!({"rating": 4, "libraryFolder": "/lightcraft-enable-filters"})).unwrap(); + s.execute("library.toggleFilters", &json!({})).unwrap(); + s.source = LibrarySource::LibraryFolder; + s.library_folder = Some("/lightcraft-enable-filters".into()); + s.execute("library.toggleFilters", &json!({})).unwrap(); + assert_eq!(s.filter.rating, 4); + assert_eq!(s.filter.library_folder, None); +} + +#[test] +fn removing_a_folder_while_off_drops_it_from_the_filter_put_aside() { + let mut s = session().unwrap(); + s.execute("library.filter", &json!({"rating": 4, "libraryFolder": "/lightcraft-enable-filters"})).unwrap(); + s.execute("library.toggleFilters", &json!({})).unwrap(); + s.execute("library.removeFolder", &json!({"path": "/lightcraft-enable-filters"})).unwrap(); + s.execute("library.toggleFilters", &json!({})).unwrap(); + assert_eq!(s.filter.library_folder, None, "the removed folder is not brought back"); + assert_eq!(s.filter.rating, 4, "the rest of the filter is"); +} + +#[test] +fn removing_the_only_criterion_put_aside_leaves_filtering_on() { + let mut s = session().unwrap(); + s.execute("library.filter", &json!({"libraryFolder": "/lightcraft-enable-filters"})).unwrap(); + s.execute("library.toggleFilters", &json!({})).unwrap(); + s.execute("library.removeFolder", &json!({"path": "/lightcraft-enable-filters"})).unwrap(); + assert!(s.shelved_filter.is_none(), "an empty filter is not left put aside"); +} + +#[test] +fn a_keyword_renamed_while_off_is_followed_by_the_filter_put_aside() { + let mut s = session().unwrap(); + let mut photo = Photo::new(PhotoId(5), Source::File { path: "/lightcraft-enable-filters/5.jpg".into() }, "5.jpg", "JPEG", 40, 30, "2026-10-23"); + photo.meta.keywords = vec!["cat".into()]; + s.catalog.apply(Op::AddPhoto { photo: Box::new(photo) }).unwrap(); + s.filter.keyword = Some("cat".into()); + assert_eq!(s.visible().len(), 1); + s.execute("library.toggleFilters", &json!({})).unwrap(); + s.execute("keyword.rename", &json!({"from": "cat", "to": "dog"})).unwrap(); + let r = s.execute("library.toggleFilters", &json!({})).unwrap(); + assert_eq!(r["enabled"], json!(true)); + assert_eq!(s.filter.keyword.as_deref(), Some("dog"), "the put-aside filter follows the rename"); + assert_eq!(s.visible().len(), 1); +} diff --git a/crates/engine/src/cmd/masks.rs b/crates/engine/src/cmd/masks.rs index dc403df9b..2fd44d566 100644 --- a/crates/engine/src/cmd/masks.rs +++ b/crates/engine/src/cmd/masks.rs @@ -8,6 +8,18 @@ use super::{CommandSpec, bad, bool_or, cmd, f64_or, has_active, ok, point, str_p use crate::segment::{Click, MAX_CLICKS}; use crate::{Result, Session}; +/// A colour-range mask compares every pixel against every sample, so render and export cost is +/// pixels x samples. The interactive sampler stops at 5; automation (`mask.add`/`mask.update`) +/// passed its `samples` array straight through, so a few hundred samples on a large render hung. +const MAX_COLOR_RANGE_SAMPLES: usize = 64; + +/// Caps a colour-range shape's sample list at [`MAX_COLOR_RANGE_SAMPLES`] (render cost is pixels x samples). +fn cap_color_range_samples(shape: &mut MaskShape) { + if let MaskShape::ColorRange { samples, .. } = shape { + samples.truncate(MAX_COLOR_RANGE_SAMPLES); + } +} + fn shape_from(kind: &str, p: &Value, c: &str) -> Result { Ok(match kind { "brush" => MaskShape::Brush { strokes: vec![] }, @@ -35,7 +47,12 @@ fn shape_from(kind: &str, p: &Value, c: &str) -> Result { let samples: Vec<[f64; 3]> = p .get("samples") .and_then(Value::as_array) - .map(|a| a.iter().filter_map(|s| Some([s.get(0)?.as_f64()?, s.get(1)?.as_f64()?, s.get(2)?.as_f64()?])).collect()) + .map(|a| { + a.iter() + .filter_map(|s| Some([s.get(0)?.as_f64()?, s.get(1)?.as_f64()?, s.get(2)?.as_f64()?])) + .take(MAX_COLOR_RANGE_SAMPLES) + .collect() + }) .unwrap_or_default(); MaskShape::ColorRange { samples, refine: f64_or(p, "refine", 50.0) } } @@ -381,8 +398,9 @@ pub fn specs() -> Vec { } ), cmd!("mask.update", "Update Mask Shape", [], None, "{id?, component?: index (0), shape: MaskShape JSON}", has_active, |s, p| { - let shape: MaskShape = + let mut shape: MaskShape = serde_json::from_value(p.get("shape").cloned().unwrap_or_default()).map_err(|e| bad("mask.update", e.to_string()))?; + cap_color_range_samples(&mut shape); let comp = optional_index(p, "component", "mask.update")?.unwrap_or(0); let mid = mask_id(p, s.active_mask, "mask.update")?; masks_edit(s, "mask.update", "Edit Mask", |masks, _| { @@ -1123,3 +1141,30 @@ fn pressure_list(v: Option<&Value>, points: usize) -> Result> { .collect::>>() .ok_or_else(|| bad("mask.brushStroke", "pressure must be a list of numbers 0..1")) } + +#[cfg(test)] +mod tests_color_range { + use super::*; + + /// Automation (`mask.add`/`mask.update`) used to take any number of colour-range samples; render + /// cost is pixels x samples, so a few hundred hung a large render. The list is now capped. + #[test] + fn color_range_samples_are_capped() { + let samples: Vec = (0..1000).map(|i| json!([i as f64 / 1000.0, 0.1, 0.2])).collect(); + let shape = shape_from("colorRange", &json!({ "samples": samples }), "mask.add").unwrap(); + let MaskShape::ColorRange { samples, .. } = shape else { panic!("expected ColorRange") }; + assert_eq!(samples.len(), MAX_COLOR_RANGE_SAMPLES); + // A reasonable request is unchanged. + let few = shape_from("colorRange", &json!({ "samples": [[0.1, 0.2, 0.3], [0.4, 0.5, 0.6]] }), "mask.add").unwrap(); + let MaskShape::ColorRange { samples, .. } = few else { panic!() }; + assert_eq!(samples.len(), 2); + // mask.update deserializes the shape directly; it is capped the same way. + let mut shape = shape_from("colorRange", &json!({ "samples": [[0.1, 0.2, 0.3]] }), "mask.add").unwrap(); + if let MaskShape::ColorRange { samples, .. } = &mut shape { + samples.resize(1000, samples[0]); + } + cap_color_range_samples(&mut shape); + let MaskShape::ColorRange { samples, .. } = shape else { panic!() }; + assert_eq!(samples.len(), MAX_COLOR_RANGE_SAMPLES); + } +} diff --git a/crates/engine/src/cmd/missing.rs b/crates/engine/src/cmd/missing.rs index 1bff30d2b..c6c692c35 100644 --- a/crates/engine/src/cmd/missing.rs +++ b/crates/engine/src/cmd/missing.rs @@ -197,13 +197,24 @@ impl FindPlan { } } +/// How Find Missing compares file names: the part after the last `/` or `\`, in lower case. A +/// library made on Windows keeps Windows paths, whose `\` isn't a separator to `Path` on macOS or +/// Linux. Files on disk are named by the same rule, so a macOS or Linux file name that holds a +/// `\` (`a\b.jpg`) still meets its photo, and a `b.jpg` beside it makes that match ambiguous +/// rather than a guess. With no `a\b.jpg` there, such a photo may be found as `b.jpg`, if the +/// size on record agrees. +fn name_key(path: &str) -> String { + path.rsplit(['/', '\\']).next().unwrap_or_default().to_lowercase() +} + /// The file-system half of Find Missing Photos: which `candidates` are gone, and which file in /// `folder` (recursively) is each one's. Touches only the disk — never the session — so the app /// runs it on a worker thread (walking a big folder on a network share and hashing look-alikes /// takes a while) and relinks the result with `library.findMissing {found, missing, ambiguous}`. /// /// Matching, per photo: -/// 1. a file with the same name (any letter case), the same size and the same content hash; +/// 1. a file with the same name ([`name_key`]: any letter case), the same size and the same +/// content hash; /// 2. renamed: a file of the same size with the same content hash (only same-size files are /// read, so this stays cheap); /// 3. without a hash to confirm (or the file was edited in place): the one file with the same @@ -228,8 +239,9 @@ pub fn plan_find_missing(candidates: &[FindCandidate], folder: &str) -> std::res let mut by_name: HashMap> = HashMap::new(); let files: Vec = crate::import::expand(&[folder.to_string()], None).into_iter().filter(|f| !in_use.contains(f.as_str())).collect(); for f in &files { - if let Some(n) = Path::new(f).file_name() { - by_name.entry(n.to_string_lossy().to_lowercase()).or_default().push(f.clone()); + let name = name_key(f); + if !name.is_empty() { + by_name.entry(name).or_default().push(f.clone()); } } let mut by_size: Option>> = None; @@ -239,7 +251,7 @@ pub fn plan_find_missing(candidates: &[FindCandidate], folder: &str) -> std::res let mut used: HashSet = HashSet::new(); let mut plan = FindPlan::default(); for c in &lost { - let name = Path::new(&c.path).file_name().map(|n| n.to_string_lossy().to_lowercase()).unwrap_or_default(); + let name = name_key(&c.path); let size = c.size; let hash = c.hash.as_deref(); let named: Vec<&String> = by_name diff --git a/crates/engine/src/export.rs b/crates/engine/src/export.rs index bfeade59c..0e2254afd 100644 --- a/crates/engine/src/export.rs +++ b/crates/engine/src/export.rs @@ -1827,10 +1827,14 @@ impl PreparedExport { }; let Some(dng) = dng else { // the file is copied byte for byte: refuse rather than leak a location it embeds - if remove_location && lightcraft_meta::extract(&bytes).gps.is_some() { - return Err(format!( - "{label}: can't remove the location from an original file that embeds GPS; export as JPEG, TIFF or DNG instead" - )); + if remove_location { + let metadata = lightcraft_meta::try_extract(&bytes) + .map_err(|e| format!("{label}: can't verify location metadata in this original file: {e}"))?; + if metadata.gps.is_some() { + return Err(format!( + "{label}: can't remove the location from an original file that embeds GPS; export as JPEG, TIFF or DNG instead" + )); + } } return Ok(Exported { file_name, diff --git a/crates/engine/src/files.rs b/crates/engine/src/files.rs index ae5a9ae0a..82d4c57d6 100644 --- a/crates/engine/src/files.rs +++ b/crates/engine/src/files.rs @@ -1177,6 +1177,22 @@ pub(crate) mod tests { assert_eq!(super::meta_of(&m).0.keywords, ["travel|Italy|Rome", "beach"]); } + #[test] + fn psd_probe_reads_embedded_capture_time_gps_and_title() { + let capture = lightcraft_meta::DateTime::parse_iso("2023-05-14T14:40:39").unwrap(); + let metadata = lightcraft_meta::Metadata { + capture_time: Some(capture), + gps: Some(lightcraft_meta::Gps { latitude: 45.0, longitude: 7.0, altitude: None }), + title: Some("PSD metadata".into()), + ..Default::default() + }; + let bytes = crate::tests_xmp::synthetic_psd_with_metadata(&metadata); + let probe = probe_bytes("metadata.psd", &bytes).unwrap(); + assert_eq!(probe.captured.as_deref(), Some("2023-05-14T14:40:39")); + assert_eq!(probe.meta.gps, Some((45.0, 7.0))); + assert_eq!(probe.meta.title, "PSD metadata"); + } + use super::*; use lightcraft_codecs::{ChromaSubsampling, EncodeImage, EncodeMeta, Samples, encode_jpeg}; diff --git a/crates/engine/src/import.rs b/crates/engine/src/import.rs index bc8c1c962..62bd33820 100644 --- a/crates/engine/src/import.rs +++ b/crates/engine/src/import.rs @@ -36,7 +36,7 @@ use crate::media::ProbeInfo; /// This includes preview-only raws; the file probe decides whether sensor decoding is available. pub const EXTENSIONS: &[&str] = &[ "jpg", "jpeg", "png", "tif", "tiff", "webp", "dng", "cr2", "cr3", "nef", "nrw", "arw", "sr2", "srf", "raf", "orf", "rw2", "rwl", "raw", "pef", - "srw", "psd", "jxl", "gif", "bmp", + "srw", "psd", "psb", "jxl", "gif", "bmp", // Decoded when built with the codecs' `heif` feature; otherwise the import reports them as // failed with the reason ("HEIC/HEIF support isn't included in this build"). "heic", "heif", "avif", // no AV1 decoder yet: reported as failed, like a HEIC without the feature diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 323ed90ef..4ad5695dc 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -207,6 +207,8 @@ pub struct Session { pub catalog: Catalog, pub source: LibrarySource, pub filter: Filter, + /// The filter `library.toggleFilters` put aside (Library > Enable Filters off); not persisted. + pub(crate) shelved_filter: Option, pub sort: Sort, pub selection: Selection, /// The grid order for the current source/filter/sort (cached by catalog revision). @@ -422,6 +424,7 @@ impl Session { catalog: Catalog::new(), source: LibrarySource::All, filter: Filter::default(), + shelved_filter: None, sort: Sort::default(), selection: Selection::default(), visible: Vec::new().into(), @@ -1027,6 +1030,11 @@ impl Session { // ---------------------------------------------------------------- library view + /// Library > Enable Filters is off: a filter is put aside (`library.toggleFilters`). + pub fn filters_off(&self) -> bool { + self.shelved_filter.is_some() + } + /// Photos shown in the grid/filmstrip for the current source, filter and sort. pub fn visible(&mut self) -> &[PhotoId] { let mut key = diff --git a/crates/engine/src/sync.rs b/crates/engine/src/sync.rs index 713c697a7..f20ad9c95 100644 --- a/crates/engine/src/sync.rs +++ b/crates/engine/src/sync.rs @@ -845,13 +845,12 @@ impl SyncCommit { } } SyncStep::Remove(gone) => { - let ops: Vec = gone - .iter() - .filter(|id| s.catalog.photo(**id).is_some_and(|p| p.in_library())) - .map(|id| Op::SetDeleted { id: *id, deleted: true }) - .collect(); + let ids: Vec = gone.into_iter().filter(|id| s.catalog.photo(*id).is_some_and(|p| p.in_library())).collect(); + let mut ops: Vec = ids.iter().map(|id| Op::SetDeleted { id: *id, deleted: true }).collect(); let n = ops.len(); if n > 0 { + // Like `photo.delete`, take them out of their stacks. + ops.extend(s.catalog.remove_from_stacks_ops(&ids)); match s.commit("Remove Missing Photos", Op::Batch { ops }) { Ok(()) => report.removed += n, Err(e) => report.failed.push((String::new(), e.to_string())), diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 1e8f4b7e3..218965812 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -452,8 +452,14 @@ fn command_sweep() { for c in command_specs() { assert!(ids.insert(c.id), "duplicate {}", c.id); assert!(!c.label.is_empty() && !c.params.is_empty(), "{}", c.id); + // empty params make this one download from download.geonames.org into the real settings folder; its own tests run it against a loopback server + if c.id == "metadata.downloadPlaceNames" { + continue; + } let _ = s.execute(c.id, &Value::Null); } + // empty params must never start a download from the internet + assert!(!s.place_names.download_started(), "the sweep started a place names download"); assert!(s.commands().len() >= 70, "{}", s.commands().len()); } @@ -1244,6 +1250,43 @@ fn find_similar_filters_to_look_alikes() { assert!(s.visible_cloned().len() > vis.len()); } +#[test] +fn find_similar_with_no_hits_shows_no_photos() { + let mut s = demo(); + let r = s.execute("library.findSimilar", &json!({"similarity": 1.5})).unwrap(); + assert_eq!(r["photos"].as_array().map(Vec::len), Some(0)); + assert_eq!(s.visible_cloned().len(), 0, "an empty Find Similar result must show no photos, not the whole library"); + s.execute("library.clearFilter", &json!({})).unwrap(); + assert_eq!(s.visible_cloned().len(), s.catalog.len(), "Clear Filters brings everything back"); +} + +#[test] +fn find_similar_of_a_flat_photo_shows_an_empty_view_and_counts_zero() { + let mut s = demo(); + let photo = s.catalog.photos().next().unwrap().clone(); + // a flat photo's signature is all zeros: it is not similar even to itself + s.signatures.insert(crate::media::content_key(&photo), [0.0; 64]); + let r = s.execute("library.findSimilar", &json!({"id": photo.id.0})).unwrap(); + assert_eq!(r["photos"].as_array().map(Vec::len), Some(0)); + assert_eq!(s.visible_cloned().len(), 0, "no matches show nothing, not the whole library"); + assert_eq!(s.catalog.count(&s.filter), 0); + let chips = crate::filter_chips(&s.filter, &s.catalog); + assert_eq!(chips.iter().map(|c| c.label.as_str()).collect::>(), ["Only 0 photos"]); + s.execute("library.filter", &chips[0].clear).unwrap(); + assert_eq!(s.filter, Default::default(), "the chip clears the empty state"); + assert_eq!(s.visible_cloned().len(), s.catalog.len()); +} + +#[test] +fn find_similar_values_stay_within_zero_and_one() { + let mut s = demo(); + let id = s.catalog.photos().next().unwrap().id; + let r = s.execute("library.findSimilar", &json!({"id": id.0, "similarity": -1.0})).unwrap(); + let sims: Vec = r["photos"].as_array().unwrap().iter().map(|h| h["similarity"].as_f64().unwrap()).collect(); + assert_eq!(sims.len(), s.catalog.len()); + assert!(sims.iter().all(|v| (0.0..=1.0).contains(v)), "{sims:?}"); +} + #[test] fn quick_develop_adds_to_each_photo() { use lightcraft_catalog::PhotoId; diff --git a/crates/engine/src/tests_export.rs b/crates/engine/src/tests_export.rs index 4830490b5..a835ecba4 100644 --- a/crates/engine/src/tests_export.rs +++ b/crates/engine/src/tests_export.rs @@ -4,7 +4,7 @@ use serde_json::json; use crate::Session; use crate::export::{ExportFormat, ExportOptions, export_photo}; -use crate::tests_xmp::{synthetic_dng_with, temp_dir}; +use crate::tests_xmp::{synthetic_dng_with, synthetic_psd_with_metadata, temp_dir}; #[test] fn full_size_export_of_a_cropped_photo_is_not_upscaled() { @@ -657,6 +657,92 @@ fn remove_location_refuses_a_raw_that_embeds_gps() { let _ = std::fs::remove_dir_all(&dir); } +#[test] +fn remove_location_refuses_a_psd_that_embeds_gps() { + let dir = temp_dir("export-noloc-psd"); + let gps = lightcraft_meta::Metadata { gps: Some(lightcraft_meta::Gps { latitude: 45.0, longitude: 7.0, altitude: None }), ..Default::default() }; + let path = dir.join("Geo.psd"); + let bytes = synthetic_psd_with_metadata(&gps); + assert_eq!(lightcraft_meta::extract(&bytes).gps, gps.gps, "the fixture embeds EXIF GPS"); + std::fs::write(&path, &bytes).unwrap(); + let mut session = Session::new().with_fs(); + session.execute("library.import", &json!({"paths": [path.to_string_lossy()]})).unwrap(); + let id = session.catalog.photos().next().unwrap().id; + let err = export_photo(&mut session, id, &ExportOptions::from_json(&json!({"format": "original", "removeLocation": true})), 1).err().unwrap(); + assert!(err.contains("can't remove the location") && err.contains("export as JPEG"), "{err}"); + let _ = std::fs::remove_dir_all(&dir); +} + +#[test] +fn remove_location_refuses_a_psd_with_unreadable_xmp() { + let dir = temp_dir("export-noloc-psd-xmp"); + let path = dir.join("Unreadable.psd"); + let pixels = [40, 80, 120]; + let image = lightcraft_codecs::EncodeImage::new(1, 1, 3, lightcraft_codecs::Samples::U8(&pixels)); + let xmp = " path.clone(), + _ => String::new(), + }; + let r = s.execute("library.findMissing", &json!({"folder": dir.join("found").to_string_lossy()})).unwrap(); + assert_eq!(path_of(&s, a), dir.join("found").join("a.jpg").to_string_lossy(), "found by its name: {r}"); + assert_eq!(path_of(&s, b), dir.join("found").join("deep").join("b.jpg").to_string_lossy(), "found by its name, any case, and size: {r}"); + assert_eq!(path_of(&s, twin), r"C:\Users\me\Photos\twin.jpg", "two files of its name: not guessed: {r}"); + assert_eq!(r["ambiguous"][0]["candidates"].as_array().map(Vec::len), Some(2), "both look-alikes reported: {r}"); + assert_eq!(r["missing"], 1, "{r}"); + s.execute("edit.undo", &json!({})).unwrap(); + assert_eq!( + (path_of(&s, a), path_of(&s, b)), + (r"C:\Users\me\Photos\a.jpg".to_string(), r"D:\Shoot\2019\B.JPG".to_string()), + "one undo step puts back the Windows paths as they were spelled" + ); + let _ = std::fs::remove_dir_all(&dir); +} + +/// On macOS and Linux `\` may be part of a file name. Find Missing names the files it finds by +/// the same rule as the missing photos, so `a\b.jpg` isn't relinked to a `b.jpg` beside it +/// unasked: the two are reported as look-alikes. +#[cfg(unix)] +#[test] +fn find_missing_does_not_take_a_look_alike_for_a_file_name_with_a_backslash() { + use lightcraft_catalog::{Op, Photo, Source}; + let dir = temp_dir("missing-backslash"); + std::fs::create_dir_all(dir.join("found")).unwrap(); + for name in [r"a\b.jpg", "b.jpg"] { + std::fs::write(dir.join("found").join(name), b"x").unwrap(); + } + let mut s = Session::new(); + let gone = dir.join("gone").join(r"a\b.jpg").to_string_lossy().to_string(); + let id = s.catalog.alloc_photo_id(); + let p = Photo::new(id, Source::File { path: gone.clone() }, "x.jpg", "JPEG", 60, 40, "2026-01-01T10:00:00"); + s.catalog.apply(Op::AddPhoto { photo: Box::new(p) }).unwrap(); + let r = s.execute("library.findMissing", &json!({"folder": dir.join("found").to_string_lossy()})).unwrap(); + assert!(matches!(&s.catalog.photo(id).unwrap().source, Source::File { path } if *path == gone), "not relinked to b.jpg: {r}"); + assert_eq!(r["ambiguous"][0]["candidates"].as_array().map(Vec::len), Some(2), "both reported: {r}"); + let _ = std::fs::remove_dir_all(&dir); +} + #[test] fn missing_files_are_found_and_relinked() { let dir = std::env::temp_dir().join(format!("lc-missing-{}", std::process::id())); diff --git a/crates/engine/src/tests_sync.rs b/crates/engine/src/tests_sync.rs index 88c184335..8b1ec4f67 100644 --- a/crates/engine/src/tests_sync.rs +++ b/crates/engine/src/tests_sync.rs @@ -739,3 +739,21 @@ fn a_cancelled_scan_does_not_check_the_cached_probes_against_the_disk() { let new = out.candidates.iter().find(|c| c.name == "new.png").expect("new.png is a candidate"); assert!(new.error.is_some(), "the cached probe was used after ✕: {new:?}"); } + +#[test] +fn removing_missing_photos_takes_them_out_of_their_stacks() { + let dir = Scratch::new("remove-stacked"); + let mut s = library(&dir); + let id = |s: &Session, name: &str| s.catalog.photos().find(|p| p.file_name == name).unwrap().id; + let (a, b) = (id(&s, "a.png"), id(&s, "b.png")); + let group = s.catalog.group_ops(a, &[b], true).unwrap(); + s.catalog.apply(group).unwrap(); + assert!(s.catalog.stack_of(b).is_some()); + std::fs::remove_file(dir.path("trip/day1/b.png")).unwrap(); + let r = s.execute("folder.synchronize", &json!({"path": dir.path("trip"), "removeMissing": true})).unwrap(); + assert_eq!(r["removed"].as_u64(), Some(1), "{r}"); + assert!(s.catalog.photo(b).unwrap().deleted); + assert!(s.catalog.stack_of(b).is_none(), "like photo.delete, a removed photo leaves its stack: {:?}", s.catalog.stack_of(b)); + s.execute("edit.undo", &json!({})).unwrap(); + assert!(s.catalog.stack_of(b).is_some(), "one undo step puts b back in its stack"); +} diff --git a/crates/engine/src/tests_xmp.rs b/crates/engine/src/tests_xmp.rs index 1a4d86fa3..129662e4d 100644 --- a/crates/engine/src/tests_xmp.rs +++ b/crates/engine/src/tests_xmp.rs @@ -43,6 +43,34 @@ fn write_png(path: &Path, seed: u8) { std::fs::write(path, bytes).unwrap(); } +pub(crate) fn synthetic_psd_with_metadata(metadata: &lightcraft_meta::Metadata) -> Vec { + let exif = lightcraft_meta::write_exif(metadata); + let xmp = lightcraft_meta::write_xmp(metadata, None); + let pixels = [40, 80, 120]; + let image = lightcraft_codecs::EncodeImage::new(1, 1, 3, lightcraft_codecs::Samples::U8(&pixels)); + let embedded = lightcraft_codecs::EncodeMeta { exif: Some(&exif), xmp: Some(&xmp), ..Default::default() }; + lightcraft_codecs::encode_psd(&image, &embedded).unwrap() +} + +pub(crate) fn synthetic_psb_with_metadata(metadata: &lightcraft_meta::Metadata) -> Vec { + let exif = lightcraft_meta::write_exif(metadata); + let xmp = lightcraft_meta::write_xmp(metadata, None); + let pixels = [1024u16, 2048, 3072]; + let image = lightcraft_codecs::EncodeImage::new(1, 1, 3, lightcraft_codecs::Samples::U16(&pixels)); + let embedded = lightcraft_codecs::EncodeMeta { exif: Some(&exif), xmp: Some(&xmp), ..Default::default() }; + let psd = lightcraft_codecs::encode_psd(&image, &embedded).unwrap(); + let colour_length = usize::try_from(u32::from_be_bytes(psd[26..30].try_into().unwrap())).unwrap(); + let resource_length_offset = 30 + colour_length; + let resource_length = usize::try_from(u32::from_be_bytes(psd[resource_length_offset..resource_length_offset + 4].try_into().unwrap())).unwrap(); + let resource_end = resource_length_offset + 4 + resource_length; + + let mut psb = psd[..resource_end].to_vec(); + psb[4..6].copy_from_slice(&2u16.to_be_bytes()); + psb.extend_from_slice(&0u64.to_be_bytes()); // PSB layer and mask information length + psb.extend_from_slice(&psd[resource_end + 4..]); // compression and uncompressed 16-bit pixel planes + psb +} + fn only(s: &Session) -> PhotoId { s.catalog.photos().next().unwrap().id } @@ -1036,10 +1064,21 @@ fn external_edit_copies_every_selected_photo() { fn external_edit_copy_as_psd_round_trips() { let dir = temp_dir("extedit-psd"); let src = dir.join("pier.png"); - write_png(&src, 40); + let capture = lightcraft_meta::DateTime::parse_iso("2023-05-14T14:40:39").unwrap(); + let source_meta = lightcraft_meta::Metadata { capture_time: Some(capture), title: Some("Dated original".into()), ..Default::default() }; + let exif = lightcraft_meta::write_exif(&source_meta); + let xmp = lightcraft_meta::write_xmp(&source_meta, None); + let rgba = lightcraft_raster::Rgba8::from_fn(40, 24, |x, y| [(x * 6) as u8, (y * 9) as u8, 40, 255]); + let png = lightcraft_codecs::encode_png( + &lightcraft_codecs::EncodeImage::rgba8(&rgba), + &lightcraft_codecs::EncodeMeta { exif: Some(&exif), xmp: Some(&xmp), ..Default::default() }, + ) + .unwrap(); + std::fs::write(&src, png).unwrap(); let mut s = Session::new().with_fs(); s.execute("library.import", &json!({"paths": [src.to_string_lossy()]})).unwrap(); let orig = s.active().unwrap(); + assert_eq!(s.catalog.photo(orig).unwrap().captured.as_deref(), Some("2023-05-14T14:40:39")); s.execute("develop.set", &json!({"control": "light.exposure", "value": 0.5})).unwrap(); let r = s.execute("photo.editExternal", &json!({"format": "psd", "bitDepth": 16})).unwrap(); let path = r["path"].as_str().unwrap().to_string(); @@ -1048,6 +1087,13 @@ fn external_edit_copy_as_psd_round_trips() { assert_eq!(d.bit_depth, 16); assert!(d.icc.is_some(), "the colour space travels with the file"); let new = lightcraft_catalog::PhotoId(r["id"].as_u64().unwrap()); + assert_eq!(s.catalog.photo(new).unwrap().captured.as_deref(), Some("2023-05-14T14:40:39")); + let all_metadata = s.execute("photo.allMetadata", &json!({"id": new.0})).unwrap(); + assert!(all_metadata["exif"].as_array().unwrap().iter().any(|row| row["tag"] == json!(0x9003)), "{all_metadata}"); + assert!( + all_metadata["xmp"].as_array().unwrap().iter().any(|row| row["name"] == "dc:title" && row["value"] == "Dated original"), + "{all_metadata}" + ); assert_eq!(s.catalog.stack_of(new).map(|st| (st.top(), st.photos.contains(&orig))), Some((new, true))); // the editor saves the document back over the same file: Reload shows its pixels let before = s.render_now(new, 16, 16).unwrap().image.data; @@ -1069,6 +1115,33 @@ fn external_edit_copy_as_psd_round_trips() { let _ = std::fs::remove_dir_all(&dir); } +#[test] +fn psb_import_retains_capture_time_and_all_metadata() { + let dir = temp_dir("import-psb-metadata"); + let path = dir.join("dated.psb"); + let capture = lightcraft_meta::DateTime::parse_exif("2023:05:14 14:40:39"); + let metadata = lightcraft_meta::Metadata { + capture_time: capture, + gps: Some(lightcraft_meta::Gps { latitude: 45.0, longitude: 7.0, altitude: None }), + title: Some("PSB original".into()), + ..Default::default() + }; + let bytes = synthetic_psb_with_metadata(&metadata); + assert_eq!(lightcraft_meta::extract(&bytes).capture_time, capture); + assert_eq!(lightcraft_meta::extract(&bytes).gps, metadata.gps); + std::fs::write(&path, bytes).unwrap(); + + let mut session = Session::new().with_fs(); + let imported = session.execute("library.import", &json!({"paths": [path.to_string_lossy()]})).unwrap(); + assert_eq!(imported["imported"].as_array().map(Vec::len), Some(1), "{imported}"); + let id = session.catalog.photos().next().unwrap().id; + assert_eq!(session.catalog.photo(id).unwrap().captured.as_deref(), Some("2023-05-14T14:40:39")); + let all_metadata = session.execute("photo.allMetadata", &json!({"id": id.0})).unwrap(); + assert!(all_metadata["exif"].as_array().unwrap().iter().any(|row| row["tag"] == json!(0x9003)), "{all_metadata}"); + assert!(all_metadata["xmp"].as_array().unwrap().iter().any(|row| row["name"] == "dc:title" && row["value"] == "PSB original"), "{all_metadata}"); + let _ = std::fs::remove_dir_all(&dir); +} + /// Reload: a file changed on disk (an external editor saved it) gets its new size / hash, the /// cached source is dropped and renders change; unchanged files are left alone. #[test] @@ -1375,6 +1448,38 @@ const OTHER_APP_SIDECAR: &str = r#" "#; +/// A foreign namespace may use a familiar prefix without becoming LightCraft-owned metadata. +#[test] +fn saving_preserves_foreign_namespace_prefix_collisions() { + let src = temp_dir("namespace-src"); + let lib = temp_dir("namespace-lib"); + write_png(&src.join("shot.png"), 1); + let foreign = r#"also keep"#; + let packet = format!(r#"{foreign}"#); + let path = src.join("shot.xmp"); + std::fs::write(&path, packet).unwrap(); + let mut s = Session::new().with_fs(); + s.open_library(&lib, false).unwrap(); + s.execute("library.import", &json!({"paths": [src.join("shot.png")]})).unwrap(); + let id = only(&s); + assert_eq!(s.catalog.photo(id).unwrap().rating, 0, "a foreign rating must not become catalog metadata"); + s.execute("library.select", &json!({"ids": [id.0]})).unwrap(); + s.execute("photo.rate", &json!({"rating": 5})).unwrap(); + for _ in 0..2 { + let result = s.execute("photo.saveMetadataToFile", &json!({})).unwrap(); + assert_eq!(result["failed"], json!([]), "{result}"); + assert_eq!(result["backups"], json!([]), "a valid packet must merge: {result}"); + assert_eq!(result["merged"], json!([path.to_string_lossy()]), "{result}"); + let saved = std::fs::read_to_string(&path).unwrap(); + assert!(saved.contains(foreign), "lost foreign bytes: {saved}"); + assert_eq!(lightcraft_meta::parse_xmp(&saved).unwrap().metadata.rating, Some(5)); + } + drop(s); + std::fs::remove_dir_all(src).unwrap(); + std::fs::remove_dir_all(lib).unwrap(); +} + /// Issue #92: saving merges into another app's sidecar instead of replacing it. #[test] fn saving_merges_into_another_apps_sidecar() { diff --git a/crates/engine/src/view.rs b/crates/engine/src/view.rs index 5b32999bb..0d5cde3cf 100644 --- a/crates/engine/src/view.rs +++ b/crates/engine/src/view.rs @@ -214,8 +214,9 @@ pub fn filter_chips(f: &Filter, cat: &Catalog) -> Vec { if let Some(rs) = f.rule_set.as_ref().filter(|r| !r.rules.is_empty()) { add(format!("Rules: {}", rs.describe_with(cat)), json!({"ruleSet": Null})); } - if !f.only.is_empty() { - add(format!("Only {} photos", f.only.len()), json!({"only": []})); + if f.only_none || !f.only.is_empty() { + let n = if f.only_none { 0 } else { f.only.len() }; + add(format!("Only {n} photos"), json!({"only": [], "onlyNone": false})); } v } diff --git a/crates/meta/README.md b/crates/meta/README.md index 83ee9e601..519426784 100644 --- a/crates/meta/README.md +++ b/crates/meta/README.md @@ -8,11 +8,12 @@ Photo metadata: EXIF, XMP (read + write), IPTC-IIM, and the containers that carr |---|---| | `read_exif(&[u8]) -> Metadata` | TIFF-structured Exif block (whole TIFF/DNG/raw file, or JPEG APP1 payload with/without `Exif\0\0`). `try_read_exif` reports header errors. | | `from_tiff(&Tiff) -> Metadata` | Same, from an already parsed stream (used by `lightcraft-raw`). | -| `embedded(&[u8]) -> Embedded` | Sniffs JPEG / PNG / WebP / TIFF and returns the Exif, XMP (+ extended XMP), ICC and IPTC blocks. `jpeg_segments`, `png_chunks`, `webp_chunks` are the per-container walkers. | +| `embedded(&[u8]) -> Embedded` | Sniffs JPEG / PNG / WebP / TIFF / PSD / PSB and returns the Exif, XMP (+ extended XMP), ICC and IPTC blocks. PSD/PSB EXIF resource 1058 and XMP resource 1060 are read from the bounded image-resource section without decoding pixels. `jpeg_segments`, `png_chunks`, `webp_chunks` are the per-container walkers. | | `parse_xmp(&str) -> Result` | Metadata + the opaque `lc:settings` JSON + every property (`prefix:name`, structs flattened as `a:b/c:d`). | | `write_xmp(&Metadata, Option<&str>) -> String` | Complete `` packet; the second argument is LightCraft's full develop settings JSON. | | `parse_iptc(&[u8]) -> Metadata` | IIM record 2: title (2:5), keywords (2:25), by-line (2:80), copyright (2:116), caption (2:120), date/time created (2:55/2:60). | | `extract(&[u8]) -> Metadata` | Everything for a whole file. Precedence: EXIF → IPTC fills gaps → XMP overrides user fields (rating, label, title, caption, artist, copyright, keywords, GPS) and fills the rest. | +| `try_extract(&[u8]) -> Result` | Like `extract`, but reports malformed PSD/PSB headers, image-resource sections, or embedded EXIF/XMP structure (including duplicate EXIF/XMP resources) so callers can fail closed when location absence cannot be established. Other formats remain best-effort. | `Metadata` carries camera (make, model, serial, software), lens (make, model, serial, specification), capture (date-time with UTC offset and milliseconds, exposure time, f-number, ISO incl. extended ISO, focal length and @@ -27,10 +28,19 @@ prefixes by namespace URI, handles attribute and element forms, `rdf:Seq`/`Bag`/ `rdf:parseType="Resource"` and nested-description structs, predefined and numeric entities. Nesting depth and node count are limited. +An explicitly foreign namespace using a recognized prefix (for example `lc` or `xmp`) cannot supply +recognized properties. Its keys use `Q{hex-encoded namespace URI}local` in `properties` and `values`; +hex encoding keeps `/` reserved for struct paths. Other unknown prefixes keep their existing QName keys. +Merging preserves the original foreign XML rather than writing these opaque keys into the packet. +The parser and merge scanner each cap generated opaque names at 8 MiB per packet, returning `TooComplex` +if repeated use of a long namespace URI would exceed that budget. + ## Robustness No panics on malformed input: proptests feed random bytes to every container walker and random text to the -XMP parser; truncation tests cut a sample JPEG at every byte. +XMP parser; truncation tests cut a sample JPEG at every byte and exercise truncated PSD/PSB headers and +image-resource records with hostile declared lengths. +PSD/PSB XMP resources require valid UTF-8; trailing NUL padding is trimmed after validation. ## Sources diff --git a/crates/meta/src/container.rs b/crates/meta/src/container.rs index 3ac60907f..3cfb7e287 100644 --- a/crates/meta/src/container.rs +++ b/crates/meta/src/container.rs @@ -7,6 +7,7 @@ //! - WebP (RIFF container spec): `EXIF`, `XMP `, `ICCP` chunks. //! - TIFF-based files (TIFF, DNG and most raws): the whole file is the Exif block; XMP (700), ICC (34675) and //! IPTC (33723) from IFD0. +//! - Photoshop PSD/PSB: EXIF (image resource 1058) and XMP (image resource 1060) from the bounded image-resource section. //! - Minolta MRW (see [`crate::mrw`]): the `TTW` block is the Exif block. //! - Canon CR3 (ISO base media file, see [`crate::cr3`]): `CMT1`/`CMT2`/`CMT4` merged into one Exif block; the //! XMP `uuid` box. @@ -29,6 +30,9 @@ pub struct Embedded { /// Sniff the container and collect its metadata blocks. Unknown containers yield an empty result. pub fn embedded(bytes: &[u8]) -> Embedded { + if bytes.starts_with(b"8BPS") { + return crate::psd::embedded(bytes); + } if bytes.starts_with(b"FUJIFILMCCD-RAW") { // Fujifilm RAF: metadata lives in the embedded JPEG whose (offset, length) are big-endian u32s at byte 84 let be = |i: usize| bytes.get(i..i + 4).map(|s| u32::from_be_bytes([s[0], s[1], s[2], s[3]]) as usize); @@ -71,6 +75,12 @@ pub fn embedded(bytes: &[u8]) -> Embedded { } } +/// Locate metadata blocks while preserving PSD/PSB resource-structure errors for callers that +/// must establish whether embedded GPS is absent. Other containers keep their best-effort walk. +pub(crate) fn try_embedded(bytes: &[u8]) -> Result { + if bytes.starts_with(b"8BPS") { crate::psd::parse(bytes).ok_or(crate::ExtractError::MalformedPsd) } else { Ok(embedded(bytes)) } +} + const XMP_SIG: &[u8] = b"http://ns.adobe.com/xap/1.0/\0"; const XMP_EXT_SIG: &[u8] = b"http://ns.adobe.com/xmp/extension/\0"; diff --git a/crates/meta/src/lib.rs b/crates/meta/src/lib.rs index 59062dfda..baebbfe0f 100644 --- a/crates/meta/src/lib.rs +++ b/crates/meta/src/lib.rs @@ -7,7 +7,7 @@ //! - [`parse_xmp`] / [`write_xmp`] — XMP packets (interchange fields + the opaque `lc:settings` JSON). //! - [`parse_iptc`] — IPTC-IIM record 2 datasets. //! - [`parse_gpx`] — GPS track logs (GPX) for geotagging by capture time. -//! - [`extract`] — all of the above for a whole file (JPEG, PNG, WebP, TIFF/DNG/raw, Canon CR3). +//! - [`extract`] — all of the above for a whole file (JPEG, PNG, WebP, TIFF/DNG/raw, PSD/PSB, Canon CR3). //! - [`cr3`] — the Canon CR3 container (metadata blocks, XMP, the JPEG and raw tracks). #![forbid(unsafe_code)] @@ -20,6 +20,7 @@ mod focus_read; mod gpx; mod iptc; pub mod mrw; +mod psd; pub mod tags; mod xmp; mod xmp_merge; @@ -282,17 +283,57 @@ impl Metadata { /// Precedence: EXIF, then IPTC fills gaps, then XMP overrides user fields and fills the rest. pub fn extract(bytes: &[u8]) -> Metadata { let e = embedded(bytes); - let mut m = e.exif.as_deref().map(read_exif).unwrap_or_default(); + let exif = e.exif.as_deref().map(read_exif).unwrap_or_default(); + let xmp = e.xmp.as_deref().and_then(|xmp| parse_xmp(xmp).ok()).map(|xmp| xmp.metadata); + extract_embedded(e, exif, xmp) +} + +/// Read metadata while reporting malformed PSD/PSB containers, embedded EXIF, or embedded XMP instead of treating +/// them as empty. Other container formats retain [`extract`]'s best-effort behavior. +pub fn try_extract(bytes: &[u8]) -> Result { + let e = container::try_embedded(bytes)?; + let is_psd = bytes.starts_with(b"8BPS"); + let exif = match e.exif.as_deref() { + Some(exif) if is_psd => try_read_exif(exif).map_err(|_| ExtractError::MalformedPsd)?, + Some(exif) => read_exif(exif), + None => Metadata::default(), + }; + let xmp = match e.xmp.as_deref() { + Some(xmp) if is_psd => Some(parse_xmp(xmp).map_err(|_| ExtractError::MalformedPsd)?.metadata), + Some(xmp) => parse_xmp(xmp).ok().map(|xmp| xmp.metadata), + None => None, + }; + Ok(extract_embedded(e, exif, xmp)) +} + +fn extract_embedded(e: Embedded, mut m: Metadata, xmp: Option) -> Metadata { if let Some(i) = e.iptc.as_deref() { m.fill_missing(&parse_iptc(i)); } - if let Some(x) = e.xmp.as_deref().and_then(|x| parse_xmp(x).ok()) { - m.overlay_user_fields(&x.metadata); - m.fill_missing(&x.metadata); + if let Some(xmp) = xmp { + m.overlay_user_fields(&xmp); + m.fill_missing(&xmp); } m } +/// A container-level error reported by [`try_extract`]. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum ExtractError { + /// The PSD/PSB header, image-resource section, or embedded EXIF/XMP structure is malformed. + MalformedPsd, +} + +impl std::fmt::Display for ExtractError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::MalformedPsd => f.write_str("malformed PSD/PSB header, image-resource section, or embedded EXIF/XMP structure"), + } + } +} + +impl std::error::Error for ExtractError {} + /// A lens name from its focal and aperture range (Exif LensSpecification, or a Nikon maker note's Lens), written the /// way Lightroom shows a lens it has no name for: `50.0 mm f/1.8`, `18.0-135.0 mm f/3.5-5.6`. pub fn lens_spec_name(spec: [f64; 4]) -> Option { @@ -351,6 +392,17 @@ mod tests { assert_eq!(m.exposure_display(), None); } + #[test] + fn try_extract_keeps_non_psd_exif_best_effort() { + let payload = b"Exif\0\0not a TIFF stream"; + let mut jpeg = vec![0xff, 0xd8, 0xff, 0xe1]; + jpeg.extend_from_slice(&u16::try_from(payload.len() + 2).unwrap().to_be_bytes()); + jpeg.extend_from_slice(payload); + jpeg.extend_from_slice(&[0xff, 0xd9]); + + assert!(matches!(try_extract(&jpeg), Ok(metadata) if metadata == Metadata::default())); + } + #[test] fn merge_rules() { let mut a = Metadata { make: Some("A".into()), rating: Some(1), ..Default::default() }; diff --git a/crates/meta/src/psd.rs b/crates/meta/src/psd.rs new file mode 100644 index 000000000..bda16d960 --- /dev/null +++ b/crates/meta/src/psd.rs @@ -0,0 +1,285 @@ +//! EXIF and XMP image resources in Photoshop PSD and PSB files. + +use crate::container::Embedded; + +struct Reader<'a> { + bytes: &'a [u8], + offset: usize, +} + +impl<'a> Reader<'a> { + fn take(&mut self, length: usize) -> Option<&'a [u8]> { + let end = self.offset.checked_add(length)?; + let bytes = self.bytes.get(self.offset..end)?; + self.offset = end; + Some(bytes) + } + + fn u16(&mut self) -> Option { + let bytes = self.take(2)?; + Some(u16::from_be_bytes([*bytes.first()?, *bytes.get(1)?])) + } + + fn u32(&mut self) -> Option { + let bytes = self.take(4)?; + Some(u32::from_be_bytes([*bytes.first()?, *bytes.get(1)?, *bytes.get(2)?, *bytes.get(3)?])) + } +} + +pub(crate) fn embedded(bytes: &[u8]) -> Embedded { + parse(bytes).unwrap_or_default() +} + +pub(crate) fn parse(bytes: &[u8]) -> Option { + let mut reader = Reader { bytes, offset: 0 }; + if reader.take(4)? != b"8BPS" || !matches!(reader.u16()?, 1 | 2) { + return None; + } + // The fixed header after the version is reserved bytes, channels, dimensions, depth and colour mode. + reader.take(6 + 2 + 4 + 4 + 2 + 2)?; + let colour_mode_length = usize::try_from(reader.u32()?).ok()?; + reader.take(colour_mode_length)?; + let resource_length = usize::try_from(reader.u32()?).ok()?; + let resources = reader.take(resource_length)?; + parse_resources(resources) +} + +fn parse_resources(bytes: &[u8]) -> Option { + let mut reader = Reader { bytes, offset: 0 }; + let (mut exif, mut xmp) = (None, None); + while reader.offset < bytes.len() { + if reader.take(4)? != b"8BIM" { + return None; + } + let id = reader.u16()?; + let name_length = usize::from(*reader.take(1)?.first()?); + let pascal_length = 1usize.checked_add(name_length)?.checked_add(1)? & !1; + reader.take(pascal_length.checked_sub(1)?)?; + let data_length = usize::try_from(reader.u32()?).ok()?; + let data = reader.take(data_length)?; + if !data_length.is_multiple_of(2) { + reader.take(1)?; + } + match id { + 1058 if exif.is_some() => return None, + 1058 => exif = Some(data), + 1060 if xmp.is_some() => return None, + 1060 => xmp = Some(std::str::from_utf8(data).ok()?), + _ => {} + } + } + + Some(Embedded { + exif: exif.map(crate::strip_exif_header).map(<[u8]>::to_vec), + xmp: xmp.map(|text| text.trim_end_matches('\0').to_string()), + ..Default::default() + }) +} + +#[cfg(test)] +mod tests { + use super::embedded; + use crate::container::Embedded; + use crate::{DateTime, Gps, Metadata}; + + fn resource(id: u16, name: &[u8], data: &[u8]) -> Vec { + let mut out = b"8BIM".to_vec(); + out.extend_from_slice(&id.to_be_bytes()); + out.push(u8::try_from(name.len()).unwrap()); + out.extend_from_slice(name); + if !(name.len() + 1).is_multiple_of(2) { + out.push(0); + } + out.extend_from_slice(&u32::try_from(data.len()).unwrap().to_be_bytes()); + out.extend_from_slice(data); + if !data.len().is_multiple_of(2) { + out.push(0); + } + out + } + + fn image(version: u16, colour_data: &[u8], resources: &[u8]) -> Vec { + let mut out = b"8BPS".to_vec(); + out.extend_from_slice(&version.to_be_bytes()); + out.extend_from_slice(&[0; 6]); + out.extend_from_slice(&3u16.to_be_bytes()); // channels + out.extend_from_slice(&1u32.to_be_bytes()); // height + out.extend_from_slice(&1u32.to_be_bytes()); // width + out.extend_from_slice(&8u16.to_be_bytes()); // depth + out.extend_from_slice(&3u16.to_be_bytes()); // RGB + out.extend_from_slice(&u32::try_from(colour_data.len()).unwrap().to_be_bytes()); + out.extend_from_slice(colour_data); + out.extend_from_slice(&u32::try_from(resources.len()).unwrap().to_be_bytes()); + out.extend_from_slice(resources); + out + } + + fn dated_metadata() -> Metadata { + Metadata { + capture_time: DateTime::parse_exif("2023:05:14 14:40:39"), + gps: Some(Gps { latitude: 45.0, longitude: 7.0, altitude: None }), + ..Default::default() + } + } + + fn dated_image(version: u16) -> Vec { + let exif = crate::write_exif(&dated_metadata()); + image(version, &[], &resource(1058, b"", &exif)) + } + + #[test] + fn psd_capture_time_is_extracted() { + let expected = dated_metadata().capture_time; + let actual = crate::extract(&dated_image(1)); + assert_eq!(actual.capture_time, expected); + assert_eq!(actual.gps, dated_metadata().gps); + } + + #[test] + fn psb_capture_time_is_extracted() { + let expected = dated_metadata().capture_time; + let actual = crate::extract(&dated_image(2)); + assert_eq!(actual.capture_time, expected); + assert_eq!(actual.gps, dated_metadata().gps); + } + + #[test] + fn psd_extracts_xmp_without_exif() { + let metadata = Metadata { + capture_time: DateTime::parse_iso("2024-02-03T04:05:06Z"), + gps: Some(Gps { latitude: -33.9, longitude: 151.2, altitude: Some(25.0) }), + title: Some("Embedded XMP".into()), + ..Default::default() + }; + let xmp = crate::write_xmp(&metadata, None); + let file = image(1, &[], &resource(1060, b"", xmp.as_bytes())); + let actual = crate::extract(&file); + assert_eq!(actual.capture_time, metadata.capture_time); + assert_eq!(actual.gps, metadata.gps); + assert_eq!(actual.title, metadata.title); + } + + #[test] + fn psd_normalizes_the_optional_exif_resource_prefix() { + let mut exif = b"Exif\0\0".to_vec(); + exif.extend(crate::write_exif(&dated_metadata())); + let file = image(1, &[], &resource(1058, b"", &exif)); + assert_eq!(crate::extract(&file).capture_time, dated_metadata().capture_time); + } + + #[test] + fn try_extract_rejects_structurally_malformed_psd_exif() { + for version in [1, 2] { + let file = image(version, &[], &resource(1058, b"", b"not a TIFF stream")); + assert!(matches!(crate::try_extract(&file), Err(crate::ExtractError::MalformedPsd))); + } + } + + #[test] + fn try_extract_accepts_unknown_exif_tags_in_psd() { + let mut ifd0 = lightcraft_tiff::IfdBuilder::new(); + ifd0.set(0xc001, lightcraft_tiff::Value::Ascii("vendor metadata".into())); + let exif = lightcraft_tiff::TiffWriter::default().write(&[ifd0]).unwrap(); + let file = image(1, &[], &resource(1058, b"", &exif)); + assert!(crate::try_extract(&file).is_ok()); + } + + #[test] + fn try_extract_rejects_malformed_psd_xmp_but_extract_remains_best_effort() { + let file = image(1, &[], &resource(1060, b"", b" Vec { } /// [`tag_rows`] of a file's bytes: a TIFF-based file directly, else its embedded EXIF block -/// (JPEG, PNG, WebP, HEIF…). Empty when there is none. +/// (JPEG, PNG, WebP, HEIF, PSD/PSB…). Empty when there is none. pub fn file_tag_rows(bytes: &[u8]) -> Vec { if Tiff::sniff(bytes).is_some() && let Ok(t) = Tiff::parse(bytes) diff --git a/crates/meta/src/xmp.rs b/crates/meta/src/xmp.rs index 1fb789164..2dbf59482 100644 --- a/crates/meta/src/xmp.rs +++ b/crates/meta/src/xmp.rs @@ -69,7 +69,8 @@ pub struct XmpData { /// The opaque `lc:settings` JSON string, if present. pub lc_settings: Option, /// Every property found, keyed `prefix:name` (struct fields as `prefix:name/prefix:field`); arrays keep - /// item order, language alternatives put `x-default` first. + /// item order, language alternatives put `x-default` first. Foreign namespaces that reuse a recognized + /// prefix use `Q{hex-encoded namespace URI}local` instead, so they cannot impersonate known properties. pub properties: BTreeMap>, /// Every top-level property with its full structure (arrays of structs such as local /// corrections, which [`XmpData::properties`] flattens). @@ -150,26 +151,46 @@ impl Node { const MAX_DEPTH: usize = 64; const MAX_NODES: usize = 200_000; +// Expanding a long namespace URI on many short names must not amplify a small packet without bound. +pub(crate) const MAX_OPAQUE_NAME_BYTES: usize = 8 * 1024 * 1024; -/// `prefix:local` with the prefix canonicalised by namespace URI (see [`NAMESPACES`]). -pub(crate) fn canonical_name(qname: &str, scopes: &[Vec<(String, String)>]) -> String { - canonical(qname, scopes) +/// Names canonicalised by namespace URI (see [`NAMESPACES`]); conflicting foreign prefixes are opaque. +pub(crate) fn canonical_name(qname: &str, scopes: &[Vec<(String, String)>], opaque_budget: &mut usize) -> Result { + canonical(qname, scopes, opaque_budget) } -fn canonical(qname: &str, scopes: &[Vec<(String, String)>]) -> String { +fn canonical(qname: &str, scopes: &[Vec<(String, String)>], opaque_budget: &mut usize) -> Result { let (prefix, local) = qname.split_once(':').unwrap_or(("", qname)); if prefix == "xml" || prefix == "xmlns" { - return qname.to_string(); + return Ok(qname.to_string()); } let uri = scopes.iter().rev().flat_map(|s| s.iter().rev()).find(|(p, _)| p == prefix).map(|(_, u)| u.as_str()); - match uri { + Ok(match uri { Some(u) => match NAMESPACES.iter().find(|(_, nu)| *nu == u) { Some((p, _)) => format!("{p}:{local}"), + None if NAMESPACES.iter().any(|(p, _)| *p == prefix) => { + // A foreign `lc:settings` must never match our `lc:*` ownership rule (nor may a + // foreign `xmp:Rating` become a rating). Braces cannot occur in an XML QName; + // hex keeps URI slashes out of the flattened property's `/` path separators. + let size = + u.len().checked_mul(2).and_then(|n| n.checked_add(3)).and_then(|n| n.checked_add(local.len())).ok_or(XmpError::TooComplex)?; + *opaque_budget = opaque_budget.checked_sub(size).ok_or(XmpError::TooComplex)?; + let mut key = String::with_capacity(size); + key.push_str("Q{"); + for byte in u.bytes() { + for nibble in [byte >> 4, byte & 15] { + key.push(char::from(if nibble < 10 { b'0' + nibble } else { b'a' + (nibble - 10) })); + } + } + key.push('}'); + key.push_str(local); + key + } None if prefix.is_empty() => local.to_string(), None => qname.to_string(), }, None => qname.to_string(), - } + }) } fn entity(name: &str) -> Option { @@ -202,7 +223,7 @@ fn xml_safe(s: &str) -> String { } /// Unescape attribute values (predefined + numeric references; unknown references kept verbatim). -fn unescape_attr(s: &str) -> String { +pub(crate) fn unescape_attr(s: &str) -> String { let mut out = String::with_capacity(s.len()); let mut rest = s; while let Some(i) = rest.find('&') { @@ -229,6 +250,7 @@ fn parse_tree(s: &str) -> Result { let mut stack: Vec = vec![Node { name: "#root".into(), ..Default::default() }]; let mut scopes: Vec> = vec![vec![("xml".into(), "http://www.w3.org/XML/1998/namespace".into())]]; let mut count = 0usize; + let mut opaque_budget = MAX_OPAQUE_NAME_BYTES; loop { let ev = reader.read_event().map_err(|e| XmpError::Xml(e.to_string()))?; match ev { @@ -251,8 +273,9 @@ fn parse_tree(s: &str) -> Result { } } scopes.push(scope); - let name = canonical(&String::from_utf8_lossy(e.name().as_ref()), &scopes); - let attrs = raw_attrs.into_iter().map(|(k, v)| (canonical(&k, &scopes), v)).collect(); + let name = canonical(&String::from_utf8_lossy(e.name().as_ref()), &scopes, &mut opaque_budget)?; + let attrs = + raw_attrs.into_iter().map(|(k, v)| Ok((canonical(&k, &scopes, &mut opaque_budget)?, v))).collect::>()?; let node = Node { name, attrs, ..Default::default() }; if matches!(ev, Event::Empty(_)) { scopes.pop(); @@ -767,6 +790,43 @@ mod tests { assert_eq!(d.lc_settings, None); } + #[test] + fn foreign_namespace_cannot_impersonate_known_metadata() { + for body in [ + r#"xmp:Rating="2" lc:settings="foreign settings"/>"#, + r#">2foreign settings"#, + ] { + let packet = format!( + r#""# + ); + let d = parse_xmp(&packet).unwrap(); + assert_eq!(d.metadata.rating, None, "foreign rating: {packet}"); + assert_eq!(d.lc_settings, None, "foreign settings: {packet}"); + assert!(!d.properties.contains_key("xmp:Rating")); + assert!(!d.values.contains_key("lc:settings")); + assert!(d.properties.values().any(|v| v == &["foreign settings"])); + assert!(d.properties.keys().all(|key| !key.contains('/')), "opaque names must not create struct paths"); + } + } + + #[test] + fn foreign_namespace_names_are_distinct_and_scoped() { + let packet = r#" + + 1 + 2 + 4 + "#; + let d = parse_xmp(packet).unwrap(); + assert_eq!(d.metadata.rating, Some(4), "a local rebinding must not escape its scope"); + assert_eq!(d.properties.len(), 3, "different foreign URIs must not collide"); + assert_eq!(d.values.len(), 3); + assert_eq!(d.properties["xmp:Rating"], vec!["4"]); + assert!(d.properties.values().any(|v| v == &["1"])); + assert!(d.properties.values().any(|v| v == &["2"])); + } + #[test] fn attribute_form_foreign_prefixes_and_structs() { let x = r#" diff --git a/crates/meta/src/xmp_merge.rs b/crates/meta/src/xmp_merge.rs index 022f962ec..e1e4bca5e 100644 --- a/crates/meta/src/xmp_merge.rs +++ b/crates/meta/src/xmp_merge.rs @@ -23,7 +23,7 @@ use std::collections::{BTreeSet, HashSet}; use quick_xml::events::Event; -use crate::xmp::{XmpError, canonical_name, parse_xmp}; +use crate::xmp::{MAX_OPAQUE_NAME_BYTES, XmpError, canonical_name, parse_xmp, unescape_attr}; /// Which properties [`merge_xmp`] takes from the fresh packet. Names are canonical /// `prefix:name` (prefixes as [`crate::parse_xmp`] reports them); `prefix:*` is a whole namespace. @@ -101,6 +101,7 @@ fn scan(s: &str) -> Result { let mut stack: Vec = Vec::new(); let mut scopes: Vec> = vec![vec![("xml".into(), "http://www.w3.org/XML/1998/namespace".into())]]; let mut count = 0usize; + let mut opaque_budget = MAX_OPAQUE_NAME_BYTES; loop { let before = pos(&reader); let ev = reader.read_event().map_err(|e| XmpError::Xml(e.to_string()))?; @@ -124,18 +125,21 @@ fn scan(s: &str) -> Result { } let v = String::from_utf8_lossy(&a.value).into_owned(); if k == "xmlns" { - scope.push((String::new(), v.clone())); + scope.push((String::new(), unescape_attr(&v))); } else if let Some(p) = k.strip_prefix("xmlns:") { - scope.push((p.to_string(), v.clone())); + scope.push((p.to_string(), unescape_attr(&v))); } raw_attrs.push((k, v)); } scopes.push(scope); let raw_name = String::from_utf8_lossy(e.name().as_ref()).into_owned(); - let name = canonical_name(&raw_name, &scopes); + let name = canonical_name(&raw_name, &scopes, &mut opaque_budget)?; let kind = match stack.last() { Some(Kind::Rdf) if name == "rdf:Description" => { - let attrs = raw_attrs.into_iter().map(|(raw, value)| Attr { name: canonical_name(&raw, &scopes), raw, value }).collect(); + let attrs = raw_attrs + .into_iter() + .map(|(raw, value)| Ok(Attr { name: canonical_name(&raw, &scopes, &mut opaque_budget)?, raw, value })) + .collect::>()?; out.descs.push(Desc { tag_start: before, tag_end: after, raw_name, empty, attrs, props: Vec::new(), end: after }); Kind::Desc(out.descs.len() - 1) } @@ -399,6 +403,47 @@ mod tests { assert_eq!(d.properties.get("exif:DateTimeOriginal"), Some(&vec!["2024-05-06T07:08:09".to_string()])); } + #[test] + fn preserves_foreign_properties_using_known_prefixes() { + for property in [r#"lc:Secret="keep-me"/>"#, r#">keep-me"#] { + let desc = format!(r#"{desc}"#); + let out = merge_xmp(&old, &fresh(4, &[]), RULES).unwrap(); + assert!(out.contains(&desc), "lost foreign description: {out}"); + assert_eq!(parse_xmp(&out).unwrap().metadata.rating, Some(4)); + assert_eq!(merge_xmp(&out, &fresh(4, &[]), RULES).unwrap(), out); + } + } + + #[test] + fn namespace_aliases_remain_distinct_during_merge() { + // The alias URI includes a character reference: scanning and parsing must resolve it identically. + let foreign = r#""#; + let old = format!( + r#"{foreign} + + "# + ); + let out = merge_xmp(&old, &fresh(5, &[]), RULES).unwrap(); + assert!(out.contains(foreign), "lost foreign rating: {out}"); + assert!(!out.contains("a:Rating="), "genuine alias must be replaced: {out}"); + assert_eq!(parse_xmp(&out).unwrap().metadata.rating, Some(5)); + assert_eq!(merge_xmp(&out, &fresh(5, &[]), RULES).unwrap(), out); + } + + #[test] + fn opaque_namespace_expansion_is_bounded() { + // A roughly 40 KiB packet must not expand the same long URI into every property name indefinitely. + let packet = format!( + r#"{}"#, + "a".repeat(1024), + "".repeat(5000) + ); + assert_eq!(parse_xmp(&packet).unwrap_err(), XmpError::TooComplex); + assert!(matches!(scan(&packet), Err(XmpError::TooComplex))); + } + #[test] fn many_attributes_scan_in_linear_time() { // quick-xml's duplicate-name check compares each attribute with all earlier ones: 200 000 attributes on diff --git a/crates/pipeline/src/local.rs b/crates/pipeline/src/local.rs index ba0298226..705b4026e 100644 --- a/crates/pipeline/src/local.rs +++ b/crates/pipeline/src/local.rs @@ -227,22 +227,47 @@ fn median_row(src: &[f32], out: &mut [f32], r: usize) { if n == 0 { return; } - let at = |i: isize| src[i.clamp(0, n as isize - 1) as usize]; + let at = |i: isize| total_key(src[i.clamp(0, n as isize - 1) as usize]); let r_ = r as isize; - let mut win: Vec = (-r_..=r_).map(at).collect(); - win.sort_unstable_by(f32::total_cmp); + let mut win: Vec = (-r_..=r_).map(at).collect(); + win.sort_unstable(); for (x, o) in out.iter_mut().enumerate() { - *o = win[r]; + *o = f32::from_bits(total_key_inv(win[r])); let (old, new) = (at(x as isize - r_), at(x as isize + r_ + 1)); - if old.to_bits() != new.to_bits() { - let i = win.binary_search_by(|v| v.total_cmp(&old)).unwrap_or_else(|i| i.min(win.len() - 1)); - win.remove(i); - let j = win.binary_search_by(|v| v.total_cmp(&new)).unwrap_or_else(|j| j); - win.insert(j, new); + if old != new { + // Ranks by counting the smaller keys in one branch-free pass the compiler vectorizes. + // Binary searches here mispredicted on most steps and, with `remove` + `insert`, cost + // 2.4 s per 24 MP at r = 9 on one core. + let (mut i, mut j) = (0u32, 0u32); + for &v in &win { + i += u32::from(v < old); + j += u32::from(v < new); + } + let (i, j) = (i as usize, j as usize); + // `old` sits at `i`; shift the keys between the two ranks by one over it + if j > i { + win.copy_within(i + 1..j, i); + win[j - 1] = new; + } else { + win.copy_within(j..i, j + 1); + win[j] = new; + } } } } +/// `v`'s bits as an `i32` that orders like [`f32::total_cmp`] (the same transform it applies), so +/// equal keys are equal bits. +fn total_key(v: f32) -> i32 { + let b = v.to_bits() as i32; + b ^ (((b >> 31) as u32) >> 1) as i32 +} + +/// The bits [`total_key`] came from (the transform is its own inverse: it keeps the sign bit). +fn total_key_inv(k: i32) -> u32 { + (k ^ (((k >> 31) as u32) >> 1) as i32) as u32 +} + fn transpose(p: &Plane) -> Plane { let (w, h) = (p.width, p.height); let mut t = Plane::new(h, w); diff --git a/crates/pipeline/src/tests_geometry.rs b/crates/pipeline/src/tests_geometry.rs index d7105acfc..ac244cb0d 100644 --- a/crates/pipeline/src/tests_geometry.rs +++ b/crates/pipeline/src/tests_geometry.rs @@ -488,3 +488,39 @@ fn level_degrees_sign_matches_crop_angle() { let deg = crate::upright::level_degrees(&src, &SourceInfo::default(), &DevelopSettings::default()).expect("lines found"); assert!((deg + 6.0).abs() < 0.5, "{deg}"); } + +/// A "horizon" of `deg` degrees: sky above, below it bands of `band` px (0: one edge only). +fn tilted_scene(deg: f32, band: f32) -> Rgb32f { + let (w, h) = (900usize, 600usize); + let t = deg.to_radians().tan(); + Rgb32f::from_fn(w, h, |x, y| { + let yh = h as f32 / 2.0 + (x as f32 - w as f32 / 2.0) * t; + if (y as f32) < yh { + [0.6, 0.7, 0.9] + } else if band > 0.0 && (((y as f32 - yh) / band) as i32) % 2 == 1 { + [0.1, 0.2, 0.3] + } else { + [0.05, 0.1, 0.2] + } + }) +} + +/// Auto straighten acts on several agreeing lines only: one edge (the frame of a window, a branch) +/// turned straight photos in a measurement on real ones, so it is left alone. +#[test] +fn auto_straighten_needs_more_than_one_line() { + let level = |src: &Rgb32f| crate::upright::level_degrees(src, &SourceInfo::default(), &DevelopSettings::default()); + assert_eq!(level(&tilted_scene(4.0, 0.0)), None, "one edge"); + let deg = level(&tilted_scene(4.0, 30.0)).expect("a horizon and the bands below it"); + assert!((deg + 4.0).abs() < 0.5, "{deg}"); +} + +/// Auto straighten turns by at most `STRAIGHTEN_MAX_DEGREES`: larger "levels" came from texture +/// (planks, a bird's wing) on photos that were straight. +#[test] +fn auto_straighten_leaves_steep_lines_alone() { + let level = |src: &Rgb32f| crate::upright::level_degrees(src, &SourceInfo::default(), &DevelopSettings::default()); + assert_eq!(level(&tilted_scene(20.0, 30.0)), None, "20° is not a horizon to level"); + let deg = level(&tilted_scene(8.0, 30.0)).expect("8° is"); + assert!((deg + 8.0).abs() < 0.5, "{deg}"); +} diff --git a/crates/pipeline/src/tests_tone.rs b/crates/pipeline/src/tests_tone.rs index a6ec70eb5..87e1b659a 100644 --- a/crates/pipeline/src/tests_tone.rs +++ b/crates/pipeline/src/tests_tone.rs @@ -187,6 +187,27 @@ fn median_matches_a_plain_per_pixel_median() { } } +#[test] +fn median_orders_special_values_and_ties_like_total_cmp() { + // the running window keeps keys in `f32::total_cmp` order: NaNs of either sign, signed zeros, + // infinities and long runs of equal values come out bit for bit as a sorted per-pixel window + // does, also on planes narrower or shorter than the window + let special = [f32::NAN, -f32::NAN, 0.0, -0.0, f32::INFINITY, f32::NEG_INFINITY, f32::MIN_POSITIVE, -1.0, 1.0, 1.0, 1.0]; + let bits = |p: &Plane| p.data.iter().map(|v| v.to_bits()).collect::>(); + for (w, h) in [(1, 1), (2, 3), (5, 1), (19, 7), (61, 9)] { + let p = Plane::from_fn(w, h, |x, y| special[(x * 7 + y * 3 + (x * y) % 5) % special.len()]); + for r in [1, 2, 9, 24] { + let med = |v: &mut Vec| { + v.sort_by(f32::total_cmp); + v[r] + }; + let rows = Plane::from_fn(w, h, |x, y| med(&mut (0..=2 * r).map(|k| p.get((x + k).saturating_sub(r).min(w - 1), y)).collect())); + let want = Plane::from_fn(w, h, |x, y| med(&mut (0..=2 * r).map(|k| rows.get(x, (y + k).saturating_sub(r).min(h - 1))).collect())); + assert_eq!(bits(&local::median_hv(&p, r)), bits(&want), "{w}x{h} r {r}"); + } + } +} + #[test] fn v1_keeps_the_base_it_always_had() { // V1's base is the edge-aware filter of log luminance itself, as before process V2: no median diff --git a/crates/pipeline/src/upright.rs b/crates/pipeline/src/upright.rs index 93dffad0c..58a61adbe 100644 --- a/crates/pipeline/src/upright.rs +++ b/crates/pipeline/src/upright.rs @@ -375,6 +375,12 @@ fn focal_from_vps(v: [f64; 3], h: [f64; 3]) -> Option { /// Pure rotation (degrees, clockwise +) that levels the dominant lines. fn level_angle(vert: &[Segment], horiz: &[Segment]) -> Option { + level_angle_within(vert, horiz, 90.0, 1) +} + +/// [`level_angle`] that only answers for at most `max` degrees, backed by at least `min_lines` +/// agreeing segments. +fn level_angle_within(vert: &[Segment], horiz: &[Segment], max: f64, min_lines: usize) -> Option { let dev = |s: &Segment, vertical: bool| { let (dx, dy) = (s.b.x - s.a.x, s.b.y - s.a.y); // signed deviation in degrees of the segment from the axis @@ -406,8 +412,9 @@ fn level_angle(vert: &[Segment], horiz: &[Segment]) -> Option { })?; // consensus: most of the line length must agree with the median within 2°, otherwise the "lines" are scene // texture (tree flanks, ridges) rather than a horizon or plumb lines - let agree: f64 = v.iter().filter(|(d, _)| (d - median.0).abs() <= 2.0).map(|x| x.1).sum(); - (agree >= 0.5 * total).then_some(-median.0) + let agreeing: Vec<_> = v.iter().filter(|(d, _)| (d - median.0).abs() <= 2.0).collect(); + let agree: f64 = agreeing.iter().map(|x| x.1).sum(); + (agree >= 0.5 * total && agreeing.len() >= min_lines && median.0.abs() <= max).then_some(-median.0) } fn rot_z_h(deg: f64) -> Homography { @@ -569,11 +576,27 @@ pub fn analyze_source(src: &Rgb32f, info: &SourceInfo, s: &DevelopSettings) -> A a } +/// The most Auto straighten turns a photo (degrees). Measured on 68 photos straightened by hand +/// in Lightroom Classic and 300 left as shot: none was turned by more than 6°, while a larger +/// "level" came from texture (wooden planks, a branch, a bird's wing) and turned photos that were +/// straight. +pub const STRAIGHTEN_MAX_DEGREES: f64 = 10.0; + +/// The fewest agreeing lines Auto straighten acts on: one line (the frame's edge, a branch) said +/// nothing about the horizon in that measurement. +pub const STRAIGHTEN_MIN_LINES: usize = 3; + /// The rotation (degrees, clockwise +) that levels `src`'s dominant horizon/plumb lines, if they agree -/// (the consensus rule of Upright Level). Used by the crop panel's Auto straighten. +/// (the consensus rule of Upright Level), within [`STRAIGHTEN_MAX_DEGREES`] and backed by at least +/// [`STRAIGHTEN_MIN_LINES`] lines. Used by the crop panel's Auto straighten. pub fn level_degrees(src: &Rgb32f, info: &SourceInfo, s: &DevelopSettings) -> Option { let a = analyze_source(src, info, s); - level_angle(&a.vertical, &a.horizontal) + straighten_angle(&a) +} + +/// [`level_degrees`] of an analysis. +pub fn straighten_angle(a: &Analysis) -> Option { + level_angle_within(&a.vertical, &a.horizontal, STRAIGHTEN_MAX_DEGREES, STRAIGHTEN_MIN_LINES) } /// The automatic Upright correction for `mode` on `src` (identity for Off and Guided). diff --git a/crates/raw/tests/it/corpus.rs b/crates/raw/tests/it/corpus.rs index 0ec60849a..94d4647d6 100644 --- a/crates/raw/tests/it/corpus.rs +++ b/crates/raw/tests/it/corpus.rs @@ -45,10 +45,12 @@ fn corpus_raw_decodes() { let tp = t0.elapsed().as_secs_f64() * 1e3; // DNG and generic CFA TIFF previews can be absent or uncompressed RGB; other vendor raws embed a JPEG, // except the Panasonic `.RAW` files of 2005–2007, Canon's explicit HEVC preview tracks and Mamiya MEF. + // A Hasselblad 3FR / FFF without a JPEG hands its 8-bit RGB image on as a baseline TIFF (`rgb_preview`). if let Some(p) = &preview { let jpeg = p.starts_with(&[0xff, 0xd8]) && p.ends_with(&[0xff, 0xd9]); let jxl = p.starts_with(&[0xff, 0x0a]) || p.starts_with(b"\0\0\0\x0cJXL "); - assert!(jpeg || jxl, "{name}: preview is neither a JPEG nor a JPEG XL file"); + let tiff = fmt == RawFormat::CfaTiff && p.starts_with(b"II*\0"); + assert!(jpeg || jxl || tiff, "{name}: preview is neither a JPEG, a JPEG XL nor (Hasselblad) a TIFF file"); } else { let hevc_preview = fmt == RawFormat::Cr3 && lightcraft_meta::cr3::parse_cr3(&bytes) diff --git a/crates/ui-egui/locales/de.json b/crates/ui-egui/locales/de.json index 67a305ddf..cfed522cf 100644 --- a/crates/ui-egui/locales/de.json +++ b/crates/ui-egui/locales/de.json @@ -445,6 +445,7 @@ "Download the SAM 3 Model?": "SAM-3-Modell herunterladen?", "Downloads": "Downloads", "Drag along the horizon; double-click for Auto": "Entlang des Horizonts ziehen; Doppelklick für Automatik", + "No horizon or vertical lines to level by": "Kein Horizont und keine senkrechten Linien zum Ausrichten gefunden", "Drag over an eye on the photo; the pupil inside is found automatically.": "Ziehe über ein Auge im Foto; die Pupille darin wird automatisch gefunden.", "Draw Guides": "Hilfslinien zeichnen", "Duplicate": "Duplizieren", diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index 1b2517907..6f23c9bd7 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -81,6 +81,8 @@ mod tests_dialog_fit; #[cfg(test)] mod tests_dialog_primary; #[cfg(test)] +mod tests_dialog_tab; +#[cfg(test)] mod tests_dialogs; #[cfg(test)] mod tests_edit_layout; @@ -635,6 +637,10 @@ impl LightcraftApp { .map(|d| d.crop.aspect.map_or(serde_json::json!("free"), |(w, h)| serde_json::json!([w as f64 / 100.0, h as f64 / 100.0]))) }; } + if r.is_ok() && id == "library.toggleFilters" { + // the search field shows the text filter: empty while the filters are off, back with them + self.ui.search = self.session.filter.text.clone(); + } if r.is_ok() && id == "library.clearFilter" { // the search field shows the text filter: cleared from anywhere (the loupe's pill, the // menu), it must not keep a search that no longer applies (issue #1648) @@ -812,6 +818,18 @@ impl LightcraftApp { self.toast_for(ctx, text, TOAST_SECONDS); } + /// Auto straighten (the crop panel's Auto, a double-click with the Straighten tool), saying so + /// when it finds nothing to level by instead of leaving the photo as it is without a word. + pub fn auto_straighten(&mut self, ctx: &egui::Context) { + match self.run("crop.autoStraighten", serde_json::json!({})) { + Ok(r) if r.get("changed").and_then(Value::as_bool) == Some(false) => { + self.toast(ctx, crate::i18n::tr("No horizon or vertical lines to level by")); + } + Ok(_) => {} + Err(e) => self.toast(ctx, e), + } + } + /// A toast that stays `secs` seconds (messages that say where to look or what to do next). pub fn toast_for(&mut self, ctx: &egui::Context, text: impl Into, secs: f64) { let t = ctx.input(|i| i.time); diff --git a/crates/ui-egui/src/menubar.rs b/crates/ui-egui/src/menubar.rs index 1ac3e9926..f315b105a 100644 --- a/crates/ui-egui/src/menubar.rs +++ b/crates/ui-egui/src/menubar.rs @@ -172,6 +172,7 @@ const LAYOUT: &[(&str, &[&str])] = &[ "@Sort", "@Stacks", "view.filterBar", + "library.toggleFilters", "library.clearFilter", "library.refresh", "library.holdView", @@ -353,6 +354,7 @@ pub fn checked(app: &LightcraftApp, id: &str) -> Option { "view.beforeAfterSplit" => Some(u.before_after == crate::state::BeforeAfter::Split), "view.autoAdvance" => Some(u.auto_advance), "view.filterBar" => Some(u.filter_bar), + "library.toggleFilters" => Some(!app.session.filters_off()), "view.soloSections" => Some(u.single_panel), "view.navigator" => Some(u.navigator), "view.layoutImage" => Some(u.layout_image.show && !u.layout_image.path.trim().is_empty()), @@ -1395,6 +1397,23 @@ mod tests { let _ = std::fs::remove_dir_all(&dir); } + // Regression test for #49: the in-window menu headings (File, Edit, ...) sat only egui's default + // 8 px apart, too close to read or aim at. + #[test] + fn in_window_menu_headings_are_spaced_apart() { + let mut a = app(); + a.native_menu = false; + let mut h = crate::headless::Headless::new(a, [1400.0, 900.0], 1.0); + h.step(); + h.step(); + let rect = |id: &str| h.app.widgets.iter().find(|(w, _)| w == id).map(|(_, r)| *r).unwrap_or_else(|| panic!("{id} is on screen")); + let titles: Vec<_> = ["File", "Edit", "View", "Photo", "Window", "Help"].iter().map(|t| rect(&format!("menu:{t}"))).collect(); + for pair in titles.windows(2) { + let gap = pair[1].left() - pair[0].right(); + assert!(gap >= 24.0 - 0.5, "headings {:?} and {:?} are only {gap} px apart", pair[0], pair[1]); + } + } + /// A backup the host refuses (changes still unsaved, issue #1508) says why when started from /// the menu, as a refused export does; the refusal used to reach only the control channel. #[test] diff --git a/crates/ui-egui/src/menus.rs b/crates/ui-egui/src/menus.rs index 04d43e0d3..747cfbccb 100644 --- a/crates/ui-egui/src/menus.rs +++ b/crates/ui-egui/src/menus.rs @@ -340,7 +340,7 @@ fn import_into_album(app: &mut LightcraftApp, p: &Value, r: Result Option<[u8; 3]> { if let Some(s) = v.as_str() { let h = s.strip_prefix('#').unwrap_or(s); - if h.len() != 6 { + if h.len() != 6 || !h.bytes().all(|b| b.is_ascii_hexdigit()) { return None; } let c = |i: usize| u8::from_str_radix(h.get(i..i + 2)?, 16).ok(); diff --git a/crates/ui-egui/src/panels/chips.rs b/crates/ui-egui/src/panels/chips.rs index 30f03971b..3a8f0ce8e 100644 --- a/crates/ui-egui/src/panels/chips.rs +++ b/crates/ui-egui/src/panels/chips.rs @@ -55,7 +55,7 @@ pub(crate) fn display_label(chip: &FilterChip, filter: &lightcraft_catalog::Filt }; } if chip.clear.get("only").is_some() { - return format!("{} {}", tr("Only"), crate::i18n::tr_format!("{n} photos", n = filter.only.len())); + return format!("{} {}", tr("Only"), crate::i18n::tr_format!("{n} photos", n = if filter.only_none { 0 } else { filter.only.len() })); } if let Some((prefix, value)) = chip.label.split_once(": ") { let value = if matches!(prefix, "Flag" | "Type") { diff --git a/crates/ui-egui/src/panels/detail.rs b/crates/ui-egui/src/panels/detail.rs index b95d7b056..082508d08 100644 --- a/crates/ui-egui/src/panels/detail.rs +++ b/crates/ui-egui/src/panels/detail.rs @@ -2245,9 +2245,15 @@ fn mask_overlay(app: &mut LightcraftApp, ui: &mut egui::Ui, resp: &egui::Respons } for m in &d.masks { let sel = Some(m.id) == active; + // Ctrl+Shift+O (`mask_overlay_all`) shows every visible mask's region; outline each one + // too, not just the selected mask. Handles and the rotation grip stay on the selected + // mask so nothing else becomes draggable. The component check keeps this in step with the + // painted region (`view_overlay` at detail.rs:1354 only covers masks with components). + let show_outline = sel || (app.ui.mask_overlay_all && m.visible && !m.components.is_empty()); + let stroke_a: u8 = if sel { 230 } else { 170 }; for (ci, c) in m.components.iter().enumerate() { match &c.shape { - MaskShape::Radial { center, rx, ry, angle, .. } if sel => { + MaskShape::Radial { center, rx, ry, angle, .. } if show_outline => { let pts: Vec = (0..64) .map(|i| { let a = i as f64 / 64.0 * std::f64::consts::TAU; @@ -2257,39 +2263,44 @@ fn mask_overlay(app: &mut LightcraftApp, ui: &mut egui::Ui, resp: &egui::Respons map.screen(Point::new(center.x + (x * co - y * s) * l.0, center.y + (x * s + y * co) * l.1)) }) .collect(); - p.add(egui::Shape::closed_line(pts, Stroke::new(1.5, Color32::from_white_alpha(230)))); + p.add(egui::Shape::closed_line(pts, Stroke::new(1.5, Color32::from_white_alpha(stroke_a)))); + + if sel { + let l = frame_long_norm(map); + for (i, a_deg) in [0.0_f64, 90.0_f64, 180.0_f64, 270.0_f64].into_iter().enumerate() { + let a_local = a_deg.to_radians(); + let (s, co) = angle.to_radians().sin_cos(); + let (x, y) = (rx * a_local.cos(), ry * a_local.sin()); + let q = map.screen(Point::new(center.x + (x * co - y * s) * l.0, center.y + (x * s + y * co) * l.1)); + p.circle_stroke(q, 5.0, Stroke::new(1.5, Color32::WHITE)); + register(ui.ctx(), format!("maskHandle:{}:{ci}:{}", m.id, i + 1), Rect::from_center_size(q, vec2(14.0, 14.0))); + grips.push((m.id, ci, i as u8 + 1, q)); + } - let l = frame_long_norm(map); - for (i, a_deg) in [0.0_f64, 90.0_f64, 180.0_f64, 270.0_f64].into_iter().enumerate() { - let a_local = a_deg.to_radians(); let (s, co) = angle.to_radians().sin_cos(); - let (x, y) = (rx * a_local.cos(), ry * a_local.sin()); - let q = map.screen(Point::new(center.x + (x * co - y * s) * l.0, center.y + (x * s + y * co) * l.1)); - p.circle_stroke(q, 5.0, Stroke::new(1.5, Color32::WHITE)); - register(ui.ctx(), format!("maskHandle:{}:{ci}:{}", m.id, i + 1), Rect::from_center_size(q, vec2(14.0, 14.0))); - grips.push((m.id, ci, i as u8 + 1, q)); + let rot_r = rx + 0.08; + let rot_q = map.screen(Point::new(center.x + (rot_r * co) * l.0, center.y + (rot_r * s) * l.1)); + let h1_q = map.screen(Point::new(center.x + (rx * co) * l.0, center.y + (rx * s) * l.1)); + p.line_segment([h1_q, rot_q], Stroke::new(1.0, Color32::from_white_alpha(200))); + p.circle_stroke(rot_q, 4.0, Stroke::new(1.5, Color32::WHITE)); + register(ui.ctx(), format!("maskHandle:{}:{ci}:5", m.id), Rect::from_center_size(rot_q, vec2(14.0, 14.0))); + grips.push((m.id, ci, 5, rot_q)); } - - let (s, co) = angle.to_radians().sin_cos(); - let rot_r = rx + 0.08; - let rot_q = map.screen(Point::new(center.x + (rot_r * co) * l.0, center.y + (rot_r * s) * l.1)); - let h1_q = map.screen(Point::new(center.x + (rx * co) * l.0, center.y + (rx * s) * l.1)); - p.line_segment([h1_q, rot_q], Stroke::new(1.0, Color32::from_white_alpha(200))); - p.circle_stroke(rot_q, 4.0, Stroke::new(1.5, Color32::WHITE)); - register(ui.ctx(), format!("maskHandle:{}:{ci}:5", m.id), Rect::from_center_size(rot_q, vec2(14.0, 14.0))); - grips.push((m.id, ci, 5, rot_q)); } - MaskShape::Linear { start, end } if sel => { + MaskShape::Linear { start, end } if show_outline => { let (a, b) = (map.screen(*start), map.screen(*end)); let dir = (b - a).normalized(); let perp = vec2(-dir.y, dir.x) * 3000.0; - for (q, alpha) in [(a, 230), (b, 110)] { + let (a1, a2): (u8, u8) = if sel { (230, 110) } else { (170, 80) }; + for (q, alpha) in [(a, a1), (b, a2)] { p.line_segment([q - perp, q + perp], Stroke::new(1.0, Color32::from_white_alpha(alpha))); } - for (i, q) in [a, b].iter().enumerate() { - p.circle_stroke(*q, 5.0, Stroke::new(1.5, Color32::WHITE)); - register(ui.ctx(), format!("maskHandle:{}:{ci}:{i}", m.id), Rect::from_center_size(*q, vec2(14.0, 14.0))); - grips.push((m.id, ci, i as u8 + 1, *q)); + if sel { + for (i, q) in [a, b].iter().enumerate() { + p.circle_stroke(*q, 5.0, Stroke::new(1.5, Color32::WHITE)); + register(ui.ctx(), format!("maskHandle:{}:{ci}:{i}", m.id), Rect::from_center_size(*q, vec2(14.0, 14.0))); + grips.push((m.id, ci, i as u8 + 1, *q)); + } } } _ => {} @@ -3047,7 +3058,7 @@ mod preview_geometry_tests { fn straighten_overlay(app: &mut LightcraftApp, ui: &mut egui::Ui, resp: &egui::Response, held: bool) { ui.ctx().set_cursor_icon(egui::CursorIcon::Crosshair); if !held && resp.double_clicked() { - let _ = app.run("crop.autoStraighten", json!({})); + app.auto_straighten(ui.ctx()); app.ui.tool.clear(); app.gesture = None; return; diff --git a/crates/ui-egui/src/panels/dialogs.rs b/crates/ui-egui/src/panels/dialogs.rs index 894ad2bd5..c24f6da7c 100644 --- a/crates/ui-egui/src/panels/dialogs.rs +++ b/crates/ui-egui/src/panels/dialogs.rs @@ -2244,13 +2244,15 @@ fn person_rename_plan(cat: &lightcraft_catalog::Catalog, from: &str, to: &str) - PersonRenamePlan { faces, needs_merge: needs_merge && from_key != to_key, block } } -/// A dialog's one text field: it keeps the focus (except while its own menu has it) and opens -/// with its text selected, so typing replaces it. True when Return confirms the dialog; Esc -/// cancels the dialog (`show`). Taking the focus back with a bare `TextEdit` every frame hid the -/// focus Return gives up, and Return did nothing. +/// A dialog's one text field: it takes the focus when the dialog opens, and opens with its text +/// selected, so typing replaces it. It takes the focus back only while nothing else has it: taking +/// it back every frame would keep Tab from ever reaching the dialog's buttons and check boxes. +/// True when Return confirms the dialog; Esc cancels the dialog (`show`). fn dialog_field(ui: &mut egui::Ui, widget: &str, text: &mut String, hint: &str) -> bool { let r = crate::text_field::TextField::singleline(widget, text).hint(crate::i18n::tr(hint)).width(f32::INFINITY).select_on_focus(true).show(ui); - if !r.editing { + // (and takes it back when nothing has it: after a click on the dialog's background or a check box, + // or when Return gave it up on a refusal; a control Tab reached keeps it) + if just_opened(ui, widget) || (!r.editing && r.ending.is_none() && ui.memory(|m| m.focused().is_none())) { r.response.request_focus(); } r.ending == Some(crate::text_field::Ending::Return) diff --git a/crates/ui-egui/src/panels/right.rs b/crates/ui-egui/src/panels/right.rs index 76ecc746a..79cd28712 100644 --- a/crates/ui-egui/src/panels/right.rs +++ b/crates/ui-egui/src/panels/right.rs @@ -317,7 +317,7 @@ fn crop(app: &mut LightcraftApp, ui: &mut egui::Ui, id: PhotoId) { app.ui.tool = if on { String::new() } else { "straighten".into() }; } if text_button(ui, "straightenAuto", crate::i18n::tr("Auto"), false).clicked() { - let _ = app.run("crop.autoStraighten", json!({})); + app.auto_straighten(ui.ctx()); } }); }); diff --git a/crates/ui-egui/src/shortcuts.rs b/crates/ui-egui/src/shortcuts.rs index e7b58a9b5..91e6555b9 100644 --- a/crates/ui-egui/src/shortcuts.rs +++ b/crates/ui-egui/src/shortcuts.rs @@ -1317,6 +1317,10 @@ mod relative_rating_tests; #[path = "shortcuts/tests_flag_cycle.rs"] mod flag_cycle_tests; +#[cfg(test)] +#[path = "shortcuts/tests_enable_filters.rs"] +mod enable_filters_tests; + #[cfg(test)] #[path = "shortcuts/tests_filter_bar_key.rs"] mod filter_bar_key_tests; diff --git a/crates/ui-egui/src/shortcuts/tests_enable_filters.rs b/crates/ui-egui/src/shortcuts/tests_enable_filters.rs new file mode 100644 index 000000000..0f42c3eff --- /dev/null +++ b/crates/ui-egui/src/shortcuts/tests_enable_filters.rs @@ -0,0 +1,69 @@ +//! Cmd+L (Library > Enable Filters) switches the filter off and on from the keyboard and the menu. +use egui::{Key, Modifiers}; +use lightcraft_catalog::{Op, Photo, PhotoId, Source}; +use serde_json::json; + +use crate::LightcraftApp; +use crate::state::ViewMode; + +fn app() -> lightcraft_engine::Result { + let mut s = lightcraft_engine::Session::new(); + for id in 1..=3 { + let photo = Photo::new( + PhotoId(id), + Source::File { path: format!("/lightcraft-enable-filters-keys/{id}.jpg") }, + &format!("{id}.jpg"), + "JPEG", + 40, + 30, + "2026-10-23", + ); + s.catalog.apply(Op::AddPhoto { photo: Box::new(photo) })?; + } + s.execute("library.select", &json!({"ids": [1]}))?; + s.execute("photo.rate", &json!({"ids": [1], "rating": 4}))?; + s.execute("library.filter", &json!({"rating": 4, "text": "1.jpg"}))?; + let mut app = LightcraftApp::new(s, crate::Services { png: None, ..Default::default() }); + app.ui.search = "1.jpg".into(); + Ok(app) +} + +fn press(app: &mut LightcraftApp, ctx: &egui::Context) { + let events = vec![egui::Event::Key { key: Key::L, physical_key: Some(Key::L), pressed: true, repeat: false, modifiers: Modifiers::COMMAND }]; + let mut out = ctx.run_ui( + egui::RawInput { events, screen_rect: Some(egui::Rect::from_min_size(egui::Pos2::ZERO, egui::vec2(800.0, 600.0))), ..Default::default() }, + |ui| super::handle(app, ui.ctx()), + ); + out.textures_delta.clear(); +} + +#[test] +fn the_key_is_cmd_l() { + assert_eq!(super::shortcut_of(&Default::default(), "library.toggleFilters"), Some("Cmd+L")); +} + +#[test] +fn cmd_l_switches_the_filter_off_and_on_in_the_grids_and_the_loupe() { + for view in [ViewMode::PhotoGrid, ViewMode::SquareGrid, ViewMode::Detail] { + let mut app = app().unwrap(); + app.ui.view = view; + let ctx = egui::Context::default(); + press(&mut app, &ctx); + assert_eq!(app.session.visible().len(), 3, "{view:?}: off shows every photo"); + assert_eq!(app.ui.search, "", "{view:?}: the search field follows"); + assert_eq!(crate::menubar::checked(&app, "library.toggleFilters"), Some(false)); + press(&mut app, &ctx); + assert_eq!(app.session.visible().len(), 1, "{view:?}: on brings the filter back"); + assert_eq!(app.ui.search, "1.jpg"); + assert_eq!(crate::menubar::checked(&app, "library.toggleFilters"), Some(true)); + } +} + +#[test] +fn the_menu_item_does_the_same() { + let mut app = app().unwrap(); + crate::menubar::run_item(&mut app, "library.toggleFilters", serde_json::Value::Null).unwrap(); + assert_eq!(app.session.visible().len(), 3); + assert_eq!(app.ui.search, ""); + assert_eq!(crate::menubar::shortcut_text("Cmd+L", true), "⌘L"); +} diff --git a/crates/ui-egui/src/tests_activity.rs b/crates/ui-egui/src/tests_activity.rs index bce9d2eb1..e66c2b7ef 100644 --- a/crates/ui-egui/src/tests_activity.rs +++ b/crates/ui-egui/src/tests_activity.rs @@ -242,6 +242,14 @@ fn dead_export_worker_leaves_no_row() { /// A folder of `n` stand-in JPEGs and an app whose file probe takes 100 ms each (a slow drive). fn slow_import(tag: &str, n: usize) -> (Headless, std::path::PathBuf) { + gated_import(tag, n, None) +} + +/// Like `slow_import`, and while `gate` is false the probe holds every file after the first batch +/// (`batch_size()` files, which join the catalog together), so a test can act mid-import without +/// racing the 100 ms-per-file clock. The hold gives up after 60 s, so a failed test never leaves +/// the import thread spinning. +fn gated_import(tag: &str, n: usize, gate: Option>) -> (Headless, std::path::PathBuf) { let dir = std::env::temp_dir().join(format!("lc-activity-{tag}-{}", std::process::id())); let _ = std::fs::remove_dir_all(&dir); std::fs::create_dir_all(&dir).unwrap(); @@ -249,8 +257,17 @@ fn slow_import(tag: &str, n: usize) -> (Headless, std::path::PathBuf) { std::fs::write(dir.join(format!("IMG_{i:03}.jpg")), format!("not really a jpeg {i}")).unwrap(); } let mut s = lightcraft_engine::Session::new(); - s.media.file_probe = Some(std::sync::Arc::new(|p: &str| { + s.media.file_probe = Some(std::sync::Arc::new(move |p: &str| { std::thread::sleep(Duration::from_millis(100)); + let index = p.rsplit_once("IMG_").and_then(|(_, n)| n.strip_suffix(".jpg")).and_then(|n| n.parse::().ok()); + if let (Some(gate), Some(index)) = (&gate, index) + && index >= lightcraft_engine::import::batch_size() + { + let t0 = std::time::Instant::now(); + while !gate.load(std::sync::atomic::Ordering::Acquire) && t0.elapsed() < Duration::from_secs(60) { + std::thread::sleep(Duration::from_millis(1)); + } + } Ok(lightcraft_engine::media::ProbeInfo { width: 60, height: 40, @@ -286,14 +303,17 @@ fn import_cancelled_by_command_reports_cancelled() { /// selects its first photo only if nobody picked something else meanwhile (issue #1446). #[test] fn a_photo_chosen_during_an_import_stays_selected() { - let (mut h, dir) = slow_import("chosen", 20); + let gate = std::sync::Arc::new(std::sync::atomic::AtomicBool::new(false)); + let (mut h, dir) = gated_import("chosen", 20, Some(gate.clone())); crate::import::start_paths(&mut h.app, vec![dir.to_string_lossy().to_string()]).unwrap(); assert!(h.step_until(Duration::from_secs(60), |h| h.app.import.as_ref().is_some_and(|t| t.imported >= 2))); // the newest photo so far, never the import's first let chosen = h.app.session.catalog.photos().map(|p| p.id).max().expect("photos arrived"); let r = h.request("engine.execute", json!({"command": "library.select", "params": {"ids": [chosen.0]}}), T); assert_eq!(r["ok"], true, "{r}"); + // held at the gate until now, so the import cannot have ended however slowly the frames ran assert!(h.app.import.is_some(), "still importing"); + gate.store(true, std::sync::atomic::Ordering::Release); assert!(h.step_until(Duration::from_secs(60), |h| h.app.import.is_none())); assert_eq!(h.app.session.catalog.len(), 20, "all imported"); assert_eq!(h.app.session.selection.active, Some(chosen)); diff --git a/crates/ui-egui/src/tests_crop_rotate.rs b/crates/ui-egui/src/tests_crop_rotate.rs index 848b1b035..71368fba2 100644 --- a/crates/ui-egui/src/tests_crop_rotate.rs +++ b/crates/ui-egui/src/tests_crop_rotate.rs @@ -454,3 +454,31 @@ fn a_drag_from_just_inside_a_handle_resizes_not_moves() { assert!(r.x0 > 0.0, "the left edge moved in: {r:?}"); assert!((r.x1 - 1.0).abs() < 1e-6, "the right edge stays: {r:?}"); } + +/// Auto on a photo without a horizon or plumb lines says so, instead of leaving the photo as it is +/// without a word. +#[test] +fn auto_straighten_says_when_it_finds_nothing() { + let dir = std::env::temp_dir().join(format!("lc-ui-auto-straighten-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + let img = lightcraft_raster::Rgba8 { width: 64, height: 48, data: vec![[90, 120, 160, 255]; 64 * 48] }; + let bytes = lightcraft_codecs::encode_png(&lightcraft_codecs::EncodeImage::rgba8(&img), &lightcraft_codecs::EncodeMeta::default()).unwrap(); + let path = dir.join("flat.png"); + std::fs::write(&path, bytes).unwrap(); + let mut session = lightcraft_engine::Session::new().with_fs(); + session.execute("library.import", &json!({"paths": [path.to_string_lossy()]})).unwrap(); + let app = LightcraftApp::new(session, Services { png: None, ..Default::default() }); + let mut h = Headless::new(app, [1200.0, 800.0], 1.0); + for (method, params) in [("ui.set", json!({"view": "detail"})), ("engine.execute", json!({"command": "panel.crop"}))] { + let r = h.request(method, params, T); + assert_eq!(r["ok"], true, "{r}"); + } + h.settle(SETTLE); + let r = h.request("ui.clickWidget", json!({"id": "button:straightenAuto"}), T); + assert_eq!(r["ok"], true, "{r}"); + h.settle(SETTLE); + assert!(h.app.ui.toast.as_ref().is_some_and(|t| t.0 == "No horizon or vertical lines to level by"), "{:?}", h.app.ui.toast); + assert_eq!(angle(&h), 0.0); + let _ = std::fs::remove_dir_all(dir); +} diff --git a/crates/ui-egui/src/tests_dialog_tab.rs b/crates/ui-egui/src/tests_dialog_tab.rs new file mode 100644 index 000000000..5ffb99580 --- /dev/null +++ b/crates/ui-egui/src/tests_dialog_tab.rs @@ -0,0 +1,153 @@ +//! A dialog's name field takes the keyboard focus once, when the dialog opens: Tab then carries it +//! on to the dialog's other controls (a check box, Cancel, OK) and Shift+Tab back, instead of the +//! field taking it back every frame. + +use std::time::Duration; + +use serde_json::json; + +use crate::headless::Headless; +use crate::state::Dialog; +use crate::{LightcraftApp, Services}; + +const T: Duration = Duration::from_secs(20); + +fn demo_with_selection() -> Headless { + let mut session = lightcraft_engine::Session::with_demo(); + session.selection.ids = session.catalog.photos().take(2).map(|p| p.id).collect(); + let mut h = Headless::new(LightcraftApp::new(session, Services::default()), [1400.0, 900.0], 1.0); + h.settle(T); + h +} + +/// The registered widget (`field:…`, `check:…`, `button:…`) that has the keyboard focus. +fn focused(h: &Headless) -> Option { + let id = h.view.ctx.memory(|m| m.focused())?; + let rect = h.view.ctx.read_response(id)?.rect; + h.app.widgets.iter().find(|(_, r)| *r == rect).map(|(w, _)| w.clone()) +} + +fn key(h: &mut Headless, params: serde_json::Value) { + let r = h.request("ui.key", params, T); + assert_eq!(r["ok"], true, "{r}"); + h.step(); + h.step(); +} + +/// Open `dialog`, then Tab through it; the focus order, starting with the field it opens on. +fn tab_order(dialog: Dialog) -> Vec { + let mut h = demo_with_selection(); + h.app.ui.dialog = Some(dialog); + for _ in 0..4 { + h.step(); + } + let mut order = vec![focused(&h).unwrap_or_default()]; + for _ in 0..4 { + key(&mut h, json!({"key": "Tab"})); + order.push(focused(&h).unwrap_or_default()); + } + order +} + +#[test] +fn new_album_opens_on_its_name_and_tab_reaches_the_check_box_cancel_and_ok() { + let order = tab_order(Dialog::NewAlbum { name: "Trip".into(), folder: false, parent: None, include_selected: true }); + assert_eq!(order[..4], ["field:albumName", "check:albumIncludeSelected", "button:dialogCancel", "button:dialogOk"], "{order:?}"); +} + +#[test] +fn new_smart_album_opens_on_its_name_and_tab_reaches_cancel_and_ok() { + let order = tab_order(Dialog::NewSmartAlbum { name: "Rejected".into(), parent: None }); + let (cancel, ok) = (order.iter().position(|w| w == "button:dialogCancel"), order.iter().position(|w| w == "button:dialogOk")); + assert_eq!(order[0], "field:smartAlbumName", "{order:?}"); + assert!(cancel.is_some() && ok.is_some() && cancel < ok, "Cancel then OK are reached by Tab: {order:?}"); +} + +#[test] +fn save_metadata_preset_opens_on_its_name_and_tab_reaches_cancel_and_ok() { + let prompt = Dialog::TextPrompt { + title: "Save Metadata Preset".into(), + hint: "Preset name".into(), + value: String::new(), + command: "metadata.savePreset".into(), + params: json!({}), + key: "name".into(), + }; + let order = tab_order(prompt); + assert_eq!(order[..3], ["field:prompt", "button:dialogCancel", "button:dialogOk"], "{order:?}"); +} + +#[test] +fn shift_tab_goes_back_from_ok_to_the_name() { + let mut h = demo_with_selection(); + h.app.ui.dialog = Some(Dialog::NewAlbum { name: "Trip".into(), folder: false, parent: None, include_selected: true }); + for _ in 0..4 { + h.step(); + } + for _ in 0..3 { + key(&mut h, json!({"key": "Tab"})); + } + assert_eq!(focused(&h).as_deref(), Some("button:dialogOk")); + for _ in 0..3 { + key(&mut h, json!({"key": "Tab", "shift": true})); + } + assert_eq!(focused(&h).as_deref(), Some("field:albumName")); +} + +#[test] +fn the_name_still_takes_typing_and_enter_after_opening() { + let mut h = demo_with_selection(); + h.app.ui.dialog = Some(Dialog::NewAlbum { name: String::new(), folder: false, parent: None, include_selected: false }); + for _ in 0..4 { + h.step(); + } + let r = h.request("ui.text", json!({"text": "Holiday"}), T); + assert_eq!(r["ok"], true, "{r}"); + h.step(); + key(&mut h, json!({"key": "Enter"})); + h.settle(T); + assert!(h.app.ui.dialog.is_none(), "Enter confirmed the dialog"); + assert!(h.app.session.catalog.albums().any(|a| a.name == "Holiday"), "the typed name made the album"); +} + +#[test] +fn escape_still_cancels_after_tabbing_away_from_the_name() { + let mut h = demo_with_selection(); + h.app.ui.dialog = Some(Dialog::NewAlbum { name: "Trip".into(), folder: false, parent: None, include_selected: true }); + for _ in 0..4 { + h.step(); + } + key(&mut h, json!({"key": "Tab"})); + key(&mut h, json!({"key": "Escape"})); + assert!(h.app.ui.dialog.is_none(), "Esc closed the dialog"); +} + +#[test] +fn a_click_on_a_check_box_leaves_return_confirming_the_name() { + let mut h = demo_with_selection(); + h.app.ui.dialog = Some(Dialog::NewAlbum { name: "Trip".into(), folder: false, parent: None, include_selected: true }); + for _ in 0..4 { + h.step(); + } + let r = h.request("ui.clickWidget", json!({"id": "check:albumIncludeSelected"}), T); + assert_eq!(r["ok"], true, "{r}"); + h.step(); + h.step(); + key(&mut h, json!({"key": "Enter"})); + h.settle(T); + assert!(h.app.ui.dialog.is_none(), "Enter confirmed the dialog after the click"); + assert!(h.app.session.catalog.albums().any(|a| a.name == "Trip")); +} + +#[test] +fn a_refused_return_leaves_the_name_focused_for_the_correction() { + let mut h = demo_with_selection(); + h.app.ui.dialog = Some(Dialog::NewAlbum { name: String::new(), folder: false, parent: None, include_selected: false }); + for _ in 0..4 { + h.step(); + } + key(&mut h, json!({"key": "Enter"})); + h.step(); + assert!(h.app.ui.dialog.is_some(), "an empty name is refused and the dialog stays"); + assert_eq!(focused(&h).as_deref(), Some("field:albumName"), "the field has the focus again"); +} diff --git a/crates/ui-egui/src/tests_masking.rs b/crates/ui-egui/src/tests_masking.rs index a1e5846e4..2ce24355f 100644 --- a/crates/ui-egui/src/tests_masking.rs +++ b/crates/ui-egui/src/tests_masking.rs @@ -295,6 +295,21 @@ fn radial_body_drag_keeps_resize_and_rotation_handles() { } } +#[test] +fn mask_overlay_color_refuses_anything_but_six_hex_digits() { + let mut h = detail("panel.masking"); + exec(&mut h, "view.maskOverlayColor", json!({"color": "#2870f0"})); + for bad in ["#+f+f+f", "+12345", "#-0-0-0", "##ffffff"] { + let r = h.request("engine.execute", json!({"command": "view.maskOverlayColor", "params": {"color": bad}}), T); + assert_eq!(r["ok"], false, "{bad:?} is not a colour: {r}"); + assert_eq!(h.app.ui.mask_overlay_color, [0x28, 0x70, 0xf0], "{bad:?} leaves the overlay colour alone"); + } + exec(&mut h, "view.maskOverlayColor", json!({"color": "e63228"})); + assert_eq!(h.app.ui.mask_overlay_color, [0xe6, 0x32, 0x28]); + exec(&mut h, "view.maskOverlayColor", json!({"color": [10, 20, 30]})); + assert_eq!(h.app.ui.mask_overlay_color, [10, 20, 30]); +} + #[test] fn mask_overlay_keys_and_pins() { use lightcraft_pipeline::{MaskView, Overlay}; @@ -348,6 +363,63 @@ fn mask_overlay_keys_and_pins() { h.settle(SETTLE); } +/// #2210: Ctrl+Shift+O (the all-masks overlay) draws the white outline of every visible +/// Radial/Linear mask, not just the selected one — but only the selected mask registers +/// `maskHandle:` elements, so nothing else becomes draggable. +#[test] +fn all_masks_overlay_outlines_every_visible_mask_but_only_the_selected_one_is_draggable() { + let mut h = detail("panel.masking"); + // Two visible radial masks; mask 2 ends up selected (mask.add selects the new one). + exec(&mut h, "mask.add", json!({"kind": "radial", "center": [0.3, 0.5], "rx": 0.12, "ry": 0.12})); + exec(&mut h, "mask.add", json!({"kind": "radial", "center": [0.7, 0.5], "rx": 0.12, "ry": 0.12})); + exec(&mut h, "view.maskOverlayAll", json!({})); + assert!(h.app.ui.mask_overlay_all); + assert_eq!(h.app.session.active_mask, Some(2)); + // Pins add their own elements; keep them out of the handle count. + exec(&mut h, "view.maskPins", json!({"show": false})); + h.settle(SETTLE); + + let handles_of = |h: &Headless, id: usize| h.app.widgets.iter().filter(|(w, _)| w.starts_with(&format!("maskHandle:{id}:"))).count(); + // The selected mask keeps its 4 edge handles and its rotation grip. + assert_eq!(handles_of(&h, 2), 5, "the selected mask keeps its handles"); + // The other visible mask draws its outline but registers no handle: it is not draggable. + assert_eq!(handles_of(&h, 1), 0, "an unselected mask must not become draggable"); + // …while a mask with no components has nothing to outline and nothing to grab either. + exec(&mut h, "mask.add", json!({"kind": "luminanceRange"})); + exec(&mut h, "mask.addComponent", json!({"kind": "radial", "op": "add", "center": [0.5, 0.8], "rx": 0.08, "ry": 0.08})); + assert_eq!(h.app.session.active_mask, Some(3)); + assert_eq!(handles_of(&h, 1), 0); + h.settle(SETTLE); +} + +/// #2210: the outlines of *unselected* masks are painted, not registered as widgets, so the way to +/// observe them is the rendered pixels. With the all-masks overlay on, the unselected mask's +/// ellipse edge must carry white stroke pixels the photo alone does not have. +#[test] +fn all_masks_overlay_paints_the_outline_of_an_unselected_mask() { + let mut h = detail("panel.masking"); + exec(&mut h, "mask.add", json!({"kind": "radial", "center": [0.3, 0.5], "rx": 0.12, "ry": 0.12})); + exec(&mut h, "mask.add", json!({"kind": "radial", "center": [0.7, 0.5], "rx": 0.12, "ry": 0.12})); + assert_eq!(h.app.session.active_mask, Some(2)); + exec(&mut h, "view.maskPins", json!({"show": false})); + + // Sample the whole canvas: the outline is white (alpha 170) over the photo, so the overlay adds + // near-white pixels along the unselected mask's ellipse edge. + let white_pixels = |h: &mut Headless, on: bool| -> usize { + let overlay = h.app.ui.mask_overlay_all; + if overlay != on { + exec(h, "view.maskOverlayAll", json!({})); + } + assert_eq!(h.app.ui.mask_overlay_all, on); + h.settle(SETTLE); + let px = h.view.paint(&Default::default()); + px.pixels.iter().filter(|p| p.r() > 200 && p.g() > 200 && p.b() > 200).count() + }; + let off = white_pixels(&mut h, false); + let on = white_pixels(&mut h, true); + assert!(on > off, "the unselected mask's outline must add white pixels: off {off}, on {on}"); +} + /// #518: holding the brush still, however long, paints the same feathered spot as a quick press. #[test] fn holding_the_brush_still_does_not_harden_the_spot() { diff --git a/docs/gaps.md b/docs/gaps.md index be411a205..329dcf531 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-21 · **Change:** minor (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-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) 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 @@ -43,7 +43,8 @@ Hours are Opus 5.5 agent wall-clock hours (see [target-app-parity.md](target-app | 26 | **Performance at scale**: 85k-photo library opens in 1.7–4.7 s; 100k grid test pending; colour NR at half resolution; at 100k photos stack and album lookups are indexed (an "In a Stack" or "Album" smart album counts in ~14 ms, was 0.6–2.7 s) | ROADMAP log, gpu-pipeline.md | large-catalog users | 15–30 | [target-app-parity](target-app-parity.md#by-dimension) | | 27 | **Windows / Linux runtime coverage**: the Windows x64 installer is verified on Windows 11 (0.9.1: install and upgrade end on the completion page, a custom folder is kept, Launch works, removal is clean), but uninstalling from Settings → Apps ends with no confirmation (#892, #931); GPU path proven on Apple and on one AMD GPU on Linux (Radeon 8060S, Vulkan); NVIDIA, Intel and DX12 unverified; the Linux riscv64 tarball (new in 0.6.0, #844) is cross-compiled and never run beyond a QEMU smoke test | ROADMAP, issues, `release.yml` | non-Mac users find bugs first | 13–27 | [hardware-parity](hardware-parity.md) | | 29 | **HDR merge leftovers** (LR-MERGE-HDR): AI Denoise doesn't run on merged (linear) DNGs; JPEG brackets are treated as linear (no camera response curve); every frame is held as 32-bit float, ~0.86 GB per 67 MP frame (half-float frames would halve it) | tested on Sony A7R VI brackets 2026-10-14 ([merge.md](merge.md)); Lightroom's Denoise supports HDR DNGs | HDR shooters who denoise their merges, merge JPEGs, or merge 7+ frames of high-resolution raws on 16 GB machines | 10–20 (+ denoise linear-RGB contract, gap 11) | [merge](merge.md) | -| 28 | **Small Classic shortcuts and keys**: ⌘L (Enable Filters), Tab / ⇧Tab (hide the side panels / all panels; Tab moves the keyboard focus here) and F5. Rating `[` `]`, flag cycling (`` ` ``), `\` (Filter Bar), F1 help, F6 / F7 / F8 (filmstrip, left panel, right panel) are bound | [ui-parity](ui-parity.md#shortcuts) | Classic muscle memory | 2–4 | [ui-parity](ui-parity.md) | +| 28 | **Small Classic shortcuts and keys**: Tab / ⇧Tab (hide the side panels / all panels; Tab moves the keyboard focus here) and F5. Rating `[` `]`, flag cycling (`` ` ``), `\` (Filter Bar), ⌘L (Enable Filters), F1 help, F6 / F7 / F8 (filmstrip, left panel, right panel) are bound | [ui-parity](ui-parity.md#shortcuts) | Classic muscle memory | 2–4 | [ui-parity](ui-parity.md) | +| 30 | **Multi-select metadata editing in the UI**: `photo.setMeta` already iterates all selected photos, so the engine applies changes to every target, but the Info panel and All Metadata dialog always show one photo's values with no "mixed" indication when selected photos differ — users cannot tell whether a field is uniform or mixed across the selection before editing | standard in Lightroom Classic's multi-select Info panel | anyone editing metadata on a batch (copyright, keywords, custom fields) | 4–8 | [ui-parity](ui-parity.md) | Beta-blocking gaps (B) total **~230–430 h**; with the stability and platform work they need around them, **~340–600 h** to beta (incl. 40–80 h for the ~92 open core-path user issues) (see [target-app-parity.md](target-app-parity.md#stage)). @@ -51,7 +52,7 @@ Beta-blocking gaps (B) total **~230–430 h**; with the stability and platform w ## Gaps by kind - **Feature:** 5, 8, 11, 12, 13, 15, 16, 18, 19, 29 -- **UI / UX:** 4, 23, 24, 28 +- **UI / UX:** 4, 23, 24, 28, 30 - **File format:** 2, 6, 7, 10, 13, 20, 25 (raw ones in [raw-parity.md](raw-parity.md)) - **Hardware:** 14, 17, 22, 27 - **Localization:** 21 @@ -60,6 +61,10 @@ Beta-blocking gaps (B) total **~230–430 h**; with the stability and platform w ## Closed recently (kept for calibration) +- 2026-10-22: saving XMP preserves foreign namespaces even when they use `lc:` or `xmp:` prefixes; importing them no longer interprets their properties as native settings or ratings. Parser, merge and save-command regressions cover the correction; no ranked gap or readiness estimate changes. + +- 2026-10-22: PSD/PSB imports and metadata readers now expose embedded EXIF resource 1058 and XMP resource 1060 for capture dates, GPS and All Metadata (#2519); imported PSD edit copies retain their capture date, and the `removeLocation` Original-export guard sees embedded PSD/PSB GPS. No ranked gap or headline feature count closes. + - 2026-10-20: the import review album picker follows the sidebar folder hierarchy and searches album paths (#2362, LR-IMP-ADD-DIALOG), retaining None/New album and existing destination IDs. This closes a usability subgap; no ranked gap or headline feature count closes. - 2026-10-12: original-pixel detail at 100% (gap 4, LR-VIEW-ZOOM, #323): soft proofing gets zoom windows like @@ -79,6 +84,9 @@ Beta-blocking gaps (B) total **~230–430 h**; with the stability and platform w | Date | Change | Summary | |---|---|---| +| 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 | | 2026-10-21 | minor | Gap 28 shrinks: F8 shows / hides the right panel (`view.rightPanel`); rating `[` `]`, flag cycling and F1 help were already bound, so the entry lists only filter-bar keys, Tab / ⇧Tab and F5; 2–4 h (was 4–8) | | 2026-10-20 | minor | #2362: import destination albums use the shared hierarchy/search picker instead of a flat name list; existing import workflows retained; ranked gaps/readiness counts unchanged | | 2026-10-19 | minor | Gap 21: Indonesian (`id`) shipped with every catalog message but What's New; four new languages left (Hindi, Arabic, Korean, Vietnamese), 135–239 h | @@ -86,6 +94,7 @@ Beta-blocking gaps (B) total **~230–430 h**; with the stability and platform w | 2026-10-18 | patch | Gap 1: six X-Trans CMOS 4 Fujifilm bodies (X-T30 …) take the X-T3's spectral matrices instead of the neutral matrix | | 2026-10-15 | minor | Gap 15 shrinks: the Page panel's Page Options (Page Numbers, Page Info, Crop Marks) print on Single Image and Contact Sheet pages; identity plate, watermark, photo info, Cut Guides, direct printing and JPEG output remain | | 2026-10-12 | minor | Gap 10: SR2/SRF folder discovery fixed; F828 imports as preview-only. Sensor decoding remains a gap; readiness scores unchanged | +| 2026-10-23 | minor | Gap 28: ⌘L / Ctrl+L (Library ▸ Enable Filters) puts the filter aside and brings it back as it was; the entry drops ⌘L; hours unchanged | | 2026-10-15 | minor | Gap 28: `\` shows / hides the Filter Bar in the grids; the entry drops `[` `]` (#509), flag cycling (#1490) and F1 (Help), already on main; ⌘L is the filter-bar key left; hours unchanged | | 2026-10-14 | minor | Gap 26: at 100k photos stack and album lookups are indexed, so an "In a Stack" or "Album" smart album counts in ~14 ms (was 0.6–2.7 s); open time unchanged | | 2026-10-14 | minor | Gap 10: Camera-coverage sweep (#1327): 30 models verified on 40 CC0 files (coverage.py 515 → 545 verified); sensor verification does not establish colour parity (#1277) | diff --git a/docs/library-shortcuts.md b/docs/library-shortcuts.md index 211b00b76..81bc2cede 100644 --- a/docs/library-shortcuts.md +++ b/docs/library-shortcuts.md @@ -17,6 +17,7 @@ Select photos in Photo Grid or Square Grid, then use: | `Cmd`/`Ctrl` + `↑` / `↓` | Raise / lower flag status (reject < unflagged < pick; stops at either end) | | `Shift` + `0`–`9`, `P`, `X` or `U` | Apply the action and select the next photo | | `\` (backslash) | Show / hide the Filter Bar | +| ⌘L / Ctrl+L | Enable Filters: switch the filter off and on | Enter opens the active photo without changing the selection. Text fields, dialogs and open popups keep the key; a custom Enter binding takes precedence. Enter has no additional action in Detail. diff --git a/docs/parity-checklist.md b/docs/parity-checklist.md index 21140ae26..9790523e1 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-21 · **Change:** minor (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-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 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 @@ -33,7 +33,7 @@ cited here still exists. Formerly docs/parity.md. | E. Metadata (META) | 5 | 1 | 0 | 0 | 2/2 (100%) | 2/2 (100%) | | F. Edit panel — global adjustments (EDIT) | 44 | 2 | 3 | 1 | 28/28 (100%) | 13/14 (93%) | | G. Profiles (PROF) | 7 | 1 | 3 | 0 | 3/4 (75%) | 3/3 (100%) | -| H. Crop & rotate (CROP) | 11 | 0 | 0 | 1 | 8/8 (100%) | 3/3 (100%) | +| H. Crop & rotate (CROP) | 10 | 1 | 0 | 1 | 8/8 (100%) | 2/3 (67%) | | I. Remove / healing (REM) | 8 | 1 | 2 | 2 | 4/4 (100%) | 2/3 (67%) | | J. Red eye (EYE) | 2 | 0 | 0 | 0 | — | 1/1 (100%) | | K. Masking (MASK) | 15 | 5 | 5 | 0 | 7/8 (88%) | 7/7 (100%) | @@ -53,9 +53,9 @@ cited here still exists. Formerly docs/parity.md. | Y. Menus | 85 | 0 | 4 | 8 | 47/47 (100%) | 23/23 (100%) | | Z. Keyboard shortcuts (desktop) | 78 | 4 | 2 | 1 | 49/52 (94%) | 24/26 (92%) | | Lightroom Classic extras | 40 | 25 | 21 | 8 | — | 24/26 (92%) | -| **Total** | 422 | 63 | 57 | 35 | 196/203 (97%) | 148/161 (92%) | +| **Total** | 421 | 64 | 57 | 35 | 196/203 (97%) | 147/161 (91%) | -Weighted completion (✅ = 1, 🟡 = ½, 🚫 left out): **83.7%** of 542 in-scope rows — P0 98.3% of 203 · P1 95.0% of 161 · P2 56.5% of 177. +Weighted completion (✅ = 1, 🟡 = ½, 🚫 left out): **83.6%** of 542 in-scope rows — P0 98.3% of 203 · P1 94.7% of 161 · P2 56.5% of 177. ## Top gaps and shortcut notes @@ -168,8 +168,8 @@ 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` | manual rereading also reads sidecar-free JPEG embedded XMP (first complete standard APP1 before SOS/EOI, within 16 MiB of headers and 4096 marker/fill steps; no extended assembly or between-scan packets, not strict whole-JPEG validation; reader cases in `crates/engine/src/sidecar.rs`); a discovered unreadable sidecar fails without embedded fallback; procedural refresh/undo, non-UTF-8 sidecar and two explicit malformed fixtures in `crates/engine/src/tests_xmp.rs` (file-write checks use auto-write off); 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; an import ranks the files it is adding the same way, so a TIFF or JPEG arriving beside its raw reads its own `.xmp` and never the raw's `IMG_0001.xmp`, issue #1905); 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); radial masks (`Mask/CircularGradient`) keep Lightroom's inside / outside: `crs:Flipped="true"` is the effect-inside flag of current masks and of the legacy Radial Filter, `crs:MaskInverted` the inversion, checked against Lightroom Classic's renders (issue #1815); 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); settings Lightroom writes only for itself (`CompatibleVersion`, the profile digests, the lens profile's name / file / setup, `CropConstrainToUnitSquare`, the lens profile's strength at 100 %) are not reported as unimported (`crates/engine/src/crs.rs`) | -| 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` | `photo.allMetadata` reuses loaded original bytes for embedded XMP and keeps sidecar precedence (`crates/engine/src/tests_xmp.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 | +| 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` | manual rereading also reads sidecar-free JPEG embedded XMP (first complete standard APP1 before SOS/EOI, within 16 MiB of headers and 4096 marker/fill steps; no extended assembly or between-scan packets, not strict whole-JPEG validation; reader cases in `crates/engine/src/sidecar.rs`); a discovered unreadable sidecar fails without embedded fallback; procedural refresh/undo, non-UTF-8 sidecar and two explicit malformed fixtures in `crates/engine/src/tests_xmp.rs` (file-write checks use auto-write off); 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 (ownership follows namespace URI: foreign namespaces using `lc:` or `xmp:` cannot supply native settings or ratings and survive saving), 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; an import ranks the files it is adding the same way, so a TIFF or JPEG arriving beside its raw reads its own `.xmp` and never the raw's `IMG_0001.xmp`, issue #1905); 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); radial masks (`Mask/CircularGradient`) keep Lightroom's inside / outside: `crs:Flipped="true"` is the effect-inside flag of current masks and of the legacy Radial Filter, `crs:MaskInverted` the inversion, checked against Lightroom Classic's renders (issue #1815); 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); settings Lightroom writes only for itself (`CompatibleVersion`, the profile digests, the lens profile's name / file / setup, `CropConstrainToUnitSquare`, the lens profile's strength at 100 %) are not reported as unimported (`crates/engine/src/crs.rs`) | +| 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` | `photo.allMetadata` reuses loaded original bytes for embedded XMP and keeps sidecar precedence (`crates/engine/src/tests_xmp.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; PSD/PSB EXIF resource 1058 and XMP resource 1060 feed the same metadata rows and preserve sidecar precedence; metadata presets | ## F. Edit panel — global adjustments (EDIT) @@ -250,7 +250,7 @@ differences and missing bindings are in [ui-parity.md](ui-parity.md#shortcuts). | LR-CROP-RECT | Crop rectangle | P0 | ✅ | `cmd:crop.set`, `cmd:crop.drag`, `crates/geom/src/crop_drag.rs`, `crates/ui-egui/src/panels/detail.rs` | double-click inside the box applies the crop (like Return); on a straightened photo a moved box slides along the tilted edges to where the pointer asks, so a box that fills the photo's height still reaches its sides (issue #2299) | | LR-CROP-ASPECT | Aspect ratios | P0 | ✅ | `cmd:crop.aspect`, `cmd:crop.rotateAspect`, `crates/ui-egui/src/panels/right.rs` | menu with presets + custom W × H (a preset takes the crop's orientation, portrait after X; a custom ratio is used as entered, issue #2253), visible Lock toggle (also A), button names the active ratio; every handle honours the lock, holding Shift while dragging a handle keeps the frame's proportions for that drag (issue #614), and the drag stops at the image edge instead of shifting the frame (issue #295); "Original" is the photo's own shape (no separate As Shot); the last chosen aspect is saved as a crop tool default, resolving Original per photo and preserving existing crops; opening Crop or browsing never edits a photo: the default applies only when a crop gesture starts, within its single undo step; a crop starts locked to the photo's own shape until the Lock is switched off once, then as last set (#991) | | LR-CROP-STRAIGHTEN | Straighten tool | P0 | ✅ | `cmd:crop.straighten`, `cmd:crop.autoStraighten`, `crates/ui-egui/src/panels/detail.rs` (`rotate_cursor`, `angle_readout`), `crates/ui-egui/src/panels/right.rs` (Angle field), `crates/ui-egui/src/tests_crop_rotate.rs` | Straighten Tool button: drag along a horizon/vertical; double-click or Auto levels automatically. Dragging outside the crop box rotates it: there the pointer is a curved double arrow that bulges away from the side of the box it is on, diagonally off a corner and following a rotated box (`rotate_cursor_turn`, tested by `rotate_cursor_faces_away_from_the_crop_box`) (with ⌘ held, the straighten crosshair), and while rotating the angle shows next to it, kept on the canvas (issue #534); an exact angle goes in the Angle field under Straighten (`cropAngleField`: applies on Return or leaving it, Esc keeps the old angle, takes a decimal comma; a drag on it is one undo step) or by clicking the Straighten value (its hover says so). Every way of setting the angle (rotate handle, Straighten slider, Angle field, `crop.straighten`, `crop.set {angle}`, and the generic `develop.set` / `develop.adjust` / `develop.quickAdjust` / `develop.resetControl` on `crop.angle`, as MCP, the control channel and `lightcraft-cli render --set` use them) keeps the crop if it still fits, else shrinks it about its own centre, keeping its aspect, until it fits; the full photo (or a crop an earlier turn shrank to the largest of its aspect) stays the largest, so it grows back towards 0° (issue #1388), and no other crop ever grows (issue #1794: a crop dragged in from a corner turned into nearly the whole photo) (`CropGeometry::with_angle`, `crates/engine/src/tests_crop_angle.rs`; before issue #742 the generic path left the box, and the export, past the photo with smeared edge pixels in the corners). The dimming outside the box is the photo minus the (rotated) box, drawn as one mesh (`outside_crop_mesh`), so rotating never dims wedges inside the box or doubles them outside (issue #731) | -| LR-CROP-AUTO | Auto straighten | P1 | ✅ | `cmd:crop.autoStraighten` | crop-angle leveling from detected horizon/plumb lines (consensus required) | +| LR-CROP-AUTO | Auto straighten | P1 | 🟡 | `cmd:crop.autoStraighten` | crop-angle leveling from detected horizon/plumb lines (consensus required); turns by at most 10° (`STRAIGHTEN_MAX_DEGREES`) and only on at least 3 agreeing lines (`STRAIGHTEN_MIN_LINES`), and says so when it finds nothing (`auto_straighten_says_when_it_finds_nothing`). Measured on 68 photos straightened by hand in Lightroom Classic and 300 left as shot: it levels 14 of the 68 within 0.5° (1 more than 1° off) and turns 19 of the 300 by more than 1° (before the limits: 79, some by 20° or more). 🟡 because it still leaves most tilted photos alone: a soft sea horizon among wave crests, and verticals that converge in perspective, aren't used yet | | LR-CROP-ANGLE | Angle slider | P0 | ✅ | `ctl:crop.angle`, `crates/ui-egui/src/panels/right.rs` | the Straighten slider; an exact angle can also be typed in the Angle field under it (see LR-CROP-STRAIGHTEN); setting `crop.angle` through `develop.set` fits the crop inside the photo as Straighten does (issue #742) | | LR-GUIDED-SELECT | Guide selection in Guided Upright mode | P0 | ✅ | `cmd:tool.guidedUpright`, `cmd:geometry.guides`, `crates/ui-egui/src/panels/detail.rs`, `crates/ui-egui/src/state.rs` (`selected_guide`), `crates/ui-egui/src/tests_guided_guides.rs` | a press near a guide's line selects it (`selected_guide = Some((photo id, index))`; another press on it deselects), a press on an empty spot starts a new guide at that point, and an endpoint drags to reshape the guide (`Gesture::GuideEndpoint`, one undo step; clicking an endpoint only selects); tested in `tests_guided_guides` 8/8 | | LR-GUIDED-DELETE | Delete removes the selected guide alone | P0 | ✅ | `cmd:photo.delete`, `crates/ui-egui/src/shortcuts.rs`, `crates/ui-egui/src/tests_guided_guides.rs` | with the Guided Upright tool open (`RightPanel::Crop && tool == "guidedUpright"`), Delete / Backspace removes only the selected guide and clears the selection; with nothing selected it removes nothing (the photo itself is never deleted while the tool is open); tested in `tests_guided_guides` | @@ -407,7 +407,7 @@ differences and missing bindings are in [ui-parity.md](ui-parity.md#shortcuts). | LR-EXP-ORIGINAL | Original + XMP | P1 | ✅ | `cmd:app.export` (`format: original`), `crates/engine/src/tests_export.rs` | file copied byte for byte, sidecar named after the output and subject to the conflict policy | | LR-EXP-RENDERDNG | Render to DNG (Classic 15.5) | P2 | 🟡 | `cmd:app.export` (`format: rendereddng`), `crates/raw/src/dngwrite.rs` (`write_rendered_dng`), `crates/raw/tests/it/rendered_dng.rs` | export format `rendereddng` (dialog: Format DNG, "DNG of: Rendered photo"): the rendered photo (edit, crop, size, sharpening, watermark baked in) as a LinearRaw DNG of 16-bit float Deflate tiles in the export colour space, with `ColorMatrix1` / `ForwardMatrix1` for that space, a neutral `AsShotNeutral` and an identity `ProfileToneCurve` so raw converters show it as rendered; HDR edits keep highlights above white. Not a menu command on selected photos (Lightroom adds the DNG to the catalog next to the original); export only | | LR-EXP-PHOTOS | Export to the system photo library | P2 | 🟡 | `cmd:app.export` (`addToPhotos`, `photosAlbum`), `cmd:export.addToPhotos`, `cmd:export.photosImports`, `crates/engine/src/apple_photos.rs`, `crates/ui-egui/src/panels/dialogs.rs`, `packaging/macos/entitlements.plist`, `docs/apple-photos.md` | macOS: the exported files go to Apple Photos through Apple Events to its import command (a constant AppleScript run by `/usr/bin/osascript`; album names and paths are separate arguments), into a top-level album of exactly that name, made when missing (several of that name: refused); the Export dialog's Add to Apple Photos, Export with Previous, presets, MCP and `lightcraft-cli` all reach it; imports are jobs, off the UI thread in the desktop app and awaited in one-shot headless runs, one at a time, reserved by an export before it writes; permission, missing Photos, timeouts and partial imports are reported; the packaged app carries the Apple Events entitlement and usage text. Tested end to end with the packaged app on macOS 26.6.2 / Photos 11.0 (manual steps in `docs/apple-photos.md`). Other platforms' photo libraries are not covered | -| LR-EXP-PSD | Round trip to an external editor | P2 | ✅ | `cmd:photo.editExternal`, `cmd:photo.editInPhotoCraft`, `cmd:photo.editInExternal`, `cmd:photo.openOriginalInExternal`, `crates/engine/src/cmd/convert.rs`, `apps/lightcraft/src/editors.rs` | an `-Edit` copy of each selected photo (up to 50 at once, issue #920) with the edits (PSD or TIFF, 16- or 8-bit, Adobe RGB / ProPhoto / P3 / sRGB, chosen per editor in Settings ▸ General ▸ External Editing) next to the original, added stacked on top of it and opened in PhotoCraft (found where its installers put it; a PSD by default, which PhotoCraft saves back to in place) or the other editor set there (or the system default); also on the photo's right-click menu (Edit In). Re-read when LightCraft is focused again. Open Original in Other Editor (RAW, no copy) hands that editor each selected photo's original file itself instead (a raw stays a raw, e.g. for DxO PhotoLab: no render, no copy, nothing added; several photos go to one start of the editor, which DxO PhotoLab 8 needs to get them all; demo photos, missing files and originals that are not a file type LightCraft reads are skipped with the reason, so a path an imported catalog named cannot hand a program to the system opener, issue #2135). No "Edit a copy / Edit original" choice for non-raw files yet | +| LR-EXP-PSD | Round trip to an external editor | P2 | ✅ | `cmd:photo.editExternal`, `cmd:photo.editInPhotoCraft`, `cmd:photo.editInExternal`, `cmd:photo.openOriginalInExternal`, `crates/engine/src/cmd/convert.rs`, `apps/lightcraft/src/editors.rs` | an `-Edit` copy of each selected photo (up to 50 at once, issue #920) with the edits (PSD or TIFF, 16- or 8-bit, Adobe RGB / ProPhoto / P3 / sRGB, chosen per editor in Settings ▸ General ▸ External Editing) next to the original, added stacked on top of it and opened in PhotoCraft (found where its installers put it; a PSD by default, which PhotoCraft saves back to in place) or the other editor set there (or the system default); also on the photo's right-click menu (Edit In). Re-read when LightCraft is focused again. Open Original in Other Editor (RAW, no copy) hands that editor each selected photo's original file itself instead (a raw stays a raw, e.g. for DxO PhotoLab: no render, no copy, nothing added; several photos go to one start of the editor, which DxO PhotoLab 8 needs to get them all; demo photos, missing files and originals that are not a file type LightCraft reads are skipped with the reason, so a path an imported catalog named cannot hand a program to the system opener, issue #2135). Imported PSD/PSB photos retain embedded capture dates; PSD edit copies retain EXIF/XMP fields in the catalog and All Metadata. Original with Remove Location Info is refused when embedded PSD/PSB GPS is detected. No "Edit a copy / Edit original" choice for non-raw files yet | ## T. Share (SHARE) @@ -479,7 +479,7 @@ differences and missing bindings are in [ui-parity.md](ui-parity.md#shortcuts). | LR-BEHAV-TOOLTIPS | Tooltips with shortcuts | P0 | ✅ | `crates/ui-egui/src/panels/bottombar.rs` | | | LR-BEHAV-TEXTFIELD | Text fields behave alike | P1 | 🟡 | `crates/ui-egui/src/text_field.rs`, `crates/ui-egui/src/panels/topbar.rs`, `crates/ui-egui/src/widgets.rs`, `crates/ui-egui/src/panels/right.rs` (`meta_field`), `crates/ui-egui/src/panels/dialogs.rs`, `crates/ui-egui/src/panels/unnamed.rs` | one shared field: right-click Cut / Copy / Paste / Select All (no Paste in the browser), Esc gives back the text from before the edit, Return / leaving / Esc reported to the caller, optional select-on-focus; agents use `ui.clipboard` (MCP `clipboard`) and `ui.inspect` → `copied`. Used by Search Photos, slider values, the Info panel's metadata fields (multi-line captions), the one-field dialogs (New / Rename Album, New Smart Album, Rename / Merge Keywords, text prompts) and the unnamed faces' name field so far; the other text fields (most dialogs, import, settings, masking, keywords …) still use egui's own and have no menu | | LR-BEHAV-HEADLESS | Headless UI snapshots | P2 | ✅ | `apps/lightcraft-cli/src/main.rs`, `crates/ui-egui/src/headless.rs`, `docs/control-protocol.md` | `snapshot` rasterizes the UI and defaults photo development to the CPU before the first frame, without adapter discovery; scaled dimensions are validated before allocation, and UI zoom preserves the requested physical viewport; JSON-lines control scripts are supported | -| LR-BEHAV-ACCESS | Accessibility | P2 | 🟡 | `crates/ui-egui/src/widgets.rs`, `crates/ui-egui/src/text_field.rs`, `crates/ui-egui/src/panels/grid.rs`, `crates/ui-egui/tests/it/accessibility.rs` | AccessKit (VoiceOver / Narrator / AT-SPI): sliders announce control, value, range and step and take Increment / Decrement, buttons / icon buttons / dropdowns / section headers / sources their labels and state, text fields their caption, grid thumbnails file name, rating, flag and label. `tests/it/accessibility.rs` fails on an unnamed control, a slider without range or an untitled dialog in every view, Develop tool and 13 dialogs, and checks that Crop and Remove sliders are reached with Tab and that Tab walks the right panels top to bottom. Canvas tools by keyboard (`crates/ui-egui/src/canvas_keys.rs`, `crates/ui-egui/src/tests_canvas_keys.rs`): crop corners, edges and box, spot targets and sources, a New spot point (Return adds), radial / linear gradient handles and mask pins take the focus, are announced with their position and move with the arrows (Shift: further). Brush strokes and red-eye ellipses are pointer-only; not tried with a real screen reader ([audit](ui-parity.md#accessibility-audit)) | +| LR-BEHAV-ACCESS | Accessibility | P2 | 🟡 | `crates/ui-egui/src/widgets.rs`, `crates/ui-egui/src/text_field.rs`, `crates/ui-egui/src/panels/grid.rs`, `crates/ui-egui/tests/it/accessibility.rs` | AccessKit (VoiceOver / Narrator / AT-SPI): sliders announce control, value, range and step and take Increment / Decrement, buttons / icon buttons / dropdowns / section headers / sources their labels and state, text fields their caption, grid thumbnails file name, rating, flag and label. `tests/it/accessibility.rs` fails on an unnamed control, a slider without range or an untitled dialog in every view, Develop tool and 13 dialogs, and checks that Crop and Remove sliders are reached with Tab and that Tab walks the right panels top to bottom. Canvas tools by keyboard (`crates/ui-egui/src/canvas_keys.rs`, `crates/ui-egui/src/tests_canvas_keys.rs`): crop corners, edges and box, spot targets and sources, a New spot point (Return adds), radial / linear gradient handles and mask pins take the focus, are announced with their position and move with the arrows (Shift: further). Dialogs with one name field (New Album, New Smart Album, Save Metadata Preset, the renames) take the focus once when they open, so Tab and Shift+Tab reach their check box, Cancel and OK, and Return and Esc still confirm and cancel (`crates/ui-egui/src/tests_dialog_tab.rs`). Brush strokes and red-eye ellipses are pointer-only; not tried with a real screen reader ([audit](ui-parity.md#accessibility-audit)) | | LR-BEHAV-LOCALE | Language options | P2 | ✅ | `cmd:app.language.english`, `cmd:app.language.simplifiedChinese`, `cmd:app.language.traditionalChinese`, `cmd:app.language.japanese`, `cmd:app.language.portuguese`, `cmd:app.language.french`, `cmd:app.language.spanish`, `cmd:app.language.german`, `cmd:app.language.russian`, `cmd:app.language.ukrainian`, `cmd:app.language.georgian` | Edit > Language lists every language in the table and the active one is checked; shared with Settings > General (docs/localization.md) | | LR-BEHAV-LOCALIZE | Localisation | P2 | 🟡 | `crates/ui-egui/src/i18n.rs`, `crates/ui-egui/locales/`, `docs/localization.md`, `docs/localization-zh-hans.md`, `docs/localization-zh-hant.md`, `docs/localization-ja.md`, `docs/localization-pt-br.md`, `docs/localization-es.md`, `docs/localization-de.md`, `docs/localization-ru.md`, `docs/localization-ka.md` | One table entry per language (code, endonym, ISO 15924 script, catalog): English, Simplified Chinese, Traditional Chinese (Taiwan), Japanese, Brazilian Portuguese, Spanish, German, Russian, Ukrainian and Georgian UI with a persisted language preference and craft-fonts regular/bold faces per script (a variable face, as for Arabic, is drawn heavier for bold from its one file); menus, edit, crop/masks, settings, import/export and primary progress messages. Adding a language registers the locale and its language command, plus two catalogs. Stock presets/profiles, history steps, library headings, date headings and capture times are localised (user-named items stay verbatim); the Ukrainian catalog covers every existing catalog key, command/control label and rule-editor label; the German catalog reports newly added gaps; the other catalogs fall back to English for messages they lack. Technical errors remain English; release notes are translated in German and Ukrainian; craft-fonts faces for other scripts (Thai, Arabic, …) are UI fallbacks in every language, for file, folder and album names (#736); Traditional Chinese borrows the Simplified Chinese face (no TC face in craft-fonts yet), and the web build embeds the Japanese, Thai and Georgian faces in WASM while loading the Simplified Chinese face from a separate font file. | | LR-BEHAV-LEARN | Tutorials | OOS | 🚫 | | | @@ -779,7 +779,7 @@ From `08-lightroom-classic-extras.md` (Classic-only features) and part 2 of `06- | KEYC-SECONDWINDOW | Classic secondary-window keys | P2 | ⬜ | | | | KEYC-CATALOG | Classic photo/catalog keys (⇧⌘I, ⌘', ⌘R, F2, ⌫, ⇧⌘E…) | P2 | ✅ | `cmd:library.import`, `cmd:photo.delete`, `cmd:photo.virtualCopy`, `cmd:app.showInFinder`, `cmd:dialog.rename`, `cmd:photo.editInExternal` | ⇧⌘I add, ⌘' virtual copy, ⌘R show in Finder, F2 rename, ⌫ delete, ⇧⌘E external editor | | KEYC-COMPARE | Classic grid/compare keys (Z, Home/End, =/−, ⌘⇧D, S…) | P2 | 🟡 | `cmd:view.zoomToggle`, `cmd:view.thumbSizeUp`, `cmd:view.thumbSizeDown`, `crates/ui-egui/src/tests_zoom_keys.rs`, `cmd:library.select`, `crates/ui-egui/src/shortcuts.rs`, `crates/ui-egui/src/tests_keymap.rs` (`home_and_end_select_the_ends_of_the_grid`, `shift_arrows_extend_the_grid_selection`) | Z toggles zoom; `=` / `-` (and `+`) step the grid thumbnail size by 30 pt within the bottom-bar slider's 90–480 range in Photo Grid, Square Grid and People (nothing in Detail, Compare, Survey or Reference; a text field keeps the keys; ⌘= / ⌘− still zoom); Home / End make the first / last photo of the view the active, selected one and ⇧← / ⇧→ add the previous / next photo to the selection (the added photo becomes active; nothing is ever removed; at either end nothing happens), in Photo Grid and Square Grid only: they wait in the other views, in the full-screen preview, while a text field or a dialog has the keyboard, and when the user gave the key to a command; no compare, stacks keys | -| KEYC-RATING | Classic rating/flag keys (1–5, ⇧1–5, 6–9, P, X, U, ⇧X, ⇧U, `[` `]`, \`, ⌘↑ ⌘↓) | P2 | 🟡 | `cmd:photo.rate`, `cmd:photo.pick`, `cmd:photo.flag`, `cmd:photo.cycleFlag`, `cmd:photo.increaseFlag`, `cmd:photo.decreaseFlag` | 0–5 / Shift+0–5 and 6–9 / Shift+6–9; P/X/U, Shift+X/U and grid Shift+P work; ` (backtick) cycles each selected photo's flag unflagged → pick → reject → unflagged and ⌘↑ / ⌘↓ (Ctrl on Windows/Linux) raise / lower it (reject < unflagged < pick, clamped); one undo step, Compare/Survey act on the active photo, Auto Advance follows; tests cover batch/undo, active Compare candidate and text focus; `\` shows / hides the Filter Bar in the grids (Show Original in Detail). Missing: Shift+P outside grids (Presets), ⌘L Enable Filters | +| KEYC-RATING | Classic rating/flag keys (1–5, ⇧1–5, 6–9, P, X, U, ⇧X, ⇧U, `[` `]`, \`, ⌘↑ ⌘↓) | P2 | 🟡 | `cmd:photo.rate`, `cmd:photo.pick`, `cmd:photo.flag`, `cmd:photo.cycleFlag`, `cmd:photo.increaseFlag`, `cmd:photo.decreaseFlag`, `cmd:library.toggleFilters` | 0–5 / Shift+0–5 and 6–9 / Shift+6–9; P/X/U, Shift+X/U and grid Shift+P work; ` (backtick) cycles each selected photo's flag unflagged → pick → reject → unflagged and ⌘↑ / ⌘↓ (Ctrl on Windows/Linux) raise / lower it (reject < unflagged < pick, clamped); one undo step, Compare/Survey act on the active photo, Auto Advance follows; tests cover batch/undo, active Compare candidate and text focus; `\` shows / hides the Filter Bar in the grids (Show Original in Detail); ⌘L (Ctrl+L) Enable Filters puts the filter aside and brings it back as it was. Missing: Shift+P outside grids (Presets) | | KEYC-COLLECTIONS | Classic collection keys (⌘N, B…) | P2 | 🟡 | `cmd:dialog.newAlbum` | ⌘N new album; no quick collection | | KEYC-METADATA | Classic keyword/metadata keys (⌘K, ⌘S, ⌘⌥⇧C/V…) | P2 | 🟡 | `cmd:photo.saveMetadataToFile` | ⌘S saves metadata; no keyword sets, metadata copy/paste | | KEYC-DEVELOP | Classic develop keys (V, ⌘U, ⇧⌘U, R, Q, K, M, ⇧M, ⇧W, ⇧J, ⇧Q…) | P2 | 🟡 | `cmd:develop.treatment`, `cmd:develop.reset`, `cmd:crop.reset` | V, ⇧⌘R, ⌥⌘R, W, J, Y, ⇧Y, \ match; R/K/M/⇧M differ; no Classic keymap layer | @@ -798,6 +798,8 @@ Latin stays upright. Advanced Japanese composition remains open. Regression cove | Date | Change | Summary | |---|---|---| +| 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 | | 2026-10-20 | minor | LR-IMP-ADD-DIALOG: #2362 gives the import review the shared hierarchy/search album picker while retaining None/New album and existing destination IDs; status and feature counts unchanged | | 2026-10-21 | patch | KEY-SLIDER-NUDGE: vertical wheel nudges need a click or drag in the slider's row; leaving it (even between two frames) or Tab focus alone does not arm them (issue #1355); status unchanged | @@ -927,3 +929,6 @@ Latin stays upright. Advanced Japanese composition remains open. Regression cove | 2026-10-16 | minor | LR-VIEW-PHOTOGRID: when the photos shown change by those an edit touched (an import's batch, a capture time, a crop), the grid reads only those photos' dates and shapes again instead of every photo of the list (a 16-photo batch at 181,749 photos: grid frame 101 ms → 7 ms); status unchanged | | 2026-10-19 | minor | LRC-LIB-LIBFOLDERS: the sidebar trees indent by a narrower step (14 / 12 / 10 / 8 pt) when the deepest open row would not fit the panel, instead of cutting its name or scrolling sideways at once (issue #2298); status unchanged (✅) | | 2026-10-15 | patch | LR-EXP-DIM: Width & Height (`fit: widthHeight`) limits the width and the height as the photo stands, beside Dimensions (W × H, either orientation), as in Lightroom (#1620); status unchanged (✅) | +| 2026-10-22 | minor | MENU-BAR: on macOS the native menu gives a key back to the window while a text field has focus or a shortcut is being recorded (muda's `set_accelerator(None)` does nothing there, so the menu item is now swapped for one without a key, and the key is put back on that item afterwards); status unchanged (✅) | +| 2026-10-23 | patch | LR-BEHAV-ACCESS: the name field of New Album, New Smart Album, Rename and the other one-field dialogs takes the keyboard focus once when the dialog opens instead of every frame, so Tab and Shift+Tab reach the check box, Cancel and OK; status unchanged (🟡) | +| 2026-10-22 | minor | LR-CROP-AUTO: Auto straighten turns by at most 10° and only on at least 3 agreeing lines, and says when it finds nothing to level by; measured on 68 photos straightened by hand and 300 left as shot, unwanted turns of more than 1° fell from 79 to 19 of the 300. Downgraded ✅ → 🟡: it still leaves most tilted photos alone | diff --git a/docs/ui-parity.md b/docs/ui-parity.md index c92a65968..7a746fdf3 100644 --- a/docs/ui-parity.md +++ b/docs/ui-parity.md @@ -1,6 +1,6 @@ # UI parity -> **Last reviewed:** 2026-10-11 · **Last updated:** 2026-10-21 · **Change:** minor (context menus: a right-click elsewhere on the same widget moves its open menu instead of flashing it, issue #1877; Reference Escape, catalog-local pins and deleted-photo handling verified on Windows; KEYC-RATING: `\` shows / hides the Filter Bar in the grids; earlier: zoom row soft proofing sharp at 1:1, canvas tools by keyboard, focus-order test, ~50 unnamed hand-drawn widgets named) · **Target:** Adobe Lightroom Classic 15.5 / Lightroom 9.6 +> **Last reviewed:** 2026-10-11 · **Last updated:** 2026-10-23 · **Change:** minor (KEYC-RATING: ⌘L Enable Filters; context menus: a right-click elsewhere on the same widget moves its open menu instead of flashing it, issue #1877; Reference Escape, catalog-local pins and deleted-photo handling verified on Windows; KEYC-RATING: `\` shows / hides the Filter Bar in the grids; earlier: zoom row soft proofing sharp at 1:1, canvas tools by keyboard, focus-order test, ~50 unnamed hand-drawn widgets named) · **Target:** Adobe Lightroom Classic 15.5 / Lightroom 9.6 How LightCraft's interaction compares with Lightroom's: views, panels, tools and handles, modifier keys, shortcuts, zoom, context menus and feel. Rows for individual menu items and shortcuts live in @@ -66,7 +66,7 @@ under the UI command that wraps it (a no-op error); the UI command now wins. | Create Version | ⌘⇧S (+ ⇧M) | ⇧M (Windows: Ctrl+⇧S) | — | | Select None | ⌘⇧A (+ ⌘D) | ⌘D | — | -**Library culling (KEYC-RATING):** `0–5`, `6–9` and `P/X/U` reach egui on macOS even when displayed in the native menu (adapted from PR #261; issue #283). `Shift+0–9` applies and advances once; `Shift+P` picks and advances in Photo Grid / Square Grid and retains Presets elsewhere. See [library shortcuts](library-shortcuts.md) for the verified Classic mapping and regression coverage. F6, F7 and F8 show / hide the filmstrip, the left panel and the right panel (F8 brings back the panel it hid; `view.rightPanel`). `` ` `` cycles the flag (unflagged → pick → reject → unflagged) and ⌘↑ / ⌘↓ raise / lower it; the native menu leaves ⌘↑ / ⌘↓ to egui so Compare/Survey act on the active photo. `\` shows / hides the Filter Bar in Photo Grid / Square Grid and keeps Show Original in Detail. Remaining Classic gaps: ⌘L (Enable Filters). +**Library culling (KEYC-RATING):** `0–5`, `6–9` and `P/X/U` reach egui on macOS even when displayed in the native menu (adapted from PR #261; issue #283). `Shift+0–9` applies and advances once; `Shift+P` picks and advances in Photo Grid / Square Grid and retains Presets elsewhere. See [library shortcuts](library-shortcuts.md) for the verified Classic mapping and regression coverage. F6, F7 and F8 show / hide the filmstrip, the left panel and the right panel (F8 brings back the panel it hid; `view.rightPanel`). `` ` `` cycles the flag (unflagged → pick → reject → unflagged) and ⌘↑ / ⌘↓ raise / lower it; the native menu leaves ⌘↑ / ⌘↓ to egui so Compare/Survey act on the active photo. `\` shows / hides the Filter Bar in Photo Grid / Square Grid and keeps Show Original in Detail. ⌘L / Ctrl+L (Enable Filters) puts the filter aside and shows every photo, and the next press brings back the same filter, stars, flag and text together; setting or clearing a filter while it is off drops the one put aside (not documented by Adobe, our choice). **Grid thumbnail size (KEYC-COMPARE):** Classic's `=` and `-` increase and decrease the Grid thumbnail size (same keys on macOS and Windows; `+` also works for the larger size here, as Shift+`=` or the keypad). They step the bottom-bar slider by 30 pt inside its 90–480 pt range, in Photo Grid, Square Grid and People only; Detail, Compare, Survey and Reference ignore them, and ⌘= / ⌘− stay Zoom In / Out. See [library shortcuts](library-shortcuts.md). @@ -149,6 +149,7 @@ establish OS keyboard delivery or cross-platform acceptance. | Date | Change | Summary | |---|---|---| | 2026-10-15 | patch | Context menus: a right-click elsewhere on the same widget while its menu is open moves the menu to the pointer; before it flashed at the new spot and closed (issue #1877); every site uses `widgets::context_menu`; 70% unchanged | +| 2026-10-23 | patch | KEYC-RATING: ⌘L / Ctrl+L (Library ▸ Enable Filters) switches the filter off and on, restoring it as it was | | 2026-10-15 | patch | KEYC-RATING: `\` shows / hides the Filter Bar in Photo Grid and Square Grid (Show Original in Detail); ⌘L is the filter-bar key left | | 2026-10-14 | patch | Shortcuts: Visualize HDR loses its default ⇧H, which cycles the Tool Overlay (issue #224); both PRs had claimed it | | 2026-10-12 | minor | Accessibility: crop handles, spots, gradient handles and mask pins operable by keyboard with AccessKit names and values; Tab order test for the right panels; hand-drawn icons and rows named; 55% → 75%; UI remaining 34–58 h → 32–54 h | diff --git a/docs/whats-new.md b/docs/whats-new.md index cffff83e2..c09dddc09 100644 --- a/docs/whats-new.md +++ b/docs/whats-new.md @@ -1,5 +1,51 @@ # What's new in LightCraft +## 2026-10-23 + +### Tab leaves the name field in New Album, New Smart Album and the name prompts +- In New Album, New Smart Album, Save Metadata Preset and the other dialogs with one name field, the field has the + keyboard focus when the dialog opens, and Tab now moves on to the check box, Cancel and OK (Shift+Tab goes back). + Before, the field took the focus back every frame, so only Return and Esc got you out of it. Return still + confirms and Esc still cancels. + +### Foreign XMP metadata survives namespace-prefix collisions +- Saving a sidecar keeps another application's properties even when its namespace uses prefixes such as `lc` + or `xmp`. Those foreign properties no longer become LightCraft settings or ratings when imported. + +### Find Missing Photos finds the files of a library made on Windows +- A library made on Windows records its originals with Windows paths. Opened on macOS or Linux, its photos are + missing until relinked, and Find Missing Photos found them only by their content, so it missed every photo it had + no fingerprint for (such as photos brought in from a Lightroom catalog) or that had been edited since, even in a + folder holding every file under the same name. It now matches them by name as it does any other photo: the name + after the last `/` or `\`, in any letter case, checked against the size when the library has one. Several files + of that name are still reported rather than guessed, and undo puts back the paths as they were. + +### Auto straighten leaves straight photos alone +- Auto straighten (Auto in the crop panel, or a double-click with the Straighten tool) no longer turns a photo by + more than 10°, and acts only when at least three lines agree. Before, it often turned photos that were straight: + the edges of wooden planks, a branch or a bird's wing were taken for the horizon, sometimes for a 25° turn. +- When it finds no horizon or vertical lines, it says so instead of doing nothing without a word. + +### PSD and PSB metadata stays available +- LightCraft now imports PSD and PSB files with EXIF and XMP image resources. Capture dates, GPS and descriptive fields + stay available in All Metadata; PSD edit copies keep their capture date. Original exports with Remove Location Info enabled + are refused when a PSD or PSB embeds GPS, since LightCraft cannot strip metadata from a byte-for-byte original copy. + +## 2026-10-22 + +### Reliability +- Opening a library no longer writes through a link left at the lock's owner note (`catalog.lock.owner`): the link + is removed and a new note made in its place, so the file it pointed to is never changed (as for the catalog's + temporary files, issue #1502). + +### Keys reach text fields and the shortcut editor on macOS +- While a text field has focus, the native menu now lets go of the keys the field needs (`G`, the digits `0`–`9`, + `P`, `U` and the like), and while Keyboard Shortcuts is recording a key it lets go of all of them (⌘, and ⌘Q in + the LightCraft menu too), except the system's own items (Hide LightCraft ⌘H, Hide Others ⌥⌘H, Close Window ⌘W + and Minimize ⌘M) and the two keys of the Help menu (⌘/ and F1). Before, the menu item kept its key on macOS, so the + key could still run the menu command instead of reaching the field or the editor. The keys come back on the menu + items when the field loses focus or recording ends; the menu's order, checkmarks and enabled state are unchanged. + ## 2026-10-21 ### Sharper resampling in HDR and panorama merges diff --git a/docs/xmp-interop.md b/docs/xmp-interop.md index b90e76c40..7fd11a172 100644 --- a/docs/xmp-interop.md +++ b/docs/xmp-interop.md @@ -81,6 +81,9 @@ the only copy of those edits outside that application's catalog. Saving never dr ones), `xmp:CreatorTool`, comments, the packet wrapper and padding. LightCraft's properties go into one `rdf:Description` of their own; owned properties written by another application (in attribute or element form) are removed from its description, which otherwise stays as it was. +- Ownership follows the namespace URI, not the prefix spelling. A foreign namespace bound to `lc:` or `xmp:` + stays foreign: its properties are preserved and do not become LightCraft settings or ratings. Genuine namespace + aliases remain interchangeable, including namespace declarations containing XML character references. - A sidecar that can't be read as XMP (not well-formed, not UTF-8, no `rdf:RDF`) is first copied to `.xmp.bak-