Closed Bug 595104 Opened 14 years ago Closed 11 years ago

Support plural forms in messenger.properties where possible

Categories

(Thunderbird :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 23.0

People

(Reporter: standard8, Assigned: sshagarwal)

References

()

Details

(Keywords: intl, Whiteboard: [good first bug][mentor=aceman][lang=javascript])

Attachments

(1 file, 3 obsolete files)

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.
Flags: wanted-thunderbird+
The idea of the change to be done here is the same as in bug 719456.
Whiteboard: [good first bug] → [good first bug][mentor=aceman][lang=javascript]
The same problem seems to exist in Seamonkey so bonus points for patching that too in the same way.
Keywords: intl
Assignee: nobody → syshagarwal
Status: NEW → ASSIGNED
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #745697 - Flags: review?(neil)
Attachment #745697 - Flags: review?(mbanner)
Attachment #745697 - Flags: feedback?(acelists)
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!
Attachment #745697 - Flags: feedback?(acelists) → feedback+
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.
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?
Attached patch Patch (obsolete) — Splinter Review
Sir,

I have made the changes.
Attachment #745697 - Attachment is obsolete: true
Attachment #745697 - Flags: review?(neil)
Attachment #745697 - Flags: review?(mbanner)
Attachment #745700 - Flags: review?(neil)
Attachment #745700 - Flags: review?(mbanner)
Attachment #745700 - Flags: feedback?(acelists)
Comment on attachment 745700 [details] [diff] [review]
Patch

Yes, that's what I wanted.
Attachment #745700 - Flags: feedback?(acelists) → feedback+
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.)
Attachment #745700 - Flags: review?(neil) → feedback+
Neil, do you think we should also do the same in the Thunderbird files?
Flags: needinfo?(neil)
Attached patch Patch v2 (obsolete) — Splinter Review
Sir,

I have made the changes as directed.
Attachment #745700 - Attachment is obsolete: true
Attachment #745700 - Flags: review?(mbanner)
Attachment #745711 - Flags: review?(neil)
Attachment #745711 - Flags: review?(mbanner)
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.
Attachment #745711 - Flags: review?(neil) → review+
Flags: needinfo?(neil)
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.
Attachment #745711 - Flags: review?(mbanner) → review+
Thanks for the reviews.
Carrying over reviews from standard8 and neil.
Attachment #745711 - Attachment is obsolete: true
Attachment #746943 - Flags: review+
Keywords: intlcheckin-needed
Keywords: intl
https://hg.mozilla.org/comm-central/rev/4c366a1789a7
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 23.0
You need to log in before you can comment on or make changes to this bug.