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);