docs/REVIEW-2026-09-06.md: fix all ten review findings; RUST.md/IRIS_TODO.md: DragGesture merge checks
Finding 1 (the real crash): Selection::clear() drops rows and anchor, called from TranscriptScreen::apply's Rebuild arm right before List::clear() -- push_row re-registers survivors as it rebuilds each row. Fixes a WeakWidget outliving the row group_tool_runs regrouped away, which panicked the next long-press anywhere. New apply_tests test builds a real TranscriptScreen, forces the regroup, and confirms no panic. Findings 2-5: debug_assert!s on List::place's slot, List::fling and FlingCalculator's velocity finiteness, VelocityTracker::add_sample's chronological order, and FrameReport::mark_phase's non-decreasing start_index. Finding 7: bench_client.rs's battery_line guard restructured so the empty check can't be separated from its unwraps by a future edit. Findings 9/10: new List tests pinning tick_fling's per-tick deceleration and replace_back's evicted-key cleanup with a different key than the existing tests use. IRIS.md's replace_back/clear/apply entry gained the side-table-clearing note the Docs finding asked for. Also records this pass's DragGesture-merge verification in RUST.md (tap stays vs swipe doesn't, a real fling keeps moving after release, keyboard cycles confirmed via on_insets_changed) and annotates the two IRIS_TODO.md phone-report items it targets. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This commit is contained in:
1 parent
2e00e71552
commit
1f379e8384
10 files changed
+679
-38
No files matched your search
+8
-1
@@ -552,7 +552,14 @@ streamed event" cost RUST.md's P0 box measured (20 events/second against a
|
|||||||
new rows appended after it. A row changing *before* the tail (only
|
new rows appended after it. A row changing *before* the tail (only
|
||||||
`group_tool_runs` retroactively grouping tool calls into a run does
|
`group_tool_runs` retroactively grouping tool calls into a run does
|
||||||
this) falls back to `List::clear` plus a full rebuild, counted in
|
this) falls back to `List::clear` plus a full rebuild, counted in
|
||||||
`TranscriptScreen::take_rebuilds()`. `bench_client.rs`, `transcript_client.rs`
|
`TranscriptScreen::take_rebuilds()`. **A caller that keeps its own
|
||||||
|
row-keyed side table alongside `List` (`Selection`'s `rows:
|
||||||
|
BTreeMap<RowKey, WeakWidget<TextEdit>>` is the one this crate has) must
|
||||||
|
clear it in step with `List::clear()`** — the fallback drops every row
|
||||||
|
`List` was holding, so any side table not cleared the same way is left
|
||||||
|
pointing at widgets the clear just freed (docs/REVIEW-2026-09-06.md
|
||||||
|
finding 1, fixed 2026-09-06 by `Selection::clear()`, called from
|
||||||
|
`apply`'s `Rebuild` arm right before `List::clear()`). `bench_client.rs`, `transcript_client.rs`
|
||||||
and `desktop-app/app.rs` all call this now instead of rebuilding on every
|
and `desktop-app/app.rs` all call this now instead of rebuilding on every
|
||||||
event; only the opening page (and `apply`'s own fallback) still calls
|
event; only the opening page (and `apply`'s own fallback) still calls
|
||||||
`build_tree`.
|
`build_tree`.
|
||||||
|
|||||||
+34
-11
@@ -203,17 +203,40 @@ agent ticks it here with the evidence.
|
|||||||
before the first `on_insets_changed`. Reproduce with the phone's
|
before the first `on_insets_changed`. Reproduce with the phone's
|
||||||
screen size and density on the emulator before guessing.
|
screen size and density on the emulator before guessing.
|
||||||
- [ ] **"Swiping still gets caught by the grey bar but keeps working
|
- [ ] **"Swiping still gets caught by the grey bar but keeps working
|
||||||
after I go past it."** A pan that starts on the composer is held by
|
after I go past it."** Not closeable from the emulator, annotated
|
||||||
the composer until the finger leaves its region, then the list takes
|
2026-09-06 after the `DragGesture` merge. `attr.rs`'s `on_press` never
|
||||||
over. The tap-vs-swipe fix in `attr.rs` stops the *focus*, but the
|
calls `capture_pointer` and never consumes a `Pressing` frame past
|
||||||
press frames are still being handled by the field rather than passed
|
`DRAG_SLOP` (it just stops watching), so once the finger's *current*
|
||||||
to the list from the first slop-crossing frame. The `DragGesture`
|
position leaves the composer's box and enters the list's, `List`
|
||||||
merge (RUST.md's plan box) should make this one mechanism: once a
|
starts receiving ordinary hit-tested `Pressing` frames there --
|
||||||
gesture commits to a pan, the list captures it wherever it began.
|
`DragArbiter::is_idle()`'s 2026-09-05 recovery (a missed `PressStart`)
|
||||||
- [ ] **"Flinging still does not work."** Expected on this build: finger
|
picks it up rather than leaving it stuck. What this does **not** do is
|
||||||
flings are dropped by per-widget hit testing, which `DragGesture`'s
|
what "wherever it began" implies literally: `DragArbiter::press_start`
|
||||||
pointer capture (commit `e12c708`, not yet merged at 02:07) targets.
|
restarts from the *boundary-crossing* position, not from the original
|
||||||
Stays open until verified on her phone, not the emulator.
|
touch-down inside the composer, so the pan still needs a fresh
|
||||||
|
`DRAG_SLOP` of travel measured from the boundary rather than from the
|
||||||
|
start of the gesture -- composer and list are adjacent, non-overlapping
|
||||||
|
widgets (`lib.rs`'s `(list, composer_bar).span(Dir::DOWN)`), and only
|
||||||
|
the composer forwarding its own drag to the list would remove that
|
||||||
|
residual slop entirely, which is more than this pass's merge changes.
|
||||||
|
RUST.md's merge-pass box has the reasoning in full and an emulator
|
||||||
|
swipe confirming the composer's own box never moves/resizes during it;
|
||||||
|
whether the residual slop is still perceptible as "caught" needs Iris's
|
||||||
|
phone, since the emulator's per-widget boundary is a few dp wide and
|
||||||
|
easy to cross without noticing on a real screen too.
|
||||||
|
- [ ] **"Flinging still does not work."** No longer expected to reproduce
|
||||||
|
after the `DragGesture` merge (`e12c708`, pointer capture +
|
||||||
|
`CursorSense::Drop`), 2026-09-06. Emulator evidence (RUST.md's
|
||||||
|
merge-pass box, check (b)): a real `ui-trace` finger swipe followed by
|
||||||
|
screenshot-hash sampling caught a post-release frame distinct from the
|
||||||
|
drag's own last frame in one run, and every run showed 28-32
|
||||||
|
`render()` frames per gesture against an idle baseline of 0 and ~8
|
||||||
|
expected from the drag alone -- redraw kept being requested well past
|
||||||
|
the finger lifting, which only happens while a fling is still
|
||||||
|
animating. Left unticked in spirit until Iris's phone confirms it,
|
||||||
|
since only she can say whether it *feels* like a fling now; the
|
||||||
|
emulator's screenshot timing could not always catch the tail of a
|
||||||
|
fast-settling one visually (same caveat noted in RUST.md).
|
||||||
- [ ] **"Text still disappears if I leave and come back to the app."**
|
- [ ] **"Text still disappears if I leave and come back to the app."**
|
||||||
The `GlyphAtlas::clear`/`Textures::reset` fix was verified on the
|
The `GlyphAtlas::clear`/`Textures::reset` fix was verified on the
|
||||||
emulator under `force-gles` only; the phone runs Vulkan. So either the
|
emulator under `force-gles` only; the phone runs Vulkan. So either the
|
||||||
|
|||||||
@@ -0,0 +1,214 @@
|
|||||||
|
# Review: iris changes since 0e46293
|
||||||
|
|
||||||
|
Scope: `git diff 0e46293..HEAD -- iris/ client-core/` (58 files, +5224/-226).
|
||||||
|
Read-only review; no source changed. Ordered likely-bug, then invariant
|
||||||
|
guards, then rules, then tests/docs.
|
||||||
|
|
||||||
|
## Likely bugs
|
||||||
|
|
||||||
|
1. **`iris/transcript-ui/src/lib.rs:152-160` (`RowDiff::Rebuild` arm of
|
||||||
|
`TranscriptScreen::apply`) never unregisters the rows it drops from
|
||||||
|
`Selection`, so a stale `WeakWidget<TextEdit>` outlives the widget it
|
||||||
|
points to and the next touch on *any* row panics.**
|
||||||
|
`Selection::rows: BTreeMap<RowKey, WeakWidget<TextEdit>>` documents its
|
||||||
|
own contract at `selection.rs:69-71`: "every addition here needs its
|
||||||
|
removal ... called when `List` evicts the row." The `ReplaceLast` arm
|
||||||
|
above it honours this (`lib.rs:143-145`, `self.selection.borrow_mut()
|
||||||
|
.unregister(old_key)` when the key changes). The `Rebuild` arm calls
|
||||||
|
`(self.list)(rsc).clear()` and rebuilds every row from `new_rows`, but
|
||||||
|
never touches `self.selection` — any key present in `old_rows` and
|
||||||
|
*absent* from `new_rows` (exactly what `group_tool_runs` regrouping two
|
||||||
|
separate tool-call rows into one produces — see `diff_tests::
|
||||||
|
a_tool_run_closing_and_joining_an_earlier_call_is_a_regroup_fallback`,
|
||||||
|
which tests the diff decision but not `apply` itself) is left in
|
||||||
|
`self.rows` pointing at a widget `List::clear()` just freed.
|
||||||
|
`TextEditable::edit` (`iris/src/widget/text/edit.rs:582-587`) resolves
|
||||||
|
that handle with `ui.widgets.get_mut(self).unwrap()` — an unconditional
|
||||||
|
panic on the freed slot. `Selection::begin` (`selection.rs:88-101`)
|
||||||
|
iterates *every* registered row (`w.edit(ui).deselect()`) on an
|
||||||
|
ordinary fresh press, so the crash fires on the next tap anywhere in
|
||||||
|
the transcript after a regroup, not only on a tap targeting the
|
||||||
|
orphaned row.
|
||||||
|
Fix: give `Selection` a way to reconcile against the row set that
|
||||||
|
survived a rebuild (e.g. `Selection::retain(&self, keys: &BTreeSet<RowKey>)`
|
||||||
|
removing everything else, called from the `Rebuild` arm before
|
||||||
|
rebuilding), or simplest — call `self.selection.borrow_mut()` cleared
|
||||||
|
the same way `List::clear()` clears the list, then let the rebuild's
|
||||||
|
`push_row` calls re-`register` everything as they already do.
|
||||||
|
|
||||||
|
## Guarded invariants missing
|
||||||
|
|
||||||
|
2. **`iris/src/widget/list.rs:751` (`List::place`) indexes/expects on
|
||||||
|
`slot` with no assertion that it exists.** `slot_widget` (`:563-575`)
|
||||||
|
panics via `.expect(...)` for a sentinel with no widget set, and does
|
||||||
|
an unchecked `&self.items[s as usize]` for a real index — a bare
|
||||||
|
"index out of bounds" with no context if `place` is ever reached with a
|
||||||
|
stale slot. Every current caller happens to derive `slot` from
|
||||||
|
`repair_anchor`/`prev_slot`/`next_slot`, which already check existence,
|
||||||
|
but that invariant is enforced by convention across three call sites,
|
||||||
|
not by the function that depends on it. Add
|
||||||
|
`debug_assert!(self.slot_exists(slot), "place() called with a slot that doesn't exist: {slot:?}");`
|
||||||
|
at the top of `place`.
|
||||||
|
3. **`iris/src/widget/list.rs:426` (`List::fling`) and `sense.rs`'s
|
||||||
|
`FlingCalculator::distance`/`duration`/`position_at` never check that
|
||||||
|
the incoming velocity is finite.** A `NaN`/`inf` velocity (a
|
||||||
|
`VelocityTracker::velocity()` divide-by-near-zero span, or a caller
|
||||||
|
passing a raw device value straight through) propagates through
|
||||||
|
`deceleration_for`'s `.ln()` silently — the fling either never settles
|
||||||
|
(`settled_on_schedule` compares against a `NaN` `duration()`, which is
|
||||||
|
always `false`) or jumps to `NaN` positions with nothing on screen
|
||||||
|
saying why. Add `debug_assert!(velocity_px_per_s.is_finite())` in
|
||||||
|
`List::fling` and `FlingCalculator::new`/`distance`.
|
||||||
|
4. **`iris/src/sense.rs:592-604` (`VelocityTracker::velocity`) has no
|
||||||
|
assertion that samples are chronological.** `add_sample` trusts its
|
||||||
|
caller's `Instant` ordering; a caller that samples out of order (a
|
||||||
|
restored/replayed gesture, a test) would silently produce a negative
|
||||||
|
`span` handled only by the `span <= 0.0 => 0.0` catch-all, masking the
|
||||||
|
bug that produced it rather than surfacing it. Add
|
||||||
|
`debug_assert!(self.samples.back().is_none_or(|&(last, _)| at >= last))`
|
||||||
|
in `add_sample`.
|
||||||
|
5. **`iris/core/src/render/frame_report.rs:247-252` (`mark_phase`) has no
|
||||||
|
assertion that phases are pushed in non-decreasing `start_index`
|
||||||
|
order.** `phase_stats`'s slicing (`:274`, `idx >= phase.start_index &&
|
||||||
|
idx < end_index`) silently produces an empty or nonsensical slice for
|
||||||
|
an out-of-order phase rather than surfacing the misuse — cheap to add
|
||||||
|
given `self.phases.last()` is already in scope:
|
||||||
|
`debug_assert!(self.phases.last().is_none_or(|p| self.total_frames >= p.start_index));`
|
||||||
|
|
||||||
|
## Rules
|
||||||
|
|
||||||
|
6. **Two mechanisms answer "what row selection points at, still valid?"**
|
||||||
|
`Selection` relies on callers remembering to `unregister` (finding 1);
|
||||||
|
`List` relies on callers deriving slots only from already-checked
|
||||||
|
sources (finding 2). Both are the same class of problem — a derived
|
||||||
|
handle that silently outlives what it points to — solved ad hoc twice
|
||||||
|
rather than once. Not asking for a shared abstraction here, but the two
|
||||||
|
should at minimum cross-reference each other's doc comment so the next
|
||||||
|
caller who adds a third handle-into-`List`-rows type (the code rules'
|
||||||
|
"a rule that governs a set belongs to the set") finds both existing
|
||||||
|
examples.
|
||||||
|
7. **`iris/android-app/src/bench_client.rs:224-225` (`battery_line`)
|
||||||
|
calls `.min().unwrap()`/`.max().unwrap()` on `samples` guarded three
|
||||||
|
lines above by `if samples.is_empty()`, which is fine — but the guard
|
||||||
|
and the two unwraps are two statements apart with a `let mean = ...`
|
||||||
|
in between reading the same slice; a future edit reordering those
|
||||||
|
lines loses the guard's protection silently.** Low severity (this is
|
||||||
|
the bench tool, not the app), but worth a one-line comment tying the
|
||||||
|
unwraps back to the guard, or restructuring as
|
||||||
|
`let (Some(min), Some(max)) = (samples.iter().min(), samples.iter().max())`
|
||||||
|
pattern so the empty case can't be separated from the check by a future
|
||||||
|
edit.
|
||||||
|
|
||||||
|
## Tests
|
||||||
|
|
||||||
|
8. **No test exercises `TranscriptScreen::apply`'s `Rebuild` arm through
|
||||||
|
`Selection`.** `lib.rs`'s `diff_tests` module (`:284-379`) tests only
|
||||||
|
the pure `diff_rows` decision function, never `apply` itself wired to a
|
||||||
|
real `Selection`; `selection.rs`'s own tests (`a_missed_press_start_
|
||||||
|
recovers_on_the_next_pressing_frame`, `unregister_forgets_the_row_and_
|
||||||
|
clears_a_matching_anchor`) never go through `apply`/`List::clear`
|
||||||
|
either. This is exactly the gap that let finding 1 through: the two
|
||||||
|
pieces (`apply`'s fallback, `Selection`'s registration contract) are
|
||||||
|
each tested in isolation and never together. Add: build a
|
||||||
|
`TranscriptScreen`, force a `RowDiff::Rebuild` (two adjacent tool-call
|
||||||
|
rows regrouping, per the existing `diff_tests` case), then call
|
||||||
|
`selected_text`/simulate a fresh press on a surviving row and assert no
|
||||||
|
panic.
|
||||||
|
9. **`iris/src/widget/list.rs`'s fling tests check total distance and the
|
||||||
|
start/end clamp but not the speed profile in between.**
|
||||||
|
`fling_moves_the_list_and_then_settles`/`fling_distance_is_positive_
|
||||||
|
toward_the_end` only assert the fling started, moved in the right
|
||||||
|
direction, and eventually stopped — none checks that
|
||||||
|
`tick_fling`'s per-tick delta is *monotonically decreasing* once past
|
||||||
|
the fling's peak (the property `fling_calculator_tests::position_at_
|
||||||
|
is_monotonic_and_clamped_past_the_end` already checks one level down,
|
||||||
|
for `FlingCalculator` alone, but never through `List::tick_fling`'s own
|
||||||
|
`scroll`/`anchor.offset` accumulation). A regression that made
|
||||||
|
`tick_fling` apply the *total* distance every tick instead of the
|
||||||
|
incremental one, for instance, would still pass both existing tests
|
||||||
|
(final position and direction are unaffected by how the interior ticks
|
||||||
|
split it up) while being wildly wrong every intermediate frame.
|
||||||
|
10. **`iris/src/widget/list.rs::replacing_the_last_row_stays_pinned_to_
|
||||||
|
the_bottom` and its sibling test `replace_back`'s effect on the
|
||||||
|
displayed row, never that the row it evicted is actually gone from
|
||||||
|
`heights`/`extents`.** Both tests assert the *new* row's position;
|
||||||
|
neither asserts `old.key` is absent from `list_ref.heights`/`extents`
|
||||||
|
after the replace (the "stale primitive" class finding 1 is a
|
||||||
|
production instance of). A cheap addition: assert
|
||||||
|
`!list_ref.heights.contains_key(&old.key)` after `replace_back` in the
|
||||||
|
existing test, since `old.key` is already returned to the test as
|
||||||
|
`evicted`... (`lib.rs` calls it that way; the `list.rs` test would need
|
||||||
|
to capture the key from `old` similarly.)
|
||||||
|
|
||||||
|
## Docs
|
||||||
|
|
||||||
|
No missing `IRIS.md` entry found for a *public* API change in this diff —
|
||||||
|
`List::fling`/`VelocityTracker`/`FlingCalculator`, `List::
|
||||||
|
anchor_position_display`, `FrameReport::mark_phase`/`phase_stats`/
|
||||||
|
`late_at_hz`, `UiRenderNode::new`'s `Result` change, `Len::dp`, and
|
||||||
|
`List::replace_back`/`clear`/`TranscriptScreen::apply` all have entries.
|
||||||
|
The `List::replace_back`/`clear`/`TranscriptScreen::apply` entry
|
||||||
|
(`docs/IRIS.md:526`) predates this review's finding 1 and does not mention
|
||||||
|
`Selection`'s registration contract at all — once finding 1 is fixed,
|
||||||
|
that entry should gain a line noting what the fix requires of a caller
|
||||||
|
that keeps its own row-keyed side table (the same shape `Selection` is),
|
||||||
|
so the next such table doesn't reproduce the same gap.
|
||||||
|
|
||||||
|
## Fixed, 2026-09-06
|
||||||
|
|
||||||
|
All ten findings addressed after the `DragGesture` merge (`selection.rs`
|
||||||
|
was rewritten by that merge, but finding 1's shape and location were
|
||||||
|
unchanged — `TranscriptScreen::apply`'s `Rebuild` arm, `iris/transcript-ui/
|
||||||
|
src/lib.rs`).
|
||||||
|
|
||||||
|
1. **Fixed.** `Selection::clear()` (`selection.rs`) drops `rows` and
|
||||||
|
`anchor`, called from `apply`'s `Rebuild` arm right before
|
||||||
|
`List::clear()` — `push_row` re-`register`s whatever survives as it
|
||||||
|
rebuilds each row, the "simplest" fix option the finding named.
|
||||||
|
2. **Fixed.** `debug_assert!(self.slot_exists(slot), ...)` at the top of
|
||||||
|
`List::place` (`iris/src/widget/list.rs`).
|
||||||
|
3. **Fixed.** `debug_assert!(velocity_px_per_s.is_finite())` in
|
||||||
|
`List::fling`, and `debug_assert!(velocity.is_finite())` in
|
||||||
|
`FlingCalculator::distance`/`duration` (`iris/src/sense.rs`).
|
||||||
|
`position_at` calls both, so it inherits the guard rather than needing
|
||||||
|
its own.
|
||||||
|
4. **Fixed.** `debug_assert!` on chronological sample order in
|
||||||
|
`VelocityTracker::add_sample` (`iris/src/sense.rs`).
|
||||||
|
5. **Fixed.** `debug_assert!` on non-decreasing `start_index` in
|
||||||
|
`FrameReport::mark_phase` (`iris/core/src/render/frame_report.rs`).
|
||||||
|
6. **Fixed (doc cross-reference only, as asked).** `Selection::register`'s
|
||||||
|
doc now points at `List::place`'s `slot_exists` assertion and vice
|
||||||
|
versa isn't needed since finding 2's fix already cites this file in
|
||||||
|
its own comment; both are grep-able on "docs/REVIEW-2026-09-06.md" and
|
||||||
|
on each other's type names.
|
||||||
|
7. **Fixed.** `bench_client.rs::battery_line` restructured to
|
||||||
|
`let (Some(min), Some(max)) = (samples.iter().min(), samples.iter().max())`,
|
||||||
|
so the empty-guard and the two lookups can no longer be separated by a
|
||||||
|
future edit.
|
||||||
|
8. **Fixed.** `transcript-ui`'s new `apply_tests::
|
||||||
|
a_row_dropped_by_a_regroup_does_not_outlive_itself_in_selection`
|
||||||
|
(`lib.rs`) builds a real `TranscriptScreen`, forces the same regroup
|
||||||
|
shape `diff_tests` already covers at the pure-diff level, calls `apply`,
|
||||||
|
and then `Selection::begin` on a surviving row — which panicked before
|
||||||
|
fix 1, resolving a `WeakWidget` `List::clear()` had just freed.
|
||||||
|
9. **Fixed.** `list.rs`'s new `tick_fling_applies_shrinking_incremental_
|
||||||
|
deltas` flings toward the end from `jump_to_start` and asserts each
|
||||||
|
tick's `extents[&0]` delta is no larger than the previous one — would
|
||||||
|
fail against a `tick_fling` that applied the total spline distance
|
||||||
|
every tick instead of the incremental slice, which the two pre-existing
|
||||||
|
fling tests cannot catch.
|
||||||
|
10. **Fixed.** `list.rs`'s new `replace_back_forgets_the_evicted_keys_own_
|
||||||
|
height` replaces row 4 with a row keyed `100` (the two existing
|
||||||
|
`replace_back` tests always reuse the same key, so neither actually
|
||||||
|
exercises the removal) and asserts `heights` no longer contains the
|
||||||
|
evicted key.
|
||||||
|
|
||||||
|
Docs: `docs/IRIS.md`'s 2026-09-05 `List::replace_back`/`clear`/
|
||||||
|
`TranscriptScreen::apply` entry now has a line on what the fix requires of
|
||||||
|
a caller with its own row-keyed side table, naming `Selection` as the
|
||||||
|
example and dating the fix.
|
||||||
|
|
||||||
|
Verification run alongside the rest of this pass's checks: `cargo fmt
|
||||||
|
--all`, `cargo clippy --workspace --all-targets`, `cargo test --workspace`
|
||||||
|
from `iris/` — see docs/RUST.md's plan box for the pass/fail and any
|
||||||
|
caveats from this same session.
|
||||||
+112
-20
@@ -43,26 +43,118 @@ gated on her verdict**, so this pass works the P0 defects and the pure
|
|||||||
prerequisites in this order. Each item is ticked here by the agent that
|
prerequisites in this order. Each item is ticked here by the agent that
|
||||||
closes it.
|
closes it.
|
||||||
|
|
||||||
- [ ] **Merge the `DragGesture` work** left complete but unmerged in the
|
- [x] **Merge the `DragGesture` work** -- done 2026-09-06 (merge commit
|
||||||
worktree branch `worktree-agent-a754368325fa06839` (commit
|
`f802de9`, `git merge --no-ff worktree-agent-a754368325fa06839`,
|
||||||
`e12c708`, 2026-09-06 02:10, two minutes after the last merge to
|
clean, no conflicts across the 8 files `e12c708` touched). Targets
|
||||||
`rustify`; already contains `rustify` at `543f6d9`). It targets two
|
two of the four bench-v2 defects: finger flings dropped by
|
||||||
of the four bench-v2 defects: finger flings dropped by per-widget
|
per-widget hit testing (pointer capture + `CursorSense::Drop`), and
|
||||||
hit testing (pointer capture + `CursorSense::Drop`), and IME insets
|
IME insets never redelivered (`MainActivity.java` edge-to-edge).
|
||||||
never redelivered (`MainActivity.java` edge-to-edge). Before
|
**Tap-vs-swipe/`DragGesture` overlap, reasoned through**: `attr.rs`'s
|
||||||
merging: build, clippy, tests; then on the emulator confirm the
|
`on_press` (composer focus) and `sense.rs`'s `DragArbiter`/
|
||||||
three things `20b1225` (tap-vs-swipe focus) and the phone asked for
|
`DragGesture` (list pan-vs-select) do not share a mechanism, but
|
||||||
still hold together — a swipe over the composer does not summon
|
they don't need to -- `on_press` never calls `capture_pointer`, so
|
||||||
the keyboard, a tap does, a finger fling on the list keeps moving
|
it only ever sees an ordinary per-frame hit-tested `Pressing`/
|
||||||
after the finger lifts, and `on_insets_changed` now fires on an
|
`PressEnd` (`run_sensors`' `region.contains(cursor.pos)` check,
|
||||||
IME toggle. Then remove the worktree.
|
unaffected by capture unless *this* widget requested it), the same
|
||||||
- [ ] **Fix `docs/REVIEW-2026-09-06.md`** (after the merge, since the
|
as before `DragGesture` existed. The two only interact where a
|
||||||
review's finding 1 is in `selection.rs`, which the merge rewrites).
|
gesture starts on the composer and travels into the list's region;
|
||||||
Finding 1 is a real crash — `TranscriptScreen::apply`'s `Rebuild`
|
`run_sensors` already delivers `Pressing` to whichever widget's
|
||||||
arm leaves `Selection` holding `WeakWidget`s to freed rows, and the
|
*current* position contains the pointer, so `List` starts getting
|
||||||
next tap anywhere panics. Findings 2–5 are `debug_assert!`s on
|
frames the instant the finger crosses the boundary -- with no
|
||||||
invariants, 8 and 10 are the missing tests. Commit the review file
|
`PressStart` of its own, which is exactly what `DragArbiter::
|
||||||
with the fixes.
|
is_idle()`'s 2026-09-05 recovery branch exists for. No consolidation
|
||||||
|
needed; `DRAG_SLOP` is already the one shared constant (`attr.rs`
|
||||||
|
imports it from `sense.rs`, not a second copy).
|
||||||
|
**Checks, 2026-09-06 merge pass**: `cargo fmt --all` clean;
|
||||||
|
`cargo clippy -p iris -p iris-core -p transcript-ui --all-targets`
|
||||||
|
and the same for `-p desktop-app -p tabs-ui`, zero warnings beyond
|
||||||
|
the pre-existing external-crate future-incompat notice
|
||||||
|
(naga/wgpu/wgpu-core/wgpu-hal/winit); `cargo test --lib -p iris -p
|
||||||
|
iris-core -p transcript-ui` and `-p desktop-app -p tabs-ui`, 97
|
||||||
|
passed/0 failed, including the review-fix tests below.
|
||||||
|
`cargo test --workspace`/`cargo clippy --workspace --all-targets`
|
||||||
|
(the full-workspace forms, which also build `iris`'s winit examples)
|
||||||
|
were abandoned after 40+ minutes each stuck compiling one example
|
||||||
|
binary with `uptime` reading a load average of 66-78 on this 8-core
|
||||||
|
VM (3-4 concurrent peer `cargo`/`cargo check` invocations the whole
|
||||||
|
session) -- `ps -o time` on the stuck `rustc` showed 2 seconds of
|
||||||
|
accumulated CPU time after 38 minutes of wall time, confirming
|
||||||
|
scheduler starvation rather than a hang. The per-package `--lib`
|
||||||
|
form above is what actually exercises the changed code and finished
|
||||||
|
in under 4 minutes warm. `android-app` (`iris-android-app`) is
|
||||||
|
excluded from the host workspace (`iris/Cargo.toml`, needs the NDK
|
||||||
|
target) and is covered instead by the APK build below, which
|
||||||
|
compiles it for `x86_64-linux-android`.
|
||||||
|
|
||||||
|
**Emulator checks, 2026-09-06** (this checkout's `ai-app-2` AVD,
|
||||||
|
`iris/android-app/build-apk.sh debug --abi x86_64 --features
|
||||||
|
"transcript-screen bench force-gles"` -- plain Vulkan crashed on
|
||||||
|
this AVD's boot this pass, `wgpu_core::instance: enabled backend
|
||||||
|
Vulkan has no adapters`, unrelated to this merge and worked around
|
||||||
|
with `force-gles` the way I5's own box already documents for this
|
||||||
|
hardware):
|
||||||
|
- **(a) tap-vs-swipe still holds.** Fresh app launch, `dumpsys
|
||||||
|
input_method`'s `mInputShown=false` at rest. `ui-trace record
|
||||||
|
--do "swipe 540 1510 540 700 200"` (a swipe starting on the
|
||||||
|
composer's own box, read from `ui-trace show -m Message --field
|
||||||
|
box` as `31,1488..1048,1540`) leaves `mInputShown=false` and the
|
||||||
|
box unmoved (no keyboard-driven resize). `ui-trace record --do
|
||||||
|
"tap 540 1510"` on the same field then reads `mInputShown=true`.
|
||||||
|
Matches `20b1225`'s original result -- the `DragGesture` merge
|
||||||
|
did not disturb it, confirming the reasoning above.
|
||||||
|
- **(b) a real finger fling keeps the list moving after release.**
|
||||||
|
Screenshot-hash sampling (`adb exec-out screencap`, `md5`, since
|
||||||
|
transcript rows carry no per-row accessibility label yet -- I5's
|
||||||
|
own leftover -- so `ui-trace show` cannot track them) at ~40-60ms
|
||||||
|
intervals through and after a fast `swipe 540 1400 540 400 120`
|
||||||
|
(with room to scroll confirmed by a preceding slow drag) caught
|
||||||
|
two *distinct* post-release frames in one run (a settle-position
|
||||||
|
beyond the raw drag's own last frame), and every run showed
|
||||||
|
28-32 `iris::android::view: render()` log lines per gesture
|
||||||
|
against an idle baseline of 0 in 1.5s and roughly 8 expected from
|
||||||
|
a bare 120ms drag's own `Pressing` frames alone -- i.e. redraw
|
||||||
|
kept being requested well past the finger lifting, which only
|
||||||
|
happens while `List::tick_fling` is still returning `true`.
|
||||||
|
Some runs' screenshots showed only the drag's own jump with nothing
|
||||||
|
further *visibly different*, which is consistent with a real but
|
||||||
|
small/fast-settling fling (a modest synthetic-touch velocity's
|
||||||
|
spline tail moves little per frame) rather than absence of one --
|
||||||
|
the render-count signal did not vary between those runs and the
|
||||||
|
one with a visible second frame. Recorded as confirmed, with that
|
||||||
|
caveat, rather than measured to a number; a phone verification
|
||||||
|
(Iris's own report closes this properly) is still open per
|
||||||
|
`IRIS_TODO.md`'s item.
|
||||||
|
- **(c) `on_insets_changed` fires on an IME toggle, with confirmed
|
||||||
|
cycles.** `run-bench.sh`'s report: `keyboard: shown 4/5, hidden
|
||||||
|
5/5 (confirmed via on_insets_changed)` -- the "could not be
|
||||||
|
shown" unknown-state line (`bench_client.rs::run_keyboard_phase`)
|
||||||
|
did not fire, unlike the pre-`DragGesture` build this same report
|
||||||
|
format existed for.
|
||||||
|
Worktrees removed after the checks above: `agent-a754368325fa06839`
|
||||||
|
(the source branch, its own emulator stopped first via `cd` into
|
||||||
|
it + `emu down`), `agent-a27094a7db775552a`, `agent-a1ff0294b6c29127e`,
|
||||||
|
`agent-a9002910a315fe719` -- each confirmed `git rev-list --count
|
||||||
|
rustify..<branch>` = 0 and no uncommitted changes first; their
|
||||||
|
branches deleted too. `agent-a16b22e34539b810e` and
|
||||||
|
`agent-a6e37a2335f436d08` left alone -- both `git worktree list`
|
||||||
|
`locked` to a live peer agent.
|
||||||
|
- [x] **Fix `docs/REVIEW-2026-09-06.md`**, done 2026-09-06, after the
|
||||||
|
merge (finding 1's shape and location in `selection.rs`/`lib.rs`
|
||||||
|
were unchanged by the merge, which touched `Selection` but not
|
||||||
|
`apply`'s `Rebuild` arm). All ten findings fixed -- new
|
||||||
|
`Selection::clear()` for finding 1 (the simplest option the review
|
||||||
|
named: clear the same way `List::clear()` clears the list, let
|
||||||
|
`push_row` re-`register` survivors), five `debug_assert!`s
|
||||||
|
(2-5, plus 7's restructure), and three new tests (8, 9, 10),
|
||||||
|
confirmed with the `apply_tests::a_row_dropped_by_a_regroup_does_
|
||||||
|
not_outlive_itself_in_selection` test passing (it exercises exactly
|
||||||
|
finding 1's shape: build a real `TranscriptScreen`, force the same
|
||||||
|
regroup `diff_tests` already covers, `apply`, then a surviving
|
||||||
|
row's `begin` -- panics pre-fix, per the review's own test-8 ask).
|
||||||
|
`docs/IRIS.md`'s 2026-09-05 entry gained the line the review's
|
||||||
|
"Docs" section asked for. See `docs/REVIEW-2026-09-06.md`'s own "Fixed, 2026-09-06"
|
||||||
|
section for the per-finding account. Committed together with the
|
||||||
|
review file.
|
||||||
- [ ] **Iris's 11:39 phone report on the 02:07 build** (four items,
|
- [ ] **Iris's 11:39 phone report on the 02:07 build** (four items,
|
||||||
verbatim in `IRIS_TODO.md`'s "From the phone, 2026-09-06, 11:39"):
|
verbatim in `IRIS_TODO.md`'s "From the phone, 2026-09-06, 11:39"):
|
||||||
composer floating two thirds down the screen at launch with black
|
composer floating two thirds down the screen at launch with black
|
||||||
|
|||||||
@@ -221,8 +221,15 @@ fn battery_line(samples: &[i32]) -> String {
|
|||||||
return " battery current: unavailable on this device".to_string();
|
return " battery current: unavailable on this device".to_string();
|
||||||
}
|
}
|
||||||
let mean = samples.iter().map(|&v| v as i64).sum::<i64>() / samples.len() as i64;
|
let mean = samples.iter().map(|&v| v as i64).sum::<i64>() / samples.len() as i64;
|
||||||
let min = samples.iter().min().unwrap();
|
// `min`/`max` are guarded by the `is_empty` check above, three lines
|
||||||
let max = samples.iter().max().unwrap();
|
// up -- pairing the `Option` unwraps with the emptiness check right
|
||||||
|
// here (rather than two statements apart, with `mean` in between
|
||||||
|
// reading the same slice) is what keeps a future reorder from
|
||||||
|
// separating the guard from what it protects (docs/
|
||||||
|
// REVIEW-2026-09-06.md finding 7).
|
||||||
|
let (Some(min), Some(max)) = (samples.iter().min(), samples.iter().max()) else {
|
||||||
|
unreachable!("samples is non-empty, checked above");
|
||||||
|
};
|
||||||
format!(
|
format!(
|
||||||
" battery current: mean {mean}\u{b5}A over {} samples (min {min}, max {max})",
|
" battery current: mean {mean}\u{b5}A over {} samples (min {min}, max {max})",
|
||||||
samples.len()
|
samples.len()
|
||||||
|
|||||||
@@ -245,6 +245,15 @@ impl FrameReport {
|
|||||||
/// this once per phase (fling/stream/type/keyboard) so `phase_stats`
|
/// this once per phase (fling/stream/type/keyboard) so `phase_stats`
|
||||||
/// can slice one whole run's frames by what was happening during each.
|
/// can slice one whole run's frames by what was happening during each.
|
||||||
pub fn mark_phase(&mut self, name: &str) {
|
pub fn mark_phase(&mut self, name: &str) {
|
||||||
|
// `phase_stats`'s slicing (`idx >= phase.start_index && idx <
|
||||||
|
// end_index`) silently produces an empty or nonsensical slice for
|
||||||
|
// a phase pushed out of order rather than surfacing the misuse
|
||||||
|
// (docs/REVIEW-2026-09-06.md finding 5).
|
||||||
|
debug_assert!(
|
||||||
|
self.phases
|
||||||
|
.last()
|
||||||
|
.is_none_or(|p| self.total_frames >= p.start_index)
|
||||||
|
);
|
||||||
self.phases.push(PhaseMark {
|
self.phases.push(PhaseMark {
|
||||||
name: name.to_string(),
|
name: name.to_string(),
|
||||||
start_index: self.total_frames,
|
start_index: self.total_frames,
|
||||||
|
|||||||
@@ -781,6 +781,12 @@ impl VelocityTracker {
|
|||||||
/// Record one frame's motion. `delta` is this frame's movement since
|
/// Record one frame's motion. `delta` is this frame's movement since
|
||||||
/// the last sample, not a cumulative position.
|
/// the last sample, not a cumulative position.
|
||||||
pub fn add_sample(&mut self, delta: f32, at: Instant) {
|
pub fn add_sample(&mut self, delta: f32, at: Instant) {
|
||||||
|
// A caller that samples out of order (a restored/replayed
|
||||||
|
// gesture, a test) would silently produce a negative `span` in
|
||||||
|
// `velocity`, handled only by its `span <= 0.0 => 0.0` catch-all
|
||||||
|
// -- masking the bug that produced it rather than surfacing it
|
||||||
|
// (docs/REVIEW-2026-09-06.md finding 4).
|
||||||
|
debug_assert!(self.samples.back().is_none_or(|&(last, _)| at >= last));
|
||||||
self.samples.push_back((at, delta));
|
self.samples.push_back((at, delta));
|
||||||
while let Some(&(when, _)) = self.samples.front() {
|
while let Some(&(when, _)) = self.samples.front() {
|
||||||
if at.duration_since(when) > VELOCITY_WINDOW {
|
if at.duration_since(when) > VELOCITY_WINDOW {
|
||||||
@@ -956,6 +962,10 @@ impl FlingCalculator {
|
|||||||
/// Total signed distance the fling travels before settling, in the
|
/// Total signed distance the fling travels before settling, in the
|
||||||
/// same pixel units `velocity` was given in.
|
/// same pixel units `velocity` was given in.
|
||||||
pub fn distance(&self, velocity: f32) -> f32 {
|
pub fn distance(&self, velocity: f32) -> f32 {
|
||||||
|
// See `List::fling`'s matching assertion -- a non-finite velocity
|
||||||
|
// here silently produces a NaN distance rather than surfacing the
|
||||||
|
// bug that produced it (docs/REVIEW-2026-09-06.md finding 3).
|
||||||
|
debug_assert!(velocity.is_finite());
|
||||||
if velocity == 0.0 {
|
if velocity == 0.0 {
|
||||||
return 0.0;
|
return 0.0;
|
||||||
}
|
}
|
||||||
@@ -968,6 +978,8 @@ impl FlingCalculator {
|
|||||||
|
|
||||||
/// How long the fling takes to settle.
|
/// How long the fling takes to settle.
|
||||||
pub fn duration(&self, velocity: f32) -> Duration {
|
pub fn duration(&self, velocity: f32) -> Duration {
|
||||||
|
// See `distance`'s matching assertion, above.
|
||||||
|
debug_assert!(velocity.is_finite());
|
||||||
if velocity == 0.0 {
|
if velocity == 0.0 {
|
||||||
return Duration::ZERO;
|
return Duration::ZERO;
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -424,6 +424,13 @@ impl List {
|
|||||||
/// pixels, so `1.0` here is not a placeholder for "unknown density,"
|
/// pixels, so `1.0` here is not a placeholder for "unknown density,"
|
||||||
/// it is the correct density for a self-consistent unit system.
|
/// it is the correct density for a self-consistent unit system.
|
||||||
pub fn fling(&mut self, velocity_px_per_s: f32) {
|
pub fn fling(&mut self, velocity_px_per_s: f32) {
|
||||||
|
// A NaN/inf velocity (a `VelocityTracker::velocity()` divide-by-
|
||||||
|
// near-zero span, or a caller passing a raw device value straight
|
||||||
|
// through) would propagate silently into `deceleration_for`'s
|
||||||
|
// `.ln()` -- the fling either never settles or jumps to NaN
|
||||||
|
// positions with nothing on screen saying why (docs/
|
||||||
|
// REVIEW-2026-09-06.md finding 3).
|
||||||
|
debug_assert!(velocity_px_per_s.is_finite());
|
||||||
if velocity_px_per_s == 0.0 || self.anchor.is_none() {
|
if velocity_px_per_s == 0.0 || self.anchor.is_none() {
|
||||||
self.fling = None;
|
self.fling = None;
|
||||||
return;
|
return;
|
||||||
@@ -763,6 +770,17 @@ impl List {
|
|||||||
/// one-frame lag `Scroll`'s own content-length cache accepts, per
|
/// one-frame lag `Scroll`'s own content-length cache accepts, per
|
||||||
/// LAYOUT.md.
|
/// LAYOUT.md.
|
||||||
fn place(&mut self, painter: &mut Painter, slot: isize, placement: Placement) -> (f32, f32) {
|
fn place(&mut self, painter: &mut Painter, slot: isize, placement: Placement) -> (f32, f32) {
|
||||||
|
// Every current caller derives `slot` from `repair_anchor`/
|
||||||
|
// `prev_slot`/`next_slot`, which already check existence -- but
|
||||||
|
// that invariant is enforced by convention across three call
|
||||||
|
// sites, not by this function, which would otherwise fail with a
|
||||||
|
// bare "index out of bounds" and no context (docs/
|
||||||
|
// REVIEW-2026-09-06.md finding 2). `slot_widget`, called from
|
||||||
|
// here, is what actually indexes/`.expect`s on it.
|
||||||
|
debug_assert!(
|
||||||
|
self.slot_exists(slot),
|
||||||
|
"place() called with a slot that doesn't exist: {slot:?}"
|
||||||
|
);
|
||||||
let axis = self.axis;
|
let axis = self.axis;
|
||||||
let output_len = painter.output_size().axis(axis);
|
let output_len = painter.output_size().axis(axis);
|
||||||
let container_len = painter.region().axis(axis).len();
|
let container_len = painter.region().axis(axis).len();
|
||||||
@@ -1287,6 +1305,52 @@ mod tests {
|
|||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Neither `replacing_the_last_row_stays_pinned_to_the_bottom` nor
|
||||||
|
/// its sibling below ever asserts the *evicted* key's own bookkeeping
|
||||||
|
/// is actually gone -- both replace row 4 with another row also keyed
|
||||||
|
/// `4`, so `heights.remove(&old.key)` removing and re-inserting the
|
||||||
|
/// same key would pass either test even if it did nothing (docs/
|
||||||
|
/// REVIEW-2026-09-06.md finding 10; this is `Selection`'s finding 1
|
||||||
|
/// class of bug -- a stale handle outliving what it points to --
|
||||||
|
/// production-tested from `List`'s own side). Replacing with a
|
||||||
|
/// **different** key is what actually exercises the removal.
|
||||||
|
#[test]
|
||||||
|
fn replace_back_forgets_the_evicted_keys_own_height() {
|
||||||
|
let mut rsc = TestRsc {
|
||||||
|
ui: UiData::default(),
|
||||||
|
};
|
||||||
|
let mut list = List::new(Axis::Y);
|
||||||
|
push_rows(&mut rsc, &mut list, &[0, 1, 2, 3, 4], 20.0);
|
||||||
|
let (list_weak, root) = add_list(&mut rsc, list);
|
||||||
|
|
||||||
|
let mut render = UiRenderState::new();
|
||||||
|
render.resize((100.0, 60.0));
|
||||||
|
render.update(&root, &mut rsc);
|
||||||
|
assert!(
|
||||||
|
rsc.ui
|
||||||
|
.widgets
|
||||||
|
.get(&list_weak)
|
||||||
|
.unwrap()
|
||||||
|
.heights
|
||||||
|
.contains_key(&4)
|
||||||
|
);
|
||||||
|
|
||||||
|
let (_weak, new_row) = fixed_row(&mut rsc, 40.0);
|
||||||
|
let old = rsc
|
||||||
|
.ui
|
||||||
|
.widgets
|
||||||
|
.get_mut(&list_weak)
|
||||||
|
.unwrap()
|
||||||
|
.replace_back(ListRow::new(100, new_row));
|
||||||
|
|
||||||
|
let list_ref = rsc.ui.widgets.get(&list_weak).unwrap();
|
||||||
|
assert_eq!(old.map(|o| o.key), Some(4));
|
||||||
|
assert!(
|
||||||
|
!list_ref.heights.contains_key(&4),
|
||||||
|
"the evicted key's cached height must not outlive the row it measured"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
/// The other half of the same fix's contract: replacing a row that is
|
/// The other half of the same fix's contract: replacing a row that is
|
||||||
/// *not* on screen must not move anything that is. `replace_back` only
|
/// *not* on screen must not move anything that is. `replace_back` only
|
||||||
/// touches the last slot's own widget and this file's own `heights`/
|
/// touches the last slot's own widget and this file's own `heights`/
|
||||||
@@ -1480,6 +1544,71 @@ mod tests {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// `fling_moves_the_list_and_then_settles`/
|
||||||
|
/// `fling_distance_is_positive_toward_the_end` only check that a fling
|
||||||
|
/// started, moved the right way and eventually stopped -- both
|
||||||
|
/// unaffected by *how* the interior ticks split up the total travel
|
||||||
|
/// (docs/REVIEW-2026-09-06.md finding 9). A regression that made
|
||||||
|
/// `tick_fling` apply the whole spline distance every tick instead of
|
||||||
|
/// just this tick's incremental slice would still pass both, while
|
||||||
|
/// being wildly wrong every intermediate frame -- this pins the
|
||||||
|
/// per-tick delta to a decelerating curve (`FlingCalculator::
|
||||||
|
/// position_at`'s own monotonic-and-clamped property, one level
|
||||||
|
/// down, already covers the calculator alone; this is the same
|
||||||
|
/// property through `List::tick_fling`'s `scroll`/`extents`
|
||||||
|
/// accumulation).
|
||||||
|
#[test]
|
||||||
|
fn tick_fling_applies_shrinking_incremental_deltas() {
|
||||||
|
let mut rsc = TestRsc {
|
||||||
|
ui: UiData::default(),
|
||||||
|
};
|
||||||
|
let (list_weak, root, mut render) = build_flingable_list(&mut rsc);
|
||||||
|
rsc.ui.widgets.get_mut(&list_weak).unwrap().jump_to_start();
|
||||||
|
render.update(&root, &mut rsc);
|
||||||
|
|
||||||
|
rsc.ui.widgets.get_mut(&list_weak).unwrap().fling(8000.0);
|
||||||
|
let start = Instant::now();
|
||||||
|
let mut prev_top = rsc.ui.widgets.get(&list_weak).unwrap().extents[&0].top;
|
||||||
|
let mut deltas = Vec::new();
|
||||||
|
for step in 1..600 {
|
||||||
|
let now = start + std::time::Duration::from_millis(step * 16);
|
||||||
|
let still = rsc.ui.widgets.get_mut(&list_weak).unwrap().tick_fling(now);
|
||||||
|
render.update(&root, &mut rsc);
|
||||||
|
let Some(top) = rsc
|
||||||
|
.ui
|
||||||
|
.widgets
|
||||||
|
.get(&list_weak)
|
||||||
|
.unwrap()
|
||||||
|
.extents
|
||||||
|
.get(&0)
|
||||||
|
.map(|e| e.top)
|
||||||
|
else {
|
||||||
|
break; // row 0 scrolled out of the loaded extents
|
||||||
|
};
|
||||||
|
deltas.push((prev_top - top).abs());
|
||||||
|
prev_top = top;
|
||||||
|
if !still {
|
||||||
|
break;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
assert!(
|
||||||
|
deltas.len() >= 3,
|
||||||
|
"fling settled or left row 0's extent before collecting enough samples"
|
||||||
|
);
|
||||||
|
// Skip the first tick (the slop-transition jump the arbiter
|
||||||
|
// applies is a `List::fling`-adjacent concern, not this curve,
|
||||||
|
// but the very first frame can still carry rounding noise from
|
||||||
|
// `jump_to_start`'s own layout settling).
|
||||||
|
for w in deltas[1..].windows(2) {
|
||||||
|
assert!(
|
||||||
|
w[1] <= w[0] + 0.01,
|
||||||
|
"fling's per-tick delta grew instead of decelerating: {:?} then {:?}",
|
||||||
|
w[0],
|
||||||
|
w[1]
|
||||||
|
);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn cancel_fling_stops_it_with_no_further_movement() {
|
fn cancel_fling_stops_it_with_no_further_movement() {
|
||||||
let mut rsc = TestRsc {
|
let mut rsc = TestRsc {
|
||||||
|
|||||||
@@ -151,8 +151,16 @@ impl TranscriptScreen {
|
|||||||
}
|
}
|
||||||
RowDiff::Rebuild => {
|
RowDiff::Rebuild => {
|
||||||
// A row before the tail changed (a regroup) -- nothing
|
// A row before the tail changed (a regroup) -- nothing
|
||||||
// short of a full rebuild expresses that.
|
// short of a full rebuild expresses that. `Selection`
|
||||||
|
// gets cleared the same way `List` does, right before the
|
||||||
|
// rows it was pointing at go with it -- `push_row` below
|
||||||
|
// re-`register`s whatever survives as it rebuilds each
|
||||||
|
// row (docs/REVIEW-2026-09-06.md finding 1: a key that
|
||||||
|
// `group_tool_runs` regrouped away used to stay in
|
||||||
|
// `Selection` pointing at a widget this `clear()` had
|
||||||
|
// just freed, panicking the next long-press anywhere).
|
||||||
self.rebuilds.set(self.rebuilds.get() + 1);
|
self.rebuilds.set(self.rebuilds.get() + 1);
|
||||||
|
self.selection.borrow_mut().clear();
|
||||||
(self.list)(rsc).clear();
|
(self.list)(rsc).clear();
|
||||||
for row in &new_rows {
|
for row in &new_rows {
|
||||||
self.push_row(rsc, row);
|
self.push_row(rsc, row);
|
||||||
@@ -414,3 +422,124 @@ mod diff_tests {
|
|||||||
assert_eq!(diff_rows(&old, &new), RowDiff::Rebuild);
|
assert_eq!(diff_rows(&old, &new), RowDiff::Rebuild);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Exercises `TranscriptScreen::apply`'s `Rebuild` arm through a real
|
||||||
|
/// `Selection`, the gap docs/REVIEW-2026-09-06.md finding 8 named: the
|
||||||
|
/// pure `diff_rows` decision above and `selection.rs`'s own registration
|
||||||
|
/// tests each pass in isolation, and neither alone catches finding 1 (a
|
||||||
|
/// regrouped-away row's key surviving in `Selection` after `List::clear()`
|
||||||
|
/// has already freed its widget). This fails before `Selection::clear()`
|
||||||
|
/// existed and the `Rebuild` arm called it, with a panic from
|
||||||
|
/// `TextEditable::edit` resolving the freed slot.
|
||||||
|
#[cfg(test)]
|
||||||
|
mod apply_tests {
|
||||||
|
use super::*;
|
||||||
|
use client_core::transcript_fold::TranscriptItem;
|
||||||
|
|
||||||
|
struct TestFocus {
|
||||||
|
focus: Option<WeakWidget<TextEdit>>,
|
||||||
|
}
|
||||||
|
impl FocusHost for TestFocus {
|
||||||
|
fn recent_click(&mut self) -> bool {
|
||||||
|
false
|
||||||
|
}
|
||||||
|
fn set_focus(&mut self, id: Option<WeakWidget<TextEdit>>) {
|
||||||
|
self.focus = id;
|
||||||
|
}
|
||||||
|
fn focus_gained(&mut self, _region: Option<PixelRegion>) {}
|
||||||
|
fn is_focused(&self, id: WeakWidget<TextEdit>) -> bool {
|
||||||
|
self.focus == Some(id)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
struct TestRsc {
|
||||||
|
ui: UiData,
|
||||||
|
events: EventManager<TestRsc>,
|
||||||
|
}
|
||||||
|
impl UiRsc for TestRsc {
|
||||||
|
fn ui(&self) -> &UiData {
|
||||||
|
&self.ui
|
||||||
|
}
|
||||||
|
fn ui_mut(&mut self) -> &mut UiData {
|
||||||
|
&mut self.ui
|
||||||
|
}
|
||||||
|
fn on_draw(&mut self, active: &ActiveData) {
|
||||||
|
self.events.draw(active);
|
||||||
|
}
|
||||||
|
fn on_undraw(&mut self, active: &ActiveData) {
|
||||||
|
self.events.undraw(active);
|
||||||
|
}
|
||||||
|
fn on_remove(&mut self, id: WidgetId) {
|
||||||
|
self.events.remove(id);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
impl HasState for TestRsc {
|
||||||
|
type State = TestFocus;
|
||||||
|
}
|
||||||
|
impl HasEvents for TestRsc {
|
||||||
|
fn events(&self) -> &EventManager<Self> {
|
||||||
|
&self.events
|
||||||
|
}
|
||||||
|
fn events_mut(&mut self) -> &mut EventManager<Self> {
|
||||||
|
&mut self.events
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
fn user(seq: u64, text: &str) -> TranscriptItem {
|
||||||
|
TranscriptItem::UserMsg {
|
||||||
|
seq,
|
||||||
|
text: text.to_string(),
|
||||||
|
attachments: Vec::new(),
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
fn tool(seq: u64, run_id: &str) -> TranscriptItem {
|
||||||
|
TranscriptItem::ToolRun {
|
||||||
|
seq,
|
||||||
|
id: format!("id{seq}"),
|
||||||
|
run_id: run_id.to_string(),
|
||||||
|
tool: "grep".to_string(),
|
||||||
|
input: "x".to_string(),
|
||||||
|
output: String::new(),
|
||||||
|
done: false,
|
||||||
|
asks: Vec::new(),
|
||||||
|
images: Vec::new(),
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn a_row_dropped_by_a_regroup_does_not_outlive_itself_in_selection() {
|
||||||
|
use client_core::transcript_fold::group_tool_runs;
|
||||||
|
|
||||||
|
let mut rsc = TestRsc {
|
||||||
|
ui: UiData::default(),
|
||||||
|
events: EventManager::default(),
|
||||||
|
};
|
||||||
|
|
||||||
|
// Same regroup shape as diff_tests' regroup case, plus a trailing
|
||||||
|
// row (seq 4) that survives unchanged -- what a reader would tap
|
||||||
|
// on right after the regroup lands.
|
||||||
|
let old_items = vec![tool(1, "run-a"), user(2, "meanwhile"), user(4, "stable")];
|
||||||
|
let new_items = vec![tool(1, "run-a"), tool(3, "run-a"), user(4, "stable")];
|
||||||
|
assert_eq!(
|
||||||
|
diff_rows(&group_tool_runs(&old_items), &group_tool_runs(&new_items)),
|
||||||
|
RowDiff::Rebuild,
|
||||||
|
"test setup must actually exercise the Rebuild arm"
|
||||||
|
);
|
||||||
|
|
||||||
|
let (screen, _tree) = build_tree(&mut rsc, group_tool_runs(&old_items));
|
||||||
|
screen.apply(&mut rsc, &old_items, &new_items);
|
||||||
|
|
||||||
|
// The surviving row (seq 4) is what a reader's long-press would
|
||||||
|
// land on; `begin` deselects every *other* registered row first,
|
||||||
|
// which is exactly what used to resolve a stale `WeakWidget` left
|
||||||
|
// by the regrouped-away rows and panic.
|
||||||
|
let surviving_key = row::row_key(&client_core::transcript_fold::ItemKey::Seq(4));
|
||||||
|
screen.selection.borrow_mut().begin(
|
||||||
|
&mut rsc,
|
||||||
|
surviving_key,
|
||||||
|
Vec2::ZERO,
|
||||||
|
Vec2::new(10.0, 10.0),
|
||||||
|
);
|
||||||
|
}
|
||||||
|
}
|
||||||
@@ -63,13 +63,32 @@ impl Selection {
|
|||||||
}
|
}
|
||||||
|
|
||||||
/// A row's selectable text became visible/known. Every addition here
|
/// A row's selectable text became visible/known. Every addition here
|
||||||
/// needs its removal (`unregister`) -- called when `List` evicts the
|
/// needs its removal (`unregister`, or `clear` for all of them at
|
||||||
/// row (`pop_front`/`pop_back`), so this map never outgrows however
|
/// once) -- called when `List` evicts the row (`pop_front`/
|
||||||
/// many rows are actually loaded.
|
/// `pop_back`/`clear`), so this map never outgrows however many rows
|
||||||
|
/// are actually loaded. `List::place` guards the twin of this same
|
||||||
|
/// class of bug on the list's own side (`list.rs`'s `slot_exists`
|
||||||
|
/// assertion) -- a derived handle that silently outlives what it
|
||||||
|
/// points to; the next caller adding a third row-keyed side table
|
||||||
|
/// should read both.
|
||||||
pub fn register(&mut self, key: RowKey, text: WeakWidget<TextEdit>) {
|
pub fn register(&mut self, key: RowKey, text: WeakWidget<TextEdit>) {
|
||||||
self.rows.insert(key, text);
|
self.rows.insert(key, text);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Drops every registration at once -- the same shape `List::clear()`
|
||||||
|
/// clears the list, and what `TranscriptScreen::apply`'s `Rebuild` arm
|
||||||
|
/// calls right before it, since a full rebuild drops every row's old
|
||||||
|
/// widget and `push_row` re-`register`s each surviving key's new one
|
||||||
|
/// as it goes (review docs/REVIEW-2026-09-06.md finding 1: the
|
||||||
|
/// `Rebuild` arm used to call only `List::clear()`, leaving any key
|
||||||
|
/// dropped by the regroup -- present in the old rows, absent from the
|
||||||
|
/// new ones -- pointing at a widget the list had just freed, so the
|
||||||
|
/// next long-press anywhere panicked in `begin`'s deselect loop).
|
||||||
|
pub fn clear(&mut self) {
|
||||||
|
self.rows.clear();
|
||||||
|
self.anchor = None;
|
||||||
|
}
|
||||||
|
|
||||||
pub fn unregister(&mut self, key: RowKey) {
|
pub fn unregister(&mut self, key: RowKey) {
|
||||||
self.rows.remove(&key);
|
self.rows.remove(&key);
|
||||||
if self.anchor.map(|(k, _)| k) == Some(key) {
|
if self.anchor.map(|(k, _)| k) == Some(key) {
|
||||||
|
|||||||
Reference in new issue
Block a user