Last Comment Bug 719456 - Use PluralForm.jsm for feed import messages, to allow proper localization and translation
: Use PluralForm.jsm for feed import messages, to allow proper localization and...
Status: RESOLVED FIXED
: intl
Product: MailNews Core
Classification: Components
Component: Feed Reader (show other bugs)
: Trunk
: All All
: -- minor (vote)
: Thunderbird 13.0
Assigned To: :aceman
:
:
Mentors:
Depends on: 307629 716706
Blocks: 721517
  Show dependency treegraph
 
Reported: 2012-01-19 08:50 PST by Pavel Franc - Mozilla.cz
Modified: 2012-02-28 04:23 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (13.30 KB, patch)
2012-01-19 14:15 PST, :aceman
iann_bugzilla: review+
Details | Diff | Splinter Review
patch v2 (15.39 KB, patch)
2012-01-23 11:14 PST, :aceman
standard8: review-
Details | Diff | Splinter Review
patch v3 (20.59 KB, patch)
2012-01-30 09:10 PST, :aceman
standard8: review-
Details | Diff | Splinter Review
patch v4 (20.90 KB, patch)
2012-01-31 07:40 PST, :aceman
standard8: review+
Details | Diff | Splinter Review
patch v5 (8.26 KB, patch)
2012-02-24 17:07 PST, :aceman
standard8: review+
Details | Diff | Splinter Review

Description Pavel Franc - Mozilla.cz 2012-01-19 08:50:05 PST
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 2012-01-19 08:58:15 PST
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, afk until Dec) 2012-01-19 11:45:13 PST
aceman: see https://developer.mozilla.org/en/Localization_and_Plurals for more info about PluralForm.
Comment 3 :aceman 2012-01-19 14:15:39 PST
Created attachment 589987 [details] [diff] [review]
patch

Also convert to Services.prompt while at it (per hints in bug 224831).
Comment 4 Ian Neal 2012-01-21 08:57:35 PST
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 2012-01-21 09:37:19 PST
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 2012-01-21 10:14:22 PST
(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 2012-01-23 11:14:03 PST
Created attachment 590793 [details] [diff] [review]
patch v2

OK, I just removed the variable after advice from standard8. Carrying over r=ian neal.
Comment 8 Rimas Kudelis 2012-01-30 04:19:35 PST
It would be nice to get this in before the next repo migration (that is, before tomorrow).
Comment 9 :aceman 2012-01-30 04:45:03 PST
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, afk until Dec) 2012-01-30 07:30:50 PST
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 2012-01-30 08:07:15 PST
(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 2012-01-30 08:38:50 PST
(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 2012-01-30 08:40:54 PST
(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 2012-01-30 09:10:25 PST
Created attachment 592738 [details] [diff] [review]
patch v3
Comment 15 Mark Banner (:standard8, afk until Dec) 2012-01-30 16:20:36 PST
(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 2012-01-31 00:11:25 PST
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 2012-01-31 07:40:21 PST
Created attachment 593095 [details] [diff] [review]
patch v4
Comment 18 Mark Banner (:standard8, afk until Dec) 2012-02-21 03:28:47 PST
(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, afk until Dec) 2012-02-21 03:45:43 PST
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 2012-02-21 03:47:51 PST
(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 2012-02-24 15:36:39 PST
Yeah and let's start all over thanks to bug 716706...
Comment 22 :aceman 2012-02-24 17:07:22 PST
Created attachment 600575 [details] [diff] [review]
patch v5

So I dropped everything except the real string changes.
Comment 23 Mark Banner (:standard8, afk until Dec) 2012-02-28 04:23:33 PST
Checked in: http://hg.mozilla.org/comm-central/rev/9dad12835d44

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