From b2e6657b95ad4df8c2e1473f636a3cb9efd3a131 Mon Sep 17 00:00:00 2001 From: Nathaniel Ledford Date: Sat, 17 Oct 2026 09:28:31 -0400 Subject: [PATCH] LR-VIEW-PHOTOGRID: the active ring shows in Compare, Survey and the face tiles too, with no seam at fractional scales Review of the selection rings found the grid test never checked the active (top-left) photo, a selected ring that fell under 3:1 on label-tinted cells, and other places still framing the selection in fixed white. - tests_grid.rs: the active photo is now checked (it was skipped by a leftover x filter): a 3 pt `ring_active` stroke over a 1 pt `ring_inner` one, and not white in a light theme - theme.rs: `ring_selected` is also tested at 3:1 against the selected cell and its label-tinted forms; Silver and Paper's goes darker (0x4a) and Charcoal and Midnight's lighter (0xc0) to pass - panels/grid.rs: the active ring is one 3 pt stroke with the 1 pt inner line on top, both on the same rect, so two separately snapped strokes cannot leave a half-pixel seam at 1.25x, 1.5x or 1.75x - panels/compare.rs: Compare, Reference and Survey frame the active tile with `selection_ring` instead of a white 2 pt stroke; the placeholder uses the tile's own tokens - panels/settings.rs: the theme preview's mini filmstrip rings its selected cell with the theme's `ring_active` - panels/people.rs: a selected face tile uses the same ring - tests_compare.rs, tests_filmstrip.rs: a Survey test in a light theme and the filmstrip test adjusted to the new widths Co-Authored-By: Claude --- .../src/dialogs/settings/appearance.rs | 2 +- crates/ui-egui/src/panels/compare.rs | 6 +-- crates/ui-egui/src/panels/grid.rs | 9 ++-- crates/ui-egui/src/panels/people.rs | 2 +- crates/ui-egui/src/tests/compare.rs | 41 +++++++++++++++++++ crates/ui-egui/src/tests/filmstrip.rs | 5 +-- crates/ui-egui/src/tests/grid.rs | 13 +++--- crates/ui-egui/src/theme.rs | 19 +++++++-- 8 files changed, 75 insertions(+), 22 deletions(-) diff --git a/crates/ui-egui/src/dialogs/settings/appearance.rs b/crates/ui-egui/src/dialogs/settings/appearance.rs index f714a97e6..5f8f428d4 100644 --- a/crates/ui-egui/src/dialogs/settings/appearance.rs +++ b/crates/ui-egui/src/dialogs/settings/appearance.rs @@ -221,7 +221,7 @@ fn theme_preview(ui: &mut egui::Ui, kind: crate::theme::ThemeKind, width: f32) { let thumb = cell.shrink(2.5); painter.rect_filled(thumb, 0.0, Color32::from_rgb(0x6f + 8 * i as u8, 0x80, 0x8f)); if selected { - painter.rect_stroke(thumb, 0.0, Stroke::new(1.0, p.pick), StrokeKind::Outside); + crate::panels::grid::selection_ring(&painter, &p, thumb, lightcraft_engine::SelectionState::Active); } } } diff --git a/crates/ui-egui/src/panels/compare.rs b/crates/ui-egui/src/panels/compare.rs index 9b9a23673..88463a285 100644 --- a/crates/ui-egui/src/panels/compare.rs +++ b/crates/ui-egui/src/panels/compare.rs @@ -2,7 +2,7 @@ //! pan) and **Survey** (the selected photos tiled). Rating, flag and label keys act on the active //! photo only here, and with Auto Advance on they move to the next candidate / photo. -use egui::{Align2, Color32, Rect, Sense, Stroke, StrokeKind, pos2, vec2}; +use egui::{Align2, Color32, Rect, Sense, Stroke, pos2, vec2}; use lightcraft_catalog::{Flag, PhotoId}; use serde_json::{Value, json}; @@ -286,7 +286,7 @@ fn photo_tile(app: &mut LightcraftApp, ui: &mut egui::Ui, id: PhotoId, slot: Slo display = super::detail::fit_rect(img_area, texture_aspect, zoom, native, ppp, app.ui.pan); p.image(tex.tex.id(), display, uv, Color32::WHITE); } else { - p.rect_filled(img.intersect(img_area), 0.0, Tokens::get(ui.ctx()).skeleton); + p.rect_filled(img.intersect(img_area), 0.0, t.placeholder); } if photo.flag == Flag::Reject { p.rect_filled(display, 0.0, Color32::from_black_alpha(110)); @@ -296,7 +296,7 @@ fn photo_tile(app: &mut LightcraftApp, ui: &mut egui::Ui, id: PhotoId, slot: Slo } let active = app.session.selection.active == Some(id); if active { - p.rect_stroke(display.intersect(img_area.shrink(1.0)), 0.0, Stroke::new(2.0, Color32::WHITE), StrokeKind::Outside); + super::grid::selection_ring(&p, &t, display.intersect(img_area.shrink(3.0)), lightcraft_engine::SelectionState::Active); } // Compare and Reference are where sharpness is judged: as in the loupe, say when what is on // screen isn't the render on its way (a thumbnail, the last render after an edit, a magnified diff --git a/crates/ui-egui/src/panels/grid.rs b/crates/ui-egui/src/panels/grid.rs index 656ddda91..ed7614590 100644 --- a/crates/ui-egui/src/panels/grid.rs +++ b/crates/ui-egui/src/panels/grid.rs @@ -651,16 +651,17 @@ pub fn layout(aspects: &[f32], groups: &[(usize, usize)], avail_w: f32, target: out } -/// The ring round a selected photo, drawn outside `photo`. The active one is 3 pt: a 1 pt inner line -/// in [`Tokens::ring_inner`] against the photo's edge (so the ring reads on a white photo and on a -/// black one) and a 2 pt line in [`Tokens::ring_active`]. The others are a single 1.5 pt line in +/// The ring round a selected photo, drawn outside `photo`. The active one is 3 pt in +/// [`Tokens::ring_active`] with a 1 pt line in [`Tokens::ring_inner`] over its inner edge, against +/// the photo (so the ring reads on a white photo and on a black one). The others are a single 1.5 pt line in /// [`Tokens::ring_selected`]; the two are told apart by weight, not by lightness alone. pub(crate) fn selection_ring(p: &egui::Painter, t: &Tokens, photo: Rect, state: lightcraft_engine::SelectionState) { use lightcraft_engine::SelectionState; match state { SelectionState::Active => { + // both on the same rect, so no seam opens between them at a fractional scale + p.rect_stroke(photo, 0.0, Stroke::new(3.0, t.ring_active), StrokeKind::Outside); p.rect_stroke(photo, 0.0, Stroke::new(1.0, t.ring_inner), StrokeKind::Outside); - p.rect_stroke(photo.expand(1.0), 0.0, Stroke::new(2.0, t.ring_active), StrokeKind::Outside); } SelectionState::Selected => { p.rect_stroke(photo, 0.0, Stroke::new(1.5, t.ring_selected), StrokeKind::Outside); diff --git a/crates/ui-egui/src/panels/people.rs b/crates/ui-egui/src/panels/people.rs index 73d19cbaa..f81b90372 100644 --- a/crates/ui-egui/src/panels/people.rs +++ b/crates/ui-egui/src/panels/people.rs @@ -174,7 +174,7 @@ fn card(app: &mut LightcraftApp, ui: &mut egui::Ui, person: &Person, r: Rect, ed p.rect_filled(pill, 4.0, Color32::from_black_alpha(175)); p.galley(pill.min + vec2(5.0, 3.0), g, Color32::WHITE); if selected { - p.rect_stroke(face, 3.0, Stroke::new(2.0, Color32::WHITE), StrokeKind::Outside); + super::grid::selection_ring(p, &t, face, lightcraft_engine::SelectionState::Active); } else if resp.hovered() { p.rect_stroke(face, 3.0, Stroke::new(1.0, t.text_dim), StrokeKind::Outside); } diff --git a/crates/ui-egui/src/tests/compare.rs b/crates/ui-egui/src/tests/compare.rs index c816b3c67..b0ccb0b14 100644 --- a/crates/ui-egui/src/tests/compare.rs +++ b/crates/ui-egui/src/tests/compare.rs @@ -318,3 +318,44 @@ fn info_panel_label_in_compare_labels_the_active_photo_and_shows_it() { assert_eq!(photo(&h, cand).rating, 3); assert_eq!(photo(&h, sel).rating, 0, "the select is not rated with it"); } + +/// Survey frames its active tile in the theme's active ring, which must not be the white that +/// vanishes on a light theme's canvas. +#[test] +fn survey_rings_the_active_tile_in_the_theme_colour() { + use egui::epaint::Shape; + let mut h = demo(); + h.app.ui.settings.appearance_mode = crate::state::AppearanceMode::Light; + let vis: Vec = h.app.session.visible_cloned().iter().map(|p| p.0).collect(); + h.request("engine.execute", json!({"command": "library.select", "params": {"ids": [vis[0], vis[1], vis[2]], "active": vis[1]}}), T); + let r = h.request("engine.execute", json!({"command": "view.survey"}), T); + assert_eq!(r["ok"], true, "{r}"); + h.settle(SETTLE); + let t = crate::theme::Tokens::get(&h.view.ctx); + assert_ne!(t.ring_active, egui::Color32::WHITE); + // the filmstrip below rings its active cell the same way: only the tile counts here + let films: Vec = h.app.widgets.iter().filter(|(w, _)| w.starts_with("film:")).map(|(_, r)| *r).collect(); + let rings: Vec<(f32, egui::Color32)> = h + .view + .painted_shapes() + .iter() + .filter_map(|s| match s { + Shape::Rect(rs) + if rs.stroke_kind == egui::StrokeKind::Outside + && rs.stroke.color == t.ring_active + && !films.iter().any(|f| f.intersects(rs.rect)) => + { + Some((rs.stroke.width, rs.stroke.color)) + } + _ => None, + }) + .collect(); + assert_eq!(rings, vec![(3.0, t.ring_active)], "one active tile, ringed once"); + let white = h + .view + .painted_shapes() + .iter() + .filter(|s| matches!(s, Shape::Rect(rs) if rs.stroke.width == 2.0 && rs.stroke.color == egui::Color32::WHITE)) + .count(); + assert_eq!(white, 0, "no white 2 pt frame"); +} diff --git a/crates/ui-egui/src/tests/filmstrip.rs b/crates/ui-egui/src/tests/filmstrip.rs index a95530b2a..de05996d1 100644 --- a/crates/ui-egui/src/tests/filmstrip.rs +++ b/crates/ui-egui/src/tests/filmstrip.rs @@ -95,8 +95,7 @@ fn the_filmstrip_marks_the_active_photo_heavier_than_the_other_selected_one() { .collect() }; let (on_active, on_other, on_unselected) = (strokes(active), strokes(other), strokes(c)); - assert!(on_active.iter().map(|(w, _)| w).sum::() >= 3.0 && on_active.iter().any(|(_, col)| *col == t.ring_active), "{on_active:?}"); - assert_eq!(on_other.iter().map(|(_, col)| *col).collect::>(), vec![t.ring_selected], "{on_other:?}"); - assert!(on_other.iter().map(|(w, _)| w).sum::() < 3.0, "{on_other:?}"); + assert_eq!(on_active, vec![(3.0, t.ring_active), (1.0, t.ring_inner)]); + assert_eq!(on_other, vec![(1.5, t.ring_selected)]); assert!(on_unselected.is_empty(), "{on_unselected:?}"); } diff --git a/crates/ui-egui/src/tests/grid.rs b/crates/ui-egui/src/tests/grid.rs index 888021198..4e32756e4 100644 --- a/crates/ui-egui/src/tests/grid.rs +++ b/crates/ui-egui/src/tests/grid.rs @@ -164,24 +164,25 @@ fn unloaded_thumbnails_show_their_selection() { .collect() }; let mut checked = 0; + let mut saw_active = false; for (w, r) in h.app.widgets.iter().filter(|(w, _)| w.starts_with("thumb:")) { - if !canvas.shrink(4.0).contains_rect(*r) || r.min.x < 6.0 { + if !canvas.shrink(4.0).contains_rect(*r) { continue; } let id = w.trim_start_matches("thumb:").parse::().unwrap(); let strokes = ring(*r); - let width: f32 = strokes.iter().map(|(w, _)| w).sum(); if id == active.0 { - assert!(width >= 3.0, "photo {id}: active ring {strokes:?}"); - assert!(strokes.iter().any(|(_, c)| *c == t.ring_active), "photo {id}: {strokes:?}"); - assert!(strokes.iter().all(|(_, c)| *c != egui::Color32::WHITE), "photo {id}: a white ring vanishes on the light grid: {strokes:?}"); + assert_ne!(t.ring_active, egui::Color32::WHITE, "the light theme's ring must not vanish on its grid"); + assert_eq!(strokes, vec![(3.0, t.ring_active), (1.0, t.ring_inner)], "photo {id}: active ring"); + saw_active = true; } else { - assert!((1.0..3.0).contains(&width), "photo {id}: selected ring {strokes:?}"); + assert_eq!(strokes.iter().map(|(w, _)| *w).sum::(), 1.5, "photo {id}: selected ring {strokes:?}"); assert_eq!(strokes.iter().map(|(_, c)| *c).collect::>(), vec![t.ring_selected], "photo {id}"); } checked += 1; } assert!(checked >= 3, "checked {checked}"); + assert!(saw_active, "the active photo (top left) was checked"); } /// A folder of unedited raws on a slow share: the grid must not decode every raw to draw a 512 px diff --git a/crates/ui-egui/src/theme.rs b/crates/ui-egui/src/theme.rs index a7b129e8c..d57258a4a 100644 --- a/crates/ui-egui/src/theme.rs +++ b/crates/ui-egui/src/theme.rs @@ -140,7 +140,7 @@ impl Default for Tokens { skeleton: Color32::from_rgb(0x25, 0x25, 0x25), ring_active: Color32::from_rgb(0xff, 0xff, 0xff), ring_inner: Color32::from_rgb(0x00, 0x00, 0x00), - ring_selected: Color32::from_rgb(0xaa, 0xaa, 0xaa), + ring_selected: Color32::from_rgb(0xc0, 0xc0, 0xc0), divider: Color32::from_rgb(0x1c, 0x1c, 0x1c), inset: Color32::from_rgb(0x23, 0x23, 0x23), field: Color32::from_rgb(0x22, 0x22, 0x22), @@ -203,7 +203,7 @@ impl Tokens { ThemeKind::Silver => Tokens { ring_active: rgb(0x141414), ring_inner: rgb(0xffffff), - ring_selected: rgb(0x5a5a5a), + ring_selected: rgb(0x4a4a4a), chrome: rgb(0xe6e6e6), canvas: rgb(0xcfcfcf), grid_bg: rgb(0xd9d9d9), @@ -237,7 +237,7 @@ impl Tokens { ThemeKind::Paper => Tokens { ring_active: rgb(0x141414), ring_inner: rgb(0xffffff), - ring_selected: rgb(0x5a5a5a), + ring_selected: rgb(0x4a4a4a), chrome: rgb(0xfafafa), canvas: rgb(0xececec), grid_bg: rgb(0xf2f2f2), @@ -489,7 +489,18 @@ mod tests { for (what, surface) in [("grid", t.grid_bg), ("cell", t.cell), ("selected cell", t.cell_selected)] { assert!(contrast(t.ring_active, surface) >= 3.0, "{kind:?}: active ring on the {what}: {:.2}", contrast(t.ring_active, surface)); } - assert!(contrast(t.ring_selected, t.grid_bg) >= 3.0, "{kind:?}: selected ring on the grid: {:.2}", contrast(t.ring_selected, t.grid_bg)); + // the selected ring also sits on a selected cell (square grid, filmstrip), label-tinted or not + let mut selected_cells = vec![("grid", t.grid_bg), ("selected cell", t.cell_selected)]; + for label in lightcraft_catalog::ColorLabel::ALL { + selected_cells.push(("labelled selected cell", label_background(t.cell_selected, Some(label), true))); + } + for (what, surface) in selected_cells { + assert!( + contrast(t.ring_selected, surface) >= 3.0, + "{kind:?}: selected ring on the {what}: {:.2}", + contrast(t.ring_selected, surface) + ); + } // the thin inner line of the active ring is the ring's opposite: it parts the ring from the photo's edge assert!(contrast(t.ring_inner, t.ring_active) >= 3.0, "{kind:?}: inner line against the active ring"); }