947353 - L10N string " tabs.closeWarningMultipleTabs" needs pluralization

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Pavel Cvrcek [:JasnaPaka]]

http://hg.mozilla.org/mozilla-central/file/default/browser/locales/en-US/chrome/browser/tabbrowser.properties

tabs.closeWarningMultipleTabs=You are about to close %S tabs. Are you sure you want to continue?

We need pluralization here.
https://developer.mozilla.org/en-US/docs/Localization_and_Plurals

Comment 1

[Francesco Lodolo [:flod]]

Tentatively assigning this to myself, in order to check if I can fix this.

Comment 2

[Francesco Lodolo [:flod]]

Attachment: bug947353.patch

Use proper plural form for warning on close with multiple tabs.

@dao
Please another reviewer if I chose the wrong person (no suggested reviewer, based my choice on the history of tabbrowser.xml).

Comment 3

[Francesco Lodolo [:flod]]

Attachment: bug947353v1.patch

Right patch without trailing blank spaces around

Comment 4

[Dão Gottwald [:dao]]

Comment on attachment 8367899 [details] [diff] [review]
bug947353v1.patch

>+          Components.utils.import("resource://gre/modules/PluralForm.jsm");
>+          var warningMessage = PluralForm.get(tabsToClose,
>+                                              bundle.getString("tabs.closeWarningMultiple")
>+                                              ).replace("#1", tabsToClose);

PluralForm is already available here, no need to import anything.

>+tabs.closeWarningMultiple=You are about to close #1 tab. Are you sure you want to continue?;You are about to close #1 tabs. Are you sure you want to continue?

The first string doesn't really make sense, as it shouldn't be possible to get that message for only one tab.

Comment 5

[Francesco Lodolo [:flod]]

Attachment: bug947353v3.patch

Thanks a lot for the review.

As suggested dropped unnecessary inclusion of PluralForm.jsm (didn't realize it was already available) and singular version of the string.

I find it less clear to read, but the same structure is used in other strings so I guess it's fine
http://hg.mozilla.org/mozilla-central/file/default/browser/locales/en-US/chrome/browser/browser.properties#l337

Comment 6

[Dão Gottwald [:dao]]

Comment on attachment 8367939 [details] [diff] [review]
bug947353v3.patch

>+          var warningMessage = PluralForm.get(tabsToClose,
>+                                              bundle.getString("tabs.closeWarningMultiple")
>+                                              ).replace("#1", tabsToClose);

please format that this way:

>          var warningMessage = PluralForm.get(tabsToClose,
>                                              bundle.getString("tabs.closeWarningMultiple"))
>                                         .replace("#1", tabsToClose);

or:

>          var warningMessage =
>            PluralForm.get(tabsToClose, bundle.getString("tabs.closeWarningMultiple"))
>                      .replace("#1", tabsToClose);

Comment 7

[Francesco Lodolo [:flod]]

Attachment: bug947353_reviewed.patch

Comment addressed, carrying on r+ from previous patch (hoping that's the correct procedure). Thanks again for helping.

I'll need someone to check-in the patch since I don't have access outside of l10n.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/2d7d07a85dd0

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2d7d07a85dd0

Comment 10

[Jesper Kristensen]

(In reply to Dão Gottwald [:dao] from comment #4)
> >+tabs.closeWarningMultiple=You are about to close #1 tab. Are you sure you want to continue?;You are about to close #1 tabs. Are you sure you want to continue?
> 
> The first string doesn't really make sense, as it shouldn't be possible to
> get that message for only one tab.

But the first string helps localizers understand that this is a normal plural forms string. Leaving it blank makes the string much harder to understand for localizers not using the same plural rules as English, for no good reason.

Comment 11

[Francesco Lodolo [:flod]]

Pavel, could you verify this one?

Comment 12

[Pavel Cvrcek [:JasnaPaka]]

It looks good. Thanks!

v=pcvrcek