Closed Bug 403006 Opened 12 years ago Closed 11 years ago

Pref driven calendar registration

Categories

(Calendar :: Internal Components, enhancement)

enhancement
Not set

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: dbo, Assigned: dbo)

References

Details

(Keywords: dev-doc-needed, relnote)

Attachments

(1 file, 2 obsolete files)

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.
Blocks: 377845
Flags: wanted-calendar0.9+
Blocks: 410287
Flags: wanted-calendar0.9+ → wanted-calendar1.0+
Attached patch patch - v1 (obsolete) β€” β€” Splinter Review
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? :)
Assignee: nobody → daniel.boelzle
Status: NEW → ASSIGNED
Keywords: qawanted
Whiteboard: [patch in hand]
Attachment #355128 - Flags: review?(philipp)
Whiteboard: [patch in hand]
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
Attachment #355128 - Flags: review?(philipp) → review+
No longer blocks: 328603
Attached patch patch - v2 (obsolete) β€” β€” Splinter Review
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.
Attachment #355128 - Attachment is obsolete: true
Attachment #355754 - Flags: review?(philipp)
BTW: I've fixed upgrading from 0.5 with this patch. Forgot about lightning-main-in-composite, ... The zombies are back! ;-)
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.
Attachment #355754 - Flags: review?(philipp) → review+
Attached patch patch - v3 β€” β€” Splinter Review
(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
Attachment #355754 - Attachment is obsolete: true
Attachment #355796 - Flags: review+
> > >+            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.* ?
Or shorter "calendar.list.*"?
I'd prefer .registration. I don't see a need to make the pref short here.
I've chosen "calendar.registry.<uuid>".

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

-> FIXED
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: qawanted
Resolution: --- → FIXED
Target Milestone: --- → 1.0
Relevant for Release Notes? It might affect developers that directly access storage.sdb, e.g. some synchronizing extension or application.
Yes, we should relnote this, explain the pref-schema.
Keywords: relnote
Keywords: dev-doc-needed
Checked in lightning and sunbird build 20090129 -> VERIFIED.
Status: RESOLVED → VERIFIED
Depends on: 479867
Can someone please supply with a text for the release notes?
Target Milestone: 1.0 → 1.0b1
You need to log in before you can comment on or make changes to this bug.