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

RESOLVED FIXED in Thunderbird 53.0

Status

MailNews Core
Database
RESOLVED FIXED
5 months ago
3 months ago

People

(Reporter: Jorg K (GMT+2), Assigned: Jorg K (GMT+2))

Tracking

Trunk
Thunderbird 53.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Comment 1

5 months ago
Created attachment 8821715 [details] [diff] [review]
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.
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
(Assignee)

Comment 2

5 months ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 53.0
(Assignee)

Comment 3

5 months ago
See bug 1301640 comment #104.
Blocks: 1301640
(Assignee)

Comment 4

5 months ago
Raised bug 1325751 for the formatting issue.

Comment 5

5 months ago
(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

5 months ago
(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

5 months ago
(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 ;)
(Assignee)

Comment 8

5 months ago
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.
Attachment #8821715 - Flags: review?(mkmelin+mozilla)
Attachment #8821715 - Flags: review?(aleth)
Attachment #8821715 - Flags: review?(acelists)
(Assignee)

Comment 9

5 months ago
(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.
(Assignee)

Comment 10

5 months ago
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
Attachment #8821715 - Flags: review?(mkmelin+mozilla)
Attachment #8821715 - Flags: review?(aleth)
Attachment #8821715 - Flags: review?(acelists)
Backed out after back out of m-c bug.
https://hg.mozilla.org/comm-central/rev/b5cbf4a5f294c84cee905a0e22c32ea34cc66359
(Assignee)

Comment 12

5 months ago
Created attachment 8823978 [details] [diff] [review]
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.
Attachment #8821715 - Attachment is obsolete: true
Attachment #8823978 - Flags: review?(mkmelin+mozilla)
(Assignee)

Comment 13

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

Comment 14

5 months ago
(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.
(Assignee)

Comment 15

5 months ago
(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

5 months ago
Created attachment 8824660 [details]
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

5 months ago
Created attachment 8824661 [details]
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.
(Assignee)

Comment 18

5 months ago
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?
Flags: needinfo?(richard.marti)

Comment 19

5 months ago
Created attachment 8824667 [details]
Inconsistency in the date format

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

Comment 20

5 months ago
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. :)
Date column on Linux and macOS looks like before with today builds.
Flags: needinfo?(richard.marti)

Comment 22

5 months ago
(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

5 months ago
(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
(Assignee)

Comment 24

5 months ago
Created attachment 8824695 [details]
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.
(Assignee)

Comment 25

5 months ago
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.
Yes, mine have also this comma but are always in short format.
(Assignee)

Comment 27

5 months ago
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

5 months ago
(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?
(Assignee)

Comment 29

5 months ago
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.
(Assignee)

Comment 30

5 months ago
Created attachment 8824739 [details] [diff] [review]
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.
Attachment #8824739 - Flags: review?(aleth)

Comment 31

5 months ago
(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

5 months ago
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?
Attachment #8824739 - Flags: review?(aleth) → review+

Comment 33

5 months ago
(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

5 months ago
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.
(Assignee)

Comment 35

5 months ago
(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.
(Assignee)

Updated

5 months ago
Attachment #8823978 - Attachment description: 1301640-tb-nsIDateTimeFormat-bustage.patch - refreshed → 1301640-tb-nsIDateTimeFormat-bustage.patch - refreshed [landed, comment #13]
(Assignee)

Comment 36

5 months ago
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 ;-)
Attachment #8824739 - Attachment description: 1325745-disallow-kDateFormatShort-kDateFormatWeekday.patch (v1) → 1325745-disallow-kDateFormatShort-kDateFormatWeekday.patch (v1) [landed, comment #36]
(Assignee)

Comment 37

5 months ago
(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.

Updated

5 months ago
Blocks: 1329841

Updated

5 months ago
No longer blocks: 1329841

Updated

5 months ago
Attachment #8823978 - Flags: review?(mkmelin+mozilla) → review+
(Assignee)

Comment 38

3 months ago
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)
Attachment #8824739 - Attachment description: 1325745-disallow-kDateFormatShort-kDateFormatWeekday.patch (v1) [landed, comment #36] → 1325745-disallow-kDateFormatYearMonth-kDateFormatWeekday.patch (v1) [landed, comment #36, backed out: comment #38]
Attachment #8824739 - Attachment filename: 1325745-disallow-kDateFormatShort-kDateFormatWeekday.patch → 1325745-disallow-kDateFormatYearMonth-kDateFormatWeekday.patch
(Assignee)

Comment 39

3 months ago
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)
(Assignee)

Comment 40

3 months ago
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

3 months ago
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
(Assignee)

Comment 42

3 months ago
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.
(Assignee)

Comment 43

3 months ago
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.
(Assignee)

Comment 44

3 months ago
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

3 months ago
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.
(Assignee)

Comment 46

3 months ago
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
(Assignee)

Comment 47

3 months ago
See bug 1342753 comment #21.
(Assignee)

Comment 48

3 months ago
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.
You need to log in before you can comment on or make changes to this bug.