From 53bf0a40d9a5bbe6c9b8aef9c19454dfbb201bfa Mon Sep 17 00:00:00 2001 From: Trevor Brown Date: Mon, 9 Nov 2026 15:09:23 -0500 Subject: [PATCH] M8: Zoom to selection centres a point above the toolbar and ignores non-finite rectangles; tests pin the margins (#2139 review) fit_selection checks every coordinate is finite (a NaN y or x1 left a NaN centre) and leaves the view as it is for such a rectangle. A point selection is centred in the area above the toolbar at its zoom, not in the whole viewport. Tests pin the exact short-canvas and no-inset positions, the narrow-canvas margins, lines, tiny and huge rectangles, the bottomInset clamp and that it is never saved, the unchanged Zoom to fit centre, the pill strip the app hands the engine (shown and hidden), and the Shift+2 centre without reading the inset back. Co-Authored-By: Claude Sonnet 5.5 Model-Effort: Opus 5.5 triaged and briefed; Sonnet 5.5 verified and wrote at medium --- crates/engine/src/view_behaviour_tests.rs | 88 ++++++++++++++++++++++- crates/engine/src/view_state.rs | 11 ++- crates/ui-egui/src/shortcuts_tests.rs | 19 ++++- 3 files changed, 113 insertions(+), 5 deletions(-) diff --git a/crates/engine/src/view_behaviour_tests.rs b/crates/engine/src/view_behaviour_tests.rs index 9c77314e0..c470afe18 100644 --- a/crates/engine/src/view_behaviour_tests.rs +++ b/crates/engine/src/view_behaviour_tests.rs @@ -806,6 +806,9 @@ fn zoom_to_selection_on_a_wide_layer_is_limited_by_the_width() { fn zoom_to_selection_without_a_toolbar_inset_uses_the_whole_height() { let (v, _) = zoom_on("view.zoomSelection", 0.0, 100.0, 100.0); assert!((v.zoom - (903.0 - 80.0) / 100.0).abs() < 1e-6, "{}", v.zoom); + let (_, top) = v.page_to_screen(0.0, 0.0); + let (_, bottom) = v.page_to_screen(100.0, 100.0); + assert!((top - 40.0).abs() < 1e-6 && (bottom - 863.0).abs() < 1e-6, "top {top}, bottom {bottom}"); } #[test] @@ -822,6 +825,86 @@ fn zoom_to_selection_in_a_short_canvas_still_ends_above_the_toolbar() { let (_, bottom) = v.page_to_screen(100.0, 100.0); assert!(top >= 0.0 && bottom <= 70.0 + 1e-6, "top {top}, bottom {bottom}"); assert!((top - (70.0 - bottom)).abs() < 1e-6, "centred above the toolbar: top {top}, bottom {bottom}"); + // The margin shrinks to a quarter of the 70 points (17.5) and the zoom is the exact fit, not the floor. + assert!((v.zoom - 0.35).abs() < 1e-9, "{}", v.zoom); + assert!((top - 17.5).abs() < 1e-6 && (bottom - 52.5).abs() < 1e-6, "top {top}, bottom {bottom}"); +} + +/// A canvas narrower than 160 points keeps a quarter of its width on each side. +#[test] +fn zoom_to_selection_in_a_narrow_canvas_shrinks_the_side_margins() { + let mut s = Session::new(); + let r = rect(&mut s, 0.0, 0.0, 100.0, 100.0, None); + ex(&mut s, "view.set", json!({"vw": 100, "vh": 903, "bottomInset": 0})); + ex(&mut s, "select.set", json!({"ids": [r]})); + ex(&mut s, "view.zoomSelection", json!({})); + let v = view(&s); + let (left, _) = v.page_to_screen(0.0, 0.0); + let (right, _) = v.page_to_screen(100.0, 100.0); + assert!((v.zoom - 0.5).abs() < 1e-9, "{}", v.zoom); + assert!((left - 25.0).abs() < 1e-6 && (right - 75.0).abs() < 1e-6, "left {left}, right {right}"); +} + +/// A view 1000 by 800 with a 100-point toolbar strip, for the edge cases of the fit. +fn strip_view() -> ViewState { + ViewState { vw: 1000.0, vh: 800.0, bottom_inset: 100.0, ..ViewState::default() } +} + +#[test] +fn fitting_a_selection_survives_points_lines_extremes_and_junk() { + // A point: the zoom stays, and it lands in the middle of the area above the toolbar (y = 350). + let mut v = strip_view(); + v.zoom = 2.0; + v.fit_selection(Rect::new(10.0, 20.0, 10.0, 20.0), 32.0); + assert_eq!(v.zoom, 2.0); + let (x, y) = v.page_to_screen(10.0, 20.0); + assert!((x - 500.0).abs() < 1e-9 && (y - 350.0).abs() < 1e-9, "{x}, {y}"); + // A zero-width line is limited by its height, a zero-height one by its width. + let mut v = strip_view(); + v.fit_selection(Rect::new(0.0, 0.0, 0.0, 100.0), 32.0); + assert!((v.zoom - (700.0 - 80.0) / 100.0).abs() < 1e-9, "{}", v.zoom); + let mut v = strip_view(); + v.fit_selection(Rect::new(0.0, 0.0, 100.0, 0.0), 32.0); + assert!((v.zoom - (1000.0 - 80.0) / 100.0).abs() < 1e-9, "{}", v.zoom); + // A tiny one stops at the maximum zoom, a huge one at the minimum. + let mut v = strip_view(); + v.fit_selection(Rect::new(0.0, 0.0, 0.001, 0.001), 32.0); + assert_eq!(v.zoom, 32.0); + v.fit_selection(Rect::new(0.0, 0.0, 1e12, 1e12), 32.0); + assert_eq!(v.zoom, ViewState::MIN_ZOOM); + // A rectangle with any coordinate that is not a finite number changes nothing. + for bad in [ + Rect::new(0.0, f64::NAN, 10.0, 10.0), + Rect::new(0.0, 0.0, f64::INFINITY, 10.0), + Rect::new(0.0, 0.0, 10.0, f64::NAN), + Rect::new(f64::NAN, 0.0, 10.0, 10.0), + ] { + let mut v = strip_view(); + let before = (v.zoom, v.cx, v.cy); + v.fit_selection(bad, 32.0); + assert_eq!((v.zoom, v.cx, v.cy), before, "{bad:?}"); + } +} + +#[test] +fn the_bottom_inset_is_clamped_and_never_saved() { + let mut s = Session::new(); + ex(&mut s, "view.set", json!({"bottomInset": -5})); + assert_eq!(view(&s).bottom_inset, 0.0); + ex(&mut s, "view.set", json!({"bottomInset": 1e12})); + assert_eq!(view(&s).bottom_inset, 20000.0); + // Larger than the viewport: the fit still leaves a sliver of canvas and a finite centre. + ex(&mut s, "view.set", json!({"vw": 400, "vh": 300, "bottomInset": 5000})); + let r = rect(&mut s, 0.0, 0.0, 10.0, 10.0, None); + ex(&mut s, "select.set", json!({"ids": [r]})); + ex(&mut s, "view.zoomSelection", json!({})); + let v = view(&s); + assert!(v.zoom.is_finite() && v.cx.is_finite() && v.cy.is_finite(), "{v:?}"); + // It is not part of what the view saves. + let saved = serde_json::to_string(&v).unwrap(); + assert!(!saved.to_lowercase().contains("inset"), "{saved}"); + let back: ViewState = serde_json::from_str(&saved).unwrap(); + assert_eq!(back.bottom_inset, 0.0); } #[test] @@ -831,5 +914,8 @@ fn zoom_to_fit_is_unchanged() { rect(&mut s, 0.0, 0.0, 100.0, 100.0, None); ex(&mut s, "view.set", json!({"vw": 1389, "vh": 903, "bottomInset": 58})); ex(&mut s, "view.zoomFit", json!({})); - assert!((view(&s).zoom - (903.0 - 128.0) / 100.0).abs() < 1e-6); + let v = view(&s); + assert!((v.zoom - (903.0 - 128.0) / 100.0).abs() < 1e-6); + // No toolbar offset: the rectangle's own centre stays in the middle of the whole viewport. + assert_eq!((v.cx, v.cy), (50.0, 50.0)); } diff --git a/crates/engine/src/view_state.rs b/crates/engine/src/view_state.rs index 212bb93b4..8a8cc8a01 100644 --- a/crates/engine/src/view_state.rs +++ b/crates/engine/src/view_state.rs @@ -113,13 +113,18 @@ impl ViewState { /// Fit a page rectangle for Zoom to selection: a 40 point margin on all four sides of the area /// above the bottom toolbar (`vw` by `vh - bottom_inset`), centred in that area. pub fn fit_selection(&mut self, r: figcraft_geom::Rect, max_zoom: f64) { - if !(r.width() > 0.0 || r.height() > 0.0) || !r.x0.is_finite() { - self.cx = r.center().x; - self.cy = r.center().y; + // A rectangle that isn't all finite numbers leaves the view as it is. + if ![r.x0, r.y0, r.x1, r.y1].iter().all(|v| v.is_finite()) { return; } let inset = if self.bottom_inset.is_finite() { self.bottom_inset.clamp(0.0, (self.vh - 1.0).max(0.0)) } else { 0.0 }; let area_h = (self.vh - inset).max(1.0); + // A point (nothing to fit) is centred in the area above the toolbar at the zoom it has. + if !(r.width() > 0.0 || r.height() > 0.0) { + self.cx = r.center().x; + self.cy = r.center().y + (self.vh - area_h) / 2.0 / self.zoom.max(1e-6); + return; + } // 40 points as measured; a canvas too small for that keeps a quarter of its size on each // side instead, so the selection still ends above the toolbar. let mx = (self.vw / 4.0).clamp(0.0, 40.0); diff --git a/crates/ui-egui/src/shortcuts_tests.rs b/crates/ui-egui/src/shortcuts_tests.rs index e9c822c2b..fea6f21cd 100644 --- a/crates/ui-egui/src/shortcuts_tests.rs +++ b/crates/ui-egui/src/shortcuts_tests.rs @@ -91,7 +91,11 @@ fn shift_digits_zoom() { assert!(zoom(&u) > fit, "Shift+2 fits the smaller selection"); let v = &u.app.session.ds().unwrap().view; // Centred in the canvas above the bottom toolbar: the page centre sits half the toolbar's strip lower. - let expect_cy = 40.0 + v.bottom_inset / 2.0 / v.zoom; + // The strip is the canvas's bottom edge less the pill's top, worked out here, not read back. + let strip = f64::from(u.app.canvas.rect.max.y - crate::toolbar::pill_rect(u.app.canvas.rect, 0.0).min.y); + assert!(strip > 20.0 && strip < 200.0, "the pill is a toolbar's height: {strip}"); + assert!((v.bottom_inset - strip).abs() < 1e-3, "the app gives the engine the pill's strip: {} vs {strip}", v.bottom_inset); + let expect_cy = 40.0 + strip / 2.0 / v.zoom; assert_eq!(v.cx, 110.0, "centred on it"); assert!((v.cy - expect_cy).abs() < 1e-9, "centred above the toolbar: {} vs {expect_cy}", v.cy); // `+` and `-` step the zoom. @@ -204,3 +208,16 @@ fn shortcuts_panel_does_not_show_on_the_home_screen() { u.frame(vec![]); assert!(!u.app.ui.shortcuts_panel, "the panel closes on the home screen"); } + +/// The strip the bottom toolbar covers is given to the engine while the toolbar shows and is zero +/// when the interface is hidden. +#[test] +fn the_toolbar_strip_is_zero_when_the_interface_is_hidden() { + let (mut u, _, _) = setup(); + u.frame(vec![]); + u.frame(vec![]); + assert!(u.app.session.ds().unwrap().view.bottom_inset > 0.0, "shown"); + u.app.session.ds_mut().unwrap().view.ui_hidden = true; + u.frame(vec![]); + assert_eq!(u.app.session.ds().unwrap().view.bottom_inset, 0.0, "hidden"); +}