362648 - Google Calendar Provider: Support for Alarms

Status: RESOLVED FIXED

(Calendar :: Provider: GData, enhancement)

Description

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Add Alarm support to the Google Calendar Provider.

I have a patch at hand for at least basic alarm funtionality. It uses gd:extendedProperty to add snooze time or last acknowlege time.

The Google docs say that snooze should be implemented by setting a second alarm with an absolute time, but the Google API dosen't support this yet.

Comment 1

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: GData Alarms - v3

This is a patch that should do it. It also works around Google's lack of support for alarms on non-default calendars. Note this WILL NOT FIX GOOGLE. The alarm will not fire on any other program that uses the Google API (unless they use my extendedProperty), and will not fire in the Google UI.

Please test extensivly. I have done some tests, but that doesn't mean it works :)

Comment 2

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 262618 [details] [diff] [review]
GData Alarms - v3

Google just mutilated their alarm support. I'm going to have to make this patch follow that scheme [1], or else there will be errors and alarms might not be set at all.

[1] http://groups.google.com/group/google-calendar-help-dataapi/browse_thread/thread/16c57bcd593fc282/

Comment 3

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: GData Alarms - v4

New patch that copes with Google's new alarm "feature".

Comment 4

[Daniel Boelzle [:dbo]]

Comment on attachment 262737 [details] [diff] [review]
GData Alarms - v4

Hi Philipp, just some quick comments looking over the patch.

>+    // gd:reminder
>+    if (aItem.alarmOffset) {
>+        var gdReminder = <gd:reminder xmlns:gd={gd}/>;
>+        var alarmOffset = aItem.alarmOffset.clone();
>+        if (aItem.alarmRelated == Ci.calIItemBase.ALARM_RELATED_END) {
>+            // Google always uses an alarm offset related to the start time
>+            alarmOffset.addDuration(duration);
>+        }

I do it the same in WCAP (i.e. the server only supports alarm relation to DTSTART), though essentially this falsifies the event. I used to store the ALARM_RELATION in a separate X-prop, so roundtripping the event I could calculate / set the same alarm relation, but as you know X-props are not that well supported in WCAP. For now, I think it's ok to do so, most people won't care about that, maybe a future RFE...

>+        // Google only accepts certain alarm values. Snap to them. See
>+        // http://code.google.com/p/google-gdata/issues/detail?id=55
>+        var alarmValues = [ 300, 600, 900, 1200, 1500, 1800, 2700, 3600, 7200,
>+                            10800, 86400, 172800, 604800 ];
>+        var discreteValue = alarmValues[alarmValues.length - 1] / 60;
>+
>+        for each (var av in alarmValues) {
>+            if (-aItem.alarmOffset.inSeconds <= av) {
>+                discreteValue = av / 60;
>+                break;
>+            }
>+        }
Do we want provider specific alarm values in the future presented at UI level? IMO this fits into the general provider capabiities discussion; we should discuss this.
AFAIK by spec "for each" doesn't follow any order; iterate using an index variable to stay on the safe side.

>+    // gd:extendedProperty (alarmLastAck)
>+    var gdAlarmLastAck = <gd:extendedProperty xmlns:gd={gd}/>;
>+    gdAlarmLastAck.@name = "X-MOZ-ALARM-LAST-ACK";
name it X-MOZ-LASTACK (like in calItemBase)

>+    gdAlarmLastAck.@value = toRFC3339(aItem.alarmLastAck);
>+    entry.gd::extendedProperty += gdAlarmLastAck;

I think you should only write those X-MOZ props iff an item.alarmOffset is set.

>+    // gd:extendedProperty (snooze time)
>+    var gdAlarmSnoozeTime = <gd:extendedProperty xmlns:gd={gd}/>;
>+    var itemSnoozeTime = aItem.getProperty("X-MOZ-SNOOZE-TIME");
>+    var icalSnoozeTime = createDateTime();
>+    if (itemSnoozeTime) {
>+        // The propery is saved as a string, translate back to calIDateTime.
>+        icalSnoozeTime.icalString = itemSnoozeTime;
>+        icalSnoozeTime.normalize();
>+    }
>+    gdAlarmSnoozeTime.@name = "X-MOZ-SNOOZE-TIME";
>+    gdAlarmSnoozeTime.@value = toRFC3339(icalSnoozeTime);
>+    entry.gd::extendedProperty += gdAlarmSnoozeTime;

I suspect you are covering parents' X-MOZ-SNOOZE-TIME<some-wicked-recurrence-id> when implementing recurrence support. However as discussed on the face-2-face, we all doubt that the use of X-props is the right way and will last.

>+    function compareNotNull(prop) {
>+        return (a[prop] && !b[prop] ||
>+                !a[prop] && b[prop] ||
>+                a[prop] && b[prop] &&
>+                a[prop].compare(b[prop]));
>+    }
maybe a premature optimization, but why don't you spend local variables for a[prop] and b[prop]?

r+=dbo (gdata is still under construction, further improvements to come)

Comment 5

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: GData Alarms - v5 (patch as checked in)

> I do it the same in WCAP (i.e. the server only supports alarm relation to
> DTSTART), though essentially this falsifies the event. I used to store the
> ALARM_RELATION in a separate X-prop, so roundtripping the event I could
> calculate / set the same alarm relation, but as you know X-props are not that
> well supported in WCAP. For now, I think it's ok to do so, most people won't
> care about that, maybe a future RFE...

Why does this falsify the event? When the event times are changed, the alarm is also changed. Therefore the relation is still valid?


> Do we want provider specific alarm values in the future presented at UI level?
> IMO this fits into the general provider capabiities discussion; we should
> discuss this.
If Google keeps their alarms policy, it would be good to implement this. If not in the capabilities, then I would need to modify the event dialog per hand. So an easy way would be nice in any case :)

> I think you should only write those X-MOZ props iff an item.alarmOffset is set.
I would, but I think there is a bug on Google's side that doesn't remove the X-prop if you leave it out. The only way to clear it is to set an empty prop.

> I suspect you are covering parents'
> X-MOZ-SNOOZE-TIME<some-wicked-recurrence-id> when implementing recurrence
> support. However as discussed on the face-2-face, we all doubt that the use of
> X-props is the right way and will last.
I have always tried to ignore that, since I fear Google has no way to implement per-occurrence alarms :) I will do that when we have decided on what to do with alarms and its x-props.

Comment 6

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Checked in on MOZILLA_1_8_BRANCH and HEAD

-> FIXED

Comment 7

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Fix v5

Small additional patch needed. I'm checking this in directly since there is no big change and the gdata provider is not in the default build anyway.

The only change is to set the X-MOZ-ALARM-WORKAROUND xprop in any case, to work around a google bug.