Merge pull request 'docs(android): M12 phase 8 as landed, and the two defects AC-5 found' (#119) from docs/android-theming-nav-phase-8 into edge
Reviewed-on: #119 Reviewed-by: Colby Whitlock <whitlocktech@gmail.com>
This commit is contained in:
@@ -202,3 +202,28 @@ class LoginViewModelTest {
|
|||||||
- `MainDispatcherRule` + a documented ViewModel test pattern exist and are reused.
|
- `MainDispatcherRule` + a documented ViewModel test pattern exist and are reused.
|
||||||
- Coverage exclusions list only genuinely non-unit-testable files (UI composables, Android-framework
|
- Coverage exclusions list only genuinely non-unit-testable files (UI composables, Android-framework
|
||||||
glue) — no ViewModel, repository, DTO, or pure core-logic file is excluded.
|
glue) — no ViewModel, repository, DTO, or pure core-logic file is excluded.
|
||||||
|
|
||||||
|
## 6. Amendment (2026-08-08, M12 phase 8): `ui/theme/**` narrowed to one file
|
||||||
|
|
||||||
|
**A directory glob in the exclusion list went stale as soon as another milestone put testable code
|
||||||
|
in that directory.** §2 phase 0 excluded `app/src/main/java/**/ui/theme/**` because at the time the
|
||||||
|
directory held `Color.kt`, `Type.kt` and the composables — constants and composable bodies, nothing
|
||||||
|
a JVM test could execute. M12 then added three *pure* resolvers to it: `ShardPalette`,
|
||||||
|
`ShardStructure` and `ShardTypeface`, which exist precisely so the theming milestone's no-op
|
||||||
|
invariants could be plain JVM assertions. JaCoCo measures them at **98%, 100% and 100%**, and the
|
||||||
|
glob was discarding every line.
|
||||||
|
|
||||||
|
The exclusion is now the single file it was really about, `ui/theme/Theme.kt` (the composable, 52%).
|
||||||
|
Everything else in `ui/theme/` is measured and all of it covers at 93% or better.
|
||||||
|
|
||||||
|
Two things worth carrying forward:
|
||||||
|
|
||||||
|
- **This did not rescue the gate and was not meant to.** M12's already-measured code
|
||||||
|
(`data/appearance/` at 100%, `ui/navigation/` at 93–100%) clears `new_coverage ≥ 50` on its own.
|
||||||
|
The point is that a future change deleting those resolvers' tests would now move the number, where
|
||||||
|
before it would not have — the exclusion was hiding well-tested code, which is the opposite of what
|
||||||
|
§1 built the list for and what §5's third bullet asks for.
|
||||||
|
- **Prefer file globs to directory globs when a directory is mixed.** `ui/components/**` stays a
|
||||||
|
directory glob and correctly so: `BrandAssets.kt` sits at 11% because only `brandAssetUrl` is pure,
|
||||||
|
and the rest is composable bodies. The distinction is whether the directory is *uniformly*
|
||||||
|
untestable, not whether it is under `ui/`.
|
||||||
|
|||||||
@@ -1048,9 +1048,42 @@ push, and Play (M6–M8) follow the designed app.
|
|||||||
the shipped palette reproduces the pre-M12 `ColorScheme` role for role. **Phase 2 landed
|
the shipped palette reproduces the pre-M12 `ColorScheme` role for role. **Phase 2 landed
|
||||||
2026-08-08** (Android-app#35 / docs#114): the four radii scale the app's own dp by their ratio
|
2026-08-08** (Android-app#35 / docs#114): the four radii scale the app's own dp by their ratio
|
||||||
to the `runic-gateway` baseline, and `--shadow-card` maps onto card elevation. The shadow is
|
to the `runic-gateway` baseline, and `--shadow-card` maps onto card elevation. The shadow is
|
||||||
the milestone's **one deliberate change to an untouched instance** — the app has been flat
|
a **deliberate change to an untouched instance** — the app has been flat since M5 while the
|
||||||
since M5 while the preset it was drawn from selects a shadow — and is why every `Card(` became
|
preset it was drawn from selects a shadow — and is why every `Card(` became a `ShardCard(`:
|
||||||
a `ShardCard(`: Material takes elevation as a default argument, not from the theme.
|
Material takes elevation as a default argument, not from the theme. **Phase 3 landed
|
||||||
|
2026-08-08** (Android-app#36 / docs#115): seven bundled families beside Cinzel, resolved from
|
||||||
|
the stacks the theme publishes. It **nearly tripled the APK** — 4.80 → 12.94 MiB against a
|
||||||
|
drafted 1.5–2.5 MB estimate, with Merriweather alone 6.08 MiB of the growth because upstream
|
||||||
|
ships it as a barely-compressible three-axis variable font; verbatim bundling was chosen with
|
||||||
|
the cheaper options costed and declined. **Phase 4 landed 2026-08-08** (Android-app#37 /
|
||||||
|
docs#116): logo in the drawer header and top bar, hero on Home, no new dependency and no APK
|
||||||
|
cost. **Phase 5 landed 2026-08-08** (Android-app#38 / docs#117): the public nav's label, order
|
||||||
|
and hiding, keyed by the path→route table — whose sort keys must be indices into the
|
||||||
|
*website's* sixteen-row nav, not the app's nine. **Phase 6 landed 2026-08-08**
|
||||||
|
(Android-app#39 / docs#118): the drawer gains admin-authored sections and added links, a link
|
||||||
|
the app can open natively doing so and one it cannot handing off to a Custom Tab.
|
||||||
|
|
||||||
|
**Phase 8 landed 2026-08-08** (Android-app#40 / docs#119) and did more than its name. Scoped
|
||||||
|
as docs, coverage and the cutover, its **AC-5 on-device walk** — two AVD passes against a local
|
||||||
|
website, the themed one at every role rung — confirmed everything phases 1–6 had deferred to it
|
||||||
|
(including that a Custom Tab really opens, checked in `dumpsys`, and that an anonymous caller
|
||||||
|
sees the reordered, sectioned nav with no player or staff row leaking) **and found two theming
|
||||||
|
defects that 476 passing tests could not see**:
|
||||||
|
|
||||||
|
- every `ShardCard` — 26 sites in 20 files — drew in Material's grey rather than the shard's
|
||||||
|
panel colour, because `CardDefaults.cardColors()` takes its container from
|
||||||
|
`surfaceContainerHighest` and the mapping had `surfaceContainer`, `…High` and `…Low` but not
|
||||||
|
`…Highest`; and
|
||||||
|
- the drawer's selected row ignored `--radius-pill`, because `NavigationDrawerItem` takes
|
||||||
|
`shape` as a default argument.
|
||||||
|
|
||||||
|
Both are **phase 2's trap repeating**: Material takes these as default arguments, not from the
|
||||||
|
theme. Phase 2 found it for depth and swept the call sites; nobody asked whether colour and
|
||||||
|
shape had the same problem. The card fix also **changes the untouched app** — those grey cards
|
||||||
|
predate M12 and go back to M5 — so the milestone ships two deliberate changes to a shard that
|
||||||
|
has set nothing, the shadow and the card container, with AC-1 rewritten to record the second
|
||||||
|
rather than absorb it. Phase 8 also narrowed a stale `ui/theme/**` Sonar coverage glob that was
|
||||||
|
discarding three 98–100%-covered resolvers. **477 tests green.**
|
||||||
|
|
||||||
### Deferred (not a milestone)
|
### Deferred (not a milestone)
|
||||||
|
|
||||||
|
|||||||
@@ -665,7 +665,7 @@ PR against `main`.
|
|||||||
| **5** ✅ | **Public nav: label / order / hidden** | The path→route table (§6.2), `Routes.news(category)`, the merge, drawer wiring. AC-3 |
|
| **5** ✅ | **Public nav: label / order / hidden** | The path→route table (§6.2), `Routes.news(category)`, the merge, drawer wiring. AC-3 |
|
||||||
| **6** ✅ | **Public nav: sections & added links** | Drawer groups, `pruneNav` port, link path validation, native-route resolution + Custom Tab fallback (§6.3) |
|
| **6** ✅ | **Public nav: sections & added links** | Drawer groups, `pruneNav` port, link path validation, native-route resolution + Custom Tab fallback (§6.3) |
|
||||||
| **7** | **Authenticated navs** | ❌ **cancelled** — was: `GET /api/v1/settings/nav` behind a session-keyed repository; label/hidden for the four mapped rows (§6.4). See "Phase 7, cancelled" below |
|
| **7** | **Authenticated navs** | ❌ **cancelled** — was: `GET /api/v1/settings/nav` behind a session-keyed repository; label/hidden for the four mapped rows (§6.4). See "Phase 7, cancelled" below |
|
||||||
| **8** | **Docs, coverage & cutover** | This doc's "as landed" notes and any amendments the build forces, the `PLAN.md` §9 M12 entry refreshed, Sonar coverage for the new modules, AC-5 on-device walk, then `edge` → `main` |
|
| **8** ✅ | **Docs, coverage & cutover** | This doc's "as landed" notes and any amendments the build forces, the `PLAN.md` §9 M12 entry refreshed, Sonar coverage for the new modules, AC-5 on-device walk, then `edge` → `main`. **The walk found two theming defects and phase 8 fixed them** — see "Phase 8 as landed" |
|
||||||
|
|
||||||
Phase 0 is the one with a hard rule attached: **it must change nothing on
|
Phase 0 is the one with a hard rule attached: **it must change nothing on
|
||||||
screen.** Everything after it is additive on top of a store that is already
|
screen.** Everything after it is additive on top of a store that is already
|
||||||
@@ -762,7 +762,12 @@ proven not to have moved anything.
|
|||||||
|
|
||||||
### Phase 2 as landed
|
### Phase 2 as landed
|
||||||
|
|
||||||
- **The app is flat, so §5.4 is the milestone's one visible default change.**
|
- **The app is flat, so §5.4 is a visible default change.** (It was written here
|
||||||
|
as the milestone's *only* one. Phase 8 found a second — the card *container*
|
||||||
|
colour — and a third that is not a change at all but a correction to this
|
||||||
|
record: an instance with a `BRAND_HERO_IMAGE` and no `brand_assets` row now
|
||||||
|
draws a hero on Home, because `brand.hero` carries the env default. See
|
||||||
|
"Phase 8 as landed".)
|
||||||
Material3 1.3.0's filled `Card` is `ElevationTokens.Level0` — 0dp, checked in
|
Material3 1.3.0's filled `Card` is `ElevationTokens.Level0` — 0dp, checked in
|
||||||
the artifact's bytecode, not assumed — and `FeatureCard` was a `Box` with a
|
the artifact's bytecode, not assumed — and `FeatureCard` was a `Box` with a
|
||||||
clip, a gradient and a border, drawing none of the "soft shadow" its own KDoc
|
clip, a gradient and a border, drawing none of the "soft shadow" its own KDoc
|
||||||
@@ -1022,6 +1027,108 @@ Consequences, all of them wanted:
|
|||||||
|
|
||||||
If it is ever picked up, §6.4 is the spec and the merge would reuse phase 5's.
|
If it is ever picked up, §6.4 is the spec and the merge would reuse phase 5's.
|
||||||
|
|
||||||
|
### Phase 8 as landed
|
||||||
|
|
||||||
|
Phase 8 was scoped as docs, coverage and the cutover. The AC-5 walk turned it
|
||||||
|
into a phase with code in it, which is the outcome the walk existed to produce.
|
||||||
|
|
||||||
|
**AC-5 confirmed what the JVM could not reach.** Two passes on the AVD against a
|
||||||
|
local website, the themed one walked at every role rung. Everything phases 1–6
|
||||||
|
had deferred to it held: the Fantasy palette on screen; radii visibly square
|
||||||
|
against the untouched build's rounded; **EB Garamond arriving in the *sans* role**,
|
||||||
|
which is phase 3's preset-bypass finding proved on a device rather than in a test;
|
||||||
|
the logo replacing the top bar's text with the 6× width cap holding; the hero
|
||||||
|
centre-cropped into its 180dp band with the top and bottom of a 4:3 image cut;
|
||||||
|
reorder, relabel and hide; the section header with its indented children; the
|
||||||
|
added link with a native route carrying **no** badge and the handing-off one
|
||||||
|
carrying it; and the Custom Tab genuinely opening — confirmed as
|
||||||
|
`CustomTabActivity` in `dumpsys activity`, not by eye. AC-3's role dimension held
|
||||||
|
at every rung: an anonymous caller saw the reordered, sectioned public nav with
|
||||||
|
**no player or staff row leaking**, and the player rung stopped at `My houses`.
|
||||||
|
AC-4 fell out for free when the second AVD started against an unreachable server:
|
||||||
|
shipped theme, coded menu, one retry, no error surface.
|
||||||
|
|
||||||
|
**A correction the walk forced, with no code behind it.** An untouched instance
|
||||||
|
**gains a hero on Home**. `brand.hero` carries the `BRAND_HERO_IMAGE` env default
|
||||||
|
— `/assets/img/runic-emblem.png` on this one — so "admin has set nothing" does not
|
||||||
|
mean "the slot is empty", and phase 4's `BrandHero` draws it. Nothing is wrong
|
||||||
|
here: the website's own Home shows the same emblem, and §5.6's rule is that a
|
||||||
|
blank slot draws nothing, which still holds. What was wrong is the *record* —
|
||||||
|
phase 2 claimed the card shadow was the milestone's only visible change to an
|
||||||
|
untouched app, and this was the second before the card container made it three.
|
||||||
|
Worth stating plainly because a shard operator upgrading the app sees it without
|
||||||
|
having touched Appearance at all.
|
||||||
|
|
||||||
|
**Two defects, one root cause, and it is phase 2's.** Material takes some themed
|
||||||
|
values as **default arguments** rather than from the theme. Phase 2 found this for
|
||||||
|
card elevation and answered it by sweeping every `Card(` into `ShardCard(`. Nobody
|
||||||
|
then asked whether *colour* and *shape* had the same problem. They did:
|
||||||
|
|
||||||
|
- **Every `ShardCard` drew in Material's grey, not the shard's panel colour.**
|
||||||
|
`CardDefaults.cardColors()` takes its container from `surfaceContainerHighest`
|
||||||
|
— `FilledCardTokens.ContainerColor`, checked in the material3 1.3.0 artifact's
|
||||||
|
bytecode the same way phases 1 and 2 checked theirs — and `shardColorScheme`
|
||||||
|
mapped `surfaceContainer`, `…High` and `…Low` but not `…Highest`. All 26 call
|
||||||
|
sites in 20 files. `FeatureCard` was unaffected because it reads
|
||||||
|
`LocalShardPalette` directly, which is exactly why Home looked right and every
|
||||||
|
other screen did not — the single most misleading thing about the bug.
|
||||||
|
- **The drawer's selected row ignored `--radius-pill`.** `NavigationDrawerItem`
|
||||||
|
takes `shape` as a default argument (`CircleShape`); the three call sites set
|
||||||
|
`colors` and never `shape`. On Fantasy every other radius went square while the
|
||||||
|
selected row stayed a full pill.
|
||||||
|
|
||||||
|
**The card fix changes the untouched app, and that is the honest framing.** The
|
||||||
|
grey cards are not an M12 regression — an untouched instance draws them too, and
|
||||||
|
has since M5. M12 only made it visible by theming everything around them. So
|
||||||
|
fixing it moves the shipped app's cards from Material's grey to `--panel-flat`:
|
||||||
|
**the milestone's second deliberate change to a shard that has set nothing**,
|
||||||
|
alongside phase 2's card shadow. §9's last bullet had already called this shot —
|
||||||
|
"if a screen looks wrong under a warm preset, that is a token the screen should
|
||||||
|
have been reading and did not" — and this is that, at 26 call sites.
|
||||||
|
|
||||||
|
There is no version of the fix that spares untouched instances: the shipped
|
||||||
|
palette *is* the `runic-gateway` preset, so the value the cards should take is the
|
||||||
|
same value either way. AC-1 therefore **records** the change instead of absorbing
|
||||||
|
it — every other role is still asserted byte-for-byte against the verbatim pre-M12
|
||||||
|
scheme, and the two that moved are named, given their new values, and checked to
|
||||||
|
have genuinely differed before, so the test cannot pass by coincidence.
|
||||||
|
|
||||||
|
**The role sweep, so the next person does not redo it.** Of the roles
|
||||||
|
`shardColorScheme` leaves at `darkColorScheme()`'s defaults, exactly one had a
|
||||||
|
live reader in this app: `surfaceContainerHighest`. Checked against the token
|
||||||
|
class of every Material component the app actually draws —
|
||||||
|
`NavigationDrawerTokens`, `SheetBottomTokens`, `AssistChipTokens`,
|
||||||
|
`FilterChipTokens`, `TopAppBarSmallTokens`, `BadgeTokens` — all of which read only
|
||||||
|
roles already mapped. `scrim` is read by the modal drawer and **stays Material's
|
||||||
|
black on purpose**; a tinted scrim is a design change, not a gap.
|
||||||
|
`surfaceContainerLowest` is mapped for consistency with `…Low` and has no reader.
|
||||||
|
`primaryContainer`, `tertiaryContainer`, `inverse*`, `surfaceBright`, `surfaceDim`
|
||||||
|
and `surfaceTint` are latent — the app draws no Snackbar, tonal button, elevated
|
||||||
|
or outlined card. Adding one of those components means checking this list again.
|
||||||
|
|
||||||
|
**Coverage: the exclusion list had gone stale under the milestone.**
|
||||||
|
`sonar.coverage.exclusions` carried a `ui/theme/**` directory glob from the M11
|
||||||
|
coverage push, when that directory held only constants and composables. M12 put
|
||||||
|
three pure resolvers in it — `ShardPalette`, `ShardStructure`, `ShardTypeface`,
|
||||||
|
the reason phases 1–3 could prove their no-op invariants as JVM assertions — and
|
||||||
|
the glob was dropping all three out of the denominator at 98%, 100% and 100%. It
|
||||||
|
is now the one file it was really about, `ui/theme/Theme.kt`. This did not rescue
|
||||||
|
the gate: M12's already-measured code (`data/appearance/` and `ui/navigation/`,
|
||||||
|
93–100%) clears `new_coverage ≥ 50` on its own. It makes the number honest about
|
||||||
|
which code the tests hold, so deleting those tests would move it.
|
||||||
|
|
||||||
|
**Tests: 477 green** (476 + the card-container assertion), `lintDebug` and
|
||||||
|
`assembleDebug` clean. The JVM limit from phase 4 is unchanged and is why this
|
||||||
|
phase needed a device at all: no Robolectric, no `androidTest` source set, so a
|
||||||
|
composable body cannot run in a unit test. Both defects were invisible to 476
|
||||||
|
passing tests and obvious within two minutes of looking at a themed screen.
|
||||||
|
|
||||||
|
**Rig notes, for the next walk.** The emulator reaches a local website through
|
||||||
|
`adb reverse tcp:3000 tcp:3000`, which lets the app use `http://127.0.0.1:3000`
|
||||||
|
and the debug `network_security_config`'s loopback exception **unchanged** — do
|
||||||
|
not add `10.0.2.2` to it for a test rig. The Custom Tab resolves against the same
|
||||||
|
base URL, so hand-off links work under the same tunnel.
|
||||||
|
|
||||||
## 9. Out of scope
|
## 9. Out of scope
|
||||||
|
|
||||||
- **Light mode / a light preset.** The website cancelled its Parchment phase; the
|
- **Light mode / a light preset.** The website cancelled its Parchment phase; the
|
||||||
|
|||||||
Reference in New Issue
Block a user