Last Comment Bug 322458 - BYSETPOS not considered when getting occurrences
: BYSETPOS not considered when getting occurrences
Status: NEW
[needs patch]
: dataloss, relnote
Product: Calendar
Classification: Client Software
Component: Internal Components (show other bugs)
: unspecified
: All All
: -- normal with 15 votes (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
http://www.calconnect.org/tests/PayDa...
: 173026 332293 479775 560366 584581 644742 (view as bug list)
Depends on:
Blocks: 390620
  Show dependency treegraph
 
Reported: 2006-01-05 03:56 PST by Joey Minta
Modified: 2011-11-04 20:40 PDT (History)
21 users (show)
dbo.moz: wanted‑calendar1.0+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Simple positive BYSETPOS example from Outlook (1.24 KB, text/plain)
2006-11-27 22:50 PST, cmtalbert
no flags Details
A simple negative BYSETPOS example from Outlook (1.25 KB, text/plain)
2006-11-27 22:51 PST, cmtalbert
no flags Details

Description Joey Minta 2006-01-05 03:56:28 PST
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@) 2006-03-30 22:37:45 PST
*** Bug 332293 has been marked as a duplicate of this bug. ***
Comment 2 Michael Büttner (no reviews TFN) 2006-05-09 23:21:14 PDT
taking this one.
Comment 3 Michael Büttner (no reviews TFN) 2006-05-09 23:21:49 PDT
assigned.
Comment 4 Dan Mosedale (:dmose) 2006-09-12 15:04:27 PDT
This seems like fairly basic dataloss; setting as blocking 0.3.
Comment 5 Michiel van Leeuwen (email: mvl+moz@) 2006-09-14 13:35:38 PDT
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) 2006-09-15 00:04:02 PDT
(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) 2006-09-15 00:11:46 PDT
(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) 2006-09-15 02:01:34 PDT
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 (:dmose) 2006-09-19 11:46:54 PDT
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 2006-09-21 01:05:28 PDT
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 2006-09-23 21:39:05 PDT
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 2006-09-26 22:23:19 PDT
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) 2006-09-26 23:46:17 PDT
(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 (:dmose) 2006-09-27 09:39:02 PDT
Sold.  Setting blocking0.3-.
Comment 15 cmtalbert 2006-11-27 22:47:19 PST
(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 2006-11-27 22:50:35 PST
Created attachment 246754 [details]
Simple positive BYSETPOS example from Outlook
Comment 17 cmtalbert 2006-11-27 22:51:23 PST
Created attachment 246755 [details]
A simple negative BYSETPOS example from Outlook
Comment 18 Michael Büttner (no reviews TFN) 2007-01-09 07:04:55 PST
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 2007-03-30 09:19:16 PDT
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 20 Martin Schröder [:mschroeder] 2007-07-05 16:10:38 PDT
*** Bug 173026 has been marked as a duplicate of this bug. ***
Comment 21 Simon Paquet [:sipaq] 2007-11-24 12:12:06 PST
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 2009-02-05 15:10:17 PST
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 23 Stefan Sitter 2009-02-23 06:11:30 PST
*** Bug 479775 has been marked as a duplicate of this bug. ***
Comment 24 Martin Schröder [:mschroeder] 2009-03-08 11:04:15 PDT
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 2009-10-07 09:06:31 PDT
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 26 Stefan Sitter 2010-04-19 22:38:47 PDT
*** Bug 560366 has been marked as a duplicate of this bug. ***
Comment 27 browerg 2010-05-14 14:59:10 PDT
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 2010-05-14 15:29:09 PDT
(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 2011-02-23 16:42:41 PST
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 30 Stefan Sitter 2011-03-24 13:43:30 PDT
*** Bug 644742 has been marked as a duplicate of this bug. ***
Comment 31 Felix Möller 2011-03-28 06:55:53 PDT
Will this be solved by bug #637064?
Comment 32 Stefan Sitter 2011-04-02 01:04:50 PDT
*** Bug 584581 has been marked as a duplicate of this bug. ***
Comment 33 Szimszon 2011-05-16 21:43:44 PDT
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

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