Apply user language selected in WordPress app to Jetpack app during migration by ovitrif · Pull Request #17768 · wordpress-mobile/WordPress-Android

ovitrif · 2023-01-16T18:13:25Z

This PR enhances the implementation of the Jetpack Content Migration Flow by applying the interface language set by the user in the WordPress app to the Migration screens.

Note:
During implementation I experimented with the widely popular approach of inheriting from LocaleAwareActivity in the JetpackMigrationActivity, but it did not work correctly in cases when the WordPress app theme is also customized by the user:
ℹ️ More specifically upon navigating to another non-migration specific screen (Help / My Site / Login), if the user changes the system theme to be different than the custom app theme migrated from the WordPress app (system theme = light, Jetpack app theme = dark) the next screen could end up not translated into the user language.
ℹ️ On top of this issue, using LocaleAwareActivity is not working correctly with Compose, or at least not for the JetpackMigrationActivity, where recreating the fragment is avoided in order fix the error when changing the device settings mid-flow.
🚀 As a solution to these issues I introduced the new LocaleAwareComposable utility that listens to language changes and synchronizes the local configuration inside its content composable when needed.

To test:

Build, install, launch the WordPress app and login
Go to Me → App Settings → Appearance and Set the WordPress app theme to Dark mode (or the opposite of the system default)
Go to Interface Language and change the app language to Arabic (or a language you know, Arabic helps to verify the RTL texts)
Build and install the Jetpack app of the same flavor
Open Jetpack app and expect that the migration Welcome screen is shown
Verify on the migration welcome screen:
- The app theme is in Dark mode (or whatever was manually set in WordPress)
- The language is Arabic (or whatever was manually set in WordPress)
- The layout direction is RTL mode if Arabic/ LTR if in a LTR language
Explore the different options to interrupt & resume the flow, eg:
- Tap back to end up on Login Screen, force close & restart the app
- Tap Need help? → Log out, force close & restart the app
- Complete the migration flow by tapping on the primary green button
  - On My Site tap on the card with the WordPress logo (to open "Please Delete WordPress.." screen)
- Eventually set the system theme to dark mode, then to light mode and navigate to another screen (there was an issue with this behavior and specific fixes were needed for this)
Verify the app theme, language and layout direction are persisted (same as on Welcome Screen)

🎬 Video recording preview

(click to expand)

app_language_transfer.mp4

Regression Notes

Potential unintended areas of impact
Migration of user language and theme from WordPress to Jetpack app.
What I did to test those areas of impact (or what existing automated tests I relied on)
Manual testing & unit tests from JetpackMigrationViewModelTest.
What automated tests I added (or what prevented me from doing so)
Added unit tests to JetpackMigrationViewModelTest for the updated language transfer logic.

PR submission checklist:

I have completed the Regression Notes.
I have considered adding accessibility improvements for my changes.
I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

This was already handled by the application callbacks and therefore not needed.

This fix was needed to address possible issues when the language and the app theme is set manually by the user in the WordPress app.

# Conflicts: # WordPress/src/main/java/org/wordpress/android/localcontentmigration/LocalMigrationResult.kt # WordPress/src/main/java/org/wordpress/android/ui/main/jetpack/migration/JetpackMigrationViewModel.kt

wpmobilebot · 2023-01-16T18:38:14Z

📲 You can test these changes on Jetpack by downloading jetpack-installable-build-pr17768-11e73fa.apk

	💡 Scan this QR code with your Android phone to download and install the APK directly on it.
	App	`Jetpack`
	Build Flavor	`Jalapeno`
	Build Type	`Debug`
	Commit	`11e73fa`

Note: This installable build uses the JalapenoDebug build flavor, and does not support Google Login.

wpmobilebot · 2023-01-16T18:39:03Z

📲 You can test these changes on WordPress by downloading wordpress-installable-build-pr17768-11e73fa.apk

	💡 Scan this QR code with your Android phone to download and install the APK directly on it.
	App	`WordPress`
	Build Flavor	`Jalapeno`
	Build Type	`Debug`
	Commit	`11e73fa`

Note: This installable build uses the JalapenoDebug build flavor, and does not support Google Login.

antonis · 2023-01-17T14:04:03Z

+ * @param locale The locale to be used in the [LocalContext] configuration override.
+ * @param onLocaleChange Callback to be invoked when the locale is overridden, useful to update other app components.
+ * @param content The Composable function to be rendered with the overridden locale.
+ */


Thank you for documenting this properly 🙇

antonis

Great work @ovitrif 👍
I've tested the implementation on a Pixel 5 (Android 13) and everything worked as described for me. The code changes also look good to me

🚀 As a solution to these issues I introduced the new LocaleAwareComposable utility that listens to language changes and synchronizes the local configuration inside its content composable when needed.

I also validated that this case is cover too 🎉

I'll restrain from merging in case @mkevins has any more feedback 🙇

mkevins · 2023-01-18T05:11:42Z

+    private fun emitLanguageRefreshIfNeeded(languageCode: String) {
+        if (languageCode.isNotEmpty()) {
+            val shouldEmitLanguageRefresh = !localeManagerWrapper.isSameLanguage(languageCode)
+            if (shouldEmitLanguageRefresh) {
+                _refreshAppLanguage.value = languageCode
+            }
+        }
+    }


Not suggesting a change here, but wanted to note that my first thought here was if we could instead use:

val refreshAppLanguage: LiveData<String> = _refreshAppLanguage.distinct()

but I guess this could result in one unnecessary refresh at the start compared to the approach used here, since this way compares to the locale set in the current context. 🤷‍♂️

In the future, it would be nice if this idempotency was intrinsic to the implementation, so we wouldn't need to worry about it at the call site, but that's out of scope for this PR 😅 .

In the future, it would be nice if this idempotency was intrinsic to the implementation, so we wouldn't need to worry about it at the call site

Top idea 👏 , totally agreeing with you on this one, that would add to the reusability of the code 🚀

I've added a card on our team's private GitHub project to further pursue this when there's time for technical improvements 🙇 . On the other hand, it's possible we won't need this at all if we add support for the Android 13 Per-app language preferences mechanism soon 🤔

Good point, this could all become moot in the future 😄 👍

mkevins · 2023-01-18T05:17:03Z

I tested all flows described on a Pixel 3a (physical device) and it is working as expected. Great work on this Ovi! Also, the code looks good! 👍

mkevins

As mentioned above, LGTM!

ovitrif · 2023-01-18T06:58:49Z

Thank you both @antonis & @mkevins for reviewing, testing & validating the changes 🙇

I'll proceed to merge this PR 🚀

ovitrif added 18 commits January 13, 2023 22:01

Emit user flags to migration state flow

10a270d

Add util to change language

d5b4d57

Apply user language from WP app on migration screens

2e651e4

Change refreshAppLanguage event type to Unit

9adba19

Rename isSameLanguage param to language

dad7407

Add Context.primaryLocale util

254b390

Apply language change in migration compose UI

a7d0e2b

Refactor to use LocaleAwareComposable utility

8410370

Remove redundant lang change tracking in migration

2e80e00

This was already handled by the application callbacks and therefore not needed.

Refactor: rename migration lang util and fns

c4edb96

Remove unused refreshLanguage fn to recreate

6e47137

Remove unused getLocaleFromLanguage

2ee9c9e

Do not inherit LocaleAwareActivity in migration UI

5b0c669

This fix was needed to address possible issues when the language and the app theme is set manually by the user in the WordPress app.

Emit language refresh for delete wp screen

96960ff

This fix was needed to address possible issues when the language and the app theme is set manually by the user in the WordPress app.

Fix RTL text arranged LTR in migration UI

1445500

Merge branch 'trunk' into issue/17685-migration-apply-user-language

25df97b

# Conflicts: # WordPress/src/main/java/org/wordpress/android/localcontentmigration/LocalMigrationResult.kt # WordPress/src/main/java/org/wordpress/android/ui/main/jetpack/migration/JetpackMigrationViewModel.kt

Fix migration orchestrator tests

18205b5

Add tests for language refresh during migration

55f58df

ovitrif added i18n Jetpack Focus labels Jan 16, 2023

ovitrif added this to the 21.6 milestone Jan 16, 2023

ovitrif requested review from antonis and mkevins January 16, 2023 18:13

ovitrif added 2 commits January 16, 2023 19:16

Revert formatting of JetpackMigrationScreen fn

9fc6aa7

Restore and add new test for theme changes

11e73fa

antonis reviewed Jan 17, 2023

View reviewed changes

antonis approved these changes Jan 17, 2023

View reviewed changes

mkevins reviewed Jan 18, 2023

View reviewed changes

mkevins approved these changes Jan 18, 2023

View reviewed changes

ovitrif merged commit 06bf582 into trunk Jan 18, 2023

ovitrif deleted the issue/17685-migration-apply-user-language branch January 18, 2023 06:58

ovitrif mentioned this pull request Jan 19, 2023

Trigger Migration Flow when launching the app from deep links #17742

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Apply user language selected in WordPress app to Jetpack app during migration - #17768

Apply user language selected in WordPress app to Jetpack app during migration#17768
ovitrif merged 20 commits into
trunkfrom
issue/17685-migration-apply-user-language

ovitrif commented Jan 16, 2023

Uh oh!

wpmobilebot commented Jan 16, 2023

Uh oh!

wpmobilebot commented Jan 16, 2023

Uh oh!

antonis Jan 17, 2023

Uh oh!

antonis left a comment

Uh oh!

mkevins Jan 18, 2023

Uh oh!

mkevins Jan 18, 2023

Uh oh!

ovitrif Jan 18, 2023 •

edited

Loading

Uh oh!

mkevins Jan 19, 2023

Uh oh!

mkevins commented Jan 18, 2023

Uh oh!

mkevins left a comment

Uh oh!

ovitrif commented Jan 18, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

ovitrif commented Jan 16, 2023

🎬 Video recording preview

Regression Notes

Uh oh!

wpmobilebot commented Jan 16, 2023

Uh oh!

wpmobilebot commented Jan 16, 2023

Uh oh!

antonis Jan 17, 2023

Choose a reason for hiding this comment

Uh oh!

antonis left a comment

Choose a reason for hiding this comment

Uh oh!

mkevins Jan 18, 2023

Choose a reason for hiding this comment

Uh oh!

mkevins Jan 18, 2023

Choose a reason for hiding this comment

Uh oh!

ovitrif Jan 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkevins Jan 19, 2023

Choose a reason for hiding this comment

Uh oh!

mkevins commented Jan 18, 2023

Uh oh!

mkevins left a comment

Choose a reason for hiding this comment

Uh oh!

ovitrif commented Jan 18, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ovitrif Jan 18, 2023 •

edited

Loading