426759 - Create calIAlarm interface

Status: RESOLVED FIXED

(Calendar :: Alarms, defect)

Description

[Philipp Kewisch [:Fallen][☁️📆]]

Attachment: Add calIAlarm interface

This patch takes care of the first steps for bug 353492. It consists of an interface that correctly creates and reads a VALARM component.

This interface is far from frozen for the following reasons:
* The link to the item will probably cause a circular reference
* It needs to be investigated if the way the item attribute is implemented now
  works together with the alarm service. Especially regarding recurring alarms
  and the way alarms are snoozed and dismissed.


The way the patch is now, the calIAlarm component is not used anywhere other than in the unit tests, but I think its better to split up the patches and this interface is a good candidate.

Comment 1

[Daniel Boelzle [:dbo]]

Comment on attachment 313333 [details] [diff] [review]
Add calIAlarm interface

>+    // Property bag
>+    boolean hasProperty(in AUTF8String name);
>+    nsIVariant getProperty(in AUTF8String name);
>+    void setProperty(in AUTF8String name, in nsIVariant value);
>+    void deleteProperty(in AUTF8String name);
We probably also need an iterator here, to support X-props. Else e.g. providers would need to read out from the ical component.

>+    get repeat() {
>+        if (!!this.mRepeat ^ !!this.mDuration) {
please avoid !! and use == 0 resp. == null

>+    get repeatOffset() {
>+        if (!!this.mRepeat ^ !!this.mDuration) {
same here

>+    get repeatDate() {
>+        var alarmDate = this._getAlarmDate();
>+        if (!this.mRepeat || !this.mDuration || !alarmDate) {
>+            return null;
>+        }
>+
>+        alarmDate = alarmDate.clone();
>+
>+        // XXX All Day events
>+        alarmDate.isDate = false;
I think it's ok to handle all-day events like 00:00:00 in this case.

>+    set icalString(val) {
>+        this.ensureMutable();
>+        var icssvc = Components.classes["@mozilla.org/calendar/ics-service;1"]
>+                               .getService(Components.interfaces.calIICSService);
>+        return (this.icalComponent = icssvc.parseICS(val, null));
please use getIcsService() from calUtils.js

>+    get icalComponent() {
>+        var icssvc = Components.classes["@mozilla.org/calendar/ics-service;1"]
>+                               .getService(Components.interfaces.calIICSService);
>+        var comp = icssvc.createIcalComponent("VALARM");
same here

>+        // Set up trigger (REQUIRED)
>+        var triggerProp = icssvc.createIcalProperty("TRIGGER");
>+        if (this.mAbsoluteDate) {
>+            // Set the trigger to a specific datetime
>+            triggerProp.setParameter("VALUE", "DATE-TIME");
>+            triggerProp.valueAsDatetime = this.mAbsoluteDate;
AFAIR this needs to be in UTC, please enforce that.

>+        if (durationProp && repeatProp) {
...
>+        } else if (!(durationProp ^ repeatProp)) {
IMO a simple (!durationProp && !repeatProp)) is more readable

>+    hasProperty: function cA_hasProperty(aName) {
>+        var name = aName.toUpperCase();
>+        return (name in this.promotedProps ||
I am not sure whether we should state all "promoted" properties are in; some of them actually haven't been in the ics.

>+                name in this.mProperties);
>+    },

else this first patch looks good to me, thanks for starting the work!

r=dbo

Comment 2

[Daniel Boelzle [:dbo]]

Comment on attachment 313333 [details] [diff] [review]
Add calIAlarm interface

>+    clone: function cA_clone() {
...
>+        // x-prop params
>+        for (var prop in this.mPropertyParams) {
>+            for (var param in this.mPropertyParams[prop]) {
>+                m.mPropertyParams[prop][param] =
>+                    this.mPropertyParams[prop][param];
I think you need to create m.mPropertyParams[prop] = {} first; please add some test code for cloning.

Comment 3

[Philipp Kewisch [:Fallen][☁️📆]]

> >+        // Set up trigger (REQUIRED)
> >+        var triggerProp = icssvc.createIcalProperty("TRIGGER");
> >+        if (this.mAbsoluteDate) {
> >+            // Set the trigger to a specific datetime
> >+            triggerProp.setParameter("VALUE", "DATE-TIME");
> >+            triggerProp.valueAsDatetime = this.mAbsoluteDate;
> AFAIR this needs to be in UTC, please enforce that.
While all examples in the rfc are in UTC, I could fine no explicit note, that this time should be in UTC. Why shouldn't an alarm trigger be in a timezone? I would want the alarm to happen in the timezones I have set up, and not have it rely on UTC being correct.


> 
> >+        if (durationProp && repeatProp) {
> ...
> >+        } else if (!(durationProp ^ repeatProp)) {
> IMO a simple (!durationProp && !repeatProp)) is more readable
Won't do whats needed. I would basically need (!durationProp && !repeatProp) || (durationProp && repeatProp) if I were to avoid xor. The above if doesn't help, since it could be the case that durationProp but not repeatProp.

d r = !
0 0 0 1
1 0 1 0
0 1 1 0
1 1 0 1

> 
> >+    hasProperty: function cA_hasProperty(aName) {
> >+        var name = aName.toUpperCase();
> >+        return (name in this.promotedProps ||
> I am not sure whether we should state all "promoted" properties are in; some of
> them actually haven't been in the ics.
I'm not sure what you mean by this...

Updated patch soon (tm).

Comment 4

[Stefan Sitter [:ssitter]]

> While all examples in the rfc are in UTC, I could fine no explicit
> note, that this time should be in UTC.

http://tools.ietf.org/html/draft-ietf-calsify-rfc2445bis-08#section-3.8.6.3
   Property Name:  TRIGGER
   Purpose:  This property specifies when an alarm will trigger.
   Value Type:  The default value type is DURATION.  The value type can
      be set to a DATE-TIME value type, in which case the value MUST
      specify a UTC formatted DATE-TIME value.

Comment 5

[Daniel Boelzle [:dbo]]

(In reply to comment #3)
> > >+        if (durationProp && repeatProp) {
> > ...
> > >+        } else if (!(durationProp ^ repeatProp)) {
> > IMO a simple (!durationProp && !repeatProp)) is more readable
> Won't do whats needed. I would basically need (!durationProp && !repeatProp) ||
> (durationProp && repeatProp) if I were to avoid xor. The above if doesn't help,
> since it could be the case that durationProp but not repeatProp.
IMO the remainder "|| (durationProp && repeatProp)" term is irrelevant, because it's already tested in the if-clause; that's why I suggested the simpler term.

> > >+    hasProperty: function cA_hasProperty(aName) {
> > >+        var name = aName.toUpperCase();
> > >+        return (name in this.promotedProps ||
> > I am not sure whether we should state all "promoted" properties are in; some of
> > them actually haven't been in the ics.
> I'm not sure what you mean by this...
You may get true even though a "promoted" property hasn't been set. It's a static check only.

Comment 6

[Philipp Kewisch [:Fallen][☁️📆]]

ssitter, thanks for the note. I should have seen that ;-)

(In reply to comment #5)
> IMO the remainder "|| (durationProp && repeatProp)" term is irrelevant, because
> it's already tested in the if-clause; that's why I suggested the simpler term.
After some more thinking, you are right, this should work. I'll see what the tests say to be sure.

> You may get true even though a "promoted" property hasn't been set. It's a
> static check only.
I see what you mean, I'll take care.

Comment 7

[Philipp Kewisch [:Fallen][☁️📆]]

Attachment: Add calIAlarm interface - v2

Changes made. Rerequesting review for the remaining bits, since some were not just one-liners.

Comment 8

[Daniel Boelzle [:dbo]]

Comment on attachment 315119 [details] [diff] [review]
Add calIAlarm interface - v2

r=dbo

Comment 9

[Philipp Kewisch [:Fallen][☁️📆]]

Attachment: add calAlarm.js to packages static

Additional patch for packages static, r=dbo as discussed.

Comment 10

[Philipp Kewisch [:Fallen][☁️📆]]

Checked in on HEAD and MOZILLA_1_8_BRANCH

-> FIXED