310503 - readonly calendar support

Status: RESOLVED FIXED

(Calendar :: Internal Components, defect)

Description

[Dan Mosedale (:dmosedale, :dmose)]

We need to have a readonly mode for calendars.  The most important use case is
for when Sunbird detects that it can't read an item into an ICS calendar for
some reason, meaning that if the user were to change anything in the calendar
and cause it to be written, the unreadable thing would be lost.  So the app
should prompt the user saying something along the lines of "had an error reading
your calendar; unless you uncheck this box, your calendar will be put into
read-only mode to avoid dataloss."  At some point in the future, we could
actually support attempts at recovery.  

Another use case would be for online calendars that you don't have permissions
to write (eg bug 243919).

I suspect this wants to be an IDL attribute on calICalendar.  Perhaps the
front-end should just take care of setting it if it gets back errors when it
thinks it shouldn't (eg in onLoad callbacks).

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

The attr is already on the interface; jminta is working on a patch to implement
and use it.

Comment 2

[Joey Minta]

Attachment: implement readonly

This implements readonly support for calendars.  Each calendar now gets an
observer (defined in calCalendarManager) that will alert the user if that
particular calendar goes bad.  The readOnly attribute can be changed in the
calendarProperties dialog.  Only the ics calendar, at this point, can actually
handle being set as readOnly=true.  In that case, it throws an error if you
attempt to modify an event in the calendar.  In order to prevent this from
being thrown too often, the event dialog no longer adds readOnly calendars to
the list of calendars for an event.  Furthermore, opening an edit-dialog of an
event in a readOnly calendar disables the OK button.

Comment 3

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

Comment on attachment 198842 [details] [diff] [review]
implement readonly

You need to add readOnly setters for all the providers. (it isn't a readonly
attribute)

How much work would it be to implement readonly support for the other providers
(like storage?) If you don't want to do this in this bug, please do file a new
one.

Comment 4

[Joey Minta]

Attachment: implement readonly v2

Updated to reflect mvl's comment.  Now all calendar provider types implement
setting readOnly.  Note that ICS is still the only one that makes any attempt
at setting it automatically.

Comment 5

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

Comment on attachment 199173 [details] [diff] [review]
implement readonly v2

>Index: mozilla/calendar/providers/memory/calMemoryCalendar.js

>+    set readOnly(bool) {
>+        getCalendarManager().setCalendarPref(this, "READONLY", bool);
>+    },},

ehm, those brackets don't look like you want them to look.

r=mvl

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 199173 [details] [diff] [review]
implement readonly v2

Looks good; thanks for the patch!  r=dmose with a few nits fixed and a couple
of bugs filed to sort out some architectural issues later.

>Index: mozilla/calendar/base/content/calendar-event-dialog.js
>===================================================================
>RCS file: /cvsroot/mozilla/calendar/base/content/calendar-event-dialog.js,v
>retrieving revision 1.24
>diff -p -U8 -r1.24 calendar-event-dialog.js
>--- mozilla/calendar/base/content/calendar-event-dialog.js	15 Aug 2005 21:49:44 -0000	1.24
>+++ mozilla/calendar/base/content/calendar-event-dialog.js	11 Oct 2005 16:21:22 -0000
>@@ -45,16 +45,19 @@ function onLoad()
>     window.mode = args.mode;
>     window.recurrenceInfo = null;
> 
>     /* add calendars to the calendar menulist */
>     var calendarList = document.getElementById("item-calendar");
>     var calendars = getCalendarManager().getCalendars({});
>     for (i in calendars) {
>         var calendar = calendars[i];
>+        if (calendar.readOnly && !window.calendarItem.calendar.uri.equals(calendar.uri)) {
>+            continue;
>+        }

A comment here say what the if is doing would be nice.

>+    // When a calendar fails, it's onError doesn't point back to the calendar.

"its" 

>+    // Therefore, we add a new announcer for each calendar to tell the user that
>+    // a specific calendar has failed.  The calendar itself is responsible for
>+    // putting itself in readonly mode.

I think we would do way better here to just add a "calendar" parameter to all
the observer callbacks: fewer objects, and will be useful in other cases too. 
Since you've already got something working, let's go ahead with this for now,
but please file a bug to tweak the observers in the future.

>+
>+// This is a prototype object for announcing the fact that a calendar error has
>+// happened and that the calendar has therefore been put in readOnly mode.  We
>+// implement a new one of these for each calendar registered to the calmgr.
>+function readOnlyAnnouncer(calendar) {
>+    this.calendar = calendar;
>+    var savedthis = this;

Can you pick a variable name that more obviously describes the object in
question?  "savedthis" doesn't really mean anything without figuring out the
context it was saved in.

>+    this.observer = {
>+        QueryInterface: function(aIID) {
>+            if (!aIID.equals(Components.interfaces.calIObserver) &&
>+                !aIID.equals(Components.interfaces.nsISupports)) {
>+                throw Components.results.NS_ERROR_NO_INTERFACE;
>+            }
>+            return this;
>+        },
>+        onStartBatch: function() {},
>+        onEndBatch: function() {},
>+        onLoad: function() {},
>+        onAddItem: function(aItem) {},
>+        onModifyItem: function(aNewItem, aOldItem) {},
>+        onDeleteItem: function(aDeletedItem) {},
>+        onAlarm: function(aAlarmItem) {},
>+        onError: function(aErrNo, aMessage) {
>+            if (!savedthis.calendar.readOnly)
>+                return;
>+            savedthis.announceError();
>+        }
>+    }
>+}

We may want a slightly different message if someone is using a calendar that is
readonly anyway (eg subscribed to a HTTP calendar with no PUT permissions). 
Possibly onError() also wants as a parameter "setToReadonly" in order to make
it possible to distinguish these cases.  This is probably good fodder for the
bug mentioned above.

>+readOnlyAnnouncer.prototype.announceError = function(aErrNo) {
>+    var promptService = Components.classes[
>+                        "@mozilla.org/embedcomp/prompt-service;1"]
>+                        .getService(Components.interfaces.nsIPromptService);
>+    var sbs = Components.classes["@mozilla.org/intl/stringbundle;1"]
>+                    .getService(Components.interfaces.nsIStringBundleService);
>+    var props = sbs.createBundle("chrome://calendar/locale/calendar.properties");
>+    var emessage = props.formatStringFromName("readOnlyMode", [this.calendar.name],1);
>+    promptService.alert(null, 'Warning', emessage);
>+}

Having this not be parented seems wrong to me.	My suspicion is that the code
that calls the promptservice doesn't want to live inside the calendar manager
at all, but wants to be one layer further out:	I could imagine someone wanting
to do something like SunTray or some sync code using calCalendarManager, and
the UI for that code is likely to require different handling of such an error. 
Please add this to the "future" bug.

>Index: mozilla/calendar/providers/caldav/calDavCalendar.js
>===================================================================
>RCS file: /cvsroot/mozilla/calendar/providers/caldav/calDavCalendar.js,v
>retrieving revision 1.46
>diff -p -U8 -r1.46 calDavCalendar.js
>--- mozilla/calendar/providers/caldav/calDavCalendar.js	14 Sep 2005 22:11:01 -0000	1.46
>+++ mozilla/calendar/providers/caldav/calDavCalendar.js	11 Oct 2005 16:21:26 -0000
>@@ -168,16 +175,18 @@ calDavCalendar.prototype = {
>     // void removeObserver( in calIObserver observer );
>     removeObserver: function (aObserver) {
>         this.mObservers = this.mObservers.filter(
>             function(o) {return (o != aObserver);});
>     },
> 
>     // void addItem( in calIItemBase aItem, in calIOperationListener aListener );
>     addItem: function (aItem, aListener) {
>+        if (this.readOnly) 
>+            throw Components.interfaces.calIErrors.CAL_IS_READONLY;

For (at least) the CalDAV changes, please use braces around then clauses.

>+readOnlyMode=There has been an error reading data for calendar: %1$S. It has been placed in read-only mode, since changes to this calendar will likely result in data-loss.  You may edit the calendar to change this.

Instead of "You may edit the calendar to change this.", how about "Repairing
this is likely to require either fixing the calendar file using a text editor
or else filing a bug against Mozilla Calendar."?

Comment 7

[Joey Minta]

Attachment: better UI

This is the same as before, except it's gone through a bunch of testing and as
a result, some of the UI has changed.  Instead of not adding calendars to the
menulist in the event-dialog (User:"Where'd my calendar go?"), we display a
warning and disable the OK button if a read-only calendar is selected.	Also,
we display a message explaining why an event can't be edited, if it's in a
read-only calendar.  (Also incorporates dmose's last review comments.)

Comment 8

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

Comment on attachment 199477 [details] [diff] [review]
better UI

Looks good. r=mvl

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 199477 [details] [diff] [review]
better UI

>+// This is a prototype object for announcing the fact that a calendar error has
>+// happened and that the calendar has therefore been put in readOnly mode.  We
>+// implement a new one of these for each calendar registered to the calmgr.
>+function readOnlyAnnouncer(calendar) {
>+    this.calendar = calendar;
>+    var announcer = this;
>+    this.observer = {
>+        QueryInterface: function(aIID) {
>+            if (!aIID.equals(Components.interfaces.calIObserver) &&
>+                !aIID.equals(Components.interfaces.nsISupports)) {
>+                throw Components.results.NS_ERROR_NO_INTERFACE;
>+            }
>+            return this;
>+        },

In theory, at least, you shouldn't need to implement QI by hand here; XPConnect
should do it for you since only one interface is supported.

>Index: mozilla/calendar/providers/caldav/calDavCalendar.js
>===================================================================
>RCS file: /cvsroot/mozilla/calendar/providers/caldav/calDavCalendar.js,v
>retrieving revision 1.46
>diff -p -U8 -r1.46 calDavCalendar.js
>--- mozilla/calendar/providers/caldav/calDavCalendar.js	14 Sep 2005 22:11:01 -0000	1.46
>+++ mozilla/calendar/providers/caldav/calDavCalendar.js	13 Oct 2005 22:52:07 -0000
>@@ -168,16 +175,18 @@ calDavCalendar.prototype = {
>     // void removeObserver( in calIObserver observer );
>     removeObserver: function (aObserver) {
>         this.mObservers = this.mObservers.filter(
>             function(o) {return (o != aObserver);});
>     },
> 
>     // void addItem( in calIItemBase aItem, in calIOperationListener aListener );
>     addItem: function (aItem, aListener) {
>+        if (this.readOnly) 
>+            throw Components.interfaces.calIErrors.CAL_IS_READONLY;

Please brace all "then" clauses that you add, at least in calDavCalendar.js.

r=dmose with the above changes + filed bugs as described in the previous
iteration

Comment 10

[Joey Minta]

Patch checked in.

Checking in mozilla/calendar/base/content/calendar-event-dialog.js;
/cvsroot/mozilla/calendar/base/content/calendar-event-dialog.js,v  <-- 
calendar-event-dialog.js
new revision: 1.25; previous revision: 1.24
done
Checking in mozilla/calendar/base/content/calendar-event-dialog.xul;
/cvsroot/mozilla/calendar/base/content/calendar-event-dialog.xul,v  <-- 
calendar-event-dialog.xul
new revision: 1.15; previous revision: 1.14
done
Checking in mozilla/calendar/base/public/calIErrors.idl;
/cvsroot/mozilla/calendar/base/public/calIErrors.idl,v  <--  calIErrors.idl
new revision: 1.4; previous revision: 1.3
done
Checking in mozilla/calendar/base/src/calCalendarManager.js;
/cvsroot/mozilla/calendar/base/src/calCalendarManager.js,v  <-- 
calCalendarManager.js
new revision: 1.21; previous revision: 1.20
done
Checking in mozilla/calendar/providers/caldav/calDavCalendar.js;
/cvsroot/mozilla/calendar/providers/caldav/calDavCalendar.js,v  <-- 
calDavCalendar.js
new revision: 1.47; previous revision: 1.46
done
Checking in mozilla/calendar/providers/composite/calCompositeCalendar.js;
/cvsroot/mozilla/calendar/providers/composite/calCompositeCalendar.js,v  <-- 
calCompositeCalendar.js
new revision: 1.15; previous revision: 1.14
done
Checking in mozilla/calendar/providers/ics/calICSCalendar.js;
/cvsroot/mozilla/calendar/providers/ics/calICSCalendar.js,v  <--  calICSCalendar.js
new revision: 1.30; previous revision: 1.29
done
Checking in mozilla/calendar/providers/memory/calMemoryCalendar.js;
/cvsroot/mozilla/calendar/providers/memory/calMemoryCalendar.js,v  <-- 
calMemoryCalendar.js
new revision: 1.37; previous revision: 1.36
done
Checking in mozilla/calendar/providers/storage/calStorageCalendar.js;
/cvsroot/mozilla/calendar/providers/storage/calStorageCalendar.js,v  <-- 
calStorageCalendar.js
new revision: 1.68; previous revision: 1.67
done
Checking in mozilla/calendar/resources/content/calendarProperties.js;
/cvsroot/mozilla/calendar/resources/content/calendarProperties.js,v  <-- 
calendarProperties.js
new revision: 1.4; previous revision: 1.3
done
Checking in mozilla/calendar/resources/content/calendarProperties.xul;
/cvsroot/mozilla/calendar/resources/content/calendarProperties.xul,v  <-- 
calendarProperties.xul
new revision: 1.5; previous revision: 1.4
done
Checking in mozilla/calendar/resources/content/eventDialog.js;
/cvsroot/mozilla/calendar/resources/content/eventDialog.js,v  <--  eventDialog.js
new revision: 1.173; previous revision: 1.172
done
Checking in mozilla/calendar/resources/content/eventDialog.xul;
/cvsroot/mozilla/calendar/resources/content/eventDialog.xul,v  <--  eventDialog.xul
new revision: 1.99; previous revision: 1.98
done
Checking in mozilla/calendar/resources/locale/en-US/calendar.dtd;
/cvsroot/mozilla/calendar/resources/locale/en-US/calendar.dtd,v  <--  calendar.dtd
new revision: 1.113; previous revision: 1.112
done
Checking in mozilla/calendar/resources/locale/en-US/calendar.properties;
/cvsroot/mozilla/calendar/resources/locale/en-US/calendar.properties,v  <-- 
calendar.properties
new revision: 1.40; previous revision: 1.39
done

Comment 11

[Robin Edrenius]

*** Bug 243919 has been marked as a duplicate of this bug. ***