Repository navigation
battery: say whose charge it is on all seven surfaces, not three - #2216
Conversation
Refs #2208, reported by @pipiche38 from a code read. Every claim in that report checks out at the lines given. Three surfaces were routed through LiveConsoleReadout in #2076. Five more kept reading the raw field, and two of those had no gate at all. The mechanism is one shared mutable field. LiveState.batteryPct is the WHOOP's and is never cleared, deliberately; clearBiometrics nils ouraBatteryPct and leaves it. `connected` goes true the moment ANY source streams, including the first live HR from a ring. So both halves of the old gate pass and the screen draws the strap's charge under an active ring. Seven surfaces now, in one pass rather than a third: Liquid Today header ring Liquid Today sources row Classic Today battery row Android Today header ring Android Today sources pill macOS sidebar macOS menu-bar stat The last two are the ones the report flagged and left out, and they are worse than the five it filed rather than equivalent. Neither gated on `connected` at all, so they showed the strap's last percentage with nothing connected to anything, needing no ring to reproduce. The sidebar is the sharper case: its only honest branch is the nil one, and since batteryPct is never cleared that branch stopped being reachable the first time a WHOOP ever connected. "Strap not connected" was dead text on any install that had paired a strap once. Today cannot ask the device registry. It observes BLEManager and NOT AppModel on purpose, because AppModel publishes on the 1 Hz heart-rate tick and observing it would re-render the whole screen every second. Repository.activeDeviceIsOura exists but answers the wrong question: a Polar or Garmin strap is neither Oura nor WHOOP, and the strap's charge is just as wrong under one. So activeIsWhoop is published on LiveState, beside the batteryPct it qualifies, written in ONE place from the same activeDeviceId transition that decides which live source runs. It is set before the Apple Watch branch returns, or a watch would read as a WHOOP. It defaults to true, matching the seam's WHOOP-first default, and clearBiometrics does not touch it: which device is active is not a biometric and does not stop being true when a link drops. StrapBatteryDisplay.resolve takes activeIsWhoop with NO default value. A defaulted flag is one a future call site can forget, and forgetting it reinstates this bug in a form that still compiles. charging and the #713 runtime estimate are gated the same way. Both are strap facts: charging is the strap's BATTERY_LEVEL event and the estimate is banked from strap SoC samples. Tests: four new rows on the Swift truth table, two vectors on the Kotlin seam, and every existing Swift row carries activeIsWhoop: true so the old meaning is preserved rather than rewritten. Negative control: forcing the Kotlin seam to ignore the flag fails 4, including two that predate this change. com.noop.ui 1054 tests 0 failures, compileFullDebugKotlin clean. Apple targets compile on CI. Left as the report left them: the Settings strap card and the onboarding line, which sit under explicitly WHOOP-labelled headings.
CI caught a second StrapBatteryDisplay.resolve call, inside `#if DEBUG` for DemoSyncHarness. It stands in for a connected WHOOP, so it answers true. Worth recording why this was missed and why it still failed loudly. Missed because my sweep grepped `.resolve(connected:` on one line, and this call puts `connected:` on the line after the open paren. A single-line pattern cannot see a multi-line call, and I used one while fixing a bug whose whole shape is "the previous sweep missed some readers". Re-swept with a multi-line matcher; the only other hit is RecordingState.resolve, a different type. Failed loudly because the parameter has no default value. That choice was made so a future call site could not forget the flag and silently reinstate the bug, and the first thing it caught was me forgetting it. A defaulted parameter would have compiled and left the demo harness on the old behaviour.
Second re-review pass on #2216, and this one was a bug I had introduced rather than one I was fixing. I routed the Android sources pill through LiveConsoleReadout.batteryPercent, which SUBSTITUTES the ring's charge when the active device is not a WHOOP. That is right for a readout naming the active device, which is what the Live Console and the Devices list are, and wrong here. This value lands in the SourceRow badged `today_source_whoop`, and it also feeds that row's `present` flag: val whoopPresent = (footer.whoopDays ?: 0) > 0 || strapBatteryPct != null So the substitution would have put the ring's percentage under a WHOOP label and asserted a WHOOP source that is not there. That is worse than the stale reading being fixed: the old bug at least showed a number that really was the strap's. Gated instead, matching the six other surfaces and the iOS row beside it, which is labelled "Strap battery" and hides for the same reason. Nothing is the honest answer for a strap that is not the active device. The report suggested the substitution too, and was wrong for the same reason mine was: the seam is correct for a readout that names whichever device is active, and a WHOOP-labelled row is not one. Applying a seam mechanically is what both of us did; what the destination MEANS is the part that has to be read. ouraBatteryPct is no longer collected in TodayScreen, since nothing there reads it now. com.noop.ui 1054 tests 0 failures, compileFullDebugKotlin clean.
|
Second re-review pass, and it found a bug I had introduced rather than one I was fixing. I routed the Android sources pill through the seam, and the seam is wrong for that destination.
val whoopPresent = (footer.whoopDays ?: 0) > 0 || strapBatteryPct != nullSo the substitution would have put the ring's percentage under a WHOOP label and asserted a WHOOP source that is not there. That is worse than the reading being fixed: the old bug at least showed a number that really was the strap's. Gated instead, matching the six other surfaces and the iOS row directly beside it, which is labelled "Strap battery" and hides for the same reason. Nothing is the honest answer for a strap that is not active. @pipiche38, your suggested fix had this too, and for the same reason mine did. The seam is correct for a readout that names the active device; a WHOOP-labelled row is not one. Applying it mechanically is what we both did. What the destination means is the part that has to be read, and I did not read it until this pass. Not a criticism of the report, which is otherwise the most precise one I have had this week; just worth naming so the next person routing through that seam checks where the number lands.
|
Third re-review pass on #2216. No behaviour change. Two things came out of it. First, the sweep is now enumerated rather than asserted. Every reader of the strap battery field, both platforms: seven fixed here, six already correct because they route through LiveConsoleReadout, four deliberately out of scope under WHOOP-labelled headings (Settings and onboarding on each platform), one diagnostic in Test Centre where the raw strap SoC is the thing being inspected, and three that are not readouts at all (an internal BLEManager read and two LaunchedEffect keys). Nothing is left unaccounted for. Second, and the reason this keeps happening: `batteryPct` carried NO documentation at all. A field that is the strap's alone, is never cleared by design, and cannot be qualified by the `connected` flag sitting next to it, was a bare declaration. That is how it ended up read raw by seven surfaces across two separate reports. It now says all three things, and says which of the two correct readings a surface needs: LiveConsoleReadout.batteryPercent for a readout naming the ACTIVE device, or pairing with activeIsWhoop and showing nothing for a readout labelled as the strap's. Getting that choice wrong is the bug the previous pass found in my own change, so the distinction is written where a reader meets it rather than in a commit message nobody will look up.
|
Same shape as the one I had built for this (#2214, closing it as superseded — yours carries the Kotlin vectors, the negative control and one writer for One delta left between the two, and it's your call. A streaming ring now draws the offline glyph on the Today header. Everything else matched line for line after your second pass, sidebar and menu bar included. Not hardware-checked on my side yet; I'll run the ring + WHOOP pair against whichever shape lands. |
… offline @pipiche38's review of #2216, and they are right. The previous shape traded one false claim for another on the same screen. StrapBatteryDisplay.resolve returned .offline for a non-WHOOP active device, so a wearer with a ring streaming 93% saw the crossed-out bolt on the Today header and VoiceOver read "Strap battery, strap not connected". That is a different wrong statement from the one reported in #2208, not a repair of it, and a ring-only wearer would meet it every day, which is exactly who the Ring 5 report came from. .notActiveDevice is now its own case, and the header control is not drawn at all when it applies. Adding a case rather than reusing .offline means the compiler required both switches to answer, which is the property that already caught a missed call site earlier in this PR. Keeping .offline for a strap that IS active and disconnected, because that is a real claim worth making. The two answers were being collapsed, and a test now asserts they differ. Android matches: LiquidTodayHeader takes strapIsActiveDevice separately from batteryPct, and skips the ring entirely when it is false. Separate parameters on purpose: batteryPct says what the control reads, the flag says whether the control should exist. A null percentage while the strap IS active means "connected, no reading yet" and is worth drawing; a strap that is not the active device has nothing to say. Not taking the other option on offer. Showing the RING's charge on these surfaces would need "Ring battery" in nine locales and a ring-charging source, and @pipiche38 did not argue for it either. Hiding is what the two Today rows already do, so this is now one answer across all of them. com.noop.ui 1054 tests 0 failures, compileFullDebugKotlin clean.
|
You are right, and thank you for standing #2214 down in favour of this rather than the other way round. The delta you named was the thing wrong with mine. Fixed in e63ff26. The previous shape traded one false claim for another on the same screen.
Android matches. I took the hiding option rather than the ring's charge, which is the one you suggested as the honest middle and did not argue against. Your counter-argument holds: the label says "Strap battery", "Ring battery" is a nine-locale string, and ring charging would need its own source through If you want the ring's own charge on the brand-neutral surfaces later, that is a real feature rather than a bug fix, and it wants the label and the locales doing properly. On the seam note: agreed, and I did the same thing you did. Your issue said "route both Sweep is enumerated rather than asserted now: seven fixed here, six already correct through the seam, four out of scope under WHOOP-labelled headings, one Test Centre diagnostic where the raw strap SoC is the point, and three that are not readouts. Hardware would be welcome whenever you get to it. Nothing here is hardware-checked either, and the ring-plus-WHOOP pair is the case neither of us can reason our way to. |
…nstead of nothing (ryanbr#2208 second half) ryanbr#2208 stopped the Liquid Today header drawing the STRAP's charge under an active ring, and the control has shown nothing under a ring since: `StrapBatteryDisplay.resolve` answered `.notActiveDevice` for every non-WHOOP active device because it could not tell whose number it held. The ring's own charge was already there - `LiveState.ouraBatteryPct` on iOS and `SourceCoordinator.ouraBatteryPct` on Android, both cleared with the link (ryanbr#2075), both already resolved by `LiveConsoleReadout.batteryPercent` for the Live console and the Devices list. The header was the one surface still blind to it. - iOS `StrapBatteryDisplay.resolve` takes `ringPct` + `ringCharging` (no defaults, for the same reason `activeIsWhoop` has none): a non-WHOOP active device with a ring charge reported this link is `.charge(isRing: true)`; with none it stays `.notActiveDevice`, so a generic HR strap or a machine - which never write `ouraBatteryPct` - keep the control off the header exactly as before. Charging under a ring is the ring's charger state (`OuraWearState.charging`), never the strap's BATTERY_LEVEL bit. - `.charge` carries `isRing` so the VoiceOver label names the device the number belongs to: "Ring battery N percent(, charging)" - "Strap battery" over a ring's charge would be the ryanbr#2208 misattribution again, in the label instead of the number. Strings in every catalog locale. - Android: `HeaderBatteryDisplay.resolve`, a pure twin of the iOS resolver (minus the charging bit the Android ring never drew), replaces the `strapIsActiveDevice` gate + raw `batteryPct` pair on `LiquidTodayHeader`; `LiquidBatteryRing` takes `isRing` for its label (`today_ring_battery_percent`, every locale). Verification: `LiquidBatteryDisplayTests` 15/15 under `xcodebuild test` (Strand, macOS), four new ring cases; `HeaderBatteryDisplayOracleTest` 4/4 - the 60-line expected literal is the verbatim stdout of the Swift enum compiled standalone (`swiftc -O`) over the whole activeIsWhoop x connected x strap-% x ring-% grid, plus the named ryanbr#2208/ryanbr#2216 cases; `Strand` (macOS) and `NOOPiOS` build; `compileFullDebugKotlin` clean; `i18n_audit --ci` (both platforms) and `doc_comment_lint` green. Display-only - the values come from the same fields the Live console already shows on hardware. Refs ryanbr#2208, ryanbr#2216, ryanbr#2075. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QrsZczdGcUyQSJVuYZuwUF
…nstead of nothing (ryanbr#2208 second half) ryanbr#2208 stopped the Liquid Today header drawing the STRAP's charge under an active ring, and the control has shown nothing under a ring since: `StrapBatteryDisplay.resolve` answered `.notActiveDevice` for every non-WHOOP active device because it could not tell whose number it held. The ring's own charge was already there - `LiveState.ouraBatteryPct` on iOS and `SourceCoordinator.ouraBatteryPct` on Android, both cleared with the link (ryanbr#2075), both already resolved by `LiveConsoleReadout.batteryPercent` for the Live console and the Devices list. The header was the one surface still blind to it. - iOS `StrapBatteryDisplay.resolve` takes `ringPct` + `ringCharging` (no defaults, for the same reason `activeIsWhoop` has none): a non-WHOOP active device with a ring charge reported this link is `.charge(isRing: true)`; with none it stays `.notActiveDevice`, so a generic HR strap or a machine - which never write `ouraBatteryPct` - keep the control off the header exactly as before. Charging under a ring is the ring's charger state (`OuraWearState.charging`), never the strap's BATTERY_LEVEL bit. - `.charge` carries `isRing` so the VoiceOver label names the device the number belongs to: "Ring battery N percent(, charging)" - "Strap battery" over a ring's charge would be the ryanbr#2208 misattribution again, in the label instead of the number. Strings in every catalog locale. - Android: `HeaderBatteryDisplay.resolve`, a pure twin of the iOS resolver (minus the charging bit the Android ring never drew), replaces the `strapIsActiveDevice` gate + raw `batteryPct` pair on `LiquidTodayHeader`; `LiquidBatteryRing` takes `isRing` for its label (`today_ring_battery_percent`, every locale). Verification: `LiquidBatteryDisplayTests` 15/15 under `xcodebuild test` (Strand, macOS), four new ring cases; `HeaderBatteryDisplayOracleTest` 4/4 - the 60-line expected literal is the verbatim stdout of the Swift enum compiled standalone (`swiftc -O`) over the whole activeIsWhoop x connected x strap-% x ring-% grid, plus the named ryanbr#2208/ryanbr#2216 cases; `Strand` (macOS) and `NOOPiOS` build; `compileFullDebugKotlin` clean; `i18n_audit --ci` (both platforms) and `doc_comment_lint` green. Display-only - the values come from the same fields the Live console already shows on hardware. Refs ryanbr#2208, ryanbr#2216, ryanbr#2075. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QrsZczdGcUyQSJVuYZuwUF
#2208 stopped the Liquid Today header drawing the STRAP's charge under an active ring, and the control has shown nothing under a ring since: StrapBatteryDisplay.resolve answered notActiveDevice for every non-WHOOP active device because it could not tell whose number it held. The ring's own charge was already there, in LiveState.ouraBatteryPct and SourceCoordinator's twin, cleared with the link and already resolved for the Live console and the Devices list. The header was the one surface still blind to it. resolve now takes ringPct and ringCharging with no defaults, for the reason activeIsWhoop has none: a defaulted flag is one a future call site can forget, and forgetting it reinstates the bug in a form that still compiles. A non-WHOOP active device with a ring charge reported this link draws it; with none it stays notActiveDevice, so a generic HR strap or a machine keeps the control off the header exactly as before. charge carries isRing so the VoiceOver label names the device the number belongs to, since "Strap battery" over a ring's charge would be the same misattribution in the label instead of the number. Android gets HeaderBatteryDisplay.resolve, a pure twin minus the charging bit the Android ring never drew. Reviewed. The Kotlin oracle's provenance was checked rather than taken on trust: the Swift enum was extracted, compiled standalone and run over the same activeIsWhoop x connected x strap-% x ring-% grid, and all 60 rows matched the literal, before and after rebasing onto current main. Localisation was counted on both sides: 7 Android locale files, one per values dir that exists, and 9 xcstrings locales for both new keys. The accessibility label really does read isRing; the discard at the drawing site is a separate mapping where whose number it is cannot matter. Rebased onto current main and re-verified there: HeaderBatteryDisplayOracleTest 4/4, doc-comment lint clean, 5/5 in CI. Known parity gap, not a blocker: Android omits the charging bit, so a ring on a charger shows the percentage without the bolt. Author: Pipiche (@pipiche38). Refs #2208, #2216, #2075.
The header circle is the way into Devices from Today, but it was not drawn when the active device was neither a WHOOP nor a ring with a reported charge. Making a heart-rate strap active, or removing it while active, took the button away. It now always draws. With no charge to report it shows the Devices glyph and reads "Manage devices", so it still claims nothing about a strap nobody is wearing (ryanbr#2208, ryanbr#2216). The resolver's answers are unchanged; only what the control does with notActiveDevice moves. Android's header ring gets the same rule. Android is not compiled locally (no JDK on this machine). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Refs #2208, reported by @pipiche38 from a code read. Every claim in that report checks out at the lines given; the review comment there lists what I verified.
The mechanism
One shared mutable field.
LiveState.batteryPctis the WHOOP's and is never cleared, deliberately:clearBiometricsnilsouraBatteryPctand leaves it.connectedgoes true the moment any source streams, including the first live HR from a ring. So both halves of the old gate pass and the screen draws the strap's charge under an active ring.Seven surfaces, not five
Three were routed through the seam in #2076. This report found five more. I did seven.
The two extra are the ones the report flagged and left out as "arguably the same bug one screen over". They are worse than the five it filed: the macOS sidebar and the menu-bar stat gated on nothing at all, so they showed the strap's last percentage with nothing connected, needing no ring to reproduce.
The sidebar is the sharper case. Its only honest branch is the
nilone, and sincebatteryPctis never cleared, that branch stopped being reachable the first time a WHOOP ever connected. "Strap not connected" was dead text on any install that had paired a strap once.Leaving them would have made a third pass over one class inevitable, and the reason each pass keeps missing something is that the class is defined by a shared field rather than by anything visible at a call site.
Why a new field, given a stale field caused this
A fair objection, so the difference is worth stating.
batteryPcthas many writers (every source), no owner, and is deliberately never cleared.activeIsWhoophas exactly one writer, is overwritten on every active-device transition, and never needs clearing.It is published on
LiveStatebecause Today cannot ask the device registry.LiquidTodayViewobservesBLEManagerand notAppModelon purpose:AppModelpublishes on the 1 Hz heart-rate tick, so observing it would re-render the whole screen every second.Repository.activeDeviceIsOuraexists but answers the wrong question, since a Polar or Garmin strap is neither Oura nor WHOOP and the strap's charge is just as wrong under one.The writer is
SourceCoordinator.activeDeviceChanged, the sameactiveDeviceIdtransition that decides which live source runs. Set before the Apple Watch branch returns, or a watch would read as a WHOOP. Defaults to true, matching the seam's own WHOOP-first default for an unresolvable row.StrapBatteryDisplay.resolvetakes the flag with no default value. A defaulted flag is one a future call site can forget, and forgetting it reinstates this bug in a form that still compiles.chargingand the #713 runtime estimate are gated the same way: charging is the strap's BATTERY_LEVEL event, the estimate is banked from strap SoC samples.Tests
Four new rows on the Swift truth table, two vectors on the Kotlin seam. Every existing Swift row carries
activeIsWhoop: true, so the old meaning is preserved rather than rewritten.Negative control: forcing the Kotlin seam to ignore the flag fails 4, including two that predate this change.
LiveConsoleReadout.ktwas restored unchanged afterwards and is not in the diff.com.noop.ui: 1054 tests, 0 failures.compileFullDebugKotlinclean,doc_comment_lintclean. Apple targets compile on CI.Checked while re-reviewing
StrapBatteryDisplay.resolvecall sites; the other.resolve(hits areChargeDisplayandRecordingState.test-only-callsite|Packages/WhoopStoredrift tracked in Parity ledger fails on main, and --refresh-derived cannot repair it #2211, which reproduces on main.wireSourceCoordinatorcallsstart()when the store opens, andregistry.$activeDeviceIdemits on subscribe.Worth knowing, not fixed here
There are now several spellings of this one fact:
whoopIsActiveDeviceandactiveDeviceIsWhoopon the Android client,activeIsWhoopon the Android view model,LiveConsoleReadout.activeIsWhoopas the shared seam, and this. Consolidating them is its own change.Left as the report left them: the Settings strap card and the onboarding line, which sit under explicitly WHOOP-labelled headings.