1424681 - [tracking] Migrate the "General" section of Preferences to the new Localization API

Status: RESOLVED FIXED

(Firefox :: Settings UI, enhancement, P3)

Description

[Zibi Braniecki [:zbraniecki][:gandalf]]

Part of bug 1415730 focused on the General category

Comment 1

[Zibi Braniecki [:zbraniecki][:gandalf]]

Attachment: Bug 1424681 - Migrate the "General" section of Preferences to the new Localization API

Review commit: https://reviewboard.mozilla.org/r/214742/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/214742/

Comment 2

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8944579 [details]
Bug 1424681 - Migrate the "General" section of Preferences to the new Localization API

I was very optimistic about the size of this patch based on writing preliminary patches for other panes, but... This is massive.

I have to revise my plan. I believe we should focus on General pane exclusively after we land the chrome and get it landed in stages.

This is the first stage - it transitions most of the DTD except of a couple places where strings from shared .DTD files are being used.
Fortunately, it should just work this way, and as we move on we'll target those shared DTDs as well.

This patch also barely touches main.js file because it unfortunately will require a refactor to get to it. Many .properties strings are used there in synchronous fashion for sorting of the app handlers view.

We'll have to refactor the whole sorting, which shouldn't be super hard but I'd prefer not to pile on top of this patch.

The good news is that there don't seem to be any new Fluent features in play, so once we have Chrome ready, we should be able to just focus on this.

The bad news is that maintaining this patch and preventing it from bitrotting while applying all the feedback I'll probably get is going to be very hard.

:flod - can you take a look at the patch in the current scope? I can't test it yet, since we're blocked on 0.5, but maybe you'll be able to provide initial feedback already so that we can start crystallizing what we can hope to migrate here.

Comment 3

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 8944579 [details]
Bug 1424681 - Migrate the "General" section of Preferences to the new Localization API

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/214742/diff/1-2/

Comment 4

[Zibi Braniecki [:zbraniecki][:gandalf]]

To recap, my plan is to divide this bug into three parts:

1) Migrate most of the main.xul (and all of the main.dtd) to .ftl
2) Migrate main.js to .ftl
3) Migrate remaining pieces of DTD used in main.xul to .ftl

I'll file separate bugs for (2) and (3), and may also file a new one for (1) and keep this is an umbrella tracking bug for the pane migration.

:jaws - Does it look like a reasonable split to you?

Comment 5

[Francesco Lodolo [:flod]]

Comment on attachment 8944579 [details]
Bug 1424681 - Migrate the "General" section of Preferences to the new Localization API

https://reviewboard.mozilla.org/r/214742/#review220526

::: browser/locales/en-US/browser/preferences/preferences.ftl:90
(Diff revision 2)
>  #   $toolbarIcon (HTML): An icon for the toolbar menu
>  extension-controlled-enable = To enable the extension go to { $addonIcon } Add-ons in the { $toolbarIcon } menu.
> +
> +## General Section
> +
> +pane-general-title = General

This is already defined on line 30.

Ideally, we should run compare-locales to check for duplicates, but right now it has a lot of noise because of the syntax.

::: browser/locales/en-US/browser/preferences/preferences.ftl:95
(Diff revision 2)
> +pane-general-title = General
> +
> +startup-label = Startup
> +
> +separate-profile-mode =
> +    .label = Allow { -brand-short-name } and Firefox to run at the same time

This could benefit from a localization comment, e.g.

{ -brand-short-name } will be 'Firefox Developer Edition', since this setting is only exposed in Firefox Developer Edition

::: browser/locales/en-US/browser/preferences/preferences.ftl:96
(Diff revision 2)
> +
> +startup-label = Startup
> +
> +separate-profile-mode =
> +    .label = Allow { -brand-short-name } and Firefox to run at the same time
> +use-firefox-sync = Tip: This uses separate profiles. Use Sync to share data between them.

Sync -> { -sync-brand-short-name }

::: browser/locales/en-US/browser/preferences/preferences.ftl:129
(Diff revision 2)
> +
> +home-page-label = Home page
> +
> +use-current-page =
> +    .label =
> +        { $tabCount ->

Nice.

We'll need to document cases like this, i.e. a locale will be allowed to use less plural forms than requested, since the number is not exposed.

::: browser/locales/en-US/browser/preferences/preferences.ftl:145
(Diff revision 2)
> +    .label = Restore to Default
> +    .accesskey = R
> +
> +tabs-group-label = Tabs
> +
> +

nit: how do we feel about multiple empty lines?

::: browser/locales/en-US/browser/preferences/preferences.ftl:150
(Diff revision 2)
> +
> +ctrl-tab-recently-used-order =
> +    .label = Ctrl+Tab cycles through tabs in recently used order
> +    .accesskey = T
> +
> +new-windows-as-tabs =

I think we can pick a better name here, e.g. open-new-link-as-tabs

The old string ID comes from "Open new windows in a new tab instead", which was the previous form of the string (still in 58)

::: browser/locales/en-US/browser/preferences/preferences.ftl:163
(Diff revision 2)
> +warn-on-open-many-tabs =
> +    .label = Warn you when opening multiple tabs might slow down { -brand-short-name }
> +    .accesskey = d
> +
> +switch-links-to-new-tabs =
> +    .label = When you open a link in a new tab, switch to it immediatel

typo: immediately

::: browser/locales/en-US/browser/preferences/preferences.ftl:188
(Diff revision 2)
> +        [one] If you disable Container Tabs now, { $tabCount } container tab will be closed. Are you sure you want to disable Container Tabs?
> +       *[other] If you disable Container Tabs now, { $tabCount } container tabs will be closed. Are you sure you want to disable Container Tabs?
> +    }
> +
> +containers-disable-alert-ok-button =
> +    { $tabsCount ->

$tabCount

::: browser/locales/en-US/browser/preferences/preferences.ftl:225
(Diff revision 2)
> +
> +translate-web-pages =
> +    .label = Translate web content
> +    .accesskey = T
> +
> +translation-options-attribution = Translations by <image />

Let's add a comment to explain that image will be replaced by the translation provider's logo

::: browser/locales/en-US/browser/preferences/preferences.ftl:284
(Diff revision 2)
> +    .accesskey = P
> +
> +play-drm-content-learn-more = Learn more
> +
> +# Strings from aboutDialog.dtd are displayed in this section of the preferences.
> +# Please check for possible accesskey conflicts.

Both comments are not relevant: no more DTDs, and we gave up on accesskey conflicts with the the prefs reorg.

::: browser/locales/en-US/browser/preferences/preferences.ftl:334
(Diff revision 2)
> +performance-allow-hw-accel =
> +    .label = Use hardware acceleration when available
> +    .accesskey = r
> +
> +performance-limit-content-process-option = Content process limit
> +    .accesskey = L

L -> l

::: browser/locales/en-US/browser/preferences/preferences.ftl:356
(Diff revision 2)
> +browsing-use-smooth-scrolling =
> +    .label = Use smooth scrolling
> +    .accesskey = m
> +
> +browsing-use-onscreen-keyboard =
> +    .label = Show a touch keyboard when necessary

label -> Always use the cursor keys to navigate within pages
accesskey -> c

Comment 6

[Francesco Lodolo [:flod]]

Comment on attachment 8944579 [details]
Bug 1424681 - Migrate the "General" section of Preferences to the new Localization API

A few nits, but it mostly looks good to me.

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #4)
> To recap, my plan is to divide this bug into three parts:
> 
> 1) Migrate most of the main.xul (and all of the main.dtd) to .ftl
> 2) Migrate main.js to .ftl
> 3) Migrate remaining pieces of DTD used in main.xul to .ftl
> 
> I'll file separate bugs for (2) and (3), and may also file a new one for (1)
> and keep this is an umbrella tracking bug for the pane migration.
> 
> :jaws - Does it look like a reasonable split to you?

Yeah that split sounds find to me. I'm most concerned that a project like this will get started and not run to completion, but it seems that you have time dedicated to the project and as such I'm not as worried.

Comment 8

[Zibi Braniecki [:zbraniecki][:gandalf]]

Morphing this bug to be a tracking bug.

So far each patch is taking longer than expected to iron out and that makes stacking patches and complex patch series very prone to bitrotting.

In result I believe it's better to divide&conquer at least until we improve our turnaround. That's especially true for the most complex patch of the series - the General section is by far the largest.

I'm going to split out two bugs initially - one for XUL and one for JS - and then likely follow up with a third bug to clean up the remaining pieces, but that may instead wait until we move all panels and are ready to clean up the leftovers.