Skip to content

Feature/spanish i18n - #201

Open
miguelsg29 wants to merge 13 commits into
starbrightlab:mainfrom
miguelsg29:feature/spanish-i18n
Open

Feature/spanish i18n#201
miguelsg29 wants to merge 13 commits into
starbrightlab:mainfrom
miguelsg29:feature/spanish-i18n

Conversation

@miguelsg29

Copy link
Copy Markdown
Contributor

Thanks for shipping the Google Photos fix in v1.65!
As requested, I've created a dedicated feature branch feature/spanish-i18n rebased onto current main (v1.67) with only the i18n changes, and opened a fresh PR for it.
I've also rebased the screensaver transition PR (#166) on top of current main. Thanks!

@starbrightlab starbrightlab left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for splitting this out onto its own branch — much easier to consider on its own. Three things, one a straightforward bug and one that's a decision rather than a defect.

The Home Assistant setup steps will render their numbers twice

MultiRoomStep(n, text) draws the number itself:

private fun MultiRoomStep(n: String, text: String) {
  Row(...) {
    Text("$n.", ...)     // ← the number
    Text(text, ...)
  }
}

The Spanish translations embed their own on top of that:

"In Home Assistant, add the Mosquitto broker add-on…" -> "1. En Home Assistant, añade el complemento Mosquitto broker…"
"Turn on the toggle below and enter your broker's address…" -> "2. Activa el interruptor…"
"This Portal appears automatically under Settings → Devices…" -> "3. Este Portal aparecerá automáticamente…"

So all three steps on the Home Assistant screen render as "1. 1. En Home Assistant…". The English originals correctly carry no number. Fix is to drop the 1. / 2. / 3. prefixes from those three strings. Worth grepping the rest of the map for the same pattern anywhere else a caller supplies the marker.

Needs a rebase, and it'll be a real one

The base here is 13432318. Since then SettingsDomains.kt and ImmortalSettingsActivity.kt have both changed substantially across #208 and #210 — the MQTT screen gained a sensors section and then lost a row again, and the mqtt and immortal domains both changed shape. This PR touches both files heavily, so expect genuine conflicts rather than a clean replay. The green CI here is from 11 Aug and doesn't reflect the current tree.

The architecture is worth an explicit decision

This introduces a 779-line I18n.kt that maps hardcoded English strings to translations at runtime, called as I18n.translate("Some English sentence", userLang) at each site. That's a bigger commitment than the diff suggests: every future string in the app inherits the choice, and the English text becomes a lookup key, so a later copy edit silently drops the translation back to English with nothing to catch it.

Android's own resources are the conventional alternative (strings.xml + values-es/strings.xml, which this PR already adds a small one of) — compile-time keys, no key drift, per-locale fallback handled by the platform, and no runtime map. The tradeoff is a much larger mechanical diff to extract the strings.

I'm not asking you to redo it — I'd just rather @starbrightlab make that call deliberately before it lands, since it's hard to reverse once the codebase is written against it. Flagging it as a question, not a change request.

The translations themselves read well, and the coverage of the Home Assistant screens in particular is thorough.


Generated by Claude Code

@miguelsg29
miguelsg29 force-pushed the feature/spanish-i18n branch from 9d9db40 to 727641b Compare August 18, 2026 18:19
@miguelsg29

Copy link
Copy Markdown
Contributor Author

Thanks — that's a fair thing to flag, and I agree strings.xml is the right long-term destination.

The argument that convinces me isn't key drift so much as contributor cost: once Spanish lands, German/French/Portuguese will follow, and with a single 779-line I18n.kt every translator edits the same Kotlin file — each language PR conflicts with the last, and you end up reviewing code diffs to approve translations. With values-de/, values-fr/ and so on, each contributor touches a file nobody else does, the diff is plain text, and it opens the door to Weblate or Crowdin, both of which speak Android resources natively and neither of which speaks a Kotlin map. Plus platform plurals, per-locale fallback, lint MissingTranslation to catch gaps per language, and stringResource() working inside @Preview.

One caveat on scope: the App Store strings in this PR include app descriptions. If those ever come from a remote catalog or from APK metadata they can't live in strings.xml, so I'd expect the end state to be resources for all UI chrome plus a small runtime map only for externally-sourced text.

The good news is that this isn't a one-way door. stringResource(R.string.x) and I18n.translate(...) coexist without friction, so the migration can go screen by screen and stop at any point without leaving the codebase half-consistent. Concretely I'd suggest: land this as-is to unblock Spanish, open a tracking issue for the incremental migration, and adopt a rule from the merge onward that new strings go straight to strings.xml, so the map stops growing while the plan gets made. Happy to migrate one small screen first as a proof of the pattern — worth noting that an in-app language picker on Portal-era Android needs a ContextWrapper over createConfigurationContext(), since LocaleManager is API 33.

On the concrete asks: the 1. / 2. / 3. prefixes are gone from the Home Assistant steps, and the multi-room ones carried the same pattern so those are fixed too. I grepped the rest of the map and found no other cases, and added a test in I18nTest.kt asserting that no translation value starts with a list marker, so this particular bug can't come back.

Rebased onto current main. Only ImmortalSettings.kt needed hand resolution (cameraEnabled/portalPresence landing next to setLanguage); SettingsDomains.kt and ImmortalSettingsActivity.kt merged cleanly since the changes didn't overlap in the same lines.

One thing to flag because it goes beyond the original diff: the sensors and camera sections that landed in main since this branch was cut were rendering untranslated, so the MQTT and settings screens came out half-English in Spanish. I wrapped those strings and added the translations rather than leave the screen inconsistent. Happy to pull them into a separate follow-up PR if you'd rather keep this one to its original scope.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants