diff --git a/.claude/skills/architecture-review/SKILL.md b/.claude/skills/architecture-review/SKILL.md index 89d7f128df..db1d2b401f 100644 --- a/.claude/skills/architecture-review/SKILL.md +++ b/.claude/skills/architecture-review/SKILL.md @@ -56,8 +56,9 @@ For each changed first-party file, check the applicable rules. Each rule cites i | 9 | **Dependency substitution:** don't add a Maven coordinate for something already vendored/substituted (`build-deps*`); don't add a new dependency without checking `gradle/libs.versions.toml` first. | ADR 0003 | | 10 | **Strings** live in the `:resources` module's `strings.xml` (not per-module, not inline literals). | REVIEW.md §7 | | 11 | **UI never drawn over the two system bars** (top status bar, bottom navigation bar). | CLAUDE.md | +| 12 | **Text scales:** new/changed screens hold up at font scale 2.0 — `sp` for text and `dp` for spacing (no `sp` dimen used as margin/padding), no text boxed in a fixed `dp` size, a scroll container on content that can grow, and `maxLines`/`singleLine`/`ellipsize` only on genuinely disposable text. | CLAUDE.md, REVIEW.md §8 | -Rules 1, 2, 6 apply to UI changes; 4, 5 to data/model changes; 8, 9 to Gradle changes. Judge by what the diff touches — don't flag rules a file doesn't engage. +Rules 1, 2, 6, 11, 12 apply to UI changes; 4, 5 to data/model changes; 8, 9 to Gradle changes. Judge by what the diff touches — don't flag rules a file doesn't engage. For a **large diff (~15+ first-party files)**, fan out: spawn a subagent per dimension (UI/state, DI, persistence, Gradle/modules), each instructed to read the relevant ADR and report only its dimension's findings; then merge. For a small diff, do it inline. diff --git a/CLAUDE.md b/CLAUDE.md index 22b9a794b6..0e238095d1 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -29,6 +29,17 @@ Every module carries `v7` (`armeabi-v7a`) and `v8` (`arm64-v8a`) flavors, so bui At least one Android emulator or device is available. Find it with `adb devices -l | grep -v offline`, then target it with the `ANDROID_SERIAL` env var. Note the app is **arm-only** (`v7`/`v8` flavors, no x86) — an x86_64 emulator can't run it (not always even via a translation layer), so testing often needs a **physical arm device** or an arm-translation emulator. +**Font-scale check.** Read the current value first so you can put it back. Each change recreates the activity (only `EditorActivityKt` declares `fontScale` in `configChanges`), so this doubles as a state-restoration test: + +```bash +adb shell settings get system font_scale # save it (prints "null" if never set) +adb shell settings put system font_scale 2.0 +adb exec-out screencap -p > /tmp/scale-2.0.png +adb shell settings put system font_scale 1.0 # restore +``` + +At 2.0, look for text cut off mid-word, labels overrunning their control, actions pushed off the bottom with no way to scroll to them, and overlapping rows. + ## Architecture See **[ARCHITECTURE.md](ARCHITECTURE.md)** — the single source of truth for the module map, layering/data flow, dependency rules, tech stack (DI, async, persistence, networking), state management, and testing strategy. Don't re-document those here; update ARCHITECTURE.md. @@ -38,6 +49,7 @@ See **[ARCHITECTURE.md](ARCHITECTURE.md)** — the single source of truth for th - **Avoid new dependencies** — the build almost certainly already has what's needed. Check `gradle/libs.versions.toml` and `build.gradle.kts` first. - **Persistence:** prefer **Room** for relational data and the filesystem/preferences for settings; raw SQLite only for justified exceptions — see [ADR 0001](docs/adr/0001-prefer-room-for-persistence.md). - **Protect the two Android system bars** in any UI work: the top status bar (clock, notifications, status icons) and the bottom navigation bar (home, back, recents). Don't draw over or intercept them. +- **Every screen must survive 2x font scale.** Users with low vision run large system fonts, and a screen that clips or hides content at 2.0 is broken for them. Verify any new or changed screen at font scale **1.0 and 2.0** (see Build & test, Emulator / device) and say in the PR that you did. Text grows, so: use `sp` for text and `dp` for spacing — never an `sp` dimen as a margin or padding; don't box text in a fixed `dp` height or width; give content that can grow somewhere to scroll; and reserve `maxLines`/`singleLine`/`ellipsize` for text that is genuinely disposable. - **Plan and size before building.** Prefer **one PR per ticket/use case** — don't force-split a coherent change (splitting has its own overhead when later edits span the pieces). When a change is large, break it into **reviewable commits** — mechanical/refactor commits separate from behavioral ones — and offer review-by-commit. Treat ~500 LOC / ~10 files as a signal to reach for that commit structure, not a hard cap; the ceiling rises as LLM-assisted review matures. For staged multi-commit refactors (e.g. removing a dependency across many files/modules), order stages easiest-to-hardest and independently compile/test each stage (see Build & test's fast-iteration guidance) before moving to the next, so a failure is isolated to the stage that caused it. - **Keep docs in step with code.** When you change code, update the docs that describe it in the same change — a module's `README.md`, `ARCHITECTURE.md`, or an ADR — so a doc never outlives the API it documents (see REVIEW.md, Code quality). If the doc fix is out of scope, file a ticket rather than let it drift. - `.androidide_root` is a sentinel file tests use to locate the project root — don't delete it. diff --git a/REVIEW.md b/REVIEW.md index 39de210c00..4d1d9b2e50 100644 --- a/REVIEW.md +++ b/REVIEW.md @@ -22,7 +22,7 @@ A review isn't done because it *looks* fine; it's done when you can **show what | §4 Security | Which untrusted inputs were validated; secrets checked | | §5 Tests & coverage | JaCoCo numbers for new non-UI code (line & branch) | | §7 Code quality | Duplication/cohesion pass done; no reimplementation of existing helpers | -| §8–§9 A11y & help | contentDescription + long-press help on new interactive elements | +| §8–§9 A11y & help | contentDescription + long-press help on new interactive elements; font scale 1.0/2.0 verified on new or changed screens | | §10 Architecture | Checklist below, each item pass/fail | | §13 Plugins | API-surface touched? impact check result | @@ -40,6 +40,7 @@ Keep it proportional — a two-line change needs a two-line ledger. - [ ] **Docs:** public classes/functions have KDoc/Javadoc explaining *why*, not *what*; any module `README`/`ARCHITECTURE.md`/ADR the change affects is updated in the same PR. - [ ] **Strings** are in the **`:resources`** module's `strings.xml` (not per-module, not inline literals) — keeps localization centralized. - [ ] **Accessibility:** every actionable view has a `contentDescription` (XML *or* programmatic); decorative views are marked `importantForAccessibility="no"`. +- [ ] **Font scale:** new or changed screens verified at **2.0** — nothing clipped, nothing unreachable — or explicitly noted as not applicable. - [ ] **Contextual help:** new interactive elements (and any new screen/panel) have long-press help wired to the 3-tier tooltip system. - [ ] **Analytics:** meaningful user/build actions emit an event (see below). - [ ] **Scope/size:** PR is focused on one ticket/use case; if large, it's split into **reviewable commits** (mechanical separate from behavioral) rather than force-split into multiple PRs (`CLAUDE.md`). @@ -142,7 +143,7 @@ Keep event names/params stable and low-cardinality; **no PII, file paths with us - **Strings in `strings.xml`.** User-facing text must be a string resource, never an inline literal — lint flags `HardcodedText`, and externalized strings feed our Crowdin translation flow. Use plurals/`getQuantityString` and positional args for formatting. Log messages and analytics keys are *not* user-facing and stay in code. - **Dependencies:** don't add one without checking `gradle/libs.versions.toml` first — we probably already have it (`CLAUDE.md`). -## 8. Accessibility — every actionable view speaks +## 8. Accessibility — every actionable view speaks, and every screen scales CoGo serves visually-impaired developers, so TalkBack support is a correctness requirement, not a nice-to-have (pattern set by ADFA-2667). New UI is Compose ([§10](#10-architecture-alignment) / [ADR 0009](docs/adr/0009-jetpack-compose-for-new-ui.md)), so each rule gives the View and Compose form — the requirement is the same in either. @@ -159,6 +160,15 @@ CoGo serves visually-impaired developers, so TalkBack support is a correctness r - **Externalize, with the `cd_` convention.** Content descriptions live in `strings.xml` as `cd_*` — greppable, translatable, reusable; check for an existing one first. `HardcodedText` lint does **not** catch Compose literals, so reviewers must. - **Bonus — it stabilizes tests.** Screen-reader semantics are what UI tests match on (`ACTION_CLICK` for Views, `onNodeWithContentDescription(…)` for Compose), so a11y and reliable instrumentation tests are the same work. +**Text scales, so layouts must too.** Low vision means large system fonts as often as it means TalkBack. A screen isn't done until it works at **2x**. + +- **Verify new or changed screens at font scale 1.0 and 2.0**, and put the result in the PR — a screenshot at 2.0, or one line saying what you checked. Recipe in `CLAUDE.md` (Build & test → Emulator / device). "No visual change" or "no text on this surface" is a valid one-line opt-out; silence is not. +- **Spacing in `dp`, text in `sp`.** An `sp` dimension used as a margin or padding grows with the font scale and squeezes the text it was meant to frame — as `layout-land/fragment_onboarding_greeting.xml` does today with `@dimen/_32sp`. +- **Don't box text in a fixed size.** A control sized `40dp x 40dp` can't hold a label that doubled. Let the container wrap its content and set a `minWidth`/`minHeight` for the touch target instead of a fixed one. + - *Compose:* the same trap is `Modifier.height(44.dp)`/`.size(36.dp)` on chrome that contains text — use `defaultMinSize` and let it grow. +- **Give growth somewhere to go.** Content that can reflow past the viewport needs a `NestedScrollView` (Compose: `verticalScroll`/`LazyColumn`). Only 14 of the 108 layouts in `app/src/main/res/layout/` have one today — don't add to the pile. +- **`maxLines`/`singleLine`/`ellipsize` are a decision, not a default.** Clamping is fine for a preview line, wrong for anything the user must read to proceed. `ellipsize="none"` with `maxLines` clips mid-glyph and is almost never what you want. + ## 9. Contextual help — long-press works everywhere Help in CoGo is reached by **long-press**, anywhere: a progressive three-tier experience — **Tiers 1 & 2 are tooltips** (anchored popups from `idetooltips`), **Tier 3 is a full help web page** via the tooltip's "See More" link. A long-press should never be met with silence.