From 8d67b834e4c457b5be2cd1946f809ac7dbd998fd Mon Sep 17 00:00:00 2001 From: dotCooCoo Date: Sat, 24 Oct 2026 09:57:22 -0700 Subject: [PATCH] LR-PREF-SHORTCUTS: a declared shortcut yields to a key the saved keymap gives another command When a saved keymap gave a key to one command and a later build declared the same key as another command's default, one press ran both commands, and menus and Help > Keyboard Shortcuts listed the key on both. binding() now drops a declared shortcut while the keymap gives its key to another known command, other than its contextual partner. handle(), conflicts(), shortcut_of(), the menus and the shortcut editor read it. In the editor such a row counts as changed, so its restore button takes the key back. handle() also let every saved entry stop the fixed keys on its key (Enter in the grid, secondary keys such as Shift+X, the rating and label digits, Home, End and Shift+arrows), including entries for commands the build does not know. It now reads only the entries of known commands, as binding() does, so a stale entry in ui.json no longer silences Shift+X, a rating digit or a label digit. --- crates/ui-egui/src/panels/keymap.rs | 3 +- crates/ui-egui/src/shortcuts.rs | 67 +++++++++++++++++++++++++++-- crates/ui-egui/src/tests_keymap.rs | 21 +++++++++ docs/parity-checklist.md | 2 +- 4 files changed, 87 insertions(+), 6 deletions(-) diff --git a/crates/ui-egui/src/panels/keymap.rs b/crates/ui-egui/src/panels/keymap.rs index 1bda17758..11290b1b1 100644 --- a/crates/ui-egui/src/panels/keymap.rs +++ b/crates/ui-egui/src/panels/keymap.rs @@ -80,7 +80,8 @@ fn menu_text(sc: &str, mac: bool) -> String { } fn row(app: &mut LightcraftApp, ui: &mut egui::Ui, t: &Tokens, b: &Bindable, sc: Option<&str>, mac: bool) { - let changed = app.ui.settings.keymap.contains_key(b.id); + // a declared key that yields to another command's saved key counts as changed: ↺ takes it back + let changed = app.ui.settings.keymap.contains_key(b.id) || (sc.is_none() && b.default.is_some()); let mut name = RichText::new(crate::i18n::tr(b.label)).color(t.text); if changed { name = name.strong(); diff --git a/crates/ui-egui/src/shortcuts.rs b/crates/ui-egui/src/shortcuts.rs index 032548f6a..32fc6bea4 100644 --- a/crates/ui-egui/src/shortcuts.rs +++ b/crates/ui-egui/src/shortcuts.rs @@ -54,15 +54,30 @@ pub fn find_bindable(id: &str) -> Option<&'static Bindable> { /// The shortcut `id` has now: the user's choice (`""` = none), else the declared one. An entry /// that doesn't parse (a hand-edited `ui.json`, a modifier-only key saved by an older build) is -/// ignored. +/// ignored. The declared shortcut yields to a saved entry that gives its key to another known +/// command other than its contextual partner. pub fn binding<'a>(keymap: &'a Keymap, id: &str, default: Option<&'a str>) -> Option<&'a str> { match keymap.get(id) { Some(sc) if sc.is_empty() => None, Some(sc) if parse(sc).is_some() => Some(sc.as_str()), - _ => default, + _ => default.filter(|sc| !assigned_elsewhere(keymap, id, sc)), } } +/// Whether the keymap gives the key `sc` to a known command other than `id` and its contextual +/// partner. +fn assigned_elsewhere(keymap: &Keymap, id: &str, sc: &str) -> bool { + let Some(key) = parse(sc) else { return false }; + saved_keys(keymap) + .any(|(other, k)| other != id && k == key && !CONTEXTUAL.iter().any(|(x, y)| (*x == id && *y == other) || (*y == id && *x == other))) +} + +/// The keys the keymap gives to known commands, with each command's id. Entries for commands this +/// build doesn't know and entries that don't parse are ignored. +fn saved_keys(keymap: &Keymap) -> impl Iterator { + keymap.iter().filter(|(id, _)| find_bindable(id).is_some()).filter_map(|(id, sc)| Some((id.as_str(), parse(sc)?))) +} + /// The effective shortcut of command `id` (`None` for unknown commands). pub fn shortcut_of<'a>(keymap: &'a Keymap, id: &str) -> Option<&'a str> { find_bindable(id).and_then(|b| binding(keymap, id, b.default)) @@ -373,8 +388,8 @@ pub fn handle(app: &mut LightcraftApp, ctx: &egui::Context) { // The grid navigation keys act on the photos in view, so they wait while the full-screen preview // covers the grid: a key must not move the selection onto photos nobody can see. let grid_navigation = grid && !app.ui.fullscreen; - // keys the user gave to a command: the fixed bindings below (aliases, ratings) yield to them - let taken: Vec<(Modifiers, Key)> = keymap.values().filter_map(|s| parse(s)).collect(); + // keys the user gave to a known command: the fixed bindings below (aliases, ratings) yield to them + let taken: Vec<(Modifiers, Key)> = saved_keys(keymap).map(|(_, k)| k).collect(); // an open popup (a menu, a date picker's calendar) closes on Esc itself: Esc's command (Back, // which also closes dialogs) waits until nothing is open let popup_open = egui::Popup::is_any_open(ctx); @@ -828,6 +843,50 @@ mod tests { } } + /// A saved keymap entry that gives a key to one command (kept from before another command + /// declared that key) wins: the press runs that command alone, and the other command's declared + /// key comes back once the entry is gone. + #[test] + fn a_saved_key_wins_over_another_commands_declared_key() { + use lightcraft_catalog::{Flag, PhotoId}; + let mut h = library(); + h.app.ui.settings.keymap.insert("photo.increaseRating".into(), "P".into()); + assert_eq!(super::shortcut_of(&h.app.ui.settings.keymap, "photo.pick"), None); + assert_eq!(super::conflicts(&h.app.ui.settings.keymap, "view.survey", "P"), vec!["photo.increaseRating"]); + key(&mut h, "P", false); + let photo = h.app.session.catalog.photo(PhotoId(1)).unwrap(); + assert_eq!(photo.rating, 1, "the saved key raised the rating"); + assert_eq!(photo.flag, Flag::None, "Flag as Pick's declared key yielded"); + // with the entry gone, P picks again + h.app.ui.settings.keymap.remove("photo.increaseRating"); + assert_eq!(super::shortcut_of(&h.app.ui.settings.keymap, "photo.pick"), Some("P")); + key(&mut h, "P", false); + assert_eq!(h.app.session.catalog.photo(PhotoId(1)).unwrap().flag, Flag::Pick); + // a contextual partner's saved key does not silence the other side + h.app.ui.settings.keymap.insert("crop.rotateAspect".into(), "X".into()); + assert_eq!(super::shortcut_of(&h.app.ui.settings.keymap, "photo.reject"), Some("X")); + // nor does an entry for a command this build doesn't know + h.app.ui.settings.keymap.insert("no.suchCommand".into(), "U".into()); + assert_eq!(super::shortcut_of(&h.app.ui.settings.keymap, "photo.unflag"), Some("U")); + } + + /// Keymap entries for commands this build doesn't know take no key: an alias, a rating key and + /// a declared shortcut on the same keys keep working. + #[test] + fn junk_keymap_entries_leave_aliases_ratings_and_declared_keys() { + use lightcraft_catalog::{Flag, PhotoId}; + let mut h = library(); + for (id, sc) in [("no.suchCommand", "Shift+X"), ("no.otherCommand", "5"), ("no.thirdCommand", "P")] { + h.app.ui.settings.keymap.insert(id.into(), sc.into()); + } + key(&mut h, "5", false); + assert_eq!(h.app.session.catalog.photo(PhotoId(1)).unwrap().rating, 5, "5 still rates"); + key(&mut h, "P", false); + assert_eq!(h.app.session.catalog.photo(PhotoId(1)).unwrap().flag, Flag::Pick, "P still picks"); + key(&mut h, "X", true); + assert_eq!(h.app.session.catalog.photo(PhotoId(1)).unwrap().flag, Flag::Reject, "Shift+X still rejects"); + } + #[test] fn classic_f6_and_f7_toggle_the_filmstrip_and_left_panel() { let mut h = library(); diff --git a/crates/ui-egui/src/tests_keymap.rs b/crates/ui-egui/src/tests_keymap.rs index b48da7cdf..9f2e0504a 100644 --- a/crates/ui-egui/src/tests_keymap.rs +++ b/crates/ui-egui/src/tests_keymap.rs @@ -103,6 +103,27 @@ fn a_key_given_to_a_command_overrides_a_fixed_key() { assert_eq!(before, after, "the rating key must not fire too"); } +/// A declared key that yields to another command's saved key can be taken back with ↺. +#[test] +fn restore_takes_back_a_declared_key_another_command_holds() { + use crate::shortcuts::shortcut_of; + let mut h = demo(); + h.app.ui.settings.keymap.insert("photo.increaseRating".into(), "P".into()); + assert_eq!(shortcut_of(&h.app.ui.settings.keymap, "photo.pick"), None); + h.app.run("app.shortcuts", json!({})).unwrap(); + h.step(); + h.step(); + click(&mut h, "check:shortcuts.showAll"); + click(&mut h, "field:shortcutsSearch"); + assert_eq!(h.request("ui.text", json!({"text": "flag as pick"}), T)["ok"], true); + h.step(); + h.step(); + click(&mut h, "button:shortcutReset-photo.pick"); + assert_eq!(shortcut_of(&h.app.ui.settings.keymap, "photo.pick"), Some("P")); + assert_eq!(shortcut_of(&h.app.ui.settings.keymap, "photo.increaseRating"), None); + assert!(h.app.ui.settings.keymap.is_empty(), "{:?}", h.app.ui.settings.keymap); +} + /// Open the editor, filtered to `search`, and start recording for `id`. fn record(h: &mut Headless, search: &str, id: &str) { if h.app.ui.dialog.is_none() { diff --git a/docs/parity-checklist.md b/docs/parity-checklist.md index d059494ce..df3481f67 100644 --- a/docs/parity-checklist.md +++ b/docs/parity-checklist.md @@ -437,7 +437,7 @@ differences and missing bindings are in [ui-parity.md](ui-parity.md#shortcuts). | LR-PREF-PERFORMANCE | GPU / performance | P1 | ✅ | `cmd:app.gpu`, `cmd:app.memoryBudget`, `cmd:app.settings` | GPU on/off (a crash while the GPU device or its kernels are built, issue #250, turns it off for the next launch), preview size (Automatic = the whole-frame loupe render at canvas size, up to 2560 px; or a 1600–5120 px size for it; zoomed views add a window at up to 100 %, issue #323), memory budget (a quarter of it is the idle per-view stage caches' share; the open photo's views are counted and reported, and cut only past four times that, the before side's and the zoom window's first — a Before/After at 1:1 on a 2800 px canvas holds about 1 GB of GPU stages, plus the one 288 MB copy of the original, which `ui.inspect` reports as `sharedSourceBytes`), thumbnail cache size in Settings | | LR-PREF-PEOPLE | Face recognition | P2 | 🟡 | `cmd:faces.enable`, `cmd:faces.models.list`, `cmd:faces.models.inspect`, `cmd:faces.models.install`, `cmd:faces.models.download`, `cmd:faces.models.downloads`, `cmd:faces.models.downloadCancel`, `cmd:faces.models.remove`, `cmd:faces.models.select`, `cmd:dialog.faceModel`, `crates/faces/src/lib.rs`, `crates/ui-egui/src/panels/faces.rs` | Settings ▸ Faces: an off-by-default toggle and the face model list. Models are opt-in and never bundled, the 232 KB YuNet detector included. Press Download (terms first; then a pinned address, size and SHA-256 checked, fetched over pure-Rust https with `lightcraft-fetch`), or add any `.onnx` by picking or dropping it: it is recognised by hash or described from its shape, shows its licence terms and training-data note, runs a self-test, and installs only after an "I accept" box; an installed model is chosen and recognition switched on at once (the newest wins, each model keeps its own embedding cache). Non-commercial models are add-by-file only, never offered for download. Suggestions of names run on the shared pure-Rust CPU interpreter (`crates/faces/src/net.rs`, `crates/faces/src/runtime.rs`; bounded im2col/GEMM, checked ONNX shapes/operators; no external inference runtime; 40 public crops agree within 2.1e-6, one-thread SFace 34–36 ms / AuraFace 233–252 ms, see docs/faces.md) in a background worker and are never applied automatically. Thresholds are starting points calibrated on busts, not yet on live faces; the Recognise faces switch is off until a recognition model is chosen and installed (`faces.enable {enabled: true}` refuses otherwise; removing the last recogniser switches it off), issue #678 | | LR-PREF-WATERMARK | Watermark settings | P1 | ✅ | `cmd:export.savePreset`, `crates/ui-egui/src/panels/dialogs.rs` | set in the Export dialog; kept with Export with Previous and in saved export presets | -| LR-PREF-SHORTCUTS | Shortcut customisation | — | ✅ | `cmd:app.setShortcut`, `cmd:app.resetShortcuts`, `crates/ui-egui/src/panels/keymap.rs` | an extra (the reference app has no keymap editor): Help ▸ Keyboard Shortcuts (⌘/) lists every command; click a shortcut and press the new keys (Esc cancels), × removes, ↺ restores, Reset All; a key another command had moves over; saved in `ui.json`; native menu accelerators follow. Rating/label digits and secondary keys are listed read-only and yield to a key given to a command | +| LR-PREF-SHORTCUTS | Shortcut customisation | — | ✅ | `cmd:app.setShortcut`, `cmd:app.resetShortcuts`, `crates/ui-egui/src/panels/keymap.rs` | an extra (the reference app has no keymap editor): Help ▸ Keyboard Shortcuts (⌘/) lists every command; click a shortcut and press the new keys (Esc cancels), × removes, ↺ restores, Reset All; a key another command had moves over; a declared key that `ui.json` already gives another command stays with that command until ↺ takes it back; saved in `ui.json`; native menu accelerators follow. Rating/label digits and secondary keys are listed read-only and yield to a key given to a command | | LR-PREF-TECHPREVIEW | Early-access toggles | P2 | ⬜ | | | | LR-PREF-NOTIFICATIONS | Notifications | OOS | 🚫 | | | | LR-PREF-DEVICE | Device settings | P2 | ⬜ | | |