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

VERIFIED FIXED in 0.7

Status

Calendar
General
VERIFIED FIXED
11 years ago
11 years ago

People

(Reporter: Pete Riley, Assigned: Sebastian Schwieger)

Tracking

unspecified
Bug Flags:
blocking-calendar0.7 +

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

11 years ago
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.
Confirmed using Tb 2.0.0.6pre + Lightning 0.7pre (2007072704) on WinXP.
Status: UNCONFIRMED → NEW
Component: Calendar Views → General
Ever confirmed: true
Flags: blocking-calendar0.7?
QA Contact: views → general
(Assignee)

Comment 2

11 years ago
Created attachment 276286 [details] [diff] [review]
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.
Assignee: nobody → sebo.moz
Status: NEW → ASSIGNED
Attachment #276286 - Flags: review?
Sebo, you forgot to give a reviewer for your request. :)
(Assignee)

Comment 4

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

Martin, thanks for your hint. I could swear I had Daniel in there...
Attachment #276286 - Flags: review? → review?(daniel.boelzle)

Updated

11 years ago
Flags: blocking-calendar0.7? → blocking-calendar0.7+

Comment 5

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

Comment 6

11 years ago
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).
Attachment #276286 - Flags: review?(daniel.boelzle) → review-
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
(Assignee)

Comment 8

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

Comment 11

11 years ago
Created attachment 278210 [details] [diff] [review]
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.
Attachment #276286 - Attachment is obsolete: true
Attachment #278210 - Flags: review?(daniel.boelzle)
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?
(Assignee)

Comment 13

11 years ago
Created attachment 278640 [details] [diff] [review]
comments addressed

This is the version that addresses the previous comments. Alternative implementation coming up next.
Attachment #278210 - Attachment is obsolete: true
Attachment #278210 - Flags: review?(daniel.boelzle)
(Assignee)

Comment 14

11 years ago
Created attachment 278641 [details] [diff] [review]
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.
Attachment #278641 - Flags: review?(daniel.boelzle)
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.
Attachment #278641 - Flags: review?(daniel.boelzle) → review+
Checked in on HEAD and MOZILLA_1_8_BRANCH.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → 0.7

Comment 17

11 years ago
verified with lightning 2007083103
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.