Closed
Bug 341776
Opened 18 years ago
Closed 17 years ago
calICalendar::superCalendar attribute for calendar composition
Categories
(Calendar :: Internal Components, enhancement)
Calendar
Internal Components
Tracking
(Not tracked)
RESOLVED
FIXED
0.8
People
(Reporter: dbo, Assigned: Fallen)
References
Details
Attachments
(1 file, 1 obsolete file)
55.89 KB,
patch
|
dbo
:
review+
|
Details | Diff | Splinter Review |
Currently the ICS provider caches its items using a memory calendar to speed-up reading. The memory calendar has a (private) property called "calendarToReturn", so all items it posts to listeners point to the (logically) correct ICS provider. Generalizing this concept, I propose to add an attribute calICalendar::superCalendar.
Reporter | ||
Comment 1•18 years ago
|
||
Assignee: base → daniel.boelzle
Status: NEW → ASSIGNED
Attachment #225879 -
Flags: first-review?(dmose)
Comment 2•18 years ago
|
||
Chances are reasonable that once we move to the calICachingCalendar architecture, the ICS calendar will no longer have an inner memory calendar. That aside, what is useful about exposing this via public interface, rather than leaving it as a private implementation detail?
Comment 3•18 years ago
|
||
Ah, I see. This is essentially a calICachingCalendar piece.
Reporter | ||
Comment 4•18 years ago
|
||
Dan, I have (prototypically) implemented a caching mechanism for the WCAP stuff to reduce network traffic, sync'ing in changes into a local storage calendar. Similar to the ICS provider, two calendars have to logically act as one. So I thought it would be time to generalize this. So far, the sync'ing works, but I am experiencing major performance problems when my local storage cal gets fat (getItems() then takes 10 seconds or more); I am going to profile this soon. Because, it is only a prototype, this issue does not really block issue 340949.
Comment 5•18 years ago
|
||
Comment on attachment 225879 [details] [diff] [review] adding superCalendar attribute I only found this patch just now... I like the idea, but unfortunately, the patch doesn't apply anymore. Quite a few conflicts. We need this patch for caching etc, I think. >+ /** >+ * Multiple calendar instances may be composited, logically acting as a >+ * single calendar, e.g. for caching puorposing. >+ * This attribute determines the super calendar instance that all provided >+ * items belong to, ensuring calls enter the super instance first. >+ * @see calIItemBase::calendar >+ */ >+ attribute calICalendar superCalendar; The comment could use some more explanation about what this implies for implementations of the interface. You should mention here and/or at getItem(s) that the calendar returned must be the superCalendar (if set). Also, comment about what the attribute will be when it is not set. r-, because it is bitrotten too much.
Attachment #225879 -
Flags: first-review?(dmose) → first-review-
Reporter | ||
Comment 6•18 years ago
|
||
(In reply to comment #5) Before I create a new patch, we should first come to a conclusion whether this approach (and the name superCalendar) is ok.
Comment 7•18 years ago
|
||
Sorry for not getting back to you on this much sooner. This seems like a reasonable strategy. Is there really anything other than caching that we might use this for? If not, I'd be inclined to suggest calling it cachedCalendar and having any providers that we don't intend to test caching with (e.g. everything but memory and storage, I suspect) throw NS_ERROR_NOT_IMPLEMENTED for the getters and setters.
Comment 8•18 years ago
|
||
I don't really like the suggestion of naming it 'cachedCalendar'. There might be other, not caching related, cases where you want to use this attribute. So I vote for the more general 'superCalendar'.
Assignee | ||
Updated•17 years ago
|
Assignee | ||
Comment 9•17 years ago
|
||
Unbitrotted patch, but depends on the patch in bug 404085 being applied.
Attachment #225879 -
Attachment is obsolete: true
Attachment #289182 -
Flags: review?(daniel.boelzle)
Reporter | ||
Comment 10•17 years ago
|
||
Comment on attachment 289182 [details] [diff] [review] adding superCalendar attribute - v2 >+ // attribute calICalendar superCalendar; >+ get superCalendar() { >+ // If we have a superCalendar, check this calendar for a superCalendar. >+ // This will make sure the topmost calendar is returned >+ return this.mSuperCalendar && this.mSuperCalendar.superCalendar || this; >+ }, Please rewrite the above like if (this.mSuperCalendar) { return this.mSuperCalendar.superCalendar; } return this; for me ;) r=dbo
Attachment #289182 -
Flags: review?(daniel.boelzle) → review+
Assignee | ||
Comment 11•17 years ago
|
||
Checked in on HEAD and MOZILLA_1_8_BRANCH -> FIXED
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Target Milestone: --- → 0.8
You need to log in
before you can comment on or make changes to this bug.
Description
•