Closed Bug 1444480 Opened 2 years ago Closed 2 years ago

Omit time zone in timestamps

Categories

(Thunderbird :: Instant Messaging, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 60.0

People

(Reporter: freaktechnik, Assigned: freaktechnik)

Details

Attachments

(2 files, 6 obsolete files)

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.
Attached patch bug1444480.patch (obsolete) — Splinter Review
Attachment #8957634 - Flags: review?(clokep)
Status: NEW → ASSIGNED
Attached patch bug1444480-v2.patch (obsolete) — Splinter Review
Uses a module-global lazy getter for the formatter, as florian suggested on IRC.
Attachment #8957634 - Attachment is obsolete: true
Attachment #8957634 - Flags: review?(clokep)
Attachment #8957638 - Flags: review?(florian)
Attached patch bug1444480-v3.patch (obsolete) — Splinter Review
This follows the changes of https://hg.mozilla.org/comm-central/rev/2bc6c38ccf68 and passes the list of OS date locales to the formatters.
Attachment #8957638 - Attachment is obsolete: true
Attachment #8957638 - Flags: review?(florian)
Attachment #8957687 - Flags: review?(florian)
Comment on attachment 8957687 [details] [diff] [review]
bug1444480-v3.patch

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

Looks great!
Attachment #8957687 - Flags: review?(florian) → review+
Keywords: checkin-needed
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.
Please answer my questions in comment #5 before checking this in.
Keywords: checkin-needed
(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.
(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 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.
(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.
> 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.
(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" ;-)
Attached patch bug1444480-v4.patch (obsolete) — Splinter Review
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)
Attachment #8957687 - Attachment is obsolete: true
Attachment #8957895 - Flags: review?(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.
Attachment #8957895 - Flags: review?(florian) → review+
Attached patch bug1444480-v5.patch (obsolete) — Splinter Review
Carrying over r+, since the only change is to revert the change mentioned in the review.
Attachment #8957895 - Attachment is obsolete: true
Attachment #8957905 - Flags: review+
Attached patch bug1444480-v5.1.patch (obsolete) — Splinter Review
Noticed that I hadn't updated the commit message after uploading. Oops.
Attachment #8957905 - Attachment is obsolete: true
Attachment #8957906 - Flags: review+
Keywords: checkin-needed
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().
Please change the en-US to 'undefined' and fix the function name or give me the permission to do that when checking it in.
Keywords: checkin-needed
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.
Attachment #8957906 - Attachment is obsolete: true
Attachment #8957947 - Flags: review+
Keywords: checkin-needed
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/3cc1812a10b4
Omit timezone in some timestamps. r=florian
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Component: General → Instant Messaging
Product: Chat Core → Thunderbird
Target Milestone: --- → Thunderbird 60.0
Version: trunk → Trunk
You need to log in before you can comment on or make changes to this bug.