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

628: Support XLIFF tags in android imports. #636

Merged
merged 17 commits into from Mar 21, 2017

Conversation

Projects
None yet
4 participants
@toolstack
Contributor

toolstack commented Jan 14, 2017

Resolves #628.

@toolstack toolstack added this to the 2.3 milestone Jan 14, 2017

@toolstack toolstack requested a review from ocean90 Jan 14, 2017

@ocean90

Needs a unit test with an example XML file.

Show outdated Hide outdated gp-includes/formats/format-android.php
@toolstack

This comment has been minimized.

Show comment
Hide comment
@toolstack

toolstack Jan 15, 2017

Contributor

Unit test added.

Contributor

toolstack commented Jan 15, 2017

Unit test added.

@ocean90

The export doesn't look right:

<resources>
	<string name="with_xliff">Please don\'t translate &lt;xliff:g id="excluded" example="this">this&lt;/xliff:g> text</string>
</resources>
@toolstack

This comment has been minimized.

Show comment
Hide comment
@toolstack

toolstack Jan 16, 2017

Contributor

That's because the current code encodes all less than signs as HTML entities (see https://github.com/GlotPress/GlotPress-WP/blob/develop/gp-includes/formats/format-android.php#L164). It will get "worse" once we include double quotes in the escaping as well.

The current code assumes that only displayable text is in the export.

I would think that with the current GP workflow that the originals import would contain the xliff tag and then the translators would remove it from the translation. That would maintain the current assumption about the output.

We could do a similar hack and after we've encoded the less than signs, decode any "<xliff:g"'s that exist, but that will become much more cumbersome with the double quote escaping happening.

In an ideal world, when we imported the originals we'd parse the xliff tag and store the metadata somewhere else so the editor could "enforce" the non-editable text in the translation.

Contributor

toolstack commented Jan 16, 2017

That's because the current code encodes all less than signs as HTML entities (see https://github.com/GlotPress/GlotPress-WP/blob/develop/gp-includes/formats/format-android.php#L164). It will get "worse" once we include double quotes in the escaping as well.

The current code assumes that only displayable text is in the export.

I would think that with the current GP workflow that the originals import would contain the xliff tag and then the translators would remove it from the translation. That would maintain the current assumption about the output.

We could do a similar hack and after we've encoded the less than signs, decode any "<xliff:g"'s that exist, but that will become much more cumbersome with the double quote escaping happening.

In an ideal world, when we imported the originals we'd parse the xliff tag and store the metadata somewhere else so the editor could "enforce" the non-editable text in the translation.

@ocean90 ocean90 modified the milestones: 2.3, Future Jan 17, 2017

@toolstack

This comment has been minimized.

Show comment
Hide comment
@toolstack

toolstack Feb 10, 2017

Contributor

Ok, I've updated the PR to change the behaviour of the xliff tags.

The previous code simply kept them as part of the imported text, the new code instead strips them out and adds a comment to indicate what should be done.

For example, if you import an original with the following string in the xml file:

<string name="FileExistsMsg">A file with the specified name (<xliff:g id="filename" example="spreadsheet.xls">%s</xliff:g>) already exists.</string>

the importer will create this original:

A file with the specified name (%s) already exists.

but also add this comment to the translation:

This string has content that should not be translated, the "%s" component of the original, which is identified as the "filename" attribute by the developer may be replaced at run time with text like this: spreadsheet.xls

The translation row the looks like:
xliff-example

I haven't tracked down why the comment doesn't wrap but instead forces the entire meta area to appear below the original/translation, but that should be just a css rule somewhere (any suggestions on that one would be appreciated).

Contributor

toolstack commented Feb 10, 2017

Ok, I've updated the PR to change the behaviour of the xliff tags.

The previous code simply kept them as part of the imported text, the new code instead strips them out and adds a comment to indicate what should be done.

For example, if you import an original with the following string in the xml file:

<string name="FileExistsMsg">A file with the specified name (<xliff:g id="filename" example="spreadsheet.xls">%s</xliff:g>) already exists.</string>

the importer will create this original:

A file with the specified name (%s) already exists.

but also add this comment to the translation:

This string has content that should not be translated, the "%s" component of the original, which is identified as the "filename" attribute by the developer may be replaced at run time with text like this: spreadsheet.xls

The translation row the looks like:
xliff-example

I haven't tracked down why the comment doesn't wrap but instead forces the entire meta area to appear below the original/translation, but that should be just a css rule somewhere (any suggestions on that one would be appreciated).

@toolstack

This comment has been minimized.

Show comment
Hide comment
@toolstack

toolstack Feb 10, 2017

Contributor

@ocean90 the new code should export cleanly now that the xliff tags have been removed from the strings.

Contributor

toolstack commented Feb 10, 2017

@ocean90 the new code should export cleanly now that the xliff tags have been removed from the strings.

@pdalfarr

This comment has been minimized.

Show comment
Hide comment
@pdalfarr

pdalfarr Feb 11, 2017

Great. When will this be available as 'worpress plugin update'?
Thanks !

pdalfarr commented Feb 11, 2017

Great. When will this be available as 'worpress plugin update'?
Thanks !

@toolstack

This comment has been minimized.

Show comment
Hide comment
@toolstack

toolstack Feb 11, 2017

Contributor

@pdalfarr that depends, assuming it gets merged, we currently don't have a date for the next release.

If you want to give it a try though there's only one file you need to replace in your local install.

Contributor

toolstack commented Feb 11, 2017

@pdalfarr that depends, assuming it gets merged, we currently don't have a date for the next release.

If you want to give it a try though there's only one file you need to replace in your local install.

@pdalfarr

This comment has been minimized.

Show comment
Hide comment
@pdalfarr

pdalfarr Feb 13, 2017

@toolstack Import is now OK, thanks!
But export only allow to retrieve '%s'. xliff node seems to be lost :/

pdalfarr commented Feb 13, 2017

@toolstack Import is now OK, thanks!
But export only allow to retrieve '%s'. xliff node seems to be lost :/

@toolstack

This comment has been minimized.

Show comment
Hide comment
@toolstack

toolstack Feb 13, 2017

Contributor

@pdalfarr Correct, since the xliff is only there to let the translators know what to do with the component, there's no need to export it in the final translation file.

Contributor

toolstack commented Feb 13, 2017

@pdalfarr Correct, since the xliff is only there to let the translators know what to do with the component, there's no need to export it in the final translation file.

@ocean90

This comment has been minimized.

Show comment
Hide comment
@ocean90

ocean90 Feb 13, 2017

Member

I'm pretty sure that the tag has to stay. The docs mention another example:

<string name="promo_message">
    Please use the "<xliff:g id="promotion_code">ABCDEFG</xliff:g>” to get a discount.
</string>
Member

ocean90 commented Feb 13, 2017

I'm pretty sure that the tag has to stay. The docs mention another example:

<string name="promo_message">
    Please use the "<xliff:g id="promotion_code">ABCDEFG</xliff:g>” to get a discount.
</string>
@pdalfarr

This comment has been minimized.

Show comment
Hide comment
@pdalfarr

pdalfarr Feb 13, 2017

@ocean90 yes, I think tags has to remains.
Other tools allow to export xliff .. (example: https://support.crowdin.com/api/export-file/ )

But, most important, android translated strings.xml usually contain xliff node if source string.xml does.
This is how it's done is most apps. And this is how it's done in the Android SDK itself.

Example online (from Android platform framework base source):

./values/strings.xml contains:

<-- Format string used to add a suffix like "KB" or "MB" to a number
    to display a size in kilobytes, megabytes, or other size units.
    Some languages (like French) will want to add a space between
    the placeholders. -->
<string name="fileSizeSuffix"><xliff:g id="number" example="123">%1$s</xliff:g><xliff:g id="unit" example="KB">%2$s</xliff:g></string>

and
./values-fr/strings.xml contains:

<string name="fileSizeSuffix" msgid="9164292791500531949">"<xliff:g id="NUMBER">%1$s</xliff:g> <xliff:g id="UNIT">%2$s</xliff:g>"</string>

This means that translations can contains xliff:g with some differences.

In this example, the '%2$s' is present in all translations, but it can be 'wrapped' into xliff nodes with different id value, depending on translation.
As you can see it in this example, some translations can contains 'example' xliff attribute, while other do not contains 'example' attribute.

So I think xliff nodes should be kept when exporting any files, may it be the reference language, but also per language.

This is my understanding of the Android xliff usage. @toolstack What is your opinion about that?

Thanks a lot!

Pascal

pdalfarr commented Feb 13, 2017

@ocean90 yes, I think tags has to remains.
Other tools allow to export xliff .. (example: https://support.crowdin.com/api/export-file/ )

But, most important, android translated strings.xml usually contain xliff node if source string.xml does.
This is how it's done is most apps. And this is how it's done in the Android SDK itself.

Example online (from Android platform framework base source):

./values/strings.xml contains:

<-- Format string used to add a suffix like "KB" or "MB" to a number
    to display a size in kilobytes, megabytes, or other size units.
    Some languages (like French) will want to add a space between
    the placeholders. -->
<string name="fileSizeSuffix"><xliff:g id="number" example="123">%1$s</xliff:g><xliff:g id="unit" example="KB">%2$s</xliff:g></string>

and
./values-fr/strings.xml contains:

<string name="fileSizeSuffix" msgid="9164292791500531949">"<xliff:g id="NUMBER">%1$s</xliff:g> <xliff:g id="UNIT">%2$s</xliff:g>"</string>

This means that translations can contains xliff:g with some differences.

In this example, the '%2$s' is present in all translations, but it can be 'wrapped' into xliff nodes with different id value, depending on translation.
As you can see it in this example, some translations can contains 'example' xliff attribute, while other do not contains 'example' attribute.

So I think xliff nodes should be kept when exporting any files, may it be the reference language, but also per language.

This is my understanding of the Android xliff usage. @toolstack What is your opinion about that?

Thanks a lot!

Pascal

@toolstack

This comment has been minimized.

Show comment
Hide comment
@toolstack

toolstack Feb 13, 2017

Contributor

@ocean90 that example is not really any different than any of the others, just what text is not to be translated.

@pdalfarr No doubt you can export the xliff's again, but they're never used by an Android app from my understanding, they're there only to let translators know what not to translate. They would never change based on which language you were translating to.

If you're going to use a centralized tool like GP to do the translation, then you would import the xliff's as part of the originals, then only do the exports when you're ready to release the app. You wouldn't let your contributors directly edit the xml files but instead direct them to contribute to your GP install.

This is caused by a fundamental difference between the workflows for po/mo files (which contain both the original and the translation) and Android XML files, which only contain the translation.

As GP is designed around several assumptions based on the gettext standard, there's a bit of shoehorning to get Android or other formats that don't share the same assumptions to work in GP.

Storing the xliffs and re-exporting them would require significant changes to GP.

Contributor

toolstack commented Feb 13, 2017

@ocean90 that example is not really any different than any of the others, just what text is not to be translated.

@pdalfarr No doubt you can export the xliff's again, but they're never used by an Android app from my understanding, they're there only to let translators know what not to translate. They would never change based on which language you were translating to.

If you're going to use a centralized tool like GP to do the translation, then you would import the xliff's as part of the originals, then only do the exports when you're ready to release the app. You wouldn't let your contributors directly edit the xml files but instead direct them to contribute to your GP install.

This is caused by a fundamental difference between the workflows for po/mo files (which contain both the original and the translation) and Android XML files, which only contain the translation.

As GP is designed around several assumptions based on the gettext standard, there's a bit of shoehorning to get Android or other formats that don't share the same assumptions to work in GP.

Storing the xliffs and re-exporting them would require significant changes to GP.

@pdalfarr

This comment has been minimized.

Show comment
Hide comment
@pdalfarr

pdalfarr Feb 13, 2017

@ocean90 Thanks for the reply.

I understand your point of view, and "Storing the xliffs and re-exporting them would require significant changes to GP" close the discussion then ;-)

The new functionality to take xliff into account is really great. The additional one I just mentioned was just 'cherry on the cake'. Without it, the cake is still really tasty :-)

Thanks

PS: I tested your WordPress GP Require Login plugin: it's working fine. Thanks!

pdalfarr commented Feb 13, 2017

@ocean90 Thanks for the reply.

I understand your point of view, and "Storing the xliffs and re-exporting them would require significant changes to GP" close the discussion then ;-)

The new functionality to take xliff into account is really great. The additional one I just mentioned was just 'cherry on the cake'. Without it, the cake is still really tasty :-)

Thanks

PS: I tested your WordPress GP Require Login plugin: it's working fine. Thanks!

@toolstack

This comment has been minimized.

Show comment
Hide comment
@toolstack

toolstack Feb 13, 2017

Contributor

@pdalfarr We've had discussions on "non-translatable" areas before and it is on the radar, just not a priority yet.

Glad the plugin did what you needed.

Contributor

toolstack commented Feb 13, 2017

@pdalfarr We've had discussions on "non-translatable" areas before and it is on the radar, just not a priority yet.

Glad the plugin did what you needed.

@toolstack toolstack modified the milestones: 2.4, Future Feb 14, 2017

@toolstack

This comment has been minimized.

Show comment
Hide comment
@toolstack

toolstack Feb 15, 2017

Contributor

I've updated the PR to fix the css issue as well as add some additional test cases and support single quote attributes in the xliff tag.

@ocean90 when you get some time take a look at the css "fix" it changes the behaviour of the translation row a bit but I think it works better overall.

Contributor

toolstack commented Feb 15, 2017

I've updated the PR to fix the css issue as well as add some additional test cases and support single quote attributes in the xliff tag.

@ocean90 when you get some time take a look at the css "fix" it changes the behaviour of the translation row a bit but I think it works better overall.

Show outdated Hide outdated assets/css/style.css
Show outdated Hide outdated gp-includes/formats/format-android.php
Show outdated Hide outdated gp-includes/formats/format-android.php
@ocean90

This comment has been minimized.

Show comment
Hide comment
@ocean90

ocean90 Feb 21, 2017

Member

Can we move the plural stuff into its own PR?

Member

ocean90 commented Feb 21, 2017

Can we move the plural stuff into its own PR?

@toolstack

This comment has been minimized.

Show comment
Hide comment
@toolstack

toolstack Feb 21, 2017

Contributor

@ocean90 Yes, I was considering it after it became obvious it was going to be more complex than at first sight.

Contributor

toolstack commented Feb 21, 2017

@ocean90 Yes, I was considering it after it became obvious it was going to be more complex than at first sight.

@toolstack toolstack merged commit d7eeeed into develop Mar 21, 2017

3 checks passed

codecov/project 38.24% (+0.5%) compared to cb79407
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@toolstack toolstack deleted the 628-support-xliff-in-android-files branch Mar 21, 2017

@toolstack toolstack modified the milestones: 3.0, 2.4 Aug 15, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment