1380799 - Investigate the remaining uses of toLocale[Date|Time]String() in chat/ and replace with mozIntl

Status: RESOLVED FIXED

(Thunderbird :: Instant Messaging, defect)

Description

[Jorg K (CEST = GMT+2)]

+++ This bug was initially created as a clone of Bug #1313659 +++

In bug 1313659 I removed some uses of toLocale[Date|Time]String() which were visible in the UI, but there are still some left:

https://dxr.mozilla.org/comm-central/search?q=path%3Achat+tolocale&redirect=false

Particularly in logging (chat/components/src/logger.js) and in ToLocaleFormat.jsm which can apparently produce custom formats. I don't know how to configure those.

Comment 1

[Zibi Braniecki [:zbraniecki][:gandalf]]

(In reply to Jorg K (GMT+2) from comment #0)
> Particularly in logging (chat/components/src/logger.js) and in
> ToLocaleFormat.jsm which can apparently produce custom formats. I don't know
> how to configure those.

It seems to me that code in logger.js is not really internationalizing the dates. It's just constructing non-international ones.

So sth like `${date.getYear()}-${date.getMonth()}-${date.getDay()}` is completely appropriate.

Comment 2

[Jorg K (CEST = GMT+2)]

Sure, but I was referring to:
" at " + (new Date(this._conv.startDate / 1000)).toLocaleString() +
let line = "(" + date.toLocaleTimeString() + ") ";

And that's potentially or almost certainly not right.

Comment 3

[Jorg K (CEST = GMT+2)]

Attachment: 1380799-chat-logging.patch

The same code we've used a million times, so checking that it works is good enough as review here.

Comment 4

[Richard Marti (:Paenglab)]

Hmm, I was wrong with my assumption it's used for the system log files. This one looks to be used for system log files: https://dxr.mozilla.org/comm-central/source/chat/components/src/logger.js#331

Comment 5

[Jorg K (CEST = GMT+2)]

One could argue whether chat/modules/ToLocaleFormat.jsm
126 x: () => aDate.toLocaleDateString(locale),
127 X: () => aDate.toLocaleTimeString(locale),
should use mozIntl or not.

The description says:
http://pubs.opengroup.org/onlinepubs/007908799/xsh/strftime.html
%x is replaced by the locale's appropriate date representation.
%X is replaced by the locale's appropriate time representation.

The code does that, so maybe just leave this code alone, especially given that no one knows how to trigger it. Read here for more details: Bug 1332351 comment #16 (quote):
"Since message styles can use any strftime/toLocaleFormat format they like, ...".
Does anyone know how that time format is configured?

Comment 6

[Jorg K (CEST = GMT+2)]

(In reply to Richard Marti (:Paenglab) from comment #4)
> Hmm, I was wrong with my assumption it's used for the system log files. This
> one looks to be used for system log files:
> https://dxr.mozilla.org/comm-central/source/chat/components/src/logger.js#331
OK, that uses %c which is implemented as
c: () => aDate.toLocaleString(locale, localeStringOptions),
Another candidate for replacement (or not) like %x and %X, see previous comment.

Comment 7

[Richard Marti (:Paenglab)]

Comment on attachment 8886687 [details] [diff] [review]
1380799-chat-logging.patch

I remove the r? as I can't test this patch with my actual chat accounts.

Comment 8

[Jorg K (CEST = GMT+2)]

So you have no idea where
https://dxr.mozilla.org/comm-central/source/chat/components/src/logger.js#200
      header = "Conversation with " + this._conv.name +
               " at " + (new Date(this._conv.startDate / 1000)).toLocaleString() +
               " on " + account.name +
               " (" + account.protocol.normalizedName + ")" + kLineBreak;
is executed? Have you searched through your logs for "Conversation with"?

Comment 9

[Richard Marti (:Paenglab)]

Not found in ISC and Twitter logs. This could be for a protocol which is for user to user communication like Google Talk.

Comment 10

[Florian Quèze [:florian]]

(In reply to Jorg K (GMT+2) from comment #8)
> So you have no idea where
> https://dxr.mozilla.org/comm-central/source/chat/components/src/logger.js#200
>       header = "Conversation with " + this._conv.name +
>                " at " + (new Date(this._conv.startDate /
> 1000)).toLocaleString() +
>                " on " + account.name +
>                " (" + account.protocol.normalizedName + ")" + kLineBreak;
> is executed? Have you searched through your logs for "Conversation with"?

This is the old-style plain text logger. It will be used if the purple.logging.format preference is set to "txt" (we seem to be logging in json format by default for both Thunderbird and Instantbird, so this is almost dead code at this point).

Comment 11

[Florian Quèze [:florian]]

(In reply to Jorg K (GMT+2) from comment #2)
> Sure, but I was referring to:
> " at " + (new Date(this._conv.startDate / 1000)).toLocaleString() +
> let line = "(" + date.toLocaleTimeString() + ") ";
> 
> And that's potentially or almost certainly not right.

This code that mixes a hard coded English string with a localized date indeed feels broken.
This plain text logger was (initially) meant to reproduce the Pidgin logging format, so either it's a bug in our implementation, or it's a bug in theirs that we tried to imitate.

Given that this code isn't used anymore (unless some users are playing in about:config), I think we can take the attached patch without thinking about this further.

Comment 12

[Jorg K (CEST = GMT+2)]

Thanks. So we change logging at #200 as per the patch.

Do you have any opinion whether to move this other one:
https://dxr.mozilla.org/comm-central/source/chat/components/src/logger.js#338
using %c and chat/modules/ToLocaleFormat.jsm
126 x: () => aDate.toLocaleDateString(locale),
127 X: () => aDate.toLocaleTimeString(locale),
to mozIntl or not?

Still unanswered, how to use ToLocaleFormat.jsm, so where to configure a date/time using, say, %x.

Comment 13

[Florian Quèze [:florian]]

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

> "Since message styles can use any strftime/toLocaleFormat format they like,
> ...".
> Does anyone know how that time format is configured?

Message styles can provide a format parameter for the timeOpened and time replacements in templates.

This is documented at https://developer.mozilla.org/en-US/docs/Mozilla/Chat_Core/Message_Styles

Eg. it's possible for a message theme to contain %timeOpened{%H:%M:%S}% rather than %timeOpened%.

This is a rarely used feature, but we had to support it for compatibility with Adium message styles.

Comment 14

[Florian Quèze [:florian]]

(In reply to Jorg K (GMT+2) from comment #12)

> Do you have any opinion whether to move this other one:
> https://dxr.mozilla.org/comm-central/source/chat/components/src/logger.js#338
> using %c and chat/modules/ToLocaleFormat.jsm
> 126 x: () => aDate.toLocaleDateString(locale),
> 127 X: () => aDate.toLocaleTimeString(locale),
> to mozIntl or not?

I don't think we currently have any code attempting to parse these system logs, and it again seems to be a kinda broken situation with hard coded English strings but localized dates, so I don't think it matters much either way. But I wasn't involved in the creation of ToLocaleFormat.jsm / bug 1332351, so I would need to dig further to have an opinion. I hope clokep remembers.

Comment 15

[Jorg K (CEST = GMT+2)]

Let's summarise:
The logging at https://dxr.mozilla.org/comm-central/source/chat/components/src/logger.js#200 uses hard-coded English with a localised date/time: date.toLocaleTimeString().
That's wrong, it should use "en-US", either via toLocaleTimeString() or mozIntl as in the patch.

The logging at https://dxr.mozilla.org/comm-central/source/chat/components/src/logger.js#338 uses hard-coded English with a localised date/time %c.
That's wrong, %c (http://pubs.opengroup.org/onlinepubs/007908799/xsh/strftime.html) will always be localised, so that should be replaced with either via toLocaleTimeString("en-US", ...) or mozIntl.

So the decision we need to make is whether to use toLocaleTimeString() or mozIntl:

If we use toLocaleTimeString() #200 and #338 should both use toLocaleTimeString("en-US", ...) and we leave ToLocaleFormat.jsm as is.

If we use mozIntl, line #200 needs to change as per the patch, equally #338 (fixed "en-US") and ToLocaleFormat.jsm needs to change for %c, %x and %X.

Either way, changes need to be made to make things consistent.

Personally I'm in favour of using mozIntl since M-C is working on enhancing that, whereas the stock-standard toLocaleTimeString() will not give any configuration options.

At the time of creating ToLocaleFormat.jsm, mozIntl wasn't available, but since its creation, FF has moved all chrome code there, and that's what we should do.

Comment 16

[Jorg K (CEST = GMT+2)]

Attachment: 1380799-chat-logging.patch (v2).

Here's my proposal. Richard can also check that his logs now follow his en-GB settings.

Comment 17

[Richard Marti (:Paenglab)]

Comment on attachment 8887024 [details] [diff] [review]
1380799-chat-logging.patch (v2).

Unfortunately I have to f- this patch. There is no log file written anymore and the tweets during the test are lost (not severe, I read them before ;-) ). In the console I see this:

ReferenceError: Services is not defined[Learn More]  ToLocaleFormat.jsm:83:7

This comes with IRC too.

Also without this patch I get this error:

ReferenceError: reference to undefined property "fill"[Learn More]  ToLocaleFormat.jsm:163:11

With IRC too.

Comment 18

[Jorg K (CEST = GMT+2)]

Attachment: 1380799-chat-logging.patch (v2b).

This works now, I tried it ;-) My system log shows:
System log for account [e-mail] (irc) connected at 17 July 2017, 20:10:21
---- +++ [e-mail] signed on @ 17 July 2017 20:10:21 ----

Sorry for the additional round.

Re. the ReferenceError: reference to undefined property "fill" ToLocaleFormat.jsm:163:11
That's this line:
  let {fill = "", width = 0} = padding[specifier] || {};
now moved to line 167 with the patch.

That's not my fault and if you still see it, we need to ask Florian how to fix that. I don't even understand the JS syntax here.

Comment 19

[Richard Marti (:Paenglab)]

Comment on attachment 8887163 [details] [diff] [review]
1380799-chat-logging.patch (v2b).

Log format looks good now.

Comment 20

[Jorg K (CEST = GMT+2)]

Comment on attachment 8887163 [details] [diff] [review]
1380799-chat-logging.patch (v2b).

Maybe Patrick can take the review if Florian is too busy. This is really code we have dozens of times in C-C.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/5298286c9521
use mozIntl in chat's logging and ToLocaleFormat.jsm. r=clokep

Comment 22

[Jorg K (CEST = GMT+2)]

Comment on attachment 8887163 [details] [diff] [review]
1380799-chat-logging.patch (v2b).

[Triage Comment]

Comment 23

[Jorg K (CEST = GMT+2)]

Beta (TB 57):
https://hg.mozilla.org/releases/comm-beta/rev/624e38d6b4e3045ab5cffedc4470ca16ce062ef2