Closed Bug 393395 Opened 17 years ago Closed 16 years ago

Offline Support infrastructure: Synchronization interfaces

Categories

(Calendar :: Internal Components, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: dbo, Assigned: dbo)

References

Details

(Whiteboard: [roadmap 0.8])

Attachments

(3 files, 1 obsolete file)

0.7 roadmap item: sketch how offline will work and flesh out interfaces.

Referencing related work in progress bugs: bug 380060, bug 366923, bug 274967.
Assignee: nobody → daniel.boelzle
Severity: normal → enhancement
Flags: blocking-calendar0.7+
Whiteboard: [roadmap 0.7]
Status: NEW → ASSIGNED
Flags: blocking-calendar0.7+
Flags: wanted-calendar0.8+
This is one of the main three goals for version 0.8 [1]. A release shouldn't
happen without this bug fixed. Requesting blocking stauts.

[1] http://wiki.mozilla.org/Calendar:Status_Meetings:2007-10-31
Flags: blocking-calendar0.8?
Whiteboard: [roadmap 0.7] → [roadmap 0.8]
Target Milestone: --- → 0.8
Depends on: 341776
Blocks: 380060
It looks like there is a broad disagreement with my point of view. Removing blocking request.
Flags: blocking-calendar0.8?
We will not release 0.8 without this.
Flags: blocking-calendar0.8+
Attached patch WIP patch (obsolete) — Splinter Review
first shot by Philipp and me
Attachment #294750 - Flags: review?(philipp)
A high-level description of what this patch does and how would be welcome... (without getting into code details)
This patch implements
- offline gadget for sunbird
- adds "Cache (requires restart)" to calendar property page, so users can switch caching on
- implements calCachedCalendar that triggers either full sync or partial sync (calIchangelog based)
- "relaxedMode" changes for storage calendar
- calIChangeLog for WCAP
- misc improvements
(I might have forgotten some stuff)
I was more hoping for a comment along the lines of:
'this patch implements cacheing by having an object calCachingCalendar which all providers inherit from, etc, etc' (or however the patch _works_). I mean en explanation of the process of storing stuff offline.
From a totally not complete review:

Index: calendar/base/public/calIChangeLog.idl
+/**
+ * Tagging interface for synchronously working providers, e.g. storage.
+ */
+interface calISyncCalendar : calICalendar {
+};

I don't understand the comment above. I also don't understand why the interface
is empty. (maybe those two problems are related :) )

+interface calIChangeLog : nsISupports {
+    /**
+     * Instructs the calendar to replay remote changes into the given
+     * calendar. The calendar itself is responsible for storing anything needed
+     * to keep track of what items need updating.

Why do you leave that thoe the calendar? I guess that the code would be the
same for all calendar. So why not make it seperate code? (yes, you can include
code in all providers, but that is not nearly as nive. For starters, it doesn't
work if the proveder is written in something else then javascript)

+ * Not implementing this interface is perfectly valid for calendars, that need
+ * to do a full sync each time anyway (i.e ics)

That's not true. You want to be able to know what changed locally and what
changed remotely (while offline) to figure out how to merge the local and remote
ics file. So you do need a changelog for ics too.

Index: calendar/base/src/calCalendarManager.js

     unregisterCalendar: function(calendar) {
+        // calendar may be a calICalendar wrapper:
+        if (calendar.wrappedJSObject instanceof calCachedCalendar) {
+            calendar.wrappedJSObject.onCalendarUnregistering();
+        }

You can't use .wrappedJSObject here. You don't know if the provider is
implemented in javascript or some other language.

+    mRelaxedMode: undefined,
+    get relaxedMode() {
+        if (this.mRelaxedMode === undefined) {
+            this.mRelaxedMode = this.getProperty("relaxedMode");
+        }
+        return this.mRelaxedMode;
+    },
+
+calWcapCalendar.prototype.syncChangesTo_ =
+function calWcapCalendar_syncChangesTo_(destCal, itemFilter, dtFrom_, listener)

You know what I think about leading underscores: don't do it! It doesn't mean
anything, it's not documented (afaik) and just makes it harder to read. For the
parameter, use the common a prefix (aDtFrom) to prevent name clashing. For
the function, a common style is to prefix 'Internal': cWC_syncChangesToInternal

Comment on attachment 294750 [details] [diff] [review]
WIP patch

Since caching is still experimental, it is off by default, it can be enabled using the moz pref calendar.cache.enabled. As soon as this is enabled, it also needs to be enabled on a per-calendar basis. This patch modifies the calendar properties dialog with an additional checkbox. Changing this requires a restart though. Some minor problems might show up if not, since the calendar wrapper is not created before restart, but the property is changed instantly. If it turns out to be a problem we could either force the user to restart, or better yet do a full unregister-register cycle to allow the calendar manager to create the wrapper or discard it.

In Sunbird, UI is added to allow switching online/offline modes. The same icon as in thunderbird is used and its placement is also the same. Currently there is only the status bar icon. Additionally we should add a menu entry similar to thunderbird's "Work Offline". Currently there is a problem with changing offline mode while in calendar mode in lightning, due to missing menu elements. This will be taken care of in a separate patch or will be worked around by showing the work offline menu also in calendar mode.

The calendar manager itself checks for caching support and enabling when registering the calendar. At this moment, a wrapper object (calCachedCalendar) is created, that behaves like a calICalendar on the outside, so that the usual provider calls work fine. Most calls are then forwarded to an internal storage calendar, that keeps the items that have been synchronized. calCachedCalendar also knows of the real provider and will synchronize items in different ways, depending on if the provider supports custom synchronization steps.

Providers can optionally implement a calIChangeLog interface, which allows them more fine-grained control as to how the synchronization should happen. If this interface is not implemented, the calendar manager defaults to a full getItems() call on the provider to fill the internal storage calendar. This interface does not need to be implemented, since this patch is only step 1 of offline support, allowing the user to view his events while in offline mode. Changes to events while offline cannot be made in this step!

Some steps required to keep the user from trying to create events on remote calendars while offline have been taken, but most of the work will happen in a second patch that creates a command dispatcher that is shared between lightning and sunbird. This will also take care of some problems we've had with the "delete selected events" being enabled when all of them are on readonly calendars.

The calISyncCalendar is only for tagging. The interface itself contains only methods and attributes of calICalendar, but if the object can be QI'd to calISyncCalendar, then the methods described in calICalendar can be handled as synchronous, i.e it is guaranteed that the item has been added when returning from the calICalendar::addItem call.

The storage provider was changed a bit to speed up changes for caching. The previously mentioned "relaxedMode" means that it is possible to call addItem with an item whose id is already present in the database. In this case the item is removed first. Also for modifyItem(), it is possible to call it with aOldItem = null. In this case the old item is looked up using the ID of the new item.

calCachedCalendar currently has a Problem that the item is added twice: It happens once in the observer and once in the onOperationComplete listener. Also, calendars that are local but still have caching enabled (I don't know if thats a valid use-case?) will fail in calCachedCalendars add/modify/delete methods, since it displays an assertion when the ioservice is offline and the call is done.

Locally I have introduced a isCalendarWritable helper, that checks if the calendar is not readonly and if we are offline also if the calendar is local or remote. This function can be used by the UI to better check if it is possible to create an event.
(In reply to comment #8)
> + * Tagging interface for synchronously working providers, e.g. storage.
> I don't understand the comment above. I also don't understand why the interface
> is empty. (maybe those two problems are related :) )
I hope this is explained in my above comment?

> Why do you leave that thoe the calendar? I guess that the code would be the
> same for all calendar. So why not make it seperate code? (yes, you can include
> code in all providers, but that is not nearly as nive. For starters, it doesn't
> work if the proveder is written in something else then javascript)
I don't think this is the same for each calendar. Some providers may use e-tags, others may need to revert to last-modified, others may check the datestamp on certain local files to see if changes were made.

> That's not true. You want to be able to know what changed locally and what
> changed remotely (while offline) to figure out how to merge the local and
> remote
> ics file. So you do need a changelog for ics too.
I hope this is explained in my above comment?


> 
> Index: calendar/base/src/calCalendarManager.js
> 
>      unregisterCalendar: function(calendar) {
> +        // calendar may be a calICalendar wrapper:
> +        if (calendar.wrappedJSObject instanceof calCachedCalendar) {
> +            calendar.wrappedJSObject.onCalendarUnregistering();
> +        }
> 
> You can't use .wrappedJSObject here. You don't know if the provider is
> implemented in javascript or some other language.
calCachedCalendar and the calendar manger are pretty much depend on each other. Since we didn't create a separate interface for calCachedCalendar, I think its perfectly valid to use wrappedJSObject here. If we decide to implement calCachedCalendar in C++, then we need to move to a separate interface anyway. using wrappedJSObject is only to make sure calendar is not passed as a wrapped object.

> +calWcapCalendar.prototype.syncChangesTo_ =
> +function calWcapCalendar_syncChangesTo_(destCal, itemFilter, dtFrom_,
> listener)
> 
> You know what I think about leading underscores: don't do it! It doesn't mean
> anything, it's not documented (afaik) and just makes it harder to read. For the
> parameter, use the common a prefix (aDtFrom) to prevent name clashing. For
> the function, a common style is to prefix 'Internal': cWC_syncChangesToInternal
Although I agree and don't like underscores myself, I'd say since this is wcap-provider-land, the choice of style goes to daniel :)

(In reply to comment #9)
> The calISyncCalendar is only for tagging. The interface itself contains only
> methods and attributes of calICalendar, but if the object can be QI'd to
> calISyncCalendar, then the methods described in calICalendar can be handled as
> synchronous, i.e it is guaranteed that the item has been added when returning
> from the calICalendar::addItem call.

I don't like the use of an empty interface. Just use a flag on the interface, or some property. There are already ways to do that. And besides, you don't seem to use the interface anywhere in the code... Also, why would you need to know if the calendar is sync or not? I don't see the need for that. Sure, writing async code causes a bit more headaches, but it's not impossible.

> The storage provider was changed a bit to speed up changes for caching. The
> previously mentioned "relaxedMode" means that it is possible to call addItem
> with an item whose id is already present in the database.

This should be documented somewhere (just like all the other properties. Maybe we should remove the whole properties idea, and move to real attributes that are documented in the idl. The properties are now abused to be a mixed bag of undocumented mess)
(In reply to comment #10)
> I don't think this is the same for each calendar. Some providers may use
> e-tags, others may need to revert to last-modified, others may check the
> datestamp on certain local files to see if changes were made.

That's the part that keeps a log of the remote changes. But you also want a log of local changes. And that's the same for all providers. But I think you indeed want to separate those two.

> calCachedCalendar and the calendar manger are pretty much depend on each other.

Why is that? In stead of doing new calCachedCalendar, you can use a component, just like we do for all the other types of providers.


> Although I agree and don't like underscores myself, I'd say since this is
> wcap-provider-land, the choice of style goes to daniel :)

There are a few other palces where underscores are used, for example in calendar-manager.js.
(In reply to comment #11)
> I don't like the use of an empty interface. Just use a flag on the interface,
> or some property. There are already ways to do that. And besides, you don't
> seem to use the interface anywhere in the code... Also, why would you need to
I disagree. Tagging/marker interfaces are a well-known design principle to express object capabilities and restrictions and are preferable in this case.

> know if the calendar is sync or not? I don't see the need for that. Sure,
> writing async code causes a bit more headaches, but it's not impossible.
Implementing that safely puts a lot of burden on provider implementors. I don't see a current need for that since storage provider actually runs synchronously.

> This should be documented somewhere (just like all the other properties. Maybe
> we should remove the whole properties idea, and move to real attributes that
> are documented in the idl. The properties are now abused to be a mixed bag of
> undocumented mess)
Take a breath, mvl. It's not sensible to specify everything in IDL since providers are diverse and share only a small common denominator. BTW: The common properties are already documented at calICalendar::getProperty.

(In reply to comment #12)
> > calCachedCalendar and the calendar manger are pretty much depend on each other.
> 
> Why is that? In stead of doing new calCachedCalendar, you can use a component,
> just like we do for all the other types of providers.
There's simply no immediate need to do so. It's very easy to decouple that once the code has calmed down.
(In reply to comment #13)
> > know if the calendar is sync or not? I don't see the need for that. Sure,
> > writing async code causes a bit more headaches, but it's not impossible.
> Implementing that safely puts a lot of burden on provider implementors. I don't
> see a current need for that since storage provider actually runs synchronously.

Where does the current patch assume that the provider is sync? I tried to read the code of calWcapCalendar_syncChangesTo_, but all the closures and functions made my head spin. Is the assumption somewhere in there?
I'm trying to understand why you would need a sync calendar. All you do (as this is read-only sync) is apply remote changes to the local calendar. You don't need to get any items. So what must be sync?
The assumption is passed via IDL: calIChangeLog::replayChangesOn passes a calISyncCalendar. We need to know whether an error has occurred when replaying the changes, else it's invalid to shift the sync token (time stamp). A sync addItem/modifyItem/deleteItem helps a lot.
Blocks: 410287
Attachment #294750 - Attachment is obsolete: true
Attachment #296516 - Flags: ui-review?(christian.jansen)
Attachment #294750 - Flags: review?(philipp)
Attachment #296517 - Flags: ui-review?(christian.jansen)
Comment on attachment 296516 [details]
sunbird pinstripe offline icon

r=christian
Attachment #296516 - Flags: ui-review?(christian.jansen) → ui-review+
No longer blocks: 402325
Attachment #296517 - Flags: ui-review?(christian.jansen) → ui-review+
Comment on attachment 296517 [details]
sunbird winstripe offline icon

r=christian
Attached patch patchSplinter Review
updated patch
Attachment #296991 - Flags: review?(philipp)
Comment on attachment 296991 [details] [diff] [review]
patch

>+            // temporary hack unless all group scheduling features are supported
>+            // by the caching facade (calCachedCalendar):
>+            var provider = calendar.getProperty("private.wcapCalendar")
>+                                   .QueryInterface(Components.interfaces.calIWcapCalendar);
Can you create a bug to un-hack this?

>+/**
>+ * Calendar implementing this interface have improved means of replaying their
>+ * changelog data. This could for example mean, that the provider can retrieve
>+ * changes between now and the last sync.
>+ *
>+ * Not implementing this interface is perfectly valid for calendars, that need
>+ * to do a full sync each time anyway (i.e ics)
>+ */

>+        // xxx todo open problem:
>+        // how do we get rid of calendars for which caching has been switched off, then unregistered?
...
               // TODO put changes into a different calendar and delete
>+                // afterwards.
Bug this

>+                var calType = getPrefSafe("calendar.cache.type", "storage");
add to prefs.js

>+                    // xxx todo: this works, because we govern our own cache.sqlite
>+                    uri.spec += ("?id=" + this.id);
This isn't todo, is it? Sounds like a valid solution to me.

>+    onOfflineStatusChanged: function cCC_onOfflineStatusChanged(aNewState) {
>+        if (aNewState) {
>+            // Going offline: (XXX get items before going offline?) => we may ask the user to stay online a bit longer
Bug this if applicable

>+                    var supported = cal.getProperty("cache.supported");
>+                    if ((supported || supported === null) && ) {
>+                        cal = new calCachedCalendar(cal);
>+                    }
if (supported !== false && cal.getProperty("cache.enabled"))

>+function getCalendarDirectory() {
>+    if (getCalendarDirectory.mDir === undefined) {
>+        var dirSvc = Components.classes["@mozilla.org/file/directory_service;1"]
>+                               .getService(Components.interfaces.nsIProperties);
>+        var dir = dirSvc.get("ProfD", Components.interfaces.nsILocalFile);
>+        dir.append("calendar-data");
>+        if (!dir.exists()) {
>+            try {
>+                dir.create(Components.interfaces.nsIFile.DIRECTORY_TYPE, 0700);
>+            } catch (exc) {
>+                ASSERT(false, exc);
>+                throw exc;
>+            }
>+        }
>+        getCalendarDirectory.mDir = dir;
>+    }
>+    return getCalendarDirectory.mDir.clone();
>+}
What happened to calendar.data.directory? I was about to write "what happens if calendar.data.directory changes" ;)

>+# offline support strings (SUNBIRD ONLY)
>+offlineTooltip=You are currently offline.
>+onlineTooltip=You are currently online.
We should create a bug to move sunbird only locale strings into calendar/locales/sunbird or such.

>+                    // xxx todo: work around value types here unless we save into the prefs...
If this is temporary, can't we just put this into get/setCalendarPref, always converting "true" to true and "false" to false?

>+            this.mDeleteEventExtras[i].reset();
Good catch!

>+    get name calWcapCalendar_nameGetter() {
Do you want to change this for all wcap [sg]etters? I used something like cGC_getName / cGC_setName for google, maybe we should put something like this into the style guide.

>     // Setup the command controller
>     injectCalendarCommandController();
>+    // Setup the command controller
>+    injectCalendarCommandController();
Change unneeded, lines already there

>+if (!calendarOfflineManager) {
>+    Components.utils.reportError("calendar-management.js not included!");
>+}
I was thinking, maybe we should write a function require() that checks if certain files were included, along with something like initFile()

>+        ioService.manageOfflineStatus = getPrefSafe("offline.autoDetect", true);
Add to prefs if not already there?


r=philipp, lets get this show on the road!
Attachment #296991 - Flags: review?(philipp) → review+
(In reply to comment #21)
> >+# offline support strings (SUNBIRD ONLY)
> >+offlineTooltip=You are currently offline.
> >+onlineTooltip=You are currently online.
> We should create a bug to move sunbird only locale strings into
> calendar/locales/sunbird or such.

This is bug 395567.
Small nit: I would prefer a file name that shows it's relation to Sunbird/Lightning, e.g. "calendarcache.sqlite" instead of just "cache.sqlite".
(In reply to comment #21)
.QueryInterface(Components.interfaces.calIWcapCalendar);
> Can you create a bug to un-hack this?
will be done with bug 370150

> If this is temporary, can't we just put this into get/setCalendarPref, always
> converting "true" to true and "false" to false?
may lead to problems e.g. saving a calendar name "true"

> >+    get name calWcapCalendar_nameGetter() {
> Do you want to change this for all wcap [sg]etters? I used something like
> cGC_getName / cGC_setName for google, maybe we should put something like this
> into the style guide.
I'll do that when passing code like that (but in another bug). Putting that into the style guide makes sense.

(In reply to comment #23)
> Small nit: I would prefer a file name that shows it's relation to
> Sunbird/Lightning, e.g. "calendarcache.sqlite" instead of just "cache.sqlite".
The cache.sqlite file resides in <profile-dir>/calendar-data/. IMO that's enough scoping.

Checked in on HEAD and MOZILLA_1_8_BRANCH.
-> FIXED
No longer blocks: 380060
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Verified with Lightning 2008012900 and Mozilla/5.0 (Windows; U; Windows NT 5.1;
pl; rv:1.8.1.12pre) Gecko/20080127 Calendar/0.8pre
Status: RESOLVED → VERIFIED
Blocks: 412817
You need to log in before you can comment on or make changes to this bug.