Closed Bug 1567070 Opened 5 years ago Closed 4 years ago

port [bug 1519923 - Migrate about:rights to Fluent] to thunderbird

Categories

(Thunderbird :: General, task, P3)

Product:
Thunderbird
Component:
General
Type:
task

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 78.0

People

(Reporter: mkmelin, Assigned: khushil324)

References

(Blocks 1 open bug)

Details

(Whiteboard: [tb-fluent])

Attachments

(2 files, 3 obsolete files)

Bug-1567070_migrate-about-rights-to-fluent.patch
30.04 KB, patch
mkmelin
: review-
Details | Diff | Splinter Review
Bug-1567070_migrate-about-rights-to-fluent.patch
27.35 KB, patch
Details | Diff | Splinter Review

+++ This bug was initially created as a clone of Bug #1519923 +++

Bug 1519923 converted toolkit about:rights to Fluent. The Thunderbird aboutRights.xhtml also needs to be migrated.

At the moment, if you open about:rights from the about dialog you'll see
JavaScript error: chrome://global/content/aboutRights.js, line 17: TypeError: rightsIntro is null
... because the scripts expect fluent l10n in the page - https://searchfox.org/mozilla-central/source/toolkit/content/aboutRights-unbranded.xhtml

The (small) things done here is thus broken for Thunderbird: https://searchfox.org/mozilla-central/source/toolkit/content/aboutRights.js

Assignee: nobody → khushil324
Attachment #9088150 - Flags: review?(mkmelin+mozilla)
Status: NEW → ASSIGNED
Comment on attachment 9088150 [details] [diff] [review]
Bug-1567070_migrate-about-rights-to-fluent.patch

Review of attachment 9088150 [details] [diff] [review]:
-----------------------------------------------------------------

Please look at what they did in the firefox bug: https://hg.mozilla.org/mozilla-central/rev/75766cc3befd

What you've done now doesn't look correct. For instance, you should use something like
<li data-l10n-id="rights-intro-point-2">

... and not localize each part separately as e.g. rights-intro-point2a

Also, importantly, you need to add the fluent migration recipe like they did in the ff bug. See https://firefox-source-docs.mozilla.org/intl/l10n/l10n/fluent_migrations.html
Attachment #9088150 - Flags: review?(mkmelin+mozilla) → review-
Attachment #9088150 - Attachment is obsolete: true
Attachment #9089850 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9089850 [details] [diff] [review]
Bug-1567070_migrate-about-rights-to-fluent.patch

Review of attachment 9089850 [details] [diff] [review]:
-----------------------------------------------------------------

Please name the migration recipe file like bug_1567070_aboutRights.py

::: mail/base/content/aboutRights.xhtml
@@ +52,5 @@
>  # Terms only apply to official builds, unbranded builds get a placeholder.
> +    <li data-l10n-id="rights-webservices-term1"></li>
> +    <li data-l10n-id="rights-webservices-term2"></li>
> +    <li><strong data-l10n-id="rights-webservices-term3"></strong></li>
> +    <li><strong data-l10n-id="rights-webservices-term4"></strong></li>

looks like ff  put the <strong> in the translated content. 
Let's sync with them, also so the ids are the same (term-4 vs term4 etc.). This is a semi forked page so we want it as close as possible, for easy updates.
Attachment #9089850 - Flags: review?(mkmelin+mozilla)
Attachment #9089850 - Attachment is obsolete: true
Attachment #9090452 - Flags: review?(mkmelin+mozilla)
Attachment #9090452 - Attachment is obsolete: true
Attachment #9090452 - Flags: review?(mkmelin+mozilla)
Attachment #9093235 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9093235 [details] [diff] [review]
Bug-1567070_migrate-about-rights-to-fluent.patch

Review of attachment 9093235 [details] [diff] [review]:
-----------------------------------------------------------------

We have to discuss where to put these...

::: mail/base/content/aboutRights.xhtml
@@ +18,3 @@
>  </head>
>  
> +<body id="your-rights" data-l10n-id="rights-locale-direction" class="aboutPageWideContainer">

Firefox doesn't set a dir, so we can skip this as well

::: mail/locales/en-US/messenger/aboutRights.ftl
@@ +3,5 @@
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +# rights-locale-direction instead of the usual local-dir entity, so RTL can skip translating page.
> +rights-locale-direction =
> +    .dir  = ltr

should be just one space

@@ +14,5 @@
> +# split into chunks for text before the link, the link text, and the text
> +# after the link. If a localized grammar doesn't need the before or after
> +# chunk, it can be left blank. Also note the leading/trailing whitespace
> +# in strings here, which is deliberate for formatting around the embedded
> +# links.

Remove this note

@@ +24,5 @@
> +# point 3 text for official branded builds.
> +rights-intro-point-3 = Privacy policies for { -vendor-short-name }'s products may be found <a data-l10n-name="mozilla-privacy-policy-link">here</a>.
> +
> +# point 3 text for unbranded builds.
> +rights-intro-point-3-unbranded  = Any applicable privacy policies for this product should be listed here.

there's a double space before =

@@ +38,5 @@
> +# Note that this paragraph references a couple of entities from
> +# preferences/security.dtd, so that we can refer to text the user sees in
> +# the UI, without this page being forgotten every time those strings are
> +# updated.
> +# intro paragraph for branded builds.

Please remove this note, which would be wrong

@@ +39,5 @@
> +# preferences/security.dtd, so that we can refer to text the user sees in
> +# the UI, without this page being forgotten every time those strings are
> +# updated.
> +# intro paragraph for branded builds.
> +rights-webservices = { -brand-full-name } offers optional services (“Services”), such as the add-on update service, that are available for your use with this binary version of { -brand-short-name } as described below. If you do not want to use the Services or the terms below are unacceptable, you may disable the add-on update service by opening the application preferences, selecting the <code data-l10n-name="rights-webservices-advanced">Advanced</code> section, followed by the <code data-l10n-name="rights-webservices-update">Update</code> tab, and unchecking the options for “Automatically check for updates to”;.

We don't have that option anymore. Looks like we should just use the toolkit version: https://searchfox.org/mozilla-central/rev/ebe492edacc75bb122a2b380e4cafcca3470864c/toolkit/locales/en-US/toolkit/about/aboutRights.ftl#50

@@ +50,5 @@
> +
> +# points 1-7 text for branded builds.
> +rights-webservices-term-1 = { -vendor-short-name } may discontinue or change the Services at its discretion.
> +rights-webservices-term-2 = You are welcome to use these Services with the accompanying version of { -brand-short-name }, and you have all the rights necessary to do so.  { -vendor-short-name } and its licensors reserve all other rights in the Services.  These terms are not intended to limit any rights granted under open source licenses applicable to { -brand-short-name } and to corresponding source code versions of { -brand-short-name }.
> +rights-webservices-term-3 = <strong>Services are provided “as-is.”;  { -vendor-short-name }, its contributors, licensors, and distributors, disclaim all warranties, whether express or implied, including without limitation, warranties that the Services are merchantable and fit for your particular purposes. You bear the entire risk as to selecting the Services for your purposes and as to the quality and performance of the Services. Some jurisdictions do not allow the exclusion or limitation of implied warranties, so this disclaimer may not apply to you.</strong>

You've missed the "The" here

@@ +51,5 @@
> +# points 1-7 text for branded builds.
> +rights-webservices-term-1 = { -vendor-short-name } may discontinue or change the Services at its discretion.
> +rights-webservices-term-2 = You are welcome to use these Services with the accompanying version of { -brand-short-name }, and you have all the rights necessary to do so.  { -vendor-short-name } and its licensors reserve all other rights in the Services.  These terms are not intended to limit any rights granted under open source licenses applicable to { -brand-short-name } and to corresponding source code versions of { -brand-short-name }.
> +rights-webservices-term-3 = <strong>Services are provided “as-is.”;  { -vendor-short-name }, its contributors, licensors, and distributors, disclaim all warranties, whether express or implied, including without limitation, warranties that the Services are merchantable and fit for your particular purposes. You bear the entire risk as to selecting the Services for your purposes and as to the quality and performance of the Services. Some jurisdictions do not allow the exclusion or limitation of implied warranties, so this disclaimer may not apply to you.</strong>
> +rights-webservices-term-4 = <strong>Except as required by law, { -vendor-short-name }, its contributors, licensors, and distributors will not be liable for any indirect, special, incidental, consequential, punitive, or exemplary damages arising out of or in any way relating to the use of { -brand-short-name } and the Services.  The collective liability under these terms will not exceed $500 (five hundred dollars). Some jurisdictions do not allow the exclusion or limitation of certain damages, so this exclusion and limitation may not apply to you.</strong>

rights-webservices-term-4 in your version is rights-webservices-term-5 in mozilla. Could be better to have the same number for the same content, and skip one number when not needed

rights-webservices-term-5 would be rights-webservices-term-6 
rights-webservices-term-6 would be rights-webservices-term-7

::: python/l10n/fluent_migrations/Bug_1567070_aboutRights.py
@@ +11,5 @@
> +from fluent.migrate.helpers import transforms_from
> +from fluent.migrate.helpers import TERM_REFERENCE
> +
> +def migrate(ctx):
> +    """Bug Bug 1567070 - Migrate about:rights to Fluent in Thunderbird, part {index}."""

Only one bug
Attachment #9093235 - Flags: review?(mkmelin+mozilla) → review-

To check this I did this:

@ https://searchfox.org/mozilla-central/source/python/l10n/test_fluent_migrations/fmt.py#63

-   l10n_toml = mozpath.join(cmd.topsrcdir, 'browser', 'locales', 'l10n.toml')
+   l10n_toml = mozpath.join(cmd.topsrcdir, 'comm', 'mail', 'locales', 'l10n.toml')

@ https://searchfox.org/comm-central/rev/dbc116c2b1bcb8d1c50af5d45b750af2a174a9e3/mail/locales/l10n.toml#75

-    mozilla = "mozilla"
+    mozilla = ".."

Moved the new recipe over to the m-c location (python/l10n/fluent_migrations/Bug_1567070_aboutRights.py) instead, otherwise it doesn't work.

So what do we want to do with recipes? It doesn't seem ideal to put them in m-c. For the fmt.py change, maybe we can add a command line argument to override which l10n.toml file to use.

As for the l10n.toml change needed, so this is still using the old backward way where mozilla was a subdir under comm-central and not the other way around?

Run
./mach fluent-migration-test python/l10n/fluent_migrations/Bug_1567070_aboutRights.py

That does show some problems (not sure how it was smart enough to find them!).

For reference, in normal cases (this bug isn't super normal) I assume the way to create recipes is like so:

pip install lxml fluent

python python/l10n/convert_xul_to_fluent/convert.py 1567070 comm/mail/base/content/aboutRights.xhtml comm/mail/locales/en-US/messenger/aboutRights.ftl ./ comm/mail/locales/en-US/chrome/messenger/aboutRights.dtd "Migrate about:rights to Fluent"

Flod, any input on direction here?

Flags: needinfo?(francesco.lodolo)

I won't be back online until Monday, so leaving the NI open. In the meantime, pointing out that you can also run the migrations without mach (that's how I run them), although it requires to install Python packages on your own.
https://github.com/flodolo/fluent-migrations

Having said that, there are non technical questions to answer here. Who would run these migrations? It requires to:

  1. Generate the cross-channel quarantine repository.
  2. Stop sync in Pontoon for all projects relying on l10n-central.
  3. Push the quarantine repository to gecko-strings.
  4. Run the migration on all l10n-central repositories.
  5. Re-enable sync for Pontoon.

Overall it takes almost 2 hours for every batch of migrations, not including reviewing migrations (not sure I'd like to run something that I didn't review). That's a non trivial amount of work and, to be honest, not something I'd like to be responsible for. At the same time, it also requires a level of access to Pontoon that I don't think we're comfortable in providing.

The conversation would be completely different with Thunderbird having its own repository. We would only need to stop the sync for Thunderbird, and it wouldn't affect any other project.

From a very quick look at the patch:

  • The migration filename should be lowercase (bug_1567070_aboutrights.py). You're also not migrating rights-intro-point-5.
  • rights-webservices has an extra ; at the end in your FTL. Also not sure why you're moving the RTL/LTR direction over to Fluent.

As for comment 8:

  • Pike can answer questions around changes to fmt.py to better support Thunderbird.
  • Where to store these recipes depends on answer to the question above: who's on point to verify and run these recipes on all l10n repositories?

If it's unclear from my previous comment, all those steps are needed, and in that precise order. If you run the migration without stopping sync, you clog sync for all projects. If you do it without pushing changes to gecko-strings, Pontoon will ignore the new strings in the l10n repositories for the time being.

Flags: needinfo?(francesco.lodolo)

Random notes:

The browser could probably be replaced with cmd.substs['MOZ_BUILD_APP'].
Changing the default for mozilla in mail/locales/l10n.toml should be OK. There's a couple in-tree places that overwrite this, which you want to look at.
The recipe tester assumes the module in https://searchfox.org/mozilla-central/rev/d7537a9cd2974efa45141d974e5fc7c727f1a78f/python/l10n/test_fluent_migrations/fmt.py#105. That could also be taken from the file path. Then you'd need to make sure that the in-comm location for the migrations is added to the build virtualenv, so that mach can find it.

See Also: → 1573678

With this patch and the patches in bug 1621633, I now get a reasonable result running
./mach fluent-migration-test comm/python/l10n/tb_fluent_migrations/bug_1567070_aboutrights.py

I'm migrating it from the firefox copy instead. We only had small changes.

I don't quite understand why the path should start with from_path="toolkit/toolkit/......" but it works.

Comment on attachment 9133838 [details] [diff] [review]
Bug-1567070_migrate-about-rights-to-fluent.patch

Review of attachment 9133838 [details] [diff] [review]:
-----------------------------------------------------------------

::: python/l10n/tb_fluent_migrations/bug_1567070_aboutrights.py
@@ +68,5 @@
> +""", from_path="toolkit/toolkit/about/aboutRights.ftl"),
> +        )
> +
> +"""
> +rights-intro-point-2 = ### this needs s/Firefox/Thunderbird

There's no easy way to interpolate strings when migrating FTL2FTL., right now you'd need to implement things on your own
https://bugzilla.mozilla.org/show_bug.cgi?id=1453557#c18

Thanks. We'd not migrate this point when that's the case. (Well, the patch doesn't it just adds that comment as a note on differences to firefox.)

Whiteboard: [tb-fluent]
Blocks: 1637916

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/894c67ced89c
Migrate about:rights to Fluent. r=mkmelin

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 78.0
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: