327890 - ICS providers doesn't serialize exceptions (ie modified recurrences)

Status: RESOLVED FIXED

(Calendar :: Internal Components, defect)

Description

[Dan Mosedale (:dmosedale, :dmose)]

As I understand it from mvl, the ICS provider can deal with EXDATEs to delete specific recurrences, but can't yet handle moving them.

Comment 1

[Michael Büttner (no reviews TFN)]

taking this one.

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

jminta pointed out that CalDAV almost certainly has this problem too.  The fix (or most of it) is probably either to calICSService and/or libical itself.

Comment 3

[Michael Büttner (no reviews TFN)]

Attachment: ics-fix v1

this patch fixes the issue with exception handling of the ics provider, credits to part of this patch go to [e-mail]. indeed the caldav provider has similar problems, but as we had the impression that there's a lot of functionality missing, we would suggest to take the patch for the ics-provider first and concentrate on the caldav provider later.

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

Sorry for not getting to this review tonight; with luck Ill get to it tomorrow, or else convince mvl to steal it from me.  :-)

Comment 5

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

Comment on attachment 212594 [details] [diff] [review]
ics-fix v1

This patch looks really nice and elegant. Thanks a lot!
I just have a few minor comments on them, mostly style comments.

>diff -x CVS -U 8 -pN -r /cygdrive/d/pim/mozilla/mozilla_ref/calendar/base/public/calIICSService.idl mozilla/calendar/base/public/calIICSService.idl
>+    /**
>+     * The recurrence ID, a.k.a. DTSTART-of-calculated-occurrence,
>+     * or null if this isn't an occurrence.
>+     */
>+    attribute calIDateTime recurrenceId;

Any reason to make this readwrite? I think it makes no sense to assign a recurrenceId to an item. You get an item from a recurrence-set, and modify it. In that case, recurrenceId will already be set.
Hmm, thinking about it, you likely need it for construction of the object. In that case, please add a comment explaining that this generally should not be written into.

>+++ mozilla/calendar/providers/ics/calICSCalendar.js	2006-02-21 17:09:45.602246400 +0100
>+                        var item = null;
>                         switch (subComp.componentType) {
>                         case "VEVENT":
>-                            var event = Components.classes["@mozilla.org/calendar/event;1"]
>-                                                  .createInstance(Components.interfaces.calIEvent);
>-                            event.icalComponent = subComp;
>-                            this.mMemoryCalendar.adoptItem(event, null);
>+                            item = Components.classes["@mozilla.org/calendar/event;1"]
>+                                .createInstance(Components.interfaces.calIEvent);

stylenit: keep the .createInstance part aligned like it was, with the dots in the same column.

>+                        if (item != null) {
just |if (!item)| will do :)

>+            // tag "exceptions", i.e. items with rid:
>+            for each ( var item in excItems ) {

style nit: new calendar code doesn't add spaced after ( and before ), so make this:
+            for each (var item in excItems) {
Also for a couple of other cases in this code block.

>@@ -329,17 +361,28 @@ calICSCalendar.prototype = {
>             onGetResult: function(aCalendar, aStatus, aItemType, aDetail, aCount, aItems)
>             {
>                 for (var i=0; i<aCount; i++) {
>-                    calComp.addSubcomponent(aItems[i].icalComponent);
>+                    var aItem = aItems[i];

style nit: variables with an 'a' prefix, like aItem come from parameters to the method. For this local var, use |item|, so no prefix.


In testing, i found one problem: when i make an exception by changing the starttime of one of the items, and then delete that exception, an EXDATE is added, but the exception item (vevent with recurrence-id set) still stays in the file. Is that a bug in this code, or in the deletion code?

leaving off marking r+, waiting for the answer to the two questions. I will mark r+ after that, but i would like the style comments to be fixed. (or dmose can mark review. I will be sleeping and working for a while)

Comment 6

[Michael Büttner (no reviews TFN)]

Attachment: ics-fix v2

new iteration for this patch, i just adapted the style nits.

1) the recurrenceId is not readonly since this attribute is written to at http://lxr.mozilla.org/seamonkey/source/calendar/base/src/calItemBase.js#478, which will be used during construction of an item (as you already guessed). since in other circumstances it might or might not be necessary to write to this attribute as well, i didn't add an comment. feel free to add one if you think it should be necessary.

2) yes, you're absolutely right, the exceptions is persisted in the file. but it has nothing to do with the ics-provider (doublecheck with the storage provider). so it's a feature, not a bug ;-) to be serious, i think we should maybe file a follow-up bug which discusses if this behavior is desired or not. of course it wastes a lot of memory/bandwidth/etc, most notably probably with infinite recurrences and loads of deleted exceptions. but this patch has no deal with it.

remark: editing recurring events is a pain in the ass without this patch: https://bugzilla.mozilla.org/show_bug.cgi?id=328084, otherwise you'll encounter loads of other strange behavior.

Comment 7

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

Comment on attachment 213043 [details] [diff] [review]
ics-fix v2

r=mvl

Comment 8

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

After disciing with dmose on irc, i've checked in the patch. I think the risk for events without exceptions to be able to check in now.

Filed bug 328510 about the issue with deleting.

Before checking in, I added a short comment about the readonly-but-not-really attribute.

Comment 9

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

Filed bug 328576 for caldav