Closed
Bug 466686
Opened 16 years ago
Closed 15 years ago
Can't create cached calendars right away
Categories
(Calendar :: General, defect)
Calendar
General
Tracking
(Not tracked)
RESOLVED
FIXED
1.0b3
People
(Reporter: lmarcotte, Assigned: nomisvai)
Details
(Whiteboard: [needed beta][has l10n impact])
Attachments
(2 files)
|
14.70 KB,
patch
|
Fallen
:
review+
|
Details | Diff | Splinter Review |
|
1.34 KB,
patch
|
Fallen
:
review+
|
Details | Diff | Splinter Review |
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.
|
||
Comment 1•16 years ago
|
||
I think this makes sense, Daniel, do you see any other possibilities this could be solved?
Status: UNCONFIRMED → NEW
Ever confirmed: true
|
||
Comment 2•16 years ago
|
||
(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?
|
||
Updated•16 years ago
|
Whiteboard: [needs patch for code in comment#0]
|
|
Reporter | |
Comment 3•16 years ago
|
||
Should I produce a patch for this? If so, I could modify the calendar creation wizard accordingly.
|
||
Comment 4•16 years ago
|
||
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.
|
|
||
Updated•16 years ago
|
OS: Mac OS X → All
Hardware: x86 → All
|
|
Reporter | |
Comment 5•16 years ago
|
||
How about if we just add the same warning as in the calendar properties dialog? ie., "Cached (EXPERIMENTAL): []"
|
|
||
Comment 6•16 years ago
|
||
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.
|
|
||
Updated•16 years ago
|
Whiteboard: [needs patch for code in comment#0]
|
Assignee | |
Updated•15 years ago
|
Assignee: nobody → simon.at.orcl
Status: NEW → ASSIGNED
|
|
Assignee | |
Comment 7•15 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)
|
|
||
Updated•15 years ago
|
Flags: blocking-calendar1.0+
Whiteboard: [not needed beta][has l10n impact]
|
|
||
Comment 8•15 years ago
|
||
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•15 years ago
|
||
pushed to comm-central : http://hg.mozilla.org/comm-central/rev/eaf205fc21fa
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
|
|
||
Updated•15 years ago
|
Target Milestone: --- → 1.0b3
|
|
||
Updated•15 years ago
|
Whiteboard: [not needed beta][has l10n impact] → [needed beta][has l10n impact]
|
|
||
Comment 10•14 years ago
|
||
Backported to comm-1.9.2 <http://hg.mozilla.org/releases/comm-1.9.2/rev/d6dcca9f931a>
a=philipp
|
||
Comment 11•14 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.
|
|
Assignee | |
Comment 12•14 years ago
|
||
Attachment #508556 -
Flags: review?(philipp)
|
|
||
Comment 13•14 years ago
|
||
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+
|
|
||
Comment 14•14 years ago
|
||
Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/f5ea2c120b5e>
-> FIXED
Target Milestone: 1.0b3 → Trunk
|
|
||
Comment 15•14 years ago
|
||
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.
Description
•