298358 - calIAlarm needs nicer support for multiple trigger-types, etc.

Status: RESOLVED FIXED

(Calendar :: Internal Components, defect, P3)

Description

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

(or maybe to exist at all; it's a blur)

Comment 1

[Joey Minta]

Attachment: interface proposal

First draft of the proposed interface changes involved here.  The consumers of alarmTime will also have to change, once this interface is actually implemented, but that's a much bigger task.  For now I'm just looking on feedback for ways to make this more coherent, since the alarm spec is such a mess.

Comment 2

[Joey Minta]

-> me

I have the backend working for this.  Now it's just a matter of fixing all the consumers of the old alarm code.

Comment 3

[Joey Minta]

So really, what I'd like to do here is land this in 4 stages, to avoid something ridiculous like the new views patch.  The disadvantage here is that the trunk will remain broken in some respect until the final stage.  There are already dataloss issues here, but they will become more serious in the interim.

Stage 1: Land backend.  This consists of calIAlarm.idl, calAlarm.js, and changes to calIItemBase.idl and calItemBase.js.  This will completely break the event-dialogs' ability to set/unset/manage alarms.  No alarms will be fired by calAlarmService.  There will no longer be dataloss issues with simply round-tripping ics files though, as there are now.  (I have this done locally.)

Stage 2: Upgrade calAlarmService to use the new idl.  This gives us a chance to make sure that alarms really do fire, and to make sure we set a good base for email alarms in the future (lightning 0.2/0.3).

Stage 3: Upgrade the event-dialogs.  Finishing this will remove most of the severe dataloss issues.

Stage 4: Develop a conversion method for detecting when we encounter Mozilla-made calendar files created prior to stage 1.  These files are non-compliant, and need to be converted to something reasonable.

I'm open to other suggestions here as well though.

Comment 4

[Joey Minta]

VALARMs can have attachments, so we probably want that interface up and running first.

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

I guess the main worry here will be breaking folks who use the nightlies.  We'd certainly want to publicize this reasonably widely.  If this were to all land as one big patch, how big do you think that patch would be, compared to the new views patch?

Comment 6

[Joey Minta]

(In reply to comment #5)
> I guess the main worry here will be breaking folks who use the nightlies.  We'd
> certainly want to publicize this reasonably widely.  If this were to all land
> as one big patch, how big do you think that patch would be, compared to the new
> views patch?
> 

The interface+backend (stage 1) is about 45k right now.  Overall, I'd guess this will come in around 120-150k (about 1/2 of the new views).  Another option would be to land it in peices, but keep the new stuff off by default, and those who really want to test it could switch a pref (calendar.experimental.alarms).  It would create a bunch of temporary special cases in the code which isn't ideal, but we'd break less stuff.

Comment 7

[Joey Minta]

*** Bug 141406 has been marked as a duplicate of this bug. ***

Comment 8

[Joey Minta]

Attachment: create and implement calIAlarm interface

This patch creates and implements the calIAlarm interface/backend.  It does NOT remove any old code, and all of the new-interface data-reading/writing parts are in if-blocks goverened by the calendar.experimental.alarms preference.  If that preference fails (as it will for all normal users) the old code is used.  This means that normal users will notice nothing (other than the small performance hit to check the pref).  Our serious QA people can set this preference for themselves if they wish to put the code through its paces.

For those QA people: Right now, you MUST start a clean profile (or at least have a profile clear of any events with alarms) before turning on this preference.  Past alarms are invalid according to the RFC and hence cause this interface to throw.  This means dataloss is nearly certain!  However, with this preference turned on, I can successfully roundtrip alarms without any dataloss (other than that caused by our current calIAttachment failures).

Also note that this does nothing in terms of changing the front end (event-dialog or alarm-monitor).  This means that with this preference set, alarms created via the event-dialog will NOT be serialized.  The code can currently only be tested by hand-modifying an ics file.

Finally, only the ICS provider is currently capable of handling these new alarms.

I realize this is starting to sound woefully incomplete, but in my mind, it's a good first stage.  I've bumped the version of the ics files we create as well (when the pref is set), since we'll eventually need a conversion mechanism, and it can key off of this value.

Comment 9

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 207129 [details] [diff] [review]
create and implement calIAlarm interface

I think the calIAlarm service is too much targeted at ics. It can contain data that the frontend will never support, like executing programs on alarms. Won't it be better to have the interface only show what we really want to support, and maybe have the implementation deal with not loosing the contents of ics alarms that might come from external sources.
That way, it will be clear that other providers won't have to bother with storing that data, and it won't have callers think that those 'features' will really work.

Comment 10

[Michiel van Leeuwen (email: mvl+moz@)]

(In reply to comment #8)
> For those QA people: Right now, you MUST start a clean profile (or at least
> have a profile clear of any events with alarms) before turning on this
> preference.  Past alarms are invalid according to the RFC and hence cause this
> interface to throw.

I'm not too happy with this route. Is it much harder make the code able to read both types of alarms, and also output both types? No dataloss, no prefs to set, no extra addition to the testing matrix.

Comment 11

[Joey Minta]

Comment on attachment 207129 [details] [diff] [review]
create and implement calIAlarm interface

Removing review requests.  After discussion in IRC, we're going to take a slightly different approach here.  I'm going to write 3 (probably) patches that will all be reviewed before any of them land.  This will remove the need for the special testing pref.  The patches will generally follow the outline I listed in comment #3, although stages 1 and 4 will likely be combined.

Please feel free to continue to comment on this patch's interface/structure, since it will remain largely the same, just without the pref.

Comment 12

[Joey Minta]

Attachment: work in progress

Work in progress patch.  Basic testing with the ics provider and this patch was successful.  Front-end code has been updated (This was much easier with a unified dialog).  Alarms missed while Sunbird was shut down are now fired.  Old alarms from 0.2/0.3a1 are imported/subscribed to correctly.  Snooze works.  Dismissed alarms will not fire again.  Round-tripping alarms works much more effectively.

To-do:
-Storage provider drops alarms/throws on old alarms
-Snooze for variable amount of time
-Timezone testing

Comment 13

[Joey Minta]

*** Bug 334638 has been marked as a duplicate of this bug. ***

Comment 14

[Joey Minta]

Attachment: first cut at snooze

This is a first cut at getting Snooze functionality restored, which is the reason this bug is blocking 0.3.  There are still some edge cases with recurring events that I want to work through and bulletproof, but this should be enough to get the UI review started.

Comment 15

[Joey Minta]

Attachment: snooze!

This is basic, functional snooze.  Obviously for something like snooze we need a lot of exposure for testing.  There still are still edge cases for repeating events, covered by bug 348144.

Comment 16

[Joey Minta]

Attachment: alarm dialog with snooze menu

This is the alarm dialog, with the snooze menu.  It drops down to offer various snooze intervals.  onclick of one of those intervals we snooze the alarm for that amount of time.

Comment 17

[Matthew (lilmatt) Willis]

Comment on attachment 233852 [details] [diff] [review]
snooze!

>Index: calendar/base/content/calendar-alarm-dialog.js
>===================================================================
>+  // Make sure to clear out any snoozes that were here.
>+  if (item.recurrenceInfo) {
>+      item.deleteProperty("X-MOZ-SNOOZE-TIME-"+alarmWidget.item.recurrenceId.nativeTime);

Something in my head is telling me to set "X-MOZ-SNOOZE-TIME" to a constant so if we need/want to rename it later, it's not a pain.


>Index: calendar/base/content/calendar-alarm-widget.xml
>===================================================================
>+        <body><![CDATA[
>+          if (aMenuValue == "dummy") {
>+            return;
>+          }

Please either add a brief comment explaining what dummy is for, or simply rename the value to something more descriptive. Perhaps "noSnooze"?


>Index: calendar/base/src/calAlarmService.js
>===================================================================
>         onAddItem: function(aItem) {
>-            if (!aItem.alarmOffset &&
>-                !(aItem.parentItem && aItem.parentItem.alarmOffset)) {
>-                return;
>-            }
>-
>             var occs = [];
>             if (aItem.recurrenceInfo) {
>                 var start = this.alarmService.mRangeEnd.clone();
>                 // We search 1 month in each direction for alarms.  Therefore,
>                 // we need to go back 2 months from the end to get this right.
>                 start.month -= 2;
>                 start.normalize();
>                 occs = aItem.recurrenceInfo.getOccurrences(start, this.alarmService.mRangeEnd, 0, {});
>             } else {
>                 occs = [aItem];
>             }
>+
>+            occs = occs.filter(this.alarmService.hasAlarm);
>             for each (var occ in occs) {
>                 this.alarmService.addAlarm(occ);
>             }
>         },

This looks like cruft from bug 348716.



Can we lose or at least comment out the dumps throughout while you're in this file?


r1=lilmatt with those nits

Comment 18

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 233852 [details] [diff] [review]
snooze!

>+          <xul:menulist oncommand="snoozeAlarm(this.value)">

Selecting an item in a drop-down menu should not cause the window to go away. Selecting an item should not immediately store data.

Comment 19

[Michiel van Leeuwen (email: mvl+moz@)]

The dropdown is no good UI. It needs a button to really store the value.

Comment 20

[Stefan Sitter [:ssitter]]

*** Bug 349141 has been marked as a duplicate of this bug. ***

Comment 21

[Joey Minta]

(In reply to comment #19)
> The dropdown is no good UI. It needs a button to really store the value.
> 
I'm struggling to come up with a UI proposal that both addresses these concerns and doesn't end up using half of the alarm window to display Snooze options.  Without such a solution, I went ahead and followed the iCal model, which does simply use a dropdown.

Comment 22

[Dan Mosedale (:dmosedale, :dmose)]

OK, new UI that mvl and I have agreed upon:

[Snooze] for [10 minutes]

Where the thing on the left is a button, and the thing on the right is a dropdown menu.

Comment 23

[Stefan Sitter [:ssitter]]

Attachment: screenshot based on Comment #22

Due the fact that Joey is not available at the moment implemented the new UI proposal based on his patch.

Comment 24

[Stefan Sitter [:ssitter]]

Attachment: jmintas snooze! patch + UI changes

This is the snooze! patch from Joey with the changed UI (calendar-alarm-widget.xml) as shown in comment #23.

Comment 25

[Dan Mosedale (:dmosedale, :dmose)]

I think Matt and I agreed that he would take responsibility for driving this into the tree.

Comment 26

[Matthew (lilmatt) Willis]

Patch checked in on MOZILLA_1_8_BRANCH and trunk.

-> FIXED

Comment 27

[LonelyBob]

The Snooze + UI change is working well for me on Mac OSX 10.4.7 Intel.

Tested under Rosetta due to no native Intel version available using Calendar version
Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20060904 Calendar/0.3a2+

(Note- Snooze seems to be working well but Beware of trying to change details of More section there is major regression in this build but that is a different bug: Bug 351323 . Just FYI.)

Comment 28

[Omar Bajraszewski]

Snooze doesn't work very well. I created a task with alarm. When the alarm fired I set 'snooze' for 1 hour. I set the task as completed before alarm fired again. But it didn't stop the alarm and it fired after one hour. I think it should be fixed...

Comment 29

[Stefan Sitter [:ssitter]]

(In reply to comment #28)
See Bug 248342.