196942 - Ability to replicate server events to local for offline viewing

Status: VERIFIED DUPLICATE of bug 380060

(Calendar :: General, enhancement)

Description

[Grant Fengstad]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.4a) Gecko/20030304
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.4a) Gecko/20030304

It would be very useful for future consideration that as the Mozilla Calendar
begins to support servers (ie: ICAL based, Sun One, Sun CDE, etc) that the
client have an ability to replicate and keep a local copy of the calendar data
for offline viewing.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Mike Potter]

Mike and his group are doing work on a general calendar server interface, I
don't know if this applies to them or not.

Comment 2

[Mike Young]

I agree that this would be usefull. I wouldn't like the idea of being able to
edit the offline file, but viewing it would be very helpful. If we let people
edit it, we'd have to deal with merging changes and all that.

Comment 3

[Mike Potter]

New contact from [e-mail] to [e-mail]
Filter on string OttawaMBA to get rid of these messages. 
Sorry for the spam.

Comment 4

[Matthew (lilmatt) Willis]

Reassigning all automatically assigned bugs from Mostafa to nobody@m.o

Bugspam filter: TorontoMostafaMove

Comment 5

[Joey Minta]

This is going to go on the blocking radar as its an 0.2 regression.  We can mark the cal read-only for 0.3, to avoid the sync problems.

Comment 6

[Joey Minta]

Attachment: work in progress

Patch is an untested work-in-progress.  I'm putting it here for feedback on the general architecture I'm using here (re-using backup files).  Opinions?

Comment 7

[Joey Minta]

Attachment: use backup for offline v1

Full implementation of backup-as-offline source.  Kept small for 0.3 eligibility.

Comment 8

[Joey Minta]

Comment on attachment 233691 [details] [diff] [review]
use backup for offline v1

         } catch(e) {
             // File not found: a new calendar. No problem.
+            this.tryUsingBackup();
             this.unlock();
This comment needs to change slightly now.

+            if (!this.mUsingBackup) {
+                this.tryUsingBackup();
+            } else {
+                this.mUsingBackup = false;
+            }
Swap these cases and add a comment.

+        var latestFile = null;
+        while (dirEnum.hasMoreElements()) {
+            var file = dirEnum.getNext().QueryInterface(CI.nsIFile);
+            if (file.isFile()) {
+                if (!latestFile || file.lastModifiedTime > latestFile.lastModifiedTime) {
+                    latestFile = file;
+                }
+            }
+        }
You need to make sure you have the right backup file for this calendar by using the unique id.

Comment 9

[Joey Minta]

Attachment: use backup for offline v2

Patch addresses previous review comments.  The minor tweaks have been incorporated and we're now looking at the id of the file/calendar to make sure they match.

Comment 10

[Joey Minta]

Comment on attachment 234067 [details] [diff] [review]
use backup for offline v2

     locked: false,
 
+    mUsingBackup: false,
+
Comment here explaining what mUsingBackup is.

     get readOnly() { 
-        return this.mReadOnly;
+        return this.mReadOnly || this.mUsingBackup;
Comment here why mUsingBackup matters here.

+            var file = dirEnum.getNext().QueryInterface(CI.nsIFile);
+            if (file.isFile() && file.leafName.indexOf(prefix) != -1) {
+                if (!latestFile || file.lastModifiedTime > latestFile.lastModifiedTime) {
can use substr here instead of indexof.

r=dmose with that.

Comment 11

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

I'm sorry to say, but i'm not happy with this approach.
We already had other plans, with offline data in a storage cal, and using that for sync etc. Why switch to something else now, and use something that was not made for this? Adding this read-only offline code does seem like the way to go. I'm afraid it will bother us later, because we can't just remove the feature.

The other problem is with the user experience. All kind of magic things seem to happen. Suddenly your calendar is read-only, without any message as to why and how to change that to read-write again.

Comment 12

[Joey Minta]

(In reply to comment #11)
> I'm sorry to say, but i'm not happy with this approach.
> We already had other plans, with offline data in a storage cal, and using that
> for sync etc. Why switch to something else now, and use something that was not
> made for this? Adding this read-only offline code does seem like the way to go.
> I'm afraid it will bother us later, because we can't just remove the feature.
We can change the model without removing the feature.  The idea here was to provide something usable for 0.3 with minimal amount of code/review time.  Actually beginning the storage path requires much more planning/architecture that I don't think could be done in time for 0.3.  The patch is quite self-contained, and can easily be removed when something more permanent comes along.

> 
> The other problem is with the user experience. All kind of magic things seem to
> happen. Suddenly your calendar is read-only, without any message as to why and
> how to change that to read-write again.
> 
I think that the user would be generally aware of the fact that they're now offline.  If you really want a prompt to come up, that's certainly possible to do.

Comment 13

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

Comment on attachment 234067 [details] [diff] [review]
use backup for offline v2

>Index: calendar/providers/ics/calICSCalendar.js
>     set uri(aUri) {
>-        if (channel instanceof Components.interfaces.nsIHttpChannel) {
>-            this.mHooks = new httpHooks();

Don't move this code. hooks might keep state, and that likely needs to be kept for each reload. So only construct the hooks once.

>     refresh: function() {
>+        // Give ourselves another chance to download the file correctly
>+        this.mUsingBackup = false;

This really wants more documentation. One block of comments, describing the backup and using the backup would be good.

>         var streamLoader = Components.classes["@mozilla.org/network/stream-loader;1"]
>                                      .createInstance(Components.interfaces.nsIStreamLoader);
>         try {
>             streamLoader.init(channel, this, this);
>         } catch(e) {
>-            // File not found: a new calendar. No problem.
>+            // For a new calendar, this just means file-not-found, which
>+            // isn't a problem.  We won't have any backups either, so its
>+            // not a big deal.  This may, however, mean that an existing
>+            // calendar is not reachable, so we should try to use the backup.

There are better ways to guess at what really happened, like looking at the http error codes. This kind of guessing might lead to problems.

>+            this.tryUsingBackup();

>     /**
>+     * For the case where we previously had a remote calendar, but now we're
>+     * offline, we can use the backup (which was the last copy of the calendar
>+     * that the user saw anyway) to still show the items.  We'll mark this

That's not true. It is the last version before the last change was uploaded. So you always miss one change.
Actually, thinking about it, that might be a source of dataloss. Need to test.

>+     * calendar read-only for now (via mUsingBackup) in order to avoid the whole
>+     * syncing mess.

Syncing is not a mess, it is just not written yet.


>+    tryUsingBackup: function ics_tryBackup() {

>+        try {
>+            var dirService = Components.classes["@mozilla.org/file/directory_service;1"]
>+                                       .getService(CI.nsIProperties);
>+            var backupDir = dirService.get("ProfD", CI.nsILocalFile);
>+            backupDir.append("backupData");
>+            if (!backupDir.exists()) {
>+                return;
>+            }
>+        } catch(e) {
>+            // Backup dir wasn't found. Likely because we are running in
>+            // xpcshell.
>+            return;
>+        }

I smell code duplication!

>+            streamLoader.init(channel, this, this);

Calling the streamloader from a callback of the streamloader makes my head hurt...

Comment 14

[Dan Mosedale (:dmosedale, :dmose)]

This isn't going to make it in by tonight, and the fact that it's one version back as pointed out by mvl is essentially dataloss for the user.  Let's hold off on this until post-0.3.

Comment 15

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

Comment on attachment 234067 [details] [diff] [review]
use backup for offline v2

r-, based on the loss of the last edit.

Comment 16

[carl]

I'm sorry, hope this is not concidered as spam (tried to post in the mozilla calendar-forum, but no luck). But does "post0.3" mean that this bug will not get fixed in the 0.3 version? I can only speak for myself, but this feature is one of the most important why I wait for the 0.3 version.
Thanks anyway for your efforts in improving sunbird/lightning.

Comment 17

[Pooya Karimian]

Is it possible to make this patch an add-on? 

Comment 19

[Maxime Delorme]

see also bug 361955#2

Comment 20

[Bill Burgess]

This is to encourage those working on this enhancement!  I have specified SunBird for our company's Windows platforms to do very simple collaborative calendaring over an internal LAN.  To view the calendars (read-only) I use iCal on my Mac laptop.  No problem for me when the server goes down unexpectedly or I travel, but Windows users lose.  Internal support is miniscule so I can't recommend extra plugins and extra user steps.

Just a report from the user trenches.  Thanks for your great work!  

Comment 21

[etc]

work in progress bug 380060