Closed Bug 391495 Opened 17 years ago Closed 16 years ago

calRecurrenceInfo::getNextOccurrence/getNextOccurrenceDate have bugs

Categories

(Calendar :: Internal Components, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: dbo, Assigned: Fallen)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

- getNextOccurrenceDate needs to keep care of exception items
- getNextOccurrence needs to properly handle VTODO items (may have no DTSTART, fallback to DUE etc)
Attached patch Fix calRecurrenceInfo - v1 (obsolete) β€” β€” Splinter Review
This patch gives calRecurrenceInfo quite a thorough overhaul. It does not fix the task bug mentioned though. Also adds support for getting the previous occurrence.
Assignee: nobody → philipp
Status: NEW → ASSIGNED
Attachment #324265 - Flags: review?(daniel.boelzle)
Comment on attachment 324265 [details] [diff] [review]
Fix calRecurrenceInfo - v1

some spots, no full review yet:

>+  calIDateTime getNextOccurrence (in calIDateTime aRecurrenceId,
>+				                  in calIDateTime aTime);
indent correctly, untabify file

>   // return array of calIDateTime of the start of all occurrences of
>   // this event starting at aStartTime, between rangeStart and an
>   // optional rangeEnd
>   void getOccurrences (in calIDateTime aStartTime,
> 		       in calIDateTime aRangeStart,
> 		       in calIDateTime aRangeEnd,
> 		       in unsigned long aMaxCount,
dto

>+    QueryInterface: function cRI_QueryInterface(aIID) {
>+        return doQueryInterface(this, calAlarm.__proto__, aIID, null, this);
???                                     ^^^^^^^^

>+    getInterfaces: function cRI_getInterfaces(aCount) {
>+        const interfaces = [Components.interfaces.nsISupports,
>+                            Components.interfaces.calIRecurrenceInfo,
>+                            Components.interfaces.nsIClassInfo];
>+
>+        aCount = interfaces.length;
aCount.value = ...

>+function ERROR(aMessage) {
>+    dump("Error: " + aMessage + '\n');
>+    var scriptError = Components.classes["@mozilla.org/scripterror;1"]
>+                                .createInstance(Components.interfaces.nsIScriptError);
>+    scriptError.init(aMessage, null, null, 0, 0,
>+                     Components.interfaces.nsIScriptError.errorFlag,
>+                     "component javascript");
>+    getConsoleService().logMessage(scriptError);
>+}
why not just Components.utils.reportError(aMessage)?

>+function binarySearch(itemArray, low, high, newItem, comptor) {
* Please add documentation, especially that the high index is inclusive.
IMO it's better to skip that parameter, not offering it, but just using it in an internal function, because most callers probably just pass array.length - 1.
* Moreover what if an empty array is passed?
* IMO there's a number of misleading names:
- itemArray: better "sortedArray"?
- newItem: better just "item"?
- binarySearch: better "binarySearchSortedArray"?

>+function binaryInsert(itemArray, item, comptor, discardDuplicates) {
Documentation would be good, especially the return codes.
Comment on attachment 324265 [details] [diff] [review]
Fix calRecurrenceInfo - v1

>+    getPreviousOccurrence: function cRI_getPreviousOccurrence(aTime) {
>+        // TODO libical currently does not provide us with easy means of
>+        // getting the previous occurrence. This could be fixed to improve
>+        // performance greatly. Filed as libical feature request 1944020.
>+
>+        var rids = this.calculateDates(this.mBaseItem.recurrenceStartDate,
>+                                       aTime,
>+                                       0,
>+                                       true);
>+        // The returned dates are sorted, so the last one is a good
>+        // candidate, if it exists.
>+        return (rids.length ? this.getOccurrenceFor(rids[rids.length - 1].id) : null);

I think this doesn't work, you need to pass null as lower boundary to cover overridden items that fall before recurrenceStartDate and cover aTime before recurrenceStartDate. Hope that calculateDates can deal with that :/
> why not just Components.utils.reportError(aMessage)?
I wanted something that also dumps to the system console, and it nicely complements WARN and LOG

> I think this doesn't work, you need to pass null as lower boundary to cover
> overridden items that fall before recurrenceStartDate and cover aTime before
> recurrenceStartDate. Hope that calculateDates can deal with that :/
Didnt work, calculdateDates can't, but fixed in next version.
More unit tests, works with RDATES before the base item's start date, works better with exceptions.
Attachment #324265 - Attachment is obsolete: true
Attachment #325023 - Flags: review?(daniel.boelzle)
Attachment #324265 - Flags: review?(daniel.boelzle)
Comment on attachment 325023 [details] [diff] [review]
[checked in ] Fix calRecurrenceInfo - v3

first of all: great patch!

in addition to the previously mentioned:

>+  calIRecurrenceItem getRecurrenceItemAt(in unsigned long aIndex);
>+  void deleteRecurrenceItemAt(in unsigned long aIndex);
>+  void deleteRecurrenceItem(in calIRecurrenceItem aItem);
>   // inserts the item at the given index, pushing the item that was previously there forward
>-  void insertRecurrenceItemAt (in calIRecurrenceItem aItem, in unsigned long aIndex);
>+  void insertRecurrenceItemAt(in calIRecurrenceItem aItem, in unsigned long aIndex);
I really think we should get rid of the index-based API. AFAIK it's only used inside calRecurrenceInfo, and I think it would make a lot of code easier, we need only store the positive and negative items etc.

>+    ensureExceptionMap: function cRI_ensureExceptionMap() {
>+        if (!this.mExceptionMap) {
>+            this.mExceptionMap = {};
>+            for each (var ex in this.mExceptions) {
>+                this.mExceptionMap[ex.id] = ex;
make canonical, e.g. ex.id.getInTimezone(UTC()) or ensure that mExceptions' ex.id is filled up in UTC

>+                // Map all negative dates.
>+                for each (var r in rdates) {
>+                    negMap[r] = true;
make canonical

>+                    var searchDate =
>+                        (nextOccurrences[i] && nextOccurrences[i].compare(aTime) > 0 ?
>+                            nextOccurrences[i] :
>+                            aTime);
I'd prefer
cond
? a
: b

>+            if (bailCounter++ > 100) {
>+                ERROR("Could not find next occurrence after 100 runs!");
>+                return;
return null;

>+        var early = createDateTime();
>+        early.icalString = "00000101T000000Z";
Hmm, that scares me. I am not sure if we properly support dates before 1970, so I'd use 1st of jan 1970.

>+            occurrenceMap[ex.id] = true;
ensure those are canonical; there may be more places.

>+            occurrenceMap[baseOccDate] = true;
dto

>+                    // TODO PERF Theoretically we could use occurrence map
>+                    // to construct the array of occurrences. Right now I'm
>+                    // just using the occurrence map to skip the filter
>+                    // action if the occurrence isn't there anyway.
Good idea, let's do that. You could store them like
occurrenceMap[canonicalRID] = datetimeOfOccurrence
to get both, either to return rids or actual occurrence dates.

>+    getOccurrences: function cRI_getOccurrences(aRangeStart,
>+                                                aRangeEnd,
>+                                                aMaxCount,
>+                                                aCount) {
>         var results = [];
>+        var dates = this.calculateDates(aRangeStart, aRangeEnd, aMaxCount, true);
>+        if (dates.length) {
>+            var count = aMaxCount;
>+            if (!count)
>+                count = dates.length;
> 
>-        for (var i = 0; i < count; i++) {
>-            var proxy = this.getOccurrenceFor(dates[i]);
>-            results.push(proxy);
>+            for (var i = 0; i < count; i++) {
>+                results.push(this.getOccurrenceFor(dates[i].id));
>+            }
>         }
I assume this is the commonly used function, thus it may make sense to let calculateDates return an object like the populated occurrenceDates (getting rid of aReturnRIDs parameter) and avoid double-looping.

>+        this.mExceptionMap[itemtoadd.recurrenceId] = excItem;
make canonical

>+        var excItem = { id: rid, item: newex };
>+        this.mExceptions.push(excItem);
>+        this.mExceptionMap[rid] = excItem;
...

>+        if (this.mExceptionMap[aRecurrenceId]) {
>+            return this.mExceptionMap[aRecurrenceId].item;
>+        } else if (aCreate) {
...

>-    removeExceptionFor: function (aRecurrenceId) {
>-        if (!this.mBaseItem)
>-            throw Components.results.NS_ERROR_NOT_INITIALIZED;
>+    removeExceptionFor: function cRI_removeExceptionFor(aRecurrenceId) {
>+        this.ensureBaseItem();
>+        this.ensureExceptionMap();
> 
>         this.mExceptions = this.mExceptions.filter (function(ex) {
>                                                         return (ex.id.compare(aRecurrenceId) != 0);
>                                                     });
>+        delete this.mExceptionMap[aRecurrenceId];
>     },
> 


I cannot foresee whether these changes raise regressions, because they are pretty invasive.

Let's find it out; r=dbo.
Attachment #325023 - Flags: review?(daniel.boelzle) → review+
(In reply to comment #7)
> > in addition to the previously mentioned:
I've fixed the previously mentioned things?

> I really think we should get rid of the index-based API. AFAIK it's only used
> inside calRecurrenceInfo, and I think it would make a lot of code easier, we
> need only store the positive and negative items etc.
I think we can do that in a different bug. A quick search tells me that insertRecurrenceItemAt is also used in the event dialog, not sure if its strictly needed there.

> 
> >+    ensureExceptionMap: function cRI_ensureExceptionMap() {
> >+        if (!this.mExceptionMap) {
> >+            this.mExceptionMap = {};
> >+            for each (var ex in this.mExceptions) {
> >+                this.mExceptionMap[ex.id] = ex;
> make canonical, e.g. ex.id.getInTimezone(UTC()) or ensure that mExceptions'
> ex.id is filled up in UTC
Do we really need to do this? Will there ever be an item with exceptions that have the same rid but in a different timezone? Since I've heard getInTimezone isn't that cheap, I'm omiting the canonicalization changes you've suggested for now, but will create a further patch after we've talked about it. I want to get this patch in to see what else it breaks.

> >+        var early = createDateTime();
> >+        early.icalString = "00000101T000000Z";
> Hmm, that scares me. I am not sure if we properly support dates before 1970, so
> I'd use 1st of jan 1970.
Why limit this to 1970? I believe we don't have timezone information for dates before 1970, but we support dates back to 1601 or so. No harm done choosing a such early date?

> >+                    // TODO PERF Theoretically we could use occurrence map
> >+                    // to construct the array of occurrences. Right now I'm
> >+                    // just using the occurrence map to skip the filter
> >+                    // action if the occurrence isn't there anyway.
> Good idea, let's do that. You could store them like
> occurrenceMap[canonicalRID] = datetimeOfOccurrence
> to get both, either to return rids or actual occurrence dates.
Thought about that too, but an extra for() loop is needed, so I'd like to move this to a different bug to see if its really faster.

> I assume this is the commonly used function, thus it may make sense to let
> calculateDates return an object like the populated occurrenceDates (getting rid
> of aReturnRIDs parameter) and avoid double-looping.
Only works if I use the above occurrence map, I guess I see what you mean but I'd still like to do this some other time in a different bug. This patch has caused much pain already.


> I cannot foresee whether these changes raise regressions, because they are
> pretty invasive.
> 
> Let's find it out; r=dbo.
Comment on attachment 325023 [details] [diff] [review]
[checked in ] Fix calRecurrenceInfo - v3

patch checked in on both branches with commented changes.

Leaving bug open to discuss further issues
Attachment #325023 - Attachment description: Fix calRecurrenceInfo - v3 → [checked in ] Fix calRecurrenceInfo - v3
(In reply to comment #8)
> > >+    ensureExceptionMap: function cRI_ensureExceptionMap() {
> > >+        if (!this.mExceptionMap) {
> > >+            this.mExceptionMap = {};
> > >+            for each (var ex in this.mExceptions) {
> > >+                this.mExceptionMap[ex.id] = ex;
> > make canonical, e.g. ex.id.getInTimezone(UTC()) or ensure that mExceptions'
> > ex.id is filled up in UTC
> Do we really need to do this? Will there ever be an item with exceptions that
> have the same rid but in a different timezone? Since I've heard getInTimezone
> isn't that cheap, I'm omiting the canonicalization changes you've suggested for
> now, but will create a further patch after we've talked about it. I want to get
> this patch in to see what else it breaks.
I think it's mandatory, even our own ics files write RECURRENCE-IDs with TZID. Moreover, the stringified datetime object is quite verbose, I'd rather just use the icalString, e.g.

var tz = dt.timezone;
canId = (tz.isFloating || tz.isUTC
         ? tz.icalString : dt.getInTimezone(UTC()).icalString);


> > >+        var early = createDateTime();
> > >+        early.icalString = "00000101T000000Z";
> > Hmm, that scares me. I am not sure if we properly support dates before 1970, so
> > I'd use 1st of jan 1970.
> Why limit this to 1970? I believe we don't have timezone information for dates
> before 1970, but we support dates back to 1601 or so. No harm done choosing a
> such early date?
Give it a try.

> > >+                    // TODO PERF Theoretically we could use occurrence map
> > >+                    // to construct the array of occurrences. Right now I'm
> > >+                    // just using the occurrence map to skip the filter
> > >+                    // action if the occurrence isn't there anyway.
> > Good idea, let's do that. You could store them like
> > occurrenceMap[canonicalRID] = datetimeOfOccurrence
> > to get both, either to return rids or actual occurrence dates.
> Thought about that too, but an extra for() loop is needed, so I'd like to move
> this to a different bug to see if its really faster.
Really? I think one less is needed, instead of iterating the object to construct the array, then iterating the array to construct the items, the former would be obsolete.

> > I assume this is the commonly used function, thus it may make sense to let
> > calculateDates return an object like the populated occurrenceDates (getting rid
> > of aReturnRIDs parameter) and avoid double-looping.
> Only works if I use the above occurrence map, I guess I see what you mean but
> I'd still like to do this some other time in a different bug. This patch has
> caused much pain already.
of course
seems to have regressed bug 436476: EXDATEs are not correctly figured out, I suspect that calRecurrenceInfo::modifyException doesn't work correctly anymore.
Flags: blocking-calendar0.9+
Target Milestone: --- → 0.9
Attached patch fixing regression β€” β€” Splinter Review
fixing exception lookup by making rids canonical
- removing mExceptions
- fixing datetime alias unit test
Attachment #326288 - Flags: review?(philipp)
Comment on attachment 326288 [details] [diff] [review]
fixing regression

>+        var clonedExceptions = {};
>+        for (exitem in this.mExceptionMap) {
>+            clonedExceptions[exitem] = this.mExceptionMap[exitem].cloneShallow(this.mBaseItem);
Seems like I previously forgot "var" before exitem.

Thanks for taking care.
Attachment #326288 - Flags: review?(philipp) → review+
Checked in on HEAD and MOZILLA_1_8_BRANCH => FIXED again.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Blocks: 436476
checked in lightning build 2008062503 -> VERIFIED
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.