Closed Bug 328442 Opened 18 years ago Closed 17 years ago

jsDate vs calIDateTime

Categories

(Calendar :: Internal Components, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: michael.buettner, Assigned: dbo)

References

Details

Attachments

(2 files)

this is a follow-up bug from https://bugzilla.mozilla.org/show_bug.cgi?id=300117 to stop it from blocking the 0.1 release. the issue here is that jsDate uses the os timezone and calIDateTime uses the setting from the preferences. if both settings stay in sync everything is fine, but if they don't we run into all sorts of problems regarding timezone conversion issues. the above mentioned bug experiences this issue with respect to the UNTIL date of the recurring info, streaming this property to and from the event-dialog. the suggestion is to get rid of jsDate and consistently use calIDateTime, any other opinions?
This sounds like a fine idea.  After 0.1, I hope to refactor the date/timepicker code and move it to toolkit/ (bug 92174), at which time the jsDate code intermediary will disappear from there.  As far as I know, that's the main place we're still using it.
Depends on: 92174
Using calDateTime may make it more apparent that the problems are due to timezone conversions rather than jsDate/calDateTime conversions.  But dates must still be converted between the stored timezone and the display timezone.

1. The problems in bug 300117 would likely have arisen even if no jsDates were used.

The 'UNTIL' problem is a special case due to a weakness in the RFC2445 format:  TZID is usally a parameter of a date property.  UNTIL dates appear as a parameter of a RRULE property.  Parameters cannot themselves have parameters, so there is no place to put a TZID on an UNTIL date.   Thus, UNTIL dates must be either floating or zulu.  Therefore, the dialog still must convert between the stored timezone (floating or zulu) and the displayed event timezone.

2. The timezone conversion issues for other dates still occur even if the date is a calDateTime, because the display timezone may be different than the stored timezone.

3. nsIScriptableDateFormat is used to format a jsDate in the current operating system format (so it is consistent with dates and times elsewhere on the screen, particularly to avoid D/M/Y vs M/D/Y confusion, and 12/24hr time confusion).   

It is used in calDateFormatter.js and dateUtils.js/DateFormater (french spelling), which are used in many places: event/todo table, mouseover preview, text/html export, printing.

One approach would be to convert the dateFormatters to accept calDateTime instead of (or in addition to) jsDate.  Each caller may still have to convert each calDateTime to the timezone they want to display before passing it to the formatter.


(An earlier bug, suggesting methods to convert between calDateTime and jsDate "by fields" instead of by utc date, is bug 296659.  The calDateTime to jsDate by-fields conversion could be encapsulated in the dateFormatter if it is not needed anywhere else.  I expected that jsDates would be used in other applications, so the conversion methods would also be useful when other applications are integrated with calendaring.)
using calIDateTime doesn't take away the need to do timezone conversions, but it makes it possible. jsDate is (afaik randomly) in either local timezone or utc, and can't be in any other zone. So that's pretty useless for our needs.

calIDateTimeFormatter already works with calIDateTime, so I don't see the problem with formatting. The code in dateUtils and dateFormater should be phased out (also because there are not accessible from xpcom components)
The bugspam monkeys have struck again. They are currently chewing on default assignees for Calendar. Be afraid for your sanity!
Assignee: base → nobody
Blocks: 337191
All bugs related to the fact that the OS timezone differs from the one specified in calendar are caused by this underlying problem. Based on the sheer number of new bug reports related to this problem makes me feel that this issue is a major pain in the neck and should be resolved until the next release. -> Proposing wanted-calendar0.8.
Flags: wanted-calendar0.8?
Flags: wanted-calendar0.8? → wanted-calendar0.8+
Bug 309527 has introduced a fix (I consider wrong) that hinders me from fixing bug 402197 (which is uncovered by fix for bug 398724). Especially w.r.t. bug 388656 we will face more and more floating all-day events, so I'd like to back out the fix from bug 309527 and propose a proper fix for the calIDateTime/jsDate conversion problem of the event dialog.
Attachment #287551 - Flags: review?(michael.buettner)
Comment on attachment 287551 [details] [diff] [review]
[checked in] fixing event/task dialog datepicker

>+        newDate.resetTo(aDate.getFullYear(),
>+                        aDate.getMonth(),
>+                        aDate.getDate(),
>+                        aDate.getHours(),
>+                        aDate.getMinutes(),
>+                        aDate.getSeconds(),
>+                        aTimezone);
This approach indeed solves the problem when transferring a date/time from a jsDate to a calDateTime object. In particular, I like the idea with the additional argument to the 'jsDateToDateTime' function.

The changes to the event dialog not only fix the initial problem but also bring us a better structured 'dateTimeControls2State' function - well done.

I tested a set of different os/lightning/event/task timezones where I freely mixed them, and I couldn't find anything that didn't work - cool.

The original idea was to completely get rid of jsDate in favor of calDateTime, but I now see several compelling reasons for not going into this direction:

- conversions between jsDate/calDateTime now functions as expected
- the datetimepickers could move to toolkit (they couldn't if we use calDateTime)
- there are more important issues to address given the time pressure we're already facing

I would go ahead and take this patch and mark this issue as fixed based on above summarized reasoning.

r=mickey.
Attachment #287551 - Flags: review?(michael.buettner) → review+
(In reply to comment #7)
> I would go ahead and take this patch and mark this issue as fixed based on
> above summarized reasoning.
The patch fixes only the event dialog (feeding the controls 'floating', then getting back the control's values only without the timezone part). Are there more of these transitions we need to take care about?
(In reply to comment #8)
> The patch fixes only the event dialog (feeding the controls 'floating', then
> getting back the control's values only without the timezone part). Are there
> more of these transitions we need to take care about?
The views use calDateTime almost exclusively, and the locations where jsDate is used (see [1] for an example) doesn't suffer from this conversion issue. The initial problem exists where we use the datetimepicker control. There are indeed more locations that need to be fixed.

-> The 'invite attendee' dialog, see [2] and [3] needs to adapt the conversion as well.
-> The timezone dialog doesn't need to be changed, since the control never gets read back - it just gets filled in.
-> The custom reminder dialog doesn't really use the control right now - no need to do anything here.

[1] http://lxr.mozilla.org/mozilla1.8/source/calendar/base/content/calendar-multiday-view.xml#3356
[2] http://lxr.mozilla.org/mozilla1.8/source/calendar/prototypes/wcap/sun-calendar-event-dialog-attendees.xul#167
[3] http://lxr.mozilla.org/mozilla1.8/source/calendar/prototypes/wcap/sun-calendar-event-dialog-attendees.js#350
Attachment #287551 - Attachment description: fixing event/task dialog datepicker → [checked in] fixing event/task dialog datepicker
Attachment #287551 - Attachment is obsolete: true
OS: Windows XP → All
Hardware: PC → All
Version: Trunk → unspecified
Assignee: nobody → daniel.boelzle
Status: NEW → ASSIGNED
Attachment #288817 - Flags: review?(michael.buettner)
Comment on attachment 288817 [details] [diff] [review]
[checked in] attendee dialog

r=mickey.
Attachment #288817 - Flags: review?(michael.buettner) → review+
Checked in on HEAD and MOZILLA_1_8_BRANCH.

Prototype dialog is fixed.
Target Milestone: --- → 0.8
Comment on attachment 287551 [details] [diff] [review]
[checked in] fixing event/task dialog datepicker

Please don't mark patches as obsolete when you check them in. This makes it hard to keep track of them because one does not know whether that patch was obsoleted by another patch or whether it was just marked as "obsolete" on checkin.
Attachment #287551 - Attachment is obsolete: false
Attachment #288817 - Attachment description: attendee dialog → [checked in] attendee dialog
Daniel, is there any reason why you didn't set this bug to FIXED?
Reason why I left this open is pending bug 403886; this bug by now only fixes the prototype dialog.
We go with the new dialog, no need to fix this stuff in the old one => FIXED.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Blocks: 400318
This checkin regressed Bug 405490.
Depends on: 405490
check in latest nightly build -> task is fixed and verified.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.