1615501 - Convert the Preferences tabs to Fluent

Status: RESOLVED FIXED

(Thunderbird :: Preferences, enhancement, P1)

Description

[Alessandro Castellani [:aleca]]

In order to implement the Preferences Search, and clean up a lot our current file structure, we should follow the current implementation of Firefox, which uses a standard html window for the main file, and html:template elements for the various screens instead of the preftab we're currently using.

Alongside, I'm gonna move the strings left behind in the preferences.properties file to the newly generated preferences.ftl in order to kick-start the Fluent transition of the Preferences panel.

For reference
FF Preferences: https://searchfox.org/mozilla-central/rev/a4be2fbe9bd4f405c91cc16e4e3a80400f5a9301/browser/components/preferences/in-content/preferences.xhtml
FF main template (our general preftab): https://searchfox.org/mozilla-central/rev/a4be2fbe9bd4f405c91cc16e4e3a80400f5a9301/browser/components/preferences/in-content/main.inc.xhtml

Comment 1

[Alessandro Castellani [:aleca]]

Attachment: 1615501-preferences-tab-fluent.diff

This is the first of many patches necessary to migrate all the preferences strings to their fluent counterpart.
I'm trying to keep these short and easy to review since we have a lot of strings in the preferences tab, spread across multiple properties and dtd files.

Comment 2

[Richard Marti (:Paenglab)]

Comment on attachment 9133321 [details] [diff] [review]
1615501-preferences-tab-fluent.diff

You don't add the changes directly to the en-US ftl file to make the patch working without running the python script?

Comment 3

[Alessandro Castellani [:aleca]]

(In reply to Richard Marti (:Paenglab) from comment #2)

You don't add the changes directly to the en-US ftl file to make the patch working without running the python script?

Ah, damn, I forgot to qrefresh those edits.
Thanks for the heads up.

Comment 4

[Alessandro Castellani [:aleca]]

Attachment: 1615501-preferences-tab-fluent.diff

Comment 5

[Alessandro Castellani [:aleca]]

Attachment: 1615501-preferences-tab-fluent.diff

I'm removing the review request from this as I will include also the preferences.properties inside this first patch.
For now, I'm requesting a feedback to confirm that what I'm doing is good.

I update the preferences-title to use the PLATFORM() variable, and removed the GNOME variation which was using the brand name alongside the Preferences title. Any reason why we had that?

If we need to keep that, I will use the same approach I found on m-c, as it seems that it's not possible to use the PLATFORM() variable as a Fluent VARIABLE_REFERENCE.
https://firefox-source-docs.mozilla.org/l10n/migrations/legacy.html#explicit-variants

m-c solved that problem in this way: https://searchfox.org/mozilla-central/rev/f36cb2af46edd2659f446b7acdb2154e230ee445/python/l10n/fluent_migrations/bug_1613801_window_title_mac.py#26

Comment 6

[Richard Marti (:Paenglab)]

Comment on attachment 9133328 [details] [diff] [review]
1615501-preferences-tab-fluent.diff

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

For me it's okay to use only two versions of the titlte. The brand name isn't needed in the tab.

Your patch doesn't work correctly for me:
- The tab title is empty because of a typo.
- Also the categories have no text. In the DevTools I see: `label=""`. I tried different things with no luck. Maybe this needs first a conversion from <prefpane> to <richlistitem> like FX has.

::: mail/locales/en-US/messenger/preferences/preferences.ftl
@@ +4,5 @@
>  
> +close-button =
> +    .aria-label = Close
> +
> +preference-title =

Should be preferences-title. Or you remove the `s` in aboutPreferences.xhtml.

Comment 7

[Magnus Melin [:mkmelin]]

Comment on attachment 9133328 [details] [diff] [review]
1615501-preferences-tab-fluent.diff

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

Yep use what m-c uses

::: mail/locales/en-US/messenger/preferences/preferences.ftl
@@ +4,5 @@
>  
> +close-button =
> +    .aria-label = Close
> +
> +preference-title =

yeah it's preferences with an s

@@ +34,5 @@
>  
>  general-indexing-label = Indexing
>  
> +pane-composition =
> +    .label = Composition.title

Composition

Comment 8

[Alessandro Castellani [:aleca]]

Thanks both for the feedback.

• The tab title is empty because of a typo.

Fixed, thanks.

• Also the categories have no text. In the DevTools I see: label="". I tried different things with no luck. Maybe this needs first a conversion from <prefpane> to <richlistitem> like FX has.

That doesn't work because we're generating those nav elements in JS and trying to fetch the label from the pref panel, which for fluent results in a weird mix between sync and async string loading.

I will convert the menu to use statically written richlistbox like m-c.

Comment 9

[Alessandro Castellani [:aleca]]

Attachment: 1615501-preferences-tab-fluent.diff

I tried to keep the changes as small as possible in order to not make this patch grow too much.
This is only the first part of many many more that needs to happen.

In this patch I'm only tackling:

• Fluent migration for the Preferences tab container (no internal panes have been changed).
• Convert the radiogroup to richlistbox.

Comment 10

[Alessandro Castellani [:aleca]]

Attachment: 1615501-preferences-tab-fluent.diff

Missed the fluent migration file, sorry.

Also, a known issue is that the preferences panel seems to not remember the last opened, even if the method works and it's actually stored.
Any suggestion?

Comment 11

[Richard Marti (:Paenglab)]

Comment on attachment 9133745 [details] [diff] [review]
1615501-preferences-tab-fluent.diff

Looks good. The category labels are too small, a 
````
#categories {
  font-size: 1.25em;
}
````
fixes this for me on Windows. Linux and Mac need other sizes, see https://searchfox.org/comm-central/search?q=body+%7B&case=false&regexp=false&path=preferences.

Sorry, I don't know what is needed to remember the last selected pane.

Comment 12

[Alessandro Castellani [:aleca]]

(In reply to Richard Marti (:Paenglab) from comment #11)

Looks good. The category labels are too small...

Mh, I removed any font size variation to inherit what's coming from Firefox, which looks good.
Something is interfering with the overriding style.

Comment 13

[Alessandro Castellani [:aleca]]

Attachment: 1615501-preferences-tab-fluent-PART1.diff

Patch unbitrotted and updated.
The font size have been fixed and now the panel remembers the last viewed location.

I refactored a little bit more the JS part to follow closely what m-c did and in order to properly prepare the files for the Search field implementation.
The most substantial change was the removal of the event dispatching to initialize the various panels.

Magnus, would you be able to give it a review? Even if it doesn't land due to the fluent migration part, I'd like to have an approved base before continuing converting the other parts.

Richard, I update the CSS and removed the radio related styling. Can you be sure I haven't caused any UI regression with the richlistitem implementation?

Cheers

Comment 14

[Richard Marti (:Paenglab)]

Comment on attachment 9139073 [details] [diff] [review]
1615501-preferences-tab-fluent-PART1.diff

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

Looks almost ready.
The categories text is too big. You have to calculate that https://searchfox.org/mozilla-central/source/toolkit/themes/shared/in-content/common.inc.css#734 increases the size. At least on Windows it works when you use for `#categories` the same font-size as `#MailPreferences prefpane`.

::: mail/locales/en-US/chrome/messenger/preferences/preferences.dtd
@@ +5,1 @@
>  <!ENTITY  preferencesCloseButton.label     "Close">

Is this file still needed? Haven't you converted all strings to Fluent?

::: mail/themes/shared/mail/incontentprefs/preferences.css
@@ +433,1 @@
>    padding: 0.3em;

This makes the selector specificity higher than https://searchfox.org/comm-central/source/mail/themes/shared/mail/incontentprefs/applications.css#12 and adds the padding. Please add the `#preferencesContainer` to the `#handlersView > richlistitem` to increase its specificity.

@@ +447,5 @@
>  }
>  
> +#preferencesContainer richlistbox:focus > richlistitem[selected="true"] {
> +  background-color: Highlight;
> +  color: HighlightText !important;

Why have you removed the variables? With this, for example on plain Ubuntu the item would be orange instead of the in-content blue.

Comment 15

[Alessandro Castellani [:aleca]]

(In reply to Richard Marti (:Paenglab) from comment #14)

Looks almost ready.
The categories text is too big. You have to calculate that
https://searchfox.org/mozilla-central/source/toolkit/themes/shared/in-
content/common.inc.css#734 increases the size. At least on Windows it works
when you use for #categories the same font-size as #MailPreferences prefpane.

I updated everything, but I wonder why we're doing this.
It feels a bit weird to slightly tweak the font size of the container to bounce off the font size of the label.
If we really want to edit those sizes, shouldn't we update directly the .category-name font size?

::: mail/locales/en-US/chrome/messenger/preferences/preferences.dtd
Is this file still needed? Haven't you converted all strings to Fluent?

It's not used anymore, but I haven't removed it because it's necessary for the fluent migration recipe.
I don't know if by deleting that file the recipe might fail.

+#preferencesContainer richlistbox:focus > richlistitem[selected="true"] {

• background-color: Highlight;
• color: HighlightText !important;

Why have you removed the variables? With this, for example on plain Ubuntu
the item would be orange instead of the in-content blue.

Ah, that's a mistake, thanks for noticing.

Comment 16

[Alessandro Castellani [:aleca]]

Attachment: 1615501-preferences-tab-fluent-PART1.diff

Updated with the requested changes.

Comment 17

[Alessandro Castellani [:aleca]]

Attachment: 1615501-preferences-tab-fluent-PART1.diff

Itty bitty CSS fix.

Comment 18

[Richard Marti (:Paenglab)]

Comment on attachment 9139249 [details] [diff] [review]
1615501-preferences-tab-fluent-PART1.diff

Looks good now, thanks!

Comment 19

[Alessandro Castellani [:aleca]]

Attachment: 1615501-preferences-tab-fluent-PART1.diff

Patch updated to handle the case when the Calendar is disabled and we don't want to show those prefs.
Also, I updated the condition when restoring the previous tab to fallback to the General pane in case that tab doesn't exist anymore.

Comment 20

[Magnus Melin [:mkmelin]]

Comment on attachment 9139302 [details] [diff] [review]
1615501-preferences-tab-fluent-PART1.diff

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

I think it looks ok, but bitrotted a bunch.

::: mail/components/preferences/preferences.xhtml
@@ +106,5 @@
> +                        value="paneGeneral"
> +                        data-l10n-id="category-general"
> +                        align="center">
> +            <image class="category-icon"/>
> +            <label class="category-name" flex="1" data-l10n-id="pane-general-title"></label>

for xul:labels, just use a self-closing tag

::: python/l10n/tb_fluent_migrations/bug_1615501_preferences.py
@@ +24,5 @@
> +pane-compose-title = { COPY(from_path, "paneComposition.title") }
> +pane-privacy-title = { COPY(from_path, "panePrivacySecurity.title") }
> +pane-chat-title = { COPY(from_path, "paneChat.title") }
> +addons-button = { COPY(from_path, "addonsButton.label") }
> +""", from_path="mail/chrome/messenger/preferences/preferences.dtd"

you should remove the strings you're migrating from the preferences.dtd file

Comment 21

[Alessandro Castellani [:aleca]]

Attachment: 1615501-preferences-tab-fluent-PART1.diff

I deleted the preferences.dtd file as I'm translating all the strings and it wasn't used anywhere else.

Comment 22

[Alessandro Castellani [:aleca]]

Comment on attachment 9141094 [details] [diff] [review]
1615501-preferences-tab-fluent-PART1.diff

This has bitrotted after bug 1631394 landed.
Updating this patch.

Comment 23

[Alessandro Castellani [:aleca]]

Attachment: 1615501-preferences-tab-fluent.diff

Patch unbitrotted.

Comment 24

[Magnus Melin [:mkmelin]]

So this is what I get (with the c-c patch from bug 1621633)

./mach fluent-migration-test comm/python/l10n/tb_fluent_migrations/bug_1615501_preferences.py
 0:00.36 hg pull -u
 0:02.65 /home/magnus/Code/tb/mozilla/obj-x86_64-pc-linux-gnu/_virtualenvs/init_py3/bin/python -m fluent.migrate.tool --lang en-US --reference-dir /home/magnus/Code/tb/mozilla/obj-x86_64-pc-linux-gnu/python/l10n/bug_1615501_preferences/reference --localization-dir /home/magnus/Code/tb/mozilla/obj-x86_64-pc-linux-gnu/python/l10n/bug_1615501_preferences/en-US --dry-run tb_fluent_migrations.bug_1615501_preferences

Running migration tb_fluent_migrations.bug_1615501_preferences for en-US
  Writing to /home/magnus/Code/tb/mozilla/obj-x86_64-pc-linux-gnu/python/l10n/bug_1615501_preferences/en-US/mail/messenger/preferences/preferences.ftl
  Committing changeset: Bug 1615501 - Fluent migration recipe for Preferences Tab.
  Writing to /home/magnus/Code/tb/mozilla/obj-x86_64-pc-linux-gnu/python/l10n/bug_1615501_preferences/en-US/mail/messenger/preferences/preferences.ftl
  Committing changeset: Bug 1615501 - Fluent migration recipe for Preferences Tab.
  Writing to /home/magnus/Code/tb/mozilla/obj-x86_64-pc-linux-gnu/python/l10n/bug_1615501_preferences/en-US/mail/messenger/preferences/preferences.ftl
  Committing changeset: Bug 1615501 - Fluent migration recipe for Preferences Tab.
 0:02.97 /home/magnus/Code/tb/mozilla/obj-x86_64-pc-linux-gnu/_virtualenvs/init_py3/bin/python -m fluent.migrate.tool --lang en-US --reference-dir /home/magnus/Code/tb/mozilla/obj-x86_64-pc-linux-gnu/python/l10n/bug_1615501_preferences/reference --localization-dir /home/magnus/Code/tb/mozilla/obj-x86_64-pc-linux-gnu/python/l10n/bug_1615501_preferences/en-US tb_fluent_migrations.bug_1615501_preferences

Running migration tb_fluent_migrations.bug_1615501_preferences for en-US
  Writing to /home/magnus/Code/tb/mozilla/obj-x86_64-pc-linux-gnu/python/l10n/bug_1615501_preferences/en-US/mail/messenger/preferences/preferences.ftl
  Committing changeset: Bug 1615501 - Fluent migration recipe for Preferences Tab.
  Writing to /home/magnus/Code/tb/mozilla/obj-x86_64-pc-linux-gnu/python/l10n/bug_1615501_preferences/en-US/mail/messenger/preferences/preferences.ftl
  Committing changeset: Bug 1615501 - Fluent migration recipe for Preferences Tab.
  Writing to /home/magnus/Code/tb/mozilla/obj-x86_64-pc-linux-gnu/python/l10n/bug_1615501_preferences/en-US/mail/messenger/preferences/preferences.ftl
  Committing changeset: Bug 1615501 - Fluent migration recipe for Preferences Tab.
--- /home/magnus/Code/tb/mozilla/obj-x86_64-pc-linux-gnu/python/l10n/bug_1615501_preferences/reference/mail/messenger/preferences/preferences.ftl
+++ /home/magnus/Code/tb/mozilla/obj-x86_64-pc-linux-gnu/python/l10n/bug_1615501_preferences/en-US/mail/messenger/preferences/preferences.ftl
@@ -14,17 +14,9 @@
 category-general =
     .tooltiptext = { pane-general-title }
 pane-compose-title = Composition
-category-compose =
-    .tooltiptext = { pane-compose-title }
 pane-privacy-title = Privacy & Security
-category-privacy =
-    .tooltiptext = { pane-privacy-title }
 pane-chat-title = Chat
-category-chat =
-    .tooltiptext = { pane-chat-title }
 pane-calendar-title = Calendar
-category-calendar =
-    .tooltiptext = { pane-calendar-title }
 general-language-and-appearance-header = Language & Appearance
 general-incoming-mail-header = Incoming Mails
 general-files-and-attachment-header = Files & Attachments
 0:03.58 Commit messages should have "part {index}" for comm/python/l10n/tb_fluent_migrations/bug_1615501_preferences.py

Comment 25

[Alessandro Castellani [:aleca]]

Attachment: 1615501-preferences-tab-fluent.diff

Updated and fixed.
Now the fluent migration test runs properly.
It seems we can't inherit strings in fluent from another ID when doing a migration, that's why those strings were getting removed.

Comment 26

[Alessandro Castellani [:aleca]]

Attachment: 1615501-preferences-tab-fluent-PART2.diff

Second part to load on top of the first patch.
This one takes care of the general.dtd conversion.

Comment 27

[Magnus Melin [:mkmelin]]

Comment on attachment 9142581 [details] [diff] [review]
1615501-preferences-tab-fluent.diff

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

I think this is correct.

Comment 28

[Alessandro Castellani [:aleca]]

Thanks for the r+.
I'm not gonna mark this for check-in until bug 1621633 lands.
I will keep uploading follow up parts for all the other preferences sections, and continue the conversion to html:template

Comment 29

[Magnus Melin [:mkmelin]]

Comment on attachment 9142887 [details] [diff] [review]
1615501-preferences-tab-fluent-PART2.diff

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

::: mail/components/preferences/general.inc.xhtml
@@ +92,5 @@
>                  preference="pref.general.disable_button.default_mail"/>
>        </hbox>
>  #ifdef XP_WIN
>        <hbox align="start">
> +        <checkbox data-l10n-id="general-minimize-label"

I would try to keep the ids long enough to be understandable. Not sure we want to have the section prefixes through. An id like "minimize-to-tray-label" would be much more understandable

@@ +267,5 @@
>        <html:h1 data-l10n-id="general-incoming-mail-header"/>
>      </hbox>
>  
>      <html:fieldset data-category="paneGeneral">
> +      <html:legend data-l10n-id="general-new-message"></html:legend>

same here, maybe new-message-arrival

Comment 30

[Alessandro Castellani [:aleca]]

Attachment: 1615501-preferences-tab-fluent-PART2.diff

Patch updated by removing all those extra general- prefixes and by making the fluent IDs a bit more clear.

Comment 31

[Alessandro Castellani [:aleca]]

We currently have a lot of small DTD files for all those dialogs and separated sections we had in the previous iteration of the Preferences area.
Files like advanced.dtd, security.dtd, privacy.dtd, etc, all now live inline the main 4 sections.
I will be removing those files and migrate those strings into the main FTL files of those parents sections, like General, Composition, etc.

Does that sound good?

Comment 32

[Magnus Melin [:mkmelin]]

Sound good!

Comment 33

[Alessandro Castellani [:aleca]]

Attachment: 1615501-preferences-tab-fluent-PART3.diff

PART3!
This converts all the strings from the advanced.dtd file used in the General section.
I left behind a bunch of strings belonging to the Privacy section, and the updateApp.version.pre
updateApp.version.post which need to be reworked in JS with a sync method.

Running ./mach fluent-migration-test will mark all the current strings in the general.ftl for removal, which is normal since those are migrated in part 2, so are alien entities for part 3.

Comment 34

[Magnus Melin [:mkmelin]]

Comment on attachment 9143867 [details] [diff] [review]
1615501-preferences-tab-fluent-PART3.diff

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

I think it's correct. Running the test is a bit confusing when earlier parts aren't committed.

::: mail/locales/en-US/messenger/preferences/general.ftl
@@ +84,5 @@
> +
> +mbox-store-label = File per folder (mbox)
> +maildir-store-label = File per message (maildir)
> +
> +

nit: extra space here

Comment 35

[Alessandro Castellani [:aleca]]

Attachment: 1615501-preferences-tab-fluent-PART3.diff

nit fixed.

Comment 36

[Alessandro Castellani [:aleca]]

Attachment: 1615501-preferences-tab-fluent.diff

I merged all the previous patches together since we're affecting the same General pane with many strings coming from different files.
Now it's straightfoward to run the fluent migration test.

I'm getting a warning where it suggests to remove the master-password-os-auth-dialog-message-win string, but that's strange because I haven't touched that.
Is it suggesting that because we're not using that string anywhere?

Following up with other patches to handle each section separately without migration conflicts.

Comment 37

[Richard Marti (:Paenglab)]

(In reply to Alessandro Castellani (:aleca) from comment #36)

I'm getting a warning where it suggests to remove the master-password-os-auth-dialog-message-win string, but that's strange because I haven't touched that.
Is it suggesting that because we're not using that string anywhere?

We're using it here: https://searchfox.org/comm-central/rev/edbbc461dc1489444ca7ab72fe5da32c826b28fe/mail/components/preferences/privacy.js#339 it's very newly introduced by bug 1637207.

Comment 38

[Alessandro Castellani [:aleca]]

I wonder why the fluent migration test flags it to be removed.

Comment 39

[Alessandro Castellani [:aleca]]

Attachment: 1615501-preferences-tab-fluent-PART2.diff

This takes care of converting the Compose section of the preferences, and removes the compose.dtd and advanced.dtd files.

The migration test is flagging language-popup-label to be removed, which I don't understand why.
Any suggestion?

Asking a UI review to Richard to be sure no strings are missing.

Comment 40

[Alessandro Castellani [:aleca]]

Attachment: 1615501-preferences-tab-fluent.diff

I found some missing labels while testing the first patch, and I also fixed the migration issue I was having.

Comment 41

[Richard Marti (:Paenglab)]

Comment on attachment 9149570 [details] [diff] [review]
1615501-preferences-tab-fluent-PART2.diff

Looks good. I found no missing label.

Comment 42

[Richard Marti (:Paenglab)]

Comment on attachment 9149572 [details] [diff] [review]
1615501-preferences-tab-fluent.diff

Looks good too.

Comment 43

[Alessandro Castellani [:aleca]]

Attachment: 1615501-preferences-tab-fluent-PART3.diff

This one takes care of the Privacy page.

Comment 44

[Richard Marti (:Paenglab)]

Comment on attachment 9149995 [details] [diff] [review]
1615501-preferences-tab-fluent-PART3.diff

No missing string found in the pane.

Comment 45

[Alessandro Castellani [:aleca]]

Awesome, thank you both for the reviews!
I have other 2 patches to complete the Fluent migration for all the DTD files.
Then another patch to handle all the properties and JS async related strings.
An one final to convert everything into html:template

I saw that Lasana will be in charge of the Fluent migrations. Do we have a timeline for when we might start landing these r+ patches and run the migration?

Comment 46

[Alessandro Castellani [:aleca]]

Attachment: 1615501-preferences-tab-fluent-PART4.diff

This one takes care of the Chat tab.

Comment 47

[Richard Marti (:Paenglab)]

Comment on attachment 9150294 [details] [diff] [review]
1615501-preferences-tab-fluent-PART4.diff

Compared it with a Daily and I see no change. :-)

Comment 48

[Francesco Lodolo [:flod]]

Comment on attachment 9149570 [details] [diff] [review]
1615501-preferences-tab-fluent-PART2.diff

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

::: mail/locales/en-US/messenger/preferences/compose.ftl
@@ +19,5 @@
> +auto-save-label =
> +    .label = Auto Save every
> +    .accesskey = A
> +
> +auto-save-end = minutes

All these concatenations should have comments that explain how the strings are used.

Comment 49

[Francesco Lodolo [:flod]]

Comment on attachment 9149572 [details] [diff] [review]
1615501-preferences-tab-fluent.diff

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

::: mail/locales/en-US/messenger/preferences/general.ftl
@@ +109,5 @@
> +    .accesskey = N
> +
> +# LOCALIZATION NOTE: This is the search engine name for all the different platforms.
> +# Platforms that don't support it should be left blank.
> +-search-engine-name =

Passing by comments: you shouldn't use terms for this, you can reference messages in other messages.

@@ +171,5 @@
> +
> +compact-folder-size =
> +    .value = MB in total
> +
> +# LOCALIZATION NOTE: The entities use-cache-before and use-cache-after appear on a single line in preferences as follows:

`# LOCALIZATION NOTE` should not be kept when migrating to Fluent.

Fluent supports section comments, and that's what you should be using when adding comments to more than one string, then reset at the end.
Right now, this is a stand-alone comment that nobody will see (because of the empty line after). Normally, without empty line, this would be attached to the following string.

@@ +262,5 @@
> +choose-folder-label =
> +    .label =
> +        { PLATFORM() ->
> +            [macos] Choose…
> +            *[other] Browse…

nit: parenthesis should be aligned in these.

Comment 50

[Alessandro Castellani [:aleca]]

Attachment: 1615501-preferences-tab-fluent.diff

Patch updated to address Francesco's comments.

Thank you so much for looking at these.
I'm going through the other patches to apply the same edits.
Please, don't hesitate to highlight mistakes or issues you find.

Comment 51

[Alessandro Castellani [:aleca]]

Attachment: 1615501-preferences-tab-fluent-PART2.diff

Patch updated to address Francesco's comment, and fixed a tiny migration issue due to a missed uppercase letter.

Comment 52

[Alessandro Castellani [:aleca]]

Attachment: 1615501-preferences-tab-fluent-PART4.diff

Updated notes.

Comment 53

[Alessandro Castellani [:aleca]]

Attachment: 1615501-preferences-tab-fluent-PART5.diff

I'm having a problem with the calendar migration.
It seems that the migration is not able to handle files inside the calendar/ folder.
Am I doing something wrong, or there's maybe something missing from bug 1621633?

Comment 54

[Magnus Melin [:mkmelin]]

Comment on attachment 9151205 [details] [diff] [review]
1615501-preferences-tab-fluent-PART5.diff

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

It's not finding calendar, since we're using MOZ_BUILD_APP to locate the l10n.toml and that's "comm/mail". If you change that https://searchfox.org/mozilla-central/rev/ea7f70dac1c5fd18400f6d2a92679777d4b21492/python/l10n/test_fluent_migrations/fmt.py#85 to be "comm/calendar" things work.

I'm not sure how to resolve that. mail/locales/l10n.toml references calendar/locales/l10n.toml - whatever that means...

Comment 55

[Alessandro Castellani [:aleca]]

Attachment: 1615501-preferences-tab-fluent-PART5.diff

This patch takes care of the Calendar section and its subdialog.

The 6th and (hopefully) last patch will take care of all the subdialogs and JS strings.

Comment 56

[Francesco Lodolo [:flod]]

Comment on attachment 9151188 [details] [diff] [review]
1615501-preferences-tab-fluent.diff

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

::: mail/locales/en-US/messenger/preferences/general.ftl
@@ +36,5 @@
> +play-sound-label =
> +    .label =
> +        { PLATFORM() ->
> +            [macos] Play the following sound file:
> +            *[other] Play a sound

nit: align the bracket (across the file)

@@ +171,5 @@
> +
> +compact-folder-size =
> +    .value = MB in total
> +
> +# Note: The entities use-cache-before and use-cache-after appear on a single

This should be a section comment, right now it applies only to the first string. Remember to reset it afterwards with `##`.

@@ +294,5 @@
> +mark-read-no-delay =
> +    .label = Immediately on display
> +    .accesskey = o
> +
> +# Note: This will concatenate to "After displaying for [___] seconds",

Same here, it should be a section comment.

Comment 57

[Francesco Lodolo [:flod]]

Comment on attachment 9151190 [details] [diff] [review]
1615501-preferences-tab-fluent-PART2.diff

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

::: mail/locales/en-US/messenger/preferences/compose.ftl
@@ +15,5 @@
> +extension-label =
> +    .label = add extension to file name
> +    .accesskey = e
> +
> +# Note: This will concatenate to "Auto Save every [___] minutes",

Use section comment

Comment 58

[Richard Marti (:Paenglab)]

Comment on attachment 9152593 [details] [diff] [review]
1615501-preferences-tab-fluent-PART5.diff

Looks good. The only difference I spotted was that the Categories "Remove" button is now "Delete".

Comment 59

[Francesco Lodolo [:flod]]

Comment on attachment 9151204 [details] [diff] [review]
1615501-preferences-tab-fluent-PART4.diff

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

::: mail/locales/en-US/messenger/preferences/chat.ftl
@@ +97,5 @@
> +variant-label =
> +    .value = Variant:
> +    .accesskey = V
> +
> +header-label =

IDs this generic are going to bite you at some point. For the future, you really want to namespace them, and avoid possible conflicts.

Comment 60

[Richard Marti (:Paenglab)]

Attachment: 1615501-preferences-tab-fluent-PART4.diff

Part 4 does no more apply (chat.inc.xhtml). I rebased it. Aleca, please check that still all is correct.

Comment 61

[Francesco Lodolo [:flod]]

I'm giving up on using Splinter, it doesn't allow me to comment on part 5…

As explained elsewhere, you have IDs that are way too generic, one is title
See https://firefox-source-docs.mozilla.org/l10n/fluent/review.html

Migration has an hard-coded accesskey for timezone-label

Comment 62

[Alessandro Castellani [:aleca]]

Thank you Francesco for the detailed review and help, and thanks Richard for the rebase of P4.
I'll fix everything.

Comment 63

[Alessandro Castellani [:aleca]]

(In reply to Richard Marti (:Paenglab) from comment #58)

Looks good. The only difference I spotted was that the Categories "Remove" button is now "Delete".

Yes, I changed it on purpose as we have the same identical structure for the Tags section with the same 3 buttons, Add, Edit, Delete.
But for the Categories we were using Remove instead of Delete, and duplicating the other identical strings.

Comment 64

[Alessandro Castellani [:aleca]]

Attachment: 1615501-preferences-tab-fluent.diff

Patch updated and folded all the parts into one to run a single migration.

I can't fold PART 5 as that one tackles the Calendar which is in a separated folder and I'm not able to run a migration test as per comment 54

Comment 65

[Alessandro Castellani [:aleca]]

Attachment: 1615501-preferences-tab-fluent-Calendar.diff

Calendar patch updated.

Comment 66

[Richard Marti (:Paenglab)]

Comment on attachment 9152832 [details] [diff] [review]
1615501-preferences-tab-fluent.diff

Looks good, found no difference to pre-patch.

Comment 67

[Alessandro Castellani [:aleca]]

Attachment: 1615501-preferences-tab-fluent-Subdialogs.diff

This patch takes care of almost all the subdialogs.
There are only 4 missing subdialogs, which I will tackle tomorrow as it's midnight and my brain is melting with all these strings.

I'll take care of the remaining properties as well.
Once that's done, I will fold this patch into the first one so we can deal everything with a single migration.

Comment 68

[Alessandro Castellani [:aleca]]

Attachment: 1615501-preferences-tab-fluent-Subdialogs.diff

All the subdialogs have been converted.
I didn't convert all the properties as a lot of those require much more time and changes to the JS.
Those strings are mostly marginal and and dynamically applied for specific scenarios, therefore are not directly relevant to the search.

I kept the patches separated to easily review them and test the migration.

Comment 69

[Francesco Lodolo [:flod]]

Comment on attachment 9152832 [details] [diff] [review]
1615501-preferences-tab-fluent.diff

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

I think you need to clean up the commit message for this patch?

Comment 70

[Francesco Lodolo [:flod]]

Clearly I don't know how to make splinter review anymore, or it's really bugged.

I want to give an f+, but all I get is

You must provide a reviewer for review requests.

Because the reviewer flag remains set to ?. And by setting the reviewer flag to empty, I removed the original one :-\

Comment 71

[Francesco Lodolo [:flod]]

(In reply to Alessandro Castellani (:aleca) from comment #67)

Created attachment 9152925 [details] [diff] [review]
1615501-preferences-tab-fluent-Subdialogs.diff

This patch takes care of almost all the subdialogs.
There are only 4 missing subdialogs, which I will tackle tomorrow as it's midnight and my brain is melting with all these strings.

This is still an issue in the migration

timezone-label =
    .value = { COPY(from_path, "pref.timezones.caption") }:
    .accesskey = T

Comment 72

[Magnus Melin [:mkmelin]]

Comment on attachment 9152834 [details] [diff] [review]
1615501-preferences-tab-fluent-Calendar.diff

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

::: calendar/locales/en-US/calendar/preferences.ftl
@@ +19,5 @@
> +    .label = Short: { $date }
> +
> +timezone-label =
> +    .value = Timezone:
> +    .accesskey = T

Will change this back to S. Though that's not so nice :(
But we can fix the accesskeys later if we want.

@@ +132,5 @@
> +
> +event-task-legend = Events and Tasks
> +
> +default-length-label =
> +    .value = Default Event and Task Length

this one too should have a colon - will add.

::: python/l10n/tb_fluent_migrations/bug_1615501_preferences_calendar.py
@@ +40,5 @@
> +    .label = { COPY(from_path, "pref.dateformat.short") }: { $date }
> +
> +timezone-label =
> +    .value = { COPY(from_path, "pref.timezones.caption") }:
> +    .accesskey = T

.accesskey = { COPY(from_path, "pref.timezones.accesskey") }

I'll fix it.

But I see what the thing was here. For en-US, we used S which is not in the label (so ugly). So the thought was to change to T. But that's taken for "Tue". (Not sure how reasonable it is to have access keys for each weekday!) 
AND for open in tab edit-intab-label.

Looking at it, S (which we used) is also many times duplicated :(
I'll just drop accesskey for now.

Comment 73

[Magnus Melin [:mkmelin]]

Comment on attachment 9152986 [details] [diff] [review]
1615501-preferences-tab-fluent-Subdialogs.diff

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

Good work! r=mkmelin

::: python/l10n/tb_fluent_migrations/bug_1615501_preferences_subdialogs.py
@@ +654,5 @@
> +    .label = { COPY(from_path, "editKeywordButton1.label") }
> +    .accesskey = { COPY(from_path, "editKeywordButton1.accesskey") }
> +
> +keyword-button-buttons =
> +    .label = { COPY(from_path, "removeKeywordButton.label") }

looks like this should be:
keyword-remove-buttons

I'll also make it -button (without s for these three)

Comment 74

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/8f5971bfa1c6
Migrate Preferences Tab l10n to Fluent. r=mkmelin, ui-r=Paenglab
https://hg.mozilla.org/comm-central/rev/34815bba0c60
migrate calendar preferences l10n to Fluent. r=mkmelin
https://hg.mozilla.org/comm-central/rev/56288941d618
migrate preferences subdialog ll0n to Fluent. r=mkmelin

Comment 75

[Alessandro Castellani [:aleca]]

Thanks for landing these, and apologies for the mistakes, my eyes started crossing a bit with all the strings.

Comment 76

[Francesco Lodolo [:flod]]

I completely missed that you're adding punctuation to existing strings while migrating them.

timezone-label =
    .value = { COPY(from_path, "pref.timezones.caption") }:

For the future, don't do that. Locales have different rules when it comes to punctuation, you're creating an inconsistency that is really hard to catch, given the string is migrated and automatically approved.

That should have been a new string, without migration. At this point I'll keep it as it, but please point them out in the message when the migration is done. French, at least, will need to fix them.

@Magnus
Are there other things that need to land for migrations, or that's all?

Comment 77

[Magnus Melin [:mkmelin]]

Basically the migrations are finished. But there are a few things left (working on it, will check back later today):

• I noticed "custom-sound-label" and friends are duplicated (due to no namespacing)
• Will land bug 1573678 (adds to one of the new fluent files)
• Will land bug 1640897 (removes the need for a few migrations, will remove them from the .py file)

Looks like bug 1642282 is also fallout from this but seems like test only failure. Still annoying of course.

Comment 78

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/47aa8304f805
fix fluent id duplications in calendar/. r=me
https://hg.mozilla.org/comm-central/rev/64d27f1a0583
fix fluent id duplications in mail/. r=me

Comment 79

[Francesco Lodolo [:flod]]

I plan to run the migrations on Tuesday morning (EU time). Let me know if I should delay it, in case you need more time to check for issues.

Comment 80

[Magnus Melin [:mkmelin]]

I didn't find any further issues so far - so should be good to go with the migrations.