294356 - In a printed mail Thunderbird ignores the locale settings for the date generating month and day of week

Status: RESOLVED FIXED

(MailNews Core :: MIME, defect)

Description

[Frieder Engel]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; de-DE; rv:1.7.7) Gecko/20050414 Firefox/1.0.3
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; de-DE; rv:1.7.7) Gecko/20050414 Firefox/1.0.3

In a printed mail Thunderbird ignores the locale settings for the date.
The title (Date/Datum) is correct though, but the date itself is wrong (always 
English/American format)

Example:
 
English:
Date: 15 May 2005  ...

German:
Datum: 15 May 2005 ... should be: 15. Mai 2005

Reproducible: Always

Steps to Reproduce:
1. Set computers locale to German
2. Print any message
3.



Expected Results:  
Use the computers correct locale settings.

Comment 1

[Max Barry]

This is probably the same issue as bug 252508.

Comment 2

[Martijn (MozBrowser.nl)]

Just like to mention that the PrintingTools-extension now "solves" this error:
https://nic-nac-project.de/~kaosmos/printingtools-en.html

So you might have a look at it and at least it provides a workaround for people who experience this problem.

Comment 4

[Wayne Mery (:wsmwk)]

no dupes afaict

Comment 5

[Magnus Melin [:mkmelin] (away, back Aug 4)]

xref bug 107854 (for forwarding). It's the raw header, not wrong date per se.

Comment 7

[Noèl Köthe]

the date is still not translated in 2.0.0.14.

The header fields From:, To: are translated in the view and printout and the date is formated for the local format when you read the E-Mail so it should be the same in the printout.

Comment 8

[Tobias Koenig]

Attachment: Use internationalized date for printing

The attached patch fixes this bug by moving the GenerateDateString from nsMimeHtmlDisplayEmitter to nsMimeBaseEmitter and making use of it there in WriteHeaderFieldHTML() when the mailnews.display.original_date config option is set.

Comment 9

[Magnus Melin [:mkmelin] (away, back Aug 4)]

Comment on attachment 323412 [details] [diff] [review]
Use internationalized date for printing

Sorry, you need a mailnews backend reviewer for this.
See http://www.mozilla.org/owners.html#mail-and-news-backend

The patch should apply to trunk (CVS HEAD), so please make sure it does - looks like this was generated against the branch.

Comment 10

[Tobias Koenig]

Attachment: previous patch ported against HEAD

This patch prints the date in internationalized format by moving the GenerateDateString method up in the inheritance hierarchy, so that MimeBaseEmitter can make use of it.

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 324277 [details] [diff] [review]
previous patch ported against HEAD

In general, this looks good; just need a few tweaks:

> 
>+  if ( strcmp(field, "Date") == 0 )
>+  {

The method this is in is already long enough; making it even longer starts to get hard to read, especially given that this is special-case code.  Please push this down into a new method called from here.

>+    if ( !displayOriginalDate )
>+    {
>+      nsCAutoString convertedDateString;
>+      GenerateDateString(value, convertedDateString);

Check the return value, please!

Finally, please add yourself to the Contributors section of the license boilerplate of (at least) nsMimeBaseEmitter.cpp, if not the other files too.

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

Reassigning to Tobias, as he's been doing a fine job owning this bug...

Comment 13

[Tobias Koenig]

Attachment: corrected version

Moved the convertion of date header in its own method, checked for result value of GetDateString() and added my email to the contributors section

Comment 14

[Noèl Köthe]

any news/update on this?
next step?

Comment 15

[Magnus Melin [:mkmelin] (away, back Aug 4)]

The patch has bitrotted :(

Comment 16

[Dan Mosedale (:dmosedale, :dmose)]

Apologies for not having gotten to this sooner.  After we're out from under 3.0 code pressure, I'll unbitrot this up and review it for 3.1.  Alternately, if someone else wants to unbitrot sooner, bienvenu and/or BenB could also be good reviewers, though they're still somewhat busy with the 3.0 as well.

Comment 17

[Mark Banner (:standard8)]

Attachment: Updated patch

I've fixed the bitrot on this. From what I can tell it works fine.

There are try server builds here with this fix for anyone that wants to test:

http://ftp.mozilla.org/pub/mozilla.org/thunderbird/tryserver-builds/[e-mail]-dfacd3858259/

Comment 18

[Mark Banner (:standard8)]

Comment on attachment 473464 [details] [diff] [review]
Updated patch

Given this was just a bitrot fix, I've reviewed the patch and given it r+. However, as this is mime, I'd like David to take a quick look as well.

Comment 19

[David :Bienvenu]

Comment on attachment 473464 [details] [diff] [review]
Updated patch

prefer NS_ENSURE_SUCCESS(rv, rv); here:

+    mDateFormatter = do_CreateInstance(NS_DATETIMEFORMAT_CONTRACTID, &rv);
+    if (NS_FAILED(rv))
+      return rv;

I know this is just being moved, but I'd lose the commented out code:

+/*
+  else if (LL_CMP(currentTime, >, dateOfMsg))
+  {
+    PRInt64 microSecondsPerSecond, secondsInDays, microSecondsInDays;
+    LL_I2L(microSecondsPerSecond, PR_USEC_PER_SEC);
+    LL_UI2L(secondsInDays, 60 * 60 * 24 * 7); // how many seconds in 7 days.....
+    LL_MUL(microSecondsInDays, secondsInDays, microSecondsPerSecond); // turn that into microseconds
+
+    PRInt64 diff;
+    LL_SUB(diff, currentTime, dateOfMsg);
+    if (LL_CMP(diff, <=, microSecondsInDays)) // within the same week
+      dateFormat = kDateFormatWeekday;
+  }
+*/

these lines look awfully long:
+        PRInt32 senderoffset = (explodedMsgTime.tm_params.tp_gmt_offset + explodedMsgTime.tm_params.tp_dst_offset) / 60;
+        // append offset to date string
+        PRUnichar *tzstring = nsTextFormatter::smprintf(NS_LITERAL_STRING(" %+05d").get(), (senderoffset / 60 * 100) + (senderoffset % 60));

prefer NS_ENSURE_SUCCESS(rv, rv); here, which gets rid of the need for the if (prefBranch) line and the braces:

+  nsCOMPtr<nsIPrefBranch> pPrefBranch(do_GetService(NS_PREFSERVICE_CONTRACTID, &rv));
+  if (pPrefBranch)
+  {
+    pPrefBranch->GetBoolPref("mailnews.display.original_date", &displayOriginalDate);
+  }

lose the spaces around !displayOriginalData, and rv is already declared above:

+  if ( !displayOriginalDate )
+  {
+    nsCAutoString convertedDateString;
+    nsresult rv = GenerateDateString(dateString, convertedDateString);
+    if (NS_SUCCEEDED(rv))

please add a comment in the header file to the effect that the caller needs to free the result of GetLocalizedDateString();.

r+ with those addressed.

Comment 20

[Mark Banner (:standard8)]

(In reply to comment #19)
> prefer NS_ENSURE_SUCCESS(rv, rv); here, which gets rid of the need for the if
> (prefBranch) line and the braces:
> 
> +  nsCOMPtr<nsIPrefBranch> pPrefBranch(do_GetService(NS_PREFSERVICE_CONTRACTID,
> &rv));
> +  if (pPrefBranch)
> +  {
> +    pPrefBranch->GetBoolPref("mailnews.display.original_date",
> &displayOriginalDate);
> +  }

That didn't quite work because the function returns char*, so I just removed the rv and the braces.

Comment 21

[Mark Banner (:standard8)]

Attachment: Updated patch v2

Updated patch ready for checkin which I'll do once the tree is a bit healthier.

Comment 22

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/75a2d4946290

Tobias, thanks for all the work you did on this bug, sorry it took so long to get it landed.

Comment 23

[Mark Banner (:standard8)]

Comment on attachment 475494 [details] [diff] [review]
Updated patch v2

Having just granted this approval, I'm not sure I should have done so really. It isn't a fix to stability or security, and we don't normally take basic fixes for those releases. This also isn't a regression.

Therefore denying approval for the stable branch. It will make the next major release whatever version that is.