fix: a wire is picked by distance and a refusal is spoken, so the editor answers
A review of the whole node editor. The hit test measured the distance to the 24 curve samples rather than to the segments between them, so a wire spanning several rows was unpickable between two samples -- the probe now places its boxes 280 units apart and prints the nearest sample's own 8.4 against a 3.0 tolerance, which the point test would have missed. A refused connection said nothing: the message is now carried in `MathDialog::refusal` and shown once by the single writer of the label, and a self-wire is refused through the real drag-end path instead of only through `math_connect()`. Two defects were found only by the new assertions. `math_dialog_sync()` re-entered itself -- `gtk_drop_down_set_selected()` emits `notify::selected` for a programmatic set too, and the handler answers by syncing again, which consumed the refusal before it could be drawn -- so the body is now guarded one level deep. And a deleted node left `selected_wire` naming an index that afterwards belongs to a different wire, so `Delete` cleared an input on the wrong node. A node is placed in the first free grid cell rather than at the end of the list's own cell, so a node added after a delete no longer lands on top of a survivor; the label ellipsizes with a tooltip rather than wrapping, because a wrapped line makes the dialog's height depend on the text in it.
This commit is contained in:
1 parent
300b33159e
commit
f238e717b2
5 files changed
+318
-57
No files matched your search
@@ -23,9 +23,9 @@ Part of [CLAUDE.md](../../CLAUDE.md). These are rules that cost a defect to lear
|
||||
The editor is the *compact graph*: nodes as boxes on a `GtkDrawingArea`, click to select (its
|
||||
parameters in the form beside it), **drag from an output port to an input port to wire** (the old
|
||||
two-click protocol is gone), drag a box to move it; **wheel zooms about the pointer, background
|
||||
drag pans, `f` fits, `Ctrl+0` resets**. Its widgets are created once and only shown, hidden and set by `math_dialog_sync()`,
|
||||
so no rebuild can invalidate a live callback handle -- the same shape the measurements dialog
|
||||
uses. `UDPSCOPE_PROBE_MATH_MS` defines two graphs and opens the editor (a construction crash test
|
||||
drag pans, `f` fits, `Ctrl+0` resets**.
|
||||
Its widgets are created once and only shown, hidden and set by `math_dialog_sync()`, so no rebuild
|
||||
can invalidate a live callback handle -- the same shape the measurements dialog uses. `UDPSCOPE_PROBE_MATH_MS` defines two graphs and opens the editor (a construction crash test
|
||||
as much as a state check), and the dump prints each derived channel's extremes beside the
|
||||
source's. Read it on the fast mock, where the two assertions that matter are **`gaps=0`** (the
|
||||
math is on the producer thread, so what it can break is packet capture, never the frame rate)
|
||||
@@ -59,11 +59,30 @@ Part of [CLAUDE.md](../../CLAUDE.md). These are rules that cost a defect to lear
|
||||
* `[math-probe] wire: connect mode=2 in_a=0 (want 2 0)`,
|
||||
`[math-probe] detach: emptied_at_begin=-1 in_a=-1 (want -1 -1)`,
|
||||
`[math-probe] detach: escape restores in_a=0 (want 0)`.
|
||||
* `[math-probe] wire: pick node=1 which=0 -> in_a=-1 (want 1 0 -1)`.
|
||||
* `[math-probe] wire: pick node=1 which=0 -> in_a=-1 off_sample=8.4 tol=3.0 (want 1 0 -1,
|
||||
off_sample > tol)` — the pick is taken at `t = 0.52`, **between** samples 12 and 13 of the 24 the
|
||||
curve is walked at, on a wire whose two boxes are several rows apart. `off_sample` is the measured
|
||||
distance from the pick to the nearest sample point and `tol` the pick radius in the same units:
|
||||
with `off_sample` the larger, a hit test that measured to the sample *points* rather than to the
|
||||
*segments* between them would miss. Taken at `t = 0.5` — exactly sample 12 — the line passed for
|
||||
both implementations and asserted nothing, the same mistake as a hit test taken at the identity
|
||||
view.
|
||||
* `[math-probe] wire: backwards ok=1 order=[0:-1,1:0,2:1] output=2 valid=1 why=[] (want 1 ... 1 [])` —
|
||||
the re-sort. `valid=1` is the assertion; the `order=` string is the reading that explains it.
|
||||
* `[math-probe] wire: self refused=1 why=[...]` and
|
||||
`[math-probe] wire: cycle refused=1 why=[...]` — and **nothing is written** on either.
|
||||
* `[math-probe] wire: self refused=1 why=[a node cannot read itself]` and
|
||||
`[math-probe] wire: cycle refused=1 why=[that wire would make a loop]` — and **nothing is
|
||||
written** on either. Both call `math_connect()` directly, so both assert its out-parameter and
|
||||
neither says anything about the UI.
|
||||
* `[math-probe] wire: self through drag-end node=1 problem=[a node cannot read itself] (want the
|
||||
refusal text, not empty)` — the other half, and the one that matters: the refusal has to reach
|
||||
the **validation line**. The UI path used to screen the self case out before `math_connect()` and
|
||||
then drop its `why` on the floor, so the user drew a wire, nothing appeared and nothing was said
|
||||
while the two lines above passed. No synthetic GTK gesture is reachable headlessly, so the probe
|
||||
sets `drag_mode`/`wire_from`/`press_cx`/`press_cy` and calls `math_canvas_drag_end()` — which is
|
||||
still the handler under test. An empty `problem=[]` here is also how the sync **re-entrancy** was
|
||||
found: setting the op dropdown emits `notify::selected`, whose handler syncs, so the form's own
|
||||
write re-entered `math_dialog_sync()` and the inner call rewrote the label the outer had just
|
||||
written. `MathDialog::syncing` guards it.
|
||||
* `[math-probe] validate: broken problem=[..] label=[..] apply=0; fixed problem=[] apply=1 (want non-empty 0; empty 1)`,
|
||||
`[math-probe] validate: no-selection label=[..] (want non-empty)` — the label is written
|
||||
**above** `math_dialog_sync()`'s early return; a stale message when there is no selection to
|
||||
@@ -72,7 +91,14 @@ Part of [CLAUDE.md](../../CLAUDE.md). These are rules that cost a defect to lear
|
||||
dropdown lists exactly the acquired channels, not all signals.
|
||||
* `[math-probe] op pick: nodes 3->4 op_index=7 q_row=1 (want 4 7 1)` and
|
||||
`[math-probe] op pick: index round trip=1 (want 1)`.
|
||||
* `[math-probe] tidy: xs=[24,204,384,564] overlaps=0 (want 0)`.
|
||||
* `[math-probe] add node: after a delete cells=[(24,20) (24,78) (184,78) (184,20)] overlaps=0
|
||||
(want 0)` — the boxes are put back on the grid, the middle one deleted and one added. The new box
|
||||
takes cell 1, `(184,20)`, which the deletion freed; placing by `nodes.size()` would have put it on
|
||||
`(184,78)`, which is occupied, and an overlapped box is an unclickable one. `overlaps` is the
|
||||
assertion, `cells` the reading that explains it.
|
||||
* `[math-probe] tidy: xs=[24,24,204,384] overlaps=0 (want 0)` — four nodes in three depth columns
|
||||
(the delete above left two roots), so two share the first column and the x values repeat. Only
|
||||
`overlaps=0` is the assertion.
|
||||
Four rules here were wrong first and are worth keeping:
|
||||
* **A `Source` may only name an *acquired* channel** -- `ScopeModel::channels()`, which is what the
|
||||
dialog's dropdown lists, **and both places that create a node must take one from there**.
|
||||
@@ -103,10 +129,15 @@ Part of [CLAUDE.md](../../CLAUDE.md). These are rules that cost a defect to lear
|
||||
* **Boxes must not overlap on the canvas.** The first auto-place staggered nodes by 24 px for a
|
||||
132 px box, so every box sat on the one before it, `math_node_at()`'s topmost-first search always
|
||||
returned the last, and a drag or a click landed on the wrong node -- which reads as the dialog
|
||||
ignoring the pointer. The **add-node grid** places in two columns spaced 160 px apart, rows 58;
|
||||
the **tidy layout** uses a column pitch of `kNodeW + 48` (narrower), which is enough clearance
|
||||
for a wire's 40-unit horizontal tread — at 160 the bezier's control points cross and the wire
|
||||
doubles back on itself. The probe prints the box the drag *began over* for exactly this reason.
|
||||
ignoring the pointer. The **add-node grid** places in two columns spaced 160 px apart, rows 58,
|
||||
and `math_add_node()` takes the **first free cell** rather than cell `nodes.size()`: placing by
|
||||
the count reuses an occupied cell as soon as anything has been deleted — delete the middle of
|
||||
three and add one and the new box lands exactly on an existing one. The free-cell search uses the
|
||||
same overlap test the tidy probe asserts with, and falls through to cell `nodes.size()` after 64
|
||||
cells rather than searching for ever. The **tidy layout** uses a column pitch of `kNodeW + 48` =
|
||||
180, which is *wider* than that 160 and is what gives a wire's 40-unit horizontal tread its
|
||||
clearance — at 160 the bezier's control points cross and the wire doubles back on itself. The
|
||||
probe prints the box the drag *began over* for exactly this reason.
|
||||
Offering the whole signal list let a graph name something nobody is acquiring (no data for ever)
|
||||
and put the stream's timestamp signal, which has no samples, in it.
|
||||
* **`prepare_math_locked()` runs from `rebuild_locked()`, so *every* plan change re-prepares the
|
||||
@@ -158,6 +189,17 @@ Part of [CLAUDE.md](../../CLAUDE.md). These are rules that cost a defect to lear
|
||||
* **The validation line is written above `math_dialog_sync()`'s early return.** An out-of-range
|
||||
selection is exactly the state in which a stale message misleads most: the graph can still be
|
||||
broken and there is no form to read instead.
|
||||
* **The line is written from `math_dialog_sync()` and from nowhere else, it ellipsizes, and its
|
||||
tooltip carries the full text.** Three consequences. (1) A *refusal* — "a node cannot read
|
||||
itself", a cycle — is not a state `math_dialog_problem()` can describe, since it is a thing that
|
||||
did not happen, so `drag-end` leaves it on `MathDialog::refusal`, which the sync shows in
|
||||
preference to the graph's own problem and **consumes as it writes**: the message lasts until the
|
||||
next sync, and every mutation syncs. `Apply`'s sensitivity still follows the graph alone, because
|
||||
a refused wire wrote nothing. (2) The label **ellipsizes rather than wraps**, and the tooltip is
|
||||
set beside the text so the two cannot disagree — layout must not depend on content, and this
|
||||
label sits in a dialog whose canvas is `hexpand`. (3) The sync is **not re-entrant**
|
||||
(`MathDialog::syncing`): writing the op dropdown emits `notify::selected`, whose handler syncs,
|
||||
and the inner call rewrote the label the outer one had just written.
|
||||
* **`math_op_index()`, never a cast.** `math_op_at()`'s order is the dialog's — what a dropdown
|
||||
lists and what a session file stores — and `static_cast<guint>(op)` worked only because the
|
||||
two happen to agree today. The session-file rule in this file already says an op is written as
|
||||
|
||||
@@ -17,10 +17,14 @@ Two baselines every run is read against. A clean build emits **four** warnings a
|
||||
`-Wmissing-field-initializers` on `MathNode::source` and `-Woverlength-strings` on **both**
|
||||
generated GResource `.c` files — the help pages' and the icons', whose blob crossed C99's
|
||||
4095-byte string-literal limit when the two app icons joined the four action ones (a note about a
|
||||
generated file, which GCC itself supports). The fifth warning that used to appear here was an
|
||||
unused `output_built` in `on_math_apply()` — Task 6's live validation check reads that value, so
|
||||
the variable is no longer unused. And an exit-dump run emits **no** `Gtk-CRITICAL` at all. A
|
||||
fifth warning, or any critical, is a finding.
|
||||
generated file, which GCC itself supports). The fifth warning that used to appear here was an unused
|
||||
`output_built` in `math_probe()`; it was **deleted** as dead code — redundant with the
|
||||
`output_after_add` already printed on the same line — and that deletion is what dropped the baseline
|
||||
from five to four. Nothing started reading it. And an exit-dump run emits **no** `Gtk-CRITICAL` at
|
||||
all. A fifth warning, or any critical, is a finding.
|
||||
|
||||
An *incremental* build reads **two** of those four: the generated GResource `.c` files are not
|
||||
recompiled when only `src/` changed, so their `-Woverlength-strings` do not reappear.
|
||||
|
||||
That second baseline used to be "exactly two, `gtk_widget_get_height` and `gtk_widget_get_width`,
|
||||
from the dump asking a destroyed widget for its size", and tolerating them was a mistake: the dump
|
||||
|
||||
Reference in new issue
Block a user