From 51bd7eb7c3828a36fe3829e7508efb2d0cd7300a Mon Sep 17 00:00:00 2001 From: JDonaghy Date: Mon, 10 Aug 2026 17:44:30 -0500 Subject: [PATCH] fix(#554): un-invert GTK scroll-wheel direction on the ShellApp path Wheel-down scrolled the editor text up (and vice-versa) since the #540 Relm4->ShellApp migration. Two conventions meet at `ShellApp::handle`'s `UiEvent::Scroll` arm and they disagree: * GDK's `EventControllerScroll` reports positive `dy` = wheel down. * `UiEvent::Scroll.delta` is quadraui's convention, positive y = up. `quadraui::gtk::events::gdk_scroll_to_uievent` is what flips one into the other -- it builds `ScrollDelta::new(dx, -dy)`. Everything downstream of `Msg::MouseScroll` -- the `delta_y > 0.0 => dir = 1` viewport step, the `picker_scroll` sign, `handle_terminal_scroll`'s "> 0 = toward live" policy -- was written against GTK's raw polarity and is byte-identical to its pre-migration form. Pre-migration the Relm4 `connect_scroll` closure fed it GTK's `dy` directly and *separately* pushed the negated `gdk_scroll_to_uievent` form onto the backend event queue. The migration deleted that closure and left the runner's already-negated `UiEvent::Scroll` as the only source, so the second negation vanished and every wheel notch reached the engine with the sign flipped. Fix: negate y back to GTK-raw at that one boundary. Only y -- `dx` is passed through unchanged by the translator, so `delta.x` is already raw. The translator itself is untouched: TUI and macOS share it and were never wrong. Tests (src/gtk/testing.rs, in-crate GtkDriver harness): * New `gdk_wheel_down_scrolls_the_viewport_down_not_up` drives the real `gdk_scroll_to_uievent` rather than a hand-built `UiEvent`, so the whole chain GDK dy -> UiEvent -> Msg -> engine is covered in one test. It asserts both halves: the translator's polarity (dy=+1 -> delta.y=-1) and the resulting engine motion (scroll_top rises, then falls on the opposite notch). Asserting either half alone stays green with the bug. * `wheel_scrolls_the_pane_under_the_pointer_not_the_focused_one` had a `wheel_down_at` helper dispatching `delta.y = +1.0` while asserting scroll_top *increases* -- green only because of this very inversion. Corrected to `-1.0`. Both go red on the pre-fix code; full `cargo test` (GUI feature on, headless) is green. Co-Authored-By: Claude Opus 5 --- src/gtk/mod.rs | 29 +++++++++++++++- src/gtk/testing.rs | 85 +++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 112 insertions(+), 2 deletions(-) diff --git a/src/gtk/mod.rs b/src/gtk/mod.rs index a0da9738..bc842a0c 100644 --- a/src/gtk/mod.rs +++ b/src/gtk/mod.rs @@ -8869,9 +8869,36 @@ impl quadraui::ShellApp for App { // preceding motion event. self.last_editor_pointer .set(Some((position.x as f64, position.y as f64))); + // #554: **negate y back to GTK's raw polarity.** + // + // Two conventions meet at this line and they disagree: + // + // - GDK's `EventControllerScroll` reports *positive dy = wheel + // down*. + // - `UiEvent::Scroll.delta` follows quadraui's convention, + // *positive y = up toward the top of the content*. + // `quadraui::gtk::events::gdk_scroll_to_uievent` is what + // flips one into the other — it constructs + // `ScrollDelta::new(dx, -dy)`. + // + // Everything downstream of `Msg::MouseScroll` — the + // `delta_y > 0.0 => dir = 1` viewport step, the `picker_scroll` + // sign, `Engine::handle_terminal_scroll`'s "> 0 = toward live" + // policy — was written against GTK's raw polarity and is + // unchanged since before the #540 Relm4→ShellApp migration. + // Pre-migration the Relm4 `connect_scroll` closure fed it GTK's + // `dy` directly (`Msg::MouseScroll { delta_x: dx, delta_y: dy }`) + // and *separately* pushed the negated `gdk_scroll_to_uievent` + // form onto the backend event queue. The migration deleted that + // closure and left the runner's already-negated `UiEvent::Scroll` + // as the only source, so every wheel notch reached the engine + // with the sign flipped and the editor scrolled backwards. + // + // Only y is negated: `gdk_scroll_to_uievent` passes `dx` + // through unchanged, so `delta.x` is already GTK-raw. self.dispatch(Msg::MouseScroll { delta_x: delta.x as f64, - delta_y: delta.y as f64, + delta_y: -(delta.y as f64), }); } UiEvent::WindowResized { .. } => { diff --git a/src/gtk/testing.rs b/src/gtk/testing.rs index eaef8730..3fff6c4e 100644 --- a/src/gtk/testing.rs +++ b/src/gtk/testing.rs @@ -259,11 +259,19 @@ mod tests { .window_center(focused) .expect("the focused pane must have been painted"); + // #554: a wheel-**down** notch is `delta.y == -1.0`, not `+1.0`. + // `UiEvent::Scroll.delta` is in quadraui's convention (positive y = up + // toward the top of the content) — see `gdk_scroll_to_uievent`, which + // negates GTK's raw `dy` to produce it. This closure previously + // dispatched `+1.0` and still asserted `scroll_top` *increases*, which + // only held because the GTK `UiEvent::Scroll` arm was passing the + // quadraui-convention delta straight through to `Msg::MouseScroll` + // (which wants GTK-raw polarity) — the very inversion #554 reports. let wheel_down_at = |h: &mut Harness<_>, x: f32, y: f32| { h.driver.dispatch(UiEvent::Scroll { widget: None, position: Point::new(x, y), - delta: ScrollDelta::new(0.0, 1.0), + delta: ScrollDelta::new(0.0, -1.0), }); }; @@ -301,6 +309,81 @@ mod tests { } } + /// #554: scrolling the wheel **down** must move the viewport **down**. + /// + /// Drives the *real* GDK translator (`gdk_scroll_to_uievent`, re-exported + /// by `super::events` from `quadraui::gtk::events`) rather than a + /// hand-built `UiEvent`, so the whole polarity chain is under test in one + /// place: + /// + /// ```text + /// GDK dy ──gdk_scroll_to_uievent──▶ UiEvent::Scroll.delta.y + /// (+ = down) (negates) (+ = up, quadraui convention) + /// ──ShellApp::handle──▶ Msg::MouseScroll.delta_y + /// (negates back) (+ = down, GTK-raw — what every + /// downstream consumer expects) + /// ``` + /// + /// The #540 Relm4→ShellApp migration deleted the `connect_scroll` closure + /// that fed `Msg::MouseScroll` GTK's raw `dy` and left the runner's + /// already-negated `UiEvent::Scroll` as the only source, dropping the + /// second negation. Every wheel notch then reached the engine with the + /// sign flipped: wheel-down scrolled the text up. + /// + /// Both halves matter. Asserting the translator alone would stay green + /// with the bug (`gdk_scroll_to_uievent` was never wrong); asserting the + /// engine alone off a hand-built `UiEvent` would go green again the moment + /// someone "fixed" the inversion by flipping the *translator* and breaking + /// TUI/macOS, which share it. + #[test] + fn gdk_wheel_down_scrolls_the_viewport_down_not_up() { + use crate::gtk::events::gdk_scroll_to_uievent; + + let mut h = harness(engine_with_long_buffer(), 1400, 900); + let win = h.engine.borrow().active_window_id(); + let (x, y) = h + .window_center(win) + .expect("the editor pane must have been painted"); + + // Half 1 — the translation itself. GTK reports positive dy for a + // wheel-down notch; `UiEvent::Scroll` carries the negated value. + let down = gdk_scroll_to_uievent(0.0, 1.0, x as f64, y as f64); + match &down { + UiEvent::Scroll { + delta, position, .. + } => { + assert_eq!( + delta.y, -1.0, + "GDK dy=+1 (wheel down) must translate to delta.y=-1 \ + (quadraui: positive y = up)" + ); + assert_eq!(delta.x, 0.0, "a pure vertical notch must not pan x"); + assert_eq!(*position, Point::new(x, y), "the wheel position is lost"); + } + other => panic!("expected UiEvent::Scroll, got {other:?}"), + } + + // Half 2 — what that event does to the engine, through production + // dispatch. Wheel down ⇒ later lines come into view ⇒ scroll_top rises. + h.driver.dispatch(down); + let after_down = h.engine.borrow().windows[&win].view.scroll_top; + assert!( + after_down > 0, + "wheel down must move the viewport DOWN (scroll_top 0 -> >0), \ + got {after_down} — direction is inverted (#554)" + ); + + // ...and the opposite notch walks it back, so this cannot pass by a + // consumer that ignores the sign entirely. + h.driver + .dispatch(gdk_scroll_to_uievent(0.0, -1.0, x as f64, y as f64)); + let after_up = h.engine.borrow().windows[&win].view.scroll_top; + assert!( + after_up < after_down, + "wheel up must move the viewport back UP ({after_down} -> {after_up})" + ); + } + /// Three tabs in the default **single** editor group — the exact shape /// #553 reports as dead (tab clicks came back to life as soon as a second /// group existed).