719456 - Use PluralForm.jsm for feed import messages, to allow proper localization and translation

Status: RESOLVED FIXED

(MailNews Core :: Feed Reader, defect)

Description

[Pavel Franc - Mozilla.cz]

In bug 307629, there were introduced few messages containing numbers but using only plural form. Use PluralForm.jsm to allow singular and multiple plural forms.

Comment 1

[:aceman]

I'll look into this. The code spot in which this is is not very interesting (users encounter it very rarely, most users never).
But I will learn how to use the PluralForm module (for future more important changes).

Comment 2

[Mark Banner (:standard8)]

aceman: see https://developer.mozilla.org/en/Localization_and_Plurals for more info about PluralForm.

Comment 3

[:aceman]

Attachment: patch

Also convert to Services.prompt while at it (per hints in bug 224831).

Comment 4

[Ian Neal]

Comment on attachment 589987 [details] [diff] [review]
patch

>+++ b/mailnews/extensions/newsblog/content/feed-subscriptions.js
>+Components.utils.import("resource://gre/modules/Services.jsm");
>+Components.utils.import("resource://gre/modules/PluralForm.jsm");
>+
>+const promptService = Services.prompt;
I think this should be a var rather than a const (looking at code elsewhere in the tree).

r=me with that addressed, you will still need a review from the TB side.

Comment 5

[:aceman]

Comment on attachment 589987 [details] [diff] [review]
patch

Why can't it be const if it isn't changed anywhere? Also notice, there is already another service defined const. What are the considerations here?

Comment 6

[Ian Neal]

(In reply to :aceman from comment #5)
> Comment on attachment 589987 [details] [diff] [review]
> patch
> 
> Why can't it be const if it isn't changed anywhere? Also notice, there is
> already another service defined const. What are the considerations here?

As I said, everywhere else seems to use the likes of var ps = Services.prompt;
http://mxr.mozilla.org/comm-central/search?string=%3D+Services.prompt%3B&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central

Comment 7

[:aceman]

Attachment: patch v2

OK, I just removed the variable after advice from standard8. Carrying over r=ian neal.

Comment 8

[Rimas Kudelis [use needinfo to ping]]

It would be nice to get this in before the next repo migration (that is, before tomorrow).

Comment 9

[:aceman]

Comment on attachment 590793 [details] [diff] [review]
patch v2

That is true, but we need a review from somebody from Thunderbird.

Comment 10

[Mark Banner (:standard8)]

Comment on attachment 590793 [details] [diff] [review]
patch v2

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

I'm not sure we should generally do other changes in these specific bug fixing, however as Ian has already given his r+ I'll let it go this time.

::: mail/locales/en-US/chrome/messenger-newsblog/newsblog.properties
@@ +15,5 @@
>  subscribe-OPMLImportInvalidFile=The file %S does not seem to be a valid OPML file.
> +## LOCALIZATION NOTE(subscribe-OPMLImportNewFeeds): Semi-colon list of plural forms.
> +## See: http://developer.mozilla.org/en/docs/Localization_and_Plurals
> +## #1 is the count of new imported entries.
> +subscribe-OPMLImportNewFeeds=Imported #1 new feed.;Imported #1 new feeds.

You need to change the names of the string, else they won't be noticed by l10n.

@@ +20,5 @@
> +## LOCALIZATION NOTE(subscribe-OPMLImportUniqueFeeds): Semi-colon list of plural forms.
> +## There is a space at the end of the sentence, because the string
> +## subscribe-OPMLImportFoundFeeds will be appended to it.
> +## #1 is the count of new imported entries
> +subscribe-OPMLImportUniqueFeeds=Imported #1 new feed to which you aren't already subscribed ;Imported #1 new feeds to which you aren't already subscribed ;

I don't think this wants a ; on the end (your other ones don't).

::: mailnews/extensions/newsblog/content/feed-subscriptions.js
@@ +82,3 @@
>        var newsBlogBundle = document.getElementById("bundle_newsblog");
> +      dismissDialog = !(Services.prompt.confirmEx(window,
> +                                        newsBlogBundle.getString('subscribe-cancelSubscriptionTitle'),

nit: The indententation should either be:

dismissDialog = !(Services.prompt.confirmEx(window,
                                            newsBlogBundle.getString('subscribe-cancelSubscription'),

or:

dismissDialog = !(Services.prompt.confirmEx(window,
  newsBlogBundle.getString('subscribe-cancelSubscription'),

@@ +968,5 @@
>            feedsAdded++;
>        }
>  
>        if (outlines.length > feedsAdded)
> +        statusReport = PluralForm.get(feedsAdded, this.mBundle.getString("subscribe-OPMLImportUniqueFeeds")).replace("#1", feedsAdded)+

nit: space after ) and before +.

I think you should consider wrapping these lines as well.

Comment 11

[Ian Neal]

(In reply to Mark Banner (:standard8) from comment #10)
> Comment on attachment 590793 [details] [diff] [review]
> patch v2
> ::: mail/locales/en-US/chrome/messenger-newsblog/newsblog.properties
> @@ +15,5 @@
> >  subscribe-OPMLImportInvalidFile=The file %S does not seem to be a valid OPML file.
> > +## LOCALIZATION NOTE(subscribe-OPMLImportNewFeeds): Semi-colon list of plural forms.
> > +## See: http://developer.mozilla.org/en/docs/Localization_and_Plurals
> > +## #1 is the count of new imported entries.
> > +subscribe-OPMLImportNewFeeds=Imported #1 new feed.;Imported #1 new feeds.
> 
> You need to change the names of the string, else they won't be noticed by
> l10n.
Sorry, I think I went a bit cross-eyed whilst checking that, as I remember checking the other lines had changed their names.

Comment 12

[:aceman]

(In reply to Mark Banner (:standard8) from comment #10)
> @@ +20,5 @@
> > +## LOCALIZATION NOTE(subscribe-OPMLImportUniqueFeeds): Semi-colon list of plural forms.
> > +## There is a space at the end of the sentence, because the string
> > +## subscribe-OPMLImportFoundFeeds will be appended to it.
> > +## #1 is the count of new imported entries
> > +subscribe-OPMLImportUniqueFeeds=Imported #1 new feed to which you aren't already subscribed ;Imported #1 new feeds to which you aren't already subscribed ;
> 
> I don't think this wants a ; on the end (your other ones don't).

It seems it works regardless of the trailing semicolon.
I left that ; intentionally there to make the trailing space very visible. Should I still remove it?

Comment 13

[:aceman]

(In reply to Ian Neal from comment #11)
> (In reply to Mark Banner (:standard8) from comment #10)
> > Comment on attachment 590793 [details] [diff] [review]
> > patch v2
> > ::: mail/locales/en-US/chrome/messenger-newsblog/newsblog.properties
> > @@ +15,5 @@
> > >  subscribe-OPMLImportInvalidFile=The file %S does not seem to be a valid OPML file.
> > > +## LOCALIZATION NOTE(subscribe-OPMLImportNewFeeds): Semi-colon list of plural forms.
> > > +## See: http://developer.mozilla.org/en/docs/Localization_and_Plurals
> > > +## #1 is the count of new imported entries.
> > > +subscribe-OPMLImportNewFeeds=Imported #1 new feed.;Imported #1 new feeds.
> > 
> > You need to change the names of the string, else they won't be noticed by
> > l10n.
> Sorry, I think I went a bit cross-eyed whilst checking that, as I remember
> checking the other lines had changed their names.

Yes the other changed but I left this one thinking it already changed in this cycle (TB12) so localizers have not yet picked it up, so this new version will be the only one they see when they jump on Aurora 12. But I can change it too if needed. Probably thinking too much :)

Comment 14

[:aceman]

Attachment: patch v3

Comment 15

[Mark Banner (:standard8)]

(In reply to :aceman from comment #12)
> (In reply to Mark Banner (:standard8) from comment #10)
> > @@ +20,5 @@
> > > +## LOCALIZATION NOTE(subscribe-OPMLImportUniqueFeeds): Semi-colon list of plural forms.
> > > +## There is a space at the end of the sentence, because the string
> > > +## subscribe-OPMLImportFoundFeeds will be appended to it.
> > > +## #1 is the count of new imported entries
> > > +subscribe-OPMLImportUniqueFeeds=Imported #1 new feed to which you aren't already subscribed ;Imported #1 new feeds to which you aren't already subscribed ;
> > 
> > I don't think this wants a ; on the end (your other ones don't).
> 
> It seems it works regardless of the trailing semicolon.
> I left that ; intentionally there to make the trailing space very visible.
> Should I still remove it?

Hang on, if we're relying on trailing spaces, then that isn't good. Which then makes me realise that we're stringing these two plural strings together.

So to do this in a way that will satisfy rtl locales, we need to do a compositor entity and the two entities we have now, ending up with something like this:

http://hg.mozilla.org/comm-central/annotate/ed69c6a773d6/mail/locales/en-US/chrome/messenger/glodaFacetView.properties#l145

Comment 16

[:aceman]

I thought about making it that way but we haven't done it that way in bug 346306 so I thought it is not really needed.
So I'll rework it.

Comment 17

[:aceman]

Attachment: patch v4

Comment 18

[Mark Banner (:standard8)]

(In reply to :aceman from comment #16)
> I thought about making it that way but we haven't done it that way in bug
> 346306 so I thought it is not really needed.

Bug 346306 didn't use plural forms...

Comment 19

[Mark Banner (:standard8)]

Comment on attachment 593095 [details] [diff] [review]
patch v4

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

::: mailnews/extensions/newsblog/content/feed-subscriptions.js
@@ +977,5 @@
>            feedsAdded++;
>        }
>  
>        if (outlines.length > feedsAdded)
> +        statusReport = this.mBundle.getFormattedString("subscribe-OPMLImportStatus",

nit: slightly wrong indentation in this block. Should be:

statusReport = this.mBundle.getFormattedString("subscribe-OPMLImportStatus",
  [PluralForm.get(feedsAdded,
                  this.mBundle.getString("subscribe-OPMLImportUniqueFeeds"))
             .replace("#1", feedsAdded),
   PluralForm.get(outlines.length,
                  this.mBundle.getString("subscribe-OPMLImportFoundFeeds"))
             .replace("#1", outlines.length)]);

So basically, align the dots. Make sure the second attribute starts under the letter after the opening bracket of the function, and similarly with the second value in the array.

Comment 20

[:aceman]

(In reply to Mark Banner (:standard8) from comment #18)
> (In reply to :aceman from comment #16)
> > I thought about making it that way but we haven't done it that way in bug
> > 346306 so I thought it is not really needed.
> 
> Bug 346306 didn't use plural forms...

But it now has hardcoded concatenation of strings via space:
- suffix="&haveSmtp1.suffix2;">*</description>
+ suffix="&haveSmtp1.suffix3; &modifyOutgoing.suffix;">*</description>

That is what you are objecting to in comment 15.

Comment 21

[:aceman]

Yeah and let's start all over thanks to bug 716706...

Comment 22

[:aceman]

Attachment: patch v5

So I dropped everything except the real string changes.

Comment 23

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/9dad12835d44