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

RESOLVED FIXED

Status

defect
P3
normal
RESOLVED FIXED
14 years ago
13 years ago

People

(Reporter: shaver, Assigned: mattwillis)

Tracking

Details

(Whiteboard: [high risk][cal-ui-review+])

Attachments

(6 attachments, 2 obsolete attachments)

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

Updated

14 years ago
Priority: -- → P3

Updated

14 years ago
Target Milestone: --- → Lightning 0.8
Assignee: pavlov → nobody
No longer blocks: lightning-0.1
Target Milestone: Lightning 0.1 → ---

Comment 1

14 years ago
Posted patch interface proposal (obsolete) — Splinter Review
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

14 years ago
-> me

I have the backend working for this.  Now it's just a matter of fixing all the consumers of the old alarm code.
Assignee: nobody → jminta
Blocks: 308538

Comment 3

14 years ago
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

14 years ago
VALARMs can have attachments, so we probably want that interface up and running first.
Depends on: 319909
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

14 years ago
(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

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

Comment 8

14 years ago
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.
Attachment #205490 - Attachment is obsolete: true
Attachment #207129 - Flags: second-review?(mvl)
Attachment #207129 - Flags: first-review?(dmose)
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.
(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

14 years ago
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.
Attachment #207129 - Flags: second-review?(mvl)
Attachment #207129 - Flags: first-review?(dmose)

Comment 12

14 years ago
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
No longer blocks: 315051

Comment 13

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

Updated

13 years ago
Flags: blocking0.3+

Updated

13 years ago
Whiteboard: [swag:3d]

Updated

13 years ago
Whiteboard: [swag:3d] → [swag:3d][high risk]

Comment 14

13 years ago
Posted patch first cut at snooze (obsolete) — Splinter Review
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.

Updated

13 years ago
Whiteboard: [swag:3d][high risk] → [swag:3d][high risk][cal-ui-review needed]

Comment 15

13 years ago
Posted patch snooze!Splinter Review
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.
Attachment #233828 - Attachment is obsolete: true
Attachment #233852 - Flags: second-review?(dmose)
Attachment #233852 - Flags: first-review?(mattwillis)

Updated

13 years ago
Whiteboard: [swag:3d][high risk][cal-ui-review needed] → [swag:3d][high risk][cal-ui-review needed][patch in hand]

Comment 16

13 years ago
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.
Assignee

Comment 17

13 years ago
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
Attachment #233852 - Flags: first-review?(mattwillis) → first-review+
Whiteboard: [swag:3d][high risk][cal-ui-review needed][patch in hand] → [swag:3d][high risk][cal-ui-review+][patch in hand]
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.
Attachment #233852 - Flags: second-review?(dmose) → second-review-
The dropdown is no good UI. It needs a button to really store the value.
Whiteboard: [swag:3d][high risk][cal-ui-review+][patch in hand] → [swag:3d][high risk][cal-ui-review-][patch in hand]
*** Bug 349141 has been marked as a duplicate of this bug. ***

Comment 21

13 years ago
(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.
Whiteboard: [swag:3d][high risk][cal-ui-review-][patch in hand] → [swag:3d][high risk][cal-ui-review?][patch in hand]
Whiteboard: [swag:3d][high risk][cal-ui-review?][patch in hand] → [swag:3d][high risk][cal-ui-review needed][patch in hand]
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.
Assignee: jminta → dmose
Whiteboard: [swag:3d][high risk][cal-ui-review needed][patch in hand] → [swag:3d][high risk][cal-ui-review+]
Whiteboard: [swag:3d][high risk][cal-ui-review+] → [swag:2h][high risk][cal-ui-review+]
Due the fact that Joey is not available at the moment implemented the new UI proposal based on his patch.
This is the snooze! patch from Joey with the changed UI (calendar-alarm-widget.xml) as shown in comment #23.
I think Matt and I agreed that he would take responsibility for driving this into the tree.
Assignee: dmose → mattwillis
Assignee

Comment 26

13 years ago
Patch checked in on MOZILLA_1_8_BRANCH and trunk.

-> FIXED
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
Whiteboard: [swag:2h][high risk][cal-ui-review+] → [high risk][cal-ui-review+]

Comment 27

13 years ago
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

13 years ago
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...
(In reply to comment #28)
See Bug 248342.
You need to log in before you can comment on or make changes to this bug.