From dc85890a9f13d17c68f7ad5f71a66f61e8599fea Mon Sep 17 00:00:00 2001 From: Nils Pukropp Date: Mon, 10 Aug 2026 01:10:13 +0200 Subject: [PATCH] docs: android review ledger, design tokens, test notes B14-B42 record the Android review the theme and lifecycle work came out of. DESIGN-TOKENS.md states what both clients agree on and where each keeps its copy. --- docs/BUGS.md | 98 ++++++++++++++++++++++++++++++++++++++ docs/DESIGN-TOKENS.md | 108 ++++++++++++++++++++++++++++++++++++++++++ docs/DEVELOPMENT.md | 32 +++++++++++++ docs/TESTING.md | 9 ++++ 4 files changed, 247 insertions(+) create mode 100644 docs/DESIGN-TOKENS.md diff --git a/docs/BUGS.md b/docs/BUGS.md index 3a994d6..11115bd 100644 --- a/docs/BUGS.md +++ b/docs/BUGS.md @@ -146,6 +146,104 @@ and [B44](#b44--r8-broke-the-app-under-instrumentation-in-five-places). ## Fixed +### B52 — "paint the accent as a gradient" did nothing on Android + +**Symptom.** The appearance page on Android offers a *Paint the accent as a +gradient* switch, stores it, and repaints the app when it is flipped — and +nothing on the screen changed either way. The same stored setting visibly +changed the desktop client, so one file meant two things depending on which +client read it. `PinkSky`, whose whole point is that it is a gradient before it +is a colour, arrived on Android as one flat pink. + +**Cause.** No gradient was ever drawn. Nothing under `android/app/src/main` +referenced `Brush`, `linearGradient` or `verticalGradient`. `accent_alt` reached +Compose only as `secondary`, where it fills tonal containers — so the second hue +existed in the scheme but never sat next to the first one. + +**Fix.** `ui/theme/Accent.kt` draws the ramp on the surfaces the desktop draws it +on: the primary button, the seek bar, the watch-progress bar under a tile and +under an episode row. Nothing there reads the toggle, because it does not need +to — `Palette::resolve` already sets `accent_alt` equal to `accent` when +gradients are off, so the ramp collapses to a flat fill by itself and the two +clients cannot disagree about what the setting means. + +Two surfaces are deliberately not ramps. A download in flight keeps a solid +`tertiary`, because it is drawn directly under the watch-progress bar of the same +row and two ramps in the same colour two pixels apart say nothing. The navigation +pill is solid `primary`: `NavigationSuiteScaffold` takes its indicator as a +`Color` with no slot to draw into, so a gradient there would mean reimplementing +the bar — but the accent itself is still nearer the desktop's rule than +`secondaryContainer`, which is the accent taken most of the way back to the page. + +**Verified.** `./gradlew :app:testDebugUnitTest :app:lintDebug`. Still wants a +device: the ramp is the one thing here a screenshot on Proton and on Latte +settles, and the panel-banding case the toggle exists for cannot be judged from +a build at all. + +### B51 — the Android client wore only part of the palette it was given + +**Symptom.** Every flavour looked half-applied on Android. The page was the +flavour's colour and the tiles on it were not: the library grid, the +continue-watching row, every episode row, every share, every download and the +mini transport were all a flat neutral grey that belonged to no flavour and did +not change when one was picked. Latte additionally drew white status-bar icons +onto its near-white page, and every cold start flashed a dark blue-grey before +the app appeared — including for viewers who had chosen Latte. + +**Cause.** Four, all in how the resolved palette reached the platform. + +`schemeOf` (`ui/theme/Theme.kt`) built a `ColorScheme` by copying +`darkColorScheme()`/`lightColorScheme()` and overriding 22 roles. Material names +roughly fifty. A role nobody overrides keeps Material's own baseline value, and +`Card`'s container is `surfaceContainerHighest` +(`FilledCardTokens.ContainerColor`), which was one of the twenty-seven nobody +overrode. `Card` is what draws every repeated surface in the app, so the app's +most common surface was the one surface that ignored the theme. `inverseSurface` +and `inverseOnSurface` — the snackbar, which is the client's only error channel +— and `scrim`, the dim behind every dialog, were in the same set. `surfaceTint` +was too, so raised surfaces were tinted with Material's baseline purple. + +`themes.xml` hardcoded `windowBackground` to `#1E1E2E`, which is Catppuccin +Mocha's base: not the shipped default, and not the viewer's choice either. It +also pinned `windowLightStatusBar` and `windowLightNavigationBar` to `false`, +and nothing anywhere synced either to `palette.light`. + +`ProtonStreamTheme` read the palette in a `LaunchedEffect` that hopped to +`Dispatchers.IO`, so the first composition was always painted in a hardcoded +dark default — the same flash-of-the-wrong-theme the desktop client had on +Windows, for the same reason. + +**Fix.** `schemeOf` maps every role Material names. Two colours the palette does +not name are derived in Rust so the blend behind them is the shared, tested one: +`elevated`, which continues the flavour's own `card` → `card_hover` step by +extrapolating past it — so a light flavour's ladder descends and a dark one's +climbs — and `danger_dim`, an error *container* rather than error ink. Both are +new fields on `PaletteRecord`. `outline` and `outlineVariant` were also swapped +to match what Material draws with each: the visible border takes `muted`, the +divider takes the fainter `border`. `surfaceTint` is transparent, because the +desktop draws no elevation tint and its rule is that the accent is the only +strong colour in the window. + +A new `stored_palette` bridge function resolves the stored choice without +building an engine — no SQLite, no Tokio runtime, no Keystore unlock — so +`MainActivity.onCreate` can seed the palette before `setContent` rather than one +frame after it. It also sets the window background and the system bars' icon +polarity from that palette, and re-applies both whenever the flavour changes, so +picking Latte turns the status-bar icons dark in the same frame the page turns +light. `themes.xml` keeps only the shipped default's background, as a first-ever +launch has no theme file to read. + +**Verified.** `cargo test --workspace --locked`, `cargo clippy --workspace +--all-targets -- -D warnings`, `./gradlew :app:testDebugUnitTest :app:lintDebug` +(lint clean; the two unmatched baseline entries it reports pre-date this change +— the same run on the unmodified tree reports them). The Kotlin side is pinned +by `SchemeRolesTest`, which sweeps `ColorScheme` reflectively rather than from a +list and fails if any role carries a value the palette did not supply, so a role +added by a future Material version fails the day the dependency moves. The Rust +side asserts the derived rung continues the ladder in all five flavours, both +polarities. The appearance itself still wants a device: screenshots on Proton +and on Latte. + ### B50 — every episode of a show was named after the show **Symptom.** All fifty-nine Attack on Titan rows read `Attack On Titan` on both diff --git a/docs/DESIGN-TOKENS.md b/docs/DESIGN-TOKENS.md new file mode 100644 index 0000000..8b9f146 --- /dev/null +++ b/docs/DESIGN-TOKENS.md @@ -0,0 +1,108 @@ +# Design tokens + +What the desktop and Android clients agree on, and where each one keeps its copy. + +Colour needs no spec: `pstr_core::appearance` resolves one `Palette` and both +clients read it, so a flavour is right in both places or wrong in both. The rest +of the token set has no such bridge — Compose cannot consume a Rust constant, and +type and shape do not survive a round trip through `PaletteRecord` — so this file +is the contract, and each side's copy is asserted against it by a test. + +| Token | Owner | Android's copy | Pinned by | +|---|---|---|---| +| Colour roles | `pstr_core::appearance::Palette` | `ui/theme/Theme.kt` `schemeOf` | `SchemeRolesTest` | +| Type ramp | `pstr_app::theme::Role` | `ui/theme/Type.kt` | `TypeRampTest` | +| Corner radius | `pstr-app` `ui/mod.rs` | `ui/theme/Shape.kt` | `TypeRampTest` | +| Gradient rule | `Palette::resolve` | — (derived, see below) | `appearance.rs` tests | +| Depth | `pstr_app::theme::{tile_shadow, bar_shadow}` | Material elevation | — | +| Motion | `Context::animate_*` | Compose defaults | — | + +## Type + +Nine rungs. The desktop names them; Android restates them because Material's own +scale is a different one (`displayLarge` is 57sp, which is a size for a lock +screen and not for anything in a library grid). + +| Role | Size | What it is | +|---|---|---| +| Display | 26 | the name of the thing you are looking at, once per page | +| Title | 20 | an empty state, or a page heading with no display line | +| Heading | 18 | the brand in the nav bar; a dialog's heading | +| Section | 17 | a run of content under a rule | +| Subhead | 15 | a row that groups the rows under it — a season header | +| Body | 14 | everything that is prose | +| Label | 13 | the text on a small control | +| Caption | 12 | context rather than content: counts, sizes, errors under a field | +| Micro | 11 | the smallest thing that stays legible — a badge over artwork | + +Weight and leading are each side's own: egui has one weight, and Compose derives +line height as 1.35 × size rather than naming it nine more times. + +Both clients point their framework's *default* styles at the ramp — egui's five +`TextStyle`s, Compose's fifteen — so a stock widget nobody restyled still lands +on it. Material 1.4 additionally carries an `Emphasized` copy of all fifteen +styles for its expressive components; those are `internal`, cannot be set from +this module, and keep Material's sizes. Nothing in the app draws with them. + +## Shape + +**Radius 8** for everything either client paints: cards, buttons, the tab pill, +progress bars, dialogs. **4** for a badge over artwork. **16** for an Android +bottom sheet, which has no desktop counterpart. + +Two traps, both Android's: + +- Material's default shape ramp is 4 / 8 / 12 / 16 / 28, so an unstyled screen + puts a 28dp sheet beside a 12dp card beside a fully-round button. +- Material's *buttons* read their own shape token (`CornerFull`), not + `MaterialTheme.shapes`. Handing them a shape is what + `AccentButton` / `TonalButton` / `EdgedButton` / `QuietButton` + (`ui/theme/Accent.kt`) are for. Use those, not Material's, at a new call site. + +## The accent, and what `gradients` means + +The accent is the **only strong colour in the window**. Something wearing it is +something to interact with; nothing wears it for decoration. Neither client +draws an elevation tint (`surfaceTint` is transparent on Android) — a surface +that is faintly accent-coloured because it happens to be raised breaks that rule. + +Where the accent is a ramp rather than a flat fill, in both clients: + +- the primary button +- the seek bar's spent side +- the progress bar under a tile and under an episode row +- the selected navigation pill *(desktop only — see below)* + +Neither client branches on the stored `gradients` flag to decide this. +`Palette::resolve` sets `accent_alt` equal to `accent` when the flag is off, so a +two-stop brush collapses to a flat fill by itself. That is deliberate: a flag +read in two places is a flag that will eventually mean two things, which is +exactly what [B52](BUGS.md#b52--paint-the-accent-as-a-gradient-did-nothing-on-android) +was. + +Known divergences, both Android's, both because the component takes a `Color` +and offers no slot to draw into: + +- The navigation pill is solid `primary` rather than a ramp. +- A download in flight is solid `tertiary` by choice, not by limitation: it is + drawn directly under the watch-progress bar of the same row, and two ramps in + the same colour two pixels apart say nothing. + +## Depth and motion + +The desktop casts shadows from `theme::tile_shadow` / `theme::bar_shadow`, which +**halve on a light flavour** — the same black that separates a card from a dark +page is a smudge on a near-white one. Android gets Material's elevation, which +already adapts. Neither uses a tint to say "raised". + +Transitions on the desktop go through `Context::animate_*`: wall-clock driven, +frame-rate independent, no state held by the caller. Durations in use are 0.05 s +for a press, 0.10–0.12 s for a hover, 0.16 s for the tab pill's travel and 0.22 s +for artwork arriving. Compose's defaults are close enough that nothing here +overrides them. + +## Adding a token + +Name it in the owning client first, restate it on the other side, and extend the +test that pins the copy. A token that lives in only one place is not a token — +it is a literal with a comment. diff --git a/docs/DEVELOPMENT.md b/docs/DEVELOPMENT.md index 2640af0..fe7fb1d 100644 --- a/docs/DEVELOPMENT.md +++ b/docs/DEVELOPMENT.md @@ -170,6 +170,38 @@ reads an external subtitle through the same protocol); the work is in rustls. Separately a browser cannot reach the Proton API at all (CORS) without a proxy server, which the fat-client decision rules out. +- **One palette, one type ramp, one shape radius — named, not repeated.** + `pstr_core::appearance` already settled colour for both clients; + `pstr_app::theme::Role` settles size the same way. A `.size(17.0)` at a call + site is not a scale, it is one of forty independent decisions that happen to + sit near each other, and it is why the two clients read as different apps even + when their colours agreed. Anything new asks for a role. +- **Depth and motion are the theme's, not the widget's.** Shadows come from + `theme::tile_shadow` / `theme::bar_shadow`, which halve themselves on a light + flavour — the same black that separates a card from a dark page is a smudge on + a near-white one. Transitions go through `Context::animate_*`, which is driven + by wall-clock time and keeps no state of its own, so a hover that fades needs + no field anywhere. +- **The grid flexes; `CARD_WIDTH` is a minimum.** `ui::columns` hands back the + column count *and* the width they should be drawn at. Dividing by the constant + and flooring leaves up to a full card of dead space at the right of every row. +- **Android copies the ramp; a test says so.** `ui/theme/Type.kt` and + `ui/theme/Shape.kt` are `Role` and radius 8 restated for Compose, because + Material's own defaults are a different scale (57sp displays, a five-step + corner ramp ending in a fully-round button) and there is no bridge to resolve + type the way `appearance` resolves colour. `TypeRampTest` names all fifteen + Material styles and fails on a sixteenth, so the copy cannot drift quietly. + Note that Material's filled buttons read their own shape token rather than + `MaterialTheme.shapes` — that is what `AccentButton`/`TonalButton`/ + `EdgedButton`/`QuietButton` in `ui/theme/Accent.kt` exist for, and new call + sites should use them rather than Material's directly. +- **The gradient toggle is not read where the gradient is drawn.** + `Palette::resolve` sets `accent_alt` equal to `accent` when gradients are off, + so a two-stop brush collapses to a flat fill on its own. Both clients rely on + that instead of branching on the flag, which is what keeps them from + disagreeing about what the stored setting means (see + [B52](BUGS.md#b52--paint-the-accent-as-a-gradient-did-nothing-on-android)). + ## Traps - **Block sizes are not uniform.** Always `RevisionReader::block_sizes()`; never diff --git a/docs/TESTING.md b/docs/TESTING.md index 0b87e68..7d9c195 100644 --- a/docs/TESTING.md +++ b/docs/TESTING.md @@ -202,6 +202,15 @@ the two decode fallbacks that are easy to get backwards — a missing `label` falling back to the link id, and a blank `error` reading as *no* error rather than as a permanently failed download. +The other two host tests pin the half of `docs/DESIGN-TOKENS.md` that no bridge +enforces. `SchemeRolesTest` sweeps `ColorScheme` reflectively and fails on any +role carrying a value the palette did not supply — including a role a future +Material version adds, the day the dependency moves. `TypeRampTest` names all +fifteen Material text styles against the shared ramp and fails on a sixteenth, +and checks the corner radius the desktop draws at. Both are lists of the *other* +client's decisions, so a change to `pstr_app::theme::Role` that nobody carried +across fails here rather than being noticed in a screenshot. + **Instrumentation tests** (`app/src/androidTest`) are the things with no host equivalent: