From 1f5ce0d4a59f0fd91c180535cf890f67afee2ec1 Mon Sep 17 00:00:00 2001 From: Martino Ferrari Date: Mon, 28 Sep 2026 22:36:37 +0200 Subject: [PATCH] fix(plot,figure): re-index hot region, logical row floor, refuse live on a linked axis Three smaller findings at the plot/figure seam: * `Plot::remove_region()` re-indexed `region_stats_` but left `hot_.region` naming the old index, so a frame after a removal highlighted the wrong region's bound and an open drag edited a different region. The published handle (and its zone) is now cleared or decremented with the cache, and the contract documents that a host must not remove a region mid-drag. * `Figure`'s `kMinRowHeightPx` was applied as a *device*-pixel floor while chrome is logical, so at 2x the guarantee was half as strong exactly where the chrome is twice as large. It is now a logical constant converted through the row's own `ui_scale()`, 1:1 for a non-positive scale as `Style::device()` does. * `Figure::link_x()` refused a member whose axis 0 was `XFollow::Live`, but nothing refused `set_x_follow(0u, XFollow::Live)` *after* linking. The group then wrote its range into an axis that reinterpreted it against its own live edge, read it back as an external change, and tracked one member instead of the union. `Plot` now carries a linkage flag (`set_x_link_member()`), set by `link_x()` and cleared by `unlink_row()`, and `set_x_follow()` refuses `Live` on a linked axis. Tests: arming `Live` on a linked member leaves it `Off` and unlinking restores the ability to arm it; the row-height floor is 24 and 48 device pixels at 1x and 2x respectively. --- include/plot/plot/axis.hpp | 7 ++++++ include/plot/plot/plot.hpp | 13 +++++++++++ src/plot/figure.cpp | 41 ++++++++++++++++++++++++----------- src/plot/plot.cpp | 30 ++++++++++++++++++++++++++ tests/test_figure.cpp | 44 ++++++++++++++++++++++++++++++++++++++ 5 files changed, 123 insertions(+), 12 deletions(-) diff --git a/include/plot/plot/axis.hpp b/include/plot/plot/axis.hpp index 0325a7b..29e38e0 100644 --- a/include/plot/plot/axis.hpp +++ b/include/plot/plot/axis.hpp @@ -79,6 +79,13 @@ struct Axis { /// group stops following or the row leaves it. bool live_owner = false; f64 live_owner_newest = 0.0; + /// Set while this plot is a member of a `Figure` x link group (`Figure::link_x()` + /// writes it, `unlink_row()` clears it). The group owns the shared x window *and* + /// its follow state -- `Figure::link_x()` refuses a `Live` member joiner -- so + /// `Plot::set_x_follow(Live)` on this axis is refused after linking too, rather + /// than letting a later call put the member's axis back into a state the group + /// cannot drive (see `Plot::set_x_follow()`). + bool link_member = false; /// Ticks for the last `refresh()`, ascending, majors and (when asked) minors. std::vector ticks; diff --git a/include/plot/plot/plot.hpp b/include/plot/plot/plot.hpp index 0427989..be5f3bb 100644 --- a/include/plot/plot/plot.hpp +++ b/include/plot/plot/plot.hpp @@ -329,6 +329,11 @@ class Plot { /// newest sample* rather than a pair of numbers. With no live edge -- no trace on /// the axis, or none of them streaming -- nothing is recomputed and the axis /// keeps its stored range, `auto_scale` included, exactly as an `Off` axis does. + /// + /// On an axis that is a member of a `Figure` x link group, arming `Live` is + /// **refused** (the state is left as it was): the group owns the shared window and + /// its follow state, exactly as `Figure::link_x()` refuses a `Live` member joiner. + /// See `set_x_link_member()`. void set_x_follow(u32 axis, XFollow f); /// The follow state of one x axis. `Off` for an axis that does not exist. XFollow x_follow(u32 axis) const; @@ -363,6 +368,14 @@ class Plot { /// axis no owner has claimed, and for an axis that does not exist. bool x_live_owner(u32 axis, f64& newest) const noexcept; + /// Publishes -- or clears -- that this axis is a member of a `Figure` x link group. + /// `Figure::link_x()` sets it when a row joins and `unlink_row()` clears it when one + /// leaves; it is what makes `set_x_follow(Live)` refused on a linked axis, because + /// the group owns the shared window and the follow state. A no-op for an axis that + /// does not exist. Not for callers outside the figure: a `Plot` that is not in a + /// figure never has a linked axis. + void set_x_link_member(u32 axis, bool linked) noexcept; + /// Data -> pixels for one axis, built from the axis' range, log flag and the /// current layout. Always invertible: a collapsed range or a collapsed axes /// rect is widened by math::Transform, so to_data_*(to_screen_*(v)) == v. diff --git a/src/plot/figure.cpp b/src/plot/figure.cpp index 2134c8a..0faec06 100644 --- a/src/plot/figure.cpp +++ b/src/plot/figure.cpp @@ -10,16 +10,17 @@ namespace plot { namespace { -/// The height a row never drops below, in device pixels: the floor that keeps a -/// plot's chrome from erasing it in a figure with many rows or a small viewport. It -/// yields to the figure -- it is capped at an equal share of the height there is -- -/// rather than pushing rows past the viewport. +/// The height a row never drops below, in **logical** pixels -- the same unit as every other +/// chrome size, converted to device pixels by the row's own `ui_scale()` where the floor is +/// applied. The floor that keeps a plot's chrome from erasing it in a figure with many rows +/// or a small viewport. It yields to the figure -- it is capped at an equal share of the +/// height there is -- rather than pushing rows past the viewport. /// -/// Device pixels, like every rect a figure computes: a `Figure` carries no `Style` -/// -- the renderer's is the one its rows are laid out with -- so there is no logical -/// unit here to convert from. A host that wants a taller floor gives the figure a -/// taller viewport, or gives the rows weights. -constexpr f32 kMinRowHeightPx = 24.0f; +/// A `Figure` carries no `Style` -- the renderer's is the one its rows are laid out with -- +/// but every row's `Plot` has the `ui_scale` the renderer installed on it, so the conversion +/// is the row's own and the guarantee is the same *physical* height at any DPI. A host that +/// wants a taller floor gives the figure a taller viewport, or gives the rows weights. +constexpr f32 kMinRowHeightLogical = 24.0f; } // namespace @@ -72,8 +73,12 @@ u32 Figure::x_link(u32 row) const noexcept { void Figure::unlink_row(u32 row) noexcept { // A row that leaves its group stops following anything, so the hint a following group // published to it goes with the membership: leaving `has_live` set behind would leave a - // former member anchoring a zoom on an edge it no longer follows. - if (row < plots_.size()) plots_[row].set_x_live_owner(0u, false, 0.0); + // former member anchoring a zoom on an edge it no longer follows. Its linkage flag goes + // with it too: an unlinked row is an ordinary plot again, free to arm `Live` itself. + if (row < plots_.size()) { + plots_[row].set_x_live_owner(0u, false, 0.0); + plots_[row].set_x_link_member(0u, false); + } for (XLink& group : links_) { for (std::size_t i = 0; i < group.rows.size(); ++i) { if (group.rows[i] != row) continue; @@ -114,6 +119,10 @@ Status Figure::link_x(u32 row, u32 group) { const std::size_t index = static_cast(at - g.rows.begin()); g.rows.insert(at, row); g.pushed.insert(g.pushed.begin() + static_cast(index), Range{}); + // A member's axis 0 is now owned by the group (its window and its follow state), so + // `set_x_follow(Live)` on it is refused from here on -- the same rule the refusal above + // applies at the door. + p.set_x_link_member(0u, true); if (!g.has_range) { // The first row into a group is the one that frames it: there is no shared range // to apply yet, and adopting this row's is what makes the next joiner land on a @@ -356,7 +365,15 @@ const FigureLayout& Figure::layout() { // heights still sum to exactly the usable height, so the rows tile the figure and // the last one ends on its bottom edge. const bool equal = row_weights_.empty(); - const f32 floor_px = std::min(kMinRowHeightPx, usable / static_cast(rows)); + // The floor is a logical size like every other chrome metric, so it is converted through + // the row's own `ui_scale()` -- the one the renderer installed on every row -- which is + // what makes the guarantee the same *physical* height at any DPI instead of half as strong + // at 2x exactly where the chrome it protects is twice as large. A scale that is not + // positive is not a scale and converts 1:1, exactly as `Style::device()` documents. + const f32 floor_scale = plots_[0].ui_scale(); + const f32 floor_device = floor_scale > 0.0f ? kMinRowHeightLogical * floor_scale + : kMinRowHeightLogical; + const f32 floor_px = std::min(floor_device, usable / static_cast(rows)); const f32 extra = usable - floor_px * static_cast(rows); f32 total_w = 0.0f; for (const f32 w : row_weights_) total_w += w; diff --git a/src/plot/plot.cpp b/src/plot/plot.cpp index 0f2026b..2340682 100644 --- a/src/plot/plot.cpp +++ b/src/plot/plot.cpp @@ -394,6 +394,13 @@ void Plot::set_y_auto_scale(u32 axis, bool on) { void Plot::set_x_follow(u32 axis, XFollow f) { if (axis >= x_axes_.size()) return; + // A figure link group owns this axis' shared window *and* its follow state: + // `Figure::link_x()` refuses a `Live` member joiner, and arming `Live` here *after* + // linking would be the same state reached by the back door -- `push_link()` would + // write the group's range into an axis that reinterprets it against its own live + // edge, `adopt_member_changes()` would read it straight back as an external change, + // and the group would track one member instead of the union of them. Refused. + if (f == XFollow::Live && x_axes_[axis].link_member) return; x_axes_[axis].follow = f; // Arming a live axis places its window at once, so x_range() reports the window // the next layout produces rather than the one the axis held while it was Off. @@ -402,6 +409,11 @@ void Plot::set_x_follow(u32 axis, XFollow f) { place_live_x_window(axis); } +void Plot::set_x_link_member(u32 axis, bool linked) noexcept { + if (axis >= x_axes_.size()) return; + x_axes_[axis].link_member = linked; +} + XFollow Plot::x_follow(u32 axis) const { return axis < x_axes_.size() ? x_axes_[axis].follow : XFollow::Off; } @@ -706,6 +718,24 @@ void Plot::snap_region(u32 id) { void Plot::remove_region(u32 id) { if (id >= regions_.size()) return; regions_.erase(regions_.begin() + static_cast(id)); + // The published hot handles name a region by index and are re-indexed with everything + // else: the removed region's own handle is cleared (nothing is hot there any more, and a + // frame drawn before the next event would otherwise highlight the region that slid into + // its slot), and a handle naming a region above the hole follows it down. The `zone` is + // the grab that went with that index, so it is cleared with it -- a stale zone describing + // a region that is no longer there would colour the next region's bound. + // + // A host must not remove a region *mid-drag*: the controller holds the id it grabbed, and + // this setter cannot re-index a controller's private `drag_region_`. The gesture's release + // would then edit whichever region now carries that id. Remove regions between gestures. + if (hot_.region != kInvalid) { + if (hot_.region == id) { + hot_.region = kInvalid; + hot_.zone = RegionGrab::None; + } else if (hot_.region > id) { + hot_.region -= 1u; + } + } // The cache is keyed by region index, so the removed region's entries name nothing // and every entry above the hole names a region one slot further down. The // surviving regions' own intervals are untouched -- erase keeps their order -- so diff --git a/tests/test_figure.cpp b/tests/test_figure.cpp index 04e45c1..2cab564 100644 --- a/tests/test_figure.cpp +++ b/tests/test_figure.cpp @@ -294,6 +294,26 @@ PLOT_TEST(every_row_keeps_a_positive_height) { } } +/// The row-height floor is chrome-sized, so it must be *logical*: it is applied in +/// device pixels, but the 24 it was written as is a logical size like every other chrome +/// metric, and at 2x a device-pixel floor is half the physical height -- exactly where the +/// chrome it protects is twice as large. It is converted through the row's own +/// `ui_scale()`, so the guarantee is the same physical height at any DPI. +PLOT_TEST(the_row_height_floor_is_logical_and_converts_with_the_display) { + const float weights[] = {1.0f, 1.0f, 1e-30f}; + for (const plot::f32 scale : {1.0f, 2.0f}) { + plot::Figure figure(3u); + figure.set_viewport({0.0f, 0.0f, 400.0f, 600.0f}); + for (plot::u32 row = 0; row < 3u; ++row) figure.plot(row).set_ui_scale(scale); + PLOT_CHECK(figure.set_row_weights(weights)); + const std::vector rows = figure.layout().rows; + PLOT_CHECK(rows.size() == 3u); + // The underflowing-weight row gets the floor and nothing else: the leftover after + // the floor is shared by weight, and 1e-30 of it is zero in float. + PLOT_CHECK_NEAR(static_cast(rows[2].h), 24.0 * static_cast(scale), 1e-2); + } +} + // ------------------------------------------------------- y gutter alignment /// Spec ยง16, the invariant: a figure gives its rows one common left gutter, so every @@ -641,6 +661,30 @@ PLOT_TEST(linking_a_live_axis_is_refused_with_a_status) { PLOT_CHECK_NEAR(figure.plot(0u).x_range(0u).max, 4.0, 1e-12); } +/// The post-link hole the door refusal cannot close: `set_x_follow(Live)` on a member +/// *after* it linked. The group owns the shared window and the follow state, so arming +/// the member's axis would make `push_link()` write the group's range into an axis that +/// reinterprets it against its own live edge, and `adopt_member_changes()` read it back +/// as an external change -- the group then tracking one member instead of the union. The +/// arm is refused, and taking the row out of the group restores the ability to arm it. +PLOT_TEST(arming_live_on_a_linked_member_is_refused) { + plot::Figure figure = figure_with_axes(2u); + const plot::u32 group = figure.add_x_link(); + PLOT_CHECK(figure.link_x(0u, group)); + PLOT_CHECK(figure.link_x(1u, group)); + + figure.plot(0u).set_x_follow(0u, plot::XFollow::Live); + PLOT_CHECK(figure.plot(0u).x_follow(0u) == plot::XFollow::Off); // refused, not armed + PLOT_CHECK(figure.plot(1u).x_follow(0u) == plot::XFollow::Off); + + // Arming `Off` is not refused, and leaving the group frees the axis again: the + // refusal is about the membership, not about the call. + figure.plot(0u).set_x_follow(0u, plot::XFollow::Off); + PLOT_CHECK(figure.link_x(0u, 0u)); + figure.plot(0u).set_x_follow(0u, plot::XFollow::Live); + PLOT_CHECK(figure.plot(0u).x_follow(0u) == plot::XFollow::Live); +} + /// A following group advances every row together: the window is derived once per /// frame from the newest x across the members' traces, exactly as one `Live` axis' /// would be, so a streaming multi-row figure has one window and no per-row drift --