658191 - abuse of pluralform for addonsInstalled messages

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[dynamis (Tomoya ASAI)]

In short:
Pluralform should not be used single/multiple conditional branching.

Please define separate entity for single/multiple case for following messages to make it localizable.
http://mxr.mozilla.org/comm-central/source/mozilla/browser/locales/en-US/chrome/browser/browser.properties#43
addonsInstalled=#1 has been installed successfully.;#2 add-ons have been installed successfully.
addonsInstalledNeedsRestart=#1 will be installed after you restart #3.;#2 add-ons will be installed after you restart #3.

Details:
Add-on name is included when single add-on is installed.
Only a number of add-on is included when multiple add-ons are installed.

We should localize differently for single/multiple case but many locales (pluralrule=0,3,4...) cannot use different translation for single/multiple case translate this properly.
FYI: https://developer.mozilla.org/en/Localization_and_Plurals

So, please define separate entity for these messages.

Comment 1

[Axel Hecht [:Pike]]

Confirmed, but with an extra:

Keep the addonsInstalledNeedsRestart as is, but add a namedAddonInstalledRestart or so for the "we have one addon and want to show the name". ie, keep the plural magic for the numbers, because locales will still need that.

Comment 2

[dynamis (Tomoya ASAI)]

Yes, entities like this is fine:

namedAddonsInstalled=#1 has been installed successfully.
addonsInstalled=An add-on has been installed successfully.;#2 add-ons have been installed successfully.
namedAddonsInstalledNeedsRestart=#1 will be installed after you restart #3.
addonsInstalledNeedsRestart=An add-on will be installed after you restart #3.;#2 add-ons will be installed after you restart #3.

Comment 3

[Francesco Lodolo [:flod]]

Attachment: Bug 658191 - Fix plural form in Add-on installation messages

addonsInstalled and addonsInstalledNeedsRestart currently abuse
plural forms, using the singular form to expose the name of the add-on.

While the first form is singular for English, it's used for other numbers
in other languages, and in others there's only one form available,
making these strings impossible to translate.

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

Comment 4

[Francesco Lodolo [:flod]]

Comment on attachment 8934530 [details]
Bug 658191 - Fix plural form in Add-on installation messages

Does this approach make sense?

Comment 5

[Francesco Lodolo [:flod]]

@aswan
Checking the file's log, you're one the last persons to touch the file on the add-on part. Let me know if I should redirect the request elsewhere.

Comment 6

[Andrew Swan [:aswan]]

Comment on attachment 8934530 [details]
Bug 658191 - Fix plural form in Add-on installation messages

https://reviewboard.mozilla.org/r/205436/#review211174

Hopefully we'll be getting rid of some of this code soon, but if this is causing trouble for translators lets get it fixed.

::: browser/base/content/browser-addons.js:454
(Diff revision 1)
> +          messageString = messageString.replace("#1", numAddons);
> +          messageString = messageString.replace("#2", brandShortName);

Any reason not to switch over to getFormattedString() here instead of this?  (same below)

::: browser/base/content/browser-addons.js:471
(Diff revision 1)
>            accessKey: gNavigatorBundle.getString("addonInstallRestartIgnoreButton.accesskey"),
>            callback: () => {},
>          }];
>        } else {
> -        messageString = gNavigatorBundle.getString("addonsInstalled");
> +        if (numAddons == 1) {
> +          messageString = gNavigatorBundle.getFormattedString("addonInstalled",

The second parameter (brandShortName) isn't used in this string

Comment 7

[Francesco Lodolo [:flod]]

Comment on attachment 8934530 [details]
Bug 658191 - Fix plural form in Add-on installation messages

https://reviewboard.mozilla.org/r/205436/#review211178

Yeah, unfortunately these strings are still creating a lot of confusion, so it's better to fix them.

I'm starting to look into errors in plural strings (wrong number of forms), and we have a lot of them on these.

::: browser/base/content/browser-addons.js:454
(Diff revision 1)
> +          messageString = messageString.replace("#1", numAddons);
> +          messageString = messageString.replace("#2", brandShortName);

The format for plural strings requires placeholders with #1, #2, etc. while getFormattedStrings uses printf placeholders (%S, %1$s).

::: browser/base/content/browser-addons.js:471
(Diff revision 1)
>            accessKey: gNavigatorBundle.getString("addonInstallRestartIgnoreButton.accesskey"),
>            callback: () => {},
>          }];
>        } else {
> -        messageString = gNavigatorBundle.getString("addonsInstalled");
> +        if (numAddons == 1) {
> +          messageString = gNavigatorBundle.getFormattedString("addonInstalled",

Ugh, too much copy and paste. Thanks for catching it.

Comment 8

[Francesco Lodolo [:flod]]

Comment on attachment 8934530 [details]
Bug 658191 - Fix plural form in Add-on installation messages

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

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b046a40bac95
Fix plural form in Add-on installation messages r=aswan

Comment 10

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/b046a40bac95