274967 - Need to be able to sync 2 calendars

Status: NEW

(Calendar :: Internal Components, enhancement)

Description

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

For sharing etc we need to be able to sync two calendars. Oh, and for syncing
with your handheld ofcourse.

Comment 1

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

Attachment: v1

Comment 2

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

Attachment: v2

Patch updated. Now with more comments etc.

Comment 3

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

Attachment: v3

This version now compares last-modified instead of the whole event. It does
assume that other clients update last-modified, but doesn't assume their clocks
are ok.
Takes away the need for a local calendar with the changed events. Also takes
away the pain to compare the contents of two events.

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 170945 [details] [diff] [review]
v3

In general, this looks pretty good; a lot of my comments are just nits.

* avoid starting dump()s in column zero (ie out of sync with the rest of the
indentation).

* just writing pseudocode isn't really sufficient commentary; there's already
something there in code form, it's better to intersperse english comments where
appropriate to explain what each piece of code is doing rather than just
writing the algorithm in pseudocode at the very top.

>Index: base/public/calICalendar.idl
>===================================================================
>RCS file: /cvsroot/mozilla/calendar/base/public/calICalendar.idl,v
>retrieving revision 1.13
>diff -u -9 -p -d -r1.13 calICalendar.idl
>--- base/public/calICalendar.idl	2 Dec 2004 20:48:09 -0000	1.13
>+++ base/public/calICalendar.idl	11 Jan 2005 19:18:57 -0000
>@@ -289,19 +289,19 @@ interface calICompositeCalendar : calICa
>  * nsISupports data and use that instead?
>  */
> [scriptable, uuid(2953c9b2-2c73-11d9-80b6-00045ace3b8d)]
> interface calIObserver : nsISupports
> {
>   void onStartBatch();
>   void onEndBatch();
>   void onLoad();
>   void onAddItem( in calIItemBase aItem );
>-  void onModifyItem( in calIItemBase aNewItem, in calIItemBase aOldItem );
>+  void onModifyItem( in calIItemBase aOldItem, in calIItemBase aNewItem);

So did you decide if this change should stay, or if we need to fix up
consumers, or what?


>Index: base/public/calISyncEngine.idl
>===================================================================
> 
> [...]
>
>+[scriptable, uuid(e2697e63-cd5f-4b4f-b511-eecd8af02d43)]
>+interface calISyncEngine : nsISupports
>+{
>+    /**
>+     * Set the local and the remote calendar that should be synced.
>+     * The local calendar should be set before any changes are made.
>+     * If eny change is made while the sync-engine is not hooked-up,

	   "any"

>+     * the changes might get lost.
>+     * This implies that no other processes can change the local calendar.
>+     */

It would be helpful to step back and describe this at a slightly higher level,
I think.  That is, talk about what sort of calendars this is appropriate for
(ie local calendar must call back all changes via the observer), what the
lifetime of the implementing object looks like relative to the lifetime of the
calendars, and exactly how it's intended to be used.

>Index: base/src/calSyncEngine.js
>===================================================================
>
> [...]
>
>+ * If you that you are syncing with a file that is only touched by
>+ * other mozilla's,

This sentence fragment looks like it could use a re-write, I think.

>+    sync: function() {

If you also give this function a name, in addition to assigning it to the sync
property, it will be easier to debug in Venkman.

>+        var savedthis = this;
>+
>+dump("** sync start\n");
>+        this.mSyncing = true;
>+
>+        // Start by getting all the items into an array
>+        var localItems = [];
>+        var remoteItems = [];
>+
>+        // This will get the items from the calendars into the global arrays,
>+        // then call doSync()
>+        getCalandarItems();

"Calendar"

>+            /*
>+             * Part I: local to remote
>+             *
>+             * forall changeEvents
>+             *   if (!remoteEvent)
>+             *     remoteCalendar.add(localEvent);
>+             *   else if (remoteEvent.lastModified != changedEvent.lastModified)
>+             *     conflict();
>+             *   else if (!localEvent)
>+             *     remoteCalendar.deleteEvent(changedEvent.id)
>+             *   else
>+             *     remoteCalendar.modify(remoteEvent.id, localEvent);
>+             *
>+             * The last step might mean warning about conflicts if both
>+             * event got the same edit, but at different times (possibly
>+             * different clients). That's a small price for not having to
>+             * compare every detail of the events.
>+             */

"The last step"?

It would be good to explain why you're checking lastModified (ie how this
algorithm works).

Also, I don't agree with the comment at all.  Putting up confusing UI (ie
warning someone about something which isn't a problem) is really bad.  While I
think this is a reasonable heuristic for a first cut at the code, it's my
feeling that this should be marked with an XXX comment indicating that it needs
to be fixed sooner rather than later.

>+            var getListener =
>+            {
>+                onOperationComplete: function(aCalendar, aStatus, aOperationType, aId, aDetail)
>+                {

Should probably check whether aStatus is a failure code both here and in
onGetResult, and abort the sync in either case.  Otherwise dataloss seems
likely.

>+                    calendarsFinished++;
>+                    if (calendarsFinished == 2) {
>+                        doSync();
>+                    }
>+                },
>+                onGetResult: function(aCalendar, aStatus, aItemType, aDetail, aCount, aItems)
>+                {
>+                    if (aCount) {
>+                        var ar;
>+                        if (aCalendar == savedthis.mLocalCalendar)
>+                            items = localItems;
>+                        else if (aCalendar == savedthis.mRemoteCalendar)
>+                            items = remoteItems;

It's probably worth commenting here that items is a reference and not a copy,
just for clarity's sake.

I suspect you're going to need a callback interface for conflict resolution,
since sometimes this will need to be decided by the user.

Comment 5

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

Attachment: v4

Patch updated.

> So did you decide if this change should stay, or if we need to fix up
> consumers, or what?

I'm going for what the idl currently says, and will file a new bug to make
everything consitent.

Comment 6

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

I forgot to include the changes to calItemModule.js in the new patch, but those
are the same as in patch v3.

Comment 7

[Simon Paquet [:sipaq]]

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

Comment 8

[Omar Bajraszewski]

'P2 	 0.5 	 Sync (with external files / other calendars)"

So I nominate it as blocking 0.5

Comment 9

[Matthew (lilmatt) Willis]

We won't hold 0.5 up for this.