KEY-GRID: show live cycle shortcut in View menus

Fixes #2500. Preserve direct menu selection and native accelerator ownership.

Co-Authored-By: Codex <noreply@openai.com>
This commit is contained in:
tylerphotos
2026-10-21 22:32:43 -05:00
co-authored by Codex
parent bfb23db2ca
commit 48e1cde8b7
5 changed files with 158 additions and 38 deletions
+18 -34
View File
@@ -16,7 +16,7 @@ use std::collections::{HashMap, HashSet};
use std::sync::mpsc::{Receiver, channel};
use lightcraft_ui_egui::LightcraftApp;
use lightcraft_ui_egui::menubar::{MenuNode, menu_bar};
use lightcraft_ui_egui::menubar::{MenuNode, menu_bar, native_item_presentation, native_menu_delivers as menu_delivers};
use muda::accelerator::{Accelerator, Code, Modifiers};
use muda::{CheckMenuItem, Menu, MenuEvent, MenuItem, PredefinedMenuItem, Submenu};
use serde_json::Value;
@@ -154,13 +154,6 @@ fn yields_to_text(sc: &str) -> bool {
!sc.contains("Cmd") && !sc.contains("Ctrl") || TEXT_EDIT.contains(&sc)
}
/// Whether AppKit hands this shortcut to the menu bar: only key presses with ⌘ or ⌃, and function
/// keys. A plain `C` or `⇧P` never reaches it (the window's view takes the key), so egui has to run
/// those even though their menu item shows the key.
fn menu_delivers(sc: &str) -> bool {
sc.split('+').any(|part| matches!(part, "Cmd" | "Ctrl") || part.strip_prefix('F').is_some_and(|n| n.parse::<u8>().is_ok()))
}
/// The shortcuts the menu bar runs itself, for `LightcraftApp::native_shortcuts`.
fn owned_by_menu<'a>(installed: impl Iterator<Item = &'a str>, text_focus: bool) -> HashSet<String> {
installed.filter(|sc| menu_delivers(sc) && !(text_focus && yields_to_text(sc))).map(str::to_string).collect()
@@ -264,7 +257,7 @@ impl NativeMenu {
.cloned()
.collect();
let nodes = tidy_separators(nodes);
self.append_nodes(&sub, &nodes);
self.append_nodes(&sub, &nodes, &app.ui.settings.keymap);
if title == "File" {
// ⌘W, the system's own item
let _ = sub.append_items(&[
@@ -290,7 +283,7 @@ impl NativeMenu {
self.publish_shortcuts(app);
}
fn append_nodes(&mut self, sub: &Submenu, nodes: &[MenuNode]) {
fn append_nodes(&mut self, sub: &Submenu, nodes: &[MenuNode], keymap: &lightcraft_ui_egui::shortcuts::Keymap) {
for n in nodes {
match n {
MenuNode::Separator => {
@@ -298,7 +291,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, keymap);
let _ = sub.append(&s);
}
MenuNode::Item { id, params, label, shortcut, enabled, checked } => {
@@ -306,26 +299,16 @@ impl NativeMenu {
if self.items.contains_key(&key) {
continue; // muda ids must be unique
}
let accel = shortcut.as_deref().filter(|s| !CONTEXTUAL.contains(s)).and_then(accelerator);
let (native_label, native_key) = native_item_presentation(keymap, id, params, label, shortcut.as_deref());
let accel = native_key.filter(|s| !CONTEXTUAL.contains(s)).and_then(accelerator);
let handle = match checked {
Some(c) => {
let it = CheckMenuItem::with_id(
key.clone(),
lightcraft_ui_egui::menubar::display_item_label(id, params, label).replace('&', "&&"),
*enabled,
*c,
accel,
);
let it = CheckMenuItem::with_id(key.clone(), native_label.replace('&', "&&"), *enabled, *c, accel);
let _ = sub.append(&it);
Handle::Check(it)
}
None => {
let it = MenuItem::with_id(
key.clone(),
lightcraft_ui_egui::menubar::display_item_label(id, params, label).replace('&', "&&"),
*enabled,
accel,
);
let it = MenuItem::with_id(key.clone(), native_label.replace('&', "&&"), *enabled, accel);
let _ = sub.append(&it);
Handle::Plain(it)
}
@@ -338,7 +321,7 @@ impl NativeMenu {
params: params.clone(),
shortcut: shortcut.clone(),
accel,
label: label.clone(),
label: native_label,
enabled: *enabled,
checked: *checked,
},
@@ -382,18 +365,19 @@ impl NativeMenu {
self.rebuild(app);
return quit;
}
fn walk(nodes: &[MenuNode], items: &mut HashMap<String, Item>) {
fn walk(nodes: &[MenuNode], items: &mut HashMap<String, Item>, keymap: &lightcraft_ui_egui::shortcuts::Keymap) {
for n in nodes {
match n {
MenuNode::Submenu { children, .. } => walk(children, items),
MenuNode::Item { id, params, label, enabled, checked, .. } => {
MenuNode::Submenu { children, .. } => walk(children, items, keymap),
MenuNode::Item { id, params, label, shortcut, enabled, checked } => {
let Some(it) = items.get_mut(&MenuNode::key(id, params)) else { continue };
if it.label != *label {
let (native_label, _) = native_item_presentation(keymap, id, params, label, shortcut.as_deref());
if it.label != native_label {
match &it.handle {
Handle::Plain(h) => h.set_text(lightcraft_ui_egui::menubar::display_item_label(id, params, label).replace('&', "&&")),
Handle::Check(h) => h.set_text(lightcraft_ui_egui::menubar::display_item_label(id, params, label).replace('&', "&&")),
Handle::Plain(h) => h.set_text(native_label.replace('&', "&&")),
Handle::Check(h) => h.set_text(native_label.replace('&', "&&")),
}
it.label = label.clone();
it.label = native_label;
}
if it.enabled != *enabled {
match &it.handle {
@@ -414,7 +398,7 @@ impl NativeMenu {
}
}
}
walk(&bar.iter().flat_map(|(_, v)| v.clone()).collect::<Vec<_>>(), &mut self.items);
walk(&bar.iter().flat_map(|(_, v)| v.clone()).collect::<Vec<_>>(), &mut self.items, &app.ui.settings.keymap);
let focus = ctx.egui_wants_keyboard_input();
let capturing = app.recording_shortcut.is_some();
+128
View File
@@ -55,6 +55,32 @@ pub fn display_item_label<'a>(id: &str, params: &Value, label: &'a str) -> &'a s
}
}
/// Whether AppKit offers a shortcut to its menu instead of the window. Plain letters stay
/// with egui even when the native menu displays them as key equivalents.
pub fn native_menu_delivers(sc: &str) -> bool {
sc.split('+').any(|part| matches!(part, "Cmd" | "Ctrl") || part.strip_prefix('F').is_some_and(|n| n.parse::<u8>().is_ok()))
}
/// Native label and accelerator for a menu row. A grid row may borrow the cycle key as a
/// hint, but must not consume a modified key and turn cycling into direct grid selection.
/// AppKit has no separate shortcut-hint column: borrowed native-owned keys are appended to
/// the translated label instead, leaving their dispatch with egui. Ordinary G stays in the
/// native key-equivalent column because AppKit never hands it to the menu.
pub fn native_item_presentation<'a>(
keymap: &crate::shortcuts::Keymap,
id: &str,
params: &Value,
label: &str,
shortcut: Option<&'a str>,
) -> (String, Option<&'a str>) {
let label = display_item_label(id, params, label);
let borrowed = matches!(id, "view.photoGrid" | "view.squareGrid") && shortcut != crate::shortcuts::shortcut_of(keymap, id);
match shortcut.filter(|sc| borrowed && native_menu_delivers(sc)) {
Some(sc) => (format!("{label} ({})", shortcut_text(sc, true)), None),
None => (label.to_string(), shortcut),
}
}
/// Top-level menus in order (the macOS app menu is added by the host).
pub const MENUS: &[&str] = &["File", "Edit", "View", "Photo", "Window", "Help"];
@@ -740,6 +766,7 @@ fn node(app: &LightcraftApp, e: &MenuEntry) -> MenuNode {
// in an album the Delete key removes from the album (issue #1805): the menu says so
let delete_key = crate::shortcuts::shortcut_of(&app.ui.settings.keymap, "photo.delete");
let shortcut = match e.id.as_str() {
"view.photoGrid" | "view.squareGrid" => shortcut.or_else(|| crate::shortcuts::shortcut_of(&app.ui.settings.keymap, "view.gridToggle")),
"album.removePhotos" => delete_key,
"photo.delete" if matches!(app.session.source, lightcraft_engine::LibrarySource::Album(_)) => None,
_ => shortcut,
@@ -1076,6 +1103,107 @@ mod tests {
})
}
/// #2500: a direct-grid binding takes precedence over the borrowed cycle hint.
#[test]
fn grid_menu_shortcut_hints_preserve_direct_bindings_and_clicks() {
let mut a = app();
crate::shortcuts::assign(&mut a.ui.settings.keymap, "view.photoGrid", Some("Cmd+Shift+1")).unwrap();
let bar = menu_bar(&a);
let nodes = &bar.iter().find(|(title, _)| title == "View").unwrap().1;
assert!(matches!(find(nodes, "view.photoGrid"), Some(MenuNode::Item { shortcut: Some(sc), .. }) if sc == "Cmd+Shift+1"));
assert!(matches!(find(nodes, "view.squareGrid"), Some(MenuNode::Item { shortcut: Some(sc), .. }) if sc == "G"));
// A hint never assigns the cycle key to another command.
assert_eq!(crate::shortcuts::shortcut_of(&a.ui.settings.keymap, "view.squareGrid"), None);
run_item(&mut a, "view.photoGrid", Value::Null).unwrap();
assert_eq!(a.ui.view, ViewMode::PhotoGrid);
run_item(&mut a, "view.photoGrid", Value::Null).unwrap();
assert_eq!(a.ui.view, ViewMode::PhotoGrid, "clicking the active row does not cycle");
run_item(&mut a, "view.squareGrid", Value::Null).unwrap();
assert_eq!(a.ui.view, ViewMode::SquareGrid);
run_item(&mut a, "view.gridToggle", Value::Null).unwrap();
assert_eq!(a.ui.view, ViewMode::PhotoGrid);
}
/// #2500: native hints never install a borrowed modifier/function key as a direct-grid
/// accelerator. Plain G remains in AppKit's shortcut column and is dispatched by egui.
#[test]
fn grid_menu_shortcut_hints_keep_native_toggle_dispatch() {
let mut keymap = crate::shortcuts::Keymap::new();
for id in ["view.photoGrid", "view.squareGrid"] {
let (label, accelerator) = native_item_presentation(&keymap, id, &Value::Null, "Photo Grid", Some("G"));
assert_eq!(label, "Photo Grid");
assert_eq!(accelerator, Some("G"));
assert!(!native_menu_delivers(accelerator.unwrap()));
for (key, text) in [("Cmd+Shift+G", "⌘⇧G"), ("Ctrl+G", "⌃G"), ("F3", "F3")] {
crate::shortcuts::assign(&mut keymap, "view.gridToggle", Some(key)).unwrap();
let (label, accelerator) = native_item_presentation(&keymap, id, &Value::Null, "Photo Grid", Some(key));
assert_eq!(label, format!("Photo Grid ({text})"));
assert_eq!(accelerator, None, "egui keeps {key}");
}
}
crate::shortcuts::assign(&mut keymap, "view.photoGrid", Some("Cmd+Shift+1")).unwrap();
let (label, accelerator) = native_item_presentation(&keymap, "view.photoGrid", &Value::Null, "Photo Grid", Some("Cmd+Shift+1"));
assert_eq!(label, "Photo Grid");
assert_eq!(accelerator, Some("Cmd+Shift+1"), "the row's own shortcut is a real accelerator");
}
/// #2500: the control channel runs the real keyboard path exactly once and menu commands
/// continue to select their named grid after the cycle shortcut is remapped.
#[test]
fn grid_menu_shortcut_hints_preserve_control_keyboard_dispatch() {
let mut h = crate::headless::Headless::new(app(), [960.0, 720.0], 1.0);
h.app.ui.view = ViewMode::PhotoGrid;
for _ in 0..3 {
h.step();
}
let timeout = std::time::Duration::from_secs(20);
let press = |h: &mut crate::headless::Headless, params| {
let reply = h.request("ui.key", params, timeout);
assert_eq!(reply["ok"], true, "{reply}");
};
press(&mut h, json!({"key": "G"}));
assert_eq!(h.app.ui.view, ViewMode::SquareGrid);
press(&mut h, json!({"key": "G"}));
assert_eq!(h.app.ui.view, ViewMode::PhotoGrid);
crate::shortcuts::assign(&mut h.app.ui.settings.keymap, "view.gridToggle", Some("Cmd+Shift+G")).unwrap();
press(&mut h, json!({"key": "G"}));
assert_eq!(h.app.ui.view, ViewMode::PhotoGrid);
press(&mut h, json!({"key": "G", "cmd": true, "shift": true}));
assert_eq!(h.app.ui.view, ViewMode::SquareGrid);
press(&mut h, json!({"key": "G", "cmd": true, "shift": true}));
assert_eq!(h.app.ui.view, ViewMode::PhotoGrid);
for _ in 0..2 {
let reply = h.request("ui.menu.invoke", json!({"command": "view.squareGrid"}), timeout);
assert_eq!(reply["ok"], true, "{reply}");
assert_eq!(h.app.ui.view, ViewMode::SquareGrid);
}
}
/// #2500: grid view rows show the separate cycle command's current shortcut.
#[test]
fn grid_menu_shortcut_hints_follow_the_cycle_key() {
let mut a = app();
let hint = |a: &LightcraftApp, id: &str| {
let bar = menu_bar(a);
let nodes = &bar.iter().find(|(title, _)| title == "View").expect("View menu").1;
match find(nodes, id).expect("grid row") {
MenuNode::Item { shortcut, .. } => shortcut.clone(),
_ => panic!("grid row must be an item"),
}
};
for id in ["view.photoGrid", "view.squareGrid"] {
assert_eq!(hint(&a, id).as_deref(), Some("G"), "{id}");
}
crate::shortcuts::assign(&mut a.ui.settings.keymap, "view.gridToggle", Some("Cmd+Shift+G")).unwrap();
for id in ["view.photoGrid", "view.squareGrid"] {
assert_eq!(hint(&a, id).as_deref(), Some("Cmd+Shift+G"), "{id}");
}
crate::shortcuts::assign(&mut a.ui.settings.keymap, "view.gridToggle", None).unwrap();
for id in ["view.photoGrid", "view.squareGrid"] {
assert_eq!(hint(&a, id), None, "{id}");
}
}
#[test]
fn view_menu_offers_the_hold_and_the_refresh_and_checks_the_hold_while_it_is_on() {
let mut a = app();
+3 -2
View File
@@ -1,6 +1,6 @@
# Gaps: where LightCraft falls short of Lightroom
> **Last reviewed:** 2026-10-11 · **Last updated:** 2026-10-15 · **Change:** minor (gap 15: Print Page Options — page numbers, page info, crop marks)
> **Last reviewed:** 2026-10-11 · **Last updated:** 2026-10-21 · **Change:** patch (gap 28: the View menu shows the grid-cycle shortcut)
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,7 @@ 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**: Windows installer UI unverified on Windows (#79); 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**: rating `[` `]`, flag cycling, filter-bar keys, F1 help, panel keys (Tab, F5, F8; F6 and F7 toggle the filmstrip and the left panel) | [ui-parity](ui-parity.md#shortcuts) | Classic muscle memory | 4–8 | [ui-parity](ui-parity.md) |
| 28 | **Small Classic shortcuts and keys**: rating `[` `]`, flag cycling, filter-bar keys, F1 help, panel keys (Tab, F5, F8; F6 and F7 toggle the filmstrip and the left panel); the grid-cycle key is now shown beside both grid rows in View (#2500) | [ui-parity](ui-parity.md#shortcuts) | Classic muscle memory | 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)).
@@ -77,6 +77,7 @@ Beta-blocking gaps (B) total **~230–430 h**; with the stability and platform w
| Date | Change | Summary |
|---|---|---|
| 2026-10-21 | patch | Gap 28: View shows the current grid-cycle key beside Photo Grid and Square Grid (#2500); no new binding and no change to the remaining shortcut gap or hours |
| 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-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 |
+3 -2
View File
@@ -1,6 +1,6 @@
# Lightroom parity checklist
> **Last reviewed:** 2026-10-10 · **Last updated:** 2026-10-20 · **Change:** minor (LR-VIEW-ZOOM: only a full-size drag within 20 ms clears a slow-drag verdict) · **Target:** Adobe Lightroom Classic 15.5 / Lightroom 9.6
> **Last reviewed:** 2026-10-10 · **Last updated:** 2026-10-21 · **Change:** patch (KEY-GRID: live grid-cycle shortcut hints in the View menu) · **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
@@ -603,7 +603,7 @@ From `06-shortcuts.md` part 1. Evidence is our binding; conflicts are explained
| KEY-EDIT | Edit — E | P0 | ✅ | `cmd:panel.edit` | |
| KEY-FULLSCREEN | Full-screen preview — F | P1 | ✅ | `cmd:view.fullScreenPreview` | |
| KEY-LIGHTSOUT | Lights Out — Shift+L (Classic's L is Linear Gradient here) | P2 | ✅ | `cmd:view.lightsOut` | On → Dim (80 %) → Off; Esc turns the lights on |
| KEY-GRID | Grid — G | P0 | ✅ | `cmd:view.gridToggle`, `cmd:view.photoGrid` | G toggles Photo Grid ↔ Square Grid |
| KEY-GRID | Grid — G | P0 | ✅ | `cmd:view.gridToggle`, `cmd:view.photoGrid` | G toggles Photo Grid ↔ Square Grid; both View menu rows show its live shortcut (including remapping/unbinding), with direct grid bindings taking priority; menu clicks still choose the named grid (#2500) |
| KEY-INFO | Info — I | P0 | ✅ | `cmd:panel.info` | |
| KEY-KEYWORDS | Keywords — K | P0 | ✅ | `cmd:panel.keywords` | |
| KEY-CLIPBOARD | Copy / paste edit settings — ⌘C / ⌘V | P0 | ✅ | `cmd:develop.copy`, `cmd:develop.paste`, `crates/ui-egui/src/shortcuts.rs` (`matches`) | ⌘X has nothing to cut outside text fields; on Windows and Linux, where the windowing layer sends Ctrl+C / Ctrl+V as clipboard events rather than key presses, those run the shortcuts too (⇧Insert pastes plainly); there Ctrl+V only arrives while the system clipboard holds text (egui-winit drops it otherwise), so with an empty clipboard Paste Settings needs the menu |
@@ -797,6 +797,7 @@ Latin stays upright. Advanced Japanese composition remains open. Regression cove
| Date | Change | Summary |
|---|---|---|
| 2026-10-21 | patch | KEY-GRID: both grid rows in View show the cycle shortcut and follow keymap changes; direct bindings and native accelerator ownership are preserved (#2500); status and counts unchanged |
| 2026-10-20 | patch | LR-VIEW-ZOOM: the 20 ms rule clears a slow-drag verdict only on a drag timed at full size; a drag that drafts small leaves it unchanged, as the code does; status unchanged |
| 2026-10-19 | patch | LR-VIEW-ZOOM: the slow slider-drag verdict is described as per photo and control, as the code keys it; status unchanged |
| 2026-10-15 | minor | LR-VIEW-ZOOM: a slow slider-drag verdict needs two drags over 20 ms and expires after 30 s (doubling to 240 s); hidden-window frame times are ignored; the slow set forgets its oldest entry, not all (issue #330); status unchanged |
+6
View File
@@ -1,5 +1,11 @@
# What's new in LightCraft
## 2026-10-21
### Find the grid shortcut in View
- Photo Grid and Square Grid now show the grid-cycle shortcut (G by default) in the View menu. The hints follow
shortcut changes; clicking a menu row still selects that grid (issue #2500).
## 2026-10-19
### Hidden Local locations come back one at a time