-
Notifications
You must be signed in to change notification settings - Fork 632
Fixes #5039: Force LTR alignment for Terms of Service & Privacy Policy in RTL languages #6198
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Changes from 4 commits
dd20c4d
6ba69aa
2794c01
28bc5bc
57f6d62
4d1bad1
625ce05
5c89b2f
c024633
daf6e2e
58996d9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -49,14 +49,30 @@ kt_android_library( | |
| name = "injectable_auto_localized_app_compat_activity", | ||
| srcs = ["InjectableAutoLocalizedAppCompatActivity.kt"], | ||
| visibility = ["//app:app_visibility"], | ||
| deps = [":injectable_app_compat_activity"], | ||
| deps = [ | ||
| ":injectable_app_compat_activity", | ||
| "//model/src/main/proto:languages_java_proto_lite", | ||
| ], | ||
| ) | ||
|
|
||
| kt_android_library( | ||
| name = "injectable_system_localized_app_compat_activity", | ||
| srcs = ["InjectableSystemLocalizedAppCompatActivity.kt"], | ||
| visibility = ["//app:app_visibility"], | ||
| deps = [":injectable_app_compat_activity"], | ||
| deps = [ | ||
| ":injectable_app_compat_activity", | ||
| "//model/src/main/proto:languages_java_proto_lite", | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed the extra |
||
| ], | ||
| ) | ||
|
|
||
| kt_android_library( | ||
| name = "injectable_english_only_app_compat_activity", | ||
| srcs = ["InjectableEnglishOnlyAppCompatActivity.kt"], | ||
| visibility = ["//app:app_visibility"], | ||
| deps = [ | ||
| ":injectable_app_compat_activity", | ||
| "//model/src/main/proto:languages_java_proto_lite", | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed the remaining
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove the proto also. |
||
| ], | ||
| ) | ||
|
|
||
| kt_android_library( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| package org.oppia.android.app.activity | ||
|
|
||
| import androidx.appcompat.app.AppCompatActivity | ||
| import org.oppia.android.app.model.ForcedActivityLanguageMode | ||
| import org.oppia.android.app.translation.AppLanguageWatcherMixin | ||
|
|
||
| /** | ||
| * An [AppCompatActivity] that facilitates field injection to child activities and constituent | ||
| * fragments that extend [org.oppia.android.app.fragment.InjectableFragment]. | ||
| * | ||
| * This should be extended by activities which should always display in English regardless of the | ||
| * user's selected app or system language (e.g. policies pages which show canonical English | ||
| * content). | ||
| */ | ||
| abstract class InjectableEnglishOnlyAppCompatActivity : InjectableAppCompatActivity() { | ||
|
|
||
| override fun initializeMixin(appLanguageWatcherMixin: AppLanguageWatcherMixin) { | ||
| appLanguageWatcherMixin.initialize(ForcedActivityLanguageMode.USE_ENGLISH) | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| package org.oppia.android.app.policies | ||
|
|
||
| import android.os.Bundle | ||
| import android.view.Gravity | ||
| import android.view.LayoutInflater | ||
| import android.view.View | ||
| import android.view.ViewGroup | ||
|
|
@@ -44,6 +45,15 @@ class PoliciesFragmentPresenter @Inject constructor( | |
| scrollPosition = it.getInt(KEY_SCROLL_Y, 0) | ||
| } | ||
|
|
||
| // Policy content is always in English, so force LTR layout direction and left gravity | ||
| // to ensure proper alignment of all content including list items (<li> tags) even when | ||
| // the app is set to an RTL language. | ||
| binding.policyDescriptionTextView.apply { | ||
| layoutDirection = View.LAYOUT_DIRECTION_LTR | ||
| textDirection = View.TEXT_DIRECTION_LTR | ||
| gravity = Gravity.START | ||
| } | ||
|
|
||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to declare explicitly layout direction for all English only activities?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed the explicit
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks. I am not sure whether we still need to add these two. Could you please check and test on device after removing these? |
||
| setUpContentForTextViews(policiesFragmentArguments.policyPage, binding) | ||
|
|
||
| (binding.root as ScrollView).viewTreeObserver.addOnGlobalLayoutListener { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ package org.oppia.android.app.translation | |
|
|
||
| import androidx.appcompat.app.AppCompatActivity | ||
| import androidx.lifecycle.Observer | ||
| import org.oppia.android.app.model.ForcedActivityLanguageMode | ||
| import org.oppia.android.domain.locale.LocaleController | ||
| import org.oppia.android.domain.oppialogger.OppiaLogger | ||
| import org.oppia.android.domain.profile.ProfileManagementController | ||
|
|
@@ -37,9 +38,10 @@ class AppLanguageWatcherMixin @Inject constructor( | |
| * called before interacting with the locale handler to avoid inadvertent crashes in such | ||
| * situations. | ||
| * | ||
| * @param shouldOnlyUseSystemLanguage whether only the system language should be used | ||
| * @param forcedActivityLanguageMode the [ForcedActivityLanguageMode] indicating how the locale | ||
|
subhajitxyz marked this conversation as resolved.
Outdated
|
||
| * should be resolved for this activity | ||
|
subhajitxyz marked this conversation as resolved.
Outdated
|
||
| */ | ||
| fun initialize(shouldOnlyUseSystemLanguage: Boolean) { | ||
| fun initialize(forcedActivityLanguageMode: ForcedActivityLanguageMode) { | ||
| if (!appLanguageLocaleHandler.isInitialized()) { | ||
| /* The handler might have been de-initialized since bootstrapping. This can generally happen | ||
| * in two cases: | ||
|
|
@@ -68,10 +70,27 @@ class AppLanguageWatcherMixin @Inject constructor( | |
|
|
||
| val currentUserProfileId = profileManagementController.getCurrentProfileId() | ||
|
|
||
| val activityLanguageLocaleDataProvider = when { | ||
| shouldOnlyUseSystemLanguage -> translationController.getSystemLanguageLocale() | ||
| currentUserProfileId == null -> translationController.getSystemLanguageLocale() | ||
| else -> translationController.getAppLanguageLocale(currentUserProfileId) | ||
| val activityLanguageLocaleDataProvider = when (forcedActivityLanguageMode) { | ||
| ForcedActivityLanguageMode.USE_SYSTEM_LANGUAGE -> | ||
| translationController.getSystemLanguageLocale() | ||
| ForcedActivityLanguageMode.USE_ENGLISH -> | ||
| translationController.getEnglishLocale() | ||
| 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() | ||
| } | ||
| } | ||
|
Comment on lines
+84
to
96
|
||
|
|
||
| val liveData = activityLanguageLocaleDataProvider.toLiveData() | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -62,6 +62,7 @@ kt_android_library( | |
| ":app_language_locale_handler", | ||
| "//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", | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove this line.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed the now-unneeded
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also remove proto. |
||
| ], | ||
| ) | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the
forced_activity_language_modedependency because the enum now lives in the translation package.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, also remove the proto.