Makes life easier for bug 320178 and makes the review bits smaller.

Comment on attachment 322522 [details] [diff] [review] Fix - v1 I don't spot any IDL for that, so how it's accessed? Making the methods part of calIItemBase looks wrong to me, because those can only be called on occurrences: Both parent items (of recurring items) and non-recurring items will return null. Why don't you offer calIRecurrenceInfo::getPreviousOccurrence(recurrenceId) instead or some free helper functions?

(In reply to comment #1) > (From update of attachment 322522 [details] [diff] [review]) > I don't spot any IDL for that, so how it's accessed? http://mxr.mozilla.org/mozilla1.8/source/calendar/base/public/calIItemBase.idl#285 > Making the methods part of calIItemBase looks wrong to me, because those can > only be called on occurrences: Both parent items (of recurring items) and > non-recurring items will return null. > > Why don't you offer calIRecurrenceInfo::getPreviousOccurrence(recurrenceId) > instead or some free helper functions? I could live with that. Making it part of calItemBase was probably done as a shortcut.

This would be the alternative. I kind of liked the ease of the previousOccurrence/nextOccurrence attributes. This way when on an occurrence, you need to call occ.parentItem.recurrenceInfo.getPreviousOccurrence(occ.recurrenceId); Please reconsider, but r+ which ever you like :-)

Comment on attachment 323101 [details] [diff] [review] Alternative fix - v2 I like this approach better, completing calIRecurrenceInfo. The API looks good. >+ getPreviousOccurrence: function cRI_getPreviousOccurrence(aDate) { >+ var recurrenceStartDate = this.mBaseItem.recurrenceStartDate; >+ if (aDate && recurrenceStartDate && aDate.compare(recurrenceStartDate) != 0) { >+ // The above conditions are optimized, so that if this is the first >+ // occurrence, null is returned directly. >+ >+ // 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 occs = this.getOccurrenceDates(recurrenceStartDate, >+ aDate, >+ null, >+ {}); You must not use getOccurrenceDates here, because that will give you DTSTART dates not RECURRENCE-IDs that you need to feed getOccurrenceFor(). DTSTART may not match RECURRENCE-ID on exceptional/overridden items. Use calculateDates(..., true /* aReturnRIDs */); >+ return this.getOccurrenceFor(occs[occs.length - 1]); This gets the series from recurrenceStart. IMO you need to consider exceptional/overridden items which may occur before that.

This should do it: calculateDates takes exceptions into account. I tested using a debug breakpoint and it seems to work fine for exdates and exceptions.

Comment on attachment 323392 [details] [diff] [review] Fix - v3 seems I cannot make my point cl >+ if (aDate && recurrenceStartDate && aDate.compare(recurrenceStartDate) != 0) { recurrenceStartDate need not be the first occurrence of the series w.r.t overridden items. >+ var rids = this.calculateDates(recurrenceStartDate, aDate, 0, true); same applies here: If you pass recurrenceStartDate as lower boundary, you miss overridden items that occur before recurrenceStartDate (i.e. have a DTSTART before that).

Comment on attachment 323536 [details] [diff] [review] Fix - v4 No review as discussed, Philipp will come up with a revised API and more fixes of getNextOccurrence et al.

Will be fixed in bug 391495.