Can't create cached calendars right away

RESOLVED FIXED in 1.0b3

Status

defect
RESOLVED FIXED
11 years ago
8 years ago

People

(Reporter: lmarcotte, Assigned: nomisvai)

Tracking

unspecified
1.0b3
Bug Flags:
blocking-calendar1.0 +

Details

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

Attachments

(2 attachments)

Reporter

Description

11 years ago
User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.4) 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["@mozilla.org/calendar/calendar;1?type=" + type]
                                      .createInstance(Components.interfaces.calICalendar);
             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:
1.
2.
3.
I think this makes sense, Daniel, do you see any other possibilities this could be solved?
Status: UNCONFIRMED → NEW
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]
Reporter

Comment 3

10 years ago
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
Reporter

Comment 5

10 years ago
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

Updated

9 years ago
Assignee: nobody → simon.at.orcl
Status: NEW → ASSIGNED
Assignee

Comment 7

9 years ago
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 <simon.at.orcl@gmail.com
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+
Assignee

Comment 9

9 years ago
pushed to comm-central : http://hg.mozilla.org/comm-central/rev/eaf205fc21fa
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.0b3
Whiteboard: [not needed beta][has l10n impact] → [needed beta][has l10n impact]

Comment 11

8 years ago
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

r=philipp
Attachment #508556 - Flags: review?(philipp) → review+
Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/f5ea2c120b5e>
-> FIXED
Target Milestone: 1.0b3 → Trunk
Backported to comm-1.9.2 <http://hg.mozilla.org/releases/comm-1.9.2/rev/494c7a79cf1d>
a=philipp
Target Milestone: Trunk → 1.0b3
You need to log in before you can comment on or make changes to this bug.