Open Bug 274967 Opened 18 years ago Updated 11 years ago

Need to be able to sync 2 calendars

Categories

(Calendar :: Internal Components, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

People

(Reporter: mvl, Unassigned)

References

Details

Attachments

(1 file, 3 obsolete files)

For sharing etc we need to be able to sync two calendars. Oh, and for syncing
with your handheld ofcourse.
Attached patch v1 (obsolete) — Splinter Review
Attached patch v2 (obsolete) — Splinter Review
Patch updated. Now with more comments etc.
Attachment #168889 - Attachment is obsolete: true
Attached patch v3 (obsolete) — Splinter Review
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.
Attachment #169398 - Attachment is obsolete: true
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.
Attachment #170945 - Flags: first-review-
Attached patch v4Splinter Review
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.
Attachment #170945 - Attachment is obsolete: true
Attachment #172198 - Flags: first-review?(dmose)
I forgot to include the changes to calItemModule.js in the new patch, but those
are the same as in patch v3.
Attachment #172198 - Flags: first-review?(dmose)
*** Bug 215972 has been marked as a duplicate of this bug. ***
Flags: blocking0.3?
Flags: blocking0.3? → blocking0.3-
'P2 	 0.5 	 Sync (with external files / other calendars)"

So I nominate it as blocking 0.5
Flags: blocking-calendar0.5?
We won't hold 0.5 up for this.
Flags: blocking-calendar0.5? → blocking-calendar0.5-
Blocks: 287612
Flags: blocking-calendar0.5-
Severity: normal → enhancement
Assignee: mvl → nobody
You need to log in before you can comment on or make changes to this bug.