-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[Tooling] Migrate iOS13 Widgets to use App strings #17630
Conversation
This file was previously in the directory associated with the `WordPressStatsWidgets` target. But now that I plan to use this helper for other App Extension / Widget targets, it seems better suited to move it under `WordPress/Utility`. For now this commit only moves it around, next commits will add that file to more targets and start using it elsewhere.
- WordPressTodayWidget - WordPressThisWeekWidget - WordPressAllTimeWidget - WordPress & Jetpack app targets (†) (†) as some files where I'll need to use `AppLocalizedString` instead of `NSLocalizedString` are used by app extensions… but also used with the app targets too (especially views related to Stats)
This file is used not only by the app, but is also build as part of the various Widget targets Note: searching each swift file that is built as part of by `WordPressStatsWidgets` (iOS14) for their use of `NSLocalizedString` revealed that this was the only one which we missed during transition of the WordPressStatsWidgets target previously. Other widget targets have more files which need the same transition, those will be done in a subsequent commit.
You can trigger an installable build for these changes by visiting CircleCI here. |
You can trigger optional UI/connected tests for these changes by visiting CircleCI here. |
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.
Posting the code-only review before starting the manual testing, which might take a while and end up being interrupted by dinner time 😅
So far, looks great. I'm not a fan of sharing files between targets, but I think this is the best solution in the current state of the codebase and likely to remain viable and effective for a while 👍
h/t @mokagio for spotting them Co-authored-by: Gio Lodi <[email protected]>
I'm not having any luck loading the widgets... I tried different Simulators and two different schemes (Today and ThisWeek) and iOS 13.x vs 15 versions. I just don't see them in the list or the Widgets springboard page. E.g.: Anything I might be missing? Suggestions? As I was writing this comment, macOS informed me SpringBoard crashed 🤦 |
Welcome to my world 😒 Last resort I tried was rebooting the Mac. Another alternative worth trying is… testing on device? |
Other tricks I tried: once the scheme for the widget got launched, try to go to the other pages of the Springboard to locate the WordPress app itself (which got installed alongside the widgets), launch it at least once, and then go back to Xcode and retry a ⌘R (without a clean beforehand this time!). Last alternative: test on device (or reboot Mac and sacrifice a goat before retrying) I'm re-adding @wordpress-mobile/owl-team as reviewers (instead of just you) in case others 🦉 have more luck testing in their own Xcode and Simulators… 🤞 (even those those issues matches my poor and very random experience with Xcode and debugging Widgets 😒 but let's hope one of you will be able to finally have Xcode on their side at least once…) |
Debugging TodayWidget while my simulator is in French, and this is indeed super strange:
So… I'm just hoping it's because of Xcode and the simulator not being great for that and that it would work as expected on a real device 🤞 |
Yeah I agree 👍 @mokagio Can I get an official ✅ on the PR so it can be merged? 🙇 |
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.
Explicit approval based on conversation above 👍
Got a bunch of conflicts because of deleted files on `develop` that this branch modified, most were localizations because of #17630 and #17636. There was also a conflict on the change in the timeout duration of a test, which I solved by keeping the longest timeout value. I ensured the app built and the tests passed before committing 👌 Full list of conflicting files: - `WordPress/Classes/ViewRelated/Me/App Settings/About/AboutHeaderView.swift` - `WordPress/Classes/ViewRelated/Me/App Settings/About/AutomatticAboutScreen.swift` - `WordPress/Classes/ViewRelated/Me/App Settings/About/AutomatticAppLogosCell.swift` - `WordPress/WordPressShareExtension/ar.lproj/Localizable.strings` - `WordPress/WordPressShareExtension/bg.lproj/Localizable.strings` - `WordPress/WordPressShareExtension/cs.lproj/Localizable.strings` - `WordPress/WordPressShareExtension/cy.lproj/Localizable.strings` - `WordPress/WordPressShareExtension/da.lproj/Localizable.strings` - `WordPress/WordPressShareExtension/de.lproj/Localizable.strings` - `WordPress/WordPressShareExtension/en-AU.lproj/Localizable.strings` - `WordPress/WordPressShareExtension/en-CA.lproj/Localizable.strings` - `WordPress/WordPressShareExtension/en-GB.lproj/Localizable.strings` - `WordPress/WordPressShareExtension/es.lproj/Localizable.strings` - `WordPress/WordPressShareExtension/fr.lproj/Localizable.strings` - `WordPress/WordPressShareExtension/he.lproj/Localizable.strings` - `WordPress/WordPressShareExtension/hr.lproj/Localizable.strings` - `WordPress/WordPressShareExtension/hu.lproj/Localizable.strings` - `WordPress/WordPressShareExtension/id.lproj/Localizable.strings` - `WordPress/WordPressShareExtension/is.lproj/Localizable.strings` - `WordPress/WordPressShareExtension/it.lproj/Localizable.strings` - `WordPress/WordPressShareExtension/ja.lproj/Localizable.strings` - `WordPress/WordPressShareExtension/ko.lproj/Localizable.strings` - `WordPress/WordPressShareExtension/nb.lproj/Localizable.strings` - `WordPress/WordPressShareExtension/nl.lproj/Localizable.strings` - `WordPress/WordPressShareExtension/pl.lproj/Localizable.strings` - `WordPress/WordPressShareExtension/pt-BR.lproj/Localizable.strings` - `WordPress/WordPressShareExtension/pt.lproj/Localizable.strings` - `WordPress/WordPressShareExtension/ro.lproj/Localizable.strings` - `WordPress/WordPressShareExtension/ru.lproj/Localizable.strings` - `WordPress/WordPressShareExtension/sk.lproj/Localizable.strings` - `WordPress/WordPressShareExtension/sq.lproj/Localizable.strings` - `WordPress/WordPressShareExtension/sv.lproj/Localizable.strings` - `WordPress/WordPressShareExtension/th.lproj/Localizable.strings` - `WordPress/WordPressShareExtension/tr.lproj/Localizable.strings` - `WordPress/WordPressShareExtension/zh-Hans.lproj/Localizable.strings` - `WordPress/WordPressShareExtension/zh-Hant.lproj/Localizable.strings` - `WordPress/WordPressTest/LikeUserHelperTests.swift` - `WordPress/WordPressTodayWidget/ar.lproj/Localizable.strings` - `WordPress/WordPressTodayWidget/bg.lproj/Localizable.strings` - `WordPress/WordPressTodayWidget/cs.lproj/Localizable.strings` - `WordPress/WordPressTodayWidget/cy.lproj/Localizable.strings` - `WordPress/WordPressTodayWidget/da.lproj/Localizable.strings` - `WordPress/WordPressTodayWidget/de.lproj/Localizable.strings` - `WordPress/WordPressTodayWidget/en-AU.lproj/Localizable.strings` - `WordPress/WordPressTodayWidget/en-CA.lproj/Localizable.strings` - `WordPress/WordPressTodayWidget/en-GB.lproj/Localizable.strings` - `WordPress/WordPressTodayWidget/es.lproj/Localizable.strings` - `WordPress/WordPressTodayWidget/fr.lproj/Localizable.strings` - `WordPress/WordPressTodayWidget/he.lproj/Localizable.strings` - `WordPress/WordPressTodayWidget/hr.lproj/Localizable.strings` - `WordPress/WordPressTodayWidget/hu.lproj/Localizable.strings` - `WordPress/WordPressTodayWidget/id.lproj/Localizable.strings` - `WordPress/WordPressTodayWidget/is.lproj/Localizable.strings` - `WordPress/WordPressTodayWidget/it.lproj/Localizable.strings` - `WordPress/WordPressTodayWidget/ja.lproj/Localizable.strings` - `WordPress/WordPressTodayWidget/ko.lproj/Localizable.strings` - `WordPress/WordPressTodayWidget/nb.lproj/Localizable.strings` - `WordPress/WordPressTodayWidget/nl.lproj/Localizable.strings` - `WordPress/WordPressTodayWidget/pl.lproj/Localizable.strings` - `WordPress/WordPressTodayWidget/pt-BR.lproj/Localizable.strings` - `WordPress/WordPressTodayWidget/pt.lproj/Localizable.strings` - `WordPress/WordPressTodayWidget/ro.lproj/Localizable.strings` - `WordPress/WordPressTodayWidget/ru.lproj/Localizable.strings` - `WordPress/WordPressTodayWidget/sk.lproj/Localizable.strings` - `WordPress/WordPressTodayWidget/sq.lproj/Localizable.strings` - `WordPress/WordPressTodayWidget/sv.lproj/Localizable.strings` - `WordPress/WordPressTodayWidget/th.lproj/Localizable.strings` - `WordPress/WordPressTodayWidget/tr.lproj/Localizable.strings` - `WordPress/WordPressTodayWidget/zh-Hans.lproj/Localizable.strings` - `WordPress/WordPressTodayWidget/zh-Hant.lproj/Localizable.strings`
…ng is still WIP We are currently working on removing `update-translations.rb` + `fix-translations` + `extract-framework-translations.swift` scripts from the repo and migrating their logic into the release-toolkit, but that work is still in progress. The role of the (misleadingly named) `extract-frameworks-translations.swift` script was to redistribute the keys from the `*.lproj/Localizable.strings` downloaded from GlotPress into the `.strings` files of the Widgets and ShareExtension targets, back when those targets used their own `.strings` file. This is no longer the case since #17630 and #17636 landed – as Widgets and ShareExtension's code now reference the app's `.strings` file directly – which means we don't need to run the logic from `extract-frameworks-translations.swift` anymore, and in fact if we did, it would probably crash now that the `Base.lproj/Localizable.strings` files in the Widgets and ShareExtension subfolders (which that script references) don't exist anymore.
Conflicts in all the `*.lproj/Localizable.strings` from the WordPressShareExtension and WordPressTodayWidget targets. The conflicts are a byproduct of the work done in #17630 and #17636. I haven't spent the time to research why Git saw those a modified on `trunk` because, regardless, the right thing for them is to be deleted now that the extensions fetch their strings from the main bundle. Also got a conflict on the `ar-SA` release notes. I resolved it by keeping the more recent version from 19.0. Full list of conflicting files: ``` WordPress/WordPressShareExtension/ar.lproj/Localizable.strings WordPress/WordPressShareExtension/bg.lproj/Localizable.strings WordPress/WordPressShareExtension/cs.lproj/Localizable.strings WordPress/WordPressShareExtension/cy.lproj/Localizable.strings WordPress/WordPressShareExtension/da.lproj/Localizable.strings WordPress/WordPressShareExtension/de.lproj/Localizable.strings WordPress/WordPressShareExtension/en-AU.lproj/Localizable.strings WordPress/WordPressShareExtension/en-CA.lproj/Localizable.strings WordPress/WordPressShareExtension/en-GB.lproj/Localizable.strings WordPress/WordPressShareExtension/es.lproj/Localizable.strings WordPress/WordPressShareExtension/fr.lproj/Localizable.strings WordPress/WordPressShareExtension/he.lproj/Localizable.strings WordPress/WordPressShareExtension/hr.lproj/Localizable.strings WordPress/WordPressShareExtension/hu.lproj/Localizable.strings WordPress/WordPressShareExtension/id.lproj/Localizable.strings WordPress/WordPressShareExtension/is.lproj/Localizable.strings WordPress/WordPressShareExtension/it.lproj/Localizable.strings WordPress/WordPressShareExtension/ja.lproj/Localizable.strings WordPress/WordPressShareExtension/ko.lproj/Localizable.strings WordPress/WordPressShareExtension/nb.lproj/Localizable.strings WordPress/WordPressShareExtension/nl.lproj/Localizable.strings WordPress/WordPressShareExtension/pl.lproj/Localizable.strings WordPress/WordPressShareExtension/pt-BR.lproj/Localizable.strings WordPress/WordPressShareExtension/pt.lproj/Localizable.strings WordPress/WordPressShareExtension/ro.lproj/Localizable.strings WordPress/WordPressShareExtension/ru.lproj/Localizable.strings WordPress/WordPressShareExtension/sk.lproj/Localizable.strings WordPress/WordPressShareExtension/sq.lproj/Localizable.strings WordPress/WordPressShareExtension/sv.lproj/Localizable.strings WordPress/WordPressShareExtension/th.lproj/Localizable.strings WordPress/WordPressShareExtension/tr.lproj/Localizable.strings WordPress/WordPressShareExtension/zh-Hans.lproj/Localizable.strings WordPress/WordPressShareExtension/zh-Hant.lproj/Localizable.strings WordPress/WordPressTodayWidget/ar.lproj/Localizable.strings WordPress/WordPressTodayWidget/bg.lproj/Localizable.strings WordPress/WordPressTodayWidget/cs.lproj/Localizable.strings WordPress/WordPressTodayWidget/cy.lproj/Localizable.strings WordPress/WordPressTodayWidget/da.lproj/Localizable.strings WordPress/WordPressTodayWidget/de.lproj/Localizable.strings WordPress/WordPressTodayWidget/en-AU.lproj/Localizable.strings WordPress/WordPressTodayWidget/en-CA.lproj/Localizable.strings WordPress/WordPressTodayWidget/en-GB.lproj/Localizable.strings WordPress/WordPressTodayWidget/es.lproj/Localizable.strings WordPress/WordPressTodayWidget/fr.lproj/Localizable.strings WordPress/WordPressTodayWidget/he.lproj/Localizable.strings WordPress/WordPressTodayWidget/hr.lproj/Localizable.strings WordPress/WordPressTodayWidget/hu.lproj/Localizable.strings WordPress/WordPressTodayWidget/id.lproj/Localizable.strings WordPress/WordPressTodayWidget/is.lproj/Localizable.strings WordPress/WordPressTodayWidget/it.lproj/Localizable.strings WordPress/WordPressTodayWidget/ja.lproj/Localizable.strings WordPress/WordPressTodayWidget/ko.lproj/Localizable.strings WordPress/WordPressTodayWidget/nb.lproj/Localizable.strings WordPress/WordPressTodayWidget/nl.lproj/Localizable.strings WordPress/WordPressTodayWidget/pl.lproj/Localizable.strings WordPress/WordPressTodayWidget/pt-BR.lproj/Localizable.strings WordPress/WordPressTodayWidget/pt.lproj/Localizable.strings WordPress/WordPressTodayWidget/ro.lproj/Localizable.strings WordPress/WordPressTodayWidget/ru.lproj/Localizable.strings WordPress/WordPressTodayWidget/sk.lproj/Localizable.strings WordPress/WordPressTodayWidget/sq.lproj/Localizable.strings WordPress/WordPressTodayWidget/sv.lproj/Localizable.strings WordPress/WordPressTodayWidget/th.lproj/Localizable.strings WordPress/WordPressTodayWidget/tr.lproj/Localizable.strings WordPress/WordPressTodayWidget/zh-Hans.lproj/Localizable.strings WordPress/WordPressTodayWidget/zh-Hant.lproj/Localizable.strings fastlane/metadata/ar-SA/release_notes.txt ```
This is part of project ref paaHJt-2Ib-p2 & paaHJt-2J8-p2
This PR is a follow-up of #17570 and migrates the 3 iOS13-styte widgets (the legacy ones that since got kinda obsoleted by our more modern WordPressStatsWidgets migrated in #17570 but are still relevant to people who didn't update yet)
What went into this PR
AppLocalizedString.swift
file to a more appropriate, shared directory as it is now used by multiple targets (not just theWordPressStatsWidgets
from when this was introduced back in Merge 18.7.0.1 (beta 2) intodevelop
#17562)WordPress*Widget
targets, as well as to the mainWordPress
andJetpack
app targets (since some files using this from next step were included in both the*Widget
and the app targets).WordPress*Widget
targets (i.e. were part of their Compile Sources Build Phase) and for each replaced every use ofNSLocalizedString
withAppLocalizedString
, to ensure they always look up strings from the app's bundle, even in the context of an App Extension / Widget*.lproj/Localizable.strings
files from the various*Widget
targets(†) To ensure I didn't miss any of the files included in those targets' Build Phase, including the files that were not located under
WordPress/WordPress*Widget/**
but also files that lived inWordPress/Classes/**
as they were shared by the app too, I wrote some scripts as below:files_in_targets.rb
Running the script
I then ran the above script to list all the files that were in any of the Widget targets, and pipe it to
ag
viaxargs
to search for any use ofNSLocalizedString
. I then replaced all the relevant occurrences found byAppLocalizedString
instead, and this is what was left at the end of this PR (aka only occurrences in comments), showing that everything from Widgets should be migrated.How to Test
The steps to test are pretty similar to the ones from #17570:
WordPressTodayWidget
,WordPressThisWeekWidget
,WordPressAllTimeWidget
).strings
resources cached in previous build folders).strings
– which contains way more translations than the previous.strings
files that this PR deleted from the widget target – it is more likely that we'll have more translation coverage for free, so I expect that all of "Views" ,"Visitors", "Posts", "Best views ever", "Likes", "Comments" and "Today" have translations in all our Mag16 (while e.g. "Best views ever" was not part of the widget target's.strings
file before and thus likely did not show up as localized in most locales prior to this PR, but now does)Note on Widgets' titles
Note that the names/titles of the 3 iOS13 widgets (which appear in each widget's "title bar") won't be translated. This is because if I'm not mistaken, those titles come from each widget target's
Info.plist
and itsCFBundleDisplayName
key, so to have those translated we'd need to provide a localized title via anInfoPlist.strings
file for each.Given those targets are legacy (obsoleted by the new iOS14+'s
WordPressStatsWidgets
), I didn't feel it worth it to put in place a complicated logic to handle the case for those titles – which would require a fastlane action to redispatch the translated strings from GlotPress into those.strings
files on download, action which I only plan to work on later in the project anyway. So I decided to keep the status quo here and consider that not having those legacy widget's titles localized was acceptable given their obsolescence.Regression Notes
Localization of iOS13 widgets
Tested in simulator in a couple of locales
N/A
PR submission checklist:
RELEASE-NOTES.txt
if necessary.