From 3c80d9d696342bce2a11e662d6232930ebe8fb50 Mon Sep 17 00:00:00 2001 From: iris <2+iris@noreply.localhost> Date: Mon, 7 Sep 2026 20:58:58 -0400 Subject: [PATCH] iris bench: a Trace switch for the input/frame diagnostics, and the report says when it was on `iris::diagnostics::set_trace` landed with nothing to press it. It is the bench header's fourth control now, reading `Trace off` or `Trace on` -- a toggle whose own appearance never changes is a button that looks like it did nothing. Its accessibility label stays the fixed "Trace input and frames", because that is what `run-bench.sh` and `ui-trace --do "tap '...'"` find it by and a control that renames itself when pressed is one no script can find twice. Pressing it rebuilds the header and shows the diagnostics pane, so the state is on screen at the moment of the press. Both reports carry `trace_line`, from the flag read at the *start* of what is being reported as well as at the end: the switch is on screen while a benchmark runs, so "somebody moved it half way through" is a state that happens, and reported as either "on" or "off" it would be a confident sentence about a log covering half the run. The row's type size is one constant for all four labels and drops from 18 to 13: with a fourth control the labels overlapped each other on a 1080px screen. Shrinking one label to fit is what the UI rules forbid; resizing the row is a layout decision and all four still match. Checked on the emulator: the switch flips its own text and colour, the pane reads "input/frame trace: on", and `iris::frame`/`iris::input` lines appear in the ring only after it is pressed. Co-Authored-By: Claude Fable 5.1 --- docs/RUST.md | 22 +++-- iris/android-app/src/bench_client.rs | 119 +++++++++++++++++++++++++-- 2 files changed, 127 insertions(+), 14 deletions(-) diff --git a/docs/RUST.md b/docs/RUST.md index c7c885e..66fc45b 100644 --- a/docs/RUST.md +++ b/docs/RUST.md @@ -259,11 +259,15 @@ unconditionally `true` by design ("the ring wants everything"), so a `log::Level::Debug` line reaches the ring regardless of what this instrument would prefer -- the gate has to be a crate-level flag, checked before `log::debug!` is even reached, and that is what `trace_enabled()` -is. **Not wired to a button yet**: the Diagnostics pane that would hold -the switch is in `iris/android-app/src/bench_client.rs`, which another -agent had open at the same time this was written; `set_trace` is the whole -surface a control needs, so wiring one is a follow-up for whoever is free -to touch that file next. +is. **The switch is the bench header's fourth control**, beside Run +benchmark / Copy report / Diagnostics: it reads `Trace off` or `Trace on`, +because a toggle whose own appearance never changes is a button that looks +like it did nothing. Its accessibility label stays the fixed `Trace input +and frames` -- that is what `run-bench.sh` and `ui-trace --do "tap '...'"` +find it by, and a control that renames itself when pressed is one no +script can find twice. Pressing it rebuilds the header (the same +`bench_controls` path `on_insets_changed` already uses) and shows the +diagnostics pane, so the state is on screen at the moment of the press. **Why default off, and why the ring's size is the actual constraint**: the ring is 2000 lines / 256 KiB @@ -273,8 +277,12 @@ seconds, so a caller turns tracing on only for the length of whatever is being investigated, not for a whole session. This is also why the report should say at its top whether tracing was on -- a caller reading `iris::diagnostics::trace_enabled()` when building the report can print -that; nothing here does it automatically since nothing here owns the -report's own header. +that. Both reports do: `bench_client::trace_line` is the one wording, and +it takes the flag **read at the start of the run as well as at the end**, +so a switch flipped half way through is reported as exactly that rather +than as a confident "on" about a log covering half the run. Three states, +because that third one happens -- the switch is on screen while a +benchmark runs. **D1 from `docs/REVIEW-2026-09-07.md`**: the review found that this gate existed (as `iris/src/diagnostics.rs`, uncommitted at the time) but four diff --git a/iris/android-app/src/bench_client.rs b/iris/android-app/src/bench_client.rs index 924ca69..0c59ecf 100644 --- a/iris/android-app/src/bench_client.rs +++ b/iris/android-app/src/bench_client.rs @@ -415,6 +415,29 @@ const KEYBOARD_DIAGNOSTICS_DELAY_MS: u64 = 500; type Rsc = AndroidRsc; +/// What a report says about the `iris::input`/`iris::frame` trace, from +/// the flag read at the start of what is being reported and again at the +/// end. +/// +/// Three answers rather than two. Those lines are default-off and the +/// switch that turns them on is on screen while a benchmark runs, so +/// "somebody moved it half way through" is a state that actually happens +/// -- and reported as either "on" or "off" it is a confident sentence +/// about a log that only covers part of the run. The "on" wording also +/// says what it costs, because a traced run fills the ring in seconds and +/// a reader looking at a log with nothing else in it should know why. +fn trace_line(at_start: bool, at_end: bool) -> String { + match (at_start, at_end) { + (true, true) => "input/frame trace: on (iris::input and iris::frame lines are in \ + the app log, and a traced run fills the ring in seconds)" + .to_string(), + (false, false) => "input/frame trace: off".to_string(), + _ => "input/frame trace: switched during this run, so those lines cover only part \ + of it" + .to_string(), + } +} + /// The header row's own backdrop -- see `bench_controls`'s doc comment on /// why it needs one at all. A dark neutral rather than pure black /// (`android::render::CLEAR_COLOR`) so the row reads as a distinct panel @@ -442,6 +465,18 @@ const HEADER_SURFACE: UiColor = UiColor::new(28, 28, 34, 255); /// pixels) to `dp(...)` (IRIS_TODO.md's density-independent length unit), /// so the row's reserved height in the outer `Span::DOWN` /// (`AndroidAppState::new`) matches what is actually painted. +/// The size every label in the header row is drawn at. +/// +/// One constant for all four rather than a number per button, because the +/// whole row has to be sized together: it was 18 with three controls, and +/// adding the trace switch made four labels overlap each other on a +/// 1080px screen. Shrinking *one* label to fit is what the UI rules +/// forbid -- a label a different size from its neighbours for a reason the +/// reader cannot see; changing the row's own type size is a layout +/// decision, and all four still match. Whoever adds a fifth control has +/// one number to reconsider rather than four. +const HEADER_TEXT: f32 = 13.0; + fn bench_controls(rsc: &mut Rsc, top_pad: f32) -> StrongWidget { let run_rect = rect(Color::rgb(40, 70, 40)) .on( @@ -453,7 +488,9 @@ fn bench_controls(rsc: &mut Rsc, top_pad: f32) -> StrongWidget { .label("Run benchmark"); let run = ( run_rect, - wtext("Run benchmark").size(18).text_align(Align::CENTER), + wtext("Run benchmark") + .size(HEADER_TEXT) + .text_align(Align::CENTER), ) .stack() .pad(dp(8)) @@ -469,7 +506,9 @@ fn bench_controls(rsc: &mut Rsc, top_pad: f32) -> StrongWidget { .label("Copy report"); let copy = ( copy_rect, - wtext("Copy report").size(18).text_align(Align::CENTER), + wtext("Copy report") + .size(HEADER_TEXT) + .text_align(Align::CENTER), ) .stack() .pad(dp(8)) @@ -485,13 +524,49 @@ fn bench_controls(rsc: &mut Rsc, top_pad: f32) -> StrongWidget { .label("Diagnostics"); let diagnostics = ( diag_rect, - wtext("Diagnostics").size(18).text_align(Align::CENTER), + wtext("Diagnostics") + .size(HEADER_TEXT) + .text_align(Align::CENTER), ) .stack() .pad(dp(8)) .add(rsc); - let buttons = (run, copy, diagnostics).span(Dir::RIGHT).add(rsc); + // A switch rather than a button, so its own appearance says which + // state it is in: the two `iris::input`/`iris::frame` targets are + // default-off (`iris::diagnostics`'s module doc) because a 120Hz + // session fills the 2000-line ring in seconds, so "is it on right + // now" is the question somebody has while looking at a log that is + // either full of trace or has none. + // + // The visible text carries the state and the accessibility label does + // not, deliberately: the label is also what `run-bench.sh` taps by + // name, and a control that renames itself when pressed is one no + // script can find twice. + let tracing = iris::diagnostics::trace_enabled(); + let trace_rect = rect(if tracing { + Color::rgb(90, 70, 30) + } else { + Color::rgb(50, 50, 60) + }) + .on( + CursorSense::click(), + |ctx: EventIdCtx<'_, Rsc, _, _>, rsc: &mut Rsc| { + ctx.state.toggle_trace(rsc); + }, + ) + .label("Trace input and frames"); + let trace = ( + trace_rect, + wtext(if tracing { "Trace on" } else { "Trace off" }) + .size(HEADER_TEXT) + .text_align(Align::CENTER), + ) + .stack() + .pad(dp(8)) + .add(rsc); + + let buttons = (run, copy, diagnostics, trace).span(Dir::RIGHT).add(rsc); (rect(HEADER_SURFACE), buttons) .stack() @@ -526,6 +601,25 @@ impl BenchClient { self.last_report = Some(report); } + /// Turns the `iris::input`/`iris::frame` trace on or off, redraws the + /// switch that says so, and shows the pane that now reports it. + /// + /// Showing the pane is the point rather than a convenience: this is a + /// control whose whole effect is on what a *later* report says, so + /// putting the state on screen at the moment of the press is the only + /// thing that distinguishes it from a button that did nothing. + fn toggle_trace(&mut self, rsc: &mut Rsc) { + let on = !iris::diagnostics::trace_enabled(); + iris::diagnostics::set_trace(on); + log::info!( + "iris diagnostics: input/frame trace {}", + if on { "on" } else { "off" } + ); + let controls = bench_controls(rsc, self.last_top_pad); + (self.top_bar)(rsc).set(controls); + self.show_diagnostics(rsc); + } + /// The diagnostics report as text, with no side effect on what is on /// screen -- shared by the `Diagnostics` button (which shows it) and /// the keyboard-open capture (which only logs it), so the two can @@ -545,7 +639,11 @@ impl BenchClient { // composer up" cannot be told from "the listener never fired" // without it (`AndroidUiState::insets_report`). format!( - "{renderer}\n{}\n{}\n{}", + "{renderer}\n{}\n{}\n{}\n{}", + trace_line( + iris::diagnostics::trace_enabled(), + iris::diagnostics::trace_enabled() + ), self.android_state().insets_report(), // Which server this build talks to, and what to do when the // answer is "none" -- the bench itself opens a checked-in @@ -621,6 +719,11 @@ impl BenchClient { .and_then(|p| p.refresh_rate_hz()) .unwrap_or(60.0); let cpu_start = process_cpu_ms(); + // Read at the start as well as the end, because the switch is on + // screen while a run is going: a report that only asked afterwards + // would say "on" about a run whose first half has no trace in it + // -- the inferred answer presented as the measured one. + let trace_at_start = iris::diagnostics::trace_enabled(); let run_started_at = Instant::now(); rsc.spawn_task(async move |mut ctx| { @@ -718,9 +821,11 @@ impl BenchClient { " type: {} characters inserted then deleted, one per {TYPE_CHAR_MS}ms", TYPE_TEXT.chars().count() ); + let traced = trace_line(trace_at_start, iris::diagnostics::trace_enabled()); let report = format!( - "iris bench report\n{per_phase}{frames_block}\n\nbench:\n{fling_line}\n\ - {scroll_line}\n{type_line}\n{keyboard}\n{cpu_line}\n{rss_line}\n{battery}" + "iris bench report\n{traced}\n{per_phase}{frames_block}\n\nbench:\n\ + {fling_line}\n{scroll_line}\n{type_line}\n{keyboard}\n{cpu_line}\n\ + {rss_line}\n{battery}" ); log::info!("iris bench report: {report}"); state.report_display.edit(rsc).set(&report);