From 495b1ac9fb80f06e7dad8bb312fe96858bda552e Mon Sep 17 00:00:00 2001 From: Nathaniel Ledford Date: Thu, 8 Oct 2026 18:31:45 -0400 Subject: [PATCH] #323: tests: the wipe layering and pane clipping are checked on the painted shapes A review showed the wipe test asserted on a hand-written list of labels (deleting the stand-in draw calls still passed) and nothing tested the clip at the call site (replacing it with the canvas still passed). The tests now read the meshes the frame painted: the Before window comes after the stand-in and stays inside its side of the line, and side by side each window is painted inside its pane. Both mutations of the code now fail them. Co-Authored-By: Claude Sonnet 5.5 --- crates/ui-egui/src/headless.rs | 2 +- crates/ui-egui/src/lib.rs | 5 -- crates/ui-egui/src/panels/detail.rs | 8 ---- crates/ui-egui/src/tests_preview_limit.rs | 56 +++++++++++++++++++---- 4 files changed, 49 insertions(+), 22 deletions(-) diff --git a/crates/ui-egui/src/headless.rs b/crates/ui-egui/src/headless.rs index 41a55651..de206582 100644 --- a/crates/ui-egui/src/headless.rs +++ b/crates/ui-egui/src/headless.rs @@ -32,7 +32,7 @@ const FRAME_DT: f64 = 1.0 / 60.0; pub struct HeadlessView { pub ctx: egui::Context, pub textures: TextureStore, - shapes: Vec, + pub(crate) shapes: Vec, pixels_per_point: f32, size: egui::Vec2, frames: u64, diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index 56aeb97c..07b01817 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -209,9 +209,6 @@ pub struct LightcraftApp { pub(crate) size_hold: region::SizeHold, /// (photo, look, window frame size) a window was refused for: it reads more than one render holds. pub(crate) window_refused: Option<(lightcraft_catalog::PhotoId, u64, usize)>, - /// What the loupe drew last frame, in order (tests check layering). - #[cfg(test)] - pub(crate) draw_order: Vec<&'static str>, pub(crate) region_tiles: std::collections::HashMap<(bool, u64), region::RegionView>, /// Photo Merge dialog previews and background merges. pub merge: merge::MergeState, @@ -284,8 +281,6 @@ impl LightcraftApp { region_before_view: None, size_hold: Default::default(), window_refused: None, - #[cfg(test)] - draw_order: Vec::new(), region_tiles: Default::default(), merge: merge::MergeState::default(), import: None, diff --git a/crates/ui-egui/src/panels/detail.rs b/crates/ui-egui/src/panels/detail.rs index c51609f9..0a080e07 100644 --- a/crates/ui-egui/src/panels/detail.rs +++ b/crates/ui-egui/src/panels/detail.rs @@ -584,8 +584,6 @@ pub fn show(app: &mut LightcraftApp, ui: &mut egui::Ui) { } } app.loupe_shown = Some((id, shown)); - #[cfg(test)] - app.draw_order.clear(); // a wipe shows the Before whole-frame render on its side of the line… if app.ui.before_after == BeforeAfter::Split && let Some(tex) = app.renderer.textures.get(&Slot::Before).filter(|t| t.photo == id) @@ -601,17 +599,11 @@ pub fn show(app: &mut LightcraftApp, ui: &mut egui::Ui) { let top = Rect::from_min_max(before_rect.min, pos2(before_rect.right(), before_rect.center().y)); p.image(tex.tex.id(), top, Rect::from_min_max(pos2(0.0, 0.0), pos2(1.0, 0.5)), Color32::WHITE); } - #[cfg(test)] - if matches!(app.ui.before_after, BeforeAfter::Split | BeforeAfter::SplitTopBottom) { - app.draw_order.push("wipe stand-in"); - } // …and the window renders go over the whole-frame ones, all of them, where they belong in the // frame (clipped to their pane, or their side of the line) for (v, view) in &windows { let clip = crate::region::window_clip(app.ui.before_after, v.before, v.visible, canvas, img_rect); draw_window(&p.with_clip_rect(clip), app, &window_ctx, v, view); - #[cfg(test)] - app.draw_order.push(if v.before { "window RegionBefore" } else { "window Region" }); } // the line of a wipe over both if app.ui.before_after == BeforeAfter::Split { diff --git a/crates/ui-egui/src/tests_preview_limit.rs b/crates/ui-egui/src/tests_preview_limit.rs index d019d3c6..8150fd4f 100644 --- a/crates/ui-egui/src/tests_preview_limit.rs +++ b/crates/ui-egui/src/tests_preview_limit.rs @@ -536,24 +536,64 @@ mod in_the_loupe { assert!(h.app.session.memory_report().full_source.bytes > 0, "the original stayed in the cache while the drag went on"); } - // Given a wipe at 1:1, the Before window is drawn after (over) the low-resolution Before - // stand-in on its side of the line, never under it + /// The meshes the last frame drew with `slot`'s texture: (paint order, clip rect, bounds). + fn drawn(h: &Headless, slot: Slot) -> Vec<(usize, egui::Rect, egui::Rect)> { + let Some(tex) = h.app.renderer.textures.get(&slot) else { return vec![] }; + h.view + .shapes + .iter() + .enumerate() + .filter_map(|(i, c)| match &c.shape { + egui::epaint::Shape::Mesh(m) if m.texture_id == tex.tex.id() => Some((i, c.clip_rect, m.calc_bounds())), + _ => None, + }) + .collect() + } + + // Given a wipe at 1:1, the Before window is painted after (over) the low-resolution Before + // stand-in on its side of the line, never under it, and inside its side of the line #[test] - fn a_wipe_draws_the_before_window_over_its_stand_in() { + fn a_wipe_paints_the_before_window_over_its_stand_in() { for mode in [crate::state::BeforeAfter::Split, crate::state::BeforeAfter::SplitTopBottom] { let (mut h, _) = detail(); h.app.ui.before_after = mode; h.request("engine.execute", json!({"command": "view.zoom100"}), T); h.settle(SETTLE); h.step(); - let order = &h.app.draw_order; - let pos = |what: &str| order.iter().position(|d| *d == what); - let (stand_in, before, after) = (pos("wipe stand-in"), pos("window RegionBefore"), pos("window Region")); - assert!(stand_in.is_some() && before.is_some() && after.is_some(), "{mode:?}: {order:?}"); - assert!(before > stand_in, "{mode:?}: the Before window is under its stand-in: {order:?}"); + let (stand_in, window) = (drawn(&h, Slot::Before), drawn(&h, Slot::RegionBefore)); + assert!(!stand_in.is_empty() && !window.is_empty(), "{mode:?}: the stand-in {stand_in:?} and the window {window:?} are both painted"); + assert!(window[0].0 > stand_in[0].0, "{mode:?}: the Before window is under its stand-in"); + let mid = h.app.image_rect.unwrap().center(); + let clip = window[0].1; + match mode { + crate::state::BeforeAfter::Split => assert!(clip.right() <= mid.x + 0.5, "{clip:?} crosses the line at x = {}", mid.x), + _ => assert!(clip.bottom() <= mid.y + 0.5, "{clip:?} crosses the line at y = {}", mid.y), + } + let after = drawn(&h, Slot::Region); + assert!(!after.is_empty()); + match mode { + crate::state::BeforeAfter::Split => assert!(after[0].1.left() >= mid.x - 0.5), + _ => assert!(after[0].1.top() >= mid.y - 0.5), + } } } + // Given Before and After side by side at 1:1, each window is painted inside its own pane: its + // margin (a window is wider than what its pane shows) does not spill into the neighbour + #[test] + fn side_by_side_windows_stay_in_their_panes() { + let (mut h, _) = detail_in([1800.0, 900.0]); + h.app.ui.before_after = crate::state::BeforeAfter::SideBySide; + h.request("engine.execute", json!({"command": "view.zoom100"}), T); + h.settle(SETTLE); + h.step(); + let canvas = h.app.canvas_rect.unwrap(); + let (before, after) = (drawn(&h, Slot::RegionBefore), drawn(&h, Slot::Region)); + assert!(!before.is_empty() && !after.is_empty()); + assert!(before.iter().all(|(_, clip, _)| clip.right() <= canvas.center().x), "the Before window spills into the After pane: {before:?}"); + assert!(after.iter().all(|(_, clip, _)| clip.left() >= canvas.center().x), "the After window spills into the Before pane: {after:?}"); + } + // Given a Before and an After with the same look (an unedited photo), both windows are kept // apart: the job key is the same, and one tile must not replace the other #[test]