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.
Nothing OS/2 specific about this
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.
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: <200203080137.g281bQI25899@bow.cs.hmc.edu> X-Mailer: rmail Ivan: see bug 83914, Cronological order doesn't compensate for timezones.
"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.
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.
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.
*** Bug 155274 has been marked as a duplicate of this bug. ***
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?
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.
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.
Created attachment 121947 [details] [diff] [review] 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.
Created attachment 121948 [details] [diff] [review] 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.
Created attachment 121949 [details] [diff] [review] 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.
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.
Created attachment 124895 [details] 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.
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.
If you could make that a configurable option to mail-news this would be great.
Created attachment 124928 [details] [diff] [review] 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.
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 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
Created attachment 124991 [details] [diff] [review] 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 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.
Created attachment 124995 [details] [diff] [review] 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 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 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 :-)
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 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?). firstname.lastname@example.org with those tweaks
Created attachment 126228 [details] [diff] [review] patch v5 Patch with tweaks according to comment 27. r=neil.parkwaycc.co.uk, sr=dmose taken from patch v4
Fix checked in, except ToNewUnicode(nsDependentCString(...)) is slightly nicer.
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?
Without checkin to 1.4 it does me no good. There are no functional trunk builds for OS/2.
It's almost certainly too late to ask for approval1.4; as I checked this in I should mark this fixed-on-trunk.
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.
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?
Created attachment 130242 [details] 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. :-(
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.
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.