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)
Calendar
Internal Components
Tracking
(Not tracked)
RESOLVED
FIXED
4.1
People
(Reporter: bv1578, Assigned: bv1578)
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
bv1578
:
review+
|
Details | Diff | Splinter Review |
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.
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 2•10 years ago
|
||
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+
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
Updated•10 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 4.1
Comment 6•9 years ago
|
||
(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)
Comment 7•9 years ago
|
||
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)
Comment 8•9 years ago
|
||
PR'd this anyway, with tests: https://github.com/mozilla-comm/ical.js/pull/211
You need to log in
before you can comment on or make changes to this bug.
Description
•