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.

Sign up for GitHub

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

Jump to bottom

i18n: Reference ICU ids from container messages #43534

Closed
wants to merge 6 commits into from
Closed

i18n: Reference ICU ids from container messages #43534

wants to merge 6 commits into from

Conversation

petebacondarwin
Copy link
Member

@petebacondarwin petebacondarwin commented Sep 22, 2021 •
edited

When extracting i18n messages from templates, ICU messages are split out from the
message that contains them. This can make it difficult in the translation files to match up
the two messages, especially if the ICU is reused in multiple placeholders.

Fixes #17506

Local testing in the AIO project allows us to generate XLIFF that looks like:

<?xml version="1.0" encoding="UTF-8" ?>
<xliff version="1.2" xmlns="urn:oasis:names:tc:xliff:document:1.2">
  <file source-language="en-US" datatype="plaintext" original="ng2.template">
    <body>
      <trans-unit id="5982288889525183494" datatype="html">
        <source>Before <x id="ICU" equiv-text="{showTopMenu, select, true {blue} false {green} other {red}}" xid="1000974510044941815"/> After</source>
        <context-group purpose="location">
          <context context-type="sourcefile">src/app/app.component.html</context>
          <context context-type="linenumber">7,9</context>
        </context-group>
      </trans-unit>
      <trans-unit id="1000974510044941815" datatype="html">
        <source>{VAR_SELECT, select, true {blue} false {green} other {red}}</source>
        <context-group purpose="location">
          <context context-type="sourcefile">src/app/app.component.html</context>
          <context context-type="linenumber">7</context>
        </context-group>
      </trans-unit>
    </body>
  </file>
</xliff>

Note that the <x id="ICU"...> has an xid attribute that matches the id of the ICU message later in the file.

@petebacondarwin petebacondarwin added action: review The PR is still awaiting reviews from at least one requested reviewer area: i18n area: compiler Issues related to `ngc`, Angular's template compiler target: minor This PR is targeted for the next minor release labels Sep 22, 2021
@ngbot ngbot bot modified the milestone: Backlog Sep 22, 2021
@google-cla google-cla bot added the cla: yes label Sep 22, 2021
@petebacondarwin petebacondarwin force-pushed the i18n-ICU-ref-attributes branch 2 times, most recently from 8993675 to a37cba3 Compare September 22, 2021 10:11
@petebacondarwin petebacondarwin added state: WIP and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Sep 22, 2021
@petebacondarwin petebacondarwin marked this pull request as draft September 22, 2021 16:17
@petebacondarwin petebacondarwin removed the request for review from AndrewKushnir September 22, 2021 16:17
@petebacondarwin petebacondarwin force-pushed the i18n-ICU-ref-attributes branch 4 times, most recently from 4c720d6 to 43e4d95 Compare October 3, 2021 15:48
@petebacondarwin petebacondarwin added action: review The PR is still awaiting reviews from at least one requested reviewer and removed state: WIP labels Oct 4, 2021
@petebacondarwin petebacondarwin marked this pull request as ready for review October 4, 2021 13:56
@petebacondarwin petebacondarwin added the action: presubmit The PR is in need of a google3 presubmit label Oct 4, 2021
@AndrewKushnir
Copy link
Contributor

@petebacondarwin I've started looking at the change, but I need another day or two to complete the review process. Meanwhile, I've started a presubmit to see if the changes breaks anything.

AndrewKushnir
AndrewKushnir reviewed Oct 7, 2021
View reviewed changes
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

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

@petebacondarwin looks great 👍

packages/compiler/src/i18n/i18n_ast.ts Outdated Show resolved Hide resolved
packages/compiler/src/output/output_ast.ts Show resolved Hide resolved
packages/compiler/src/output/output_ast.ts Show resolved Hide resolved
packages/compiler/src/output/output_ast.ts Outdated Show resolved Hide resolved
packages/compiler/src/render3/view/i18n/meta.ts Outdated Show resolved Hide resolved
packages/localize/src/utils/src/messages.ts Outdated Show resolved Hide resolved
AndrewKushnir
AndrewKushnir approved these changes Oct 8, 2021
View reviewed changes
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

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

👍

packages/compiler/src/output/output_ast.ts Outdated Show resolved Hide resolved
packages/compiler/src/render3/view/i18n/meta.ts Outdated Show resolved Hide resolved
@AndrewKushnir
Copy link
Contributor

FYI, initial presubmit looks good, I will run a bit more tests in g3 and keep this thread updated.

@petebacondarwin
feat(localize): support "associated message ids" for placeholders
d372993 
When extracting i18n messages, the Angular compiler needs to split ICU expressions
into their own message.  Currently there is no guaranteed way to re-associate
the ICU message with the original message where the ICU was found.

This change adds support in the localize tooling so that associated ids can be
stored as metadata in the `$localize` tagged strings. These ids can then be
used in generated translation files to provide a link between the two messages.

The XLIFF 1.2 and 2.0 formats have been updated to render these relationships,
via the `xid` and `subFlows` attributes respectively.
@petebacondarwin
fixup! feat(localize): support "associated message ids" for placeholders
a25b3ff
@petebacondarwin
feat(compiler): reference ICU message IDs from their placeholders
d2a7dba 
When extracting i18n messages from templates, ICU messages are split out from the
message that contains them. This can make it difficult in the translation files to match up
the two messages, especially if the ICU is reused in multiple placeholders.

This commit builds on top of the previous one to expose the message ID of ICU messages
from the ICU placeholders as additional metadata in the `$localize` tagged strings.
Now the metablock following any placeholder can also contain the associated ID
delimited from the placeholder name by `@@`.

Fixes angular#17506
@petebacondarwin
fixup! feat(compiler): reference ICU message IDs from their placeholders
c251286
@petebacondarwin
fixup! feat(compiler): reference ICU message IDs from their placeholders
e76071f
@petebacondarwin
fixup! feat(compiler): reference ICU message IDs from their placeholders
df3dd11
@AndrewKushnir
Copy link
Contributor

@petebacondarwin presubmits for the changes in this PR are successful + extra testing went well, so this PR is good to go from g3 perspective.

@AndrewKushnir AndrewKushnir removed action: review The PR is still awaiting reviews from at least one requested reviewer action: presubmit The PR is in need of a google3 presubmit labels Oct 15, 2021
@petebacondarwin petebacondarwin added the action: merge The PR is ready for merge by the caretaker label Oct 15, 2021
@AndrewKushnir
Copy link
Contributor

This PR was merged into the repository by commit c85bcb0.

AndrewKushnir pushed a commit that referenced this pull request Oct 18, 2021
@petebacondarwin @AndrewKushnir
feat(compiler): reference ICU message IDs from their placeholders (#4…
c85bcb0 
…3534)

When extracting i18n messages from templates, ICU messages are split out from the
message that contains them. This can make it difficult in the translation files to match up
the two messages, especially if the ICU is reused in multiple placeholders.

This commit builds on top of the previous one to expose the message ID of ICU messages
from the ICU placeholders as additional metadata in the `$localize` tagged strings.
Now the metablock following any placeholder can also contain the associated ID
delimited from the placeholder name by `@@`.

Fixes #17506

PR Close #43534
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Nov 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@AndrewKushnir AndrewKushnir AndrewKushnir approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker area: compiler Issues related to `ngc`, Angular's template compiler area: i18n cla: yes target: minor This PR is targeted for the next minor release
Projects
None yet
Milestone
Backlog
Development

Successfully merging this pull request may close these issues.

Translate select and plural loses references in xliff, xliff2 and xmb files
2 participants
@petebacondarwin @AndrewKushnir