diff --git a/.claude/skills/find-missing-translations/SKILL.md b/.claude/skills/find-missing-translations/SKILL.md index ef7d1c5e40..491b2478ef 100644 --- a/.claude/skills/find-missing-translations/SKILL.md +++ b/.claude/skills/find-missing-translations/SKILL.md @@ -77,6 +77,25 @@ What this means for this skill: 2. **Source-identical entries are a small, recognizable minority.** Brand terms (`Nowhere X`), single-word loanwords (`Apps` / `Feed` / `Issues`), and bare version/format strings (`v%1$s`) are the usual cases. Skip these by inspection rather than translating them to something identical. 3. **Don't add source-identical fallbacks.** Android falls back to `values/strings.xml` at runtime, so a key intentionally kept as English already renders correctly, and Crowdin's next sync would strip a local duplicate anyway. +4. **A repo-side edit to a translated value only sticks where Crowdin's database + doesn't contradict it.** Download replaces file content with Crowdin's current + export; it does not diff or merge. So a hand fix to a locale file survives only + if Crowdin happens to hold the same value (or holds nothing for that key). If + Crowdin holds a *different* value — including an **empty** one — the next sync + silently reverts you. + + Observed 2026-08-13/14 in one pass, which is what makes the rule concrete: + `pow_estimate_minutes[few]` (pl) **survived** the sync because Crowdin's + approved value matched the fix, while `nest_listener_count[many]` (pl) was + **reverted to empty** two commits later because Crowdin stores an empty string + there. Same file, same commit, opposite outcomes. + + Consequences: fixing a *value* durably means entering it in the Crowdin web UI + — no repo commit will hold it. Changes to the **source** file are different and + do stick, because that file is Crowdin's input, not its output: deleting a key + from `values/strings.xml` removes it project-wide, and attributes declared + there propagate into every export. + > **Historical note:** an earlier version of this skill tried to auto-filter the > candidate list with a git "sync-timestamp" heuristic (skip any key added before > the last `New Crowdin translations` commit). It was **dropped** because it @@ -280,6 +299,64 @@ done For each hit, warn the user that the entry is unreachable in that locale. The fix is to **remove the ``** and, if the UX wanted distinct wording for count=0, add a separate `` plus an `if (count == 0)` branch at the call site (see "Plurals: handle with care" below). +Also audit **format-specifier parity and empty items** across the locales you +touched. These are a different defect class from a missing key — the key is +present and looks translated, but the placeholder was dropped, escaped, or the +item left blank, so the number never reaches the user: + +```bash +python3 - <<'PY' +import re, io, glob +keyre = re.compile(r']*>(.*?)', re.S) +plre = re.compile(r']*>(.*?)', re.S) +itre = re.compile(r']*>(.*?)', re.S) +# (?' \ + amethyst/src/main/res/values*/strings.xml \ + commons/src/commonMain/composeResources/values*/strings.xml +``` + +Three things this scan taught us, all of which it now encodes: + +- **The `(?`** entries, follow these rules: pluralStringResource(R.plurals.foo_items, count, dateLabel, count) } ``` +- **Converting an existing `` to ``: give every locale its FULL + category set, not just `other`.** You must convert it in every locale that + already had the `` (aapt2 rejects a resource-type mismatch across + locales, and an orphaned locale `` trips `ExtraTranslation`) — but + carrying the old text across as an `other`-only block, on the theory that + Crowdin backfills the rest, **fails `MissingQuantity` and breaks CI before + Crowdin ever gets a turn.** Supply `one`/`few`/`many` for pl, `one` for hu, and + so on, at conversion time. + + Note this contradicts `amethyst/src/main/res/CLAUDE.md` step 3, which still + advises the `other`-only shortcut. That advice is wrong; prefer this. + + Watch the declension when you do it: the retained text is usually the *plural* + form, so reusing it verbatim for `one` produces "1 odpowiedzi". (2026-08-13: + converting `poll_results_selections` with `other` only errored on both hu and + pl, and the retained pl text was the few/many form.) + +- **A `tools:ignore` suppression must go on the SOURCE entry in + `values/strings.xml`, never on a locale file.** Crowdin propagates attributes + declared on the source into every translation it exports; an attribute you add + to `values-xx/strings.xml` alone is simply absent from the next export. That is + why the existing `tools:ignore="Typos"` entries survive — they are declared on + the source, and the copies in cs/de/ar/eo/bn are the *result* of propagation, + not evidence that locale-file attributes stick. (2026-08-13: an + `ImpliedQuantity` suppression added only to `values-pt-rBR` was stripped by the + next sync and took `main`'s CI red.) + + Before reaching for a suppression at all, check whether the key is even used — + a `grep -rn "" --include='*.kt'` that returns nothing means deleting the + key is the better fix than muting the rule that objects to it. + - Reference: [Android `` docs](https://developer.android.com/guide/topics/resources/string-resource#Plurals) and [CLDR plural rules](https://unicode-org.github.io/cldr-staging/charts/latest/supplemental/language_plural_rules.html). **Then ask the user:** "Would you like me to translate these missing strings into [list of target locales]?" @@ -380,6 +488,44 @@ When adding translated strings to locale files: # ./gradlew :commons:convertXmlValueResourcesForCommonMain ``` +- **Then run Android lint. This is the gate that actually matches CI, and the + checks above do NOT substitute for it.** Duplicate-key + well-formedness + + `convertXmlValueResourcesForCommonMain` can all pass on a change that still + takes CI red, because the plural rules live in lint, not in the resource + compiler: + + ```bash + ./gradlew :amethyst:lintPlayBenchmark # the task CI runs (.github/workflows/build.yml) + ``` + + There is no `lint-baseline.xml` in this repo and only `MissingTranslation` is + disabled (`amethyst/build.gradle.kts`), so `abortOnError` bites on the first + error. Three rules matter for a translation pass: + + | Rule | Fires when | Severity | + |------|-----------|----------| + | `MissingQuantity` | a locale's `` omits a CLDR category that locale uses | **error** for core categories — gates CI | + | `ImpliedQuantity` | a `quantity` item has no format argument in a locale where that category spans more than one number | **error** — gates CI | + | `StringFormatCount` / `StringFormatMatches` | a translation's placeholder count/type disagrees with the base entry | warning | + + (2026-08-13: a pass that cleared the duplicate/XML gate above still failed + `lintPlayBenchmark` with 3 errors. Compiling is not evidence — `compileDebugKotlin` + passed on the same change.) + +- **Confirm the report says zero errors, don't just trust BUILD SUCCESSFUL** of a + wider invocation: + + ```bash + python3 -c " + import json,io,collections + d=json.load(io.open('amethyst/build/reports/lint-results-playBenchmark.sarif',encoding='utf-8')) + r=d['runs'][0]['results'] + print(dict(collections.Counter(x.get('level','warning') for x in r))) + for x in r: + if x.get('level')=='error': print('ERROR', x['ruleId'], x['locations'][0]['physicalLocation']['artifactLocation']['uri']) + " + ``` + ## Common Mistakes - **Scanning only the amethyst tree** — there are now **two** Crowdin-managed `strings.xml` trees (`amethyst/src/main/res` and `commons/src/commonMain/composeResources`). A key extracted into `commons/` will never show up in the amethyst diff. Run the whole technique once per tree (see "Resource trees") and report each separately. @@ -392,6 +538,12 @@ When adding translated strings to locale files: - **Adding source-identical fallbacks locally** — they get overwritten on the next Crowdin sync. Android falls back to `values/strings.xml` at runtime anyway, so a key intentionally kept as English already renders correctly. Skip these by inspection (brand terms, loanwords, `v%1$s`-style strings); don't translate them to an identical value. - **Skipping per-locale diffs when only diffing cs** — Crowdin can strip different keys in different locales (each translator's choice), so cs is not a reliable upper bound. Diff each target locale and union the results. - **Pasting the union set of missing keys into every locale → duplicate keys** — the union is the right set to *translate*, but the wrong set to *insert*. A key missing in only some locales, inserted into all of them, duplicates in the ones that already had it. Drive each file's insertion off its own per-locale diff (see Step 6). In `commons`, a duplicate key is build-breaking: `convertXmlValueResourcesForCommonMain` fails with `Duplicated key '…'`. **Always run the post-insertion duplicate + XML-wellformedness gate in Step 6 before declaring done.** (Happened 2026-07-21 with `ps1_save_block` / `podcast_value_for_value` / `chats_history_relays`.) +- **Declaring the pass done without running `:amethyst:lintPlayBenchmark`** — the duplicate-key + XML + `convertXmlValueResourcesForCommonMain` gate is necessary but nowhere near sufficient. `MissingQuantity` and `ImpliedQuantity` are errors, there is no lint baseline, and `abortOnError` is on, so a change that compiles and passes every check in Step 6's first half can still take CI red. Compiling is not evidence. (Happened 2026-08-13: 3 lint errors after a clean duplicate/XML gate and a green `compileFdroidDebugKotlin`.) +- **Converting a `` to `` with `other` only** — "Crowdin fills the rest" is false; `MissingQuantity` errors immediately and CI fails before any sync. Supply every category the locale uses at conversion time, and re-check the declension rather than reusing the old text for `one`. +- **Putting `tools:ignore` on a locale file** — Crowdin strips it on the next export. Suppressions belong on the source entry in `values/strings.xml`, which propagates. The `tools:ignore="Typos"` copies visible in cs/de/ar/eo/bn are the *result* of that propagation, not proof that locale-file attributes survive. (Happened 2026-08-13; it broke `main`.) +- **Suppressing a lint rule on a key nothing references** — check `grep -rn "" --include='*.kt'` first. `poll_results_voters` was a bare noun with no count, zero call sites, and an unlocalizable shape; deleting it retired the problem outright where a suppression would only have muted it. +- **Comparing placeholders without a `(?` as merely "untranslated"** — it renders as nothing at runtime, and for a category like Polish `many` (5–21, 25–31, …) that is the common case, not an edge case. Grep for them explicitly; the missing-key diff will never surface one because the key is present. - **Inserting strings in a specific position** — always append at the bottom; ordering is handled separately - **Hardcoding `"1"` in a `` `quantity="one"` item** — always use the count placeholder; otherwise non-English `one` categories produce wrong text - **Copying English's `one`/`other` set into every locale** — each language must include all CLDR plural categories it uses (e.g. Czech needs `one`, `few`, `many`, `other`) diff --git a/amethyst/src/main/res/CLAUDE.md b/amethyst/src/main/res/CLAUDE.md index e7d124f6bb..26fb7dc71e 100644 --- a/amethyst/src/main/res/CLAUDE.md +++ b/amethyst/src/main/res/CLAUDE.md @@ -51,5 +51,5 @@ Defined in `amethyst/src/main/java/com/vitorpamplona/amethyst/ui/StringResourceC 1. Add the new `` to default `values/strings.xml` with `one` + `other`. 2. If you're also adding a locale-specific translation (e.g. zh-rCN, pl-rPL), add it as `` with at least `other`. Crowdin will fan out to all CLDR categories for that locale. -3. If you're **converting** an existing `` to ``, you **must** convert it in every locale that already had the `` — otherwise aapt2 will fail with a resource-type mismatch. Use `` with `other` only to preserve existing translation (Crowdin fills the rest). +3. If you're **converting** an existing `` to ``, you **must** convert it in every locale that already had the `` — otherwise aapt2 will fail with a resource-type mismatch, and an orphaned locale `` trips `ExtraTranslation`. Give each locale its **full CLDR category set** at conversion time. Carrying the old text over as an `other`-only block does **not** work: lint's `MissingQuantity` is an error, so CI fails long before Crowdin gets a chance to backfill. Mind the declension too — the retained text is usually the plural form, so reusing it verbatim for `one` yields "1 odpowiedzi". 4. Reference: [Android `` docs](https://developer.android.com/guide/topics/resources/string-resource#Plurals) and [CLDR plural rules](https://unicode-org.github.io/cldr-staging/charts/latest/supplemental/language_plural_rules.html).