337935 - weekly recurrence with sunday hides occurrences on other weekdays

Status: VERIFIED FIXED

(Calendar :: Internal Components, defect)

Description

[gekacheka]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.0.3) Gecko/20060426 Firefox/1.5.0.3
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20060507 Mozilla Sunbird/0.3a2

If a weekly recurrence includes sunday, other days are not displayed in week view or day view.  

In multiday view or month view, the other days are shown except on the last week.

Reproducible: Always

Steps to Reproduce:
1. create a weekly recurring event that does not include sunday,
   repeating on, say, monday, thursday, and saturday.
   (monday thursday saturday occurrences are shown 
    in day,week,multiweek,and month views)
2. add sunday (edit event)
   (only sunday occurrences are shown in day and week view,
    only sunday occurrence is shown in last week of multiweek and month view)
3. remove sunday (edit event)
   (monday thursday saturday occurrences are shown 
    in day,week,multiweek,and month views)


Actual Results:  
After step 2, only sunday occurrences are shown in day and week view,
only sunday occurrence is shown in last week of multiweek and month view.

Expected Results:  
After step 2, sunday monday thursday saturday occurrences should all be shown in all views.

Similar problems happen in sunbird 0.3a1.
- last week of view problem seems to occur in 0.3a1 multiweek view, but not 0.3a1 month view

Comment 1

[Michael Büttner (no reviews TFN)]

taking this one.

Comment 2

[Michael Büttner (no reviews TFN)]

the cause of the problem is a bit involved, but i'll try to explain the behavior. assume a weekly recurrence rule, set for SU,MO,TU. next_weekday_by_week() (libical, icalrecur.c) is responsible for generating the specific occurrences for this particular rule. in this example, occurrences will be generated for sunday, monday and tuesday (in this particular order). since libical takes the week start (WKST attribute) into account, the occurrences will be generated as shown in the following table:

        |SU MO TU WE TH FR SA
        |--------------------
Week -1 |    2  3
        |--------------------
Week 0  | 1  5  6
        |--------------------
Week 1  | 4

the numbers show the order in which the occurrences will be generated. the first occurrence will end up on week 0, sunday, as expected. but the next two occurrences will be scheduled for the *previous* monday and tuesday, since the week start is (by default) set to monday. this behavior is correct, since RFC 2445 states 'A week is defined as a seven day period, starting on the day of the week defined to be the week start (see WKST).', paragraph 4.3.10, page 42.

the next occurrences will appear on the sunday of week 1, and monday, tuesday on week 0. as a net result, the occurrences will be scheduled correctly, but the order is wrong, and it will break as soon as an interval greater than 1 is used.

why the sunday occurrence 'hides' the other onces is related to how calRecurrenceRule::GetOccurrences() retrieves them from the the rules. as soon as the first occurrence outside the range is encountered, the loop stops asking for further ones. again, this behavior is correct, since we expect to receive the occurrences in a strict linear order. in the above example, this means that generating occurrences stops after item number 4 has been seen. items number 5 and 6 will therefore never be generated.

the problem is that the occurrences won't be generated in a strict linear order. if this would be the case the termination condition used in calRecurrenceRule::GetOccurrences() would just work.

Comment 3

[Michael Büttner (no reviews TFN)]

Attachment: patch v1

this patch assures that the occurrences for weekly recurrences will be generated in a strict linear order. in order to complete the big picture, all of this happens because calRecurrenceRule::SetComponent() just fills in the BY_* arrays without updating the internal state of libical. but since the design of libical allows for such modifications, sorting the BY_DAY array needs to happen right where the occurrences will be generated.

Comment 4

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 223759 [details] [diff] [review]
patch v1

> static void
>-sort_bydayrules(struct icalrecur_parser *parser)
>+sort_bydayrules(short *array,int week_start)
> {

libical style has a space between formal parameters, not just a comma.

>-    short *array;
>-    int week_start, one, two, i, j;
>+  int one, two, i, j;

Indentation is wrong throughout, please fix to match the rest of the file (which appears to use tabs, I think).

>+  /* see bug #337935 */
>+  sort_bydayrules(BYDAYPTR,impl->rule.week_start);
>+
>   /* If we get here, we need to step to tne next day */

Don't add a comment referencing a closed bug (but do use bug numbers for FIXMEs!) -- instead please at a concise description of why we need to sort here, probably cribbed from comments 2 or 3.

r=shaver with those issues repaired, thanks!

Comment 5

[Michael Büttner (no reviews TFN)]

patch checked in.

Comment 6

[Damian Szczepanik]

verified with
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060929 Sunbird/0.3