389848 - Today Pane: Yesterday's all-day recurring events are in "Today"

Status: VERIFIED FIXED

(Calendar :: General, defect)

Description

[Pete Riley]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.5) Gecko/20070713 Firefox/2.0.0.5
Build Identifier: Lightning 0.7pre 2007-07-27 04

All-day, recurring events that occurred yesterday are in the "Today" section of the today pane.

Reproducible: Always

Steps to Reproduce:
1) In month view, create an event for yesterday.  It should be an all-day event that recurs weekly.

2) In the today pane, see that it's incorrectly listed in the "Today" section.

Comment 1

[Martin Schröder [:mschroeder]]

Confirmed using Tb 2.0.0.6pre + Lightning 0.7pre (2007072704) on WinXP.

Comment 2

[Sebastian Schwieger]

Attachment: make comparison exclusive

The comparison needs to be exclusive. This way, events that end in the same second the range starts are not included. See the discussion in bug 330496 comment 19 and following as a reference. This has been tested on Linux with weekly recurring allday events.
Additionally I think we need to apply the same UTC workaround as is done for getOccurences. I admit that I didn't test this.

Comment 3

[Martin Schröder [:mschroeder]]

Sebo, you forgot to give a reviewer for your request. :)

Comment 4

[Sebastian Schwieger]

Comment on attachment 276286 [details] [diff] [review]
make comparison exclusive

Martin, thanks for your hint. I could swear I had Daniel in there...

Comment 5

[Daniel Boelzle [:dbo]]

(In reply to comment #2)
Right, I agree items should be treated as an half-open interval.

Comment 6

[Daniel Boelzle [:dbo]]

Comment on attachment 276286 [details] [diff] [review]
make comparison exclusive

>Index: calendar/base/src/calRecurrenceInfo.js
>         // workaround for UTC- timezones
>         var rangeEnd = aRangeEnd;
>         if (rangeEnd && rangeEnd.isDate) {
>             rangeEnd = aRangeEnd.clone();
>             rangeEnd.isDate = false;
>         }
>+        
>+        var rangeStart = aRangeStart;
>+        if (rangeStart && rangeStart.isDate) {
>+            rangeStart = aRangeStart.clone();
>+            rangeStart.isDate = false;
>+        }
Looking at that it comes to my mind that we either should
- make sure we compare(datetime, datetime) or compare(date, date) throughout in calculateDates
- or fix calIDatetime::compare to reasonably compare(date, datetime) and vice versa.

>         function checkRange(item) {
>             var dueDate = null;
>             var occDate = (item.getProperty("DTSTART") ||
>                            (dueDate = item.getProperty("DUE")));
>             if (!occDate) // DTSTART or DUE mandatory
>                 return null;
>             // tasks may have a due date set or no duration at all
>             var end = (item.getProperty("DTEND") ||
>                        (dueDate ? dueDate : item.getProperty("DUE")) ||
>                        occDate);
>             // is the item an intersection of the range?
>-            if ((!aRangeStart || aRangeStart.compare(end) <= 0) &&
>-                (!aRangeEnd || aRangeEnd.compare(occDate) > 0)) {
>+            if ((!rangeStart || rangeStart.compare(end) < 0) &&
>+                (!rangeEnd || rangeEnd.compare(occDate) > 0)) {
>                 return occDate;
>             }
>             return null;
>         }
That way we lose VTODOs with occDate==rangeStart, but without DUE (i.e. end==occDate).

Comment 7

[Michael Büttner (no reviews TFN)]

Without having looked at the attached patch in detail I remember that I had a similar problem with the agenda a while back. The problem also had to do with the issue that the half-open query interval was not correctly handled. I just would like to point to the code location that I touched in order to get it right, see [1]. Furthermore, the zero-length events should also be treated correctly, which added a bit of complexity to the piece of code I'm talking about. Hope this helps to get this one fixed. Probably it's a wise idea to factor out this interval comparison code into some common utility function...

[1] http://lxr.mozilla.org/mozilla1.8/source/calendar/base/src/calEvent.js#261

Comment 8

[Sebastian Schwieger]

Thanks for the pointer, Michael. Indeed, zero length events need to be special cased. Before moving the range check into calUtils, I have one question:

>            if ((!rangeStart || rangeStart.compare(end) < 0) &&
>                (!rangeEnd || rangeEnd.compare(occDate) > 0)) {

In calRecurrenceInfo.js there is this check if rangeStart and rangeEnd exist while I don't find that in calEvent.js and calTodo.js. Is this a superfluous check, a specialty of RecurrenceInfo or should it also be done in the other places?

Comment 9

[Michael Büttner (no reviews TFN)]

(In reply to comment #8)
> In calRecurrenceInfo.js there is this check if rangeStart and rangeEnd exist
> while I don't find that in calEvent.js and calTodo.js. Is this a superfluous
> check, a specialty of RecurrenceInfo or should it also be done in the other
> places?
I just checked calculateDates() in calRecurrenceInfo.js. This function gets called from getOccurrenceDates() as well as getOccurrences(). For both functions aRangeEnd is optional (we could as for a set of occurrences based on aCount, for example). On the contrary, aRangeStart is indeed mandatory.

Comment 10

[Daniel Boelzle [:dbo]]

(In reply to comment #8)
Sebo, I don't think the checks are necessary in calEvent.js nor calTodo.js, because IMO calIItemBase::getOccurrencesBetween must always get a specified range. Unlike calIRecurrenceInfo::getOccurrences et al, calIItemBase::getOccurrencesBetween has no maxCount parameter, thus "open expansion" would end up in horror since 0 is passed as maxCount, i.e.

        if (this.recurrenceInfo) {
            return this.recurrenceInfo.getOccurrences(aStartDate, aEndDate, 0, aCount);
        }

BTW: I've spotted one minor flaw in calTodo.js:251: returned count should be set to 0, i.e.

        if (!this.entryDate && !this.dueDate) {
            aCount.value = 0;
            return null;
        }

Comment 11

[Sebastian Schwieger]

Attachment: move rangeCheck into calUtils

This moves the range check into calUtils and fixes the bug because recurring Items are now treated as half open events.

Comment 12

[Daniel Boelzle [:dbo]]

Comment on attachment 278210 [details] [diff] [review]
move rangeCheck into calUtils

>Index: calendar/base/src/calEvent.js
>+        if (check) {        
>           aCount.value = 1;
>           return ([ this ]);
>         }
> 
>         aCount.value = 0;
>         return null;
although not part of this bug, but shouldn't we return an empty array [] here?

>Index: calendar/base/src/calTodo.js
>-        if (!this.entryDate && !this.dueDate)
>+        if (!this.entryDate && !this.dueDate) {
>+            aCount.value = 0;
>             return null;
>+        }
same here

>         aCount.value = 0;
>         return null;
>     },
and here?

>Index: calendar/base/src/calUtils.js
> /**
>+* Returns true if the item is in the specified Range. Returns null if not in range
>+*/
The function should return either true or false, not true or null.

>+function checkIfInRange(aRangeStart, aRangeEnd, aStartDate, aEndDate)
>+{
>+    var start = ensureDateTime(aStartDate);
>+    var end = ensureDateTime(aEndDate);
>+
>+    var queryStart = ensureDateTime(aRangeStart);
>+    var queryEnd = ensureDateTime(aRangeEnd);

>+    var isZeroLength = !start.compare(end);
IMO an explicit check for (start.compare(end) == 0) right in the if condition below is more readable.

>+    if (isZeroLength) {
>+        if (start.compare(queryStart) >= 0 &&
>+           (!queryEnd || start.compare(queryEnd) < 0)) {
please indent one more space, please

>+            return true;
>+        }
>+    } else {
>+        if (!queryEnd || start.compare(queryEnd) < 0 &&
>+           (end.compare(queryStart) > 0)) {
here, too

>+            return true;
>+        }
>+    }
Although we've discussed that current callers will specify a range start, I'd propose to check for !aRangeStart, too. IMO that makes this helper function more versatile.

>+    return null;
return false;

despite the comments we need to resolve, the patch looks fine, thanks for your work!

As an alternative approach, we could also reuse calRecurrenceInfo.js' checkRange() code like

checkIfInRange(item, rangeStart, rangeEnd);

What do you think?

Comment 13

[Sebastian Schwieger]

Attachment: comments addressed

This is the version that addresses the previous comments. Alternative implementation coming up next.

Comment 14

[Sebastian Schwieger]

Attachment: reusing checkRange()

The only reason that kept me from doing this was that we still need occDate in calRecurrenceInfo. And maybe now the helper function isn't as versatile as before since it is bound to an item. But there are probably no other use cases for it. So please choose whichever you prefer.

Comment 15

[Daniel Boelzle [:dbo]]

Comment on attachment 278641 [details] [diff] [review]
reusing checkRange()

r=dbo; I prefer that version and took the liberty to return the item's start/due date or null. This made calRecurrenceInfo.js' checkRange obsolete.

Comment 16

[Daniel Boelzle [:dbo]]

Checked in on HEAD and MOZILLA_1_8_BRANCH.

Comment 17

[Omar Bajraszewski]

verified with lightning 2007083103