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

RESOLVED FIXED

Status

Calendar
Lightning Only
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: Stefan Sitter, Assigned: Michael Büttner (no reviews TFN))

Tracking

({dataloss})

Trunk
dataloss

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

12 years ago
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
(Reporter)

Comment 1

12 years ago
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.
(Reporter)

Comment 2

12 years ago
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.

Updated

12 years ago
OS: Windows 2000 → All
Hardware: PC → All
Whiteboard: [cal relnote]

Comment 3

11 years ago
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.

Updated

11 years ago
Keywords: dataloss
(Assignee)

Updated

11 years ago
Assignee: nobody → michael.buettner
(Assignee)

Updated

11 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 4

11 years ago
Created attachment 225710 [details] [diff] [review]
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.
Attachment #225710 - Flags: first-review?(jminta)

Comment 5

11 years ago
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?
(Assignee)

Comment 6

11 years ago
(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.
(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

11 years ago
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.
(Assignee)

Comment 9

11 years ago
Created attachment 226311 [details] [diff] [review]
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.
Attachment #225710 - Attachment is obsolete: true
Attachment #226311 - Flags: first-review?(jminta)
Attachment #225710 - Flags: first-review?(jminta)

Comment 10

11 years ago
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

11 years ago
Comment on attachment 226311 [details] [diff] [review]
patch v2

minused per previous comment.  If I'm misunderstanding something, please re-request review.
Attachment #226311 - Flags: first-review?(jminta) → first-review-

Comment 12

11 years ago
I think this belongs to category "Events/tasks should not appear in views where they are not intended to appear". Requesting blocking0.3
Flags: blocking0.3?

Updated

11 years ago
Flags: blocking0.3? → blocking0.3+
(Assignee)

Comment 13

11 years ago
Created attachment 233574 [details] [diff] [review]
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.
Attachment #226311 - Attachment is obsolete: true
Attachment #233574 - Flags: second-review?(jminta)
Attachment #233574 - Flags: first-review?(thomas.benisch)

Comment 14

11 years ago
Created attachment 233638 [details]
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

11 years ago
(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

11 years ago
Comment on attachment 233574 [details] [diff] [review]
patch v3

r- due to the allday event problem (see comment #15)
Attachment #233574 - Flags: first-review?(thomas.benisch) → first-review-
(Assignee)

Comment 17

11 years ago
Created attachment 233993 [details] [diff] [review]
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.
Attachment #233574 - Attachment is obsolete: true
Attachment #233993 - Flags: second-review?(jminta)
Attachment #233993 - Flags: first-review?(thomas.benisch)
Attachment #233574 - Flags: second-review?(jminta)

Comment 18

11 years ago
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
Attachment #233993 - Flags: first-review?(thomas.benisch) → first-review+

Comment 19

11 years ago
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

11 years ago
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.
Attachment #233993 - Flags: second-review?(jminta) → second-review+
(Assignee)

Comment 21

11 years ago
patch checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Whiteboard: [cal relnote]
You need to log in before you can comment on or make changes to this bug.