392307 - Time Zone never displayed within message headers

Status: RESOLVED FIXED

(MailNews Core :: Localization, defect)

Description

[Christian Eyrich]

Attachment: proposed patch

Besides a hack for a hidden pref done in bug 118899 there's no way to display the date/time of a message in the senders time zone including that time zone offset from UTC.

Because of a change in NSPR it's now possible to have that information available in mailnews and display it. The attached patch does that.
To activate that behaviour I introduced a new pref mailnews.display.date_senders_timezone. At the same time the old pref mailnews.display.original_date gets the same meaning and is migrated to the new one.
Reason for not simply using the old pref is that I don't feel it's the right name. But maybe there's reasoning of keeping the old one in its present meaning. Please say so.

Comment 1

[WADA:World Anti-bad-Duping Agency]

Current logic with mailnews.display.original_date is as follows, and I think meaning of mailnews.display.original_date is "display original string in date: header".
> 212     if (!PL_strcasecmp("Date", headerInfo->name) && !displayOriginalDate)
> 213     {
> 214       GenerateDateString(headerValue, convertedDateString);
> 215       headerValueArray.AppendCString(convertedDateString);
> 216     }
> 217     else // append the header value as is
> 218       headerValueArray.AppendCString(nsCString(headerValue));

This looks to be slightly different from request of Bug 118899, but is convenient when mail with malformed Date: header often arrives.
I think mailnews.display.original_date is better to be renamed to mailnews.display.original_date_header_text and kept as independent option from your new mailnews.display.date_senders_timezone, although it never be mandatory.
Christian Eyrich, what do you think?

Comment 2

[Christian Eyrich]

It would be ok for me to keep the old pref with its old meaning. I just didn't see any reason to. But yes, if one often gets messages with corrupted date header that could be a reason.
Renaming is always a little problematic since users will complain their current prefs don't work anymore. In this case it's a hidden, few documented pref, so we might do that nevertheless. But I'd like to hear other opinions too.

Comment 3

[WADA:World Anti-bad-Duping Agency]

(In reply to comment #2)
> Renaming is always a little problematic
I absolutely agree with you.
I've recalled that I recommended to use it in Bug 32216 Comment #87 :-)
> Bug 32216 Mail date 1.1.1970(or 12.31.1969) when Date: tag localized or malformed

Comment 4

[WADA:World Anti-bad-Duping Agency]

Oops, Bug 32216 Comment #75. Sorry for spam.

Comment 5

[Christian Eyrich]

Attachment: same patch but works with HEAD

Due to some activity on bug 218926 I remembered this here. Basically my old patch still works and I'd really appreciate to get some feedback regarding the future of the old mailnews.display.original_date.

As I discussed with WADA, options are remove, keep with old meaning, keep with same meaning as the new one (mailnews.display.date_senders_timezone). Or maybe don't use a new one and use the old one for the new meaning?

Comment 6

[Christian Eyrich]

Comment on attachment 358606 [details] [diff] [review]
same patch but works with HEAD

No comments? Ok, I guess than this also means no objection. Thus requesting rv.

Comment 7

[David :Bienvenu]

No strong feelings either way, except I'd prefer to have the prefs meaning match the name, so I think what you've done is OK. I'll apply this and try it out, thx for the patch.

Comment 8

[David :Bienvenu]

Comment on attachment 358606 [details] [diff] [review]
same patch but works with HEAD

I've run with this patch, and it seems to work. It displays the offset, but not the timezone name (e.g., PDT). Did the old code do that?

There are a couple lines that are too long and need wrapping. r=me, with those fixed.

It might be nice to only display timezone offsets that are different from the current offset to make the display less cluttered, though for a hidden pref, probably not worth it.

Comment 9

[Christian Eyrich]

Thanks for evaluating it, David.

> I've run with this patch, and it seems to work. It displays the offset, but not the timezone name (e.g., PDT). Did the old code do that?

The current code displays the mail header verbatim. So the display depends on the mail which normally contains tz info in numeric form.

> It might be nice to only display timezone offsets that are different from the
> current offset to make the display less cluttered, though for a hidden pref,
> probably not worth it.

I don’t object a pref for this but would wait for reception of the new display. It will anyway only affect a fraction of a fraction of the users.

Comment 10

[David :Bienvenu]

(In reply to comment #9)
> 
> I don’t object a pref for this but would wait for reception of the new display.
> It will anyway only affect a fraction of a fraction of the users.

I wasn't suggesting a second pref - I was thinking it should always hide the tz info if it's the current tz, but since this the main behavior is controlled by a hidden pref, it's probably not worth bothering...

Comment 11

[Mark Banner (:standard8)]

Comment on attachment 358606 [details] [diff] [review]
same patch but works with HEAD

>diff -r 33581ce31913 mailnews/mime/emitters/src/nsMimeHtmlEmitter.cpp

>+#include "nsTextFormatter.h"
>+#include "nsIPrefBranch.h"

I don't see why you need to include nsIPrefBranch when this file is already using it...

>-  PRTime messageTime;
>-  PR_ParseTimeString(dateString, PR_FALSE, &messageTime);
>+  /* See if the user wants to have the date displayed in the senders
>+     timezone (including the timezone offset).
>+     We also evaluate the pref original_date which was introduced
>+     as makeshift in bug 118899. */

nit: please use the

/**
 *
 */

style of comments (2 places).

>+  nsCOMPtr<nsIPrefService> prefs = do_GetService(NS_PREFSERVICE_CONTRACTID, &rv);
>+  NS_ENSURE_SUCCESS(rv,rv);
>+  nsCOMPtr<nsIPrefBranch> dateFormatPrefs;
>+  rv = prefs->GetBranch("mailnews.display.", getter_AddRefs(dateFormatPrefs));
>+  NS_ENSURE_SUCCESS(rv,rv);

nit: space after the commas please (in both NS_ENSURE_SUCCESS) and insert blank line to help readability.

>+  dateFormatPrefs->GetBoolPref("date_senders_timezone", &displaySenderTimezone);
>+  dateFormatPrefs->GetBoolPref("original_date", &displayOriginalDate);
>+  // migrate to sender_timezone
>+  displaySenderTimezone |= displayOriginalDate;
>+  dateFormatPrefs->SetBoolPref("date_senders_timezone", displaySenderTimezone);

I think you should only set the preference if the value has changed from before, otherwise we're causing unnecessary function calls and maybe even writes to prefs.js.

>-pref("mailnews.display.original_date", false);   // display date string from mail headers without interpreting
>+pref("mailnews.display.date_senders_timezone", false); // display date string from mail headers in senders timezone

nit: please can you move the comment onto a line above the pref, it makes it easier to read.

sr=me with those fixed.

Comment 12

[Christian Eyrich]

Attachment: patch with Marks comments addressed

Thanks for your review, Mark. I carried over the flags to the patch that addresses your comments.

Comment 13

[Mark Banner (:standard8)]

Patch checked in: http://hg.mozilla.org/comm-central/rev/a4305dcef018