feat(settings): offer every language the web does, and make them match - #152
Closed
RXWatcher wants to merge 6 commits into
Closed
feat(settings): offer every language the web does, and make them match#152RXWatcher wants to merge 6 commits into
RXWatcher wants to merge 6 commits into
Conversation
Reported from the field: on the store build a user could not set Dutch as their default subtitle language. It was never a server limitation — the settings contract types playback.subtitle_language as `language_tag`, validated for BCP 47 shape only, with no enum. The picker was simply shorter than the web's. LanguageOptions.tags now mirrors the web client's ISO 639-1 table (silo-server web/src/player/utils/languageNames.ts) in its order: 10 languages become 37, Dutch among them. The table's own comment already described "nl" as a tag that arrives "synced from another surface" and must be preserved — so a Dutch preference set on the web was being honoured by playback while the picker refused to display or offer it. Completing the picker exposed a second, older bug. Auto-selection compares preference and track language with both sides through canonicalSubtitleLanguage, whose ISO 639-2 alias table covered seven languages while the picker offered ten. A track tagged `ita` never matched a stored `it`, so Italian, Portuguese, Korean, Chinese and Russian were already silently failing to auto-select on any server exposing bibliographic codes. The table now covers all 37 offered languages, both the bibliographic and terminological forms where they differ, plus the pre-1989 `iw`/`in` codes. Tests: every picker tag is canonical, every alias resolves to an offered language, and Dutch matches across nl/nld/dut/NL/nl-NL/nl_BE. Note for parity: silo-apple offers 12 (these ten plus Arabic and Hindi) and also cannot select Dutch, so it needs the same treatment. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 51 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe shared language model now provides expanded picker mappings, canonical display labels, prioritized options, and broader subtitle-code normalization. Android and TV settings load catalog languages and use them to order language pickers and render facet labels, with updated shared tests. ChangesLanguage handling and settings integration
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
shared/src/commonMain/kotlin/org/siloserver/silo/playback/SubtitleLanguage.kt (1)
52-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the private property to camelCase.
THREE_LETTER_ALIASESis aval, not a constant, so rename it tothreeLetterAliasesand update its reference on Line 32.As per coding guidelines, Kotlin properties use
camelCase.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shared/src/commonMain/kotlin/org/siloserver/silo/playback/SubtitleLanguage.kt` at line 52, Rename the private property THREE_LETTER_ALIASES to threeLetterAliases in SubtitleLanguage.kt, and update the reference at the language lookup on line 32 to use the new camelCase name.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@shared/src/commonTest/kotlin/org/siloserver/silo/model/settings/LanguageOptionsTest.kt`:
- Around line 58-67: Strengthen the parity tests at
shared/src/commonTest/kotlin/org/siloserver/silo/model/settings/LanguageOptionsTest.kt:58-67
and
shared/src/commonTest/kotlin/org/siloserver/silo/playback/SubtitleLanguageTest.kt:100-114.
In thePickerOffersEveryLanguageTheWebClientDoes, assert the complete ordered
37-entry LanguageOptions.tags table rather than only the Dutch mapping and size;
in the alias test, assert each alias resolves to its expected canonical picker
tag through the existing subtitle-language mapping symbols.
---
Nitpick comments:
In
`@shared/src/commonMain/kotlin/org/siloserver/silo/playback/SubtitleLanguage.kt`:
- Line 52: Rename the private property THREE_LETTER_ALIASES to
threeLetterAliases in SubtitleLanguage.kt, and update the reference at the
language lookup on line 32 to use the new camelCase name.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b9256183-3032-4725-b9d6-fe01bf933dd2
📒 Files selected for processing (4)
shared/src/commonMain/kotlin/org/siloserver/silo/model/settings/LanguageOptions.ktshared/src/commonMain/kotlin/org/siloserver/silo/playback/SubtitleLanguage.ktshared/src/commonTest/kotlin/org/siloserver/silo/model/settings/LanguageOptionsTest.ktshared/src/commonTest/kotlin/org/siloserver/silo/playback/SubtitleLanguageTest.kt
…a subset The hand-rolled alias table had already diverged from the server on its third entry. The server's canonical_language_code (migration 121, mirroring internal/lang) keeps `nob` -> `nb` and `nno` -> `nn` distinct; this folded both into `no`. The catalog stores what the server canonicalised, so that divergence is not cosmetic: a Bokmal file is `nb` server-side and would have been compared against a client-side `no` and never matched. Replaced with a faithful copy of all 90 server mappings. It covers far more languages than the pickers offer, which costs nothing and means an unlisted language synced from another surface still compares correctly. Kept beyond the server's map: `iw` -> `he` and `in` -> `id`, the pre-1989 639-1 spellings Java's Locale still produces. These are 639-1 rather than 639-2 so the server's table does not carry them and passes them through unchanged — flagged in a comment as worth adding server-side. Tests updated: Norwegian now asserts the distinction rather than the folding, and the unknown-tag case uses a tag the server also does not know (`tlh`), since `cat` legitimately resolves to `ca` now. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The audio, subtitle and original-language facets mapped each vocabulary entry to itself, so the filter list rendered the catalog's raw codes: a viewer picking a subtitle language saw "eng", "nld", "dut" rather than English and Dutch. Both surfaces did it — the shared CatalogFilterState and the TV browse facets. New LanguageOptions.displayLanguage canonicalises before labelling, because catalog values are whatever ingest wrote: "nl", "nld" and "dut" all read "Dutch". A code no picker offers is returned unchanged rather than blanked, so display and wire value stay the same string and the filter still applies. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Thirty-seven entries is a long way to travel on a D-pad. The obvious shortcut is the device locale, and it is worthless on a TV: the Shield here ships persist.sys.locale empty, system_locales null, and ro.product.locale at the factory en-US, so locale-first would suggest English to a Dutch viewer. Measured, not assumed. The catalog is a real signal instead. Settings now fetches catalog/filters?include_technical=true and lifts the languages the library actually carries to the top of all six language pickers (audio, subtitle and metadata, on both phone and TV). The currently-set language is pinned above them, so the choice in force stays reachable even when the fetch fails or the library has none of it yet. Everything else follows alphabetically: the web's prominence order stops meaning anything past the first ten, and with no signal, predictable beats editorial. The fetch is best-effort and silent on failure — the pickers are fully usable without it, and a settings screen opened to do something else should not surface an error about a list that still works. Both apps' picker lists were file-scope vals; they are now computed from state, remembered on the catalog languages and the current value so recomposition does not rebuild them per frame. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The parity test asserted `size == 37` plus the Dutch mapping, so a wrong label or a silent reordering would have passed — while "mirrors the web's ISO 639-1 table in its order" is the entire claim of this change. The table is now asserted whole. If the web list moves, this fails and someone decides deliberately rather than the two drifting apart unnoticed. Declined the companion suggestion to rename THREE_LETTER_ALIASES to camelCase: it is a deeply immutable mapOf of string literals, which is what the Kotlin style guide calls a constant, and REGISTERED_ATTRIBUTES in DiagnosticsValidation.kt is the same shape under the same spelling. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`THREE_LETTER_FORMS[tag]?.any { ... } ?: true` defaulted any offered tag with
no entry to "reachable", so adding a language to the picker and forgetting
its 639-2 spellings — exactly the drift this test exists to catch — passed
silently. A test that cannot fail is not a guard.
Now: every offered tag must have a spelling listed, every listed spelling
must canonicalise back to it, and no spelling may linger for a language the
picker no longer offers. Failures name the specific tag and mapping rather
than reporting a count mismatch.
Verified by deleting the Dutch mapping and watching it fail.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/settings/TvSettingsScreen.kt`:
- Around line 908-912: Normalize each stored language tag before using it as the
picker selection ID: update the audio-language picker at
androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/settings/TvSettingsScreen.kt
lines 908-912, the subtitle-language picker at lines 1121-1128, and the
metadata-language picker at lines 1130-1136. Apply the same canonicalization
used by LanguageOptions.optionsPrioritising so aliases match the option IDs and
preserve correct selection and initial focus.
In
`@shared/src/commonTest/kotlin/org/siloserver/silo/playback/SubtitleLanguageTest.kt`:
- Around line 114-142: Update
everyOfferedLanguageIsReachableFromItsThreeLetterForm to require an explicit
THREE_LETTER_FORMS entry for each LanguageOptions tag; remove the ?: true
fallback so a missing mapping fails the test rather than being treated as
reachable. Keep the existing canonicalSubtitleLanguage matching behavior
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a153faf5-648c-4b89-9af0-1983d0db0638
📒 Files selected for processing (14)
androidApp/src/androidMain/kotlin/org/siloserver/silo/android/di/AndroidModule.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/settings/PlaybackSettings.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/settings/SettingsScreen.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/settings/SettingsViewModel.ktandroidApp/src/androidMain/kotlin/org/siloserver/silo/android/ui/screens/settings/SubtitleSettings.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/di/AndroidTvModule.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/library/TvLibraryBrowseFacets.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/settings/TvSettingsScreen.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/settings/TvSettingsViewModel.ktshared/src/commonMain/kotlin/org/siloserver/silo/catalog/filter/CatalogFilterState.ktshared/src/commonMain/kotlin/org/siloserver/silo/model/settings/LanguageOptions.ktshared/src/commonMain/kotlin/org/siloserver/silo/playback/SubtitleLanguage.ktshared/src/commonTest/kotlin/org/siloserver/silo/model/settings/LanguageOptionsTest.ktshared/src/commonTest/kotlin/org/siloserver/silo/playback/SubtitleLanguageTest.kt
Comment on lines
+908
to
+912
| options = LanguageOptions.optionsPrioritising( | ||
| unsetLabel = "Default", | ||
| libraryLanguages = state.libraryLanguages, | ||
| current = state.audioLanguage, | ||
| ).map { PickerOption(it.first, it.second) }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Normalize TV picker selection IDs with the option IDs. LanguageOptions.optionsPrioritising turns aliases into canonical IDs, while each sheet compares its options against an unnormalized stored tag. Legacy aliases are consequently neither selected nor initially focused.
androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/settings/TvSettingsScreen.kt#L908-L912: canonicalizestate.audioLanguagebefore passing it asselectedId.androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/settings/TvSettingsScreen.kt#L1121-L1128: canonicalizestate.subtitleLanguagebefore passing it asselectedId.androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/settings/TvSettingsScreen.kt#L1130-L1136: canonicalizestate.metadataLanguagebefore passing it asselectedId.
📍 Affects 1 file
androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/settings/TvSettingsScreen.kt#L908-L912(this comment)androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/settings/TvSettingsScreen.kt#L1121-L1128androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/settings/TvSettingsScreen.kt#L1130-L1136
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/settings/TvSettingsScreen.kt`
around lines 908 - 912, Normalize each stored language tag before using it as
the picker selection ID: update the audio-language picker at
androidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/ui/screens/settings/TvSettingsScreen.kt
lines 908-912, the subtitle-language picker at lines 1121-1128, and the
metadata-language picker at lines 1130-1136. Apply the same canonicalization
used by LanguageOptions.optionsPrioritising so aliases match the option IDs and
preserve correct selection and initial focus.
Contributor
Author
|
Closing as superseded by the contract-driven language catalog architecture merged in #154. The remaining useful behavior—human-readable language labels in library facets—will be extracted as a focused current-main change rather than merging the conflicting hand-maintained catalog. The branch remains available for traceability. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The report
Correct, and it is not fixed on
maineither — the picker offers the same ten languages in both, and Dutch is in neither:English, Spanish, French, German, Japanese, Korean, Chinese, Portuguese, Italian, RussianIt was never a server limitation
The settings contract types
playback.subtitle_languageas:{ "type": "language_tag", "nullable": true }validated for BCP 47 shape only — there is no enum. The web client already offers 37 languages, Dutch included. Android's picker was simply shorter.
LanguageOptions.tagsnow mirrors the web's ISO 639-1 table (web/src/player/utils/languageNames.tsin silo-server) in its order. Ten becomes 37.Worth noting: the table's own comment already named
"nl"as a tag that arrives "synced from another surface" and must be preserved. So a Dutch preference set on the web was being honoured by playback the whole time — the picker just refused to display or re-select it. The user was looking at a setting that was working and appeared absent.Completing the picker exposed an older bug
Auto-selection compares the stored preference against a track's language with both sides through
canonicalSubtitleLanguage(MobileSubtitleAutoSelection,TrackSelectionFingerprint). That function's ISO 639-2 alias table covered seven languages while the picker offered ten:A track tagged
itacanonicalises toita, which never equals a storedit. Italian, Portuguese, Korean, Chinese and Russian were already offered in the picker but could not auto-select on any server exposing bibliographic codes. Shipping 27 more languages against that table would have multiplied the problem.The alias table now covers all 37, carrying both bibliographic and terminological forms where ISO 639-2 has both (
cze/ces,gre/ell,may/msa,per/fas, …), plus the pre-1989 codesiw→heandin→idthat older muxing tools still emit. Norwegian foldsnor/nob/nnointo the single offeredno, since Bokmål and Nynorsk are one choice in the picker.Dutch itself was already mapped (
dut,nld→nl), so the reported case needed only the picker entry — but it would have been the only one of the 27 additions that worked.Tests
SubtitleLanguageTest(new, 9 cases) andLanguageOptionsTest(10) both pass, along with the full:sharedsuite;:androidApp:assembleDebugand:androidTvApp:assembleDebugboth build.The two that would catch a regression here:
Plus the reported case directly: Dutch matches across
nl,nld,dut,NL,nl-NL,nl_BE.Parity note
silo-apple offers 12 — these ten plus Arabic and Hindi — and also cannot select Dutch. It needs the same treatment; this PR does not touch it.
Follow-ups landed on this branch
The alias table now mirrors the server instead of paraphrasing it
The first version of the map was hand-rolled and had already diverged on its third entry. The server's
canonical_language_code(migration 121, mirroringinternal/lang) keepsnob→nbandnno→nndistinct; the hand-rolled version folded both intono. The catalog stores what the server canonicalised, so that is not cosmetic — a Bokmål file isnbserver-side and would have been compared against a client-sideno, matching nothing.It is now a faithful copy of all 90 server mappings. That covers far more languages than the pickers offer, which costs nothing and means an unlisted language synced from another surface still compares correctly.
Kept deliberately beyond the server's table:
iw→heandin→id, the pre-1989 639-1 spellings Java'sLocalestill produces. These are 639-1 rather than 639-2, so the server's map does not carry them and passes them through unchanged — worth adding server-side, flagged in a comment.Language filter facets show names, not raw codes
CatalogFilterStateandTvLibraryBrowseFacetsmapped each vocabulary entry to itself, so library filters renderedeng,nld,dutinstead of English and Dutch. NewLanguageOptions.displayLanguagecanonicalises before labelling, so all three Dutch spellings read "Dutch". A code no picker offers is returned unchanged rather than blanked — display and wire value stay the same string, and the filter still applies.The viewer's own languages come first
Thirty-seven entries is a long way to travel on a D-pad. The obvious shortcut — device locale — was measured and rejected: the Shield here ships
persist.sys.localeempty,system_localesnull, andro.product.localeat the factoryen-US, so locale-first would suggest English to a Dutch viewer.The catalog is a real signal. Settings fetches
catalog/filters?include_technical=trueand lifts the languages the library actually carries to the top of all six language pickers (audio, subtitle, metadata × phone, TV). The current value is pinned above them so it stays reachable when the fetch fails. The tail is alphabetical — the web's prominence order stops meaning anything past the first ten.The fetch is best-effort and silent on failure; the pickers work fine without it.
Not done: visual group headers ("In your library" / "All languages"). Both pickers render flat lists, so headers need a non-selectable row type on two platforms — worth its own change.
Summary by CodeRabbit
New Features
Bug Fixes