Pref driven calendar registration

VERIFIED FIXED in 1.0b1

Status

Calendar
Internal Components
--
enhancement
VERIFIED FIXED
10 years ago
8 years ago

People

(Reporter: dbo, Assigned: dbo)

Tracking

({dev-doc-needed, relnote})

unspecified
1.0b1
dev-doc-needed, relnote
Dependency tree / graph
Bug Flags:
wanted-calendar1.0 +

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

10 years ago
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.
(Assignee)

Updated

10 years ago
Blocks: 377845
(Assignee)

Updated

10 years ago
Flags: wanted-calendar0.9+
(Assignee)

Updated

10 years ago
Blocks: 410287
(Assignee)

Updated

9 years ago
Flags: wanted-calendar0.9+ → wanted-calendar1.0+
(Assignee)

Comment 1

9 years ago
Created attachment 355128 [details] [diff] [review]
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? :)
Assignee: nobody → daniel.boelzle
Status: NEW → ASSIGNED
(Assignee)

Updated

9 years ago
Keywords: qawanted
Whiteboard: [patch in hand]
(Assignee)

Updated

9 years ago
Attachment #355128 - Flags: review?(philipp)
(Assignee)

Updated

9 years ago
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+
Blocks: 328603
(Assignee)

Updated

9 years ago
No longer blocks: 328603
(Assignee)

Comment 3

9 years ago
Created attachment 355754 [details] [diff] [review]
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.
Attachment #355128 - Attachment is obsolete: true
Attachment #355754 - Flags: review?(philipp)
(Assignee)

Comment 4

9 years ago
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+
(Assignee)

Comment 6

9 years ago
Created attachment 355796 [details] [diff] [review]
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
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.* ?
(Assignee)

Comment 8

9 years ago
Or shorter "calendar.list.*"?
I'd prefer .registration. I don't see a need to make the pref short here.
(Assignee)

Comment 10

9 years ago
I've chosen "calendar.registry.<uuid>".

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

-> FIXED
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Keywords: qawanted
Resolution: --- → FIXED
Target Milestone: --- → 1.0

Comment 11

9 years ago
Relevant for Release Notes? It might affect developers that directly access storage.sdb, e.g. some synchronizing extension or application.
(Assignee)

Comment 12

9 years ago
Yes, we should relnote this, explain the pref-schema.
Keywords: relnote
Blocks: 266249
(Assignee)

Updated

9 years ago
Keywords: dev-doc-needed

Comment 13

9 years ago
Checked in lightning and sunbird build 20090129 -> VERIFIED.
Status: RESOLVED → VERIFIED

Updated

9 years ago
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.