From 9aff06acee41baa5d0567c14905798dab661e9f4 Mon Sep 17 00:00:00 2001 From: Martino Ferrari Date: Mon, 28 Sep 2026 22:36:30 +0200 Subject: [PATCH] fix(glfw): route a drag by the row it began in, and drop stale hover state `controller_at()` routed every pointer event by the row the pixel was in, ignoring the row a drag started in, so a press on one figure row followed by a motion and release in another delivered the motion and release to a controller whose `mode_ == None`. The owning controller kept `mode_`, `drag_region_`, `drag_cursor_` and `drag_fixed_x_` set for ever: `publish_hot()` kept publishing the grabbed handle, `grabbed_region()` reported a drag that had ended, and a later gesture re-entering the row resumed the stale edit from the old anchor. `feed_pointer_event()` now owns the routing: a press captures the row it landed in and every motion and the release until the button comes up go to that row (`pointer_target()`), cleared on release and when another figure is set. The GLFW callbacks build the event and call it, so a windowless host or a test can drive the routing too. Two neighbours in the same seam: * `sync_figure_crosshair()` called the four-argument overload unconditionally, so a documented vertical-only crosshair (`horizontal == false`) was promoted to a full cross at the stored y == 0, clamped to the axes edge and made permanent; the change detection also keyed on x alone, missing a y-only move to another row at the same x (two rows showing one horizontal line, spec 17). It now consults `crosshair().horizontal` and compares the hovered row's y. * Only the row under the pointer republished its hot handles, so a row the pointer left kept drawing a hovered marker or bound as if it were still under the pointer, and answered `hovered_region()`/`grabbed_cursor()` for it. The run loop clears every other row's handles, keeping the captured row while a drag owns the button, and reads the frame's one layout once instead of recomputing every row five times. Test: a press on row A's region, a motion into row B, a release in row B asserts no controller keeps a drag mode, nothing reports a grabbed handle, and a later press-and-motion that re-enters row A does not resume the old edit. --- adapters/glfw/glfw_host.cpp | 97 +++++++++++++++++++++++++++++++++---- adapters/glfw/glfw_host.hpp | 44 +++++++++++++++-- tests/test_glfw_host.cpp | 86 ++++++++++++++++++++++++++++++++ 3 files changed, 214 insertions(+), 13 deletions(-) diff --git a/adapters/glfw/glfw_host.cpp b/adapters/glfw/glfw_host.cpp index fcc2a34..001cebf 100644 --- a/adapters/glfw/glfw_host.cpp +++ b/adapters/glfw/glfw_host.cpp @@ -210,12 +210,13 @@ bool Host::run(const RunConfig& cfg, const FrameFn& frame, Status* st) { // that hands each row its rect (and so its own `Plot` its viewport). The single // controller is untouched -- each row has its own, see controller_at(). figure_->set_viewport(viewport); - figure_->layout(); // Every row's controller gets that row's rect, so a gesture -- routed by - // controller_at() or injected by a scripted run -- is hit-tested against the + // pointer_target() or injected by a scripted run -- is hit-tested against the // geometry the last frame drew, and the two readers of a row's geometry cannot - // disagree about it. - const std::vector& rows = figure_->layout().rows; // the one layout of this frame + // disagree about it. The rows come off the one recomputing layout of this frame -- + // calling `layout()` again here, or in `controller_at()`, would lay every row out a + // second and a third time for the same frame. + const std::vector& rows = figure_->layout().rows; for (u32 i = 0; i < row_controllers_.size(); ++i) { if (i < rows.size()) row_controllers_[i]->set_viewport(rows[i]); } @@ -225,6 +226,18 @@ bool Host::run(const RunConfig& cfg, const FrameFn& frame, Status* st) { // signals at the one x the pointer is over. In the run loop rather than in a callback, so a // scripted run that injects its events straight into a row controller is synced too. sync_figure_crosshair(rows); + // Only the row the pointer's events go to ever republishes its own hot handles, so a row + // the pointer left keeps the `HotHandles` it last published and goes on drawing a hovered + // marker or region bound as if the pointer were still on it -- and its `hovered_region()`/ + // `grabbed_cursor()` keep answering for it. Clear every *other* row here, in the run loop, + // where that row is known (the captured row while a drag owns the button, so an in-flight + // gesture keeps its highlight). + if (have_pointer_ && !rows.empty()) { + const u32 active = pointer_row(rows); + for (u32 i = 0; i < rows.size() && i < row_controllers_.size(); ++i) { + if (i != active) figure_->plot(i).set_hot_handles(HotHandles{}); + } + } } else { controller_.set_viewport(viewport); // also Plot::set_viewport, then layout() } @@ -340,7 +353,7 @@ void Host::on_cursor_pos(f64 wx, f64 wy) noexcept { have_pointer_ = true; e.mods = mods_now(window_); e.down = any_button_down(window_); - controller_at(e.x, e.y).on_pointer(e); + feed_pointer_event(e); } void Host::on_mouse_button(int button, int action, int mods) noexcept { @@ -367,7 +380,7 @@ void Host::on_mouse_button(int button, int action, int mods) noexcept { e.pressed = action == GLFW_PRESS; e.released = action == GLFW_RELEASE; e.down = any_button_down(window_); - controller_at(e.x, e.y).on_pointer(e); + feed_pointer_event(e); } void Host::on_scroll(f64 dx, f64 dy) noexcept { @@ -418,8 +431,13 @@ void Host::set_figure(Figure* figure) noexcept { last_row_ = 0u; // A new figure holds no crosshair this host has spread: the comparison in // `sync_figure_crosshair()` starts from "nothing spread", which is the state its rows are in. + // Nor does it hold a captured drag row: the controllers are rebuilt below, so the old + // capture names a controller that no longer exists. spread_active_ = false; spread_x_ = 0.0; + spread_y_ = 0.0; + captured_ = false; + captured_row_ = 0u; if (figure_ == nullptr) return; const u32 rows = figure_->row_count(); row_controllers_.reserve(rows); @@ -449,6 +467,49 @@ input::Controller& Host::controller_at(f32 x, f32 y) noexcept { return controller; } +void Host::feed_pointer_event(const input::PointerEvent& e) noexcept { + if (figure_ == nullptr) { + controller_.on_pointer(e); + return; + } + pointer_target(e.x, e.y, e.pressed, e.released).on_pointer(e); +} + +input::Controller& Host::pointer_target(f32 x, f32 y, bool pressed, bool released) noexcept { + (void)x; // the rows tile the figure vertically, so y alone picks one -- see row_in() + if (figure_ == nullptr || row_controllers_.empty()) return controller_; + const std::vector& rows = figure_->layout().rows; + if (rows.empty() || rows.size() != row_controllers_.size()) return controller_; + + u32 row = 0u; + if (captured_) { + // A button-down gesture belongs to the row it began in for its whole life: a drag + // that crosses into a neighbouring row must not hand its motion -- or its release -- + // to a controller that never saw the press, which would leave the owner's drag open + // for ever (and let a later gesture resume it from the old anchor). + row = captured_row_; + if (released) captured_ = false; + } else { + row = row_in(rows, y); + if (pressed) { + captured_ = true; + captured_row_ = row; + } + } + // The captured row is re-read against this frame's rects: the figure can be resized + // mid-drag, and the guard keeps a stale index from reaching past the controller list. + if (row >= row_controllers_.size()) row = 0u; + last_row_ = row; + input::Controller& controller = *row_controllers_[row]; + controller.set_viewport(rows[row]); + return controller; +} + +u32 Host::pointer_row(const std::vector& rows) noexcept { + if (captured_ && captured_row_ < rows.size()) return captured_row_; + return row_in(rows, last_pointer_y_); +} + u32 Host::row_in(const std::vector& rows, f32 y) noexcept { if (rows.empty()) return 0u; // The row the pixel is in, or -- when it is in none of them, which is the row gap a @@ -485,11 +546,18 @@ void Host::sync_figure_crosshair(const std::vector& rows) noexcept { // keeps it. u32 hovered = row_count; if (have_pointer_) { - hovered = row_in(rows, last_pointer_y_); + // The row the pointer's events currently go to -- the captured row while a drag owns + // the button, the hovered row otherwise -- so the hover follows the gesture it began. + hovered = pointer_row(rows); } else { for (u32 i = 0; i < row_count; ++i) { const Crosshair& candidate = figure_->plot(i).crosshair(); - if (candidate.active != spread_active_ || (candidate.active && candidate.x != spread_x_)) { + // A row differs when its active state, its x, or the y of *its own* horizontal line + // differs from what was spread: a pointer that moves to another row at the same x is + // a new hover and must move the one horizontal line, not leave the old row's behind. + const bool y_differs = candidate.horizontal && candidate.y != spread_y_; + if (candidate.active != spread_active_ || + (candidate.active && (candidate.x != spread_x_ || y_differs))) { hovered = i; break; } @@ -505,8 +573,19 @@ void Host::sync_figure_crosshair(const std::vector& rows) noexcept { // figure's, so the rows cannot be left showing a hover the pointer has abandoned. const Plot& row = figure_->plot(hovered); if (row.crosshair().active) { - figure_->set_crosshair(row.crosshair().x, hovered, row.crosshair().y, true); + if (row.crosshair().horizontal) { + // The row carries its own y (its controller placed both coordinates), so the figure's + // four-argument overload gives the pointer row the one horizontal line. + figure_->set_crosshair(row.crosshair().x, hovered, row.crosshair().y, true); + } else { + // The row's crosshair is vertical-only -- a caller placed it through the + // single-coordinate `Figure::set_crosshair()`, which is documented vertical-only. The + // four-argument overload would turn the horizontal line on at the stored (meaningless) + // y == 0, clamped to the axes edge and made permanent. Promote only the vertical line. + figure_->set_crosshair(row.crosshair().x, true); + } spread_x_ = row.crosshair().x; + spread_y_ = row.crosshair().y; spread_active_ = true; } else { figure_->clear_crosshair(); diff --git a/adapters/glfw/glfw_host.hpp b/adapters/glfw/glfw_host.hpp index 1b1f65b..77f4b00 100644 --- a/adapters/glfw/glfw_host.hpp +++ b/adapters/glfw/glfw_host.hpp @@ -135,11 +135,25 @@ class Host { /// that leaves the data area still ends where it should. /// /// It also hands that row its own viewport, which is what keeps the controller's - /// hit-test geometry in step with the layout between two frames. The event callbacks - /// route through it, and so can a host that wants to know where a synthetic gesture - /// would land. + /// hit-test geometry in step with the layout between two frames. It is the pure + /// query -- what a gesture *would* land on -- and it does not move the capture + /// below; the GLFW callbacks and `feed_pointer_event()` are the routing. input::Controller& controller_at(f32 x, f32 y) noexcept; + /// Feeds one pointer event, stated in framebuffer pixels, through the host's own + /// routing and into the controller it belongs to. The GLFW callbacks fill the + /// position and the button transitions out of GLFW and call exactly this, so a host + /// or a test with no window can drive the routing directly. + /// + /// The routing is the row the event lands in, with one exception that is the whole + /// point of it: a press **captures** the row it landed in, and every motion and the + /// release until that button comes up go to the captured row. A drag that crosses + /// into a neighbouring figure row is therefore still the drag it began as -- the + /// owning controller sees the motion and the release that end it -- rather than + /// being handed to a controller whose `mode_ == None`, which would leave the owner's + /// drag open for ever and let a later gesture resume it from the old anchor. + void feed_pointer_event(const input::PointerEvent& e) noexcept; + /// The gesture controller the host feeds. Valid for the host's whole life. input::Controller& controller() noexcept { return controller_; } @@ -183,6 +197,19 @@ class Host { /// that has just laid the figure out does not make it lay out again for this. static u32 row_in(const std::vector& rows, f32 y) noexcept; + /// The controller a pointer event at (x, y) belongs to, applying the button capture: + /// a press claims the row it landed in (`captured_row_`) and every motion and the + /// release until the button comes up are routed to it; `controller_at()` is the pure + /// query that never captures. Also hands the target its row's viewport, so the + /// hit-test geometry is the last frame's. + input::Controller& pointer_target(f32 x, f32 y, bool pressed, bool released) noexcept; + + /// The row the pointer's events currently go to: the captured row while a drag owns + /// the button, the row under the pointer otherwise. The run loop uses it to clear + /// every *other* row's hot handles, so a row the pointer left cannot keep drawing a + /// hover or a grab it last published. + u32 pointer_row(const std::vector& rows) noexcept; + /// Spreads the hover over a whole `Figure` (spec ยง15). /// /// A figure's rows share one x, so the crosshair one row's controller placed is read back here and @@ -220,10 +247,19 @@ class Host { /// `sync_figure_crosshair()`): what a row is compared against to find the one a /// scripted run touched, since the figure writes one value to every row. /// `spread_active_` false means "spread nothing" -- the state before the first - /// sync, and the one a clear leaves. + /// sync, and the one a clear leaves. `spread_y_` is the horizontal line's own + /// value, compared as well so a y-only change -- the same x in another row -- + /// is not mistaken for "nothing changed". f64 spread_x_ = 0.0; + f64 spread_y_ = 0.0; bool spread_active_ = false; + /// The row a button-down drag has captured: the row its press landed in, which + /// owns its motions and its release until the button comes up. `captured_` is + /// false between gestures, and cleared when another figure is set. + bool captured_ = false; + u32 captured_row_ = 0u; + u32 fb_width_ = 0; ///< framebuffer size of the last refresh_sizes() u32 fb_height_ = 0; /// Framebuffer pixels per window pixel. Taken from the two sizes GLFW reports diff --git a/tests/test_glfw_host.cpp b/tests/test_glfw_host.cpp index ede4089..dde64d5 100644 --- a/tests/test_glfw_host.cpp +++ b/tests/test_glfw_host.cpp @@ -82,6 +82,92 @@ PLOT_TEST(the_host_routes_a_pixel_in_a_row_gap_to_the_nearest_row) { } } +// --- a drag that crosses rows is still one drag ------------------------------ + +namespace { + +/// A two-row figure with a wide region on row 0, handed to a host: the fixture for the +/// drag capture. The region is what a drag edits, so where it ends up -- and whether the +/// owning controller still thinks it is dragging -- says whether the gesture that began on +/// it was the gesture the release ended. +struct DragRouted { + DragRouted() : figure(2u) { + figure.set_viewport({0.0f, 0.0f, 300.0f, 600.0f}); + for (plot::u32 row = 0; row < 2u; ++row) { + plot::Plot& p = figure.plot(row); + p.add_x_axis({"t", {0.0, 1.0}, false, plot::AxisPlace::Bottom, false}); + p.add_y_axis({"v", {-1.0, 1.0}, false, plot::AxisPlace::Left, false}); + } + plot::Region region; + region.x0 = 0.25; + region.x1 = 0.75; + region.snap = plot::RegionSnap::Free; // the bounds are the test's, not a sample's + region_id = figure.plot(0u).add_region(region); + host.set_figure(&figure); + figure.layout(); + } + + plot::Figure figure; + plot::glfw::Host host; + plot::u32 region_id = 0u; +}; + +} // namespace + +/// The finding: `controller_at()` routes *every* pointer event by the row the pixel is in, +/// ignoring the row a drag began in. A press on row 0's region, a motion into row 1 and a +/// release in row 1 therefore left row 0's controller with `MoveRegion`, `drag_region_`, +/// `drag_cursor_` and `drag_fixed_x_` set for ever: it kept publishing the grabbed bound, +/// `grabbed_region()` reported a drag that had ended, and a later gesture re-entering row 0 +/// resumed the stale edit from the old anchor. The routing now captures the press's row for +/// the button's whole life, so the motion and the release reach the controller that owns them. +PLOT_TEST(a_drag_that_crosses_rows_is_ended_by_the_release_in_the_other_row) { + DragRouted r; + const std::vector rows = r.figure.layout().rows; + PLOT_CHECK(rows.size() == 2u); + plot::input::Controller& row0 = *r.host.row_controller(0u); + plot::input::Controller& row1 = *r.host.row_controller(1u); + + // The screen x of data 0.5 on row 0's own x axis, and the middle of each row. + const plot::f32 x = static_cast(r.figure.plot(0u).x_transform(0u).to_screen_x(0.5)); + const plot::f32 y_a = rows[0].y + 0.5f * rows[0].h; + const plot::f32 y_b = rows[1].y + 0.5f * rows[1].h; + + // Press on the region's body in row A. + r.host.feed_pointer_event({x, y_a, 0.0f, 0.0f, plot::input::Button::Left, 0u, true, true, false}); + PLOT_CHECK(row0.drag_mode() == plot::input::DragMode::MoveRegion); + PLOT_CHECK(row0.grabbed_region() == r.region_id); + + // Move into row B with the button still down, then release there. + r.host.feed_pointer_event( + {x + 4.0f, y_b, 4.0f, y_b - y_a, plot::input::Button::Left, 0u, true, false, false}); + r.host.feed_pointer_event( + {x + 4.0f, y_b, 0.0f, 0.0f, plot::input::Button::Left, 0u, false, false, true}); + + // (a) No controller is left holding a drag mode -- the release ended the gesture on the + // controller that began it. + PLOT_CHECK(row0.drag_mode() == plot::input::DragMode::None); + PLOT_CHECK(row1.drag_mode() == plot::input::DragMode::None); + // (b) And nothing reports a grabbed handle any more. + PLOT_CHECK(row0.grabbed_region() == plot::kInvalid); + PLOT_CHECK(row0.grabbed_cursor() == plot::kInvalid); + PLOT_CHECK(row1.grabbed_region() == plot::kInvalid); + + // (d) The region is where the release left it: the press below is in row B, so a motion + // re-entering row A must not edit it again from the old anchor. (Before the fix row A's + // stale `MoveRegion` resumed on exactly this motion and moved the region further.) + const plot::Region after = r.figure.plot(0u).region(r.region_id); + r.host.feed_pointer_event({x, y_b, 0.0f, 0.0f, plot::input::Button::Left, 0u, true, true, false}); + r.host.feed_pointer_event({x + 20.0f, y_a, 20.0f, y_a - y_b, plot::input::Button::Left, 0u, true, + false, false}); + r.host.feed_pointer_event( + {x + 20.0f, y_a, 0.0f, 0.0f, plot::input::Button::Left, 0u, false, false, true}); + const plot::Region now = r.figure.plot(0u).region(r.region_id); + PLOT_CHECK(now.x0 == after.x0); + PLOT_CHECK(now.x1 == after.x1); + PLOT_CHECK(row0.drag_mode() == plot::input::DragMode::None); +} + PLOT_TEST(the_host_keeps_its_own_controller_while_no_figure_is_set) { plot::glfw::Host host; PLOT_CHECK(host.figure() == nullptr);