322458 - BYSETPOS not considered when getting occurrences

Status: RESOLVED WORKSFORME

(Calendar :: Internal Components, defect)

Description

[Joey Minta]

It seems (from limited testing) that the BYSETPOS attribute on RRULEs is not taken into account when we call getOccurrencesBetween.  The calendar at the above URL is supposed to contain a single event repeating on the last weekday (MO,TU,WE,TH,FR) of each month.  Instead, we show it as repeating on the last 5 weekdays of the month.

Comment 1

[Michiel van Leeuwen (email: mvl+moz@)]

*** Bug 332293 has been marked as a duplicate of this bug. ***

Comment 2

[Michael Büttner (no reviews TFN)]

taking this one.

Comment 3

[Michael Büttner (no reviews TFN)]

assigned.

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

This seems like fairly basic dataloss; setting as blocking 0.3.

Comment 5

[Michiel van Leeuwen (email: mvl+moz@)]

Is anything out there really uysing bysetpos?
libical doesn't support it, so it would be quite hard to fix this.

Comment 6

[Michael Büttner (no reviews TFN)]

(In reply to comment #5)
> libical doesn't support it, so it would be quite hard to fix this.
as far as i can tell libical supports this rule (http://lxr.mozilla.org/seamonkey/source/calendar/libical/src/libical/icalrecur.h#161).
and it is true that for the calendar at the given URL shows the recurring event on the last 5 weekdays of a month. i didn't had much time to further dive into what went wrong inside libical, but i'll do this within the next days.

what really concerns me is the fact that even if we display such rules correctly in the views we run into dataloss issues in case we edit such items with the existing item dialog as we're not correctly roundtrip foreign complex recurrence rules. but this is not related to this specific problem here, but i wanted to mention it anyway.

Comment 7

[Michael Büttner (no reviews TFN)]

(In reply to comment #5)
> libical doesn't support it, so it would be quite hard to fix this.
as far as i can tell libical supports this rule (http://lxr.mozilla.org/seamonkey/source/calendar/libical/src/libical/icalrecur.h#161).
and it is true that for the calendar at the given URL shows the recurring event on the last 5 weekdays of a month. i didn't had much time to further dive into what went wrong inside libical, but i'll do this within the next days.

what really concerns me is the fact that even if we display such rules correctly in the views we run into dataloss issues in case we edit such items with the existing item dialog as we're not correctly roundtrip foreign complex recurrence rules. but this is not related to this specific problem here, but i wanted to mention it anyway.

Comment 8

[Michael Büttner (no reviews TFN)]

urgs, you're right, BYSETPOS is indeed not supported by libical. the rule is parsed correctly into the internal 'by_set_pos'-array but is silently ignored during icalrecur_iterator_next(). to complicate matters it's not trivial to track the negative parts of the 'by_set_pos'-array, how do we know the ordinal (within the scope of a rule) each element the iterator calculates has? hm, i'll try to come up with a solution, but any additional help is appreciated ;-)

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

Having dataloss around a construct which might be regularly used to track paydays seems scary.  

We need to get some more brainstorming around how this can be fixed (thus the [needs input]) annotation.

According to an old IETF proceeding found by Google, Outlook uses (used?) this at one point <http://www3.ietf.org/proceedings/00dec/00dec-11.htm>.  It would be a helpful QA exercise to see if Outlook (or iCal) uses this construct today, and to research using search engines to try and get a better handle on how frequently this is used.

Leaving as blocking until we have more data, since given the current knowledge, this seems pretty scary to ship without.

Comment 10

[Sebastian Schwieger]

Outlook 2003 (Version 11.0) does still use BYSETPOS. You can find some calendars online by searching google. I also found this post, talking about Outlook using BYSETPOS more often than it should:
http://phpicalendar.net/forums/archive/index.php/t-1591.html

Comment 11

[Andrew McMillan]

Evolution also uses BYSETPOS for FREQ=MONTHLY.

Here's an example of Evolution specifying the third thursday of every second month:
FREQ=MONTHLY;INTERVAL=2;BYDAY=TH;BYSETPOS=3

Comment 12

[Matthew (lilmatt) Willis]

While I agree that we really need to support this, unless someone can pull a gleaming mystical patch out of a hat right now, or unless I'm grossly overestimating the difficulty of this fix, I think we're too late in the release cycle to make this happen. Since this needs to be added at the libical layer, and doesn't already exist, I feel that even if we had it, it would be far too scary to take without some serious baking time.

Comment 13

[Michael Büttner (no reviews TFN)]

(In reply to comment #12)
I totally agree with this. I would feel far more comfortable to provide a clean patch for this issue within the next weeks, but after the 0.3 release. It's definately non-trivial to add this rule to libical, so we should probably go ahead without it.

Comment 14

[Dan Mosedale (:dmosedale, :dmose)]

Sold.  Setting blocking0.3-.

Comment 15

[cmtalbert]

(In reply to comment #8)
I've dug this bug back up and have been looking at a solution for it.

Unfortunately, everything I am thinking of involves some substantial refactoring. The problem here, as Michael noted, is the negative part of the bysetpos array. The only way to filter those is to know how many total occurrences a given recurrence pattern will generate, per the FREQ setting of the recurrence. 

As I'm seeing it, in order to support both positive and negative BYSETPOS values, we will need to perform the BYSETPOS filtering on the entire array of generated occurrences for the recurrence over a given FREQ. This means that we would have to use libical to generate the entire array of recurrences rather than calling icalrecur_iterator_next ourselves in GetNextOccurrence and GetOccurrences (http://lxr.mozilla.org/seamonkey/source/calendar/base/src/calRecurrenceRule.cpp#368 and #419).

libical already has such a method: icalrecur_expand_recurrence (http://lxr.mozilla.org/seamonkey/source/calendar/libical/src/libical/icalrecur.c#2426)
However, this method is not sufficient for our needs. You would want to call it with count equaling the number of possible occurrences per the given FREQ setting so that you could then have a set of occurrences on which to make the BYSETPOS determination. Getting that information might involve calculating recurrences that are outside the given recurrence range specified in GetOccurrences/GetNextOccurrence. Furthermore, this method returns its values in UTC time, and that would probably cause calIDateTime to convert each occurrence in the array back into the user's timezone.

I think a better solution would be to create a new method that takes the specified range, iterates from start to end of that range by stepping over the FREQ, and creates an array after determining the BYSETPOS restriction. The array it would return would be an array of icaltimetype structs, which could then be easily converted to a calIDateTime array in GetOccurrences.

Some pseudocode to help explain the libical algorithm we need:
k=0
freqcheck = 0
do {
   next = getNextOccurrence()
   i=0
   while ((next != ical_is_null_time) && (next < (STARTRANGE + FREQ)) {   
       testArray[i] = next;
       next = getNextOccurrence()
       i++
   }
   // checked our first FREQ period
   freqcheck++   

   // Now get the elements from the set of all occurrences in FREQ
   // that were specified by the BYSETPOS attribute in the ICS
   for each j in BysetposValues {
        finalResult[k] = filterBysetpos(BysetposValues[j], testArray);
        k++
   }   
}while(STARTRANGE + (FREQ * freqcheck) < ENDRANGE) 
  
return finalResult

The major advantage of this is that it does not change the existing libical recurrence calculation code. Because of that, we could limit the use of this BYSETPOS recurrence calculation to situations when there is a BYSETPOS attribute specified on the RRULE. 

For RRULES without BYSETPOS, calIRecurrenceRule::GetNextOccurrence and GetOccurrences could continue to function the way that they do today. Of course, it would be more consistent to change these methods so that they always call the "Bysetpos recurrence calculation" method, and simply streamline that method for situations when there is no BYSETPOS attribute specified. In those situations, I could envision it working in the exact same manner as the GetNextOccurrence and GetOccurrences methods.

Comments encouraged!

If this sounds like a plausible solution, I'd be happy to create a patch.

Comment 16

[cmtalbert]

Attachment: Simple positive BYSETPOS example from Outlook

Comment 17

[cmtalbert]

Attachment: A simple negative BYSETPOS example from Outlook

Comment 18

[Michael Büttner (no reviews TFN)]

Clint, as far as I can tell this indeed sounds like a plausible solution. At least I can't find anything wrong with this approach and it has many advantages. Sorry for my late reply :-(

Comment 19

[Mark Swanson]

Comment: ScheduleWorld also uses +/- BYSETPOS in the same way Outlook and Evolution do. For full interop BYSETPOS needs to be done. Please implement this.

Comment 21

[Simon Paquet [:sipaq]]

Setting wanted0.8- as the main Calendar developers will not devote any time to
this in the 0.8 timeframe. Patches are, of course, always welcome.

Comment 22

[Bernard Desruisseaux]

Lack of proper support for the BYSETPOS rule part prevents Lightning/Sunbird
from supporting the RRULE generated by Microsoft Outlook for the following recurrence pattern:

- Monthly the first Monday
- Monthly the first weekday
- Monthly the first weekend day
- Monthly the last day of the month
- Yearly the first Monday of April
- Yearly the first weekday of April
- Yearly the first weekend day of April
- Yearly the first day of April

See: iCalendar Recurrence Rules Interoperability Matrix
http://www.calconnect.org/tests/iCalendar-RRULE-Interop/iCalendar-RRULE-Interop-Matrix.html

Comment 24

[Martin Schröder [:mschroeder]]

From libical sourceforge.net project:

Revision: 927
Author: kecsup
Date: 13:16:21, Dienstag, 3. März 2009
Message:
Upstream patch (Maemo/Nokia): Add BYSETPOS support for recurrence rules
----
Modified : /trunk/libical/src/libical/icalrecur.c

Comment 25

[Klaus]

Subird 1.9.1.3567: BYSETPOS is still not working :-(

with todays nightlybuild "sunbird-1.0pre.en-US.win32.installer.exe" dated on "October 7th 2009" BYSETPOS is still not working :-(

my current workaround:

after exporting from msoutlook2007 

i have to do replacements in the ics file like 
replace "BYDAY=TH;BYSETPOS=2" with "BYDAY=2TH" 
replace "BYDAY=MO;BYSETPOS=-1" with "BYDAY=-1MO" 

after that the recurrences are fine with sunbird.

msoutlook2007 can interpret this format, too.
unfortunately msoutlook2007 converts them back to BYSETPOS :-(

Comment 27

[browerg]

The behavior reported in Bug 560366 (event intended to occur monthly occurs daily) also occurs when the intended repeat period is weekly.

Comment 28

[browerg]

(In reply to comment #27)
> The behavior reported in Bug 560366 (event intended to occur monthly occurs
> daily) also occurs when the intended repeat period is weekly.

Whatever its number, this bug has a "Major" effect on me because much of my work life involves events that recur ndays/week for about a quarter of the year or for nmonths on the same relative day (3d Thursday) etc.

Comment 29

[George V. Reilly]

RRULE:BYSETPOS=1;FREQ=MONTHLY;BYDAY=TH

happens only on the first Thursday of the month, but Lightning 1.0b2 displays it for every Thursday.

Comment 31

[Felix Möller]

Will this be solved by bug #637064?

Comment 33

[Szimszon]

I'll need it to because:

"example, a bysetpos of -1 if combined with a MONTHLY frequency, and a byweekday of (MO, TU, WE, TH, FR), will result in the last work day of every month. "

  http://niemeyer.net/python-dateutil#head-cf004ee9a75592797e076752b2a889c10f445418

Comment 34

[Jason]

This still needs fixing, unless there's another way to implement this example rule:

"every year on the first weekday on/after 26 January".


If this bug was fixed I could just do this:  RRULE:FREQ=YEARLY;BYDAY=MO,TU,WE,TH,FR;BYMONTHDAY=26,27,28;BYSETPOS=1

Comment 35

[pedrum]

Another data point here. I have a recurring meeting with the below RRULE (originates from Outlook). The intent is to have one event on the first workday (M-F) of the month.

In Lightning, this look like a repeating event for all M-F days in first week of the month, so 5 events total. This looks fine in OWA (Outlook Web) calendar.

RRULE:FREQ=MONTHLY;UNTIL=20191231T190000Z;INTERVAL=1;BYDAY=MO,TU,WE,TH,FR;B
YSETPOS=1

I've attached a sample ICS file.

Comment 36

[pedrum]

Attachment: Sample repeating entry first day of month example

Comment 37

[Martin Schröder [:mschroeder]]

libical has now been removed - bug 1787097. ical.js supports BYSETPOS.