Open
Bug 274967
Opened 20 years ago
Updated 2 years ago
Need to be able to sync 2 calendars
Categories
(Calendar :: Internal Components, enhancement)
Calendar
Internal Components
Tracking
(Not tracked)
NEW
People
(Reporter: mvl, Unassigned)
References
Details
Attachments
(1 file, 3 obsolete files)
18.15 KB,
patch
|
Details | Diff | Splinter Review |
For sharing etc we need to be able to sync two calendars. Oh, and for syncing with your handheld ofcourse.
Reporter | ||
Comment 1•20 years ago
|
||
Reporter | ||
Comment 2•20 years ago
|
||
Patch updated. Now with more comments etc.
Attachment #168889 -
Attachment is obsolete: true
Reporter | ||
Comment 3•20 years ago
|
||
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 4•19 years ago
|
||
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-
Reporter | ||
Comment 5•19 years ago
|
||
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)
Reporter | ||
Comment 6•19 years ago
|
||
I forgot to include the changes to calItemModule.js in the new patch, but those are the same as in patch v3.
Updated•19 years ago
|
Attachment #172198 -
Flags: first-review?(dmose)
Comment 7•19 years ago
|
||
*** Bug 215972 has been marked as a duplicate of this bug. ***
Updated•18 years ago
|
Flags: blocking0.3?
Updated•18 years ago
|
Flags: blocking0.3? → blocking0.3-
Comment 8•18 years ago
|
||
'P2 0.5 Sync (with external files / other calendars)" So I nominate it as blocking 0.5
Flags: blocking-calendar0.5?
Comment 9•17 years ago
|
||
We won't hold 0.5 up for this.
Flags: blocking-calendar0.5? → blocking-calendar0.5-
Updated•16 years ago
|
Flags: blocking-calendar0.5-
Updated•16 years ago
|
Severity: normal → enhancement
Updated•14 years ago
|
Assignee: mvl → nobody
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•