Closed Bug 466686 Opened 12 years ago Closed 11 years ago

Can't create cached calendars right away


(Calendar :: General, defect)

Not set


(Not tracked)



(Reporter: lmarcotte, Assigned: nomisvai)


(Whiteboard: [needed beta][has l10n impact])


(2 files)

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv: Gecko/2008102920 Firefox/3.0.4
Build Identifier: Lightning 0.9

Right now, calCalendarManager.js doesn't allow you to create cached calendars right away.

It would be nice to add a 3rd paramater to createCalendar() so that we can wrap calCachedCalendar around the newly created instance and use the cache right away - such as:

--- src/calendar/base/src/calCalendarManager.js cedc48f10b7d8866120d1735bc08b6866d0df6a0
+++ src/calendar/base/src/calCalendarManager.js 19ba70ce1f8d953dc663d12f97eb973edec46884
@@ -529,11 +529,15 @@ calCalendarManager.prototype = {
      * calICalendarManager interface
-    createCalendar: function cmgr_createCalendar(type, uri) {
+    createCalendar: function cmgr_createCalendar(type, uri, cached) {
         try {
             var calendar = Components.classes[";1?type=" + type]
             calendar.uri = uri;
+           if (cached == true)
+             calendar = new calCachedCalendar(calendar);

This allows, when adding a cached CalDAV calendar, Lightning to populate the cache right away instead of doing it twice (first start and second start).

Reproducible: Always

Steps to Reproduce:
I think this makes sense, Daniel, do you see any other possibilities this could be solved?
Ever confirmed: true
(In reply to comment #0)
> This allows, when adding a cached CalDAV calendar, Lightning to populate the
> cache right away instead of doing it twice (first start and second start).
You think of enhancing the calendar creation wizard?
Whiteboard: [needs patch for code in comment#0]
Should I produce a patch for this? If so, I could modify the calendar creation wizard accordingly.
I'd say as long as the offline cache feature is still experimental and not working reliable it should not be promoted in the New Calendar Wizard dialog.
OS: Mac OS X → All
Hardware: x86 → All
How about if we just add the same warning as in the calendar properties dialog? ie., "Cached (EXPERIMENTAL):   []"
Besides the wizard, I'd appreciate to keep |caching| as a calendar property. We may build up the caching facade on registration (i.e. at the time the calendar instance gets an id), e.g. let registerCalendar return the instance.
Whiteboard: [needs patch for code in comment#0]
Assignee: nobody →
This patch adds a "Cache (Experimental)" option in the wizard dialog and removes the need for restarting Thunderbird every time the cache option is changed on a calendar.

Note that I would vote for removing the "EXPERIMENTAL" next to the cache, we have fixed many issues with it lately and I know many people use it already.
Attachment #440855 - Flags: review?(philipp)
Flags: blocking-calendar1.0+
Whiteboard: [not needed beta][has l10n impact]
Comment on attachment 440855 [details] [diff] [review]
Patch v1 with some usablility improvment

>diff --git a/calendar/base/content/widgets/calendar-list-tree.xml b/calendar/base/content/widgets/calendar-list-tree.xml
>    - Contributor(s):
>+   - Simon Vaillancourt <
Missing >

>         <!--
>           - Add a calendar to the calendar list
>           -
>           - @param aCalendar     The calendar to add.
>           -->
>         <parameter name="aCalendar"/>
>         <body><![CDATA[
>           let composite = this.compositeCalendar;
>+          let initialSortOrderPos = aCalendar.getProperty("initialSortOrderPos");
>+          if (initialSortOrderPos != null && initialSortOrderPos < this.mCalendarList.length) {
>+            // Insert the calendar at the requested sort order position
>+            // and then discard the property
>+            this.mCalendarList.splice(initialSortOrderPos,0,aCalendar);
space after commas.

>+          }
>+          else {
} else { 
here and in other places.

>+                    newCal.setProperty("color",
>+                                       aCalendar.getProperty("color"));
>+                    newCal.setProperty("disabled",
>+                                       aCalendar.getProperty("disabled"));
>+                    newCal.setProperty("cache.enabled",
>+                                       aCalendar.getProperty("cache.enabled"));
>+                    newCal.setProperty("suppressAlarms",
>+                                       aCalendar.getProperty("suppressAlarms"));
>+                    newCal.setProperty("calendar-main-in-composite",
>+                                       aCalendar.getProperty("calendar-main-in-composite"));
>+                    newCal.setProperty("calendar-main-default",
>+                                       aCalendar.getProperty("calendar-main-default"));

This should work in most cases, but what if there are custom calendars that carry more important props. If we have a way to enumerate all properties it might make sense to copy them all.

If not, then I guess we can do it like this, but please add a TODO comment and file a followup bug to add a property enumerator and make use of it here.

Also, it might be cleaner to do something like:
let props = ["color", "disabled", ...];
for each (let prop in props) {
  newCal.setProperty(prop, aCalendar.getProperty(prop));

>diff --git a/calendar/locales/en-US/chrome/calendar/calendar.dtd b/calendar/locales/en-US/chrome/calendar/calendar.dtd
>-<!ENTITY calendarproperties.cache.label                    "Cache (EXPERIMENTAL, requires restart)">
>+<!ENTITY calendarproperties.cache.label                    "Cache (EXPERIMENTAL)">
Sorry this didn't make the string freeze. I hope its ok to postpone this patch for 1.0b3pre ?

>     let cal_name = document.getElementById("calendar-name").value;
>     let cal_color = document.getElementById('calendar-color').color;
Extra newline not needed, also check lines you've changed for trailing whitespaces

r+ with the above fixed, but please postpone the checkin until after the beta.
Attachment #440855 - Flags: review?(philipp) → review+
pushed to comm-central :
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.0b3
Whiteboard: [not needed beta][has l10n impact] → [needed beta][has l10n impact]
With the fix, 'readOnly' calendars become writable and 'imip.identity.key' is lost. Adding these properties to propsToCopy in calCalendarManager.js file would be nice.
Comment on attachment 508556 [details] [diff] [review]
add readOnly and imip.identity.key to the list of copied properties

Attachment #508556 - Flags: review?(philipp) → review+
Pushed to comm-central <>
Target Milestone: 1.0b3 → Trunk
Backported to comm-1.9.2 <>
Target Milestone: Trunk → 1.0b3
You need to log in before you can comment on or make changes to this bug.