If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Time Zone never displayed within message headers

RESOLVED FIXED in Thunderbird 3.0b3

Status

MailNews Core
Localization
--
minor
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: Christian Eyrich, Assigned: Christian Eyrich)

Tracking

unspecified
Thunderbird 3.0b3

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

10 years ago
Created attachment 276760 [details] [diff] [review]
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.
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?

(Assignee)

Comment 2

10 years ago
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.
(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
Oops, Bug 32216 Comment #75. Sorry for spam.

Updated

10 years ago
Summary: Time Zone never displayed within message meaders → Time Zone never displayed within message headers
Product: Core → MailNews Core
(Assignee)

Comment 5

9 years ago
Created attachment 358606 [details] [diff] [review]
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?
Attachment #276760 - Attachment is obsolete: true
(Assignee)

Comment 6

9 years ago
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.
Attachment #358606 - Attachment description: same patch but applyable to current tree → same patch but works with HEAD
Attachment #358606 - Flags: review?(bienvenu)

Comment 7

9 years ago
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

9 years ago
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.
Attachment #358606 - Flags: superreview?(bugzilla)
Attachment #358606 - Flags: review?(bienvenu)
Attachment #358606 - Flags: review+
(Assignee)

Comment 9

9 years ago
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

9 years ago
(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...
Attachment #358606 - Flags: superreview?(bugzilla) → superreview+
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.
(Assignee)

Comment 12

9 years ago
Created attachment 366792 [details] [diff] [review]
patch with Marks comments addressed

Thanks for your review, Mark. I carried over the flags to the patch that addresses your comments.
Attachment #358606 - Attachment is obsolete: true
Attachment #366792 - Flags: superreview+
Attachment #366792 - Flags: review+

Updated

9 years ago
Keywords: checkin-needed
Patch checked in: http://hg.mozilla.org/comm-central/rev/a4305dcef018
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.0b3
You need to log in before you can comment on or make changes to this bug.