Bookmark folder item count should use correct plural forms

RESOLVED FIXED in Firefox 11

Status

()

Firefox
Bookmarks & History
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: marcoos, Assigned: marcoos)

Tracking

Trunk
Firefox 11
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

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
(Assignee)

Comment 1

7 years ago
Created attachment 469932 [details] [diff] [review]
Patch

This should do it. :)
(Assignee)

Updated

7 years ago
Attachment #469932 - Flags: review?(l10n)
(Assignee)

Updated

7 years ago
Attachment #469932 - Flags: review?(mak77)

Updated

7 years ago
Assignee: nobody → marcoos+bmo
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
Attachment #469932 - Flags: review?(mak77) → review-

Updated

7 years ago
Attachment #469932 - Flags: review?(l10n)
(Assignee)

Comment 3

6 years ago
Created attachment 580572 [details] [diff] [review]
Patch with review comments adressed

Addressed the review comments.

Sorry for the one-year long break here. :)
Attachment #580572 - Flags: review?(mak77)
(Assignee)

Comment 4

6 years ago
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.
Attachment #580572 - Flags: review?(mak77) → review?
(Assignee)

Updated

6 years ago
Attachment #580572 - Flags: review?
(Assignee)

Comment 5

6 years ago
Created attachment 580576 [details] [diff] [review]
Patch with review comments adressed
Attachment #469932 - Attachment is obsolete: true
Attachment #580572 - Attachment is obsolete: true
Attachment #580576 - Flags: review?(mak77)
Attachment #580576 - Flags: feedback?(l10n)
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
Attachment #580576 - Flags: review?(mak77) → review+
(Assignee)

Comment 7

6 years ago
(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?
(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.
(Assignee)

Comment 9

6 years ago
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.
hm, may be due to the fact PlacesUtils.jsm exports more than one symbol...
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]?
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.
(Assignee)

Updated

6 years ago
Attachment #580987 - Flags: feedback?(l10n)
(Assignee)

Updated

6 years ago
Attachment #580576 - Flags: feedback?(l10n)

Updated

6 years ago
Attachment #580987 - Attachment is patch: true

Updated

6 years ago
Attachment #580987 - Flags: feedback?(l10n) → feedback+
(Assignee)

Updated

6 years ago
Attachment #580987 - Flags: checkin?
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
Created attachment 581306 [details] [diff] [review]
Patch, for check-in
Attachment #581306 - Flags: checkin?
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
(Assignee)

Updated

6 years ago
Attachment #581306 - Attachment is patch: true
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)
Attachment #580987 - Attachment is obsolete: true
Attachment #580987 - Flags: checkin?

Updated

6 years ago
Attachment #580576 - Attachment is obsolete: true
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.
(Assignee)

Updated

6 years ago
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/82187424f051
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 11
Attachment #581306 - Flags: checkin? → checkin+
You need to log in before you can comment on or make changes to this bug.