330496 - Agenda: yesterday's all day event is shown in 'Today' category

Status: RESOLVED FIXED

(Calendar :: Lightning Only, defect)

Description

[Stefan Sitter [:ssitter]]

Agenda: yesterday's all day event is shown in 'Today' category

Steps to Reproduce:
1. Go to current day (aka today)
2. Create an all day event that is scheduled yesterday
3. Check Agenda in Lightning sidebar

Actual Results:
The all day event is shown in 'Today' category

Expected Results:
Yesterday's all day event is not shown in 'Today' category.

Additional Information:
Tested with Thunderbird 1.5 (20051201) + Lightning 0.1 (20060313) on Win2K.
The timezone of the event matters. If the event is not created in the current selected timezone the result varies.

Reference:
http://forums.mozillazine.org/viewtopic.php?p=2145585#2145585

Comment 1

[Stefan Sitter [:ssitter]]

Additional Information:
Yesterday's all day event is shown in 'Today' category after creation.
After click on the 'Item' header row the event is not displayed.
Open the event for editing and leave dialog with OK.
Yesterday's all day event is shown in 'Today' category again.
After restart of Thunderbird the event is also not shown in 'Today' category.

Comment 2

[Stefan Sitter [:ssitter]]

After adding or modifying the event agendaTreeView.calendarObserver.onAddItem is called (http://lxr.mozilla.org/mozilla/source/calendar/lightning/content/agenda-tree.js#424). Then onAddItem calls getOccurrencesBetween on the new item (http://landfill.mozilla.org/mxr-test/mozilla/source/calendar/base/src/calEvent.js#240).

The following shows some dump output (I omitted the timezone part):

agendaTreeView.calendarObserver.onAddItem called
    this.agendaTreeView.today.start=2006/03/14 23:24:14 isDate=false
    this.agendaTreeView.soon.end   =2006/03/22 00:00:00 isDate=false

calIEvent::getOccurrencesBetween(aStartDate, aEndDate) called:
    argument aStartDate = 2006/03/14 23:24:14 isDate=false
    argument aEndDate   = 2006/03/22 00:00:00 isDate=false

    this.startDate      = 2006/03/13 00:00:00 isDate=true
    this.endDate        = 2006/03/14 00:00:00 isDate=true
    
    this.startDate.compare(aStartDate)=-1
    this.startDate.compare(aEndDate)  =-1
    this.endDate.compare(aStartDate)  = 0
    this.endDate.compare(aEndDate)    =-1
    
    --> This event is inside range specified by (aStartDate, aEndDate)

So the problem seems to be inclusive-vs-exclusive using of endDate for all day events.

Comment 3

[Anton van Bohemen]

One other thing I noticed: if you edit the event, changing all day to a time range, then OK, then edit again to an all day event, the agenda view is correct again.

Comment 4

[Michael Büttner (no reviews TFN)]

Attachment: patch v1

as stefan already mentioned in comment #2 the problem is that the enddate was included while comparing the intervals, which is obviously wrong. the intervals only have a common element if the overlap is not null. a similar comparison can be found at http://lxr.mozilla.org/seamonkey/source/calendar/base/src/calRecurrenceDate.cpp#183   where the same holds true.

furthermore, the actual comparison can be formulated in a shorter way, as it is now the case with this patch.

Comment 5

[Joey Minta]

Comment on attachment 225710 [details] [diff] [review]
patch v1

As I read this patch, we'll never return any occurrences for a 0-length event starting at midnight?

Comment 6

[Michael Büttner (no reviews TFN)]

(In reply to comment #5)
> (From update of attachment 225710 [details] [diff] [review] [edit])
> As I read this patch, we'll never return any occurrences for a 0-length event
> starting at midnight?
right, but IMHO this is a correct behavior since 0-length events doesn't make much sense at all. but i can imagine the following alternatives:
1) prevent creating 0-length events and gracefully handle already existing events. but keep this patch as it is.
2) allow the query.startDate and item.startDate to exactly match, thus defining the item interval as half-open [a,b(.
option 2) would solve the problem you mentioned but i'm not sure whether this approach is correct or not.

Comment 7

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

(In reply to comment #6)
> 2) allow the query.startDate and item.startDate to exactly match, thus defining
> the item interval as half-open [a,b(.

In general, the used intervals should be half-open, because the starttime is inclusive, while the endtime is exclusive. (I didn't look at the details of this patch, so not sure if that applies here.)

Comment 8

[Joey Minta]

I agree with mvl about the half-open approach.  Since other apps do allow for 0-length events, we're going to need to handle them regardless of whether or not we enforce non-0 length in our own dialogs.

Comment 9

[Michael Büttner (no reviews TFN)]

Attachment: patch v2

> I agree with mvl about the half-open approach.  Since other apps do allow for
> 0-length events, we're going to need to handle them regardless of whether or
> not we enforce non-0 length in our own dialogs.
0-length intervals are now handled as a special case, can't think of a better solution to this problem, probably because the comparison between half-open intervals where a == b is not well defined.

Comment 10

[Joey Minta]

Comment on attachment 226311 [details] [diff] [review]
patch v2

This looks like a weird mix between closed intervals and half-open intervals.  As I read the code, (and the comment), we use closed intervals everywhere except for 0-length event.  From the previous discussion here, I thought we all decided that half-open intervals are the way to go all around.

You can still special case 0-length events if necessary.  We also need to add some comments to calIItemBase.idl explicitly outlining the expected behavior here.

Comment 11

[Joey Minta]

Comment on attachment 226311 [details] [diff] [review]
patch v2

minused per previous comment.  If I'm misunderstanding something, please re-request review.

Comment 12

[Sebastian Schwieger]

I think this belongs to category "Events/tasks should not appear in views where they are not intended to appear". Requesting blocking0.3

Comment 13

[Michael Büttner (no reviews TFN)]

Attachment: patch v3

this one should do the trick. unfortunately it's necessary to special case 0-length items since otherwise we can't model the half-open interval condition reliably.

Comment 14

[Joey Minta]

Attachment: xpcshell testcase

This is an xpcshell test-suite for getOccurrencesBetween() for items that don't have recurrence info.  Without the patch, we fail the specific test designed for this bug and pass the rest.  With the patch, we fail ev3 and ev4.  I'm not sure whether or not we should pass ev4 (The half-open interval idea says we shouldn't.)  We should likely pass ev3 though.

Comment 15

[Thomas Benisch]

(In reply to comment #14)
> Created an attachment (id=233638) [edit]
> xpcshell testcase
> 
> This is an xpcshell test-suite for getOccurrencesBetween() for items that don't
> have recurrence info.  Without the patch, we fail the specific test designed
> for this bug and pass the rest.  With the patch, we fail ev3 and ev4.  I'm not
> sure whether or not we should pass ev4 (The half-open interval idea says we
> shouldn't.)  We should likely pass ev3 though.

I think we should NOT pass the test for ev4 due to the half open interval
(start time of an event included, end time excluded). If we work with closed
intervals we will get lots of other problems.

In principle I think that the patch v3 doesn't handle allday events correctly.
The patch works for allday events, if aStartDate and aEndDate of the
range matches accidentaly day borders, which is probably always the case
in the calendar code. But if one allows in getOccurrencesBetween() also
other values, e.g. you test case for ev3, then the patch doesn't work.

A possible approach could be to generate a start and end date from the
allday event, which is set to 0:00 of this and 0:00 of the next day,
but with isDate = false. Then the condition for !isZeroLength can be
used.

Comment 16

[Thomas Benisch]

Comment on attachment 233574 [details] [diff] [review]
patch v3

r- due to the allday event problem (see comment #15)

Comment 17

[Michael Büttner (no reviews TFN)]

Attachment: patch v4

first of all, thanks for the testcase ;-) as already mentioned, we shouldn't pass ev4 since otherwise we'd conflict with the half open interval idea. i followed thomas suggestion to resolve the problem with allday events.

Comment 18

[Thomas Benisch]

Comment on attachment 233993 [details] [diff] [review]
patch v4

+        var convertDate = function(date) {
+          if (date.isDate) {
+            var newDate = date.clone();
+            newDate.hour = 0;
+            newDate.minute = 0;
+            newDate.second = 0;
+            newDate.isDate = false;

I'm not sure, if you need to normalize here.

In addition, a comment which describes convertDate and why this
is necessary for allday events might be helpful.

r=tbe with this fixed

Comment 19

[Joey Minta]

Comment on attachment 233993 [details] [diff] [review]
patch v4

+        var convertDate = function(date) {
+          if (date.isDate) {
+            var newDate = date.clone();
+            newDate.hour = 0;
+            newDate.minute = 0;
+            newDate.second = 0;
+            newDate.isDate = false;
+            return newDate;
+          } else {
+            return date;
+          }
+        }
Why not just do |function convertDate(date)| rather than assigning this anonymous function to a variable?  At the very least, the variable declaration should end with a ;.

+            (!isZeroLength &&
+             start.compare(aEndDate) < 0 &&
+             end.compare(aStartDate) > 0)) {
+            
This describes an open interval, not a half open interval.  As such, we'll fail this additional test:
// Normal 1hr event, ending on the start of the occurrence range.
var ev5 = Cc["@mozilla.org/calendar/event;1"].createInstance(Ci.calIEvent);
ev5.title = "ev5";
ev5.startDate = ev1.startDate.clone();
ev5.endDate = ev1.startDate.clone();
ev5.startDate.hour -= 1;
ev5.startDate.normalize();
Unless the claim is that events are half-open intevals too?

Comment 20

[Joey Minta]

Comment on attachment 233993 [details] [diff] [review]
patch v4

OK, dmose and mvl convinced me that half-open events are what we want too.  r=jminta with the better function declaration and tbe's suggestion about a comment.

.normalize() shouldn't be necessary since setting to 0 can never overflow.

Comment 21

[Michael Büttner (no reviews TFN)]

patch checked in.