Skip to content
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

Do no escape strings with double quotes in android strings.xml #1637

Merged
merged 9 commits into from
Jan 9, 2020

Conversation

Tug
Copy link
Contributor

@Tug Tug commented Nov 28, 2019

Wrapping strings in double quotes such as

<string name="my_string">"\n    This is my string.\nHello"</string>

Will import the string in glotpress as "\n This is my string.\nHello" instead of \n This is my string.\nHello.
In order not to update glotpress and translate.wordpress.org which would be costly, let's revert to using simple escaping of xml entities and not use "". In theory we should only lose the ability to have leading and trailing whitespace characters in a string, which is not something that happens frequently. So the resulting sting in glotpress will be This is my string.\nHello.

To make sure the created xml is valid we run xmllint automatically once the npm script is complete.

To test:

  • Check that XML looks valid in bundle/android/strings.xml
  • Optional: copy-paste all the strings from bundle/android/strings.xml into WordPress/src/main/res/values/strings.xml in WPAndroid and build and run the app with those strings. You can boot the block editor in debug mode to see translations being printed in the console
  • Optional: Try to edit gutenberg-android.pot, adding special characters to it such as <, >, ", \n..., then run ./bin/po2android.js gutenberg-android.pot bundle/android/strings.xml, it should create a valid xml file. Validate file with yarn genstrings:android:check

Update release notes:

  • If there are user facing changes, I have added an item to RELEASE-NOTES.txt.

@Tug Tug added the i18n label Nov 28, 2019
@Tug Tug self-assigned this Nov 28, 2019
@Tug Tug requested a review from hypest November 28, 2019 17:10
@Tug Tug marked this pull request as ready for review November 29, 2019 19:18
@Tug Tug changed the base branch from develop to release/1.19.0 December 10, 2019 15:25
@Tug Tug changed the base branch from release/1.19.0 to develop December 12, 2019 16:29
@hypest
Copy link
Contributor

hypest commented Dec 18, 2019

Try to edit gutenberg-android.pot

👋 @Tug , where is that file?

@Tug
Copy link
Contributor Author

Tug commented Dec 18, 2019

@hypest It's at the root of the project, you should have it after running yarn genstrings once

@hypest
Copy link
Contributor

hypest commented Dec 18, 2019

Tried out manually adding \ (with the spaces) and \\ in the .pot file and what I see in the output strings.xml is that the single backslash is just gone, while the double-backslash is preserved as is. Should I have seen them be replaced with \\ and \\\\ instead?

@Tug
Copy link
Contributor Author

Tug commented Dec 18, 2019

Actually the POT format supports character escaping using backslash so \ in a pot file is actually a special character that needs to be escaped to \\ in order to register a \.

So the POT parser will have removed those when the string is given to escapeResourceXML. This function will simply add them back before we dump the string in the XML/

So basically: (POT => XML)
\ =>
\n => \n
\\ => \\

@hypest
Copy link
Contributor

hypest commented Dec 20, 2019

I see. The PR looks good to me @Tug , let's bring it up to date with develop to fix the conflicts and I'll do last review pass afterwards.

@Tug
Copy link
Contributor Author

Tug commented Dec 20, 2019

@hypest updated!

@hypest
Copy link
Contributor

hypest commented Dec 20, 2019

Chances are, the failing Android device tests are due to a recent issue (#1703), which is now fixed. Can you update again feom develop @Tug ? Thanks!

@Tug
Copy link
Contributor Author

Tug commented Dec 20, 2019

I merged in develop but e2e tests are still failing. The error seems unrelated to my changes though

@Tug Tug changed the base branch from develop to release/1.20.0 January 9, 2020 14:36
Copy link
Contributor

@hypest hypest left a comment

Choose a reason for hiding this comment

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

LGTM! Feel free to merge when CI turns green!

@Tug Tug merged commit 1aa50ea into release/1.20.0 Jan 9, 2020
@Tug Tug deleted the update/simplify-android-strings-xml branch January 9, 2020 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants