380060 - Implementation of full offline mode (esp. modification of calendar data)

Status: RESOLVED FIXED

(Calendar :: General, enhancement)

Description

[etc]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.3) Gecko/20070309 Firefox/2.0.0.3
Build Identifier: offline manager Progress 

This bug is to follow the progress of the offline support project.

Reproducible: Always

Comment 1

[etc]

Attachment: calendar manager file (include OffLineManager and ChangeList implemetations)

Include the OffLineManager and ChangeList's implemetations

Comment 2

[etc]

Attachment: The compatible ICS version

Even trying for all means not to change any provider code, I wasn't able to do it, but there are only a few small changes, so is not that bad.

Comment 3

[etc]

Attachment: The campatible Memory Version

I think I only add a line of code here.

Comment 4

[Daniel Boelzle [:dbo]]

@etc/Antonio: having a look at your changes, I would rather prefer some patch files.

Comment 5

[etc]

Attachment: The ICS patch

Comment 6

[Daniel Boelzle [:dbo]]

BTW: there is an existing parent/superCalendar calendar enh bug 341776.

Comment 7

[etc]

Attachment: the ics patch

Comment 8

[etc]

Attachment: full patch

Comment 9

[etc]

The general ideas of what I code are at: http://wiki.mozilla.org/User_talk:TonyCuba, 
There is still work to do, but I want to know if I am in the correct path

Comment 10

[etc]

Attachment: full patch

Comment 11

[etc]

Attachment: full patch

Minimizing changes to the provider

Comment 12

[Brad F]

I would like to verify that this bug will take care of being able to view a remote .ics calendar while offline.  Will it also allow changes to be made to the offline copy to be synced back to the remote calendar when Sunbird goes back online?  If that is not the purpose of this bug, I'll file a different one.  If it is, thanks for your work on this bug!

Comment 13

[etc]

(In reply to comment #12)
The work done in the patch already allow you to view, modify, etc with the ics remote calendar, the changes made in the copy will go to the server.
The purpose of this bug is to show the work in progress to give complete offline support to the calendar, so probably must of the futures you need will be include.

Comment 14

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

Comment on attachment 264369 [details] [diff] [review]
full patch

>Index: base/src/calCalendarManager.js

>+const nsISupports = Components.interfaces.nsISupports;
>+const CI = Components.interfaces;
>+
>+const CLASS_ID = Components.ID("679f15e5-4743-43b3-ae9b-8c6702330823");
>+const CLASS_NAME = "for offline support";
>+const CONTRACT_ID = "@antonio.blanca/offlinemanager;1";

CLASS_ID is a generic name, while you use it for a specific class, in a file with multiple classes.
I think that you should move the offline class to it's own file. And you don't need this manual registration code. There is a file that takes care of all that: calItemModule.js

>+//aki empieza el offline manager
I don't speak italian :)

>+function OfflineManager(uri)
>+{
>+    this.wrappedJSObject = this;
>+  	this.mLocalCalendar = Components.classes["@mozilla.org/calendar/calendar;1?type=storage"].createInstance(Components.interfaces.calICalendar);

Don't use tabs. Use 4 space indents instead. (goes for all over you patch.)

>+  	this.mUri = uri;
>+  	this.mLocalCalendar.id = 3;
That's a magic number. It might be calendar 3 in your setup, but not in mine. And what if I have more calendars that I want to use offline?

>+  	var a = this;

Use a better name then 'a' please. Some places use 'savedthis'. Still not really good, but better.

>+	timer.initWithCallback(Callback, 8000, timer.TYPE_ONE_SHOT);

8000 is a magic number. Define it as a constant somewhere. Or better, make it a pref.
Actually, why do you need this timer at all?

>+    setProvider: function(aCalendar)
>+    {
>+		this.mProvider = aCalendar;
>+		try{this.mProvider.wrappedJSObject.setParent(this);}catch(e){dump(e.toString()+'\n');}
That line can be made more readable. Add some spaces, newlines and all that.

>+  	getItems: function (aItemFilter, aCount,aRangeStart, aRangeEnd, aListener)
>+  	{
>+		return this.mProvider.getItems(aItemFilter, aCount, aRangeStart, aRangeEnd, aListener);

So, how does this work when you are offline? Most providers can't get any items while offline. ics happens to be able to do that, but that's not a general rule. I think this is the big problem in the offline story: How to get items when you are offline? How do you cache them?

I'm not sure if it makes sense to me to review the rest of the patch while i still have this big question. So i'll wait with that for now.

>+        this.mObserver.acceptableErrorNums.push(4009);
>+        this.mObserver.acceptableErrorNums.push(4010);

But i'd like to point at this. You want to create a real (named) error code. This magic number doesn't really work. (4009 isn't really a error code!).
Look at calIErrors.idl.


>+            var timer = Components.classes["@mozilla.org/timer;1"].createInstance(Components.interfaces.nsITimer);
>+            timer.initWithCallback(aCallback, 9000, timer.TYPE_ONE_SHOT);

You use timers like this all over the place. Why?

>+    setParent: function(aCalendar)
>+    {
>+        this.parent = aCalendar;
>+    },

You should split those changes (about parents) to a different bug. see comment 6.

>Index: providers/memory/calMemoryCalendar.js
>+        aNewItem.calendar = aOldItem.calendar;

The old event should already be in this calendar. How else could you modify it? So, how could .calendar not be set right?

Comment 15

[etc]

(In reply to comment #14)
Well as I said and you confirm this code should be fixed, but this won't be such a hard work, must of the things you told are not that big deal except for this:

"Most providers can't get any items while offline. ics happens to be able to do that, but that's not a general rule. I think this is the big problem in the offline story: How to get items when you are offline? How do you cache them?" 

I didn't know this. First I think there is no provider where having a memory or sotarge calendar working as a kind of buffer do not optimize the ammonut of readings to the server working online. But if this is unchangeable for any reason, to make this idea work, we will have to add a memory calendar to offline manager, that at the start it gets the items from the server or the storage calendar if you on or off line, it would be like a local copy of your calendar in the server. This will allow me to get items and cache them.

Let me know what you think.

Comment 16

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

Part of the changes needed to make offline work is caching the calendar data. There must be a good strategy for doing that. It's not trivial to do that for all providers. (but on the other hand, we can live with offline support only for some providers for now).
In your patch, you do seem to try to fill a local storage calendar, but I don't see how you use it afterwards. I think you should query that instead of the ics provider when offline.

But here comes another thing: it looks wasteful to me to parse an ics file into memory, and then copy that into a local storage. I think that (for ics) it should be enough to only store the data in a local storage file.
At the moment, that might be bad for performance. The memory provider is often faster then storage. That is something we need to fix.

Comment 17

[Bill Burgess]

From our (user) perspective it is really important -- "critical" might be a better word -- for us to have read-only access to a cached calendar copy when a user is offline due to travel, or because of a power failure at the server.  So thank you!

On the other hand, the ability to make offline changes that are later sync'ed (including conflict resolution) when the connection is restored is not that important to us.  This would be a rare case, and could be got around (in our case anyway) by asking someone who was online to make the change on the traveler's behalf.
 
FYI, Apple's iCal appears to cache four files associated with each read-only calendar subscription:  "corestorage.ics" (apparently a local duplicate of the remote .ics file), "Index" (a binary file), "Info.plist" (text key into filename structure), and "Properties.plist" (binary archive that includes address of master file and refresh data).

Comment 21

[Bill Burgess]

Our calendar server crashed with a bad system disk last week, and has been down since.  All our SubBird users have lost access to company calenders.  We are a very small business and do not have dedicated staff to jump on this, so the calendars will be unavailable for several days.

I realize this enhancement (in our present environment it seems more like a bug) is unassigned, and wish I had time to work on it myself.  Perhaps someone reading this would consider running with it.  In the meantime I am unfortunately compelled to look for alternatives to Sunbird, and would welcome any recommendations (by email, not in bugzilla!!).

Comment 23

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

I will be working on this for 0.8, any help is appreciated though. If you would like to help coding, please feel free to contact me per email or on irc (Fallen).

Comment 25

[Simon Paquet [:sipaq]]

This blocks 0.8.

Comment 26

[Daniel Boelzle [:dbo]]

We will implement step 1 of offline support with bug 393395.

Comment 29

[Bas van den Bosch]

(In reply to comment #11)
> Created an attachment (id=264369) [details]
> full patch

@etc, offline viewing has been in for some time now, did you manage to get the code for modifying calendars too?

Comment 30

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

No, I have not had time and priority to take care of full offline mode, sorry.

Comment 32

[Darlene Ford]

How do I control whether my calendar is in the new off-line mode?  I couldn't find much about it in the docs...  Does the user need to configure this, or can Sunbird go into off-line mode by itself if network access is temporarily lost? Is there a straightforward way to verify that Sunbird is currently functioning in on-line or off-line mode?

Comment 34

[Jeffrey Smith]

Just wanted to add my vote for this feature request. Would improve Thunderbird and Sunbird and be a HUGE gain for the PortableApps versions. I hope you find the time.

Comment 35

[Karel de Grootte]

Caching mode works well for only one calendar in Lightning. Caching another one results in that all events are then also in all calendars (not in the remote one.)
So it seems, when caching more than one calendar, it adds the events to all offline calendars, but not to the online server version.
Does it cache it to only one offline cached calender? Is this something that could be fixed?

Comment 36

[Stefan Sitter [:ssitter]]

Reijers, this is a known issue. See Lightning 1.0b1 release notes and Bug 479867.

Comment 39

[[e-mail]]

Hi, sorry for my bad english. i'm a idea but i don't know is good.

pre: when i'm online the edit work great

when sunbird is offline you don't memorize the change in a file. when sunbird go back online you upload the modify by read file (same a log file).

what you think?

Comment 40

[pumuchel]

is there any progress for the offline editing or is it planed to work on it? This lack of offline editing stops using me lightning and i would love to use it.

Comment 41

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Offline editing depends on a number of other things. Most importantly, someone with time to work on it :-)

Comment 42

[Karel de Grootte]

I would take it in small steps. Editing is not the first thing, but reading your calendar(s) offline would be a great first step. (As I'm not a programmer, I have no idea if that is a small step)

And you are right Philipp, we need someone with the right programming skills and some time at hand.

Comment 43

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Mohit will be working on this bug as a part of the 2011 Google Summer of Code. I am looking forward to his work!

Comment 44

[Mohit Kanwal [:redDragon]]

Hi everyone,

I am working on this feature for the summer alongwith Philipp, Wolfgang and Ludovic. 

The roadmap of the bug is up at https://wiki.mozilla.org/Lightning_Offline_Sync. You can discuss about it on the wiki discussion page and provide valuable feedback.

Hope to have an awesome summer :-)

Mohit

Comment 45

[Mohit Kanwal [:redDragon]]

Attachment: Diff file for offline mode

Attached is the first patch for implementation of full offline mode for Calendar. Along with it is also included implementation of offline-like handling capabilities when the  remote calendar is getting server unavailability error (HTTP status code 50x). It has been tested duly.

Comments?

Comment 46

[Ludovic Hirlimann [:Usul]]

Would be nice to have a build for users to test ...

Comment 47

[Mohit Kanwal [:redDragon]]

Here is a test build of lightning which has the offline mode capability.

http://www.2shared.com/file/0H41Dfjx/lightning.html

Comment 48

[Wolfgang Sourdeau]

(In reply to comment #45)

> [snip] Along with it is also included implementation of offline-like
> handling capabilities when the  remote calendar is getting server
> unavailability error (HTTP status code 50x). It has been tested duly.

Just for clarification, I would add that the 50x handling has been added only to the caldav provider.

Comment 49

[Mohit Kanwal [:redDragon]]

Attachment: Offline Mode Patch

Patch for the offline mode, some changes wrt to handling of 50x errors in caldav addressed.

Comment 50

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 264369 [details] [diff] [review]
full patch

This patch doesn't need feedback, its the previous patch. I'll look at the new patch now.

Comment 51

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 546786 [details] [diff] [review]
Offline Mode Patch

Review of attachment 546786 [details] [diff] [review]:
-----------------------------------------------------------------

Hey Mohit, here's a first pass at your patch. I'll do the code review first and then do some testing in a second pass. Please note that unless otherwise noted, the comments about code style are optional. I don't want to pester you with such minor nits. After all, the goal is to get things working and available for everyone!

A note on the review flags. review- does not mean that you are going in the wrong direction. I think this patch is very well-considered and going in the right direction. review+ is reserved for "its ok to check in now", so I'm going to set review- to get a new patch with the comments fixed. In the meanwhile I'll do some testing.

Keep up the good work!

::: calendar/base/content/calendar-common-sets.js
@@ +571,5 @@
> +    get has_cached_calendars() {
> +        var calMgr = getCalendarManager();
> +        var calendars = calMgr.getCalendars({});
> +        for (var i = 0; i < calendars.length; i++) {
> +            if (calendars[i].getProperty("cache.enabled")) {

You can use for each (let calendar in calendars) here, this way you don't need the extra i variable.

::: calendar/base/public/calIChangeLog.idl
@@ +152,5 @@
>  interface calIChangeLog : nsISupports {
>      /**
> +     * Enable the changelog calendar to retrieve offline data right after instanciation.
> +     */
> +    void setOfflineStorage(in calISyncWriteCalendar aStorage);

This makes sense as an attribute, i.e

attribute calISyncWriteCalendar offlineStorage;

@@ +186,5 @@
> +     * corresponding addItem() invocation.
> +     */
> +    calIOperation addItemOrUseCache(in calIItemBase aItem,
> +                                    in boolean useCache,
> +                                    in calIOperationListener aListener);

So these methods are in calIChangeLog, iirc that means that every calendar that wants to implement a changelog based synchronization needs to implement these methods.

I'm not through all of the patch yet, but I don't know if thats what we want. If we can't find a good solution around this issue then we can keep it this way for now with a followup bug though.

::: calendar/base/src/calCachedCalendar.js
@@ +129,5 @@
> +            }
> +        };
> +        this.mReplayTimer = Components.classes["@mozilla.org/timer;1"]
> +                                      .createInstance(Components.interfaces.nsITimer);
> +        this.mReplayTimer.initWithCallback(timerCallback,

The replay timer was previously removed in favor of refreshing the cached calendars using a per-calendar preference. Please see bug 456208 for details.

@@ +351,5 @@
> +                }
> +            },
> +            onOperationComplete: function(calendar, status, opType, id, detail) {
> +                if (this_.offline) {
> +                    LOG("[calCachedCalendar] back to offline mode, reconciliation aborted");

Just a minor nit, but "reconciliaton" might be hard to understand for some non-native speakers. I'd suggest the word "playback" instead.

@@ +371,5 @@
> +            }
> +        };
> +
> +        this.mCachedCalendar.getItems(calICalendar.ITEM_FILTER_ALL_ITEMS | calICalendar.ITEM_FILTER_OFFLINE_CREATED, //calICalendar.ITEM_FILTER_TYPE_ALL does not include calICalendar.ITEM_FILTER_COMPLETED_ALL,
> +                                      0, null, null, getListener);

To reduce line length you could use an extra variable for the filter here.

@@ +382,5 @@
> +            onGetResult: function(calendar, status, itemType, detail, count, items) {
> +            },
> +            onOperationComplete: function(calendar, status, opType, id, detail) {
> +            }
> +        };

This is used more than once, to avoid creating objects each time you could make this global and call it something like gNoOpListener.

@@ +407,5 @@
> +
> +            onGetResult: function(calendar, status, itemType, detail, count, items) {
> +                for each (var aItem in items) {
> +                    this.items.push(aItem);
> +                }

this.items = this.items.concat(items);

@@ +517,5 @@
> +        }
> +        else {
> +            /* we first ensure that any remaining offline items are reconciled with the calendar server */
> +            let this_ = this;
> +            this.reconcileAddedItems(function() { this_.downstreamRefresh(); });

You can use the new js functiom bind here:

this.reconcileAddedItems(this.downstreamRefresh.bind(this));

::: calendar/providers/caldav/calDavCalendar.js
@@ +188,5 @@
>      // calIChangeLog interface
> +    setOfflineStorage: function caldav_setOfflineStorage(storage) {
> +        this.mOfflineStorage = storage;
> +        this.fetchCachedMetaData();
> +    },

If you change this to an attribute, the lines are:

get offlineStorage() {
  return this.mOfflineStorage;
},
set offlineStorage(storage) {
  this.mOfflineStorage = storage;
  this.fetchCachedMetaData();
},

@@ +251,5 @@
> +            } else {
> +                cal.LOG("CAlDAV: cannot store meta data without an id");
> +            }
> +        } else {
> +            cal.LOG("CalDAV: calendar storage does not support meta data");

These log messages should probably rather be cal.ERROR() or even handled.

@@ +756,5 @@
>              } else if (status == 412) {
>                  thisCalendar.promptOverwrite(CALDAV_MODIFY_ITEM, aNewItem,
>                                               aListener, aOldItem);
> +            } else if ((status >= 500 && status <= 510) && (useCache && thisCalendar.mOfflineStorage)) {
> +                LOG("[calDavCalendar] doModifyItemOrUseCache received status code of server unavailibity. Putting entry in cache for later try.");

Make sure you are using cal. as a prefix for all LOG/ASSERT/etc statements.

::: calendar/providers/storage/calStorageCalendar.js
@@ +510,4 @@
>              throw Components.results.NS_ERROR_INVALID_ARG;
>          }
>  
> +        let this_ = this;

Good work on squashing those "var" statements! :)

@@ +824,5 @@
>                  sp.end_offset = aRangeEnd ? aRangeEnd.timezoneOffset * USECS_PER_SECOND : 0;
> +                sp.offline_journal = null;
> +                if (wantOfflineDeletedItems) sp.offline_journal = "d";
> +                else if (wantOfflineCreatedItems) sp.offline_journal = "c";
> +                else if (wantOfflineModifiedItems) sp.offline_journal = "m";

These character flags would be perfect in some constants!

@@ +950,5 @@
> +                    aListener.onOperationComplete(this_, Components.results.NS_OK,
> +                                                   Components.interfaces.calIOperationListener.GET, aItem.id, flag);
> +                }
> +            };
> +            if(isEvent(aItem)){

cal.isEvent/cal.isToDo

@@ +968,5 @@
> +        if(isEvent(aItem) && flag != null){
> +            this.prepareStatement(this.mEditEventOfflineFlag);
> +            this.mEditEventOfflineFlag.params.id = aID;
> +            this.mEditEventOfflineFlag.params.offline_journal = flag;
> +            this.mEditEventOfflineFlag.execute();

When executing sql statements, its very important to wrap them in a try { statement.execute() } catch (e) { ... } finally { statement.reset(); } block, I believe also when using execute().

@@ +1500,5 @@
>          }
>      },
>  
> +    mRecEventCacheOfflineFlags: {},
> +    mRecTodoCacheOfflineFlags : {},

If you set these objects here, you are setting them on the prototype. This means they will be shared for all instances of the storage calendar. Is that what you want? If not, then you need to make these null and set them in the constructor.

Comment 52

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Pretty cool, from simple testing it works without flaws!

Comment 53

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

You may be aware of this, but I tested an ics/webdav calendar in cached mode and got the following error:

Error: [Exception... "'[JavaScript Error: "this_.mUncachedCalendar.modifyItemOrUseCache is not a function" {file: "resource://calendar/modules/calUtils.jsm -> file:///Users/kewisch/mozilla/comm-central/obj-x86_64-apple-darwin10.6.0/mozilla/dist/Shredder.app/Contents/MacOS/extensions/%7Be2fda1a4-762b-4020-b5ad-a41df1933103%7D/calendar-js/calCachedCalendar.js" line: 423}]' when calling method: [calIOperationListener::onOperationComplete]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: resource://calendar/modules/calProviderUtils.jsm :: cPB_notifyOperationComplete :: line 658"  data: yes]

The ics provider doesn't implement calIChangeLog, so it seems to be bailing out somewhere.

Comment 54

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Another thing to keep in mind: when offline and the calendar is not readonly, with the cache you allow editing the item. When you go online, you attempt to write back the items. What if the server denies writing to the calendar? From what I hear you see this as a temporary outage and try again later.

But what if the calendar is really readonly? This might give the user the feeling that writing succeeds, but the items are never really written to the calendar.

Let me know what you think.

Comment 55

[Wolfgang Sourdeau]

(In reply to comment #53)

Hi Philipp,

Mohit is supposed to have talked to you about it (iirc), but I think it's reasonable to impose the cached calendars to respond to the calIChangeLog interface, as it would not really make sense otherwise. The same goes for the offline interface. This answers one of your concerns above with regards to the calIChangeLog interface and this answers the bug you currently have with cached ICS calendars, as they do not conform to the calIChangeLog interface....

Comment 56

[Wolfgang Sourdeau]

> But what if the calendar is really readonly? This might give the user the
> feeling that writing succeeds, but the items are never really written to the
> calendar.
> 

It the calendar is readonly or has disappeared, we will receive a 40x error code (probably a 403). Here, the offline code in the caldav provider stores the modifications only when a 50x is received, i.e. when the server is unavailable (network link down, server process down)

Comment 57

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(In reply to comment #56)
> It the calendar is readonly or has disappeared, we will receive a 40x error
> code (probably a 403). Here, the offline code in the caldav provider stores
> the modifications only when a 50x is received, i.e. when the server is
> unavailable (network link down, server process down)
Perfect, good thinking!



(In reply to comment #55)
> Mohit is supposed to have talked to you about it (iirc), but I think it's
> reasonable to impose the cached calendars to respond to the calIChangeLog
> interface, as it would not really make sense otherwise. The same goes for
I agree that the cached calendars need to implement calIChangeLog and if its mandatory that these functions need to be implemented then its fine with me. I was just thinking maybe there is a way to generalize that code so far that these functions don't need to be implemented in the provider but somewhere else. If thats not the case, then thats ok too.

> the offline interface. This answers one of your concerns above with regards
> to the calIChangeLog interface and this answers the bug you currently have
> with cached ICS calendars, as they do not conform to the calIChangeLog
> interface....
Right, but if possible then calendars that do not provider their own mechanism for synchronization (like ics/webdav) should still be able to use the cache. The standard methods (getItems/...) could be used and although this requires a full sync, it will make the hurdle for new providers lower. What do you think?

Comment 58

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(In reply to comment #56)
> > But what if the calendar is really readonly? This might give the user the
> > feeling that writing succeeds, but the items are never really written to the
> > calendar.
> > 
> 
> It the calendar is readonly or has disappeared, we will receive a 40x error
> code (probably a 403). Here, the offline code in the caldav provider stores
> the modifications only when a 50x is received, i.e. when the server is
> unavailable (network link down, server process down)
Hmm but you receive the 40x when you are online again, so the user can happily change the calendar while offline, but the items get thrown away when going back online?

Comment 59

[Mohit Kanwal [:redDragon]]

(In reply to comment #58)
> (In reply to comment #56)
> > > But what if the calendar is really readonly? This might give the user the
> > > feeling that writing succeeds, but the items are never really written to the
> > > calendar.
> > > 
> > 
> > It the calendar is readonly or has disappeared, we will receive a 40x error
> > code (probably a 403). Here, the offline code in the caldav provider stores
> > the modifications only when a 50x is received, i.e. when the server is
> > unavailable (network link down, server process down)
> Hmm but you receive the 40x when you are online again, so the user can
> happily change the calendar while offline, but the items get thrown away
> when going back online?

Currently, if there is a write error in the online mode (due to the remote calendar being read-only) the user is notified via an error dialog message. However, the entries are not discarded, they still remain, in the cache with their offline flags intact. At the next refresh they will again be attempted to be reconciled.

Comment 60

[Mohit Kanwal [:redDragon]]

(In reply to comment #57)
> Right, but if possible then calendars that do not provider their own
> mechanism for synchronization (like ics/webdav) should still be able to use
> the cache. The standard methods (getItems/...) could be used and although
> this requires a full sync, it will make the hurdle for new providers lower.
> What do you think?
Yes, its definitely possible to do that, the current architecture allows other providers to also share the offline syncing features of cache. But we thought that the offline syncing capability and use of the cache should go together, otherwise we can use standard functions like addItem() for ics/wcap etc but it might not be scalable.

Comment 61

[Mohit Kanwal [:redDragon]]

Attachment: Offline_Patch[version-2]

Fixed styling and other necessary issues pointed by Philipp in the last review.

As discussed with Wolfgang, it makes sense for the provider to be responsible for implementing the synchronization mechanism itself, since provider sync-actions can differ greatly. As such, I have not made changes to fix the exception raised when reading/writing to ics calendars in the offline mode. To fully allow ics provider to enjoy the benefits of sync through calCachedCalendar, it would need to implement the relevant calIChangeLog methods, addItemOrUseCache, modifyItemOrUseCache and so on...

Comment 62

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 554288 [details] [diff] [review]
Offline_Patch[version-2]

Review of attachment 554288 [details] [diff] [review]:
-----------------------------------------------------------------

So if the cache should no longer be supported for ICS until the provider itself implements caching methods, then new problems arise:

* We need to migrate previous ICS calendars that were using the cache to no longer use the cache, which might cause confusion
* The user shouldn't get the option to enable the cache for those providers/calendars that don't support it.

I think these issues are sufficiently complex that it may be better to provide a default caching strategy for calendars that don't support an accellerated caching functionality, as we have been doing before. Even if we implement the caching methods for the ics provider, we will have the same issues for any other providers that have allowed caching in the past.

Maybe a different approach, what do you think about this: How easy is it to allow readonly offline caching for all providers as we have been before and only allowing read-write offline support for those providers that support calIOfflineStorage? This way we don't need to do any risky migration code and we keep the offline-writable property for providers that can do real synchonization for now?

On the other hand I can imagine its hard for you to manage if Wolfgang and I disagree. We should set up a conf call or email discussion to resolve this issue.

Setting r- for now since we have a few open issues to discuss. If you have any questions let me know. Keep up the good work!

::: calendar/base/src/calCachedCalendar.js
@@ +298,5 @@
> +    gNoOpListener : null,
> +
> +    playbackAddedItems: function cCC_playbackAddedItems(callbackFunc) {
> +        let this_ = this;
> +        this.gNoOpListener = {

Sorry I may have not expressed myself enough. I meant global to the file, so add this at the top like:

var gNoOpListener = {
...
};

::: calendar/providers/caldav/calDavCalendar.js
@@ -364,4 @@
>      mQueuedQueries: null,
>  
>      mCtag: null,
> -    mOldCtag: null,

Why are you removing the old ctag thing? I believe this was added in case there are errors after the ctag was retrieved but before the data was successfully read.

@@ +1408,4 @@
>      },
>  
>      refresh: function caldav_refresh() {
> +        this.replayChangesOn(null);

What happens if the caldav calendar is uncached? Does replayChangesOn handle this case?

::: calendar/providers/caldav/calDavRequestHandlers.js
@@ +157,4 @@
>                      onOperationComplete: function etags_getItem_onOperationComplete() {}
>                  };
>  
> +                this.calendar.mOfflineStorage.getItem(this.calendar.mHrefIndex[path],

Does this work? Isn't this.calendar an xpcom object that needs to be QI'd to the interface that provides the offlineStorage attribute?

::: calendar/providers/storage/calStorageCalendar.js
@@ -2214,4 @@
>              this.mDeleteTodo(aID, this.id);
>              this.mDeleteAttachments(aID, this.id);
>              this.mDeleteRelations(aID, this.id);
> -            this.mDeleteMetaData(aID, this.id);

I may have asked, but why not remove metadata for this calendar when it is deleted? (I may also have the wrong context in mind, I wish the review thing would let me show more context lines)

Comment 63

[Mohit Kanwal [:redDragon]]

Hi Philipp,

The issue is with addItemOrUseCache methods right?

What if I add an additional check of checking for whether the provider implements calIOfflineStorage ? I would invoke addItem  if the provider does not implement calIOfflineStorage and would invoke addItemOrUseCache only if the provider does implement it. The logic behind introducing addItemOrUseCache was to handle related to 50x server errors. So it would be easy to insert that check in calCachedCalendar and forward it to caldav or ics accordingly...

What do you say?

Comment 64

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

I'm not sure if I understand correctly, but in the best case this is exactly what I meant with the end of comment 57. 

Are you saying that when going back online and the offline transactions are replayed, then addItems should be called for providers that don't support calIOfflineStorage? This way the items are added/modified normally, but without protection against server errors? If this is correct, couldn't you just keep the item in the offline transaction list until the addItem call returns a success code in its onOperationComplete call?

Comment 65

[Mohit Kanwal [:redDragon]]

In the normal mode, when an event is created it is put into the server, and on success in the cache. 

Yep you are right in saying that items would be modified/added normally except that there is no protection from server related issues in the case of a failure to write to the server, so the user will just be presented with a dialog that writing failed to the server, and the entry gets discarded. There would be no way of forcing an entry in the cache unless the entire application is put into offline mode.

Since the offline cache methods work only if the entire application is in offline mode therefore we can't put the server error handling methods within calCachedCalendar as entries might fail to be written to the server during playback and as such in such a situation they don't need to be overwritten but rather left in cache as it is. 

I can make that modification and upload such a patch but for new providers we need to make sure, that they provide calIOfflineStorage interface from the start.

Comment 66

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(In reply to Mohit Kanwal [:redDragon] from comment #65)
> In the normal mode, when an event is created it is put into the server, and
> on success in the cache. 
> 
> Yep you are right in saying that items would be modified/added normally
> except that there is no protection from server related issues in the case of
> a failure to write to the server, so the user will just be presented with a
> dialog that writing failed to the server, and the entry gets discarded.
> There would be no way of forcing an entry in the cache unless the entire
> application is put into offline mode.
Hmm discarding changes isn't really a good idea, especially in case of a temporary failure.

> 
> Since the offline cache methods work only if the entire application is in
> offline mode therefore we can't put the server error handling methods within
> calCachedCalendar as entries might fail to be written to the server during
> playback and as such in such a situation they don't need to be overwritten
> but rather left in cache as it is.
Don't you get notified on failure? If so you could keep the event in the cache as is on an error?

> 
> I can make that modification and upload such a patch but for new providers
> we need to make sure, that they provide calIOfflineStorage interface from
> the start.
I think we are still having trouble on communication here. If this is an easy change then go ahead, this way I can maybe better see what you mean.

Comment 67

[Mohit Kanwal [:redDragon]]

Attachment: Version 3 of offline patch

Hi Philipp,

I have made the changes you pointed out in the last review, as well as changed the method invocations from calCachedCalendar to first check if the provider implements calIChangeLog, can you help me test it on ics calendars?

Comment 68

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Hmm not sure whats going wrong here. I have a local .ics calendar with a file:// uri. I applied the patch, changed that calendar from uncached to cached, then I get the following messages and errors:

[calCachedCalendar] adding 0 items
[calCachedCalendar] modifying 0 items
[calCachedCalendar] deleting 0 items
[calCachedCalendar] Doing full sync for calendar file:///home/kewisch/mozilla/testcalendars/musicians-birthdays2.ics
Error: DB error: not an error
exc: [Exception... "Failure"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: file:///home/kewisch/mozilla/comm-central/obj-x86_64-unknown-linux-gnu/mozilla/dist/bin/extensions/%7Be2fda1a4-762b-4020-b5ad-a41df1933103%7D/components/calStorageCalendar.js :: cSC_deleteItemById :: line 2437"  data: no]
Error: DB error: not an error
exc: [Exception... "Failure"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: file:///home/kewisch/mozilla/comm-central/obj-x86_64-unknown-linux-gnu/mozilla/dist/bin/extensions/%7Be2fda1a4-762b-4020-b5ad-a41df1933103%7D/components/calStorageCalendar.js :: cSC_deleteItemById :: line 2437"  data: no]
Error: [Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [mozIStorageConnection.rollbackTransaction]"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame :: file:///home/kewisch/mozilla/comm-central/obj-x86_64-unknown-linux-gnu/mozilla/dist/bin/extensions/%7Be2fda1a4-762b-4020-b5ad-a41df1933103%7D/components/calStorageCalendar.js :: cSC_releaseTransaction :: line 2466"  data: no]
Source File: file:///home/kewisch/mozilla/comm-central/obj-x86_64-unknown-linux-gnu/mozilla/dist/bin/extensions/%7Be2fda1a4-762b-4020-b5ad-a41df1933103%7D/components/calStorageCalendar.js
Line: 2466

Any ideas?

Comment 69

[Mohit Kanwal [:redDragon]]

Ok so the ics workers work now? I will test it and make sure it supports the ics backend.

Comment 70

[Mohit Kanwal [:redDragon]]

Attachment: Patch Version 4

This patch file, removes the Memory Calendar addItem assumption, that every new item must not have an ID from before hand. Now ICS and CALDAV providers, both can use the cache for offline entries reconciliation. However, there is an issue. After making some offline entries for the ICS calendar, if I shut down TB and start it again, then the cached etnries are lost, the only error message that I receive in the error console is that "this.channel is null". Is it because the http channel is not being initialized in the start when I playback items to the server? I am not sure how the ICS provider uses both, memory calendar and the cache and keeps them synchronised. At the startup, I believe it throws the cache away, which is causing problems in playing back the items to the server. I would like help in this regard to make this patch good to go for release 1.0 

I am (most of the time) available as [redDragon] on #calendar and #maildev.

Comment 71

[Mohit Kanwal [:redDragon]]

Attachment: Updated Patch with support for ICS

This patch includes functionality of offline support for the ics provider as well.

Comment 72

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Fix - v8

I have gone through a few review iterations with Mohit via IRC and we have come to this solution. I consider this reviewed, r=philipp.

There are a few known hacks in here that we should remove as soon as possible, especially related to offline sync support for providers that don't support calIChangeLog.

Comment 73

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/844b78f8712e>

Comment 74

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Backported to comm-aurora <http://hg.mozilla.org/releases/comm-aurora/rev/c877359b9075>

Comment 75

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Backported to comm-beta <http://hg.mozilla.org/releases/comm-beta/rev/8faa43d205b9>

Comment 76

[Stefan Sitter [:ssitter]]

I vote for backing out from comm-aurora and comm-beta, at least comm-beta. Such a big change has a high potential for regressions and needs proper time for testing. This is not possible on comm-beta.

Comment 77

[Ludovic Hirlimann [:Usul]]

(In reply to Philipp Kewisch [:Fallen] from comment #72)
 
> There are a few known hacks in here that we should remove as soon as
> possible, especially related to offline sync support for providers that
> don't support calIChangeLog.

What are the followup bugs ?

Comment 78

[Mohit Kanwal [:redDragon]]

One of the followups is that memory calendar addItem function does not allow for items to be inserted with a null Id (which maybe a bit hard on the cached entries), also modifyItem requires oldItem be specified, but since using cache causes loss of the old item, and a useless getItems has to be carried out to get the oldItem from the server, which may or may not have changed. This assumption in memory calendar has been modified to suit the offline sync. It has so far not caused any issues.

Also the caldav part has already been in inverse's trunk of lightning, for almost 2 months now, and has not seen any issues related to the offline part.

Comment 79

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

(In reply to Stefan Sitter from comment #76)
> I vote for backing out from comm-aurora and comm-beta, at least comm-beta.
> Such a big change has a high potential for regressions and needs proper time
> for testing. This is not possible on comm-beta.

I should have communicated this more clearly before checking it in, sorry. I know its a risk, but I also think that being able to edit offline items is an important part of a major version like Lightning 1.0. I've done a lot of testing in the past week(s) and I think the worst case right now is that the experimental cache breaks and needs regression fixes. I'm fairly confident that the uncached mode will continue to work or at least have a manageable amount of regressions.

Comment 80

[Ludovic Marcotte]

I support Philipp's comment. The uncached mode, which is the only mode being offered transparently to Lightning users right now should continue to work as is.

Comment 81

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Freebusy fix for cached mode pushed:

comm-central rev a707b9553a99
comm-aurora rev 88d3ceed5fd2
comm-beta rev 4f4bbf63645c

Comment 82

[Cas]

A rather large google calendar with a couple of thousand events does not show any events after turning on the cache. The process thunderbird.exe is spiking for a while, and then nothing. Restarting TB or refreshing calendars does nothing. Error console says: 

----------
Fout: aOperation.operationListener is undefined
Bronbestand: resource://calendar/modules/calUtils.jsm -> file:///C:/Users/TJ/AppData/Roaming/Thunderbird/Profiles/g7j6eyxa.default/extensions/%7Ba62ef8ec-5fdc-40c2-873c-223b8a6925cc%7D/js/calGoogleCalendar.js
Regel: 1258
-----------

I can add and delete events in offline mode in this calendar, but not while being online. Error console:

-------------
Fout: [Exception... "'JavaScript component does not have a method named: "addItemOrUseCache"' when calling method: [calIChangeLog::addItemOrUseCache]"  nsresult: "0x80570030 (NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED)"  location: "JS frame :: resource://calendar/modules/calUtils.jsm -> file:///C:/Users/TJ/AppData/Roaming/Thunderbird/Profiles/g7j6eyxa.default/extensions/%7Be2fda1a4-762b-4020-b5ad-a41df1933103%7D/calendar-js/calCachedCalendar.js :: <TOP_LEVEL> :: line 700"  data: no]
Bronbestand: resource://calendar/modules/calUtils.jsm -> file:///C:/Users/TJ/AppData/Roaming/Thunderbird/Profiles/g7j6eyxa.default/extensions/%7Be2fda1a4-762b-4020-b5ad-a41df1933103%7D/calendar-js/calCachedCalendar.js
Regel: 700

and

Fout: uncaught exception: [Exception... "'JavaScript component does not have a method named: "addItemOrUseCache"' when calling method: [calIChangeLog::addItemOrUseCache]"  nsresult: "0x80570030 (NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED)"  location: "JS frame :: resource://calendar/modules/calUtils.jsm -> file:///C:/Users/TJ/AppData/Roaming/Thunderbird/Profiles/g7j6eyxa.default/extensions/%7Be2fda1a4-762b-4020-b5ad-a41df1933103%7D/calendar-js/calCachedCalendar.js :: <TOP_LEVEL> :: line 700"  data: no]
----------------------


I'm using TB8b3, lightning 1.0rc2, provider 0.9pre, windows 7.

PS: Another smaller (google) calendar works great in offline mode! Adding and deleting events offline and online works as expected.

Comment 83

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

I'm really surpried that the other Google Calendar is working. I'm aware the Provider for Google Calendar doesn't work with the new cache, I'll be posting a patch for this soon.

Comment 84

[Cas]

Thanks Philipp. I did not know provider was not working yet. I'm awaiting the patch.

Ah, sorry, didn't add that the smaller calendar works via caldav.

Comment 85

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

For the gdata provider, see bug 697553