Configure localization automation for Widgets extension #7691

mokagio · 2022-09-09T04:36:57Z

Description

The widgets extension needed only a few finishing touches to complete the localization automation. Namely:

Use the custom AppLocalizedString routine to ensure the runtime can read the localization from the correct bundle
Add a build phase step to ensure AppLocalizedString is used
Optional but good to have: Adopt the reverse-DNS key notation

I basically copied the WordPress iOS setup 1-to-1.

Testing instructions

Localization automation

I run bundle exec fastlane generate_strings_file_for_glotpress on top of this branch and verified en.lproj/Localizable.strings was updated accordingly.

index de7dc4c08c..924aa0cf1d 100644
--- a/WooCommerce/Resources/en.lproj/Localizable.strings
+++ b/WooCommerce/Resources/en.lproj/Localizable.strings
@@ -1209,8 +1209,7 @@ which should be translated separately and considered part of this sentence. */
 /* Shown while logging in with Apple and the app waits for the site creation process to complete. */
 "Continuing with Apple" = "Continuing with Apple";
 
-/* Conversion stat label on dashboard.
-   Conversion title label for the store info widget */
+/* Conversion stat label on dashboard. */
 "Conversion" = "Conversion";
 
 /* Copy address text button title — should be one word and as short as possible. */
@@ -2908,6 +2907,9 @@ This is the link to the website, and forms part of a longer sentence which it sh
 /* Message from the in-person payment card reader when payment could not be taken because multiple cards were detected */
 "Multiple Contactless Cards Detected" = "Multiple Contactless Cards Detected";
 
+/* Generic store name for the store info widget preview */
+"My Shop" = "My Shop";
+
 /* Title of the hub menu view in case there is no title for the store */
 "My Store" = "My Store";
 
@@ -2925,9 +2927,6 @@ This is the link to the website, and forms part of a longer sentence which it sh
 /* Error showed in Shipping Label Address Validation for the name field */
 "Name missing" = "Name missing";
 
-/* VoiceOver accessibility hint, informing the user the button can be used to navigate to the Settings screen. */
-"Navigates to Settings." = "Navigates to Settings.";
-
 /* Title of the banner in the Order Detail for suggesting to install WCShip extension. */
 "Need a shipping label?" = "Need a shipping label?";
 
@@ -3235,6 +3234,9 @@ This is the link to the website, and forms part of a longer sentence which it sh
 /* Navigates to Card Reader ordering screen */
 "Order card reader" = "Order card reader";
 
+/* Title for the webview used by merchants to place an order for a card reader, for use with In-Person Payments. */
+"Order Card Reader" = "Order Card Reader";
+
 /* Order notes section title */
 "Order Notes" = "Order Notes";
 
@@ -3256,7 +3258,6 @@ This is the link to the website, and forms part of a longer sentence which it sh
 "Ordered List" = "Ordered List";
 
 /* Orders stat label on dashboard - should be plural.
-   Orders title label for the store info widget
    The title of the Orders tab. */
 "Orders" = "Orders";
 
@@ -4048,8 +4049,7 @@ This is the link to the website, and forms part of a longer sentence which it sh
 /* Title for the return policy in Customs screen of Shipping Label flow */
 "Return to sender if package is unable to be delivered" = "Return to sender if package is unable to be delivered";
 
-/* Revenue stat label on dashboard.
-   Revenue title label for the store info widget */
+/* Revenue stat label on dashboard. */
 "Revenue" = "Revenue";
 
 /* Review moderation success notice message. It reads: Review marked as {new status} */
@@ -4248,9 +4248,6 @@ This is the link to the website, and forms part of a longer sentence which it sh
 /* Placeholder in Shipping Label form for the Carrier and Rates row. */
 "Select your shipping carrier and rates" = "Select your shipping carrier and rates";
 
-/* My Store > Settings > Selected Store information section. This is the heading listed above the information row that displays the store website and their username. */
-"Selected Store" = "Selected Store";
-
 /* Button title. Sends a email verification link (Magin link) for signing in. */
 "Send email verification link" = "Send email verification link";
 
@@ -4279,8 +4276,7 @@ This is the link to the website, and forms part of a longer sentence which it sh
    Header text on Add New Service Package screen in Shipping Label flow */
 "Set up the package you'll be using to ship your products. We'll save it for future orders." = "Set up the package you'll be using to ship your products. We'll save it for future orders.";
 
-/* Accessibility label for the Settings button.
-   Settings button in the hub menu
+/* Settings button in the hub menu
    Settings navigation title */
 "Settings" = "Settings";
 
@@ -4658,6 +4654,22 @@ This is the link to the website, and forms part of a longer sentence which it sh
 /* My Store > Settings > Store Settings section title */
 "Store Settings" = "Store Settings";
 
+/* Conversion title label for the store info widget
+   Orders title label for the store info widget */
+"storeWidgets.infoView.orders" = "Orders";
+
+/* Revenue title label for the store info widget */
+"storeWidgets.infoView.revenue" = "Revenue";
+
+/* Visitors title label for the store info widget */
+"storeWidgets.infoView.visitors" = "Visitors";
+
+/* Title label for the login button on the store info widget. */
+"storeWidgets.notLoggedInView.login" = "Log in";
+
+/* Title label when the widget does not have a logged-in store. */
+"storeWidgets.notLoggedInView.notLoggedIn" = "Log in to see today’s stats.";
+
 /* Accessibility label for strikethrough button on formatting toolbar. */
 "Strike Through" = "Strike Through";
 
@@ -5826,8 +5838,7 @@ This is the link to the website, and forms part of a longer sentence which it sh
    Visibility label in Product Settings */
 "Visibility" = "Visibility";
 
-/* Visitors stat label on dashboard - should be plural.
-   Visitors title label for the store info widget */
+/* Visitors stat label on dashboard - should be plural. */
 "Visitors" = "Visitors";
 
 /* Message shown on screen while waiting for Google to finish its signup process. */

Xcode build phase

Changed one AppLocalizedString usage to NSLocalizedString and verified Xcode failed the build with an informative error:

I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

This is not required but is still recommended to avoid translation context clashes.

It enforces them only in app extensions targets which currently means only StoreWidgets.

mokagio · 2022-09-09T04:40:45Z

fastlane/Fastfile

      ],
      exclude: [
        '*Vendor*',
+        '**/AppLocalizedString.swift',


We don't want to localize the localization routine as that would create issues.

This makes me wonder if we should update the format for the routines to:

routines: [ { name: 'AppLocalizedString', path: 'path/to/file.swift' } ],

And then handle excluding that path within the toolkit.

Gold plating...

I think that's a laudable idea but might not be flexible enough to all our codebases that are not as clean as WP or WC and might e.g. still want to adopt the linter and also have a custom routine already in place, but just not in a dedicated .swift file containing only that. etc.

So in the end it might end up coupling the routine name and its assumption of it being isolated into a single file a bit too much. Besides, in some codebases or in the future, the file where the routine is declared might end up being already excluded because provided by a SPM plugin or a Pod that would already be excluded from parsing 🤷

Ultimately I'm not sure it's worth doing a breaking change (due to parameter type change) on the release-toolkit just for this 😅

mokagio · 2022-09-09T04:43:51Z

Scripts/build-phases/LintAppLocalizedStringsUsage.swift

+        let colNo = line.distance(from: line.startIndex, to: range.lowerBound)
+        let message = """
+Use `AppLocalizedString` instead of `NSLocalizedString` in source files that are used in the `\(targetName)` extension target. See paNNhX-nP-p2 for more info.
+"""


I wrote in the description that I basically copied 1-1 the WordPress setup. This is an instance in which I didn't.

In WordPress, this is a string defined with "...". I used this format here because SwiftLint (which runs as a build phase in Woo) picked this up as a line that was too long.

I originally had this as a proper multiline string

let message = """ Use `AppLocalizedString` instead... See paNNhx... """

But Xcode only printed the first line of the string in its error bubble...

AliSoftware

👌 Looking good from the GitHub diff.

Tbh I didn't take the time to test it myself but I trust that the tests you made and described in your PR description, highlighting that this still detected new strings and also that the linter caught issues, are enough to consider this to be working as expected 👍

AliSoftware · 2022-09-09T11:45:43Z

Ah, CI is failing… because the linter found an issue!

So I ended up checking out the branch and testing locally after all, and confirmed that I have the same error found by the linter locally:

(Probably because #7682 got recently merged while you opened this PR prior this change in trunk?)

AliSoftware · 2022-09-09T11:47:02Z

PS: Unrelated to this PR, but just wanted to note that while testing this PR I saw Xcode warnings about a dylib linked to the Widgets while not safe to be used in app extensions:

@mokagio you might want to take a look at this one next in separate PR.

Ecarrion · 2022-09-09T12:58:44Z

Thanks! I did not know that we had to use AppLocalizedString instead of NSLocalizedString is that something new? I don't think I have seen that in the main Woocommerce target 🤔

AliSoftware · 2022-09-09T17:32:33Z

Thanks! I did not know that we had to use AppLocalizedString instead of NSLocalizedString is that something new? I don't think I have seen that in the main Woocommerce target 🤔

Well this is exactly the new thing that this PR will introduce, so no wonder you haven't seen it yet in the main WooCommerce target 😄

Basically this will now be necessary now that WooCommerce has an app extension target (the new Widget), to avoid duplicating strings in multiple products (.app and .appex bundles) and also mutualize/reuse the strings used in both when it makes sense.

Internal note: you can read more details about this in paNNhX-nP-p2#source-of-truth-for-the-strings, which was posted back when the same was done for WordPress iOS, but will now soon be true for Woo too once Gio finishes this setup for WCiOS (though don't worry, knowing Gio he already planned to P2 all about it once this lands anyway 🙂 ).

mokagio · 2022-09-13T03:59:39Z

Ah, CI is failing… because the linter found an issue!

How cool is that! 😄

I'm not sure how I missed it while working on this locally, though. I obviously tested it locally before opening the PR, as can be seen by the screenshot in the description 🤔

Probably because #7682 got recently merged while you opened this PR prior this change in trunk?

That might be it and I didn't run the tests locally before force pushing my rebase on a trunk version that included it.

Oh well, that's why we have CI...

…gets

peril-woocommerce · 2022-09-13T04:01:25Z

	Warnings
⚠️	PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS

WooCommerce/StoreWidgets/StoreInfoProvider.swift

wpmobilebot · 2022-09-13T04:33:47Z

You can test the changes from this Pull Request by:

Clicking here or scanning the QR code below to access App Center
Then installing the build number pr7691-4874e1e on your iPhone

If you need access to App Center, please ask a maintainer to add you.

mokagio added 4 commits September 9, 2022 14:34

Use reverse-DNS key in store widgets localize strings

e047f24

This is not required but is still recommended to avoid translation context clashes.

Add AppLocalizedString custom routine for localization automation

6b0a9b2

Use AppLocalizedString in widget and update automation accordingly

2cfb6bb

Add build phase run script to enforce AppLocalizedString usages

aa1416d

It enforces them only in app extensions targets which currently means only StoreWidgets.

mokagio force-pushed the mokagio/localize-widgets branch from d156101 to aa1416d Compare September 9, 2022 04:37

mokagio commented Sep 9, 2022

View reviewed changes

mokagio requested review from AliSoftware, Ecarrion and crazytonyli September 9, 2022 04:44

mokagio self-assigned this Sep 9, 2022

mokagio added category: tooling Anything that involves building & maintaining the project, including scripts, `Fastfile`, etc. category: i18n Related to localization/internationalization. labels Sep 9, 2022

mokagio added this to the 10.4 milestone Sep 9, 2022

AliSoftware approved these changes Sep 9, 2022

View reviewed changes

Use AppLocalizedString and reverse-DNS keys in StoreInfoProvider

1d83ae5

Merge remote-tracking branch 'origin/trunk' into mokagio/localize-wid…

1170399

…gets

mokagio commented Sep 13, 2022

View reviewed changes

WooCommerce/StoreWidgets/StoreInfoProvider.swift Outdated Show resolved Hide resolved

Address a couple of AppLocalizedStrings violations

4874e1e

mokagio force-pushed the mokagio/localize-widgets branch from c1bbf82 to 4874e1e Compare September 13, 2022 04:18

mokagio merged commit 7942619 into trunk Sep 13, 2022

mokagio deleted the mokagio/localize-widgets branch September 13, 2022 05:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Configure localization automation for Widgets extension #7691

Configure localization automation for Widgets extension #7691

Uh oh!

mokagio commented Sep 9, 2022 •

edited

Loading

Uh oh!

mokagio Sep 9, 2022

Uh oh!

AliSoftware Sep 9, 2022

Uh oh!

mokagio Sep 9, 2022

Uh oh!

AliSoftware left a comment

Uh oh!

AliSoftware commented Sep 9, 2022 •

edited

Loading

Uh oh!

AliSoftware commented Sep 9, 2022

Uh oh!

Ecarrion commented Sep 9, 2022

Uh oh!

AliSoftware commented Sep 9, 2022

Uh oh!

mokagio commented Sep 13, 2022

Uh oh!

peril-woocommerce bot commented Sep 13, 2022

Uh oh!

Uh oh!

wpmobilebot commented Sep 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Configure localization automation for Widgets extension #7691

Configure localization automation for Widgets extension #7691

Uh oh!

Conversation

mokagio commented Sep 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing instructions

Localization automation

Xcode build phase

Uh oh!

mokagio Sep 9, 2022

Choose a reason for hiding this comment

Uh oh!

AliSoftware Sep 9, 2022

Choose a reason for hiding this comment

Uh oh!

mokagio Sep 9, 2022

Choose a reason for hiding this comment

Uh oh!

AliSoftware left a comment

Choose a reason for hiding this comment

Uh oh!

AliSoftware commented Sep 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AliSoftware commented Sep 9, 2022

Uh oh!

Ecarrion commented Sep 9, 2022

Uh oh!

AliSoftware commented Sep 9, 2022

Uh oh!

mokagio commented Sep 13, 2022

Uh oh!

peril-woocommerce bot commented Sep 13, 2022

Uh oh!

Uh oh!

wpmobilebot commented Sep 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mokagio commented Sep 9, 2022 •

edited

Loading

AliSoftware commented Sep 9, 2022 •

edited

Loading