590283 - Bookmark folder item count should use correct plural forms

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Marek Stępień [:marcoos, inactive]]

Folder item count uses hard-coded English plural forms.

/browser/locales/en-US/chrome/browser/places/places.properties:
detailsPane.oneItem=One item
detailsPane.multipleItems=%S items

(http://mxr.mozilla.org/mozilla-central/search?find=%2F&string=detailsPane.multipleItems)

Correct, locale-specific plural forms should be used here, as described in  https://developer.mozilla.org/En/Localization_and_Plurals

Comment 1

[Marek Stępień [:marcoos, inactive]]

Attachment: Patch

This should do it. :)

Comment 2

[Marco Bonardo [:mak]]

Comment on attachment 469932 [details] [diff] [review]
Patch


>diff -r bb8020341d71 browser/components/places/src/PlacesUIUtils.jsm
>--- a/browser/components/places/src/PlacesUIUtils.jsm	Tue Jul 13 17:44:51 2010 +0200
>+++ b/browser/components/places/src/PlacesUIUtils.jsm	Fri Aug 27 19:58:17 2010 +0200
>@@ -49,6 +49,7 @@
> 
> Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> Cu.import("resource://gre/modules/Services.jsm");
>+Cu.import("resource://gre/modules/PluralForm.jsm");

make this import a defineLazyGetter please (like the PlacesUtils getter below), so that we load the module only if a pluralform is required, could be this would init it at startup when it's not immediately needed.

>+  getPluralString: function PUIU_getPluralString(key, number) {
>+    var str = bundle.GetStringFromName(key);
>+    return PluralForm.get(number, str).replace("#1", number)

missing final semicolon

the function should probably take an array of params and replace #1 with params[0], #2 with params[1] and so on, supposing we'll always need just one param is a bit restrictive.

apart these looks fine

Comment 3

[Marek Stępień [:marcoos, inactive]]

Attachment: Patch with review comments adressed

Addressed the review comments.

Sorry for the one-year long break here. :)

Comment 4

[Marek Stępień [:marcoos, inactive]]

Comment on attachment 580572 [details] [diff] [review]
Patch with review comments adressed

Uhm, wrong version of the patch. Will submit the correct one in a moment.

Comment 5

[Marek Stępień [:marcoos, inactive]]

Attachment: Patch with review comments adressed

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 580576 [details] [diff] [review]
Patch with review comments adressed

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

Looks ok, some small comments here

::: browser/components/places/src/PlacesUIUtils.jsm
@@ +52,5 @@
>  
> +XPCOMUtils.defineLazyGetter(this, "PluralForm", function() {
> +  Cu.import("resource://gre/modules/PluralForm.jsm");
> +  return PluralForm;
> +});

We have defineLazyModuleGetter that does most of this for you, just

XPCOMUtils.defineLazyModuleGetter(this, "PluralForm",
		                  "resource://gre/modules/PluralForm.jsm");

@@ +84,4 @@
>      return bundle.formatStringFromName(key, params, params.length);
>    },
>  
> +  getPluralString: function PUIU_getPluralString(key, number, params) {

per code style, even if the other methods are old and incorrect, please use the "a" prefix on input arguments, so aKey, aNumber, aParams

Also having a small javadoc above the method, explaining what it does and what the arguments mean, would be nice (see showBookmarkDialog method for an example of the right style)

@@ +86,5 @@
>  
> +  getPluralString: function PUIU_getPluralString(key, number, params) {
> +    let str = PluralForm.get(number, bundle.GetStringFromName(key));
> +
> +    // replace #1 with params[0], #2 with params[1], and so on

nit: UCfirst and end with a period please.

@@ +88,5 @@
> +    let str = PluralForm.get(number, bundle.GetStringFromName(key));
> +
> +    // replace #1 with params[0], #2 with params[1], and so on
> +    return str.replace(/\#(\d+)/g, function (matchedId, matchedNumber) {
> +      let param = params[Number(matchedNumber) - 1];

I'd prefer if you'd use parseInt(matchedNumber) here, should be a bit stricter

Comment 7

[Marek Stępień [:marcoos, inactive]]

(In reply to Marco Bonardo [:mak] from comment #6)
> We have defineLazyModuleGetter that does most of this for you, just
> 
> XPCOMUtils.defineLazyModuleGetter(this, "PluralForm",
> 		                  "resource://gre/modules/PluralForm.jsm");

Should I switch PlacesUtils (just below it) to defineLazyModuleGetter, too, or leave at as it is?

Comment 8

[Marco Bonardo [:mak]]

(In reply to Marek Stępień :marcoos from comment #7)
> Should I switch PlacesUtils (just below it) to defineLazyModuleGetter, too,
> or leave at as it is?

Sure, you're free to do that.

Comment 9

[Marek Stępień [:marcoos, inactive]]

Attachment: Updated patch

This one is without the additional PlacesUtils lazy getter change, because things started to break after I replaced defineLazyGetter with defineLazyModuleGetter.

Comment 10

[Marco Bonardo [:mak]]

hm, may be due to the fact PlacesUtils.jsm exports more than one symbol...

Comment 11

[Marek Stępień [:marcoos, inactive]]

Marco, should I ask for a separate review for attachment 580987 [details] [diff] [review]? Or does it carry your r+ from  attachment 580576 [details] [diff] [review]?

Comment 12

[Marco Bonardo [:mak]]

once you get a r+ you don't need a further one. You may ask one of you think may be useful, for example if you did larger changes than the ones in the review comments and want a second opinion.
You may still get the l10n feedback though.

Comment 13

[Marco Bonardo [:mak]]

please add checkin comment and author to your patch, see https://developer.mozilla.org/en/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

then add the checkin-needed keyword

Comment 14

[Marek Stępień [:marcoos, inactive]]

Attachment: Patch, for check-in

Comment 15

[Marco Bonardo [:mak]]

Comment on attachment 580987 [details] [diff] [review]
Updated patch

Thank you, I'm obsoleting patches, there's no need to keep them visible (people caring to see flags can install Bugzilla Tweaks add-on that unhides positively flagged attachments)

Comment 16

[Marco Bonardo [:mak]]

As a reference for your future patches, you can just use the nicknames in the commit message, so in this case r=mak f=Pike would be fine.
If I haven't said that yet, thank you for the patch.

Comment 17

[Dão Gottwald [:dao]]

https://hg.mozilla.org/mozilla-central/rev/82187424f051