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.
This commit is contained in:
Martino Ferrari committed 2026-09-28 22:36:37 +02:00
1 parent 2fbd1de7d4
commit 1f5ce0d4a5
5 files changed
+123 -12

No files matched your search

+7
View File
@@ -79,6 +79,13 @@ struct Axis {
/// group stops following or the row leaves it. /// group stops following or the row leaves it.
bool live_owner = false; bool live_owner = false;
f64 live_owner_newest = 0.0; 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. /// Ticks for the last `refresh()`, ascending, majors and (when asked) minors.
std::vector<Tick> ticks; std::vector<Tick> ticks;
+13
View File
@@ -329,6 +329,11 @@ class Plot {
/// newest sample* rather than a pair of numbers. With no live edge -- no trace on /// 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 /// 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. /// 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); void set_x_follow(u32 axis, XFollow f);
/// The follow state of one x axis. `Off` for an axis that does not exist. /// The follow state of one x axis. `Off` for an axis that does not exist.
XFollow x_follow(u32 axis) const; 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. /// axis no owner has claimed, and for an axis that does not exist.
bool x_live_owner(u32 axis, f64& newest) const noexcept; 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 /// 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 /// current layout. Always invertible: a collapsed range or a collapsed axes
/// rect is widened by math::Transform, so to_data_*(to_screen_*(v)) == v. /// rect is widened by math::Transform, so to_data_*(to_screen_*(v)) == v.
+29 -12
View File
@@ -10,16 +10,17 @@
namespace plot { namespace plot {
namespace { namespace {
/// The height a row never drops below, in device pixels: the floor that keeps a /// The height a row never drops below, in **logical** pixels -- the same unit as every other
/// plot's chrome from erasing it in a figure with many rows or a small viewport. It /// chrome size, converted to device pixels by the row's own `ui_scale()` where the floor is
/// yields to the figure -- it is capped at an equal share of the height there is -- /// applied. The floor that keeps a plot's chrome from erasing it in a figure with many rows
/// rather than pushing rows past the viewport. /// 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` /// A `Figure` carries no `Style` -- the renderer's is the one its rows are laid out with --
/// -- the renderer's is the one its rows are laid out with -- so there is no logical /// but every row's `Plot` has the `ui_scale` the renderer installed on it, so the conversion
/// unit here to convert from. A host that wants a taller floor gives the figure a /// is the row's own and the guarantee is the same *physical* height at any DPI. A host that
/// taller viewport, or gives the rows weights. /// wants a taller floor gives the figure a taller viewport, or gives the rows weights.
constexpr f32 kMinRowHeightPx = 24.0f; constexpr f32 kMinRowHeightLogical = 24.0f;
} // namespace } // namespace
@@ -72,8 +73,12 @@ u32 Figure::x_link(u32 row) const noexcept {
void Figure::unlink_row(u32 row) noexcept { void Figure::unlink_row(u32 row) noexcept {
// A row that leaves its group stops following anything, so the hint a following group // 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 // 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. // former member anchoring a zoom on an edge it no longer follows. Its linkage flag goes
if (row < plots_.size()) plots_[row].set_x_live_owner(0u, false, 0.0); // 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 (XLink& group : links_) {
for (std::size_t i = 0; i < group.rows.size(); ++i) { for (std::size_t i = 0; i < group.rows.size(); ++i) {
if (group.rows[i] != row) continue; 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<std::size_t>(at - g.rows.begin()); const std::size_t index = static_cast<std::size_t>(at - g.rows.begin());
g.rows.insert(at, row); g.rows.insert(at, row);
g.pushed.insert(g.pushed.begin() + static_cast<std::ptrdiff_t>(index), Range{}); g.pushed.insert(g.pushed.begin() + static_cast<std::ptrdiff_t>(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) { if (!g.has_range) {
// The first row into a group is the one that frames it: there is no shared 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 // 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 // heights still sum to exactly the usable height, so the rows tile the figure and
// the last one ends on its bottom edge. // the last one ends on its bottom edge.
const bool equal = row_weights_.empty(); const bool equal = row_weights_.empty();
const f32 floor_px = std::min(kMinRowHeightPx, usable / static_cast<f32>(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<f32>(rows));
const f32 extra = usable - floor_px * static_cast<f32>(rows); const f32 extra = usable - floor_px * static_cast<f32>(rows);
f32 total_w = 0.0f; f32 total_w = 0.0f;
for (const f32 w : row_weights_) total_w += w; for (const f32 w : row_weights_) total_w += w;
+30
View File
@@ -394,6 +394,13 @@ void Plot::set_y_auto_scale(u32 axis, bool on) {
void Plot::set_x_follow(u32 axis, XFollow f) { void Plot::set_x_follow(u32 axis, XFollow f) {
if (axis >= x_axes_.size()) return; 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; x_axes_[axis].follow = f;
// Arming a live axis places its window at once, so x_range() reports the window // 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. // 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); 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 { XFollow Plot::x_follow(u32 axis) const {
return axis < x_axes_.size() ? x_axes_[axis].follow : XFollow::Off; 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) { void Plot::remove_region(u32 id) {
if (id >= regions_.size()) return; if (id >= regions_.size()) return;
regions_.erase(regions_.begin() + static_cast<std::ptrdiff_t>(id)); regions_.erase(regions_.begin() + static_cast<std::ptrdiff_t>(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 // 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 // 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 // surviving regions' own intervals are untouched -- erase keeps their order -- so
+44
View File
@@ -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<plot::Rect> 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<plot::f64>(rows[2].h), 24.0 * static_cast<plot::f64>(scale), 1e-2);
}
}
// ------------------------------------------------------- y gutter alignment // ------------------------------------------------------- y gutter alignment
/// Spec §16, the invariant: a figure gives its rows one common left gutter, so every /// 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); 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 /// 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' /// 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 -- /// would be, so a streaming multi-row figure has one window and no per-row drift --