Fix #6020: Introduce API for surfacing supported audio languages - #6380
Fix #6020: Introduce API for surfacing supported audio languages#6380Kishan8548 wants to merge 6 commits into
Conversation
Fixes oppia#6020 - Added getSupportedAudioLanguages() to TranslationController that filters language definitions by hasAudioTranslationId() instead of hasAppStringId(), correctly surfacing languages like Hinglish which support audio voiceovers but not app strings. - Updated getAudioTranslationContentLocale() to use the new API, resolving TODO(oppia#6020). - Updated AudioLanguageSelectionViewModel to use getSupportedAudioLanguages(), resolving TODO(oppia#6020). - Updated testGetAudioLocale_updateLanguageToHinglish test to expect HINGLISH (correct behavior) instead of ENGLISH fallback. - Added unit test verifying HINGLISH is included in audio languages.
Coverage ReportResultsNumber of files assessed: 2 Passing coverageFiles with passing code coverage
Exempted coverageFiles exempted from coverage
|
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Introduces a dedicated API to expose supported audio voiceover languages (distinct from app-string languages), and updates audio locale resolution + UI selection to use it so languages like Hinglish no longer fall back to English.
Changes:
- Added
getSupportedAudioLanguages()toTranslationControllerfiltering byhasAudioTranslationId(). - Updated audio translation locale resolution and the audio language selection ViewModel to use the new API.
- Updated/added tests to assert Hinglish is included and correctly selected for audio.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| domain/src/main/java/org/oppia/android/domain/translation/TranslationController.kt | Adds supported-audio-languages provider and uses it for audio content locale resolution. |
| app/src/main/java/org/oppia/android/app/options/AudioLanguageSelectionViewModel.kt | Switches supported audio language list source from app languages to audio languages. |
| domain/src/test/java/org/oppia/android/domain/translation/TranslationControllerTest.kt | Updates Hinglish expectation + adds change-detector coverage for audio languages list. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
@Kishan8548 this PR is being marked as draft because the PR description must contain 'Fixes #' or 'Fixes part of #' for each issue the PR is changing, and each one on its own line with no other text. |
- Renamed lambda parameter supportedAppLanguages -> supportedAudioLanguages in getAudioTranslationContentLocale() since getSupportedAudioLanguages() is now passed (not getSupportedAppLanguages()). - Renamed SUPPORTED_AUDIO_LANGUAGES_DATA_PROVIDER_ID -> SUPPORTED_AUDIO_LANGUAGES_COMBINATION_ID to clearly distinguish the combineWith key from the list provider ID.
|
@adhiamboperes PTAL . The changes are ready for review. The remaining CI failure appears to be related to the coverage report artifact, not the code changes. |
|
Unassigning @Kishan8548 since a re-review was requested. @Kishan8548, please make sure you have addressed all review comments. Thanks! |
|
Assigning @Neer-rn for a first pass. |
Neer-rn
left a comment
There was a problem hiding this comment.
Thanks @Kishan8548. Great work I just had few comments, please address them otherwise this PR looks great to me.
|
Unassigning @Neer-rn since the review is done. |
|
Hi @Kishan8548, it looks like some changes were requested on this pull request by @Neer-rn. PTAL. Thanks! |
|
@Neer-rn Addressed the review comments. PTAL. |
|
Unassigning @Kishan8548 since a re-review was requested. @Kishan8548, please make sure you have addressed all review comments. Thanks! |
Neer-rn
left a comment
There was a problem hiding this comment.
I have no issue with the code, I think work is done here but the CI keeps failing PTAL and make sure the CI goes green.
|
Hi @Neer-rn, Thanks! The single test failure was actually the GitHub Actions runner hitting an OOM/timeout limit ( Since it's an infrastructure flake unrelated to these code changes, I've just forced the CI to re-run. Hopefully, the runner gets through it this time! |
Coverage ReportResultsNumber of files assessed: 11 Passing coverageFiles with passing code coverage
Exempted coverageFiles exempted from coverage
|
|
@adhiamboperes First pass review is done. PTAL for final review, thanks! |
|
Unassigning @Kishan8548 since a re-review was requested. @Kishan8548, please make sure you have addressed all review comments. Thanks! |
Neer-rn
left a comment
There was a problem hiding this comment.
LGTM!! Looks like the CI is green. Everything looks clean to me. @adhiamboperes PTAL
|
Will review this later today. |
adhiamboperes
left a comment
There was a problem hiding this comment.
Thanks @Kishan8548! I left a minor comment inline.
Can you test the changes in the UI? You can enable the onboarding flow v2 feature flag via the dev options menu in the app, and test the audio language selection menu during both onboarding and from the options menu.
It is a good sign that the regression tests are not failing, but also manually double checking is great, plus it lets you explore more areas of the app.
| // HINGLISH is now a supported audio language (it has audio_translation_id), so it correctly | ||
| // resolves to HINGLISH rather than falling back to ENGLISH. | ||
| assertThat(context.languageDefinition.language).isEqualTo(HINGLISH) | ||
| // This region comes from the default locale. | ||
| assertThat(context.regionDefinition.region).isEqualTo(REGION_UNSPECIFIED) |
There was a problem hiding this comment.
I think it is fine to remove the comments. Anyone reading this in future will most likely not know that hinglish was previously unsupported. You can move this information to the PR body instead, as it will support future debugging.
Explanation
Fixes #6020
Introduces a
getSupportedAudioLanguages()API inTranslationControllerto correctly surface the list of languages that support audio voiceovers.Problem:
getAudioTranslationContentLocale()andAudioLanguageSelectionViewModelwere both usinggetSupportedAppLanguages()(which filters byhasAppStringId()) to validate audio language selections. This was incorrect — languages like Hinglish have anaudio_translation_idbut noapp_string_id, so they were incorrectly excluded from the supported audio language list and falling back to English.Changes:
getSupportedAudioLanguages()toTranslationControllerfiltering byhasAudioTranslationId().getAudioTranslationContentLocale()to use the new API, resolvingTODO(#6020).AudioLanguageSelectionViewModelto usegetSupportedAudioLanguages(), resolvingTODO(#6020).testGetAudioLocale_updateLanguageToHinglishto expectHINGLISHlocale (correct behavior) instead ofENGLISHfallback.Essential Checklist
Disclosure of LLM Usage
hasAudioTranslationId()as the correct filter, debugging the existing HINGLISH fallback test failure, and code review assistance.