628: Support XLIFF tags in android imports. #636
Conversation
Needs a unit test with an example XML file. |
* | ||
* See the "Mark message parts that should not be translated" section of https://developer.android.com/distribute/tools/localization-checklist.html | ||
* | ||
* Unfortunatly simplexml will parse these are valid XML tags, which we don't want so encapsalate them in a CDATA structure |
ocean90
Jan 14, 2017
Member
Typos:
- Unfortunatly => Unfortunately
- encapsalate => encapsulate
- simplexml => SimpleXML
Typos:
- Unfortunatly => Unfortunately
- encapsalate => encapsulate
- simplexml => SimpleXML
Unit test added. |
The export doesn't look right: <resources>
<string name="with_xliff">Please don\'t translate <xliff:g id="excluded" example="this">this</xliff:g> text</string>
</resources> |
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. |
757965c
to
2912410
@ocean90 the new code should export cleanly now that the xliff tags have been removed from the strings. |
Great. When will this be available as 'worpress plugin update'? |
@toolstack Import is now OK, thanks! |
@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. |
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> |
@ocean90 yes, I think tags has to remains. But, most important, android translated strings.xml usually contain xliff node if source string.xml does. Example online (from Android platform framework base source): ./values/strings.xml contains:
and
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. 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 |
@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. |
@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 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. |
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. |
|
||
.editor .strings .textareas textarea { | ||
width: 95%; | ||
resize: none; |
ocean90
Feb 19, 2017
Member
This should be resize: vertical
to allow users to resize the textarea vertically if a string has more than ~7 lines.
This should be resize: vertical
to allow users to resize the textarea vertically if a string has more than ~7 lines.
* @param obj $string The string entry object to use. | ||
* @param string $context The context string to use. | ||
* | ||
* @return obj A translation entry object. |
ocean90
Feb 19, 2017
Member
obj
should be Translation_Entry
.
obj
should be Translation_Entry
.
* | ||
* @since 1.0.0 | ||
* | ||
* @param obj $string The string entry object to use. |
ocean90
Feb 19, 2017
Member
It seems like $string
is an array.
Generally, if the type is an object than object
should be used.
It seems like $string
is an array.
Generally, if the type is an object than object
should be used.
* | ||
* @param obj $locale The locale object to use for exporting. | ||
*/ | ||
private function get_plural_order( $locale ) { |
akirk
Feb 21, 2017
Member
This should be unit tested. I was under the impression that this info cannot be generated but needs to be read from a source like CLDR. In my tests with Russian it returned array( 'other', 'other', 'other' )
for 1,21,31
, 2,3,4
, and 0,5,6
. Would be great to see this working without an external dependency, as I wanted to implement stringsdict
for iOS which needs the same classification.
This should be unit tested. I was under the impression that this info cannot be generated but needs to be read from a source like CLDR. In my tests with Russian it returned array( 'other', 'other', 'other' )
for 1,21,31
, 2,3,4
, and 0,5,6
. Would be great to see this working without an external dependency, as I wanted to implement stringsdict
for iOS which needs the same classification.
toolstack
Feb 21, 2017
Author
Contributor
Yes, the code in that function is primarily a placeholder at the moment, it needs more work to handle the cases better.
In the end we might have to add the Android types to the locales definitions (I'd like to avoid that if possible).
Yes, the code in that function is primarily a placeholder at the moment, it needs more work to handle the cases better.
In the end we might have to add the Android types to the locales definitions (I'd like to avoid that if possible).
akirk
Feb 21, 2017
Member
https://github.com/mlocati/cldr-to-gettext-plural-rules has an implementation along the lines of what would be needed. The generated output at https://mlocati.github.io/cldr-to-gettext-plural-rules/ shows the resulting mappings.
https://github.com/mlocati/cldr-to-gettext-plural-rules has an implementation along the lines of what would be needed. The generated output at https://mlocati.github.io/cldr-to-gettext-plural-rules/ shows the resulting mappings.
Can we move the plural stuff into its own PR? |
@ocean90 Yes, I was considering it after it became obvious it was going to be more complex than at first sight. |
Other minor corner case and grammer fixes.
de1aab4
to
e2924d0
Also bump the asset version.
toolstack commentedJan 14, 2017
Resolves #628.