1325745 - Port bug 1301640 to mailnews. Remove nsIDateTimeFormat.h etc. and adapt calls

Status: RESOLVED FIXED

(MailNews Core :: Database, defect)

Comment 1

[Jorg K (CEST = GMT+2)]

Attachment: 1301640-tb-nsIDateTimeFormat-bustage.patch

This seems to compile so far.

Sadly I had to remove pref mailnews.reply_header_locale since the new mozilla::DateTimeFormat::FormatPRTime() lost its 'locale' parameter.

Comment 2

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/a6298203de72b408d580f604e9a336d2b70068bf
pushed this as immediate bustage fix, post-landing reviews welcome.

Looking at the date format, I'm not happy at all.

Before passing locale==nullptr gave me a date format as configured for my local machine. Now I get the locale of the built, which is en-US.

In the thread pane, before:
Received: 23/12/2016 22:42.

Now:
Received: 23/12/2016, 10:42 pm.

I will complain over in bug 1301640.

Comment 3

[Jorg K (CEST = GMT+2)]

See bug 1301640 comment #104.

Comment 4

[Jorg K (CEST = GMT+2)]

Raised bug 1325751 for the formatting issue.

Comment 5

[aleth [:aleth]]

(In reply to Jorg K (GMT+1) from comment #2)
> post-landing reviews welcome.

That's not very useful as a bug comment - it's better to request review from someone. This is enough of a code change to warrant review, if nothing else to help make sure nothing got overlooked.

Comment 6

[aleth [:aleth]]

(In reply to Jorg K (GMT+1) from comment #2)
> Looking at the date format, I'm not happy at all.
> 
> Before passing locale==nullptr gave me a date format as configured for my
> local machine. Now I get the locale of the built, which is en-US.
> 
> In the thread pane, before:
> Received: 23/12/2016 22:42.
> 
> Now:
> Received: 23/12/2016, 10:42 pm.
> 
> I will complain over in bug 1301640.

That's indeed strange, as even if you're American you may want to use e.g. 24h time display instead of 12h am/pm.

Comment 7

[aleth [:aleth]]

(In reply to aleth [:aleth] from comment #6)
> That's indeed strange, as even if you're American you may want to use e.g.
> 24h time display instead of 12h am/pm.

Looks like bug 1301640 has a lot of discussion about this, I guess I should have read that first ;)

Comment 8

[Jorg K (CEST = GMT+2)]

Comment on attachment 8821715 [details] [diff] [review]
1301640-tb-nsIDateTimeFormat-bustage.patch

OK, let's do a post-landing review here. Whoever comes first ;-) - The changes are straight forward.

Since mozilla::DateTimeFormat::FormatPRTime() lost its 'locale' parameter, I had to remove preference mailnews.reply_header_locale. Not a big loss, just one confusing and undocumented option less. Due to that, the changes in nsMsgCompose.cpp look complicated since two levels of indentation were removed.

(In reply to aleth [:aleth] from comment #5)
> it's better to request review from someone. This is enough of a code change
> to warrant review, if nothing else to help make sure nothing got overlooked.
Very hard to overlook something here. Since two include files were removed and the function calls changed, the compiler found all spots that needed touching.

Comment 9

[Jorg K (CEST = GMT+2)]

(In reply to aleth [:aleth] from comment #7)
> Looks like bug 1301640 has a lot of discussion about this, I guess I should
> have read that first ;)
Very hard to read 100 comments when you have to deal with bustage in limited time. So I made it my Christmas Eve reading. Glancing over it, I can't make any sense of it. Here's what I found:

Bug 1301640 comment #7 already warns that ICU doesn't give 12/24h format or honor any OS specific configuration. Similar comment in bug 1301640 comment #15. In bug 1301640 comment #18 Henri confirmed that you can configure Windows to show YYYY-MM-DD. And then the discussion drifts away, from about cmt 45 they start discussing the code and in the end they land something that loses existing functionality. I don't get it.

Comment 10

[Jorg K (CEST = GMT+2)]

Comment on attachment 8821715 [details] [diff] [review]
1301640-tb-nsIDateTimeFormat-bustage.patch

Note that bug 1301640 has been backed out for now:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6882a49e907bd01ab7b782a0985614f4565e02e
When the merge to M-C happens, I will back out:
https://hg.mozilla.org/comm-central/rev/a6298203de72b408d580f604e9a336d2b70068bf

Comment 11

[Richard Marti (:Paenglab)]

Backed out after back out of m-c bug.
https://hg.mozilla.org/comm-central/rev/b5cbf4a5f294c84cee905a0e22c32ea34cc66359

Comment 12

[Jorg K (CEST = GMT+2)]

Attachment: 1301640-tb-nsIDateTimeFormat-bustage.patch - refreshed [landed, comment #13]

Refreshed to latest trunk.

Bug 1301640 will land again, so if anyone is interested in reviewing this, here's your chance.

Note that this patch was already in production and was backed out when bug 1301640 was backed out.

Comment 13

[Jorg K (CEST = GMT+2)]

Relanded:
https://hg.mozilla.org/comm-central/rev/2231f130a183eae79746df4da367062d0ad04a9d

Comment 14

[Alfred Peters [:infofrommozilla]]

(In reply to Jorg K (GMT+1) from comment #13)
> Relanded:
> https://hg.mozilla.org/comm-central/rev/
> 2231f130a183eae79746df4da367062d0ad04a9d

I still miss the short date format in the date column of my TB.
With the old versions of the patches the date was OK.

Comment 15

[Jorg K (CEST = GMT+2)]

(In reply to Alfred Peters from comment #14)
> I still miss the short date format in the date column of my TB.
> With the old versions of the patches the date was OK.
Sorry, I don't understand what you're trying to say.

On Windows, TB uses the short date and time as defined by the Windows "format locale":
"Control Panel > Region and Language, Formats". Individual modifications applied to the "format locale" are now ignored. Details in bug 1308329 comment #30.

There is only one patch and there is only one version. The patch was landed on 24th Dec. 2016 and backed out on the 26th and relanded on the 6th of January 2017. So in Daily of the 25th you can see the same behaviour you see now after the 6th of January 2017. I'm using it now and it works fine.

Comment 16

[Hartmut Figge]

Attachment: top with backout, bottom with normal source

The problem with the date was gone. Until today. I had to backout your patch and because of a bustage the other, connected one, too.

On SM Linux x86_64 now the problem is back. The picture is taken from de.test. The top shows the date column after the backout of the two patches, the bottom shows the original without the backouts.

Comment 17

[Alfred Peters [:infofrommozilla]]

Attachment: Broken date column in windows TB

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

> on 24th Dec. 2016 and backed out on the 26th and relanded on the 6th of
> January 2017. So in Daily of the 25th you can see the same behaviour you see
> now after the 6th of January 2017. I'm using it now and it works fine.

Correct.

Comment 18

[Jorg K (CEST = GMT+2)]

What can I say. In bug 1301640 M-C changed the C++ call interface to format dates and times. We had not option but to follow.

Now that bug 1301640 has landed again, the format on Windows works as described in bug 1308329 comment #30. That's where I posted a summary, since bug 1301640 got too long an confusing.

On Windows it works well for me. I can start my Linux machine and look how it looks there.

In principle you need to take the issue up with the M-C people in bug 1301640. They don't care about TB or SM, they only care about FF. FF uses C++ formatted date/times using ICU/CLDR in the details of a certificate. So I suggest you compile or download a Nightly FF and check the dates which are displayed in the details of the certificates.

Richard, you have Mac and Linux, how does the date column look like there on current trunk?

Comment 19

[Hartmut Figge]

Attachment: Inconsistency in the date format

Something is strange. Some dates show correct values and others don't. Hm.

Comment 20

[Hartmut Figge]

First thing with oddities should be to try a fresh profile. Well, I cannot reproduce the problem there. I now assume that some strangeness is triggered in my standard profile by one of the two checkins.

So the best way would be to forget about my comments. :)

Comment 21

[Richard Marti (:Paenglab)]

Date column on Linux and macOS looks like before with today builds.

Comment 22

[Alfred Peters [:infofrommozilla]]

(In reply to Hartmut Figge from comment #20)
> First thing with oddities should be to try a fresh profile. Well, I cannot
> reproduce the problem there. I now assume that some strangeness is triggered
> in my standard profile by one of the two checkins.

Indeed!
I tried 'Add-ons Disabled' -> date still broken.
I checked the prefs and found 'mail.ui.display.dateformat.today' -> deleted -> date OK!
I reset the pref (string - value 4) -> date still OK!

This is a bit strange!

Comment 23

[Alfred Peters [:infofrommozilla]]

(In reply to Alfred Peters from comment #22)
[mail.ui.display.dateformat.today]
> I reset the pref (string - value 4) -> date still OK!

My mistake: With interger format - value 4 the Date is broken again.
HTH

Comment 24

[Jorg K (CEST = GMT+2)]

Attachment: Top: English (en-GB), bottom: German (de-DE), left: old, right new.

(In reply to Richard Marti (:Paenglab) from comment #21)
> Date column on Linux and macOS looks like before with today builds.
Thanks, but please look again, I can see subtle differences.

On my Linux Mint 17.1 (freshly updated) it looks like this:

+----+------+
|en-GB|en-GB|
| old | new |
+-----+-----+
|de-DE|de-DE|
| old | new |
+-----+-----+

So comparing old with new, a comma was added.
For en-GB, YY changed to YYYY, for de-DE, YYYY changed to YY.

Looking at attachment 8824661 [details] and attachment 8824667 [details], I can also see DD.MM.YY, HH:MM for new German.

Comment 25

[Jorg K (CEST = GMT+2)]

Yes, mail.ui.display.dateformat.today and friends are processed here:
https://dxr.mozilla.org/comm-central/rev/2231f130a183eae79746df4da367062d0ad04a9d/mailnews/base/src/nsMsgDBView.cpp#7908

Description here:
http://kb.mozillazine.org/Date_display_format

My experiment on Linux was done with the default values.

Comment 26

[Richard Marti (:Paenglab)]

Yes, mine have also this comma but are always in short format.

Comment 27

[Jorg K (CEST = GMT+2)]

Here we have the formats:
https://dxr.mozilla.org/comm-central/rev/a14094edbad78fc1d16e8d4c57902537cf286fd1/mozilla/intl/locale/nsIScriptableDateFormat.idl#8

    kDateFormatNone = 0,            // do not include the date  in the format string
    kDateFormatLong,                // provides the long date format for the given locale
    kDateFormatShort,               // provides the short date format for the given locale
    kDateFormatYearMonth,           // formats using only the year and month 
    kDateFormatWeekday              // week day (e.g. Mon, Tue)

So long==1 and short==2 and then two more.

ICU disabled the other two options, 3 and 4:
https://dxr.mozilla.org/comm-central/rev/a14094edbad78fc1d16e8d4c57902537cf286fd1/mozilla/intl/locale/DateTimeFormatICU.cpp#82 (link doesn't work right now) so:
https://dxr.mozilla.org/comm-central/source/mozilla/intl/locale/DateTimeFormatICU.cpp#82

Comment 28

[aleth [:aleth]]

(In reply to Jorg K (GMT+1) from comment #27)
> ICU disabled the other two options, 3 and 4:

Do we still need to check all instances of nsIScriptableDateFormat in c-c JS for this?

Comment 29

[Jorg K (CEST = GMT+2)]

I don't know. How does nsIScriptableDateFormat relate to nsIDateTimeFormat?

I can't see the now non-functional kDateFormatYearMonth and kDateFormatWeekday are not used,
https://dxr.mozilla.org/comm-central/search?q=kDateFormatYearMonth&redirect=false
https://dxr.mozilla.org/comm-central/search?q=kDateFormatWeekday&redirect=false
well, we could fix |if ( ( res >= kDateFormatNone ) && ( res <= kDateFormatWeekday ) )| here:
https://dxr.mozilla.org/comm-central/rev/2231f130a183eae79746df4da367062d0ad04a9d/mailnews/base/src/nsMsgDBView.cpp#7893
since that will let the now unsupported options through resulting in no date at all.

Comment 30

[Jorg K (CEST = GMT+2)]

Attachment: 1325745-disallow-kDateFormatYearMonth-kDateFormatWeekday.patch (v1) [landed, comment #36, backed out: comment #38]

I think we should disallow this since user will get a blank date column otherwise.

Comment 31

[aleth [:aleth]]

(In reply to Jorg K (GMT+1) from comment #29)
> I don't know. How does nsIScriptableDateFormat relate to nsIDateTimeFormat?

Looking at https://hg.mozilla.org/mozilla-central/rev/0f20e89eb486, its implementation also used nsIDateTimeFormat, but now no longer does.

Comment 32

[aleth [:aleth]]

Comment on attachment 8824739 [details] [diff] [review]
1325745-disallow-kDateFormatYearMonth-kDateFormatWeekday.patch (v1) [landed, comment #36, backed out: comment #38]

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

Looks good to me.

I wonder what (if anything) sets the mail.ui.display.dateformat.* prefs?

Comment 33

[aleth [:aleth]]

(In reply to Jorg K (GMT+1) from comment #25)
> Description here:
> http://kb.mozillazine.org/Date_display_format

I guess that page is out of date now as well... does anyone maintain them?

Comment 34

[Thomas Schade]

I played around a bit with date formats and that's what I've figured out. According to http://kb.mozillazine.org/Date_display_format we've the following prefs affecting the display of date / time:
mail.ui.display.dateformat.default      default value: 2     resulting in: system's short date format
                                                             Windows (German): 07.01.17, 22:15
mail.ui.display.dateformat.thisweek     default value: 2     resulting in: system's short date format
                                                             Windows (German): 07.01.17, 22:15
mail.ui.display.dateformat.today        default value: 0     resulting in: time only, no date
                                                             Windows (German): 22:15

With all values on default date / time is displayed properly. 

However, when I change:
mail.ui.display.dateformat.thisweek     new value: 4         resulting in: abbreviated day name 
                                                             Windows (German): Sat, 22:15
date column shows blank for all postings received within the past seven days. Older ones display correctly system's short date format, today's postings show correctly just the time.

Comment 35

[Jorg K (CEST = GMT+2)]

(In reply to aleth [:aleth] from comment #32)
> I wonder what (if anything) sets the mail.ui.display.dateformat.* prefs?
As pointed out in comment #25, mailnews reads the preference. It's not even defined, so it doesn't show up in the config manager, but users can set it.

(In reply to aleth [:aleth] from comment #33)
> I guess that page is out of date now as well... does anyone maintain them?
I have an account at kb.mozillazine.org, so I could change it. However, users will only notice it from TB 53.

(In reply to Thomas Schade from comment #34)
> However, when I change:
> mail.ui.display.dateformat.thisweek     new value: 4
> date column shows blank for all postings received within the past seven
> days. Older ones display correctly system's short date format, today's
> postings show correctly just the time.
Yes, of course, there are three settings, one for today, one for "this week", and one for older.
Values 3 and 4 are no longer supported, so if you set the "this week" to an unsupported value, the result is a blank date.

Comment 36

[Jorg K (CEST = GMT+2)]

Comment on attachment 8824739 [details] [diff] [review]
1325745-disallow-kDateFormatYearMonth-kDateFormatWeekday.patch (v1) [landed, comment #36, backed out: comment #38]

https://hg.mozilla.org/comm-central/rev/65c295598322d6863a3f6174de512c384ca19443

(In reply to aleth [:aleth] from comment #32)
> Looks good to me.
It even works ;-)

Comment 37

[Jorg K (CEST = GMT+2)]

(In reply to aleth [:aleth] from comment #33)
> > http://kb.mozillazine.org/Date_display_format
> I guess that page is out of date now as well... does anyone maintain them?
Updated.

Comment 38

[Jorg K (CEST = GMT+2)]

Comment on attachment 8824739 [details] [diff] [review]
1325745-disallow-kDateFormatYearMonth-kDateFormatWeekday.patch (v1) [landed, comment #36, backed out: comment #38]

kDateFormatYearMonth and kDateFormatWeekday got re-implemented in bug 1329841 after they were withdrawn in bug 1301640. So we no longer need to disallow them.

The messy part is that they were withdrawn in mozilla53/TB53 and now reappear in mozilla54/TB54.

So to state the landing of this bug very clearly one more time:
Main patch:
https://hg.mozilla.org/comm-central/rev/a6298203de72b408d580f604e9a336d2b70068bf (landed)
https://hg.mozilla.org/comm-central/rev/b5cbf4a5f294c84cee905a0e22c32ea34cc66359 (backed out)
https://hg.mozilla.org/comm-central/rev/2231f130a183eae79746df4da367062d0ad04a9d (relanded)

Patch for disable kDateFormatYearMonth and kDateFormatWeekday:
https://hg.mozilla.org/comm-central/rev/65c295598322d6863a3f6174de512c384ca19443 (landed)
https://hg.mozilla.org/comm-central/rev/d7b2c30e906ef8fc9c8e8d9fcd9fe74c753c0de9 (backed out)

Comment 39

[Jorg K (CEST = GMT+2)]

Adding version numbers:
Main patch:
TB 53: https://hg.mozilla.org/comm-central/rev/a6298203de72b408d580f604e9a336d2b70068bf (landed)
TB 53: https://hg.mozilla.org/comm-central/rev/b5cbf4a5f294c84cee905a0e22c32ea34cc66359 (backed out)
TB 53: https://hg.mozilla.org/comm-central/rev/2231f130a183eae79746df4da367062d0ad04a9d (relanded)

Patch for disable kDateFormatYearMonth and kDateFormatWeekday:
TB 53: https://hg.mozilla.org/comm-central/rev/65c295598322d6863a3f6174de512c384ca19443 (landed)
Re-enabled kDateFormatYearMonth and kDateFormatWeekday:
TB 54: https://hg.mozilla.org/comm-central/rev/d7b2c30e906ef8fc9c8e8d9fcd9fe74c753c0de9 (backed out)

Comment 40

[Jorg K (CEST = GMT+2)]

With the latest build I set mail.ui.display.dateformat.thisweek to 4. So for messages received this week I now see "Sun 17:41" since Sunday was yesterday.

Comment 41

[Thomas Schade]

With today's build

Mozilla/5.0 (Windows NT 10.0; WOW64; rv:54.0) Gecko/20100101 Thunderbird/54.0a1 ID:20170221030220 CSet: 2c5390afd3dd9514e5337521739f44a27929c1e2

'mail.ui.display.dateformat.thisweek' set to '4' leads to: 
Februar 2017, 17:32
for all mails / postings from this week except today. Not the expected result I'd say. Until yesterday it properly displayed
Di., 17:32

Comment 42

[Jorg K (CEST = GMT+2)]

Damn right! In the Daily from two days ago I saw "Mon 22:42" and right now it reverted to "February 2017, 22:42". I have to go looking who broke this now.

Comment 43

[Jorg K (CEST = GMT+2)]

Found it, as of bug 1225696, https://hg.mozilla.org/mozilla-central/rev/87908aced111#l2.8 there are now six options. What was 4 is now 6 :-(

enum
 {
     kDateFormatNone = 0,            // do not include the date  in the format string
     kDateFormatLong,                // provides the long date format for the given locale
     kDateFormatShort,               // provides the short date format for the given locale
     kDateFormatYearMonth,           // formats using only the year and month 
+    kDateFormatYearMonthLong,       // long version of kDateFormatYearMonth
+    kDateFormatMonthLong,           // long format of month name only
     kDateFormatWeekday              // week day (e.g. Mon, Tue)
 };

kDateFormatWeekday was 4 and is now 6.

Comment 44

[Jorg K (CEST = GMT+2)]

Note that in bug 1225696 the order of the values has been restored to the original order, so what was 4 and became 6 will now go back to 4. You will see it in TB Daily of tomorrow or the day after tomorrow, so be prepared ;-)

Comment 45

[Thomas Schade]

Worked nicely since yesterday's build (20170305). But today it's broken again with:
Mozilla/5.0 (Windows NT 10.0; WOW64; rv:54.0) Gecko/20100101 Thunderbird/54.0a1 ID:20170306030212 CSet: 8be1ae77d2644e401fa6480a05ff234e70d0ccdf

Whether I configure 4 or 6 dates are always displayed as:

Today:     8:30 pm
Last week: Fri 1:45 am
Older:     2/21/17, 4:01 pm

Thunderbird bravely ignores system settings which would be German notation in my case; 24 h format like 21.02.17, 16:01.

Comment 46

[Jorg K (CEST = GMT+2)]

Yes, they're still playing with this which is a real pain. Let me check it out. Most likely broken by bug 1342753 which landed just now:
https://hg.mozilla.org/mozilla-central/rev/439f5e4ed073

Comment 47

[Jorg K (CEST = GMT+2)]

See bug 1342753 comment #21.

Comment 48

[Jorg K (CEST = GMT+2)]

After much discussion in bug 1342753 the result for now is this:

If you want German time display you must install a German localised version of FF or TB. An en-US version with a German language pack currently doesn't work due to bug 1344355. The time format set in Windows is just ignored.