Skip to content

[Tooling] Migrate ShareExtension to use strings from app bundle - #17636

Merged
AliSoftware merged 5 commits into
developfrom
tooling/l10n-step2/share-extension
Dec 8, 2021
Merged

[Tooling] Migrate ShareExtension to use strings from app bundle#17636
AliSoftware merged 5 commits into
developfrom
tooling/l10n-step2/share-extension

Conversation

@AliSoftware

@AliSoftware AliSoftware commented Dec 7, 2021

Copy link
Copy Markdown
Contributor

📝 This is part of project ref paaHJt-2Ib-p2 & paaHJt-2J8-p2
➕ This builds on top of #17630 and the tooling/l10n-step2/ios13-widgets branch.

What's in this PR

Same stuff as what I did with the iOS14-style Widget and the 3 iOS13-style widgets in previous PRs:

  1. Added AppLocalizedStrings.swift to the WordPressShareExtension target. Also had to add it to the WordPressDraftActionExtension target, because files which I migrated to AppLocalizedString in step 2 happen to also be part of that target too, so I needed it to make that target continue to compile too
  2. Migrated all the usages of NSLocalizedString in any of the files that are part of that WordPressShareExtension's "Compile Sources" build phase, to use AppLocalizedString instead, so that it looks up the strings in the app bundle(†)
  3. Checked(‡) that all the keys and values in the *.lproj/Localizable.strings files that were in the WordPressShareExtension target so fare were also present in the corresponding WordPress/Resources/*.lproj/Localizable.strings, with the same value, to ensure it was safe to delete them
  4. Delete all the WordPress/WordPressShareExtension/*.lproj/Localizable.strings files from the disk and the ShareExtension target in Xcode

Note: Scripts I used to help me with steps 2 & 3

(†) Script to check that the keys in .strings files matched

This script iterated over each *.lproj/Localizable.strings of the ShareExtension target, and for each checks that all the keys and values present in that file were also present and with the same translation in the corresponding .strings file from the app bundle.

#!/usr/bin/env ruby

require 'json'

### Checks that any key that was in the `*.lproj/Localizable.strings` files of `targets`
### are also present in the main app's `Resources/*.lproj/Localizable.strings`
### and with the same translation.

def dict(plist_path)
  JSON.parse(`plutil -convert json -o - #{plist_path}`)
end

targets = ['WordPressShareExtension', 'WordPressTodayWidget', 'WordPressThisWeekWidget', 'WordPressAllTimeWidget']

targets.each do |target|
  puts "== #{target} =="
  Dir["WordPress/#{target}/*.lproj/Localizable.strings"].each do |tlf|
    lproj = File.basename(File.dirname(tlf))
    next if lproj == 'Base.lproj'
    widget_strings = dict(tlf)
    app_strings = dict("WordPress/Resources/#{lproj}/Localizable.strings")
    puts " - Checking #{lproj}..."
    ok_count = 0
    widget_strings.each do |wk,wv|
      av = app_strings[wk]
      if av != wv
        puts "    ! Key #{wk} does not have the same #{lproj} translation (#{av} vs #{wv})"
      else
        ok_count += 1
      end
    end
    puts "   > #{ok_count}/#{widget_strings.count} keys matched."
  end
end
(‡) Script to find all occurrences of NSLocalizedString used in a target

This is a script I've also already used in my previous PRs, and lists all the files that are part of the "Compile Sources" build phase of the given target(s), so that I could pipe it with xargs -0 ag 'NSLocalizedString' to find all files used by the ShareExtension where this method/macro was used, and replace those with AppLocalizedString insterad.

#!/usr/bin/env ruby

require 'xcodeproj'

def die!(error)
  puts error
  exit 1
end

def list(array)
  array.map { |x| " - #{x}" }.join("\n")
end

die! 'Name of a target is required as first parameter.' if ARGV.count == 0
targets_to_check = ARGV

project = Xcodeproj::Project.open('WordPress/WordPress.xcodeproj')
targets = targets_to_check.map do |name|
  project.targets.find { |t| t.name == name } \
    or die! "Can't find target #{name}. Available targets are:\n#{list(project.targets.map(&:name))}."
end

phases = targets.map do |t|
  t.build_phases.find { |p| p.is_a?(Xcodeproj::Project::Object::PBXSourcesBuildPhase) } \
    or die! "The target #{target} does not seem to have a Sources Build Phase."
end

files = phases.flat_map { |p| p.files_references.map(&:real_path) }.sort.uniq

# puts "Files compiled as part of targets #{targets_to_check.inspect}:"
# puts list(files.map(&:to_s))
print files.join("\0")

To Test

  • Select the WordPressShareExtension scheme in Xcode
  • Clean the target, to ensure there's no leftover of .strings files in the target's build cache
  • Check that your simulator is in a Locale other than English. Also Edit the scheme, and under Run > Options tags, select the same locale and region for which you want to test.
  • Run the Scheme. At that point Xcode should show a popup asking you which host app you want to run (see below) to test that share extension. Personally I just chose Safari
  • Once Xcode launched Safari in your Simulator, go to a site of your choice, then hit the ⏍ share button in Safari's toolbar, and choose the WordPress action
  • In the modal that shows, check that the buttons at the top (Cancel and Next) are translated (they are barely visible at least on a phone in Light mode 😞 – but eh, 🤷 that's not that PR's problem…).
  • Then tap the Next button. The next page should show a loading screen whose copy ("Fetching sites..." in English) should also be translated in the current locale (and not appear in English)
  • Once the loading screen has finished fetching the sites and shows the next screen, check that all the copies are translated as well. Note that the list of categories of the post (on the last screen) are not translated (but I think that's expected since that's likely coming from the backend… and specific to each site?)
  • Optional: continue playing with the share extension further, maybe even actually publishing the post on one of your test site, and/or turning off your network and try to publish to trigger error messages… and check that all the flow is still translated in the locale you're testing into.

Demo

Xcode asking you which host app to launch to test the Share Extension with:

Xcode asking you which host app to launch to test the Share Extension with

Demo of testing the extension in French via Xcode.

ShareExtension-Demo-FR.mp4

Regression Notes

  1. Potential unintended areas of impact

Translation of Share Extension in various locale.

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

Tested various flows of the Share Extension in simulator, with 2 locales

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

None

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.

In all files which are under WordPress/ShareExtension/**
This time for any other files that were not under WordPress/WordPressShareExtension/** (but in WordPress/Classes/** instead)… but were still built as part of the ShareExtension target
As with the changes, some files built as part of the ShareExtension and now using AppLocalizedString() are apparently also part of the DraftAction target too
@AliSoftware AliSoftware added the Tooling Build, Release, and Validation Tools label Dec 7, 2021
@AliSoftware AliSoftware added this to the 18.9 milestone Dec 7, 2021
@AliSoftware
AliSoftware requested a review from a team December 7, 2021 21:03
@AliSoftware AliSoftware self-assigned this Dec 7, 2021
@peril-wordpress-mobile

Copy link
Copy Markdown

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

@peril-wordpress-mobile

Copy link
Copy Markdown

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

@mokagio mokagio left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good. 👍 Will report later after manual testing.

@mokagio mokagio left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worked as expected with Italian localizations:

Screen Shot 2021-12-08 at 6 46 15 am Screen Shot 2021-12-08 at 6 50 16 am

Base automatically changed from tooling/l10n-step2/ios13-widgets to develop December 8, 2021 17:48
@AliSoftware
AliSoftware merged commit 594d186 into develop Dec 8, 2021
@AliSoftware
AliSoftware deleted the tooling/l10n-step2/share-extension branch December 8, 2021 17:52
mokagio added a commit that referenced this pull request Dec 9, 2021
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`
AliSoftware added a commit that referenced this pull request Dec 17, 2021
…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.
mokagio added a commit that referenced this pull request Jan 10, 2022
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
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Tooling Build, Release, and Validation Tools

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants