403006 - Pref driven calendar registration

Status: VERIFIED FIXED

(Calendar :: Internal Components, enhancement)

Description

[Daniel Boelzle [:dbo]]

We should stick to how mozilla products are configured, i.e. via prefs. This BTW gets us more data types for calendar prefs than we currently have (int, string, bool) and paves the way for auto deployment.

Comment 1

[Daniel Boelzle [:dbo]]

Attachment: patch - v1

This patch migrates storage registration to the prefs, like

calendars.<id>.name
calendars.<id>.type
calendars.<id>.uri
calendars.<id>.suppressAlarms
...

and preserves the order of registration (property sortPos).

The patch works for me, but I'd appreciate further QA before requesting review, especially on older 0.7/0.8 profiles. Stefan, you always seem to have a good set of test beds, don't you? :)

Comment 2

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 355128 [details] [diff] [review]
patch - v1

>+        homeCalendar.setProperty("sortPos", 0);
I was thinking about the sorting method too recently. I'm not sure sorting by number is the most smart way to do this. Lets say we want to reorder the calendars. This would mean we need to set the sortPos property on each subsequent calendar when moving the last calendar to the top. Do you have an idea how we could make this a bit more efficient?

Also, do we really need to set the sort pos to 0? Can't we just default to that?


>+    for each (var calendar in sortCalendarArray(calendars)) {
let vs var.


>+function getPrefBranchFor(id) {
>+    return ("calendars." + id + ".");
>+}
> 
>         // xxx todo: needs to be resolved with
>         //           Bug 377845 - Decouple calendar manager from storage provider
>         //
>         //           This code always runs before the update code of calStorageCalendar which
>         //           needs the old schema version to run.
>         //           So this code just updates its schema without updating the version number.
>         //
>         //           We may run into problems if this code has passed successfully, but the code
>         //           in calStorageCalendar hasn't, because on next startup this code will run
>         //           into the same section again...
If we now use preferences, do we still need this comment?


>+            if (db.tableExists("cal_calendars")) {
>+                // Check if we need to upgrade
>+                var version = this.getSchemaVersion(db);
let vs var.
>+        cal.ASSERT(calendar, "Invalid Calendar");
>+        cal.ASSERT(calendar.id !== null, "Calendar id needs to be an integer");
Do you now use a uuid for the calendar id? If so this string needs adaption. We should also check other users of the id to see if they expect an integer.


>+        // xxx todo: do we explicitly need to check for nsIVariant here?
nsIVariant is an object, but afaik xpconnect converts primitive types automatically.

>+        let prefService = Components.classes["@mozilla.org/preferences-service;1"]
>+                                    .getService(Components.interfaces.nsIPrefBranch);
>+        prefService.deleteBranch(getPrefBranchFor(calendar.id) + name);
Might this throw if the branch is locked?


>+                getCalendarManager().setCalendarPref_(this, aName, aValue);
Didn't we want to get rid of the underline functions when moving to prefs?


>+++ b/calendar/providers/gdata/content/gdata-migration-wizard.xul
>+++ b/calendar/providers/gdata/content/gdata-migration.js
I think this belongs to a different patch?


r=philipp

Comment 3

[Daniel Boelzle [:dbo]]

Attachment: patch - v2

Re-requesting review due to changes.

(In reply to comment #2)
> (From update of attachment 355128 [details] [diff] [review])
> >+        homeCalendar.setProperty("sortPos", 0);
> I was thinking about the sorting method too recently. I'm not sure sorting by
> number is the most smart way to do this. Lets say we want to reorder the
> calendars. This would mean we need to set the sortPos property on each
> subsequent calendar when moving the last calendar to the top. Do you have an
> idea how we could make this a bit more efficient?
As discussed: Doing sorting using a pref list of uuids now which is easier to modify.

> >+        let prefService = Components.classes["@mozilla.org/preferences-service;1"]
> >+                                    .getService(Components.interfaces.nsIPrefBranch);
> >+        prefService.deleteBranch(getPrefBranchFor(calendar.id) + name);
> Might this throw if the branch is locked?
I can really consider why, because calendar registration creates the pref branches. This might be a problem for central auto configuration.

> >+                getCalendarManager().setCalendarPref_(this, aName, aValue);
> Didn't we want to get rid of the underline functions when moving to prefs?
I am not yet sure about that; leaving as is for now.

Comment 4

[Daniel Boelzle [:dbo]]

BTW: I've fixed upgrading from 0.5 with this patch. Forgot about lightning-main-in-composite, ... The zombies are back! ;-)

Comment 5

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 355754 [details] [diff] [review]
patch - v2

>+                    setPref("calendar.weeks.inview", val);
cal.setPref() 

>+        // append by default:
>+        let sortOrder = cal.getPrefSafe("calendar.list.sortOrder", "").split(",");
>+        sortOrder.push(aCalendar.id);
>+        cal.setPref("calendar.list.sortOrder", sortOrder.join(","));
I'd prefer a space separated list, be it for the sole reason of the pref file wrapping better in the editor :-)


>+function sortCalendarArray(calendars) {
>+    let ret = calendars.concat([]);
>+    let sortOrder = cal.getPrefSafe("calendar.list.sortOrder", "").split(",");
>+    function sortFunc(cal1, cal2) {
>+        let i1 = sortOrder.indexOf(cal1.id);
>+        let i2 = sortOrder.indexOf(cal2.id);
Instead of calling indexOf each time, I'd prefer creating a hash map. Also, do we really want to actually set the pref here? what happens if we want to support manually reordering, this would break that?


>-            em.disableItem(LIGHTNING_UID);
>+            em.disableItem("{e2fda1a4-762b-4020-b5ad-a41df1933103}");
A comment that this is the lightning uid would be nice.

>+        let prefService = Components.classes["@mozilla.org/preferences-service;1"]
>+                                    .getService(Components.interfaces.nsIPrefBranch);
>+        prefService.deleteBranch(getPrefBranchFor(calendar.id));
We might want to create a service accessor for the pref service, but can be done in a different bug.

>+        if (cal.calInstanceOf(calendar, Components.interfaces.calICalendarProvider) &&
cal.cal... mind migrating that function too? Or do you want to do that in a different bug?

>+            let prefService = Components.classes["@mozilla.org/preferences-service;1"]
>+                                        .getService(Components.interfaces.nsIPrefBranch);
>+            let allCals = {};
>+            for each (let key in prefService.getChildList("calendars.", {})) { // merge down all keys
Hmm shouldn't this be getChildList("calendar.calendars.", {}) I though we prefix *all* our prefs with calendar.


r=philipp with comments considered.

Comment 6

[Daniel Boelzle [:dbo]]

Attachment: patch - v3

(In reply to comment #5)
> (From update of attachment 355754 [details] [diff] [review])
> >+                    setPref("calendar.weeks.inview", val);
> cal.setPref() 
I am not sure whether calUtils.jsm has been imported; staying with calUtils.js.

> >+        // append by default:
> >+        let sortOrder = cal.getPrefSafe("calendar.list.sortOrder", "").split(",");
> >+        sortOrder.push(aCalendar.id);
> >+        cal.setPref("calendar.list.sortOrder", sortOrder.join(","));
> I'd prefer a space separated list, be it for the sole reason of the pref file
> wrapping better in the editor :-)
I am OK with that, changed to space.

> >+function sortCalendarArray(calendars) {
> >+    let ret = calendars.concat([]);
> >+    let sortOrder = cal.getPrefSafe("calendar.list.sortOrder", "").split(",");
> >+    function sortFunc(cal1, cal2) {
> >+        let i1 = sortOrder.indexOf(cal1.id);
> >+        let i2 = sortOrder.indexOf(cal2.id);
> Instead of calling indexOf each time, I'd prefer creating a hash map. Also, do
> we really want to actually set the pref here? what happens if we want to
> support manually reordering, this would break that?
- added some hash code
- the prefs are just corrected if there are dangling entries, so it's no actually only a sanity check

> >-            em.disableItem(LIGHTNING_UID);
> >+            em.disableItem("{e2fda1a4-762b-4020-b5ad-a41df1933103}");
> A comment that this is the lightning uid would be nice.
done

> >+        if (cal.calInstanceOf(calendar, Components.interfaces.calICalendarProvider) &&
> cal.cal... mind migrating that function too? Or do you want to do that in a
> different bug?
Yes, different bug when we migrate calUtils.js into calUtils.jsm.

> >+            let allCals = {};
> >+            for each (let key in prefService.getChildList("calendars.", {})) { // merge down all keys
> Hmm shouldn't this be getChildList("calendar.calendars.", {}) I though we
> prefix *all* our prefs with calendar.
I thought about that, too, but decided against these quite long pref names. I don't see an obvious reason, because there's a calendar prefix. Any other opinions on this?

taking r+ forward

Comment 7

[Philipp Kewisch [:Fallen][📱📆]]

> > >+            let allCals = {};
> > >+            for each (let key in prefService.getChildList("calendars.", {})) { // merge down all keys
> > Hmm shouldn't this be getChildList("calendar.calendars.", {}) I though we
> > prefix *all* our prefs with calendar.
> I thought about that, too, but decided against these quite long pref names. I
> don't see an obvious reason, because there's a calendar prefix. Any other
> opinions on this?

Novice users might confuse "calendar." and "calendars." quickly. Maybe a different name... 

calendar.registration.* ?

Comment 8

[Daniel Boelzle [:dbo]]

Or shorter "calendar.list.*"?

Comment 9

[Philipp Kewisch [:Fallen][📱📆]]

I'd prefer .registration. I don't see a need to make the pref short here.

Comment 10

[Daniel Boelzle [:dbo]]

I've chosen "calendar.registry.<uuid>".

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/6cf9e90978b1>

-> FIXED

Comment 11

[Stefan Sitter [:ssitter]]

Relevant for Release Notes? It might affect developers that directly access storage.sdb, e.g. some synchronizing extension or application.

Comment 12

[Daniel Boelzle [:dbo]]

Yes, we should relnote this, explain the pref-schema.

Comment 13

[Andreas Treumann]

Checked in lightning and sunbird build 20090129 -> VERIFIED.

Comment 14

[Simon Paquet [:sipaq]]

Can someone please supply with a text for the release notes?