118899 - Time Zone Never Displayed Within Message Headers

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Message Display, defect)

Description

[Felix Miata]

2002010619. Modern.

Actual behavior:
Regardless of whether currently displayed message is showing brief, normal, or
full headers, time zone correction of message is not displayed.

Expected behavior:
Zime zone included in date/time field except possibly in brief mode. Without
access to time zone, it cannot be determined why a message received was
apparently sent prior to the the message to which it was a reply, or why news
thread messages are displayed out of apparent order when senders use incorrect
local time or zime zone settings.

Here is yet another reason to abandon too small to read chromed message headers.

Comment 1

[Mike Kaply [:mkaply]]

Nothing OS/2 specific about this

Comment 2

[Ivan Dolezal]

What's worse: when you get an e-mail from a different time zone, the sorting
function simply IGNORES the "time sequence" and so you can see the e-mail that
has just arrived sorted somewhere in a dark dusty parts of Inbox. That is VERY
inconvenient behaviour, especially when you live in Central Europe and all the
new mail from America appears somewhere in last night's e-mail, while it is
afternoon here.

Comment 3

[Jesse Ruderman]

In Brief and Normal, the time shown should be in the user's timezone, not the
sender's timezone, and the sender's timezone doesn't need to be visible.  I
think mozmail already does that correctly for most messages, although it
displays messages from Hotmail users in GMT.

I don't care what happens in Full headers mode, because IMO Full is useless and
should be removed in favor of view source.  Most of the time it just takes up
90% of the message pane with headers like:
X-UIDL: `84!!<8R!!ZdN"!NdF"!
X-Mozilla-Status: 0001
X-Mozilla-Status2: 00000000
Message-Id: <[e-mail]>
X-Mailer: rmail

Ivan: see bug 83914, Cronological order doesn't compensate for timezones.

Comment 4

[Felix Miata]

"In Brief and Normal, the time shown should be in the user's timezone, not the
sender's timezone, and the sender's timezone doesn't need to be visible."

MAYBE it doesn't need to be displayed in brief, but it CERTAINLY needs to be
displayed in normal. Too many clueless posters don't know how to set clocks or
time zones. Seeing full date stamp facilitates a reply that includes appropriate
instructions on fixing the problem their naivette creates, and dealing with it
until fixed.

There is *plenty* of space in the mostly whitespace headers chrome area to
include it, and thus no reason not to.

Comment 5

[Mike Bremford]

I beg to differ with Jesse Ruderman - the timezone displayed should be the
*senders* timezone, not the recipients. Here's my case:

1. Netscape 4 does it this way (**** reason but here for completeness)

2. The recipient (probably) already knows what timezone he's in, and it will be
the same for every message, so there's no point in displaying it - it's redundant.

3. It is EXTREMELY useful to know which zone the sender is in, particularly if
you're arranging meetings/trying to work out why you haven't heard back from
someone and so on. We find this info absolutely invaluable (so much so I may
have to switch back to NS4 in the meantime).

4. If this saves just one person from being woken up by a misguided phone call
at 4am, it's the right thing to do. There's nothing worse...


If there's room in brief display it should go there, but it should definitely be
available in normal. It takes so little effort to display it, there's plenty of
space and it's very useful.

As for sorting, all dates should be normalized to the same zone and then sorted
- but I suspect everyone thinks that.

Comment 6

[Bogdan Stancescu]

I completely agree with Mike Bremford's argument: the sender's time zone should
be displayed - and it should be shown wherever there's room for it. I don't
think five characters would make such a difference in any kind of view (not that
I ever found any such setting as "normal" headers). Heck, I'll even vote for
this one.

Comment 7

[Friedrich Lobenstock]

*** Bug 155274 has been marked as a duplicate of this bug. ***

Comment 8

[Christian Eyrich]

Today I looked through the code how to implement this.

I wanted to make it adjustable via pref string:
1. display local time like now: 04/28/03 01:52
2. display senders original time and timezone: 04/27/03 16:52 -0700
(assuming local time is GMT+2)

The effort is not that low because I didn't want to only take the original
string from the mail. This can be in any format (country specific a.s.o.). So I
took the long way through the standard routines to have it in the known and ever
same format.

It works well here on my system but implement it that way in the official code
would mean touching very much files since the functions changed are very widely
used.

While I'm trying to get this right, any comments are welcome what you think is
necessary and not yet said.
Any votes for both timezones at once? Name for the pref string?

Comment 9

[Friedrich Lobenstock]

I'd vote for displaying the date header like this:
Date: 28.04.2003 02:17 (27.04.2003 17:17 -0700)

So I know when the mail got sent/written in my time and I know 
the timezone of the sender, so I might just quickly reply, 
otherwise I wouldn't.

Comment 10

[Felix Miata]

I am the reporter and the one who supplied the 4xp keyword. Nothing would please
me more than for the default behavior to be exactly like Netscape 4.I want to
see the date format configured in my OS, plus the time zone corrector.

Comment 11

[Christian Eyrich]

Attachment: patch flavour 1

Here comes the patch in flavour 1.
This one is actually quite easy, but since the changed functions
PR_ParseTimeString and especially PR_ExplodeTime are widely used, many calls to
them have to be changed (if they were C++ code we could use default parameters
...).

It works in that it gets the difference from the senders timezone to GMT from
PR_ParseTimeString.

Normaly PR_ExplodeTime applies the difference from the local time to GMT on the
exploded time using ApplySecOffset.
The trick now is to replace this difference by the offset parameter which is
the value we got from PR_ParseTimeString. To be flexible and able to also
output the local timezone, this only happens if the pointer to offset is not 0.



This isn't a complete patch which is compileable! It only shows the directly
affected functions with codechanges, it lacks all the calls to
PR_ParseTimeString and PR_ExplodeTime that have to be changed as well as
FormatPRTime of nsDateTimeFormatUnix, nsDateTimeFormatOS2 and
nsDateTimeFormatMac.

Comment 12

[Christian Eyrich]

Attachment: patch flavour 2

Here comes the patch in flavour 2.
It affects less code and would need only PR_ParseTimeString to be extended, not
PR_ExplodeTime or FormatPRTime. But although this advantage, I don't feel so
happy with it.

It works in that it gets the difference from the senders timezone to GMT from
PR_ParseTimeString. Then it subtracts the difference from the local timezone to
GMT from it and so gets the difference from the senders to the local timezone.

This amount of microseconds gets added to the originals messageTime (which is
in GMT at this time). While the PR_ExplodeTime (direct and indirect from within
FormatPRTime) this timestamp gets expanded and converted to the local timezone.
But because we moved messageTime by the difference between local and senders
time we now get the exploded time in the senders timezone.

Yeah, somehow insane, I know. That's the reason why I'm not so happy with it,
it appears quite shaky to me though it worked through my tests.


As the first patch this isn't a complete patch which is compileable! It only
shows the directly affected functions with codechanges, it lacks all the calls
to PR_ParseTimeString that have to be changed.

Comment 13

[Christian Eyrich]

Attachment: patch flavour 3

Here comes the patch in flavour 3.
It's a in-between of variant 1 and 2. It affects PR_ParseTimeString and
FormatPRTime but not PR_ExplodeTime.

It makes use of a new function SendersTimeParameters which replaces
PR_LocalTimeParameters if we want the senders timezone to be displayed.
It's only purpose is placing senderoffset from PR_ParseTimeString in the
tm_params passed to ApplySecOffset in PR_ExplodeTime.
So by faking this, we don't need to modify PR_ExplodeTime.


As the two others this isn't a complete patch which is compileable! It only
shows the directly affected functions with codechanges, it lacks all the calls
to PR_ParseTimeString that have to be changed as well as FormatPRTime of
nsDateTimeFormatUnix, nsDateTimeFormatOS2 and nsDateTimeFormatMac.

Comment 14

[Christian Eyrich]

It doesn't look good for this bug. I was told, that NSPR is freezed and none of
it's interface can be changed.

This bug is about getting the time displayed in the senders timezone.
But after PR_ParseTimeString() we only have it in GMT. So we either have to get
the offset to calculate it back or need access to the time before normalizing in
order to display it directly.
Two ways and same problem of getting infos out of PR_ParseTimeString() without
changing it's interface.

The only way I can think of is a new function with interface like
PR_ParseTimeStringToExplodedTime (
    const char *string,
    PRBool default_to_gmt,
    PRExplodedTime *result);

The code of this function would be the same as in PR_ParseTimeString() (but
without the call to PR_ImplodeTime()). That's quite easy but really bad style
and one ot the drawbacks is synchronizing changes of the latter to the new.


Any new approach is welcome. Currently I don't know how to handle this.

Comment 15

[Felix Miata]

Attachment: Screenshot of the goal - Netscape 3 & 4 parity

The object is as stated in comment 0, to display the date header that is in the
original message, exactly as included in the original message, without
modification. The headers pane shows the local time, which should be sufficient
to please those who need the sender's time converted, just as was done with
Netscape 3 & 4. We don't need the converted time shown in both message and
header panes to the exclusion of showing the sender's time anywhere at all.

Comment 16

[Christian Eyrich]

If you really only want the date string to be taken from the mail headers and
put on the display, that's easy.

What's impossible is to cut off the weekday or display it in the users language.
If the mail is from today, it does not only display the time but the full string
too. And even the format of the string can vary.

I can think of this as an optional interims solution, but not more.

Comment 17

[Friedrich Lobenstock]

If you could make that a configurable option to mail-news this would be great.

Comment 18

[Christian Eyrich]

Attachment: patch v2

So this patch does exactly this, the date string from the mail header will not
be interpreted but simply copied to the screen. But only if the boolean pref
mailnews.display.original_date is set to true.

If that's what you're thinking off, I'll ask for review and see what will
happen.

Comment 19

[Felix Miata]

That is exactly fine with me. We aren't translating any other portion of the
message are we? If someone wants the day stripped or language conversion, that
can be a new bug. In the meantime, the patch in comment 18 sounds like it will
achieve parity with Netscape 4 and fix this bug.

Comment 20

[[e-mail]]

Comment on attachment 124928 [details] [diff] [review]
patch v2

If I've understood correctly the idea is not to parse the date when the pref is
set, in which case, you should prevent WriteHTMLHeaders from calling
GenerateDateString. You should also include a diff to mailnews.js so that the
preference appears in about:config

Comment 21

[Christian Eyrich]

Attachment: patch v3

You understood correctly, because it's very heavy to keep the original time
zone when parsing the string, I don't even try this here.

I moved the code to WriteHTMLHeaders() and created an entry in mailnews.js.
The pref name is ok for you?

Comment 22

[[e-mail]]

Comment on attachment 124991 [details] [diff] [review]
patch v3

Perhaps you failed to notice the existing prefs object defined (wrongly so feel
free to fix it) in that function? Also, other headers use different code for
the unicode conversion, perhaps you could reuse or fix that code too? Thanks.

Comment 23

[Christian Eyrich]

Attachment: patch v4

You're right, I just moved without looking around.
I now use the existing prefs object and the same UTF8toUCS2 conversion as the
code in the else case.

Comment 24

[[e-mail]]

Comment on attachment 124995 [details] [diff] [review]
patch v4

Just noticed that I couldn't directly apply this because you joined two patches
created in different folders :-( I'll have to look tomorrow now.

Comment 25

[[e-mail]]

Comment on attachment 124995 [details] [diff] [review]
patch v4

OK, so the patch functions... it might not be quite the best way to do it but
it's short and sweet :-)

Comment 26

[Christian Eyrich]

Thanks for review, Neil.

With "not quite the best way" do you mean to directly display the date string,
or to display the time zone in general?

If the latter, you're welcome to make a suggestion. I'm not very happy with this
too but my other attempts all failed as you can read in comment 14.

Comment 27

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 124995 [details] [diff] [review]
patch v4

Looks good; just a couple of nits:

>+          headerValues[numHeadersAdded] = nsCRT::strdup(NS_ConvertUTF8toUCS2(headerValue).get());

It'd probably be good to use the currently supported string fu, since that's
what gets hacked on for optimizations these days.  So prefer
ToNewUnicode(NS_Convert....()) for this sort of thing.

>+          rv = GenerateDateString(headerValue, &headerValues[numHeadersAdded]);
>+      }

Might as well drop the |rv =| entirely since it's ignored (and the surrounding
code has no infrastructure for dealing with any kind of error anyway -- can you
file a new bug on shoring up this method's error handling?).

sr=[e-mail] with those tweaks

Comment 28

[Christian Eyrich]

Attachment: patch v5

Patch with tweaks according to comment 27.

r=neil.parkwaycc.co.uk, sr=dmose taken from patch v4

Comment 29

[[e-mail]]

Fix checked in, except ToNewUnicode(nsDependentCString(...)) is slightly nicer.

Comment 30

[Christian Eyrich]

That's nice too, string classes aren't my world.
Thanks for the checkin.

Felix, is this ok for you now?
Is there a resource we should add this new pref to for documentation purposes?

Comment 31

[Felix Miata]

Without checkin to 1.4 it does me no good. There are no functional trunk builds
for OS/2.

Comment 32

[[e-mail]]

It's almost certainly too late to ask for approval1.4;
as I checked this in I should mark this fixed-on-trunk.

Comment 33

[Felix Miata]

For all intents and purposes the patch was ready on the 6th, which would almost
certainly not have been too late had the minor tweaks not been so long delayed.

I can't see that there's much danger in this. What can it hurt to at least ask
for 1.4 approval? There are still 5 blocking1.4+ bugs holding it up anyway.
Users of this major release deserve this 4xp fix.

Comment 34

[Mike Cowperthwaite]

So this patch seems to be working pretty well: with the preference set, the Date 
header in Normal (and Full) display modes is shown as copied from the message's 
Date header.  In the thread pane, it's shown per the client's specified 
date/time format.

In Brief headers, the date is shown as copied EXCEPT that the TZ is truncated 
(as are the seconds).  This could be slightly confusing, but I don't know if 
it's worth revisiting.

Felix, do you want to Verify this bug?

Comment 35

[Felix Miata]

Attachment: screenshot

I really don't consider the fix finished at this point. There's wasted chrome
space where the TZ could be displayed (see pointer). There shouldn't be another
colon and whoknowswhat to the right of the minutes unless the whole thing is to
be displayed. Either display the whole thing, or hide everything to the right
of the minutes.

Plus, it didn't make the branch, which is the only place I would see it in
normal use. :-(

Comment 36

[Christian Eyrich]

Yes, I think that's confusing, ugly and it should be changed but would prefer
filing a new bug.
I think there is a length limitation for the date filed or the string is cropped
in some js code.

Comment 37

[Christian Eyrich]

It looks to me as if this is an issue of the used theme.
Setting the width parameter for #collapseddateBox and #collapseddateValue in the
themes messageHeader.css to about 20em expatiates for the complete date.

That's the problem of direct copying the date - it contain everything: year in 4
digits, weekday, difference to GMT in digits and time zone in words. So it can
get very wide.