310221 - icalendar items with a duration and no dtend are handled incorrectly

Status: RESOLVED FIXED

(Calendar :: Internal Components, defect)

Description

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

Spin-off from bug 309868:
With the path in that bug the items from the testcase are imported, but not
displayed in the views. The unifinder lists the enddata as 1-1-1970. It seems
the duration in the vevents is ignored.

Comment 1

[Stefan Sitter [:ssitter]]

Attachment: testcase with one event

minimal ics file with one event, created at
http://www.project24.info/feiertage.php

Comment 2

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

Attachment: patch v1

This patch adds calIIcalComponent.duration. That's the major part of the patch. Then there is a small part to actually read this value, and add it to the starttime.

Comment 3

[Stefan Sitter [:ssitter]]

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

Comment 4

[Stefan Sitter [:ssitter]]

Some question to satisfy curiosity:

RFC 2445 says:
    ; either ’dtend’ or ’duration’ may appear in
    ; a ’eventprop’, but ’dtend’ and ’duration’
    ; MUST NOT occur in the same ’eventprop’

How is this handled with this patch? Does the event contains dtend and duration e.g. after export? Or is dtend only used internally (calculated at runtime) and only duration is stored? Or is duration discarded and dtend is stored?

Comment 5

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

(In reply to comment #4)
> How is this handled with this patch?

the duration is never exported, only dtend. (yes, this might be dataloss, but i consider it very minimal dataloss. Way better then without the patch)

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

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

Looks good; just a few nits...

> 
>+calDuration::calDuration(struct icaldurationtype *adurationptr)

Nit: aDurationPtr would be easier on the eyes.

Also, is there any reason not to make this be 

|const struct icalduration type *const aDurationPtr| for more compiler optimization loving?

>+{
>+    FromIcalDuration(adurationptr);
>+    mImmutable = PR_TRUE;

Why make it immutable?  This seems to have the opposite semantics of the existing copy constructor...

>+void
>+calDuration::FromIcalDuration(const struct icaldurationtype *icald)

I bet this could be a const pointer to a const object also.

>+{
>+    mDuration.is_neg  = icald->is_neg;
>+    mDuration.weeks   = icald->weeks;
>+    mDuration.days    = icald->days;
>+    mDuration.hours   = icald->hours;
>+    mDuration.minutes = icald->minutes;
>+    mDuration.seconds = icald->seconds;
>+}

Prefer to return rather than falling off the end, if you would.

>Index: base/src/calEvent.js
>===================================================================
>RCS file: /cvsroot/mozilla/calendar/base/src/calEvent.js,v
>retrieving revision 1.29
>diff -u -9 -p -d -r1.29 calEvent.js
>--- base/src/calEvent.js	30 Jun 2005 22:41:14 -0000	1.29
>+++ base/src/calEvent.js	8 Nov 2005 21:24:15 -0000
>@@ -200,18 +200,24 @@ calEvent.prototype = {
>             event = event.getFirstSubcomponent("VEVENT");
>             if (!event)
>                 throw Components.results.NS_ERROR_INVALID_ARG;
>         }
> 
>         this.setItemBaseFromICS(event);
>         this.mapPropsFromICS(event, this.icsEventPropMap);
> 
>         this.importUnpromotedProperties(event, this.eventPromotedProps);
>+        
>+        if (event.duration) {
>+            this.endDate = this.startDate.clone();
>+            this.endDate.addDuration(event.duration);
>+        }
>+   

Can you file new dataloss about the two cases this patch doesn't address, or else leave this one open?  Then, if you could add a comment here about preserving the approximate, but not exact, semantics, and include the relevant bug number(s), that'd be great.

r=dmose with the nits addressed

Comment 7

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

Patch checked in. Filed bug 317786 on the remaining issue

Comment 8

[Stefan Sitter [:ssitter]]

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

Comment 9

[kosherjava]

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