Fixes #5039: Force LTR alignment for Terms of Service & Privacy Policy in RTL languages#6198
Conversation
…olicy in RTL languages - Add ActivityLanguageMode enum (USE_APP_LANGUAGE, USE_SYSTEM_LANGUAGE, USE_ENGLISH) replacing the boolean shouldOnlyUseSystemLanguage parameter - Create InjectableEnglishOnlyAppCompatActivity for English-only content - Add getEnglishLocale() to TranslationController - Update PoliciesActivity to extend InjectableEnglishOnlyAppCompatActivity - Add textDirection=ltr and textAlignment=textStart to policy description XML - Update all activity base classes and tests to use new enum API - Add BUILD targets and validation exemptions for new activity class
- Add ForcedActivityLanguageMode proto enum to languages.proto (USE_APP_LANGUAGE, USE_SYSTEM_LANGUAGE, USE_ENGLISH) as specified in the contributing guidance - Replace Kotlin ActivityLanguageMode enum with proto ForcedActivityLanguageMode throughout all activity base classes and AppLanguageWatcherMixin - Update PoliciesFragmentPresenter to programmatically enforce LTR layout direction, text direction, and gravity on the policy description text view for proper list item (<li>) alignment in RTL locales - Update all BUILD files with proto dependencies - Update tests to use the new proto enum
There was a problem hiding this comment.
Pull request overview
Fixes RTL misalignment for canonical English policy pages (Terms of Service & Privacy Policy) by forcing English locale/LTR presentation regardless of the user’s selected app/system language.
Changes:
- Introduces
ForcedActivityLanguageModeproto enum and refactorsAppLanguageWatcherMixin.initialize()to use it (including a newUSE_ENGLISHmode). - Adds an English-only injectable base activity and migrates
PoliciesActivityto it. - Forces LTR alignment for policies content via both XML attributes and runtime view configuration.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/assets/test_file_exemptions.textproto | Exempts the new English-only activity from test requirements. |
| scripts/assets/file_content_validation_checks.textproto | Updates validation rules to allow the new injectable activity subclass. |
| model/src/main/proto/languages.proto | Adds ForcedActivityLanguageMode enum to represent activity locale resolution modes. |
| domain/src/main/java/org/oppia/android/domain/translation/TranslationController.kt | Adds getEnglishLocale() to support USE_ENGLISH. |
| app/src/test/java/org/oppia/android/app/translation/AppLanguageWatcherMixinTest.kt | Migrates tests to use the new enum-based API. |
| app/src/main/res/layout/policies_fragment.xml | Adds LTR-related text attributes to the policy description TextView. |
| app/src/main/java/org/oppia/android/app/translation/BUILD.bazel | Adds proto-lite dep for enum usage. |
| app/src/main/java/org/oppia/android/app/translation/AppLanguageWatcherMixin.kt | Refactors initialization to use enum and adds English-locale branch. |
| app/src/main/java/org/oppia/android/app/policies/PoliciesFragmentPresenter.kt | Forces LTR direction/gravity programmatically for policy content. |
| app/src/main/java/org/oppia/android/app/policies/PoliciesActivity.kt | Switches base class to English-only injectable activity. |
| app/src/main/java/org/oppia/android/app/activity/InjectableSystemLocalizedAppCompatActivity.kt | Updates mixin initialization to enum. |
| app/src/main/java/org/oppia/android/app/activity/InjectableEnglishOnlyAppCompatActivity.kt | Adds new injectable base activity that forces English locale. |
| app/src/main/java/org/oppia/android/app/activity/InjectableAutoLocalizedAppCompatActivity.kt | Updates mixin initialization to enum. |
| app/src/main/java/org/oppia/android/app/activity/BUILD.bazel | Adds new build target + proto-lite deps for activities. |
| app/BUILD.bazel | Wires new activity library into the app target. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| val activityLanguageLocaleDataProvider = when (forcedActivityLanguageMode) { | ||
| ForcedActivityLanguageMode.USE_SYSTEM_LANGUAGE -> | ||
| translationController.getSystemLanguageLocale() | ||
| ForcedActivityLanguageMode.USE_ENGLISH -> | ||
| translationController.getEnglishLocale() | ||
| ForcedActivityLanguageMode.USE_APP_LANGUAGE, | ||
| ForcedActivityLanguageMode.FORCED_ACTIVITY_LANGUAGE_MODE_UNSPECIFIED, | ||
| ForcedActivityLanguageMode.UNRECOGNIZED -> { | ||
| if (currentUserProfileId == null) { | ||
| translationController.getSystemLanguageLocale() | ||
| } else { | ||
| translationController.getAppLanguageLocale(currentUserProfileId) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The new USE_ENGLISH branch changes locale-resolution behavior, but the test suite updates only cover the enum migration. Please add a unit test that initializes the mixin with ForcedActivityLanguageMode.USE_ENGLISH and verifies the mixin selects the English locale (and does not switch when the app language changes).
There was a problem hiding this comment.
Implemented. Added coverage that initializes AppLanguageWatcherMixin with ForcedActivityLanguageMode.USE_ENGLISH and verifies English remains selected even after app-language updates. This is now in the branch.
| ForcedActivityLanguageMode.USE_APP_LANGUAGE, | ||
| ForcedActivityLanguageMode.FORCED_ACTIVITY_LANGUAGE_MODE_UNSPECIFIED, | ||
| ForcedActivityLanguageMode.UNRECOGNIZED -> { | ||
| if (currentUserProfileId == null) { | ||
| translationController.getSystemLanguageLocale() | ||
| } else { | ||
| translationController.getAppLanguageLocale(currentUserProfileId) | ||
| } | ||
| } |
There was a problem hiding this comment.
Grouping FORCED_ACTIVITY_LANGUAGE_MODE_UNSPECIFIED and UNRECOGNIZED into the USE_APP_LANGUAGE behavior can hide misconfiguration/serialization issues and make failures harder to diagnose. Consider handling these cases explicitly by logging a warning (via OppiaLogger) and falling back deterministically (e.g., to system language), while keeping USE_APP_LANGUAGE as its own case.
| ForcedActivityLanguageMode.USE_APP_LANGUAGE, | |
| ForcedActivityLanguageMode.FORCED_ACTIVITY_LANGUAGE_MODE_UNSPECIFIED, | |
| ForcedActivityLanguageMode.UNRECOGNIZED -> { | |
| if (currentUserProfileId == null) { | |
| translationController.getSystemLanguageLocale() | |
| } else { | |
| translationController.getAppLanguageLocale(currentUserProfileId) | |
| } | |
| } | |
| ForcedActivityLanguageMode.USE_APP_LANGUAGE -> { | |
| if (currentUserProfileId == null) { | |
| translationController.getSystemLanguageLocale() | |
| } else { | |
| translationController.getAppLanguageLocale(currentUserProfileId) | |
| } | |
| } | |
| ForcedActivityLanguageMode.FORCED_ACTIVITY_LANGUAGE_MODE_UNSPECIFIED, | |
| ForcedActivityLanguageMode.UNRECOGNIZED -> { | |
| oppiaLogger.w( | |
| "AppLanguageWatcherMixin", | |
| "Unexpected ForcedActivityLanguageMode: $forcedActivityLanguageMode. " + | |
| "Falling back to system language." | |
| ) | |
| translationController.getSystemLanguageLocale() | |
| } |
There was a problem hiding this comment.
Implemented. FORCED_ACTIVITY_LANGUAGE_MODE_UNSPECIFIED and UNRECOGNIZED are now handled explicitly with a warning log and deterministic fallback to system language, while USE_APP_LANGUAGE remains its own path.
| android:textAlignment="textStart" | ||
| android:textColor="@color/component_color_shared_primary_text_color" | ||
| android:textColorLink="@color/component_color_shared_link_text_color" | ||
| android:textDirection="ltr" |
There was a problem hiding this comment.
The alignment/direction is now enforced both in XML and in PoliciesFragmentPresenter, but the XML-only configuration here may not actually guarantee left alignment in RTL contexts since textStart depends on layout direction (not textDirection). To reduce drift and make the behavior self-contained, consider either (a) moving the full configuration to XML (e.g., include android:layoutDirection="ltr" and/or android:gravity="start") and removing the runtime overrides, or (b) removing these XML attributes and relying solely on the presenter.
| android:textAlignment="textStart" | |
| android:textColor="@color/component_color_shared_primary_text_color" | |
| android:textColorLink="@color/component_color_shared_link_text_color" | |
| android:textDirection="ltr" | |
| android:textColor="@color/component_color_shared_primary_text_color" | |
| android:textColorLink="@color/component_color_shared_link_text_color" |
There was a problem hiding this comment.
Addressed by choosing one source of truth. I removed the XML alignment and direction attrs and kept runtime enforcement in PoliciesFragmentPresenter to avoid drift between XML and code paths.
|
Hi @BenHenning, all review feedback has been addressed and pushed, including the Copilot suggestions and the related failing regex test fix. |
Coverage ReportResultsCoverage Analysis: SKIP ⏭️ This PR did not introduce any changes to Kotlin source or test files.
|
|
@subhajitxyz, please take a first pass on this. |
subhajitxyz
left a comment
subhajitxyz
left a comment
There was a problem hiding this comment.
@adhiamboperes , I have taken a first pass on it and leave some comment.
app/src/main/java/org/oppia/android/app/translation/AppLanguageWatcherMixin.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/org/oppia/android/app/translation/AppLanguageWatcherMixin.kt
Outdated
Show resolved
Hide resolved
|
Thanks, @nikhilkumarpanigrahi , I have left some comment. PTAL. |
|
Hi @subhajitxyz, |
|
Unassigning @nikhilkumarpanigrahi since a re-review was requested. @nikhilkumarpanigrahi, please make sure you have addressed all review comments. Thanks! |
|
@nikhilkumarpanigrahi , Some checks has failed, please fix those. |
APK & AAB differences analysisNote that this is a summarized snapshot. See the CI artifacts for detailed differences. DevExpand to see flavor specificsUniversal APKAPK file size: 19 MiB (old), 19 MiB (new), 260 bytes (Added) APK download size (estimated): 18 MiB (old), 18 MiB (new), 1191 bytes (Added) Method count: 265367 (old), 265371 (new), 4 (Added) Features: 1 (old), 1 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 7118 (old), 7118 (new), 0 (No change)
Lesson assets: 113 (old), 113 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 19 MiB (old), 19 MiB (new), 264 bytes (Added) Configuration hdpiAPK file size: 50 KiB (old), 50 KiB (new), 0 bytes (No change) Configuration ldpiAPK file size: 49 KiB (old), 49 KiB (new), 0 bytes (No change) Configuration mdpiAPK file size: 46 KiB (old), 46 KiB (new), 0 bytes (No change) Configuration tvdpiAPK file size: 86 KiB (old), 86 KiB (new), 0 bytes (No change) Configuration xhdpiAPK file size: 57 KiB (old), 57 KiB (new), 0 bytes (No change) Configuration xxhdpiAPK file size: 63 KiB (old), 63 KiB (new), 0 bytes (No change) Configuration xxxhdpiAPK file size: 64 KiB (old), 64 KiB (new), 0 bytes (No change) AlphaExpand to see flavor specificsUniversal APKAPK file size: 11 MiB (old), 11 MiB (new), 56 bytes (Added) APK download size (estimated): 10 MiB (old), 10 MiB (new), 4854 bytes (Added) Method count: 118609 (old), 118586 (new), 23 (Removed) Features: 1 (old), 1 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 6052 (old), 6052 (new), 0 (No change)
Lesson assets: 114 (old), 114 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 11 MiB (old), 11 MiB (new), 60 bytes (Added) Configuration hdpiAPK file size: 43 KiB (old), 43 KiB (new), 0 bytes (No change) Configuration ldpiAPK file size: 45 KiB (old), 45 KiB (new), 0 bytes (No change) Configuration mdpiAPK file size: 38 KiB (old), 38 KiB (new), 0 bytes (No change) Configuration tvdpiAPK file size: 73 KiB (old), 73 KiB (new), 0 bytes (No change) Configuration xhdpiAPK file size: 50 KiB (old), 50 KiB (new), 0 bytes (No change) Configuration xxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) Configuration xxxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) BetaExpand to see flavor specificsUniversal APKAPK file size: 11 MiB (old), 11 MiB (new), 220 bytes (Removed) APK download size (estimated): 10 MiB (old), 10 MiB (new), 276 bytes (Added) Method count: 118613 (old), 118590 (new), 23 (Removed) Features: 1 (old), 1 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 6052 (old), 6052 (new), 0 (No change)
Lesson assets: 114 (old), 114 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 11 MiB (old), 11 MiB (new), 224 bytes (Removed) Configuration hdpiAPK file size: 43 KiB (old), 43 KiB (new), 0 bytes (No change) Configuration ldpiAPK file size: 45 KiB (old), 45 KiB (new), 0 bytes (No change) Configuration mdpiAPK file size: 38 KiB (old), 38 KiB (new), 0 bytes (No change) Configuration tvdpiAPK file size: 73 KiB (old), 73 KiB (new), 0 bytes (No change) Configuration xhdpiAPK file size: 50 KiB (old), 50 KiB (new), 0 bytes (No change) Configuration xxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) Configuration xxxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) GaExpand to see flavor specificsUniversal APKAPK file size: 11 MiB (old), 11 MiB (new), 340 bytes (Removed) APK download size (estimated): 10 MiB (old), 10 MiB (new), 191 bytes (Removed) Method count: 118613 (old), 118590 (new), 23 (Removed) Features: 1 (old), 1 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 6052 (old), 6052 (new), 0 (No change)
Lesson assets: 114 (old), 114 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 11 MiB (old), 11 MiB (new), 340 bytes (Removed) Configuration hdpiAPK file size: 43 KiB (old), 43 KiB (new), 0 bytes (No change) Configuration ldpiAPK file size: 45 KiB (old), 45 KiB (new), 0 bytes (No change) Configuration mdpiAPK file size: 38 KiB (old), 38 KiB (new), 0 bytes (No change) Configuration tvdpiAPK file size: 73 KiB (old), 73 KiB (new), 0 bytes (No change) Configuration xhdpiAPK file size: 50 KiB (old), 50 KiB (new), 0 bytes (No change) Configuration xxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) Configuration xxxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) |
The test file ForcedActivityLanguageModeTest.kt was added but the corresponding BUILD.bazel file in the test directory was missing, causing Bazel to fail with 'no such package' error during test discovery. This commit adds the BUILD.bazel file with the necessary kt_jvm_test target to make the test discoverable and runnable by Bazel.
Coverage ReportResultsNumber of files assessed: 15 Passing coverageFiles with passing code coverage
Exempted coverageFiles exempted from coverage
|
|
Hi @subhajitxyz, all checks have passed now PTAL. |
|
Unassigning @nikhilkumarpanigrahi since a re-review was requested. @nikhilkumarpanigrahi, please make sure you have addressed all review comments. Thanks! |
subhajitxyz
left a comment
subhajitxyz
left a comment
There was a problem hiding this comment.
I’ve taken a pass on this and left a few comments. @adhiamboperes , there are a few decisions(mentioned) I’m unsure about, so passing it to you for further review.
| @@ -0,0 +1,13 @@ | |||
| package org.oppia.android.app.model | |||
There was a problem hiding this comment.
I am not sure about creating a new package for these enum. Simply, we can keep it into app/src/main/java/org/oppia/android/app/translation or inside AppLanguageWatcherMixin.kt file.
@adhiamboperes what's your thought on this?
There was a problem hiding this comment.
Moved ForcedActivityLanguageMode into the translation package so it stays with the locale-handling code and avoids introducing a separate app.modelpackage for a single enum.
There was a problem hiding this comment.
Thanks @nikhilkumarpanigrahi . But still Keeping this conversation open for @adhiamboperes 's suggestion.
| textDirection = View.TEXT_DIRECTION_LTR | ||
| gravity = Gravity.START | ||
| } | ||
|
|
There was a problem hiding this comment.
Do we need to declare explicitly layout direction for all English only activities?
I am not sure about whether this function in InjectableAppCompatActivity handles layout direction for all activities or not.
@adhiamboperes What's you thought on this ?
There was a problem hiding this comment.
Removed the explicit layoutDirection override since InjectableAppCompatActivity already handles layout direction; kept textDirection and gravity to preserve the English-only policy formatting
There was a problem hiding this comment.
Thanks. I am not sure whether we still need to add these two. Could you please check and test on device after removing these?
| deps = [ | ||
| ":injectable_app_compat_activity", | ||
| "//app/src/main/java/org/oppia/android/app/model:forced_activity_language_mode", | ||
| "//model/src/main/proto:languages_java_proto_lite", |
There was a problem hiding this comment.
Removed the forced_activity_language_mode dependency because the enum now lives in the translation package.
There was a problem hiding this comment.
Thanks, also remove the proto.
| deps = [ | ||
| ":injectable_app_compat_activity", | ||
| "//app/src/main/java/org/oppia/android/app/model:forced_activity_language_mode", | ||
| "//model/src/main/proto:languages_java_proto_lite", |
There was a problem hiding this comment.
Removed the extra forced_activity_language_mode dependency here as well after relocating the enum.
| deps = [ | ||
| ":injectable_app_compat_activity", | ||
| "//app/src/main/java/org/oppia/android/app/model:forced_activity_language_mode", | ||
| "//model/src/main/proto:languages_java_proto_lite", |
There was a problem hiding this comment.
Removed the remaining forced_activity_language_mode dependency for the same reason, since the enum is now part of translation
There was a problem hiding this comment.
Remove the proto also.
app/src/main/java/org/oppia/android/app/translation/AppLanguageWatcherMixin.kt
Outdated
Show resolved
Hide resolved
| "//app/src/main/java/org/oppia/android/app/model:forced_activity_language_mode", | ||
| "//domain/src/main/java/org/oppia/android/domain/profile:profile_management_controller", | ||
| "//domain/src/main/java/org/oppia/android/domain/translation:translation_controller", | ||
| "//model/src/main/proto:languages_java_proto_lite", |
There was a problem hiding this comment.
Removed the now-unneeded forced_activity_language_mode dependency after moving the enum into the translation package.
Coverage ReportResultsNumber of files assessed: 14 Passing coverageFiles with passing code coverage
Exempted coverageFiles exempted from coverage
|
|
Hi @subhajitxyz! addressed all the comments and made the changes PTAL when you get a chance. |
|
Thanks @nikhilkumarpanigrahi , left few feedbacks. Please update PR description based on current approach. |
Explanation
Fixes #5039
This PR fixes the issue where Terms of Service and Privacy Policy content was incorrectly right-aligned in RTL (Right-to-Left) languages like Arabic. Since these policies are canonical English content, they should always display in English with left-to-right alignment regardless of the user's selected app or system language.
Specifically, this PR:
ForcedActivityLanguageModeproto enum tolanguages.protowith valuesUSE_APP_LANGUAGE,USE_SYSTEM_LANGUAGE, andUSE_ENGLISHto represent the three possible activity language modes.InjectableEnglishOnlyAppCompatActivity.kt, a new base activity that forces the English locale via theUSE_ENGLISHmode, intended for activities displaying canonical English content.AppLanguageWatcherMixin.initialize()to acceptForcedActivityLanguageMode(proto enum) instead of the previousshouldOnlyUseSystemLanguage: Booleanparameter, enabling cleaner support for the new English-only mode.InjectableAutoLocalizedAppCompatActivityandInjectableSystemLocalizedAppCompatActivityto use the new proto enum.getEnglishLocale()toTranslationControllerto return aDataProvider<OppiaLocale.DisplayLocale>for the English language.PoliciesActivityto extendInjectableEnglishOnlyAppCompatActivityinstead ofInjectableAutoLocalizedAppCompatActivity.PoliciesFragmentPresenterto programmatically setLAYOUT_DIRECTION_LTR,TEXT_DIRECTION_LTR, andGravity.STARTon the policy description text view to ensure proper left-alignment of list items (<li>tags) and bullet points.android:textDirection="ltr"andandroid:textAlignment="textStart"topolicies_fragment.xmlfor the policy description text view.file_content_validation_checks.textprototo add exemptions forInjectableEnglishOnlyAppCompatActivityin theInjectableAppCompatActivitysubclass check and the screen name check.InjectableEnglishOnlyAppCompatActivityintest_file_exemptions.textprotosince it is a thin base class with no independent logic.AppLanguageWatcherMixinTestto use the newForcedActivityLanguageModeproto enum instead of the old boolean parameter.languages_java_proto_litedependency to the relevant BUILD targets (app_language_watcher_mixin,injectable_auto_localized_app_compat_activity,injectable_system_localized_app_compat_activity,injectable_english_only_app_compat_activity).Essential Checklist
For UI-specific PRs only
If your PR includes UI-related changes, then:
Testing performed:
Devices/Android versions tested on:
Screenshots/Videos:
ISSUE.5039.UI.TESTING.mov