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

WIP: Proposal and POC for adding support for Inline elements #13

Closed

Conversation

probertson
Copy link
Contributor

@probertson probertson commented Mar 15, 2018

WIP - DO NOT MERGE

This is a proposal for adding support for "inline" child elements that can occur in source and target values. (Related: #6)

I have built a proof-of-concept implementation for this that adds support for one type of inline element (XLIFF 1.2 <ph></ph> elements). However, because this involves making a (backwards-compatible) change to the library's object model, I wanted to open up a conversation about the object model changes first before writing a complete implementation.

I also added a file "multi-child-strawman.md" containing the text of the proposal. This file isn't meant to be merged in -- I just added it as a file in the repository so that we can add comments and discussion on a per-line basis. All the other code changes are the work for the proof-of-concept implementation.

Once a decision is reached on the object model changes, I plan to do the full implementation (probably in a separate Pull Request).

@adrai
Copy link
Contributor

adrai commented Mar 15, 2018

fyi: we are using xliff also in combination for a bidirectional conversion from and to i18next format: https://www.i18next.com/interpolation.html
would be nice if this would be considered ;-)

@adrai
Copy link
Contributor

adrai commented Mar 15, 2018

btw: you really do good considerations and I appreciate it very much as you put up for the whole thing.

There is no need to use an array of objects for the attributes -- it can just be a single object
@probertson
Copy link
Contributor Author

I added a few more commits. One is an improvement to the spec (I realized that I had proposed a structure that is more complex than it needs to be).

The other main change is that I added two sets of tests to make sure the concept works for i18next-style parameters. The strings used in the examples are from the i18next documentation referenced above.

The "example_i18next_placeholder.*" examples show how to create (and read) structured XLIFF using the parameter style that's used in i18next. For example, it includes the following XLIFF:

<source>
  <ph id="name" ctype="x-i18next-param">{{name}}</ph> is 
  <ph id="how" ctype="x-i18next-param">{{how}}</ph>
</source>

That XLIFF becomes the following JSON structure:

"source": [
  { "ph": { "id": "name", "ctype": "x-i18next-param", "content": "{{name}}" } },
  " is ",
  { "ph": { "id": "how", "ctype": "x-i18next-param", "content": "{{how}}" } }
],

The formatting of the parameter itself doesn't matter, so whether it's {myVar} or {{myVar}} or %(myVar)s or foo doesn't matter to the xliff library (and it's not part of the XLIFF spec either -- XLIFF allows whatever you want in there). It is up to library consumers to create the structure, so their code is the only thing that needs to be able to understand what the contents of the <ph> tag mean to them.

In those examples I added a ctype value. However, the ctype parameter is optional (so for i18next you might just choose to omit it). Also, XLIFF allows custom values for ctype when it is specified, as long as they begin with x-. I just invented the ctype values in the examples. Again, it's up to library consumers to use or ignore those ctype values for their own purposes.


I also added a set of examples named "example_i18next_unstructured.*". Those examples are demonstrating backwards compatibility. (I'm guessing that currently you simply include the i18next interpolation values in the strings directly -- if you do something else with them I'd be happy to modify the examples and make sure that case is still supported.)

In these examples the interpolation variables are just included directly in the string:

XLIFF:
<source>{{name}} is {{how}}</source>

JSON:
"source": "{{name}} is {{how}}"

The proposed changes don't try to do anything "smart" to identify and extract interpolation markers -- the code just takes the object structure it's given and turns it into XLIFF (and vice-versa). In this case the interpolation markers are part of the string, so they're treated as plain text and nothing happens to them.

@probertson
Copy link
Contributor Author

Also, I just noticed that having the proposal included as actual code in the MR has a downside -- it isn't as easy to read (especially the tables). So maybe it would be easier to read it by viewing the .md file directly (using the "View" button). Sorry about that!

@adrai
Copy link
Contributor

adrai commented Mar 16, 2018

cool 😎 👍

@probertson
Copy link
Contributor Author

I went ahead and added a few comments to the proposal document. Hopefully that makes it more clear what specific questions I'm hoping to get answers to. Of course feel free to comment on anything 😄

@adrai
Copy link
Contributor

adrai commented Mar 17, 2018

In addition to text, the XLIFF specifications define multiple child tags that can be included in `<source>`/`<target>` tags:

- [XLIFF 1.2 spec](http://docs.oasis-open.org/xliff/v1.2/os/xliff-core.html#Specs_Elem_Inline)
- [XLIFF 2.0 spec](http://docs.oasis-open.org/xliff/xliff-core/v2.0/xliff-core-v2.0.html#inlineCodes)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is xliff 2.1 specification the same as 2.0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a great question. I didn't actually realize that the XLIFF 2.1 spec had been release.

After looking over the 2.1 spec and comparing it to the 2.0 spec, I believe that for the point of this proposal they are the same.

Dirty details if you want them:

As a quick way of trying to identify changes I downloaded the two specs (2.0 and 2.1) and compared them using a diff tool. Surprisingly that was actually pretty effective.

From what I found, the notable changes to 2.1 (i.e. new sections, things that are more than just minor clarifications or notes) are:


While there are some consistencies, there is also some variation among the elements. Consequently, the object structure for inline elements needs to be flexible enough to support various attributes and child elements.

### Proposed implementation
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some questions related to this section:

  • Do you have a preference for Option 1 or Option 2?
  • Should the id attribute be treated differently than others (as proposed)? Or should it just be included in the attributes?
  • Should id be required? Specifically, should we do any validation of that, and throw errors if it's not present? In the XLIFF spec id is required, but I don't have any idea whether apps that consume XLIFF really care about it or not. In some cases it probably matters, if other parts of the document are referencing it, but otherwise it doesn't seem like it would matter. Certainly if an XLIFF file doesn't include an id there's no reason we can't pull the data out of it anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

Personally I prefer Option 1.
I would not throw any validation errors by default... but perhaps there could be a dedicated strict mode that would then throw... what do you think?

| Orphan end marker of spanning code | `<it></it>` | `<ec isolated="yes"/>` |
| Sub-flow text | `<sub></sub>` | (separate `<unit></unit>`) |

### Proposed implementation
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this section:

  • Do you have a preference for Option 1, Option 2, or Option 3?
  • Are you okay with the proposal to not support "Orphan" markers (inline elements that are part of a pair, where only one part of the pair is in the translation unit)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Option 1

I would say it’s ok to not support “Orphan” markers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since I first wrote this I have talked with the translation vendor that my company uses. They only "officially" support XLIFF 1.2. Also, their tools generally use <ph></ph> (the "native" span) rather than <g></g> (the generic span. Based on my conversations with them, I would personally prefer to go with Option 2. That way we can do a round-trip XLIFF 1.2 -> JS -> XLIFF 1.2 without losing the "native"-ness of the elements.

Are you okay with that @adrai?

Copy link
Contributor

Choose a reason for hiding this comment

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

for sure... this is fine.

In addition to text, the XLIFF specifications define multiple child tags that can be included in `<source>`/`<target>` tags:

- [XLIFF 1.2 spec](http://docs.oasis-open.org/xliff/v1.2/os/xliff-core.html#Specs_Elem_Inline)
- [XLIFF 2.0 spec](http://docs.oasis-open.org/xliff/xliff-core/v2.0/xliff-core-v2.0.html#inlineCodes)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a great question. I didn't actually realize that the XLIFF 2.1 spec had been release.

After looking over the 2.1 spec and comparing it to the 2.0 spec, I believe that for the point of this proposal they are the same.

Dirty details if you want them:

As a quick way of trying to identify changes I downloaded the two specs (2.0 and 2.1) and compared them using a diff tool. Surprisingly that was actually pretty effective.

From what I found, the notable changes to 2.1 (i.e. new sections, things that are more than just minor clarifications or notes) are:

@adrai
Copy link
Contributor

adrai commented Mar 19, 2018

I added my personal comments ...
But I'm not a good scale ;-)

@probertson
Copy link
Contributor Author

@spencerfdavis @birkestroem @AlexanderNoskov @digitalica @diwakarbhatt123 @helderjf @kmikodev @ntripathy

Are any of you interested in reviewing this proposal? If so, do you have an idea of approximately when you'll be able to look over this and respond? I realize this probably isn't your top priority, but I want to make sure you have an opportunity to share your thoughts.

@probertson
Copy link
Contributor Author

@adrai I've been letting this sit for a while to give other people a chance to review. At this point I doubt that anyone else is going to comment.

I added one additional comment and I am interested in your thoughts on that. Otherwise I would like to go ahead and start moving forward on an implementation of this proposal. Are you okay with moving forward? Or are there other contributors that should have a chance to review first?

@adrai
Copy link
Contributor

adrai commented Apr 9, 2018

Fully agree. Go ahead as you want/like.

@adrai adrai closed this Jul 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants