Send all translations to gutenberg-mobile #9249

Tug · 2019-02-14T16:19:39Z

Requires wordpress-mobile/gutenberg-mobile#536

This PR adds a new property our react app will receive which will contain all the translations available for WPAndroid. The idea is to have translations in gutenberg-mobile available for the beta.

Testing Instructions

Checkout this branch locally (git fetch -a && git checkout add/i18n-cache) and make sure to update the submodules after that git submodule update --init --recursive
Remove this condition to test sending all the WPAndroid strings to gutenberg-mobile: https://github.com/wordpress-mobile/WordPress-Android/pull/9249/files#diff-02f567e707c1df878dfea548cfec3da7R151
In a terminal run cd libs/gutenberg-mobile && yarn install && yarn start:reset
Connect an android device or start a virtual device, then open another terminal, go to the WordPress-Android project root folder and run ./gradlew installWasabiDebug then adb shell am start -n "org.wordpress.android.beta/org.wordpress.android.ui.WPLaunchActivity" -a android.intent.action.MAIN -c android.intent.category.LAUNCHER (or boot the app with Android studio)
Make sure your app is in english
Open http://localhost:8081/debugger-ui/ on your computer and open the console in the devtools
Open a page or a post in the app
Look at the logs and check that the locale is en, and that translations is empty
Go to the user profile settings and switch to a different language (repeat the operation several times and test for languages with a regional code such as zh-cn or pt-br)
Open a page or a post
Look at the logs and check that the locale is correct (should be one of those https://github.com/wordpress-mobile/gutenberg-mobile/pull/536/files#diff-db716afdbf5160927b4465dba3e62535R8). For the locales for which we have a translation file in libs/gutenberg-mobile/i18n-cache/data/ you should see the translated strings.

wpmobilebot · 2019-02-14T16:25:33Z

	2 Warnings
⚠️	PR is missing at least one label.
⚠️	PR is not assigned to a milestone.

Generated by 🚫 Danger

…erg-translations

peril-wordpress-mobile · 2019-02-19T10:53:14Z

Danger run resulted in 2 warnings; to find out more, see the checks page.

Generated by 🚫 dangerJS

… with region

marecar3 · 2019-02-19T22:40:52Z

...itor/WordPressEditor/src/main/java/org/wordpress/android/editor/GutenbergEditorFragment.java

        return mRetainedGutenbergContainerFragment;
    }

+    public Bundle getTranslations() {


This is a local method so we should change method access signature probably to private.

This method name doesn't tell us enough information about what is the purpose of this method.
getTranslations() is too abstract I think ?

marecar3 · 2019-02-19T22:42:02Z

...itor/WordPressEditor/src/main/java/org/wordpress/android/editor/GutenbergEditorFragment.java

            FragmentTransaction fragmentTransaction = fragmentManager.beginTransaction();
            GutenbergContainerFragment gutenbergContainerFragment =
-                    GutenbergContainerFragment.newInstance(isNewPost);
+                    GutenbergContainerFragment.newInstance(isNewPost, localeSlug, this.getTranslations());


There is no need for this in this.getTranslations()so we can remove it.

marecar3 · 2019-02-19T22:47:57Z

Hey @Tug @hypest , I must admit that I am not 100% sure what should method getTranslations()return to us.

When I see the log, there is a bunch of strings that are not label related, e.g. SVG Path, fonts names, and also class names android.support.design.widget.AppBarLayout$ScrollingViewBehavior which is odd?

Please take a look at the file.

getTranslations.txt

marecar3 · 2019-02-19T22:52:29Z

...itor/WordPressEditor/src/main/java/org/wordpress/android/editor/GutenbergEditorFragment.java


+    public Bundle getTranslations() {
+        Bundle translations = new Bundle();
+        Locale defaultLocale = new Locale("en");


This code will always return English translation, is that idea?

The english locale yes, the idea is for getTranslations to return a mapping of english string => [ translated string ] so we feed it to the @wordpress/i18n lib.

marecar3 · 2019-02-19T22:53:32Z

...itor/WordPressEditor/src/main/java/org/wordpress/android/editor/GutenbergEditorFragment.java

+            try {
+                resourceId = stringField.getInt(R.string.class);
+            } catch (IllegalArgumentException | IllegalAccessException e) {
+                e.printStackTrace();


in Android it's always better to Log the error and not print stack trace so that we can easily find and see what is the error about.

Tug · 2019-02-20T14:17:25Z

When I see the log, there is a bunch of strings that are not label related, e.g. SVG Path, fonts names, and also class names android.support.design.widget.AppBarLayout$ScrollingViewBehavior which is odd?

It's not that odd given that we extract all the strings from R.string. It might not be the right approach so I'm open to alternatives.
I just added a filter to check for the prefix gutenberg_mobile_ before adding it to translations. Should limit the size of the object as well

marecar3 · 2019-02-20T17:56:59Z

Hey @Tug, I have managed to get the strings from the main application. This is the example approach:

String packageName = getActivity().getApplication().getPackageName();
int gutenbergMobileTestStringId = getResources().getIdentifier("gutenberg_mobile_gutenberg_packages_block_library_src_paragraph_edit_native_js_115", "string", packageName);

String gutenbergMobileTestString = getResources().getString(gutenbergMobileTestStringId);

Probably you will notice that you will need to know the exact name of each string, so maybe you can define those names in WordPressEditor/srd/main/res/values/ids.xml. (it's not the best approach as we would need to maintain string names on two places...)

Let me know if you need any further help regarding this one.

… loader

marecar3 · 2019-02-20T22:20:08Z

...itor/WordPressEditor/src/main/java/org/wordpress/android/editor/GutenbergEditorFragment.java

+            return translations;
+        }
+
+        for (Field stringField : R_String.getDeclaredFields()) {


Hmm interesting, I don't like hacky solutions generally 🙂
as it can just to stop working at some API levels.
Did you test it with different Android API levels?

I agree it's hacky, especially the part where it assumes that the R class is in the same package as the main Application class.
I'm not sure what you mean by different Android API levels though so I guess I didn't test that.
In theory I think we would need to go get the strings as an argument from the main app instead of trying to access a package we should not have to be aware of here.
It's starting to become a lot of changes for this "temporary" i18n solution though...

Regarding Android API I am thinking to try this code with some devices that have Lollipop, Marshmallow... Nougat,,, and especially EMUI (Huawei) installed.

…erg-translations

hypest · 2019-02-21T14:13:05Z

...itor/WordPressEditor/src/main/java/org/wordpress/android/editor/GutenbergEditorFragment.java

+
+        DisplayMetrics metrics = new DisplayMetrics();
+        getActivity().getWindowManager().getDefaultDisplay().getMetrics(metrics);
+        configuration.locale = defaultLocale;


I'm confused by this line... should we assign the desired locale here? Like, the same as the one passed down the the RN app? Instead, this line seems to again assign en 🤔

Nop this is required to get the default (english) resources. We'll need both the current resource which is whatever the app is set to at the moment and the english ones to generate our english => [ translated ] map

Aha, right. Yeah, I guess it would be useful if we add some line comments to describe what's going on in the various code paragraphs of this method.

hypest · 2019-02-21T14:47:39Z

WordPress/src/main/java/org/wordpress/android/ui/posts/EditPostActivity.java

                        // Show the GB informative dialog on editing GB posts
                        showGutenbergInformativeDialog();
-                        return GutenbergEditorFragment.newInstance("", "", mIsNewPost);
+                        String languageString = LocaleManager.getLanguage(EditPostActivity.this);


OK, let's see, is there some specific reason we're using LocaleManager.getLanguage() here (which gets the device language) but we use Configuration.locale later in getTranslations() (which get's the current app locale)?

Unfortunately, the device and app locales can be out of sync unless the app is restarted so, this ends up having, for example, English UI (see top bar) but Spanish content strings (see the title placeholder):

Note: Locale.getLanguage(context) will only return the device language if no custom app-level language is set in SharedPreferences.

WordPress-Android/WordPress/src/main/java/org/wordpress/android/util/LocaleManager.java

Line 82 in 699d215

return prefs.getString(LANGUAGE_KEY, LanguageUtils.getCurrentDeviceLanguageCode());

hypest · 2019-02-21T15:14:42Z

...itor/WordPressEditor/src/main/java/org/wordpress/android/editor/GutenbergEditorFragment.java

+        Bundle translations = new Bundle();
+        Locale defaultLocale = new Locale("en");
+        Resources currentResources = getResources();
+        Configuration configuration = currentResources.getConfiguration();


Nit, but not it makes debugging this method easier: Let's rename this variable to currentConfiguration instead of just configuration and then at line 128 let's create a new one instead of reassigning the old one. Thanks!

…the activity context

hypest · 2019-02-21T18:09:22Z

Tested the PR at this stage by manually adding some gutenberg-mobile strings in the Spanish strings.xml file and verified that they get loaded in the RN app.

I think this PR is good to go.

hypest

LGTM!

hypest · 2019-02-21T18:10:35Z

@Tug , can you make a note to address any remaining review feedback in a follow a PR? Thanks!

hypest · 2019-02-21T18:11:31Z

Let's wait for the gutenberg-mobile PR to merge and update its hash here, then we can merge.

Tug added 2 commits February 12, 2019 12:16

Give the translations to gutenberg as a param

26806d3

Fix context not attached to fragment

882ef91

Tug added the [Status] Not Ready for Review label Feb 14, 2019

Tug self-assigned this Feb 14, 2019

Tug mentioned this pull request Feb 14, 2019

Use gutenberg translation files locally wordpress-mobile/gutenberg-mobile#536

Merged

Tug added 3 commits February 14, 2019 18:35

Pass down the locale to gutenberg-mobile

154d11d

Merge remote-tracking branch 'origin/develop' into update/send-gutenb…

af7cb42

…erg-translations

Merge remote-tracking branch 'origin/develop' into update/send-gutenb…

ba2e6f0

…erg-translations

Tug added 4 commits February 19, 2019 14:23

Update libs/gutenberg-mobile ref

c575a52

Update Aztec ref and wrap translation values in an array

4d5096c

Get the locale slug from the app settings instead and support locales…

4a94466

… with region

Update gutenberg-mobile ref

c1bb23a

Tug requested a review from hypest February 19, 2019 19:42

marecar3 reviewed Feb 19, 2019

View reviewed changes

Tug added 2 commits February 20, 2019 15:12

Filter out non-gutenberg mobile related strings

013186d

Log error instead of printing stacktrace

644579d

Update gutenberg-mobile ref after merge from develop

ac433d6

Load resource class from the main package using the Application class…

444bc1e

… loader

marecar3 approved these changes Feb 20, 2019

View reviewed changes

Tug added 4 commits February 21, 2019 09:23

Make sure Package is not null

90c591e

Update gutenberg-mobile ref

dd1f1da

Fix spacing lint issue

fdf04b6

Fix more lint issues

3afa68e

Tug added 3 commits February 21, 2019 11:36

Fix more lint issues

41ddfaa

Fix more lint issues

90fcc93

Fix more lint issues

5508ee5

Tug removed the [Status] Not Ready for Review label Feb 21, 2019

Tug added 3 commits February 21, 2019 12:25

Update gutenberg-mobile ref

4f4c1d1

Merge remote-tracking branch 'origin/develop' into update/send-gutenb…

63498b1

…erg-translations

Update gutenberg-mobile ref

fd308ea

hypest reviewed Feb 21, 2019

View reviewed changes

Add comments to getTranslations

2407b09

hypest reviewed Feb 21, 2019

View reviewed changes

Tug added 2 commits February 21, 2019 16:38

Update gutenberg-mobile ref

2b10286

Make sure we get the resources from the application context not from …

dc5bbce

…the activity context

hypest approved these changes Feb 21, 2019

View reviewed changes

hypest added 3 commits February 21, 2019 23:19

Merge branch 'develop' into update/send-gutenberg-translations

d53a9f4

Update gb-mobile hash to the merged commit

067e721

Trim comments just to make the method 200-lines long

15a9701

hypest merged commit 73f0497 into develop Feb 21, 2019

hypest deleted the update/send-gutenberg-translations branch February 21, 2019 21:58

Send all translations to gutenberg-mobile #9249

Send all translations to gutenberg-mobile #9249

Uh oh!

Conversation

Tug commented Feb 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Testing Instructions

Uh oh!

wpmobilebot commented Feb 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

peril-wordpress-mobile bot commented Feb 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marecar3 commented Feb 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tug commented Feb 20, 2019

Uh oh!

marecar3 commented Feb 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tug Feb 21, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hypest commented Feb 21, 2019

Uh oh!

hypest left a comment

Choose a reason for hiding this comment

Uh oh!

hypest commented Feb 21, 2019

Uh oh!

hypest commented Feb 21, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Tug commented Feb 14, 2019 •

edited

Loading

wpmobilebot commented Feb 14, 2019 •

edited

Loading

peril-wordpress-mobile bot commented Feb 19, 2019 •

edited

Loading

marecar3 commented Feb 19, 2019 •

edited

Loading

marecar3 commented Feb 20, 2019 •

edited

Loading

Tug Feb 21, 2019 •

edited

Loading