303826 - Recurring all day events doesn't show up in calendar

Status: RESOLVED FIXED

(Calendar :: Sunbird Only, defect)

Description

[Robin Edrenius]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050806 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b4) Gecko/20050807 Mozilla Sunbird/0.2+

If you create a recurring all-day event it doesn't show up in the calendar-view.

It does, however, show up in the eventfield.
Even if you add a title to the event by doubleclicking it in the eventfield, it
doesn't show up in the calendarview

Reproducible: Always

Steps to Reproduce:
1. Create a recurring all-day event without any title 
2. 
3.

Actual Results:  
It doesn't show up in the calendar

Expected Results:  
It should be displayed in the calendarview, like a non-all-day event, or
non-recurring event, without title does.

Comment 1

[Robin Edrenius]

Attachment: How it should appear

That's how it should appear

Comment 2

[Robin Edrenius]

This seems to appear on all recurring allday events, not only if the title is empty.

Comment 3

[Joey Minta]

Confirmed.

The events are getting skipped in createEventBox in calendarWindow.js because
they are all-day.

http://lxr.mozilla.org/mozilla/source/calendar/resources/content/calendarWindow.js#716

Blocks 0.3a1.

Comment 4

[Joey Minta]

Here's the deal:
Occurrences of recurring all-day events are being returned with identical
start/end dates, rather than having an exclusive end date a day later. 
Normally, the duration of the event (in this case, properly calculated as 1 day)
would be added to the occurrence's startdate at
http://lxr.mozilla.org/mozilla/source/calendar/base/src/calRecurrenceInfo.js#471

This isn't happening in the case of all-day events.  Why? Because addDuration
doesn't work properly when isDate is true.

A workaround patch for this specific problem is to store isDate in a temp
variable, set it to false, add the duration, and then resture isDate.  Really
though, calDateTime::AddDuration should do this itself, throwing errors if
(isDate == true && (aDuration.seconds != 0 || aDuration.minutes != 0 ||
aDuration.hours != 0))

Comment 5

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

Attachment: patch v1

This patch fixes libical to not be weird.
It used to convert the duration to seconds and then normalize. But when
normalizing a date-only datetime, it ignored additions to the seconds. So
additions to a date-only never worked.

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

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

>Index: base/src/calRecurrenceInfo.js
>===================================================================
> [...]
>-        var count = aMaxCount;
>+        var count = aMaxCount < aCount.value ? aMaxCount : aCount.value;

aCount is an out param.  Why are we reading it here at all?

>Index: libical/src/libical/icalduration.c
>===================================================================
> [...]
> struct icaltimetype  icaltime_add(struct icaltimetype t,
> 				  struct icaldurationtype  d)
> {
>-    int dt = icaldurationtype_as_int(d);
>-    
>-    t.second += dt;
>+    t.second += d.seconds;
>+    t.minute += d.minutes;
>+    t.hour += d.hours;
>+    t.day += d.days;
>+    t.day += d.weeks * 7;
>     
>     t = icaltime_normalize(t);
>     
>     return t;
> }

So I can see why the above fix works for this particular problem.  However, I'm
not convinced this is really the right fix overall.  As it stands, even if one
argues that the semantics of icaltime_adjust are correct (and I don't really
have a lot of confidence that they are), it seems to that the
icaltime_normalize() semantics are fairly clearly wrong: the time is known not
to be in normal form, so dropping the seconds/hours/minutes doesn't seem like a
reasonable thing to do.

I'd also be interested in hearing shaver & vlad's thoughts on this...

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

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

After discussion on IRC, r=dmose once the out param issue is addressed.  This
fix is reasonable independent of fixing libical's normalize issues, and almost
certainly less risky.

Comment 8

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

patch checked in