Closed Bug 1380799 Opened 2 years ago Closed 2 years ago

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

Categories

(Thunderbird :: Instant Messaging, defect)

defect
Not set

Tracking

(thunderbird_esr52 unaffected, thunderbird56 wontfix, thunderbird57 fixed, thunderbird58 fixed)

RESOLVED FIXED
Thunderbird 58.0
Tracking Status
thunderbird_esr52 --- unaffected
thunderbird56 --- wontfix
thunderbird57 --- fixed
thunderbird58 --- fixed

People

(Reporter: jorgk, Assigned: jorgk)

References

Details

Attachments

(1 file, 2 obsolete files)

+++ 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.
(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.
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.
Attached patch 1380799-chat-logging.patch (obsolete) — Splinter Review
The same code we've used a million times, so checking that it works is good enough as review here.
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Attachment #8886687 - Flags: review?(richard.marti)
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
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?
Flags: needinfo?(florian)
Flags: needinfo?(clokep)
(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 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.
Attachment #8886687 - Flags: review?(richard.marti)
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"?
Not found in ISC and Twitter logs. This could be for a protocol which is for user to user communication like Google Talk.
(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).
(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.
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.
(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.
(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.
Flags: needinfo?(florian)
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.
Attached patch 1380799-chat-logging.patch (v2). (obsolete) — Splinter Review
Here's my proposal. Richard can also check that his logs now follow his en-GB settings.
Attachment #8886687 - Attachment is obsolete: true
Attachment #8887024 - Flags: review?(florian)
Attachment #8887024 - Flags: feedback?(richard.marti)
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.
Attachment #8887024 - Flags: feedback?(richard.marti) → feedback-
This works now, I tried it ;-) My system log shows:
System log for account JK-test@irc.mozilla.org (irc) connected at 17 July 2017, 20:10:21
---- +++ JK-test@irc.mozilla.org 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.
Attachment #8887024 - Attachment is obsolete: true
Attachment #8887024 - Flags: review?(florian)
Attachment #8887163 - Flags: review?(florian)
Attachment #8887163 - Flags: feedback?(richard.marti)
Comment on attachment 8887163 [details] [diff] [review]
1380799-chat-logging.patch (v2b).

Log format looks good now.
Attachment #8887163 - Flags: feedback?(richard.marti) → feedback+
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.
Attachment #8887163 - Flags: review?(clokep)
Attachment #8887163 - Flags: review?(florian)
Attachment #8887163 - Flags: review?(clokep)
Attachment #8887163 - Flags: review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/5298286c9521
use mozIntl in chat's logging and ToLocaleFormat.jsm. r=clokep
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 58.0
Comment on attachment 8887163 [details] [diff] [review]
1380799-chat-logging.patch (v2b).

[Triage Comment]
Flags: needinfo?(clokep)
Attachment #8887163 - Flags: approval-comm-beta+
You need to log in before you can comment on or make changes to this bug.