Closed Bug 1346549 Opened 3 years ago Closed 2 years ago

Replace nsIScriptableDateFormat in mail/ and mailnews/

Categories

(Thunderbird :: General, enhancement, minor)

enhancement
Not set
minor

Tracking

(thunderbird_esr52 unaffected, thunderbird55 fixed, thunderbird56 fixed)

RESOLVED FIXED
Thunderbird 56.0
Tracking Status
thunderbird_esr52 --- unaffected
thunderbird55 --- fixed
thunderbird56 --- fixed

People

(Reporter: aceman, Assigned: jorgk)

References

(Blocks 1 open bug, )

Details

Attachments

(1 file, 3 obsolete files)

Bug 1313625 is deprecating nsIScriptableDateFormat. We can use Intl.DateTimeFormat or Date.toLocaleDateString().
Apparently Aryx is already working on this.
Assignee: acelists → aryx.bugmail
Mentor: acelists
Depends on: 1332351
Comment on attachment 8846656 [details] [diff] [review]
mail, v3

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

This will get merge conflicts with bug 1332351, for example in activity.xml where we're busily replacing |.toLocaleFormat("%A")| a few lines further down.

As a general question: In bug 1332351 Aceman is using .toLocaleDateString() instead of Intl.DateTimeFormat.format(). The calendar guys are using Intl.DateTimeFormat.format() in attachment 8846433 [details] [diff] [review]. Which one is preferred?

::: mail/components/preferences/cookies.js
@@ +516,5 @@
>      if (aExpires) {
>        var date = new Date(1000 * aExpires);
> +      const dtOptions = { year: 'numeric', month: 'long', day: 'numeric',
> +                          hour: 'numeric', minute: 'numeric', second: 'numeric' };
> +      const dateTimeFormatter = new Intl.DateTimeFormat(undefined, dtOptions);

Any reason to have a separate variable 'dtOptions'?
(In reply to Jorg K (GMT+1) from comment #3)
> Which one is preferred?
Oh, looks like that if you want to format date and time, you use Intl.DateTimeFormat(), for just date or time you can used .toLocaleDateString() or .toLocaleTimeString(). And you can use the former to prepare a formatter once outside a loop.
(In reply to Jorg K (GMT+1) from comment #3)
> As a general question: In bug 1332351 Aceman is using .toLocaleDateString()
> instead of Intl.DateTimeFormat.format(). The calendar guys are using
> Intl.DateTimeFormat.format() in attachment 8846433 [details] [diff] [review]
> [review]. Which one is preferred?

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Errors/Deprecated_toLocaleFormat

Intl.DateTimeFormat is an object that caches something so it preferable to use in tight loops converting many dates. It is probably fine to use it anywhere, I don't know if there are any disadvantages.

I just used Date.toLocaleDateString() as it is using methods on the date object we already have in hand, without having to instantiate a new Intl object and prepare the format elsewhere, e.g. on a separate line like Aryx has done.

(In reply to Jorg K (GMT+1) from comment #4)
> Oh, looks like that if you want to format date and time, you use
> Intl.DateTimeFormat(), for just date or time you can used
> .toLocaleDateString() or .toLocaleTimeString().

Or .toLocaleString() when you need both.

> And you can use the former
> to prepare a formatter once outside a loop.

Yes. But we have no loops in the patch. So it seems usage of Intl. is unnecessarily verbose and format split away from the object we apply it on.
(In reply to :aceman from comment #5)
> Yes. But we have no loops in the patch. So it seems usage of Intl. is
> unnecessarily verbose and format split away from the object we apply it on.
I was going to suggest to use .toLocale[Date|Time]String(), but I'm not the reviewer here. Also in bug 1313659 where possible.
Comment on attachment 8846656 [details] [diff] [review]
mail, v3

Thanks, I'll cancel the request for now. The patch will need update after bug 1332351 anyway and we also wait for Magnus whether to use the locale of 'undefined' or something else.
Attachment #8846656 - Flags: review?(acelists)
Bug 1332351 has just landed, so this can go ahead now.
No longer blocks: 1313659
Comment on attachment 8846656 [details] [diff] [review]
mail, v3

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

Aryx please update the patch now, it was bitrotted by the other bug.

::: mail/components/activity/content/activity.xml
@@ +238,5 @@
>          <parameter name="time"/>
>          <body>
>            <![CDATA[
> +            const timeFormatter = new Intl.DateTimeFormat(undefined, { hour: 'numeric',
> +                                                                       minute: 'numeric' });

I think this method got removed.

@@ +280,5 @@
>          <body>
>            <![CDATA[
> +            const dateTimeOptions = { year: 'numeric', month: 'long', day: 'numeric',
> +                                      hour: 'numeric', minute: 'numeric' };
> +            const dateTimeFormatter = new Intl.DateTimeFormat(undefined, dateTimeOptions);

Ok, formatTimeTip may be run in a loop (for each item in the list) so if you want to use Intl, set it up in the constructor, not in this method.

::: mail/components/preferences/cookies.js
@@ +516,5 @@
>      if (aExpires) {
>        var date = new Date(1000 * aExpires);
> +      const dtOptions = { year: 'numeric', month: 'long', day: 'numeric',
> +                          hour: 'numeric', minute: 'numeric', second: 'numeric' };
> +      const dateTimeFormatter = new Intl.DateTimeFormat(undefined, dtOptions);

formatExpiresString may be run in a loop (for each item in a list) so set up the dateTimeFormatter beforehand, e.g. instead of the _ds you remove. Yes, and merge the options into it.

::: mailnews/base/content/dateFormat.js
@@ +24,5 @@
>    try {
> +    const dateFormatter = new Intl.DateTimeFormat(undefined,
> +      { year: '2-digit', month: 'numeric', day: 'numeric' });
> +    var aDate = new Date(1999, 11, 1);
> +    var dateString = dateFormatter.format(aDate);

This doesn't need Intl.
The change probably does not make things worse, but ask myself whether the whole dateFormat.js is needed today. It analyses how the locale formats dates and then produces similar date strings when we ask it ( via convertDateToString), mimicking the locale format.
I see we may need to convert the string back to a JS date/time milliseconds (convertStringToPRTime). But I would like to see a bug about analysing if this is still needed. Maybe we could just store the seconds value at the callers and format them properly via Intl. not via convertDateToString.

::: mailnews/base/util/templateUtils.js
@@ +50,5 @@
>  {
> +  const dateFormatter = new Intl.DateTimeFormat(undefined,
> +    { year: '2-digit', month: 'numeric', day: 'numeric' });
> +  const timeFormatter = new Intl.DateTimeFormat(undefined,
> +    { hour: 'numeric', minute: 'numeric' });

OK, makeFriendlyDateAgo may be called in a loop, but then you need to setup dateFormatter and timeFormatter beforehand (at the initialization of the module, not in this function, to take advantage of Intl's internal caching. You may the want to convert all 4 branches (starting at 'if (end >= today)') to predefined formatters.
Attachment #8846656 - Flags: feedback+
Attached patch mail, v5 (obsolete) — Splinter Review
(In reply to Jorg K (GMT+2) from comment #3)
> As a general question: In bug 1332351 Aceman is using .toLocaleDateString()
> instead of Intl.DateTimeFormat.format(). The calendar guys are using
> Intl.DateTimeFormat.format() in attachment 8846433 [details] [diff] [review]
> [review]. Which one is preferred?
I switched the code Services.intl.createDateTimeFormat which caches the last used formatter and is preferred on mozilla-central now: https://dxr.mozilla.org/mozilla-central/rev/f40e24f40b4c4556944c762d4764eace261297f5/toolkit/components/mozintl/mozIntl.js#91

> ::: mail/components/preferences/cookies.js
> @@ +516,5 @@
> >      if (aExpires) {
> >        var date = new Date(1000 * aExpires);
> > +      const dtOptions = { year: 'numeric', month: 'long', day: 'numeric',
> > +                          hour: 'numeric', minute: 'numeric', second: 'numeric' };
> > +      const dateTimeFormatter = new Intl.DateTimeFormat(undefined, dtOptions);
> 
> Any reason to have a separate variable 'dtOptions'?
Inlined them.

(In reply to :aceman from comment #9)
> ::: mailnews/base/content/dateFormat.js
> @@ +24,5 @@
> >    try {
> > +    const dateFormatter = new Intl.DateTimeFormat(undefined,
> > +      { year: '2-digit', month: 'numeric', day: 'numeric' });
> > +    var aDate = new Date(1999, 11, 1);
> > +    var dateString = dateFormatter.format(aDate);
> 
> This doesn't need Intl.
> The change probably does not make things worse, but ask myself whether the
> whole dateFormat.js is needed today. It analyses how the locale formats
> dates and then produces similar date strings when we ask it ( via
> convertDateToString), mimicking the locale format.
> I see we may need to convert the string back to a JS date/time milliseconds
> (convertStringToPRTime). But I would like to see a bug about analysing if
> this is still needed. Maybe we could just store the seconds value at the
> callers and format them properly via Intl. not via convertDateToString.
Filed bug 1356242.
Attachment #8846656 - Attachment is obsolete: true
Attachment #8857963 - Flags: review?(jorgk)
Attachment #8857963 - Flags: review?(acelists)
Comment on attachment 8857963 [details] [diff] [review]
mail, v5

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

::: mail/components/activity/content/activity.xml
@@ +238,5 @@
>          <body>
>            <![CDATA[
> +            const dateTimeFormatter = Services.intl.createDateTimeFormat(undefined, {
> +              year: 'numeric', month: 'long', day: 'numeric',
> +              hour: 'numeric', minute: 'numeric'

Why don't we just use something like:
https://hg.mozilla.org/mozilla-central/rev/79673e659b31#l9.19
Services.intl.createDateTimeFormat(undefined,
+                             { dateStyle: "long",
+                               timeStyle: "short" });

::: mail/components/preferences/cookies.js
@@ +516,5 @@
>      if (aExpires) {
>        var date = new Date(1000 * aExpires);
> +      const dateTimeFormatter = Services.intl.createDateTimeFormat(undefined, {
> +        year: 'numeric', month: 'long', day: 'numeric',
> +        hour: 'numeric', minute: 'numeric', second: 'numeric'

And here. I've I'm not mistaken that goes through mozIMozIntl and even take the OS settings into account. And further down, too.
Comment on attachment 8857963 [details] [diff] [review]
mail, v5

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

Please do what Jorg says and I found some more nits.

::: mailnews/base/content/dateFormat.js
@@ +22,5 @@
>    gSearchDateLeadingZeros = true;
>  
>    try {
> +    const dateFormatter = Services.intl.createDateTimeFormat(undefined,
> +      { year: '2-digit', month: 'numeric', day: 'numeric' });

Services.intl.createDateTimeFormat(undefined,
                             { dateStyle: "short" }) ?

::: mailnews/base/util/templateUtils.js
@@ +35,5 @@
>    "chrome://messenger/locale/templateUtils.properties"
>  );
>  
> +const _dateFormatter = Services.intl.createDateTimeFormat(undefined,
> +  { year: '2-digit', month: 'numeric', day: 'numeric' });

Services.intl.createDateTimeFormat(undefined,
                             { dateStyle: "short" })

@@ +39,5 @@
> +  { year: '2-digit', month: 'numeric', day: 'numeric' });
> +const _dayMonthFormatter = Services.intl.createDateTimeFormat(undefined,
> +  { month: 'long', day: 'numeric' });
> +const _timeFormatter = Services.intl.createDateTimeFormat(undefined,
> +  { hour: 'numeric', minute: 'numeric' });

Services.intl.createDateTimeFormat(undefined,
                             { timeStyle: "short" })

@@ +83,3 @@
>    } else if (now.getFullYear() == end.getFullYear()) {
>      // activity must have been from some time ago.. show month/day
> +    dateTime = _timeFormatter.format(end);

Did you mean _dayMonthFormatter ?
Attachment #8857963 - Flags: review?(jorgk)
Attachment #8857963 - Flags: review?(acelists)
Attachment #8857963 - Flags: feedback+
Might this block (fix) bug 253883?
See Also: → 1376167
Friendly take-over, I'm just addressing the nits here and merging my fix from bug 1356403 which is also in mail/.
Assignee: aryx.bugmail → jorgk
Mentor: acelists
Summary: Use Intl.DateTimeFormat or similar instead of nsIScriptableDateFormat in Thunderbird → Replace nsIScriptableDateFormat in mail/ and mailnews/
Duplicate of this bug: 1356403
OK, I addressed the comments, and added a hunk from bug 1356403. I left the original author in place.
Attachment #8857963 - Attachment is obsolete: true
Attachment #8881526 - Flags: review?(acelists)
Comment on attachment 8881526 [details] [diff] [review]
1346549-replace-scriptable-date-in-mail-mailnews.patch (JK v1).

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

Thanks.

::: mail/components/addrbook/content/abCardViewOverlay.js
@@ +284,5 @@
>          // due to time zone/dst discontinuity
>          date = new Date(Date.UTC(year, month - 1, day));
>          date.setUTCFullYear(year); // to handle two-digit years properly
> +        formatter = Services.intl.createDateTimeFormat(undefined,
> +                      { dateStyle: "long" });

Can we keep the 'timeZone: "UTC"' here? I think it it part of the feature. Does createDateTimeFormat() support the property?
Attachment #8881526 - Flags: review?(acelists) → review+
I think it does, at least Javi claims it does ;-)
https://hg.mozilla.org/try-comm-central/rev/4e8d50eb86630a093c0602c6d261e398b9c3273e#l1.59

What's the UTC for? Does it make a difference in the display?
Added |timeZone: "UTC"| back in.
Attachment #8881526 - Attachment is obsolete: true
Attachment #8882708 - Flags: review+
(In reply to Jorg K (GMT+2) from comment #18)
> What's the UTC for? Does it make a difference in the display?
The comments say it is important. Also we create new Date(Date.UTC()) so the dates are not in local time but UTC. Then displaying them in local zone will not show the date we intended, but shifted by some hours due to local timezone.
See e.g. bug 1139167.
https://hg.mozilla.org/comm-central/rev/a78c4ac9f53cbb73b6dede90c3a5bb8f80e267af

Aceman, thanks for catching this.
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 56.0
Attachment #8882708 - Flags: approval-comm-beta+
With this deprecation, do I now need a version check to decide which interface to use? Or are the other interfaces all the way back to, say, TB 1.0?
Also, what about Seamonkey?
Nothing goes back to TB 1.0. I believe that the interfaces you need to use are available in TB 52 ESR or mozilla52 and the equivalent SM version of 2.49. Nothing before TB 52 is currently supported.
You need to log in before you can comment on or make changes to this bug.