286070 - AllDay events are broken on trunk

Status: RESOLVED FIXED

(Calendar :: General, defect)

Description

[Andrew Douglas]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050313 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050313 Firefox/1.0+

All Day events end dates impossible to set to the same date as the start date.
The events display improperly in the calendar (meaning they'll show up more than
once on the same day), and the end date value that is selected by the date
picker appears to be a day off (for example if start date = 8/2/2005 and end
date is selected as 8/2/2005, it will show 8/3/2005.. if 8/1/2005 is selected,
8/2/2005 is displayed but it still ends up making the event 3 days long.


Reproducible: Always

Steps to Reproduce:
1. Create a new event
2. Select All Day Event checkbox
3. Try to pick an end date.

Actual Results:  
End date is very wrong.

Expected Results:  
End date stays the way I set it.

This is on a Trunk build.. it's a regression from .2

Comment 1

[Jeremy Teo]

Attachment: First try at fixing this insidious bug

This horrible bug is due to some archaic logic in eventDialog.js . While fixing
that, I realised that CalendarView.prototype.createEventBox is also broken.

Key Changes

1. Event dialog now works properly. Internally we save allday events on Day X
as lasting from Day X to Day X+1
2. Made monthview draw boxes with "0:00" for multiday events, since events
spanning for example 0700H on Day X to 1500H on Day X+1 should display that
way. Ie. the second event box shows "0:00 Event Title"
3. Unifinder now displays all day events properly

Comments appreciated. We also really need to use something other than jsDate.

Comment 2

[Jeremy Teo]

Attachment: Second try at this evil

1st patch didn't work properly with recurrences. This one fixes that, and is a
slightly better fix. Comments appreciated.

Comment 3

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

Comment on attachment 178368 [details] [diff] [review]
Second try at this evil

>+    origEndDate.jsDate = aItemOccurrence.occurrenceEndDate.jsDate;
>+        /*startDate.year = startDate.jsDate.getFullYear();

General comment: Just remove code if it can be removed. there's cvs to get it
back if you ever need it. Also, the indenting is wrong here.

>+    dump("calendarWindow.js>>aItem startDate>> "+aItemOccurrence.occurrenceStartDate + "\n");

Please remove all those dumps, or make it a function, and default that to do
nothing.

>     endDate.hour = 23;
>     endDate.minute = 59;
>     endDate.second = 59;
>-    endDate.normalize();

you can't just remove the normalize call. You need it to make the call to set
the hour etc work.

>+    while ((endDate.jsDate.getDate() - origEndDate.jsDate.getDate() ) != 0) {
>+        aInteralFunction(aItemOccurrence, startDate, endDate, isMultiDay); 
>+        var tempDate = new Date(startDate.jsDate);
>+        tempDate.setDate(tempDate.getDate() + 1);
Why are you using jsDate here, not the normal calls? And why did you remove the
calls to set hour etc to 0?

>Index: mozilla/calendar/resources/content/eventDialog.js

>@@ -475,28 +479,47 @@ function onOKCommand()
>+    //var tzid = calendarDefaultTimezone();
>+    //var tzid="UTC";
>+    var tzid=event.startDate.timezone;

So you are displaying events in the timezone in which is was created? i think
that would lead to weird situations.

>+        if(event.isAllDay){
>+            //For some strange reason, this is the only way i can get the endDate to be set to endDate + 1
Maybe that reason is that you didn't use .normalize()


> function checkSetTimeDate()
> {
>    var startDate      = getElementValue("start-datetime");
>    var endDate        = getElementValue("end-datetime");
>-   var dateComparison = compareIgnoringTimeOfDay(endDate, startDate);
>+   var dateComparison = compareIgnoringTimeOfDay(startDate, endDate);
> 
>+   //If an all day event, only check to see if startDate <= endDate
>+    if( getElementValue("all-day-event-checkbox", "checked") ) {
check your indenting and whitespace use.
>+          //Sanity check. Make sure pickedDateTime is >=startdate
>+          var isBigger = compareIgnoringTimeOfDay(pickedDateTime, startDate);
>+          if(isBigger == -1)
>+            pickedDateTime.setDate( startDate.getDate() );
You can move the functioncall into the if.

Also, i saw changes to createEventBoxInternal in monthview, but not in other
views. Shouldn't they be updated too?

Comment 4

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

Comment on attachment 178368 [details] [diff] [review]
Second try at this evil

> function checkSetTimeDate()
>+   //If an all day event, only check to see if startDate <= endDate
>+    if( getElementValue("all-day-event-checkbox", "checked") ) {
>+        if(dateComparison == 0 || dateComparison == -1) { //if startDate <= endDate
why not |dateComparison <= 0| ?



>@@ -287,18 +287,24 @@ MonthView.prototype.createEventBoxIntern
>+        else{
>+            StartFormattedTime="0:00";
This isn't localizable.


>Index: mozilla/calendar/resources/content/unifinder.js
>@@ -422,29 +422,44 @@ var treeView =
>-            return formatUnifinderEventDateTime(eventStartDate, calendarEvent.allDay);
>+            return formatUnifinderEventDateTime(eventStartDate, false); //we always want to show the start time

huh? An allday event doesn't have a starttime, so how can you show it?

there is no need to copy the formatUnistuff call. Just leave as it was.

Comment 5

[Jeremy Teo]

Attachment: 3rd attempt

Have cleaned up based on mvl's suggestions. Comments appreciated.

Comment 6

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

Comment on attachment 178721 [details] [diff] [review]
3rd attempt

Some gerenal comments:

I think something went wrong with merging eventDialog.js, because this patch
backs out another one.
You are changing the timezone stuff. If it is wrong, it should be in another
bug, for shorter patches
Check for random whitespace changes.

But what i can see of all-day event stuff, it looks good.

Comment 7

[Jeremy Teo]

Attachment: 4th Try. Some misc changes.

Cleaned up and diffed against trunk. Also tried to actually bother about
setting the correct timezones. Views still need to be fixed to display based on
calIDateTime instead of jsDate.

Comments appreciated.

Comment 8

[Jeremy Teo]

Attachment: Use yearday instead of day so that we calculate numOfDays properly

Using origEndDate.yearday and startDate.yearday to give use numOfDays is more
robust. (ie, a 1 day allday event on 31st Mar is stored as lasting from 31st
Mar to 1st Apr, and should give numOfDays as 1)

Comment 9

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

Attachment: partial patch, just eventdialog

This patch takes a slightly different approach: internally, always use
displayEndDate, because that is what the user sees. Also try to keep the
duration the same when the startdate is changed. Should fix the eventdialog.

Comment 10

[Joey Minta]

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

Comment 11

[gekacheka]

Comment on attachment 182293 [details] [diff] [review]
partial patch, just eventdialog

Note: this patch no longer applies since
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&f
ile=eventDialog.js&branch=&root=/cvsroot&subdir=mozilla/calendar/resources/cont
ent&command=DIFF_FRAMESET&rev1=1.149&rev2=1.150

Comment 12

[gekacheka]

There are several bugs affecting the event dialog.
* The onDatePick handler is incorrectly computing the duration and adjusting the
date.
* Floating dates are written and read incorrectly in the event dialog, so the
saved dates include incorrect times.  (This behavior may depend on OS timezone:
because of incomplete implementation of timezone sniffing, in most timezones the
floating timezone is the default.  See calendarDefaultTimezone and
GuessSystemTimezone in calendarUtils.js.
http://lxr.mozilla.org/mozilla/source/calendar/resources/content/calendarUtils.js#153)
* Checking end against start date is incorrect.
* Floating dates appear incorrect in the other views.

I will try fixes in stages, not all at once, to keep reviewing easier.

Comment 13

[gekacheka]

Attachment: fix on datepick, save duration

(patch -l -p 2 -i file.patch)

Changes

Save duration in var gDurationMSec between events.  Initialize it when event
dialog is first loaded, and update it when on date pick events.

The onDatePicked code no longer shared any code except the first line, so
separated it into three functions for clarity.

The onDatePicked handlers do not need to set the value of the pickers, that is
handled automatically.	(I think this was left over from an untested
conversion... the old 0.2 code saved the start and end date in vars between
events, so it needed to set the vars.)

Result:

Double clicking and closing an all-day event no longer changes its dates.   

End datetime is adjusted when the start datetime is adjusted to preserve the
same duration (as in 0.2).

Comment 14

[gekacheka]

Attachment: handle calDateTime in different timezone, including floating.

(patch -l -p 2 -i file.patch)

Issue:	JSDates do not have an adjustable timezone: they are always in the OS
timezone.  Setting the date to/from a calDateTime using the millisecond time
does not preserve the y/M/d/h/m/s/ fields if it is in a different timezone.  In
particular, a floating date acts like a UTC date (probably because there
currently isn't a way for the library to find the right libical timezone id for
the the current OS timezone, see the guessSystemTimezone function). 

Issue: all day events did not have 0:00 as start/end time.

Changes:

Add function jsDateForDisplayingDateTime(calDateTime).	It constructs a date
using the individual fields, not the UTC millisecond time, so it preserves the
y/M/d/h/m/s field values for dates in timezones different from the current OS
timezone, includinng floating dates.  The "ForDisplaying" part of the name is a
reminder that the millisecond time of the jsDate will be inaccurate (e.g., for
comparing/sorting dates in different timezones).

Add function dateTimeOfJSDate(jsDate, isAllDay, timezoneId).  It constructs a
calDateTime and sets each y/M/d/h/m/s field and sets the timezone id, so it
preserves the y/M/d/h/m/s fields in the jsDate.

Convert loadCalendarEventDialog to use jsDateForDisplayingDateTime instead of
calDateTime.jsDate getter.

Convert onOKCommand to use dateTimeOfJSDate instead of jsDateToDateTime (which
uses the calDateTime.jsDate setter).

Result:

Creating an all day event, saving it, and then double clicking it shows
start/end times at 0:00 local time, not at some other local hour that
corresponds to 0:00 UTC.

(Note: may require changing your os timezone to a timezone not covered by
guessSystemTimezone to reproduce the problem this patch fixes.)
(Note: this patch does not change how all the views read dates, so they will
still show times shifted by the local timezone offset.	Use the event dialog to
see the correct dates.)

Comment 15

[gekacheka]

Attachment: fix condition for warning "end date before start date"

(patch -l -p 2 -i file.patch)

Issue:	setting all-day checkbox on and setting end date to same as start date
produces warning that end date is before start date even though it is not.

Problem is the code is testing for exclusive end date but using the display end
date which is inclusive.  (I suspect this is another case of untested
conversion from the old code which used gVars to store the start/end dates, not
the direct picker values.  The old code probably made the allday end date
exclusive when it was written to the gVar.)

Change:

Adjust test to allow inclusive end date.

Result:

Now allows same start and end date for all-day events without warning.

Comment 16

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

Comment on attachment 184881 [details] [diff] [review]
fix on datepick, save duration

The identation should be made consistent to 4 spaces.

But more important, the all-day checkbox messes with the date. If i set up a
three day event, but only check the checkbox after setting the datepickers, the
event is now a two day event. (see my patch, where i removed the oncommand on
the checkbox. I think you should add that)

If you plan to fix this in a seperate patch, i can mark this r+, because it
does really improve the situation.

Comment 17

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

Comment on attachment 184884 [details] [diff] [review]
handle calDateTime in different timezone, including floating.

I'm not sure about this one. I still tend towards killing the use of jsDate and
use calIDateTime for all date calculations in calendar. jsDate is really only
used because not all widgets can handle calIDateTime yet.

Comment 18

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

Comment on attachment 184887 [details] [diff] [review]
fix condition for warning "end date before start date"

r=mvl, but again the identation should be 4 spaces.

Comment 19

[gekacheka]

(In reply to comment #16)
> (From update of attachment 184881 [details] [diff] [review] [edit])
> The identation should be made consistent to 4 spaces.

Indent should be 2 spaces.

I strongly dislike 4-space indentation because it causes ambiguity when reading
if's, and because it reduces screen space available for code.

In an ambiguous if, you have to read the code carefully to see where the body
starts.  

   if (this.isALongTestCondition(withAComplexTestCondition) &&
       this.testConditionTakesMoreThan(oneLine)) {
       this.isTheFirstLineOf(theBody);
       this.isTheSecondLineIn(theBody)
   }

(This is even more of the problem if the right end is off the right side of the
window/screen, as happens sometimes with long xul javascript.)

This is not a problem with 2-space indent:

   if (this.isALongTestCondition(withAComplexTestCondition) &&
       this.testConditionTakesMoreThan(oneLine)) {
     this.isTheFirstLineOf(theBody);
     this.isTheSecondLineIn(theBody);
   }

Mostafah agreed to 2-space indents here:
http://groups-beta.google.com/group/netscape.public.mozilla.calendar/msg/2717faa452a4319f?hl=en

That discussion also points out 2-space offset in mode-line is also recommended
by the the mozilla code guidelines here:
http://www.mozilla.org/hacking/mozilla-style-guide.html#Visual

Comment 20

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

(In reply to comment #19)
> Indent should be 2 spaces.

The file is currently consistent in having a 4-space indent. This bug is not the
place to discuss the amount of indenting. Please just keep the indenting
consitent with the file, and maybe one day convert the entire file.

Comment 21

[gekacheka]

Attachment: fix on datepick, on allday, save and update duration (checked in)

(patch -l -p 2 -i file.patch)

As in comment 13, plus changed commandAllDay to not change end date (as mvl),
and to update duration.

Comment 22

[gekacheka]

Attachment: fix condition for warning "end date before start date", v2

(patch -l -p 2 -i file.patch)

As in comment 14, updated patch.

(Regarding idea to use all calDateTimes: I'm not sure we want to fork the
datetimepickers.  Therefore, the event dialog still needs to convert to/from
jsdates for the datetime pickers, as this code does.  Also, the locale date
formatter takes a jsDate, so everything that formats a date textually still
needs to convert to a jsdate.)

Comment 23

[gekacheka]

Attachment: handle calDateTime in different timezone, including floating, v2

(last patch copied wrong description)
(patch -l -p 2 -i file.patch)

As in comment 14, updated patch.

(Regarding idea to use all calDateTimes: I'm not sure we want to fork the
datetimepickers.  Therefore, the event dialog still needs to convert to/from
jsdates for the datetime pickers, as this code does.  Also, the locale date
formatter takes a jsDate, so everything that formats a date textually still
needs to convert to a jsdate.)

Comment 24

[gekacheka]

Attachment: fix condition for warning "end date before start date", v2 (checked in)

(real warning check patch)
(patch -l -p 2 -i file.patch)

As in comment 15, updated patch.

Comment 25

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

Comment on attachment 185115 [details] [diff] [review]
fix on datepick, on allday, save and update duration (checked in)

r=mvl

Comment 26

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

Comment on attachment 185119 [details] [diff] [review]
fix condition for warning "end date before start date", v2 (checked in)

r=mvl

Comment 27

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

Comment on attachment 185118 [details] [diff] [review]
handle calDateTime in different timezone, including floating, v2

The datetimepickers could accept both jsDate and calDate, they don't need to
get forked.

But this patch looks more like it is a workaround for a bug in .jsDate, or
possible a request for a new property on calIDateTime, (.jsDateNoTimezone or
whatever)

Comment 28

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

Comment on attachment 185115 [details] [diff] [review]
fix on datepick, on allday, save and update duration (checked in)

checked in this patch, with changes to give it a consitent 4-space indentation.
(while i agree on the if-issue, consitency is more important. We can discuss
other indentations somewhere else)

Comment 29

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

Attachment: set .isDate, convert .allDay to .isAllDay

This patch converts some code that uses .allDay instead of .isAllDay.
It sets .isDate on the start and end times when saving the event from the
dialog
It removes some weird time calculation, to make the end date show up properly
on the unifinder.

Problems left: the views. They are all confused, and all in different ways.

Comment 30

[gekacheka]

(In reply to comment #29)
> Created an attachment (id=185268) [edit]
> set .isDate, convert .allDay to .isAllDay
> 
> This patch converts some code that uses .allDay instead of .isAllDay.

These look good.

> It sets .isDate on the start and end times when saving the event from the
> dialog

Rather than doing this, I think we should develop a general solution to the
problems solved in patch "handle calDateTime in different timezone, including
floating, v2". 
https://bugzilla.mozilla.org/attachment.cgi?id=185118

If we do it piecemeal, people have to remember to do each piece everytime they
convert a date.  Pieces include:
  set date based on either 
    msec value, or 
    y/M/d/h/m/s values.
  set isDate
  if isDate and set by y/M/d/h/m/s values, 
    make sure h/m/s are zero (otherwise msec date is wrong)
  set timezone

In comment #27 you suggested that it could become part of the dateTime signature
rather than calendarUtils functions.  (Maybe the easist way to do this is to
make a javascript wrapper/prototype around calDateTimes like there is for events
and todos?  That assumes that the methods are primarily used from javascript,
not from other code.  The set method cannot be a property because there are
multiple parameters.)

> It removes some weird time calculation, to make the end date show up properly
> on the unifinder.

The "weird calculation" was part of the code to display the *next* recurrence
dates... the line to get the recurrence start date is commented out, and the
calculation computes the next recurrence end date.  This calculation should also
have been commented out, to record what it did.

Comment 31

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

(In reply to comment #30)
> (In reply to comment #29)
> > It sets .isDate on the start and end times when saving the event from the
> > dialog
> 
> Rather than doing this, I think we should develop a general solution to the
> problems solved in patch "handle calDateTime in different timezone, including
> floating, v2". 

Those two patches are not trying to archive the same thing. My patch is needed
to actually save the items as allday. (or maybe the fact that it is needed is a
bug in the providers or the event impl, but tha timezone patch doesn't address
that either)

> The "weird calculation" was part of the code to display the *next* recurrence
> dates... the line to get the recurrence start date is commented out, and the
> calculation computes the next recurrence end date.  This calculation should also
> have been commented out, to record what it did.

The calculation isn't needed anyway, because the new occurance api's also give
the enddate for an occurence. No need to calculate it. (calIItemOccurrence)

Comment 32

[gekacheka]

(In reply to comment #31)
> (In reply to comment #30)
> > (In reply to comment #29)
> > > It sets .isDate on the start and end times when saving the event from the
> > > dialog
> > 
> > Rather than doing this, I think we should develop a general solution to the
> > problems solved in patch "handle calDateTime in different timezone, including
> > floating, v2". 
> 
> Those two patches are not trying to archive the same thing. My patch is needed
> to actually save the items as allday. (or maybe the fact that it is needed is a
> bug in the providers or the event impl, but tha timezone patch doesn't address
> that either)

To save the time as all day, it needs to both set the isDate and set the h/m/s
to zero (otherwise the millisecond value is wrong, so it will sort incorrectly).

Attachment 185118 [details] [diff] does this by calling dateTimeOfJSDate(jsDate, event.isAllDay,
timezone).
https://bugzilla.mozilla.org/attachment.cgi?id=185118&action=diff#mozilla/calendar/resources/content/eventDialog.js_sec5
dateTimeOfJSDate sets isDate and sets the h/m/s to zero if isDate.
https://bugzilla.mozilla.org/attachment.cgi?id=185118&action=diff#mozilla/calendar/resources/content/calendarUtils.js_sec1

So attachment 185118 [details] [diff] [review] does cover the eventDialog.js change in attachment 185268 [details] [diff] [review].
https://bugzilla.mozilla.org/attachment.cgi?id=185268&action=diff#mozilla/calendar/resources/content/eventDialog.js_sec1


> 
> > The "weird calculation" was part of the code to display the *next* recurrence
> > dates... the line to get the recurrence start date is commented out, and the
> > calculation computes the next recurrence end date.  This calculation should also
> > have been commented out, to record what it did.
> 
> The calculation isn't needed anyway, because the new occurance api's also give
> the enddate for an occurence. No need to calculate it. (calIItemOccurrence)

so this part is ok.

Comment 33

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

Bug 296559 made setting .isDate also set h/m/s to 0.
(I'm still opossed to workarounds for calIDateTime in js. It should just work
right. If it doesn't, it should be fixed instead of worked around)

Comment 34

[gekacheka]

> > > The "weird calculation" was part of the code to display the *next* recurrence
> > > dates... the line to get the recurrence start date is commented out, and the
> > > calculation computes the next recurrence end date.  This calculation
should also
> > > have been commented out, to record what it did.
> > 
> > The calculation isn't needed anyway, because the new occurance api's also give
> > the enddate for an occurence. No need to calculate it. (calIItemOccurrence)
> 
> so this part is ok.

(Note that this part duplicates Bug 288925 where I suggested a patch that made
the same change, but you argued against it then.  Maybe we agree on this
improvement now.)

Comment 35

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

Comment on attachment 185268 [details] [diff] [review]
set .isDate, convert .allDay to .isAllDay

checked in, except the .isDate part, to get it out of my tree.

Comment 36

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

Comment on attachment 185118 [details] [diff] [review]
handle calDateTime in different timezone, including floating, v2

No, the more i think of tit, a javascript wrapper like this is just wrong.
calIDateTime should just work.
Also, the default timezone isn't a const. It should be a pref, which can change
while calendar is open.

Comment 37

[Joey Minta]

Attachment: patch

Once the isDate patch is in from bug 299182, this should properly store the
'all day' flag in the event dialog.  In testing, it no longer seemed necessary
to add 1 to the end date, so I took that part out.

Comment 38

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

Comment on attachment 188757 [details] [diff] [review]
patch

You still need the endate+1. That's what the api specifies. It also is needed
to make the even show up correctly in the ics file.
Even if the display adds a day, that's a bug in the view code, or in the
recurrence calculation code.

Comment 39

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

checked in a fix. This bug is fixed by now. If there are problems left, file a
new bug.

Comment 40

[Reed Loden [:reed]]

The bugspam monkeys have been set free and are feeding on Calendar :: General. Be afraid for your sanity!