Migrate iOS14 Stats Widgets to use app L10n

[AliSoftware]

Part of project ref paaHJt-2Ib-p2 & paaHJt-2J8-p2

What's in this PR

This PR migrates the WordPressStatsWidget (our iOS14-style Widget) to use the app bundle's localized strings.

This is in an effort to:

• Ensure the Widget's strings will now be translated (and their translations be updated when the original copy changes or if we add/remove new strings in code) as part of our regular pipeline sending the originals to GlotPress to translators
• Avoid duplicating the .strings files in both the App and the Widget, allowing us to share

How to test:

Prepare your testing environment – Xcode, Simulator, Scheme

• Open Xcode, and be sure to select the "WordPressStatsWidgets" scheme
• Choose "Product" > "Clean Build Folder"
  • This is necessary because it has been observed that sometimes resources (like .strings file), not being compiled files, are not properly deleted from the cached / previously-built Build Product directory end would end up still being present in the .app installed in the simulator during your test if they were present in a previous build
• Open your iOS Simulator (the one you intend to test this in – be sure to choose a Simulator that is IOS14 or greater as this is the minimum deployment target for this Widget), or your device if you plan to test this on a real device
• Be sure to uninstall the WordPress app in that Simulator or Device
  • This is necessary for the same reason as the Clean Build Folder above, otherwise there's a risk that the previously-installed WordPress app and its widget would not fully delete the old .strings files it had from the previous installation
• Go to the Settings app in your Simulator or Device, and choose a Language other than English for your whole device
  • Note that I've found that changing the "App Language" and "App Region" in "Edit Scheme" > "Run" > "Options" in Xcode doesn't seem to work / to affect the Widget itself when debugging Widgets, so this was the only way I found to debug widgets in other locales 🤷
• Go back to Xcode, edit the "WordPressStatsWidgets" scheme, and under "Run" > "Arguments" > "Environment Variables", ensure you check one (and only one) of the entries for _XCWidgetKind
  • Note: if you don't select any, you'll get a cryptic "SendProcessControlEvent:toPid: encountered an error: Error Domain=com.apple.dt.deviceproce…" error popup in Xcode – though clicking the "Details" button for once gives a very clear explanation of what to do, so at least there's that.
• Finally, hit ⌘-R to run the "WordPressStatsWidgets" scheme.
  • If you're testing in a Simulator, ensure that when it starts running, it did launch the simulator in which you did all the preliminary setup steps and not a different instance / model 😉

Finally check the UI is localized as expected

At that point, you should finally be able to see the Widget and app in your Simulator or Device and be ready to check the behavior:

• Ensure the widget is visible and installed on one of your Springboard page. If not, do a long-press to edit your Springboard and install it
• Check the error message that is shown on the widget.
  • It should correspond to whatever value the widget.today.unconfigured.view.title key at the end of the WordPress/Resources/*.lproj/Localizable.strings corresponding to the locale you're testing this in.
  • If the locale you're testing this in does not have a translation defined for this key (which is also a great use case to test for!), it should instead fallback to the English copy (as provided by the value: parameter in WordPress/WordPressStatsWidgets/Views/Localization/LocalizableStrings.swift for the corresponding static let unconfiguredView*Title constant)
• Open the WordPress app, then log in to your account
• Be sure to then also go into the "Stats" screen in the WordPress app at least once
  • This is required because of this bug
• Go back to your Springboard (⌘-⇧-H if you're testing on a Simulator)
• Check the widget again. It should have now loaded its data and displaying some stats.
• Check that those stats and labels matches the translations expected, as per the values found at the end of the WordPress/Resources/*.lproj/Localizable.strings of the tested locale

Ideally, repeat for a few locales, and also test for both a simulator and on-device

Finally, also test the Fastfile changes

• Run bundle exec fastlane generate_strings_file_for_glotpress
• Check that it created an Update strings for localization commit (it won't push it, don't worry!) which updated the WordPress/Resources/en.lproj/Localizable.strings file
• Check that it ended up only adding new keys for features that has been introduced in develop by recent PRs since the last code freeze (like "Join From Anywhere" or "Work With Us"), but did not delete or change any of the existing strings coming from the WordPressStatsWidgets's codebase – aka that all the keys starting with widget.* are kept intact, showing that genstrings properly found them while parsing the codebase.

---

Regression Notes

1. Potential unintended areas of impact

Known issue: as long as our global tooling (in release-toolkit) still relies on ./Scripts/fix-translations – which replaces any missing key in the .strings files downloaded from GlotPress with the "key" = "key";… which is not the right choice for the strings used in the Widget since all of them have a key that is not their English copy – we risk to have any key that would end up not being translated during the next sprint for 18.8, to display as their reverse-DNS-name key rather than falling back to the English copy as intended.

For now I'm assuming/hoping that our translators will translate most if not all of those new keys in GlotPress during the next cycle which will make this a non-issue. Besides, this potential issue won't happen until the next "upload originals to GlotPress on code-freeze then download translation and run fix-translations on it during final release" full cycle (for 18.8), so that still gives us time to fix it.

That being said, as part of one of the next steps of paaHJt-2Ib-p2, I will get soon rid of that bogus fix-translations script anyway – to entirely rely on iOS doing the right thing for us without the need for that post-processing – so I'm quite confident that even if our GlotPress translators didn't translate all those new Widget strings in all the locales, that particular hole in the tooling will be fixed by then.

2. What I did to test those areas of impact (or what existing automated tests I relied on)

Tested manually in the Simulator with different locales

3. What automated tests I added (or what prevented me from doing so)

None (I didn't see any possible way to write a good Unit Test for that except one that would just end up being a test of Apple's Bundle.localizedString(…) built-in method itself)

PR submission checklist:

• I have completed the Regression Notes.
• I have considered adding unit tests for my changes.
• 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.

[peril-wordpress-mobile]

	Warnings
⚠️	Localizable.strings should only be updated on release branches because it is generated automatically.
⚠️	This PR is assigned to a milestone which is closing in less than 4 days Please, make sure to get it merged by then or assign it to a later expiring milestone

Generated by 🚫 dangerJS

[peril-wordpress-mobile]

You can trigger an installable build for these changes by visiting CircleCI here.

[peril-wordpress-mobile]

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

[AliSoftware]

Note about this PR updating the .strings file and the timeline of merging this PR

New originals outside of a freeze

⚠️ @mokagio While we can already review this PR right now, we should still wait for the finalization of 18.7 to be done before merging it (and then we could either merge it in develop before the 18.8 code-freeze, or in the release/18.8 post-code-freeze)

Indeed, we should not update the .strings files in develop (the branch that that this PR will land into) – especially the en.lproj/Localized.strings file one which is imported by GlotPress – outside of a code-freeze, because that means that GlotPress will import and receive those new strings very close to the release date for 18.7, and the translators won't have time to translate those entries by the time you do the finalize_release, which will lead to the regression I'm talking about in the PR description.

In contrast, by merging this only after finalization of 18.7, we will still have time for the new keys to be imported in GlotPress and then re-translated by our translators in time for the 18.8 to have them all

Keeping the translations we already have…?

On this PR I've mainly copy/pasted the translations we already had in WordPress/WordPressStatsWidgets/Supporting Files/*.lproj to keep them around in the app bundle's .strings after the migration.

But, as soon as we will re-run our tooling, and do a round trip to download new translations from GlotPress, those translations I manually copied in the .strings files will be overwritten by the ones provided in GlotPress by our translators.
I still figured I'd copy the existing translations in the non-en .strings files, so that we are in a state were we have translations in the meantime (in-between 2 cycles); but expect to get a diff about those during the next cycle.

Speaking of this, when I copy/pasted those strings into the non-en *.lproj of the app, I pasted them all at the end of the .strings files, and added a // MARK: for them (truth be told, I wrote a script to automate that for all locales 🤫 😅). But when those will get re-written during next cycle when we'll download the latest translations, those keys will likely also move around somewhere else in the .strings file (I suspect that GlotPress sort the keys alphabetically when generating their exports?). Something else to be prepared for and not be surprised by during next cycle.

Finding a way to import the translations we already have for those keys in GlotPress

I asked our +i18n team in Slack (ref: p1637867994111900-slack-C02AED43D) if there was a way for us to import the already-translated strings for those new keys when we will get those new keys imported after this PR gets merged.
The goal being to avoid the translators having to re-translate something that has already been translated (out of band) previously –and also to avoid risking the wording/translation to change before/after this change if translators opt for a slightly different translation when they re-translate those keys compared to the translations we already have for them today.

Let's see what the team replies. If it's not possible, you should also be prepared during next code-freeze and cycle to potentially see slight changes in some translations for those new keys then compared to the ones we have today if translators opt for slight variations of the same meaning…

[AliSoftware]

I'm open for feedback about this, curious what you think about the idea.

During the migration I found this useful for me, in order to keep track of what was supposed to be localized (and have their value provided via a call to AppLocalizedString) vs regular, non-localized String variables/parameters.

In practice there's no compile-time guarantee, so this is just a signal for the reader of the code/API.
I tried to make it type-checked, by using a @propertyWrapper struct LocalizedString instead… but realized that doesn't work because the annotation needs to then be at the property declaration, aka on the @LocalizedString(…) static var todayWidgetTitle constants declared in LocalizableStrings.swift, which would force us to change them from static let to static var 😒 , also force us to provide the wrappedValue as part of the annotation (and not via = "key as string literal) because otherwise genstrings would not catch it… and speaking of this not even sure genstrings's regex would catch a custom routine like @LocalizedString with that @ prefix… and on top of all that, we'd still have our custom methods' argument (like here) take a String not a LocalizedString… so I abandoned the whole idea and settled for this semantic-only typealias instead.

[mokagio]

In practice there's no compile-time guarantee, so this is just a signal for the reader of the code/API.

This is my biggest frustration with typealias. I wish there was a way to get the compiler's help 😕 Improved readability is still a useful feature, though, so 👍 from me.

[AliSoftware]

Yeah I really tried hard multiple solutions to get compile-time guarantee and was quite frustrated to not be able to achieve it too 😅 Like I even considered creating a struct LocString: StringProtocol at some point… but as per the official doc of StringProtocol, it's highly discouraged (borderline "forbidden") to make custom types conform to StringProtocol – the doc clearly states that you should not do it and that only Swift-provided types like String and SubString should ever conform – so that was a no-no as well…

I think there have been discussions in Swift-Evolution to provide magic keywords or macro-driven solution to generate new types based on / transparently wrapping existing types (e.g. newtype Duration = Double or newtype LocString = String to solve this, but this still haven't seen the light of day in practice… So we'll have to accept our frustration for now and only rely on typealias, better than nothing I guess 😅

[AliSoftware]

I pondered putting this in its own Bundle+app.swift dedicated file, to respect the "one class/extension per file" convention… but figure it was not much worth it.

Especially since, in the near future (in the soon-to-come PRs which will address other Widgets and App Extensions targets as I continue progressing on that project of mine), I plan to add that helper file to other targets (i.e. make this file be a member of not only the WordPressStatsWidgets but also all other app extensions needing app localization like the pre-iOS14 widgets or the ShareExtension) – as opposed to duplicate/recreate/re-type this code and helpers in each target.

Which means that having both the Bundle.app extension and the AppLocalizedString helper function in the same file and having only one file to add new targets as its target memberships will make things easier.

PS: Obviously, when I get to that (in the future PRs) I'll start thinking on a better subdirectory to put that shared file in once I start sharing it with multiple targets

[AliSoftware]

Figured there was no chance to have localized strings in the Secrets.swift file, and I'm actually not sure why it was excluded in the first place (back when it was in localize.py which is why I initially put it there in the previous PR), so figured I'd use the occasion to remove it (I checked and the lane still works without error nor any new key it shouldn't have)

[mokagio]

🤔 the only reason I can think of for ignoring it is to avoid genstrings accidentally printing out a secret that happened to match the localization regex but wasn't meant to. Maybe? I'm not even sure if that's something that could happen on the genstrings end, though. 🤷

I agree that it should be 99.9% safe to remove it. 👍

[mokagio]

Went through the code and looks good 👍

I love the "DI for .strings" approach. I resonates with my microapps dream world.

Leaving this here before starting manual testing.

[mokagio]

In practice there's no compile-time guarantee, so this is just a signal for the reader of the code/API.

This is my biggest frustration with typealias. I wish there was a way to get the compiler's help 😕 Improved readability is still a useful feature, though, so 👍 from me.

[mokagio]

🤔 the only reason I can think of for ignoring it is to avoid genstrings accidentally printing out a secret that happened to match the localization regex but wasn't meant to. Maybe? I'm not even sure if that's something that could happen on the genstrings end, though. 🤷

I agree that it should be 99.9% safe to remove it. 👍

[mokagio]

Leaving this here before starting manual testing.

[!] CDN: trunk URL couldn't be downloaded: https://cdn.jsdelivr.net/cocoa/Specs/8/d/8/WordPressShared/1.17.0-beta.1/WordPressShared.podspec.json Response: Timeout was reached

🙃

[mokagio]

⚠️ @mokagio While we can already review this PR right now, we should still wait for the finalization of 18.7 to be done before merging it (and then we could either merge it in develop before the 18.8 code-freeze, or in the release/18.8 post-code-freeze)

18.7 has been finalized: #17573. Just waiting for review and merge.

you [@mokagio] should also be prepared during next code-freeze and cycle to potentially see slight changes in some translations for those new keys then compared to the ones we have today if translators opt for slight variations of the same meaning…

Thank you for the heads up, I'll try to put extra care in looking at the .strings diff.

[mokagio]

Behaved as described. Thank you for the detailed test steps.

Note that I've found that changing the "App Language" and "App Region" in "Edit Scheme" > "Run" > "Options" in Xcode doesn't seem to work / to affect the Widget itself when debugging Widgets, so this was the only way I found to debug widgets in other locales 🤷

How disappointing 😞

I switched an iPhone 13 iOS 15.0 Simulator to Italian, 🤌, and selected the today widget kind env var in the scheme. Before running the scheme, I erased all content and settings on that Simulator.

There's no widget.today.unconfigured.view.title key in the it file: https://github.com/wordpress-mobile/WordPress-iOS/pull/17570/files#diff-017232c167b6dbaa61eb2be975fe2a5b5607c8b9923e13fd48ef459f70eb34b7. It fallback to English as expected:

One thing I'm not sure about, though, is that, unless I missed them in my eye-based inspection of the diff, _none of the .strings include a widget.today.unconfigured.view.title key 🤔

It's possible that's legit and that key was yet to be added to the source files. I'm raising this point because you mentioned using a script, so just wanted to make sure this is not something the script missed instead.

Ideally, repeat for a few locales, and also test for both a simulator and on-device

I run through the same checks after changing the Simulator language to es. I also change the widget kind to "all time".

---

Auto-merge is disabled, so I'm going to approve this PR under the assumption that the missing widget.today.unconfigured.view.title key is not a bug.

To be fair, even if it is a bug and a minor regression in the localization coverage, and it turns out to be something we can't address quickly, I think we can live with for a short while. Af few missing strings are not worth slowing down the release train, IMHO.

---

As an aside, this work you've done also benefits us in that by testing the localization we can surface values that haven't been localized yet.

E.g. "This Week" and "Stay up to date...":

[mokagio]

[mokagio]

[mokagio]

[AliSoftware]

Thanks a lot @mokagio for taking the time to test this thoroughly and so late in your day ❤️

One thing I'm not sure about, though, is that, unless I missed them in my eye-based inspection of the diff, _none of the .strings include a widget.today.unconfigured.view.title key 🤔

Yes, this actually took me quite some time to investigate and to try to understand where those localizations came from back when we introduced the Widgets. It turns out that only the strings that were also already used in the Stats screen of the app were copied over in the Widgets, while new strings like this "unconfigured" one were specific to widgets hence why they were not (and still aren't… yet).

Spelunking Details

Those keys (the ones that were in the Widget's .strings file before I started this PR) are nowhere to be found in GlotPress (as expected, because those .strings files are not imported by the cron job, nor were merged into the app's .strings at code freeze), so I was puzzled as to how they got translated.

• At first I thought that back then @Gio2018 might have asked Team Global (+i18n) to do a one-off translation of those keys (using what the i18n team calls "Deliverables", aka one-time, on-demand translation requests).
• Then I tried to track the commit history for the non-en .strings files to try to understand where they came from, and first git log WordPress/WordPressStatsWidgets/Supporting\ Files/fr.lproj/Localizable.strings pointed me to ccbcdd7 which was part of Adds some missing translations for the iOS 14 All Time widget #16135 but I was still puzzled as to where he got the translations from (surely he didn't translate those himself for all the locales, and they were still not in GlotPress)
• Then I finally found out that those files were initially in a different path (the parent dir was renamed at some point), and git log --follow WordPress/WordPressStatsWidgets/Supporting\ Files/fr.lproj/Localizable.strings (which follows renames) finally led me to 0e17208 from Today widget localization #15614, which explains everything in the PR description (thanks @Gio2018 for explaining it in that PR, that allowed me to find it back and finally make sense of it all)

---

So, TL;DR: what happened is that back then, Gio R copied any strings / copy that were already existing in the app, to copy them over into the widget's .strings file. Then, in a subsequent commit, he later changed the key names to use reverse-DNS naming (I remember discussing that part with him and suggesting this to him myself, especially as a way to avoid potential conflicting keys used for different contexts, app vs widget)

• which is why searching for those keys in the app's .strings didn't lead to any result
• but also why some strings from the widgets still had localization (because "Visitors" or "This Week" were already strings used in the App too, especially in the "Stats" screen of the app)
• and, finally to answer your question… why there were also some of the widgets' strings that were not translated, and why the ones that aren't… are especially the ones that are specific to the widgets (and were thus brand new strings, like the ones about the widget not being configured yet), and not strings matching a similar one already used in the app's "Stats" screen

(Phew, what a digging journey! But spelunking hat finally off! 😸 )