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

NEW
Unassigned

Status

()

P3
normal
a year ago
a year ago

People

(Reporter: gandalf, Unassigned)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 obsolete attachment)

Part of bug 1415730 focused on the General category
(Reporter)

Updated

a year ago
Blocks: 1415730
Component: Internationalization → Preferences
Priority: -- → P3
Product: Core → Firefox
Comment hidden (mozreview-request)
(Reporter)

Updated

a year ago
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
(Reporter)

Comment 2

a year ago
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.
Attachment #8944579 - Flags: feedback?(francesco.lodolo)
(Reporter)

Updated

a year ago
Depends on: 1432338
Comment hidden (mozreview-request)
(Reporter)

Updated

a year ago
Attachment #8944579 - Flags: feedback?(francesco.lodolo)
(Reporter)

Comment 4

a year ago
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?
Flags: needinfo?(jaws)

Comment 5

a year ago
mozreview-review
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 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.
Attachment #8944579 - Flags: feedback?(francesco.lodolo) → feedback+
(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.
Flags: needinfo?(jaws)
(Reporter)

Updated

a year ago
Attachment #8944579 - Attachment is obsolete: true
(Reporter)

Comment 8

a year ago
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.
Assignee: gandalf → nobody
Status: ASSIGNED → NEW
Depends on: 1435912, 1435915
Summary: Migrate the "General" section of Preferences to the new Localization API → [tracking] Migrate the "General" section of Preferences to the new Localization API
(Reporter)

Updated

a year ago
No longer depends on: 1432338
(Reporter)

Updated

a year ago
Depends on: 1438375
You need to log in before you can comment on or make changes to this bug.