1444480 - Omit time zone in timestamps

Status: RESOLVED FIXED

(Thunderbird :: Instant Messaging, enhancement)

Description

[Martin Giger [:freaktechnik]]

Attachment: timestamps_before_after.png

Currently chat timestmaps include the timezone. The attached patch removes that and restores the "old" behavior. See screenshots for comparison.

The patch also caches the formatter, so it isn't created for every message.

Comment 1

[Martin Giger [:freaktechnik]]

Attachment: bug1444480.patch

Comment 2

[Martin Giger [:freaktechnik]]

Attachment: bug1444480-v2.patch

Uses a module-global lazy getter for the formatter, as florian suggested on IRC.

Comment 3

[Martin Giger [:freaktechnik]]

Attachment: bug1444480-v3.patch

This follows the changes of https://hg.mozilla.org/comm-central/rev/2bc6c38ccf68 and passes the list of OS date locales to the formatters.

Comment 4

[Florian Quèze [:florian]]

Comment on attachment 8957687 [details] [diff] [review]
bug1444480-v3.patch

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

Looks great!

Comment 5

[Jorg K (CEST = GMT+2)]

Comment on attachment 8957687 [details] [diff] [review]
bug1444480-v3.patch

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

Sorry about the unsolicited comments, but I'm suffered through all those date/time formatting changes :-(

I think you can lose the hunks to do with gDateLocales and just use 'undefined' (fixing the 'locale' left-over).

::: chat/content/imtooltip.xml
@@ +490,5 @@
>             if ((new Date()).toDateString() == date.toDateString()) {
> +             const dateTimeFormatter = new Services.intl.DateTimeFormat(this.gDateLocales, {
> +               hour: "2-digit",
> +               minute: "2-digit",
> +               second: "2-digit"

So before we used "long" and now we specify hh:mm:ss. I guess the timezone is only appended for some platforms, I don't see it on Windows. I guess "short" would only give hh:mm. Have you tried "medium"?

@@ +495,5 @@
>               });
>               text = dateTimeFormatter.format(date);
>             }
>             else {
> +             const dateTimeFormatter = new Services.intl.DateTimeFormat(this.gDateLocales, {

Did 'undefined' not work? That falls back to whichever local the rest of the UI is using.

::: chat/modules/ToLocaleFormat.jsm
@@ +29,5 @@
>    function DayWithinYear(t) {
>      return Day(t) - DayFromYear(t.getFullYear());
>    }
>    function weekday(option) {
> +    return aDate.toLocaleString(gDateLocales, {weekday: option});

Wow, I removed 'locale' here and left these in place :-((
https://hg.mozilla.org/comm-central/rev/5298286c9521#l2.27
Sorry about that. But would 'undefined' not work like in the other places? We use 'undefined' everywhere in TB, so I'd like to avoid to introduce different behaviour here.

Also, TB has "Tools > Options, Advance, General, Date and Time Formatting", where you can chose between application locale and regional settings locale which exposes the intl.regional_prefs.use_os_locales preference.

I'm not sure what your patch does, but I have the impression that chat will now always use the regional settings which gets it out of sync with the rest of TB.

Comment 6

[Jorg K (CEST = GMT+2)]

Please answer my questions in comment #5 before checking this in.

Comment 7

[Florian Quèze [:florian]]

(In reply to Jorg K (GMT+1) from comment #5)

> Also, TB has "Tools > Options, Advance, General, Date and Time Formatting",
> where you can chose between application locale and regional settings locale
> which exposes the intl.regional_prefs.use_os_locales preference.

Ah, we didn't know that.

Comment 8

[Florian Quèze [:florian]]

(In reply to Jorg K (GMT+1) from comment #5)

> ::: chat/content/imtooltip.xml
> @@ +490,5 @@
> >             if ((new Date()).toDateString() == date.toDateString()) {
> > +             const dateTimeFormatter = new Services.intl.DateTimeFormat(this.gDateLocales, {
> > +               hour: "2-digit",
> > +               minute: "2-digit",
> > +               second: "2-digit"
> 
> So before we used "long" and now we specify hh:mm:ss. I guess the timezone
> is only appended for some platforms, I don't see it on Windows. I guess
> "short" would only give hh:mm. Have you tried "medium"?

medium works for me on Mac. I think Martin was seeing the timezone on Linux.

Comment 9

[Florian Quèze [:florian]]

Comment on attachment 8957687 [details] [diff] [review]
bug1444480-v3.patch

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

::: chat/content/imtooltip.xml
@@ +500,1 @@
>                 dateStyle: "short", timeStyle: "long"

This 'timeStyle: "long"' also needs to be changed.

Comment 10

[Jorg K (CEST = GMT+2)]

(In reply to Jorg K (GMT+1) from comment #5)
> Sorry about the unsolicited comments, but I'm suffered through all those
> date/time formatting changes :-(
Typo: "I've suffered", last here: https://hg.mozilla.org/mozilla-central/rev/9fd5dc9d8245#l1.13
That was a bug where two time zones showed, at least on Linux.

(In reply to Florian Quèze [:florian] from comment #8)
> medium works for me on Mac. I think Martin was seeing the timezone on Linux.
Yes, Linux adds the time zone. Maybe if medium works on Mac, it might also work on Linux if the comment quoted above is correct.

Comment 11

[Martin Giger [:freaktechnik]]

> Sorry about the unsolicited comments

Thank you for those, actually.

> So before we used "long" and now we specify hh:mm:ss. I guess the timezone is only appended for some platforms, I don't see it on Windows. I guess "short" would only give hh:mm. Have you tried "medium"?

I do not see any advantage over using the timeStyle shorthand. We want a specific time-format and thus we request that very format. Especially since timeStyle is not a standardize property of Intl, while those are, and thus I'd say the code is more robust. I guess this is about the user being able to choose their date and time formats on the platform? I'm very much in favor of enforcing fixed-width timestamps for chat (logs) though, so they line up and take up an even amount of space.

> Also, TB has "Tools > Options, Advance, General, Date and Time Formatting", where you can chose between application locale and regional settings locale which exposes the intl.regional_prefs.use_os_locales preference.

As florian already said, we were not aware of that setting and were indeed trying to get that behavior, since it is the current behavior in chat.

Comment 12

[Jorg K (CEST = GMT+2)]

(In reply to Martin Giger [:freaktechnik] from comment #11)
> I do not see any advantage over using the timeStyle shorthand.
That's you call, I don't insist on a "shorthand" ;-)

Comment 13

[Jorg K (CEST = GMT+2)]

"your call", of course.

Comment 14

[Martin Giger [:freaktechnik]]

Attachment: bug1444480-v4.patch

This removes the OS preference stuff, as it can be toggled in a user-facing place.

It also uses "medium" timeStyles for most timestamps, except:
 - Chat logs are fixed to 2-digit representations of hour, minute and second.
 - Debug logs get a long timeStyle, so timezones are included.
 - ToLocaleFormat is wizardry that I want to touch as little as possible (I did change the locale references Jörg missed to undefined though)

Comment 15

[Florian Quèze [:florian]]

Comment on attachment 8957895 [details] [diff] [review]
bug1444480-v4.patch

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

Thanks for updating the patch! :-) (note: I didn't test this version of the patch locally)

::: chat/modules/ToLocaleFormat.jsm
@@ +75,5 @@
>        Math.floor(Math.abs(offset) / 60) * 100 + Math.abs(offset) % 60;
>      return (offset < 0 ? "+" : "-") + String(tzoff).padStart(4, "0");
>    }
>    function timeZone() {
> +    let dtf = Intl.DateTimeFormat(undefined, {timeZoneName: "short"});

I don't really know what this code does, but given that it was hard coding the en-US locale, I'm tempted to err on the side of caution and assume it had a good reason to do so (eg maybe some JS code expects to be able to parse the result against a predefined set of constants). I would revert the change on this line.

Comment 16

[Martin Giger [:freaktechnik]]

Attachment: bug1444480-v5.patch

Carrying over r+, since the only change is to revert the change mentioned in the review.

Comment 17

[Martin Giger [:freaktechnik]]

Attachment: bug1444480-v5.1.patch

Noticed that I hadn't updated the commit message after uploading. Oops.

Comment 18

[Jorg K (CEST = GMT+2)]

Comment on attachment 8957906 [details] [diff] [review]
bug1444480-v5.1.patch

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

Please excuse further unsolicited comments :-(

(In reply to Florian Quèze [:florian] from comment #15)
> ::: chat/modules/ToLocaleFormat.jsm
> @@ +75,5 @@
> >        Math.floor(Math.abs(offset) / 60) * 100 + Math.abs(offset) % 60;
> >      return (offset < 0 ? "+" : "-") + String(tzoff).padStart(4, "0");
> >    }
> >    function timeZone() {
> > +    let dtf = Intl.DateTimeFormat(undefined, {timeZoneName: "short"});
> 
> I don't really know what this code does, but given that it was hard coding
> the en-US locale, I'm tempted to err on the side of caution and assume it
> had a good reason to do so (eg maybe some JS code expects to be able to
> parse the result against a predefined set of constants). I would revert the
> change on this line.

Can I make a point for changing this to 'undefined' here. The whole point of the library is to provide date/time formatting to chat since somewhere chat can configure the formats, although I never found out where.

Elsewhere in the library we use 'undefined' like here:
  function dayPeriod() {
    let dtf = Intl.Dateformat(undefined, {hour: "2-digit"}); // Intl.Dateformat is actually a bug :-(
so the AM/PM stuff comes out as localised, or doesn't come out at all if the locale doesn't have that. Try
Intl.DateTimeFormat("en", {hour: "2-digit"}).formatToParts(Date.now())[2];
Intl.DateTimeFormat("de", {hour: "2-digit"}).formatToParts(Date.now())[2];
in the console. The first one gives "PM", the second one undefined since the Germans don't do AM/PM or equivalent.

Now try
Intl.DateTimeFormat("en-US", {timeZoneName: "short"}).formatToParts(Date.now())[6];
Intl.DateTimeFormat("de", {timeZoneName: "short"}).formatToParts(Date.now())[6];
you get GMT+1 and MEZ.

So if chat users configure their own time format, I assume that they want the time zone name to match their language and not en-US.

I've checked and the library has always been like this from day one when Aleth imported it in bug 1332351, but is has another bug which you will see below.

::: chat/modules/ToLocaleFormat.jsm
@@ +33,5 @@
>        hour: "2-digit", minute: "2-digit", second: "2-digit"
>      });
>    }
>    function dayPeriod() {
> +    let dtf = Intl.Dateformat(undefined, {hour: "2-digit"});

This function doesn't exist, it's Intl.DateTimeFormat().

Comment 19

[Jorg K (CEST = GMT+2)]

Please change the en-US to 'undefined' and fix the function name or give me the permission to do that when checking it in.

Comment 20

[Martin Giger [:freaktechnik]]

Attachment: bug1444480-v5.2.patch

Alright, corrected the usage of internals. We looked into the usage of the code and turns out those bits are unused by code in c-c either way, so doesn't matter much.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/3cc1812a10b4
Omit timezone in some timestamps. r=florian