279189 - New calendar manager APIs

Status: RESOLVED FIXED

(Calendar :: Internal Components, defect)

Description

[Stuart Parmenter]

Need a central place to create/find/register/etc calendars

Comment 1

[Stuart Parmenter]

Attachment: new api/partial impl

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 171934 [details] [diff] [review]
new api/partial impl

>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ base/public/calICalendarManager.idl	20 Jan 2005 23:46:09 -0000
>+  /* return a list of all calendars currently registered */
>+  void getCalendars(out PRUint32 count,
>+                    [array, size_is(out), retval] out calICalendar aCalendars);
>+};

size_is(count)


>Index: base/src/calCalendarManager.js
>+    initDB: function() {
>+        var calendarTable = "id INTEGER PRIMARY KEY, name STRING, type STRING";

need a uri string in there as well, also a params string/blob

>+    registerCalendar: function(calendar) {
>+        this.mDB.executeSimpleSQL("INSERT INTO cal_calendar VALUES(" + calendar.name + ", " + calendar.uri.spec")");

Can't use executeSimpleSQL for any of these; need to create statements and bind
variables.  This will break extremely badly if the name/uri have any 's in
them; as written this will also break very badly since the strings aren't
quoted at all.

>+    },
>+
>+    unregisterCalendar: function(calendar) {
>+        this.mDB.executeSimpleSQL("DELETE FROM cal_calendar WHERE name = " + calendar.name + " AND uri = " + calendar.uri.spec)

Same as above.


The addition of "name" looks fine; should probably land that first, since it
won't affect any code, and can keep the Manager patch smaller/less confusing.

Comment 3

[Stuart Parmenter]

Attachment: Better patch

I know i need to remove the hard coded database path, which i'll do as soon as
vlad lands his new uri stuff for storage.  Aside from that I think it can land
and start being used.

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 172644 [details] [diff] [review]
Better patch

>-[scriptable, uuid(4c352774-2c4f-11d9-9c63-00045ace3b8d)]
>+[scriptable, uuid(2ab0652d-9a66-43db-b481-fe77a53eb4ea)]
> interface calICalendar : nsISupports
> {
>   /**
>+   * Name of the calendar
>+   */
>+  attribute AUTF8String name;

Add comment saying that this should be changed only through the manager
interface, unless the caller knows what they're doing.


>+
>+  /**
>+   * Type of the calendar
>+   *   'memory', 'storage', 'caldav', etc
>+   */
>+  readonly attribute AUTF8String type;

Comment to indicate that this will be used to reconstruct a contract id, maybe.

>Index: base/public/calICalendarManager.idl
>===================================================================

What happened to renameCalendar() and the observer stuff for add/remove/rename?
 Though that can probably come later..


>Index: base/src/calCalendarManager.js
>===================================================================

>+if (kMozStorageStatementWrapperIID) {
>+    const MozStorageStatementWrapper = new Components.Constructor(kMozStorageStatementWrapperContractID, kMozStorageStatementWrapperIID);
>+ } else {
>+    dump("*** mozStorage not available, calendar/storage provider will not function\n");
>+ }

Mmm, more than the storage provider won't function -- like the manager itself. 
I'd just get rid of the check, mozStorage is going to be required for calendar
stuffs.  This could be an argument for storing this in RDF, to remove the
dependency on mozStorage from core calendar stuff, but eh.

>+
>+    initDB: function() {
>+        var calendarTable = "id INTEGER PRIMARY KEY, name STRING, type STRING, uri STRING";
>+        var dbService = Components.classes[kStorageServiceContractID].getService(kStorageServiceIID);
>+        
>+        this.mDB = dbService.openDatabase(this.findDB().file);

Use this.mDB = dbService.getProfileStorage("profile") to obtain the db for the
current profile, which is what I think you want here.  No need for findDB()
then.


>+        this.mFindCalendar = createStatement (
>+            this.mDB,
>+            "SELECT id FROM cal_calendars WHERE name = :name AND type = :type AND uri = :uri");

Do you need to query on name?  I'd think type and uri would be enough (or even
just uri, maybe).

Looks good otherwise.

Comment 5

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

Comment on attachment 172644 [details] [diff] [review]
Better patch

>+  calICalendar createCalendar(in AUTF8String aName, in AUTF8String aType, in nsIURI aURL);
>+  void registerCalendar(in calICalendar aCalendar);

Shouldn't createCalendar register the calendar as well? I think those methods
can be moved into one.

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #5)
> (From update of attachment 172644 [details] [diff] [review] [edit])
> >+  calICalendar createCalendar(in AUTF8String aName, in AUTF8String aType, in
nsIURI aURL);
> >+  void registerCalendar(in calICalendar aCalendar);
> 
> Shouldn't createCalendar register the calendar as well? I think those methods
> can be moved into one.

Doing that removes the ability to create a private non-registered calendar, as
well as the ability to create a calendar in some way other than createCalendar
(e.g. some private calendar impl) and registering it... both I think are useful.

Comment 7

[Stuart Parmenter]

Attachment: Add prefs to calendar

Comment 8

[Stuart Parmenter]

Attachment: better pref patch

Comment 9

[Joey Minta]

Based on the description in comment #0, this is fixed (and was fixed a long time ago).