Last Comment Bug 1146500 - Wrong first occurrence for monthly recurrence with BYDAY and BYMONTHDAY
: Wrong first occurrence for monthly recurrence with BYDAY and BYMONTHDAY
Status: RESOLVED FIXED
:
Product: Calendar
Classification: Client Software
Component: Internal Components (show other bugs)
: unspecified
: All All
-- normal (vote)
: 4.1
Assigned To: Decathlon
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2015-03-23 11:37 PDT by Decathlon
Modified: 2016-03-06 01:18 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch - v1 (8.00 KB, patch)
2015-03-25 03:17 PDT, Decathlon
philipp: review+
Details | Diff | Splinter Review
patch - v2 (8.00 KB, patch)
2015-03-25 04:39 PDT, Decathlon
bv1578: review+
Details | Diff | Splinter Review

Description User image Decathlon 2015-03-23 11:37:22 PDT
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.
Comment 1 User image Decathlon 2015-03-25 03:17:49 PDT
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?
Comment 2 User image Philipp Kewisch [:Fallen] 2015-03-25 04:08:22 PDT
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.
Comment 3 User image Decathlon 2015-03-25 04:39:30 PDT
Created attachment 8583032 [details] [diff] [review]
patch - v2

deleted the white space.
Comment 4 User image Decathlon 2015-03-25 04:41:28 PDT
I will post a Pull request on GitHub for ical.js.
Comment 6 User image Geoff Lankow (:darktrojan) 2015-07-18 17:51:28 PDT
(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.
Comment 7 User image Philipp Kewisch [:Fallen] 2015-07-19 10:18:46 PDT
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
Comment 8 User image Philipp Kewisch [:Fallen] 2016-03-06 01:18:35 PST
PR'd this anyway, with tests: https://github.com/mozilla-comm/ical.js/pull/211

Note You need to log in before you can comment on or make changes to this bug.