Support plural forms in messenger.properties where possible

RESOLVED FIXED in Thunderbird 23.0

Status

Thunderbird
General
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: standard8, Assigned: sshagarwal)

Tracking

({intl})

Trunk
Thunderbird 23.0
Bug Flags:
wanted-thunderbird +

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 3 obsolete attachments)

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+

Comment 1

4 years ago
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]

Comment 2

4 years ago
The same problem seems to exist in Seamonkey so bonus points for patching that too in the same way.
Keywords: intl

Updated

4 years ago
Assignee: nobody → syshagarwal
Status: NEW → ASSIGNED
(Assignee)

Comment 3

4 years ago
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.
Attachment #745697 - Flags: review?(neil)
Attachment #745697 - Flags: review?(mbanner)
Attachment #745697 - Flags: feedback?(acelists)

Comment 4

4 years ago
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 5

4 years ago
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

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

Comment 7

4 years ago
Created attachment 745700 [details] [diff] [review]
Patch

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 8

4 years ago
Comment on attachment 745700 [details] [diff] [review]
Patch

Yes, that's what I wanted.
Attachment #745700 - Flags: feedback?(acelists) → feedback+

Comment 9

4 years ago
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+

Comment 10

4 years ago
Neil, do you think we should also do the same in the Thunderbird files?
Flags: needinfo?(neil)
(Assignee)

Comment 11

4 years ago
Created attachment 745711 [details] [diff] [review]
Patch v2

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

Comment 14

4 years ago
Created attachment 746943 [details] [diff] [review]
Patch v2 (revised)

Thanks for the reviews.
Carrying over reviews from standard8 and neil.
Attachment #745711 - Attachment is obsolete: true
Attachment #746943 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: intl → checkin-needed

Updated

4 years ago
Keywords: intl
https://hg.mozilla.org/comm-central/rev/4c366a1789a7
Status: ASSIGNED → RESOLVED
Last Resolved: 4 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.