Last Comment Bug 595104 - Support plural forms in messenger.properties where possible
: Support plural forms in messenger.properties where possible
Status: RESOLVED FIXED
[good first bug][mentor=aceman][lang=...
: intl
Product: Thunderbird
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Thunderbird 23.0
Assigned To: Suyash Agarwal (:sshagarwal)
:
:
Mentors:
http://mxr.mozilla.org/comm-central/s...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-09-10 01:53 PDT by Mark Banner (:standard8)
Modified: 2013-05-08 09:47 PDT (History)
7 users (show)
standard8: wanted‑thunderbird+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch (10.10 KB, patch)
2013-05-05 13:31 PDT, Suyash Agarwal (:sshagarwal)
acelists: feedback+
Details | Diff | Splinter Review
Patch (10.05 KB, patch)
2013-05-05 14:05 PDT, Suyash Agarwal (:sshagarwal)
acelists: feedback+
neil: feedback+
Details | Diff | Splinter Review
Patch v2 (10.08 KB, patch)
2013-05-05 15:38 PDT, Suyash Agarwal (:sshagarwal)
neil: review+
standard8: review+
Details | Diff | Splinter Review
Patch v2 (revised) (11.00 KB, patch)
2013-05-08 07:22 PDT, Suyash Agarwal (:sshagarwal)
syshagarwal: review+
Details | Diff | Splinter Review

Description Mark Banner (:standard8) 2010-09-10 01:53:39 PDT
I've just noticed that there are two strings in messenger.properties where we have numbers displayed and the strings are processed in javascript, so we should be able to update them so that they use proper plural forms:

getNextNMessages
openWindowWarningText

See http://developer.mozilla.org/en/docs/Localization_and_Plurals and other examples in that file for more details. The string ids will have to be changed as well so that localisations can detect the update.
Comment 1 :aceman 2013-05-04 13:50:34 PDT
The idea of the change to be done here is the same as in bug 719456.
Comment 2 :aceman 2013-05-04 13:57:40 PDT
The same problem seems to exist in Seamonkey so bonus points for patching that too in the same way.
Comment 3 Suyash Agarwal (:sshagarwal) 2013-05-05 13:31:40 PDT
Created attachment 745697 [details] [diff] [review]
Patch

Sir, 

I have tried to make the necessary changes as directed by :aceman.
So, I request for feedback from :aceman and for review from Neil and Standard8.
Comment 4 :aceman 2013-05-05 13:40:37 PDT
Comment on attachment 745697 [details] [diff] [review]
Patch

I have tested this on English Thunderbird and is seems to work. The messages show as expected, there is actually no visible change in English version.

So f+ from me!
Comment 5 :aceman 2013-05-05 13:52:05 PDT
Comment on attachment 745697 [details] [diff] [review]
Patch

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

::: mail/locales/en-US/chrome/messenger/messenger.properties
@@ +403,1 @@
>  # for warning the user that a tag they're trying to create already exists

One tiny nit: I think you removed a blank line here, which you should put back.
Comment 6 :aceman 2013-05-05 13:53:19 PDT
Eh, I should learn to use the splinter review tool properly :(

I meant this empty line got removed:
-openWindowWarningText=Opening %S messages may be slow.  Continue?
-
+openWindowWarningConfirmation=Opening %S message may be slow.  Continue?;Opening %S messages may be slow.  Continue?
Comment 7 Suyash Agarwal (:sshagarwal) 2013-05-05 14:05:48 PDT
Created attachment 745700 [details] [diff] [review]
Patch

Sir,

I have made the changes.
Comment 8 :aceman 2013-05-05 14:12:13 PDT
Comment on attachment 745700 [details] [diff] [review]
Patch

Yes, that's what I wanted.
Comment 9 neil@parkwaycc.co.uk 2013-05-05 14:25:22 PDT
Comment on attachment 745700 [details] [diff] [review]
Patch

>-getNextNMessages=Get Next %S News Messages
>+getNextNewsMessages=Get Next %S News Message;Get Next %S News Messages
I believe it's traditional to use #1 for plural forms rather than %S.

> Components.utils.import("resource://gre/modules/Services.jsm");
> Components.utils.import("resource:///modules/mailServices.js");
> Components.utils.import("resource:///modules/folderUtils.jsm");
>+Components.utils.import("resource://gre/modules/PluralForm.jsm");
Please put PluralForm.jsm next to Services.jsm (because they are loaded from the same path, so it looks neater).

>+    var text = PluralForm.get(numMessages, gMessengerBundle
>+                         .getString("openWindowWarningConfirmation"))
>+                         .replace("%S", numMessages);
I know it will result in a very long line, but keeping the .getString after gMessengerBundle will be less confusing than the indentation here.

>+        var menuLabel = PluralForm.get(newsServer.maxArticles,
>+          gMessengerBundle.getString("getNextNewsMessages"))
>+                          .replace("%S", newsServer.maxArticles);
Alternatively this version isn't too bad, except that the .replace needs to line up with the .get

(The other option would be to split the code up into two statements; first get the raw string from the bundle, then create the plural form.)
Comment 10 :aceman 2013-05-05 14:32:27 PDT
Neil, do you think we should also do the same in the Thunderbird files?
Comment 11 Suyash Agarwal (:sshagarwal) 2013-05-05 15:38:34 PDT
Created attachment 745711 [details] [diff] [review]
Patch v2

Sir,

I have made the changes as directed.
Comment 12 neil@parkwaycc.co.uk 2013-05-05 16:07:34 PDT
Comment on attachment 745711 [details] [diff] [review]
Patch v2

I didn't look at the mail/ files in the previous patch but the current patch style looks fine to me both for mail and suite.
Comment 13 Mark Banner (:standard8) 2013-05-08 02:48:24 PDT
Comment on attachment 745711 [details] [diff] [review]
Patch v2

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

r=me with the couple of comments I noted added to both of the messenger.properties files.

::: mail/locales/en-US/chrome/messenger/messenger.properties
@@ +12,5 @@
>  markFolderRead=Mark Folder Read;Mark Folders Read
>  markNewsgroupRead=Mark Newsgroup Read;Mark Newsgroups Read
>  folderProperties=Folder Properties
>  newTag=New Tag…
> +getNextNewsMessages=Get Next #1 News Message;Get Next #1 News Messages

Please include a localization note just before this along the lines of:

# LOCALIZATION NOTE (getNextNMessages): Semi-colon list of plural forms.
# #1 is the number of news messages to get.

@@ +398,5 @@
>  passwordTitle=Mail Server Password Required
>  
>  # for checking if the user really wants to open lots of messages
>  openWindowWarningTitle=Confirm
> +openWindowWarningConfirmation=Opening #1 message may be slow.  Continue?;Opening #1 messages may be slow.  Continue?

Please include a localization note just before this along the lines of:

# LOCALIZATION NOTE (openWindowWarningConfirmation): Semi-colon list of plural forms.
# #1 is the number of messages the user is attempting to open.
Comment 14 Suyash Agarwal (:sshagarwal) 2013-05-08 07:22:26 PDT
Created attachment 746943 [details] [diff] [review]
Patch v2 (revised)

Thanks for the reviews.
Carrying over reviews from standard8 and neil.
Comment 15 Ryan VanderMeulen [:RyanVM] 2013-05-08 09:47:44 PDT
https://hg.mozilla.org/comm-central/rev/4c366a1789a7

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