Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Wrong first occurrence for monthly recurrence with BYDAY and BYMONTHDAY

RESOLVED FIXED in 4.1

Status

Calendar
Internal Components
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: Decathlon, Assigned: Decathlon)

Tracking

unspecified

Details

Attachments

(1 attachment, 1 obsolete attachment)

8.00 KB, patch
Decathlon
: review+
Details | Diff | Splinter Review
(Assignee)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
Created attachment 8583006 [details] [diff] [review]
patch - v1

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+
(Assignee)

Comment 3

2 years ago
Created attachment 8583032 [details] [diff] [review]
patch - v2

deleted the white space.
Attachment #8583006 - Attachment is obsolete: true
Attachment #8583032 - Flags: review+
(Assignee)

Comment 4

2 years ago
I will post a Pull request on GitHub for ical.js.
Keywords: checkin-needed

Comment 5

2 years ago
https://hg.mozilla.org/comm-central/rev/64fc7fc3c435

Updated

2 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 2 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)
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.