Closed Bug 325641 Opened 14 years ago Closed 13 years ago

implement per-calendar id

Categories

(Calendar :: Internal Components, defect)

defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED
Lightning 0.5

People

(Reporter: dmose, Assigned: mvl)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

Calendar code has been using URIs (which can change or be duplicated) and interface pointers (which, among other things, have wrapping problems as per bug 325636) to test for calendar object identity.  We need an easy way to test for identity in the near-term.

My current theory involves having the ID for registered calendars be the (positive integer) key used by the calendar manager into it's databases, and have the calendar hand out negative integers for use by the provider implementors as identifiers for non-registered calendars.
Blocks: 325650
This patch is a possible first step towards divorcing uri and id.  I'm posting it here for feedback on the approach and because I'm not sure the best way to proceed from here.  To make uri's editable we can either:
a) Make uri be just another calendar pref or
b) Keep uri special and add a modifyURI function to calICalendarManager.

I'm inclined to opt for (b) because prefs can be deleted, but deleting a uri would would cause most providers to fail for null URIs and because the migration code for making uri be a pref will suck.  Opinions?
Not going to make the 0.3 train.
Target Milestone: Lightning 0.3 → Lightning 0.5
I'd like to re-try to get this for 0.5, because having an id would make life for extension authors easier.
Flags: blocking-calendar0.5+
I guess I'm not sure why we don't simply want to use a UUID for id and identify calendars that way.

Since they're URL-safe, I think the code change would be minimal.
Duplicate of this bug: 367862
Attached patch store uuid (obsolete) — Splinter Review
My attempt at this. I wanted to add a uuid to each calendar, as an attribute. That part wasn't really hard. But storing it so the calendarManager could set proved to be less nice. I tried to get away with not updating the storage schema, but the result is a bit of a hack.
Attaching the patch so i won't loose it, and other might use it. But i don't think it's ready for review.
Attached patch nicer patch (obsolete) — Splinter Review
This patch is nicer. Instead of foring the calendar.id to be an uuid, it's just the id we already had. I added a way to get a nice uuid string, if you need it for cosmetical reasons (I want it for storing a copy of calendar for offline)
Attachment #254968 - Attachment is obsolete: true
Attachment #255134 - Flags: first-review?(jminta)
Comment on attachment 255134 [details] [diff] [review]
nicer patch

Index: base/public/calICalendar.idl
+   * unique ID of this calendar
+   */
+  attribute AUTF8String id;
I'm a bit concerned that this attribute is read/write, but that we don't serialize it.  That at least needs to be made clear in the comments, if not fixed in the architecture. Can you add warning to uri, that it's not promised to be unique, since a lot of people have that false belief?


Index: base/public/calICalendarManager.idl
+  /**
+   * Get an uuid for a calendar. If you only need to check if two calendar
+   * are the same, use the calendars id attribute. If you need a more magic
+   * string, use this uuid
+   */
+  AUTF8String getCalendarUuid(in calICalendar aCalendar);
I'm not sure why this needs to be an independent method.  Since you're using the calendar-preferences structure, why can't UUID just become another known pref that calendars have?

Index: base/src/calCalendarManager.js
+function getUUID() {
This is a straight copy from calUtils.js.  Why not just use that file?

     findCalendarID: function(calendar) {
-        var stmt = this.mFindCalendar;
-        stmt.reset();
-        var pp = stmt.params;
-        pp.type = calendar.type;
-        pp.uri = calendar.uri.spec;
-
-        var id = -1;
-        if (stmt.step()) {
-            id = stmt.row.id;
-        }
-        stmt.reset();
-        return id;
+        return calendar.id;
We should just fix callers of findCalendarId to use calendar.id.  I'd be ok with doing that in a follow-up though.

+    getCalendarUuid: function cCM_getCalendarUuid(aCalendar) {
+        var uuid;
Same comment as above.  I'm also inclined to put this burden on people who care about it (ics-cache), rather than on the manager.  Since a uuid is supposed to be universally unique itself, having this function centralized doesn't add anything.
Assignee: dmose → mvl
(In reply to comment #8)
> (From update of attachment 255134 [details] [diff] [review])
> Index: base/public/calICalendar.idl
> +   * unique ID of this calendar
> +   */
> +  attribute AUTF8String id;
> I'm a bit concerned that this attribute is read/write, but that we don't
> serialize it.

It must be writeable, because there is no other way for the calendar manager to set the ID. (if only idl's had some constructor like thing) I will add some comments.


> +  AUTF8String getCalendarUuid(in calICalendar aCalendar);
> I'm not sure why this needs to be an independent method.  Since you're using
> the calendar-preferences structure, why can't UUID just become another known
> pref that calendars have?

To me, that feels like leaking an implementation detail. There are no known pref names at the moment.

> Index: base/src/calCalendarManager.js
> +function getUUID() {
> This is a straight copy from calUtils.js.  Why not just use that file?

That file didn't work when i wrote the patch. It does now. Will update.
Whiteboard: [patch in hand][needs review jminta]
I talked with mvl about this, and as I understand it, the UUID stuff is going to move to the offline interfaces, so a new patch should be coming.

(In reply to comment #9)
> It must be writeable, because there is no other way for the calendar manager to
> set the ID. (if only idl's had some constructor like thing) I will add some
> comments.
Can we at least throw NS_ERROR_ALREADY_INITIALIZED (or some similarly named error if that doesn't exist) if someone tries to set it a second time?
Whiteboard: [patch in hand][needs review jminta]
Attached patch complete patchSplinter Review
Patch updated to review comments. Now includes the id attribute on all calendars. (maybe, one day, we find a way to share stuff like that)
Attachment #255134 - Attachment is obsolete: true
Attachment #257549 - Flags: first-review?(jminta)
Attachment #255134 - Flags: first-review?(jminta)
Comment on attachment 257549 [details] [diff] [review]
complete patch

+   * unique ID of this calendar.
Capitalize 'unique'. 

+  
+  /**
+   * Get an uuid for a calendar. If you only need to check if two calendar
+   * are the same, use the calendars id attribute. If you need a more magic
+   * string, use this uuid
+   */
+  AUTF8String getCalendarUuid(in calICalendar aCalendar);
 
I thought we agreed to removed this?  It's not implemented anywhere anymore.

 Otherwise, I like this a lot.  Many of the places where we have var calendarID = calendar.id now seem silly to me, but those can probably be cleaned up later.
Attachment #257549 - Flags: first-review?(jminta) → first-review+
patch checked in (with comments fixed)
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
Flags: blocking-calendar0.5+
You need to log in before you can comment on or make changes to this bug.