Skip to content

Automate updating gutenberg translations#10719

Merged
Tug merged 25 commits intogutenberg/release-1.17.0from
gutenberg/fix-i18n
Nov 15, 2019
Merged

Automate updating gutenberg translations#10719
Tug merged 25 commits intogutenberg/release-1.17.0from
gutenberg/fix-i18n

Conversation

@Tug
Copy link
Copy Markdown
Contributor

@Tug Tug commented Oct 31, 2019 •
edited
Loading

Fixes wordpress-mobile/gutenberg-mobile#939

Gutenberg PR: wordpress-mobile/gutenberg-mobile#1520

This PR automates adding gutenberg mobile strings to the main strings.xml files in order to have those translated by glotpress. It also adds support for plurals in GutenbergEditorFragment (need to check that glotpress can parse <plurals> tags).

Testing Instructions

  • Checkout the branch locally and make sure the gutenberg-submodule is up to date (or targets develop)
  • Run python tools/merge_strings_xml.py
  • Check the main strings.xml at WordPress/src/main/res/values/strings.xml and make sure it contains all the strings from gutenberg-mobile/bundle/android
  • Build the app to make sure the main xml is valid

PR submission checklist:

  • Update the release process when the PR lands.

  • I have considered adding unit tests where possible.

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

@Tug Tug self-assigned this Oct 31, 2019
Tug added 17 commits November 7, 2019 14:05
@Tug Tug force-pushed the gutenberg/fix-i18n branch from 80560d2 to 7772480 Compare November 7, 2019 14:31
@Tug Tug requested review from hypest and loremattei November 7, 2019 14:40
@Tug Tug marked this pull request as ready for review November 7, 2019 14:44
@Tug Tug mentioned this pull request Nov 7, 2019
Merged
@peril-wordpress-mobile
Copy link
Copy Markdown

peril-wordpress-mobile bot commented Nov 7, 2019 •
edited
Loading

You can test the changes on this Pull Request by downloading the APK here.

@Tug Tug force-pushed the gutenberg/fix-i18n branch from 9ff442e to cfeaea6 Compare November 8, 2019 12:31
@Tug
Copy link
Copy Markdown
Contributor Author

Tug commented Nov 8, 2019

I have reverted any changes to libs/gutenberg-mobile as that would mean a new gutenberg release. Let's merge the scripts and check again that the strings generate as expected during the next gutenberg release.

@hypest
Copy link
Copy Markdown
Contributor

hypest commented Nov 13, 2019

Looks good to me from the gutenberg-mobile POV so, I'll leave it to @loremattei or @jkmassel to test out the release process to see if the generated strings file works fine there.

While at it @loremattei , I wonder if it would be easy to modify the release scripts to look for strings.xml as normal but also a strings-gutenberg.xml if we wanted to have the auto-generated fine as a separate file (easier to keep the main strings.xml clean). WDYT?

Tug
Tug commented Nov 13, 2019
View reviewed changes
...itor/WordPressEditor/src/main/java/org/wordpress/android/editor/GutenbergEditorFragment.java
// Filter out all strings that are not prefixed with `gutenberg_mobile_`
if (!fieldName.startsWith("gutenberg_mobile_")) {
// Filter out all strings that are not prefixed with `gutenberg_native_`
if (!fieldName.startsWith("gutenberg_native_")) {
Copy link
Copy Markdown
Contributor Author

@Tug Tug Nov 13, 2019

Choose a reason for hiding this comment

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

Given this change we actually need to update values/strings.xml in this PR if we don't want to lose the gutenberg translations in the next release.

Copy link
Copy Markdown
Contributor Author

@Tug Tug Nov 13, 2019

Choose a reason for hiding this comment

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

Let's target #10769 instead of develop se we can have access to the gutenberg-mobile strings.xml and thus re-generate the main strings.xml

@Tug Tug changed the base branch from develop to gutenberg/release-1.17.0 November 13, 2019 16:24
@Tug Tug force-pushed the gutenberg/fix-i18n branch from 46214ff to e89767d Compare November 13, 2019 16:29
@Tug
Copy link
Copy Markdown
Contributor Author

Tug commented Nov 13, 2019

Updated to target gutenberg/release-1.17.0 instead of develop.
If we merged to develop and someone runs python tools/merge_strings_xml.py it will alter the main strings.xml in an unpredictable way. We need to do it manually from this PR first in order to remove old references cleanly.

@Tug Tug force-pushed the gutenberg/fix-i18n branch from 85bd2bb to e64e860 Compare November 14, 2019 09:11
@loremattei
Copy link
Copy Markdown
Contributor

loremattei commented Nov 14, 2019

Hey @Tug! This looks good to me. I tested it and it works as expected.

I just have a question about the process as I'm not sure we ended up with an answer in the chats I saw: when are we going to run this script? Do we update the strings as a part of the PR that updates the mobile-gutenberg hash, or do we want to run it as a part of the code freeze process?
I'm fine with both solutions, I just want to make sure that the workflow is clear :-)

I wonder if it would be easy to modify the release scripts to look for strings.xml as normal but also a strings-gutenberg.xml if we wanted to have the auto-generated fine as a separate file (easier to keep the main strings.xml clean)

@hypest I'm not sure what you mean here 🤔I think @Tug and @mchowning explored the possibility of uploading more than one file to GlotPress and it turned out to be not possible... but if you meant something different, let me know.

@Tug Tug merged commit c6d80f1 into gutenberg/release-1.17.0 Nov 15, 2019
@Tug Tug deleted the gutenberg/fix-i18n branch November 15, 2019 15:34
@hypest
Copy link
Copy Markdown
Contributor

hypest commented Nov 15, 2019

I think @Tug and @mchowning explored the possibility of uploading more than one file to GlotPress and it turned out to be not possible... but if you meant something different, let me know.

What I meant is to script/generate a single file out of the two I'm proposing, and send the unified file to GlotPress.

Tug added a commit that referenced this pull request Nov 15, 2019
@Tug
Revert "Merge pull request #10719 from wordpress-mobile/gutenberg/fix…
669110e 
…-i18n"

This reverts commit c6d80f1, reversing
changes made to e0ded2a.
@Tug Tug restored the gutenberg/fix-i18n branch November 15, 2019 18:06
@Tug Tug deleted the gutenberg/fix-i18n branch November 16, 2019 15:50
@Tug Tug mentioned this pull request Nov 16, 2019
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hypest hypest Awaiting requested review from hypest

1 more reviewer

@loremattei loremattei loremattei approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@Tug Tug

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Tug @hypest @loremattei