325641 - implement per-calendar id

Status: VERIFIED FIXED

(Calendar :: Internal Components, defect)

Description

[Dan Mosedale (:dmosedale, :dmose)]

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.

Comment 1

[Joey Minta]

Attachment: stop using uri in calmgr

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?

Comment 2

[Matthew (lilmatt) Willis]

Not going to make the 0.3 train.

Comment 3

[Michiel van Leeuwen (email: mvl+moz@)]

I'd like to re-try to get this for 0.5, because having an id would make life for extension authors easier.

Comment 4

[Matthew (lilmatt) Willis]

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.

Comment 6

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: store uuid

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.

Comment 7

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: nicer patch

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)

Comment 8

[Joey Minta]

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.

Comment 9

[Michiel van Leeuwen (email: mvl+moz@)]

(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.

Comment 10

[Joey Minta]

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?

Comment 11

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: complete patch

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)

Comment 12

[Joey Minta]

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.

Comment 13

[Michiel van Leeuwen (email: mvl+moz@)]

patch checked in (with comments fixed)