Closed Bug 1146500 Opened 10 years ago Closed 10 years ago

Wrong first occurrence for monthly recurrence with BYDAY and BYMONTHDAY

Categories

(Calendar :: Internal Components, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: bv1578, Assigned: bv1578)

Details

Attachments

(1 file, 1 obsolete file)

It happens with both libical and ical.js but in different ways. Try these calendars to test. They display two Bydays starting from April when the Bymonthdays are odd days: BEGIN:VCALENDAR PRODID:-//Mozilla.org/NONSGML Mozilla Calendar V1.1//EN VERSION:2.0 BEGIN:VEVENT UID:fbacc982-a181-4dea-1c76-25219d89683b SUMMARY:every MO and FR on odd days RRULE:FREQ=MONTHLY;BYDAY=MO,FR;BYMONTHDAY=1,3,5,7,9,11,13,15,17,19,21,23,25,27,29,31 DTSTART:20150401T080000Z DTEND:20150401T090000Z DESCRIPTION:Wrong first recurrence with ical.js: Friday 3rd of April is missing END:VEVENT END:VCALENDAR BEGIN:VCALENDAR PRODID:-//Mozilla.org/NONSGML Mozilla Calendar V1.1//EN VERSION:2.0 BEGIN:VEVENT UID:fbacc982-a182-4d1f-1b76-26219d89683b SUMMARY:every MO and SA on odd days RRULE:FREQ=MONTHLY;BYDAY=MO,SA;BYMONTHDAY=1,3,5,7,9,11,13,15,17,19,21,23,25,27,29,31 DTSTART:20150401T080000Z DTEND:20150401T090000Z DESCRIPTION:Wrong first recurrence with libical: Saturday 4th of April should not exist END:VEVENT END:VCALENDAR The former fails with icaljs because it displays the first occurrence on Monday 13/4/2015 and doesn't display Friday 3/4/2015. The latter fails with libical because it displays the first recurrence on Saturday 4/4/2015 when bymonthday doesn't have a 4 monthday.
Attached patch patch - v1 (obsolete) (deleted) — Splinter Review
ical.js treats the case BYDAY+BYMONTHDAY with the function _byday_and_bymonthday(). During the initialization, when it sets the start index in the BYMONTHDAY array for the research of the matching day, it discards exactly the index that gives the BYMONTHDAY equal to lastDay. libical instead, unless I missed something, doesn't handle the case BYDAY+BYMONTHDAY when initializes the iterator so it sets the first occurrence only for the BYDAY part. Since the function next_month() finds the right date for the occurrences after the first, I've thought to use it also during the initialization instead of rewrite a code similar to the function _byday_and_bymonthday() used in ical.js. I've also added a rudimentary check with a counter to avoid an infinite loop that normally causes Thunderbird to hang when there is a rule BYDAY+BYMONTHDAY not correct, for example: BYDAY=4MO;BYMONTHDAY=1,2,3. It is useful also because such rules cause a data loss since to get again TB control, it isn't sufficient to kill its process, all the calendars files inside TB's profile must be deleted too. However using a counter like that is too simple and not a complete solution, a lot of messages "Out of memory" appear anyway in the console so maybe the problem might be matter for another bug with a more detailed and complete solution. What's your opinion Philipp?
Attachment #8583006 - Flags: review?(philipp)
Comment on attachment 8583006 [details] [diff] [review] patch - v1 Review of attachment 8583006 [details] [diff] [review]: ----------------------------------------------------------------- Code looks fine, r=philipp. I'm surprised there were differences here, but I'm happy you found the solution. Great work! r=philipp ::: calendar/base/modules/ical.js @@ +4950,5 @@ > // we have to be sure to start searching after the last > + // found date or at the last BYMONTHDAY, unless we are > + // initializing the iterator because in this case we have > + // to consider the last found date too. > + while (byMonthDay[dateIdx] <= lastDay && A whitespace snuck in here.
Attachment #8583006 - Flags: review?(philipp) → review+
Attached patch patch - v2 (deleted) — Splinter Review
deleted the white space.
Attachment #8583006 - Attachment is obsolete: true
Attachment #8583032 - Flags: review+
I will post a Pull request on GitHub for ical.js.
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 4.1
(In reply to Decathlon from comment #4) > I will post a Pull request on GitHub for ical.js. It looks like you forgot to do this.
Flags: needinfo?(bv1578)
I don't think it will be necessary, since the recurrence iterator is being replaced soon. My project partner promised he'd finish by tomorrow, I can get to merging to master afterwards. If you want to try the new one with this bug, you can checkout the recparserv2 branch on my fork. https://github.com/kewisch/ical.js/commits/recparserv2
Flags: needinfo?(bv1578)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: