326132 - Start time of all-day events is set to 1:00 to odd times

Status: VERIFIED FIXED

(Calendar :: General, defect)

Description

[Ilja Sekler]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.1) Gecko/20060127 SeaMonkey/1.0
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20060206 Mozilla Sunbird/0.3a1+

All all-day events get automatically a wrong start time 1:00 while the end time remains 0:00 resulting in an error message "Your start time is after your end time" every time you try to edit an all-day event. Unchecking [ ] All Day, modifying the start time and checking [x] All Day again gets lost if you try to edit the event next time.

Reproducible: Always

Steps to Reproduce:
1. Create an all-day event
2. Doubleclick the event to open the edit dialog


Actual Results:  
The error "Your start time is after your end time" is displayed

Expected Results:  
No error.

It is a recent regression. A build from 2006-02-01 doesn't show this behavior.

Comment 1

[Joey Minta]

Probably me (bug 165963) and probably another vote for bug 325981.  What timezone are you in?

Comment 2

[Ilja Sekler]

Europe/Berlin (+0100)

Comment 3

[Ilja Sekler]

(In reply to comment #1)
> Probably me (bug 165963)

Maybe: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20060203 Mozilla Sunbird/0.3a1+ is already affected.

Comment 4

[Joey Minta]

Once the item goes in and out of the storage provider (ie via restarting) everything seems fine.  We should look into what is going right or wrong between the event sent via onModifyItem and the event received when querying, specifically at the startTime.  Perhaps someone isn't cloning?

Comment 5

[Stefan Sitter [:ssitter]]

I looked into this one or two weeks ago. The problem is in
http://lxr.mozilla.org/mozilla/source/calendar/base/content/calendar-event-dialog.js#134

If you add a dump before the setElementValue() calls you get:

startDate        = 2006/02/10 00:00:00 /mozilla.org/20050126_1/Africa/Ceuta
endDate          = 2006/02/10 00:00:00 /mozilla.org/20050126_1/Africa/Ceuta
startDate.jsDate = Fri Feb 10 2006 01:00:00 GMT+0100
endDate.jsDate   = Fri Feb 10 2006 00:00:00 GMT+0100

The problem seen here is basically the same as in Bug 304084 (different behavior because of GMT+xx offset). The reason for this is probably Bug 296659.

The call to endDate.normalize() some lines above seemed to fix this. But when doing the same for startDate is caused other problems when testing.

Comment 6

[Stefan Sitter [:ssitter]]

Additional note: I see this since the new item dialog landing too. Before (with the old item dialog) both start and end time were set to 01:00, thus the error message was never displayed.

Comment 7

[Stefan Sitter [:ssitter]]

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

Comment 8

[Stefan Sitter [:ssitter]]

I thought this issue was solved with the check in for bug 304084. But that seems to apply only to all day events created with Sunbird 0.3a1 before.

As pointed out by RuCla in bug 326979 this issue still exist. For example if I subscribe to http://www.mozilla.org/projects/calendar/caldata/USHolidays.ics and open an all day event the start hour is still set to 01:00. (My timezone is GMT+1).

If I import the event into a local storage calendar the problem is gone after removing all day flag, changing the start time to 00:00 and setting the all day flag again.

But if I open the calendar as a local ics calendar (file:///...) the problem still exist after removing all day flag, changing the start time to 00:00 and setting the all day flag again. 
The curious thing is that I can create new events in that file but I can't change existing events (e.g. title). (Might be a problem with editing occurrence vs. series of repeating events)

Comment 9

[Joey Minta]

Attachment: look at all-day flag when comparing

This patch masks the problem somewhat, but does seem more correct than the current situation.  The (disabled) timepicker will still show an incorrect hour for the start-date, but the warning will no longer appear, because the algorithm is now aware that they are meant to be dates, not datetimes.

Comment 10

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 211735 [details] [diff] [review]
look at all-day flag when comparing

r=mvl

Comment 11

[Joey Minta]

Patch checked in.  Reducing severity to trivial and updating summary to reflect remaining work.

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

Since this blocks editing events in some-cases, it'd be good to have this fixed for 0.1.  If the fix turns out be a lot of work, we can reconsider.

Comment 13

[Stefan Sitter [:ssitter]]

Attachment: force times to 00:00

Basically we force startdate and enddate to 00:00 in local timezone by calling normalize. Read values back into local timezone for comparing in updateAccept. Call updateAccept() after All-Day flag has been set/cleared.

Comment 14

[Joey Minta]

Comment on attachment 211795 [details] [diff] [review]
force times to 00:00

-        startDate = jsDateToDateTime(getElementValue("event-starttime"));
-        endDate = jsDateToDateTime(getElementValue("event-endtime"));
+        startDate = jsDateToDateTime(getElementValue("event-starttime")).getInTimezone(kDefaultTimezone);
+        endDate = jsDateToDateTime(getElementValue("event-endtime")).getInTimezone(kDefaultTimezone);

I'm not clear on the reason for doing this.  If two calDateTimes are equal, they ought to register as such in any timezone.  Is this similar to the normalize() trick to fix .nativeTime or is there another reason for doing this?

Comment 15

[RuCla]

How can I test the patches?

Comment 16

[Stefan Sitter [:ssitter]]

(In reply to comment #14)
> I'm not clear on the reason for doing this.  If two calDateTimes are equal,
> they ought to register as such in any timezone.  Is this similar to the
> normalize() trick to fix .nativeTime or is there another reason for doing
> this?

This is required for the correct functionality. Imagine the usecase you brought up on IRC: Same date for start and end but start time > end time, user checks all day box --> no warning is expected

Lets take the example already mentioned in this bug:
  Start: Feb 10 2006 01:00:00 GMT+01
  End:   Feb 10 2006 00:00:00 GMT+01
(These are the values I see in the date/timepicker)

Calling jsDateToDateTime() returns in UTC:
  Start: Feb 10 2006 00:00:00 UTC
  End:   Feb 09 2006 23:00:00 UTC
  
Because all-day is checked you set isDate to true:
  Start: Feb 10 2006 00:00:00 UTC
  End:   Feb 09 2006 00:00:00 UTC

If you now compare the values the warning is displayed but should be hidden actually.

Depending on the GMT offset you can find different constellation where start/end/both dates are shifted to the previous/next day (in UTC).

The normalize() in loadDialog is just used to display 00:00 for start/end time in case of all-day events. But that does not prevent the use case mentioned above.

Comment 17

[Joey Minta]

Comment on attachment 211795 [details] [diff] [review]
force times to 00:00

It took me 3 tries and 2 IRC conversations to understand all the intricicies of these calls.  That's a requirement we can't place on people reading LXR.  r=jminta for the code changes, but I'd like to see an updated patch with explanatory comments for actual checkin.

Comment 18

[Stefan Sitter [:ssitter]]

Attachment: patch for check in

Added comments to explain what's going on. I also optimized the patch to call getInTimezone() only when it is required. (For non all-day events the timezone doesn't matter)

Comment 19

[Joey Minta]

Patch checked in.

Comment 20

[Stefan Sitter [:ssitter]]

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

Comment 21

[Damian Szczepanik]

verified with
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060926 Calendar/0.3a2+