Prune Iris TODO and prioritize color correctness
This commit is contained in:
1 parent
8c6e2ed9cf
commit
5428cd75c9
4 files changed
+123
-335
No files matched your search
+67
-180
@@ -12,170 +12,63 @@ and six phone-report sections went on 2026-09-08 for that reason.
|
||||
|
||||
## Fix
|
||||
|
||||
- [ ] **Where the scroll *pin* lives.** The rest of "scrolling moves out
|
||||
of the list" landed on 2026-09-08 -- `List` is `LazySpan`, the physics
|
||||
and the gesture live in one `ScrollController`, `.scrollable()` is the
|
||||
only way anything scrolls, and **`docs/SCROLL.md` is the standing
|
||||
reference**; read that rather than reconstructing it here.
|
||||
What is left is one design question. The pin ("stay at the end as rows
|
||||
are appended") is still each widget's own: `Scroll` has `snap_end` for
|
||||
an ordinary child, `LazySpan` has one for itself, and the constructor
|
||||
argument sets each. Iris asked for `amt` and "other controls (iirc only
|
||||
at end for now)" to live in `Scroll` so a caller always edits the
|
||||
`Scroll`; that is done for `amt` and not for the pin, because a pin has
|
||||
to be *applied* when a row is appended -- between frames, with no
|
||||
painter in hand -- so moving it needs either a fourth `Widget` method or
|
||||
a parameter on `apply_scroll`. Nothing external edits a pin today (the
|
||||
transcript sets it once at construction and calls `jump_to_end` on the
|
||||
span for the rest), so this is a design question rather than a missing
|
||||
capability.
|
||||
- [ ] **A read-only text display has no widget of its own — P0's bench
|
||||
report area is a `TextEdit` standing in for one (2026-09-05).** The only
|
||||
way to get selectable text on screen today is `.editable(...)` plus
|
||||
`.attr::<Selectable>(())` (`Selectable` is only implemented for
|
||||
`TextEdit`, `iris/src/attr.rs`), which also makes the field focusable —
|
||||
tapping the bench report opens the soft keyboard over text nothing lets
|
||||
you type into. Harmless for a bench-only debug screen (not fixed this
|
||||
pass), but a real "selectable, not editable" text primitive would
|
||||
remove the keyboard side effect and is worth having before another
|
||||
screen wants the same thing (P1's own transcript rows already read
|
||||
their content from a `TextEdit` for the same reason).
|
||||
- [ ] **Colours are not in a defined colour space. Fix this before a
|
||||
styling pass.** Both render backends prefer an sRGB surface
|
||||
(`default/render.rs` and `android/render.rs`), while `fs_main` returns
|
||||
`unpack4x8unorm` palette and image bytes unchanged. An sRGB attachment
|
||||
treats those values as linear and encodes them again: Mocha Crust
|
||||
(17,17,27) became (73,73,91) in a desktop screenshot measured on
|
||||
2026-09-06.
|
||||
|
||||
## Build
|
||||
The earlier entry called this desktop-only because the Android picture
|
||||
looked right. That was not a measurement: neither the startup line nor
|
||||
the diagnostics report records the selected surface format, and the
|
||||
Android backend contains the identical preference and shader path. A
|
||||
device exposing only a non-sRGB surface can happen to hide the bug; it
|
||||
does not make the pipeline correct.
|
||||
|
||||
- [ ] **Positions as a single float per scroll.** Iris raised, and half
|
||||
rejected, letting a scroll update one float rather than positions:
|
||||
input handling cares about most elements in a list, so absolute
|
||||
positions must be computed on the CPU anyway. LAYOUT.md's design
|
||||
already lands here (GPU walks the chain, CPU resolves on demand for
|
||||
hit tests). Keep the CPU resolution lazy and per query; do not
|
||||
materialise every row's absolute position per frame.
|
||||
- [ ] **Animations, last.** Cosmetic, so after everything above. Must be
|
||||
**modular — a piece of the library rather than a core part forced into
|
||||
everything, the same way input is**. Whatever the mechanism, a widget
|
||||
that does not animate must pay nothing and import nothing for it.
|
||||
|
||||
## Found by P1a (2026-09-06)
|
||||
|
||||
- [ ] **Desktop colours are washed out: the winit surface is sRGB and
|
||||
the shader writes the palette's bytes as linear.** Mocha Crust
|
||||
(17,17,27) is drawn as (73,73,91), measured off
|
||||
`run-headless.sh --shot`. Android is correct, so this is the surface
|
||||
format rather than the palette -- but it makes the desktop build
|
||||
useless as a colour reference, which is exactly what P1a needed it for
|
||||
when the emulator could not draw glyphs.
|
||||
- [ ] **The bench report pane draws over the transcript rows instead of
|
||||
replacing them.** Visible on the emulator for the first time now that
|
||||
glyphs render there (`/tmp/emu-final.png`, 2026-09-06): after a bench
|
||||
run the report's lines and the transcript's occupy the same rows in the
|
||||
top third of the screen, both legible, neither on top. Pre-existing --
|
||||
the same overlap is in a screenshot taken before the move-slot fix -- so
|
||||
it is its own item, most likely the report pane not masking or not
|
||||
claiming its region.
|
||||
|
||||
## Found by P1b (2026-09-06), all with a headless repro
|
||||
|
||||
Each was found by looking at `iris/run-headless.sh transcript -- -p
|
||||
transcript-ui` rather than at a diff. docs/RUST.md's P1b box has the
|
||||
fuller account.
|
||||
|
||||
**No entry here is worked around any more** (Iris, 2026-09-08: "All of
|
||||
those should be fixed. There should never be workaround code. Do the
|
||||
same for those; fix them if they're trivial, diagnose and report if
|
||||
not."). Two are fixed and ticked; the two that are left are missing
|
||||
*capabilities* rather than defects being dodged, and each carries its
|
||||
diagnosis and what building it actually costs.
|
||||
|
||||
- [ ] **No overflow ellipsis.** `TextAttrs` can wrap or not wrap; there is
|
||||
no "one line, ellipsised" the way `maxLines = 1` + `TextOverflow.
|
||||
Ellipsis` gives Compose. A tool card's summary is clipped instead, so
|
||||
nothing on screen says it was cut. Whichever end is cut has to be a
|
||||
choice when this lands: a path is identified by its tail, a command by
|
||||
its head.
|
||||
|
||||
**Diagnosed 2026-09-08, and it is not trivial.** parley has no
|
||||
ellipsis of its own (checked: nothing in the vendored crates), so iris
|
||||
would build it, and the shape that looks easy is the one that breaks
|
||||
something. The easy half really is easy: shape at
|
||||
`max_advance = width - ellipsis_advance` with wrapping on, take line
|
||||
0's `text_range()`, and re-shape `text[..end].trim_end() + "…"` with
|
||||
wrapping off -- parley's own line breaker finds the cut, so nothing
|
||||
here counts glyph advances by hand. The hard half is that
|
||||
`TextBuffer` has exactly one string and everything addresses it by
|
||||
byte offset: the inline spans that carry a fence's colours and a
|
||||
link's range, `TextEditCtx::byte_at` (which turns a tap into a byte to
|
||||
match a link against), `Selection`'s `select`/`selected_text`, and
|
||||
`RowBlocks::apply_delta`. Truncating the buffer moves every one of
|
||||
those. So the real work is giving `TextBuffer` a **displayed** string
|
||||
distinct from its source, with one mapping from display byte to source
|
||||
byte that all of those go through -- worth doing, and not a
|
||||
by-the-way. Doing it only for text that is neither editable nor
|
||||
selectable would avoid all of that and is exactly the kind of
|
||||
exemption that comes back later.
|
||||
|
||||
It also wants an API change while it is open: `TextAttrs::wrap: bool`
|
||||
cannot say three states. Something like `Overflow::{Wrap, Clip,
|
||||
Ellipsis(End)}` replaces it, with `End::{Head, Tail}` making
|
||||
UI_RULES's "choose which end to truncate" a thing a caller must
|
||||
answer rather than a default nobody reads.
|
||||
- [ ] **A tool card's text is not selectable.** `Selection` is keyed
|
||||
`(RowKey, block index)` and a card has no markdown blocks, so nothing in
|
||||
a card registers. Compose's `SelectionContainer` covers tool output,
|
||||
which is the text people most want to copy.
|
||||
|
||||
**Diagnosed 2026-09-08: mechanical, but more than a sitting.** There
|
||||
is no key collision to design around, which was the open question:
|
||||
a `TranscriptRow::Tools` has *only* cards and no markdown blocks at
|
||||
all, so a card is free to number its own texts from 0 in reading
|
||||
order. What it costs is the registration lifecycle rather than the
|
||||
key. Each card's `TextEdit`s have to `Selection::register` as they are
|
||||
built and `unregister` when they are not -- and a card is rebuilt from
|
||||
several directions (`redraw_card` when a result arrives,
|
||||
`Shared::set_content` when the group is toggled or a call joins the
|
||||
run, and the per-card `WidgetPtr` swap), each of which frees widgets
|
||||
the map would otherwise still point at. That is the exact shape of the
|
||||
crash `Selection::clear`'s doc records from
|
||||
review, 2026-09-06: a handle in that map outliving the widget
|
||||
panics on the *next* long press, somewhere else entirely. So the work
|
||||
is a per-card base index with a stride (and a `debug_assert` that a
|
||||
card stays inside it), one register/unregister path that every rebuild
|
||||
route goes through, and a test per route that a rebuilt card leaves no
|
||||
stale handle behind.
|
||||
|
||||
## Warnings standing in the bench build (2026-09-08)
|
||||
|
||||
Seen while checking `cargo ndk -t arm64-v8a check --lib
|
||||
--no-default-features --features "transcript-screen bench"` from
|
||||
`app-rust/`, and left rather than silenced because it is a decision:
|
||||
|
||||
- [ ] **`PlatformHandle::show_diagnostics_overlay` has no caller.** It
|
||||
and the ~60 lines of `IrisView.showDiagnosticsOverlay` behind it are a
|
||||
plain-`TextView` overlay with Copy and Close, drawn over whatever iris
|
||||
is doing -- built so a report can be read *even if iris itself has
|
||||
stopped drawing*, which is the one case the in-iris diagnostics pane
|
||||
that replaced it cannot cover. So this is a live escape hatch nobody
|
||||
calls, not dead code: deleting both halves clears the warning and
|
||||
removes the fallback, and wiring it back to something is a product
|
||||
decision (Iris has no `logcat` on her phone). Ask before doing either.
|
||||
Done means defining one convention for palette bytes, decoded images,
|
||||
colour emoji and the clear colour, then converting exactly once for the
|
||||
selected target. Record the selected format in diagnostics, and add a GPU
|
||||
test that draws known non-black, non-white pixels into an sRGB target and
|
||||
reads the stored bytes back; screenshots from desktop and Android then
|
||||
confirm the same Catppuccin values rather than serving as the definition.
|
||||
## Build (for the port)
|
||||
|
||||
Widgets `RUST.md`'s "The port, in order (decided 2026-09-05)" needs and
|
||||
iris does not have yet, one entry per gap, named against the P-step that
|
||||
first needs it. Move an entry up to "Fix" or tick it in place once built;
|
||||
do not duplicate it there.
|
||||
first needs it. Move an entry up to "Fix" if it becomes a current defect;
|
||||
delete it once built rather than duplicating it there.
|
||||
|
||||
- [ ] **A history-paging cushion measured in on-screen viewports, not a
|
||||
row count.** (**P1**.) `iris::widget::List` has no equivalent of the
|
||||
Compose app's `HISTORY_SCREENS` — AGENTS.md's "Things that have
|
||||
bitten" is explicit that a fixed row count under-fills a screen on a
|
||||
tool-heavy transcript and over-fills one on a text-heavy one, so
|
||||
whatever loads the next page has to ask the list how many viewports
|
||||
are actually on screen, not assume a constant.
|
||||
- [ ] **A scaled thumbnail/image widget for an in-transcript image.**
|
||||
(**P1**.) `SessionImage.kt`'s bitmap decode-and-downscale has no iris
|
||||
counterpart; iris's own image widget (used by `bench_images.rs`) draws
|
||||
a loaded texture but does nothing about sourcing or scaling one from a
|
||||
server-produced attachment.
|
||||
- [ ] **Selectable, read-only text.** P0's report and P1's transcript rows
|
||||
use `TextEdit` because `Selectable` is implemented only for it. That
|
||||
makes prose focusable and opens the IME over text that cannot be edited.
|
||||
A display widget needs the same selection geometry and clipboard path
|
||||
without a text-input accessibility role or keyboard focus.
|
||||
- [ ] **Overflow ellipsis with an explicit retained end.** `TextAttrs` can
|
||||
only wrap or clip, so a tool summary is cut with no mark. Parley has no
|
||||
ellipsis primitive; use its line breaker to find the cut, but keep source
|
||||
and displayed strings distinct with one byte mapping shared by spans,
|
||||
links, selection and editing. Replace `wrap: bool` with an enum that can
|
||||
say wrap, clip, head ellipsis and tail ellipsis—the caller must choose
|
||||
because a command is identified by its head and a path by its tail.
|
||||
- [ ] **Expose the distance from a `LazySpan` viewport to its unloaded
|
||||
edge.** (**P1**.) `viewport_len` and the visible extents are already
|
||||
measured internally, but a paging caller cannot ask whether it is within
|
||||
the Compose app's six-viewport `HISTORY_SCREENS` cushion. The API should
|
||||
answer in pixels or viewport multiples, never rows: a row ranges from one
|
||||
line to a screen, so a fixed row count is not a distance.
|
||||
- [ ] **Let an image fit a bounded box while preserving its aspect ratio.**
|
||||
(**P1**.) `Image` currently always reports and draws the decoded texture's
|
||||
natural pixel size. Decoding and fetching a server-produced attachment
|
||||
belong in `app-rust`; iris only owes the generic fit/scale widget used to
|
||||
draw its thumbnail.
|
||||
- [ ] **Per-range backgrounds for rich text.** (**P1**.) Inline code is
|
||||
already monospace and coloured, but matching Compose's chip also needs
|
||||
the glyph run's boxes so a surface can be drawn behind exactly that byte
|
||||
range. `TextEdit` already computes the same geometry internally for its
|
||||
selection highlight; expose one shared primitive rather than giving the
|
||||
app a second text-layout path.
|
||||
- [ ] **A modal/dialog primitive.** (**P1**, reused by **P3** and
|
||||
**P5**.) Needed for the session settings dialog, `UsageDialog`'s
|
||||
equivalent, and the delete-with-`deleteForeign` confirmation with its
|
||||
@@ -184,23 +77,23 @@ do not duplicate it there.
|
||||
- [ ] **A horizontal gauge/bar widget.** (**P1**.) For
|
||||
`SessionUsageBar`'s equivalent — a bounded fill reflecting a fraction,
|
||||
nothing fancier.
|
||||
- [ ] **A `BusyItem` equivalent: a dimmed row carrying an operation
|
||||
label that does not block its list's own scroll/drag.** (**P3**.) The
|
||||
Compose version tried an overlay first and it swallowed the drag along
|
||||
with the tap (AGENTS.md's "Shared appearance") — worth not repeating
|
||||
that attempt in iris before building the row-level version directly.
|
||||
- [ ] **A toggle switch.** (**P3**.) For the delete dialog's
|
||||
`deleteForeign` control; iris has no switch/checkbox widget yet as far
|
||||
as this pass found.
|
||||
|
||||
## Reconsider
|
||||
## Later
|
||||
|
||||
- [ ] **`WidgetView`.** Iris is unsure of it: what she wants is an easy way
|
||||
to compose a widget from others (a button is the main case). With
|
||||
sizing folded into `draw`, composing may be easy enough that `View` is
|
||||
redundant. Decide after the layout change lands, by writing a button
|
||||
both ways and keeping the one that is shorter to explain; delete the
|
||||
other rather than keeping two ways.
|
||||
- [ ] **Property/content animations.** Cosmetic, so after correctness and
|
||||
parity. Keep them modular, like input; scrolling already animates through
|
||||
`Widget::tick` and `UiData::animate`. A widget that does not opt in must
|
||||
pay nothing and import nothing for them.
|
||||
|
||||
- [ ] **Remove `WidgetView` unless a real composite adopts it first.** The
|
||||
layout change this decision was waiting for has landed. Every composite
|
||||
in `app-rust/src/ui` now uses ordinary child handles plus a returned root;
|
||||
`WidgetView` and its derive are used only by `iris/examples/view.rs`.
|
||||
Today it demonstrates itself rather than shortening production code, so
|
||||
deletion is the concrete default—not another parallel composition style.
|
||||
|
||||
- [ ] **A `Stack` that chooses its mask the way it chooses its size
|
||||
(Iris, 2026-09-08).** She asked whether `masked_by` deserves to exist:
|
||||
@@ -215,14 +108,8 @@ do not duplicate it there.
|
||||
rounded panel and then cuts its content square. Both other call sites
|
||||
(`row.rs`'s fence, `tool.rs`'s raw output) are rounded, which is why
|
||||
the method stands for now.
|
||||
Her suggestion for removing it properly: **`Stack` already names where
|
||||
its size comes from (`StackSize::Child(n)`); let it name where its
|
||||
*mask* comes from the same way.** Then `.background(x)` is the one way
|
||||
to put a surface behind something, and clipping to that surface is a
|
||||
property of the stack rather than a second wrapper -- `masked_by` goes,
|
||||
and `Masked::shape` with it. Worth checking while designing it: what a
|
||||
stack with no mask child means (today's behaviour), whether the mask
|
||||
child must also have been *drawn* first (`set_mask_to_widget` requires
|
||||
it, and `Stack` draws in order, so naming child 0 is safe and naming a
|
||||
later one is not), and what happens when the named child is the same
|
||||
one the size comes from.
|
||||
Let `Stack` name the mask child the way `StackSize::Child(n)` names the
|
||||
sizing child. Then `.background(x)` remains the one way to add a surface
|
||||
and clipping to it is a stack property; the named mask child must have
|
||||
drawn before any child that uses it. Once that exists, delete
|
||||
`masked_by` and `Masked::shape` rather than retaining two APIs.
|
||||
Reference in new issue
Block a user