Last Comment Bug 590283 - Bookmark folder item count should use correct plural forms
: Bookmark folder item count should use correct plural forms
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Bookmarks & History (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 11
Assigned To: Marek Stępień [:marcoos, inactive]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-08-24 13:15 PDT by Marek Stępień [:marcoos, inactive]
Modified: 2011-12-15 02:23 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (3.68 KB, patch)
2010-08-27 11:03 PDT, Marek Stępień [:marcoos, inactive]
mak77: review-
Details | Diff | Splinter Review
Patch with review comments adressed (4.08 KB, patch)
2011-12-09 15:14 PST, Marek Stępień [:marcoos, inactive]
no flags Details | Diff | Splinter Review
Patch with review comments adressed (4.10 KB, patch)
2011-12-09 15:30 PST, Marek Stępień [:marcoos, inactive]
mak77: review+
Details | Diff | Splinter Review
Updated patch (4.66 KB, patch)
2011-12-12 11:43 PST, Marek Stępień [:marcoos, inactive]
l10n: feedback+
Details | Diff | Splinter Review
Patch, for check-in (7.07 KB, patch)
2011-12-13 09:35 PST, Marek Stępień [:marcoos, inactive]
emorley: checkin+
Details | Diff | Splinter Review

Description Marek Stępień [:marcoos, inactive] 2010-08-24 13:15:27 PDT
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] 2010-08-27 11:03:18 PDT
Created attachment 469932 [details] [diff] [review]
Patch

This should do it. :)
Comment 2 Marco Bonardo [::mak] 2010-08-30 06:33:44 PDT
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] 2011-12-09 15:14:09 PST
Created attachment 580572 [details] [diff] [review]
Patch with review comments adressed

Addressed the review comments.

Sorry for the one-year long break here. :)
Comment 4 Marek Stępień [:marcoos, inactive] 2011-12-09 15:27:24 PST
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] 2011-12-09 15:30:19 PST
Created attachment 580576 [details] [diff] [review]
Patch with review comments adressed
Comment 6 Marco Bonardo [::mak] 2011-12-12 07:40:31 PST
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] 2011-12-12 10:38:22 PST
(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] 2011-12-12 10:59:15 PST
(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] 2011-12-12 11:43:19 PST
Created attachment 580987 [details] [diff] [review]
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] 2011-12-12 12:02:03 PST
hm, may be due to the fact PlacesUtils.jsm exports more than one symbol...
Comment 11 Marek Stępień [:marcoos, inactive] 2011-12-12 15:38:59 PST
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] 2011-12-12 16:35:11 PST
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] 2011-12-13 04:32:09 PST
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] 2011-12-13 09:35:27 PST
Created attachment 581306 [details] [diff] [review]
Patch, for check-in
Comment 15 Marco Bonardo [::mak] 2011-12-13 09:55:15 PST
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] 2011-12-13 09:57:23 PST
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] 2011-12-14 02:43:03 PST
https://hg.mozilla.org/mozilla-central/rev/82187424f051

Note You need to log in before you can comment on or make changes to this bug.