Closed Bug 402841 Opened 17 years ago Closed 16 years ago

copy an event to the calendar and it shows up as the day before

Categories

(Calendar :: Internal Components, defect)

defect
Not set
major

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jonahnaylor, Assigned: Hb)

Details

Attachments

(2 files, 7 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-GB; rv:1.8.1.9) Gecko/20071025 Firefox/2.0.0.9
Build Identifier: 

Hi I have three machines with calendars i wanted them all to read and write to one google calendar to make it easy to manage, so i copied all entries from one machine to a google calendar in sunbird using this extension. when they pasted across they were all on a day early. i have windows, google settings and sunbird all set to europe/london time zone with auto daylight saving. so what is going on? can anyone please fix this? thanks.

Reproducible: Always

Steps to Reproduce:
1. make a calendar on sunbird with lots of entries/appointments
2. make a blank calendar on google
3. do a select all or highlight all entries on calendar on sunbird, select new calendar that is linked to google and paste entries across
Actual Results:  
i have two sets of entries, one for each calendar, but google dates are wrong, all entries no on right day

Expected Results:  
should be matching by day etc
Did you keep in mind that Sunbird has an "active date"? 

The chronological first event will be placed on this date regardless of it's
own original start date. All other events in the clipboard are placed relative
to the active date. 

Selecting multiple events in the calendar views will move the active date. But
selecting them  in the unifinder NOT move the active date. 
See also bug 386989 (Thanks Hb)
I can confirm this bug for Sunbird 0.7 DE under Windows XP DE for calendars stored in ics files and in the local database. It causes data corruption. Please set the severity to Critical. 

After selecting events in the unifinder, copying and pasting the, some copied events can appear one day before their original date. Selecting the copies, coping and pasting them brings the date another day to the past.

For the 75 events in the attached test case the error is reproducable. If all events are copied, the copy of the 18th event is the first misplaced one followed by the 19, 22, 24 to 27, 29 to 35 and so on.

If not all events are copied the error may affect other events.

The error occurs even if the events are pasted to the same calender where the events were copied from. 

When copying nearly thirty events together from another calendar (no sample data) all events hit the correct time and most events hit the correct date. Only events which started at 17:00 were pasted to the day before.

When copying multiple events from one day, including a 17:00 one, to another calendar it hits the active calendar date. This behaviour seems correct until copy&pasting does not go across calendars but should not be discussed further in this bug.

When copying a 17:00 hour event and another event together the 17:00 hour event hit the day before.

Bug 386989 deals with a similar issue.
The current nightly of Sunbird 0.8pre en_US has this issue on Windows XP DE and on Ubuntu 7.10 DE too. Equally to the bevSB 0.7 the 18th event is the first event to hit a wrong date when pasted.

Please set: 
Component= General or unspecified
Version= trunk or mozilla 1.8 branch 
OS= All

QA Contact= appropriate
Flags: blocking-calendar0.8?
Status: UNCONFIRMED → NEW
Component: Provider: GData → Internal Components
Ever confirmed: true
OS: Windows Vista → All
QA Contact: gdata-provider → base
Version: unspecified → Mozilla 1.8 Branch
It needs only two events to reproduce this error. It occurs if the later event has an earlier start time. When copying them together the later one is pasted in one day before it's origin. Try it with copying the events "earliest_four" and "later_two" from the test case. The "later_two" event will be pasted one day before.

To blame is http://mxr.mozilla.org/mozilla/source/calendar/resources/content/clipboard.js#266 because the offset is calculated from the start time of the earliest event. It has to be calculated from the beginning of the day (= midnight) of the earliest event.

Status: NEW → ASSIGNED
Attached patch Patch_v1 (obsolete) — Splinter Review
The patch solves the problems with events set to wrong dates. The offset is calculated from the beginning of the selected day against the earliest item in the clipboard. This item has to be seen in the same timezone as the selected day.


>            if (!firstDate.isMutable) {
>               firstDate = firstDate.clone();
>            }
>            firstDate.isDate = false;
>
This code came in with solving bug 339083 and bug 336678 
http://bonsai.mozilla.org/cvsview2.cgi?file=clipboard.js&branch=1.18.8.14&root=/cvsroot&subdir=mozilla/calendar/resources/content&command=DIFF_FRAMESET&rev1=1.18.8.4&rev2=1.18.8.5
http://bonsai.mozilla.org/cvsview2.cgi?file=clipboard.js&branch=1.18.8.14&root=/cvsroot&subdir=mozilla/calendar/resources/content&command=DIFF_FRAMESET&rev1=1.18.8.2&rev2=1.18.8.3
and should suppress errors when changing firstDate. This is obsolete now because the new DTSTART is generated in the function makeNewStartingDate() without harming firstDate. A comment is left as reminder to the bug.
Assignee: nobody → hb
Attachment #301050 - Flags: review?(lilmatt)
All storages are affected, not only google.
Summary: copy an event to the calendar and google calendar shows it up as the day before → copy an event to the calendar and it shows up as the day before
Attachment #301050 - Attachment is obsolete: true
Attachment #301052 - Flags: review?(lilmatt)
Attachment #301050 - Flags: review?(lilmatt)
Attachment #301052 - Flags: review?(lilmatt) → review?(philipp)
Hardware: PC → All
Version: Mozilla 1.8 Branch → unspecified
Comment on attachment 301052 [details] [diff] [review]
Patch v1 with better diff headers


>+            // firstDate may not be mutable, see bug 339083
Since you don't clone firstDate, this comment can be removed.


The code looks fine, r+ for that. I'll test this later on, then we can check it in.
Attachment #301052 - Flags: review?(philipp) → review+
Attached patch Patch_v2 (obsolete) — Splinter Review
I moved the test if a calendar is selected on top of the section.

Also I replaced my homegrown timezone manipulation with the build in one getInTimezone().

The comment is removed.
Attachment #301052 - Attachment is obsolete: true
Attachment #301885 - Flags: review?(philipp)
Attachment #301885 - Attachment is patch: true
Attached patch Patch_v2 (obsolete) — Splinter Review
Attachment #301885 - Attachment is obsolete: true
Attachment #301886 - Flags: review?
Attachment #301885 - Flags: review?(philipp)
Comment on attachment 301886 [details] [diff] [review]
Patch_v2

Adding reviewer.
Attachment #301886 - Flags: review? → review?(philipp)
Attachment #301886 - Attachment is obsolete: true
Attachment #301886 - Flags: review?(philipp)
Hb: Any reason you obsoleted the patch?

In any case, would you be terribly upset if we checked this patch in after 0.8? We are already past our schedule w.r.t the 0.8 release schedule and for the sake of keeping the risk of regressions low it would be good if we could get the blocking-calendar0.8+ list down to zero before taking care of any blocking-calendar0.8? or other bugs.

Please don't get me wrong, we are very happy to have more developers contributing to the project, but we need to stay focused. I can review your patch before the release, but if you agree it would be great to check it in after 0.8.
Attached patch Patch_v3 (obsolete) — Splinter Review
Second version of this patch looked nicer but failed when pasting over the DT/DST border. Having DT events in clipboard and pasting to DST dates sets them one day before. 

To make subtractDate() DT/DST independent all time zone informations have to be deleted from the dates. Because of bug 305432 woul be possible with

> earliestDate.nativeTime = earliestDate.nativeTime

This solution is not choosen. 

The third version of the patch has an improved handling for crossing the DT/DST border.
Attachment #302677 - Flags: review?(philipp)
It would be good to have this in 0.8
Flags: blocking-calendar0.8? → blocking-calendar0.8+
Comment on attachment 302677 [details] [diff] [review]
Patch_v3

As shown in bug 416206 comment 41 the offset needs to be normalized.

The existing line "newItem.endDate = newItem.startDate.clone();" seems to be wrong too because it overwrites the endDates time zone.

Reworked version will follow.
Attachment #302677 - Flags: review?(philipp)
Version 4 of this patch uses a new way to calculate the offset when moving items over the DT/DST border. The difference between the timezoneOffset properties deltaDST reflects the shift between standard and daylight saving time. It is used for an additional correction of the offset. Adding to offset.inSeconds normalizes offset.icalString too.

The former way depended on a one hour shift between DT and DST. Some regions shift only 30 minutes (i.e. Lord Howe Island in Australia). Also it's awful to calculate floating data with if-else constructs.

This way to eliminate DT/DST border errors may also be used in bug 416206 or similar circumstances.

The function makeNewStartingDate() was obsolete after simplifying the generation of the dates for the new item. The former way overwrote the time zone information of the endDates.

Grid for 2008 with DT/DST changes on last Sundays of March and October:
                                                           
                          first earliest  offset deltaDST  corr. offset
Crossing from standard                                        
to daylight in spring: 20080331 20080324  P6DT23H    3600        P1W

Crossing from daylight
to standard in spring: 20080324 20080331 -P6DT23H   -3600       -P1W

Crossing from daylight
to standard in autumn: 20081027 20081020   P7DT1H   -3600        P1W

Crossing from standard
to daylight in autumn: 20081020 20081027  -P7DT1H    3600       -P1W


The variable deltaTZ is only needed for debugging purposes and may be omitted by: 
offset.inSeconds += firstDate.timezoneOffset - earliestDate.timezoneOffset;

The behaviour with the calendars timezone set to Auckland, New Zealand (DT +12 GMT, DST +13 GMT) was tested under a different OS timezone.

The behaviour in the Lord Howe timezone could not been tested because of TZOFFSETFROM is wrongfully equal to TZOFFSETTO for standard time in that zone. If someone has attachment 303732 [details] [diff] [review] from bug 410931 already compiled in his calendar this test would be possible for her or him.
Attachment #302677 - Attachment is obsolete: true
Attachment #303896 - Flags: review?(philipp)
Attached file Eleven Evil Events
A good dozen events and a task in March and April 2008 to test timezone and standard/daylight saving issues under copying, pasting, dragging and dropping.

The special cases are:
Three events "Baghdad_DT_Berlin_DT" where calendar DST and event DST switch on different days.

"Auckland" for which the DST is on another UTC-day.

The "Different_timezones" event starts at 12:00 Baghdad time and ends the following day at 13:00 Berlin time. This situation can't be produced with sunbirds UI but is valid ical data.
Attachment #300132 - Attachment is obsolete: true
Attachment #300967 - Attachment is obsolete: true
Comment on attachment 303896 [details] [diff] [review]
Patch v4, Accepts all DT/DST shifts

Looks good, I tested this and could find no problems on copy and paste.

If you can use this technique also in the views, it would be great :)
Attachment #303896 - Flags: review?(philipp) → review+
Attachment #303900 - Attachment description: The Evil Eleven Events → Eleven Evil Events
Attachment #303900 - Attachment mime type: application/octet-stream → text/calendar
Keywords: checkin-needed
Checked in on HEAD and MOZILLA_1_8_BRANCH

-> FIXED
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 0.8
Attachment #303900 - Attachment filename: home08.ics → elevenevents.ics
You need to log in before you can comment on or make changes to this bug.