353492 - support multiple alarms per events/task, support absolute alarms with fixed date/time

Status: RESOLVED FIXED

(Calendar :: Alarms, enhancement)

Description

[pevr]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; fr; rv:1.8.0.7) Gecko/20060909 Firefox/1.5.0.7
Build Identifier: Lightning build 2006031011

One can only assign one alarm for an event.

For example, sometimes I would like to receive an alarm the day before and half an hour before the event starts. The first one reminds me to prepare materials for the event, the second one reminds me that I have to go to the event.

Reproducible: Always

Steps to Reproduce:
1. I create a new event
2. I chose the advanced view
3. I define an alarm
4. I cannot define an additional alarm

Comment 2

[Omar Bajraszewski]

IMO this feature is not needed. If I want calendar to remind me again about the event I can use snooze for it. But snooze option should be more flexible- default times are useless. 

Comment 3

[mmindenhall]

I disagree with Omar.  I have found the multiple alarms support in Google Calendar to be very useful.  Simply using the snooze option is not equivalent, because gCal allows me to specify specify each alarm as popup, e-mail, or SMS.  So in the example given above, I could specify an alarm one day before an event as an e-mail, and then another alarm half an hour before the event as SMS or a popup.

Comment 4

[Philipp Kewisch [:Fallen][📱📆]]

I will be taking care of this, but since I have a bunch of other things to do, its not very high up on my list. If someone wants to contribute or take over, please contact me.

Comment 5

[seshu.parvataneni]

Please consider this as medium to high priority as I think this is one feature that is preventing most of the people using SeaMonkey's calendar extension from using lightning. Thanks.

Comment 6

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Current reminder dialog morphed

Some changes to the alarm dialog is needed. I have already talked with Christian about this dialog and we came to the conclusion it makes more sense to use the listbox to describe the multiple alarms, rather than alarm templates. Alarm templates could be done in an extension.

Some notes and shortcomings that need to be resolved:
* The action text is pretty long ("Show an Alarm")
  - Should we use icons instead?
  - If icons are used, I'd suggest also adding them to the selector.
  - If icons are used, I need icons :-)
* The reminder actions use up a lot of space
  - There is no room for extra options (i.e a recipient email, an alarm
    description, possibly a custom alarm sound, bla bla bla)
  - I'd suggest using a dropdown instead, the dropdown could also have icons
  - We might want to increase the overall window size. If a certain alarm
    action has many configuration parameters, the window might have to get
    larger anyway. Sure, a button could be shown to set up specifics of the
    certain alarm actions, but this means another sub-dialog, which is quite
    a bit (main window -> event dialog -> reminder dialog -> reminder action
    dialog)
* Selection scheme might be confusing
  - When selecting a reminder, the values are put into the fields
  - To create a new alarm, you need to click on new, then a new listbox item
    shows up, and you can change the values
  - I'm not saying I find this behavior confusing, just want to make sure its
    the same for non-technical/non-powerusers.

Comment 7

[seshu.parvataneni]

Can you make the options for the "Choose a Reminder Action" a check box instead of radio buttons. I would want to choose to show me an alert as well as sending an e-mail or show me an alert as well as playing a sound. It gives users more flexibility in my opinion.

Comment 8

[seshu.parvataneni]

hmmm..... on a second look at the attachment I get the usage model. Please ignore my comment #7. I think the radio buttons are fine. Thanks for taking a look at this feature. I am sure there are a lot of people waiting for this feature to be implemented.

Comment 9

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Mockup with icons and a dropdown

This is my suggestion how things could look with icons and a dropdown. I didn't increase the window size (yet), since this is only a gimp mockup and I was too lazy to create the screenshot again with a larger window size.

Comment 10

[Philipp Kewisch [:Fallen][📱📆]]

I was with checkboxes at first too, but its *much* easier to implement this way, since I can add each alarm as a listbox row easily instead of having to map together the different VALARM components with the same date/duration and different actions.

Further considerations:
* Should the alarms list be sorted?
  - I'd say so, it makes sense to have the alarm closest to the event at the top
  - Con: As the value changes, the item moves around in the listbox. This could
    be confusing.
* Where should a message be displayed, when no more alarms can be added?
  - Google Calendar supports 5 alarms per event. When 5 alarms are added, the
    "New" button should be disabled.
  - I think it would make sense to display a message somewhere, instead of just
    disabling the button without notice.

Comment 11

[seshu.parvataneni]

Hi Philip,

Looks like the space underneath the new and remove buttons is wasted. How about laying these buttons horizontally aligned below the reminders list? The list and these two buttons could be encapsulated in a frame. 

With regards to the indication when the max number of actions are defined why not  replace the "Choose A Reminder Action" with a text label saying that the maximum number of reminders have been reached and the user has to delete an existing one to add another item. And when the user deletes one of the reminders then replace this text with the "Choose A Reminder Action" frame again.

Comment 12

[Christian Jansen]

(In reply to comment #11)
> Hi Philip,
> 
> Looks like the space underneath the new and remove buttons is wasted. How about
> laying these buttons horizontally aligned below the reminders list? The list
> and these two buttons could be encapsulated in a frame. 

I like this idea.
> 
> With regards to the indication when the max number of actions are defined why
> not  replace the "Choose A Reminder Action" with a text label saying that the
> maximum number of reminders have been reached and the user has to delete an
> existing one to add another item. And when the user deletes one of the
> reminders then replace this text with the "Choose A Reminder Action" frame
> again.
> 

I'd prefer if we stick to the standards, like error messages as dialog or intergrated notification bar (like the pop-up warning in Firefox).

Please find a slightly revised dialog attached.

Comment 13

[Christian Jansen]

Attachment: Multi Reminder Dialog with Add, Remove buttons and layout adjustments

Comment 14

[Philipp Kewisch [:Fallen][📱📆]]

(In reply to comment #13)
> Created an attachment (id=325378) [details]
> Multi Reminder Dialog with Add, Remove buttons and layout adjustments
> 

The heading seems a bit strange for the alarm type. The first heading says "Reminder", while the second says "Alarm". Not very intuitive :-)

Comment 15

[seshu.parvataneni]

Also the can you make the add new reminder buttons right aligned? The Cancel and OK are right aligned but the add and remove are left aligned. Not so consistent. 

I see that the borders for the frames are truncated from the window. Was the window resized when the screen capture was taken?

Can you rename "Alarm" to "Reminder Type". It is the type of reminder the user is setting up.

Comment 16

[Christian Jansen]

(In reply to comment #15)
> Also the can you make the add new reminder buttons right aligned? The Cancel
> and OK are right aligned but the add and remove are left aligned. Not so
> consistent.

Right aligned is ok for me.

> 
> I see that the borders for the frames are truncated from the window. Was the
> window resized when the screen capture was taken?

The borders are not truncated. They were remove. This aligns to the Event dialog  design.
> 
> Can you rename "Alarm" to "Reminder Type". It is the type of reminder the user
> is setting up.
> 
Sounds much better.

We agreed to display the "The maximum number of $N reminders have been reached" warning next to the Add/Remove buttons.

Comment 17

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Current state of the dialog

These are the current looks of the dialog, as implemented:

* I decided to make the button text a bit longer, since for a11y its good to have an accesskey for the button, but putting the accesskey-underline under a + makes it look similar to a plus-minus sign.
* I found out about <notificationbox>. Adding 5 alarms on a Google Calendar makes the shown notification box show up, as shown in screenshot.

Comment 18

[seshu.parvataneni]

It is Ok. A couple of my thoughts though.
1. The warning says "The selected calendar has a...". I would prefer it say "The selected Event/Task has a limit of 5 reminders" or "The limit of 5 reminders has been reached for the selected Event/Task"?
2. The "Reminder Details" and "Choose a Reminder Action" frames have titles but not the current action list. I think it would be consistent with the rest of the window if the warning, list and the add remove buttons are encapsulated in a frame with title "Current Reminders". 
3. There is less space before the "Reminder Details" title compared to above the "Choose a Reminder Action" title. I think you can increase the padding/vertical space above the "Reminder Details".

Comment 19

[Philipp Kewisch [:Fallen][📱📆]]

(In reply to comment #18)
> It is Ok. A couple of my thoughts though.
> 1. The warning says "The selected calendar has a...". I would prefer it say
> "The selected Event/Task has a limit of 5 reminders" or "The limit of 5
> reminders has been reached for the selected Event/Task"?
I don't really mind, but if the string says something about the calendar, then its more clear how to change that.

> 3. There is less space before the "Reminder Details" title compared to above
> the "Choose a Reminder Action" title. I think you can increase the
> padding/vertical space above the "Reminder Details".
No problem

Comment 20

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Event dialog after selecting alarms

I was thinking: its kind of annoying if you specify multiple alarms, you don't know what type of alarms are set. The attached mockup shows what would be possible. For each unique action, the corresponding alarm action icon could be shown together with a link text "Multiple Alarms" (pretend the fonts match, and if you like consider it a link)

What do you think?

Comment 21

[seshu.parvataneni]

Probably I am being too picky but I think the add reminder and remove reminder buttons should be swapped. If you see the Cancel and OK button logic, the optimistic button is on the right side. So, I guess if we follow the same logic the add reminder button should be on the right side?

Comment 22

[seshu.parvataneni]

As per the "Multiple Alarms" link does it have to say "Multiple Alarms"? You are already displaying the icons. How about simply displaying the text "Reminders" and then display the icons on the right side instead? Thanks in advance for considering my opinions w.r.t this feature.

Comment 23

[seshu.parvataneni]

actually a better idea for the text. How about changing the current text, "Reminder" that exists on the left side of the drop down to "Reminder(s)" and simply display the icons on the right side of the drop down?

Comment 24

[Stefan Sitter [:ssitter]]

(In reply to comment #21)
> If you see the Cancel and OK button logic, the optimistic button 
> is on the right side. So, I guess if we follow the same logic the
> add reminder button should be on the right side?

The button order is OS dependent. On Windows [Ok] is left of the [Cancel] button.

> shown together with a link text "Multiple Alarms"

How about e.g. "Show reminder details..." or similar? And maybe the reminder icon could has a tooltip that shows detailed information about the reminder, e.g. "Show an alert 10 minutes before the event starts"?

Comment 25

[Christian Jansen]

Comment on attachment 325630 [details]
Event dialog after selecting alarms

Looks good for me. One tiny nit. Please underline the link. ui+ with the change. ui=christian.

Comment 26

[Christian Jansen]

Comment on attachment 325567 [details]
Current state of the dialog

Please right align the "Add" / "Remove" buttons. IMO a simple "+" and "-" reduces clutter. If you don't like that please use simple Add and Remove. Would it be possible to align the Reminder Details section on the right hand side?

Comment 27

[Christian Jansen]

(In reply to comment #21)
> Probably I am being too picky but I think the add reminder and remove reminder
> buttons should be swapped.

This is against the common usage.

 If you see the Cancel and OK button logic, the
> optimistic button is on the right side. So, I guess if we follow the same logic
> the add reminder button should be on the right side?
> 
The order of OK, Cnacel relates to the underlying platform. Which means the order can change.

Comment 28

[Philipp Kewisch [:Fallen][📱📆]]

I'll probably be using the thunderbird icon for "Send an Email", but I'll need a such icon for "Send a Text Message" (maybe a small blue cellphone that matches the other icons style) for gdata.

Also, how should event boxes be displayed? In the event boxes where the alarm icon currently is, should the same set of icons be shown as in the event dialog? (i.e max one of each reminder icon an alarm has been set for) This might clutter the event boxes though (i.e three icons if a gdata user has set an alarm, a popup and an sms reminder)

Comment 29

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Backend patch - v3

I'm almost there. Aside from probably regressions and the fact that the alarm dialog doesn't correctly handle recurring events, I have working patches (!).

Due to the large amount of changed code, I'd like to attach this patch early to get feedback and if possible also QA on the patches.

The problem with dismissing/snoozing recurring events is that the wrong items are used. I'll elaborate more on this as soon as I can better formulate the questions I have open.

Comment 30

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: UI patch - v3

These are the UI changes, aside from problems with recurring events, I'm pretty confident that these changes actually work :-)

Comment 31

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Reminder icons

I consolidated our alarm images into one image, except of course for the flashing alarm icon.

Comment 32

[Philipp Kewisch [:Fallen][📱📆]]

In case you are on linux and would like to test v3, please do so. Remember to back up your profile or at least your storage.sdb, since this xpi does a db schema change. Remember also that this xpi cannot handle alarms on recurring events.

An xpi is available at http://mozilla.kewis.ch/lightning-multialarms.xpi if you get errors about the timezones, I have a timezones.xpi at http://mozilla.kewis.ch/calendar-timezones.xpi

Comment 33

[Daniel Boelzle [:dbo]]

(In reply to comment #32)
...
> An xpi is available at http://mozilla.kewis.ch/lightning-multialarms.xpi if you
> get errors about the timezones, I have a timezones.xpi at
> http://mozilla.kewis.ch/calendar-timezones.xpi

Philipp, since timezones are still included into stock lightning (which WFM), could you elaborate when this trouble happens (and write a bug), please?

Comment 34

[Philipp Kewisch [:Fallen][📱📆]]

(In reply to comment #33)
> Philipp, since timezones are still included into stock lightning (which WFM),
> could you elaborate when this trouble happens (and write a bug), please?
I didn't experience this myself, but Lars seemed to have some trouble. I'll ask him to reproduce and file a bug.

Comment 35

[Philipp Kewisch [:Fallen][📱📆]]

Andreas, could you give the attached xpi some testing? See previous comments.

Some thoughts about the missing recurring alarms support:

First of all we need to make clear how recurring alarms should be supported.
* If the parent item has an alarm, IIUC this alarm should fire for each
  occurrence if it is relative.
* Also, if the item is an exception and has its own alarms defined, then
  they should also fire. Is this correct?

Next, we need to define what getAlarms() should return for a recurring item:
* On the parent item, it should only return alarms that belong to the parent
  item.
* On a calculated occurrence (not exception), should it return the parent 
  item's alarm? Also, what should the calIAlarm::item attribute be for the
  alarm?
* On an exception that defines an alarm itself, should it return only the 
  alarm defined on the exception, or also the parent item's alarm. If the
  parent item alarm should be returned, again what should calIAlarm::item be?

A quick look at the event dialog:
* With the current implementation, the event dialog only gets alarms defined
  on the occurrence itself, doesn't get any alarms from the parent item. This
  means, for a recurring item where the alarm is set on the parent item, any
  occurrence selected will say "no reminder", while the parent displays the
  alarm.
* Changing the implementation to return also alarms from the parent item lets
  the alarm from the parent item show for normal occurrences, but when
  creating exceptions, then editing the exception again had a problem that
  the alarms were added twice. This happened since I patched the calIAlarm
  returned in getAlarms() to always have the item of the occurrence, not
  of the parent item. I needed to do this so that the correct alarm time
  is shown in the alarm popup.

Now we need to take a look at what happens between the alarm service, monitor and UI.
* When getting the alarms to display from the item - given that getAlarms()
  doesn't return the parent alarms - we also need to get the alarms from the
  parent item. When these are added to the event dialog, only the time of
  the base item is shown, since calIAlarm.item was not patched.
* If I were to patch the item in code that gets the alarms, I would have to
  patch the items back correctly when snoozing an alarm, since otherwise
  exceptions would be created or the alarm would be added twice.
* I was thinking of also passing an item to the alarm monitor/UI which should
  be used to dismiss/snooze, but this didn't quite work out for reasons I
  don't remember.


In short, we need to think about where the alarms of the parent items should be retrieved, either somehow in getAlarms() call of the occurrence, or separately in the alarm service/monitor/ui.

It would be great if someone with backend experience could read this (admittedly long) text and bring in their opinion for the right solution (daniel? mvl? ...?). I've tried to format it for easier reading :-)

Comment 38

[Bas van den Bosch]

Did someone build this on win32 already? I'd like to test this, sounds great.

Comment 39

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: [checked in] Strings only - v1

Strings only, due to string freeze.

Comment 40

[Philipp Kewisch [:Fallen][📱📆]]

New testing packages available:

Windows:
http://mozilla.kewis.ch/lightning-multialarms-win32.xpi

Gdata Provider, cross platform:
http://mozilla.kewis.ch/gdata-provider-multialarms.xpi

I haven't tested the builds, please write me a private email if something obvious doesn't work. I also updated the linux xpi to include the latest nightly patches.

Comment 41

[Christian Jansen]

Comment on attachment 325567 [details]
Current state of the dialog

ui=christian

Comment 42

[Daniel Boelzle [:dbo]]

Comment on attachment 328965 [details] [diff] [review]
[checked in] Strings only - v1

r=dbo

Comment 43

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: [checked in] Strings only - v2

I accidentally introduced duplicate entities with v1. This remained unnoticed since the app doesn't complain. This patch fixes it and takes out some (currently unused) UI from the alarm dialog, resulting in less strings.

Comment 44

[seshu.parvataneni]

Hi,

I've just played with the UI and here are somethings that I found:
- The window title says "Set up reminders". Shouldn't the first letter of "reminders" be a capital letter? In other places a capital "R" was used. 
- There is a bug when add reminders. I am not sure if the extension that you sent should be fully functional for adding reminders to the list but here are the steps to reproduce:
1. Start with no items in the list
2. Click Add to add a reminder
3. Change the units for the reminder to say 10 days
4. Click on the Add button

Expected result
After adding a reminder in step 3 there should be two reminders set. One from step 2 and the other from step 4

Actual result:
There are two reminders that are set but the first reminder's trigger time is changed to the one that is set in step 3.

Comment 45

[Rimas Kudelis [use needinfo to ping]]

from calendar.properties:
# Alarm interface stringes
alarmDefaultDescription=Default Mozilla Description
alarmDefaultSummary=Default Mozilla Summary

WTF are these lines? Where will they be visible? Are you planning to just leave them like this?

Comment 46

[Rimas Kudelis [use needinfo to ping]]

Also, in the same file, these lines:
day=Day
days=Days
hour=Hour
hours=Hours

could have a localization note with an example how these strings will be used.

Comment 47

[Philipp Kewisch [:Fallen][📱📆]]

(In reply to comment #45)
> from calendar.properties:
> # Alarm interface stringes
> alarmDefaultDescription=Default Mozilla Description
> alarmDefaultSummary=Default Mozilla Summary
> 
> WTF are these lines? Where will they be visible? Are you planning to just leave
> them like this?
Localization note added. Note there is no way to verify until this bug is really fixed, but in the ICS file it will look like

DESCRIPTION:Default Mozilla Description

and other apps may display this somewhere.

(In reply to comment #46)
> Also, in the same file, these lines:
> day=Day
> days=Days
> hour=Hour
> hours=Hours
> 
> could have a localization note with an example how these strings will be used.
Belongs to a different bug but I just backed these out since a different patch removed the need again.

Comment 51

[Bas van den Bosch]

tested with local calendars: snoozing an alarm creates an extra valarm (when exported to ics), you get the original alarm plus the snoozed one (x-mox-lastack). This duplicates every time you snooze an alarm and happens also with only one alarm set. 

Comment 52

[Bas van den Bosch]

I think you might need an x-prop-alarm-id to identify the alarm which gets snoozed so you know which of the multiple alarms has to get a X-MOZ-SNOOZE-TIME.

Comment 53

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Complete patch - v7

Just so I don't loose any work, here the de-bitrotted version that should apply to 0.9. I was thinking this patch could be further split by implementing the UI with the old alarm interface, then transitioning the backend afterwards (with intermediate patches i.e for the Alarm interfaces)

Comment 55

[Daniel Boelzle [:dbo]]

(In reply to comment #35)
> First of all we need to make clear how recurring alarms should be supported.
> * If the parent item has an alarm, IIUC this alarm should fire for each
>   occurrence if it is relative.
> * Also, if the item is an exception and has its own alarms defined, then
>   they should also fire. Is this correct?
I agree.

> Next, we need to define what getAlarms() should return for a recurring item:
> * On the parent item, it should only return alarms that belong to the parent
>   item.
> * On a calculated occurrence (not exception), should it return the parent 
>   item's alarm? Also, what should the calIAlarm::item attribute be for the
>   alarm?
I don't see yet why we need to hold a reference to the item. If we do so, it ought to be a reference to the corresponding occurrence, not master.
But why do we need to hold it? A quick look only tells me about hashId. I think we could model calIAlarm to only hold the alarm data, omitting any reference to the base, and could use the same object for all occurrences.
<slightly-off-topic>IMO the same applies to some extend to calIRecurrenceInfo. I don't see why it needs to hold a reference to the master item and I don't think we should manage exception objects in calRecurrenceInfo, but at the master item. IMO it's just convenience, we could also pass in the master item when expanding the occurrences; I think that would be cleaner.</>

> * On an exception that defines an alarm itself, should it return only the 
>   alarm defined on the exception, or also the parent item's alarm. If the
>   parent item alarm should be returned, again what should calIAlarm::item be?
Only the exception's alarm.

> A quick look at the event dialog:
> * With the current implementation, the event dialog only gets alarms defined
>   on the occurrence itself, doesn't get any alarms from the parent item. This
>   means, for a recurring item where the alarm is set on the parent item, any
>   occurrence selected will say "no reminder", while the parent displays the
>   alarm.
That's not true, maybe you are referring to an earlier version, but I've fixed that bug some time ago: <http://mxr.mozilla.org/comm-central/source/calendar/base/src/calItemBase.js#302>.
If it's a "true" proxy of its master item, an occurrence inherits the alarm, else (in case it's an exception), it returns its own alarm setting (only).

> * Changing the implementation to return also alarms from the parent item lets
>   the alarm from the parent item show for normal occurrences, but when
>   creating exceptions, then editing the exception again had a problem that
>   the alarms were added twice. This happened since I patched the calIAlarm
>   returned in getAlarms() to always have the item of the occurrence, not
>   of the parent item. I needed to do this so that the correct alarm time
>   is shown in the alarm popup.
Doesn't sound good, I think you should stick to the existing mimic.

Comment 58

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: [checked in] Backend patch 1 (Roundtrip all alarms, provider support, simplify alarm service)

This patch takes care of some backend steps and requires the upcoming frontend patch. This patch is ready for review and checkin.

Some more backend steps are required after this patch, I'll do that in extra steps though:
* storing alarms in storage.sdb.
* fixing the alarm service and alarm dialog (alerting) for multiple alarms

Comment 59

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: [checked in] Frontend Patch 1 (New alarm dialog, gdata frontend)

This patch fixes the frontend for multiple alarms. Until all backend patches are there, I've limited the alarms to max 1 for now. I guess this could better be done on the base provider, but otoh I want the backend patches in soon so I don't think its worth the effort now.

This patch requires the backend patch to be applied. Note that you need both patches applied in any case for testing.

Comment 60

[Philipp Kewisch [:Fallen][📱📆]]

Bug 474213 would move the nodeIterator to a different file, i.e resource://gre/modules/iteratorUtils.jsm. If this bug lands before review please assume that this bug has the paths and namespaces changed.

Comment 61

[Berend Cornelius]

>                             <menuitem id="reminder-5hours-menuitem"
>                                       >label="&event.reminder.5hours.before.label;"
>                                       length="5"
>-                                      origin="1"
>+                                      origin="before"
>                                       relation="START"
>                                       unit="hours"/>

You are using three different attributes ("length", "origin" and "unit"), where as far as I see one would suffice. I would prefer if you would just use the "value" attribute denoting the alarm offset in seconds - either positive or negative.
Similarily the menuitems underneath the "reminder-unit" could have value attributes with "60", "3600"... instead of "minutes", "hours".
+              <menuitem label="&reminder.relation.before.label;" value="before"/>
+              <menuitem label="&reminder.relation.after.label;" value="after"/>

Same story: Why not assign directly "+1" + "-1" to the "value" attribute. Should simplify the js code that evaluates it.

>+<!ENTITY reminderdialog.title                              "Set up reminders">

I believe dialog titles should be capitalized.


>+    if (aDisableAll) {
>+        relativeDisabled = "true";
>+        absoluteDisabled = "true";
>+    } else if (relationItem) {
>+        // This is not a mistake, when this function is called from onselect,
>+        // the value has not been set.
>+        relativeDisabled = (relationItem.value == "absolute") && "true";
>+        absoluteDisabled = (relationItem.value == "relative") && "true";
>+    } else {
>+        relativeDisabled = false;
>+        absoluteDisabled = false;
>+    }

mixed types...


>+#reminder-actions-menupopup > menuitem > .menu-iconic-left > .menu-iconic-icon >{

I found, that this is enough:
.reminder-icon > .menu-iconic-left > .menu-iconic-icon {

>#reminder-notifications > notification > .notification-inner {
>    border: 0;
>}

Isn't the border 0 by default? 


>    list-style-image: url(chrome://calendar/skin/alarm-icons.png);
image should be assigned only once.


patch looks very good. r=berend.

Comment 62

[seshu.parvataneni]

cooooooooooooooooooooool.....can't wait to test this feature.....Thank you verrrrrrry much. Have been waiting for this since the calendar version for mozilla suite (about 3 years).

Comment 63

[Daniel Boelzle [:dbo]]

Comment on attachment 357551 [details] [diff] [review]
[checked in] Backend patch 1 (Roundtrip all alarms, provider support, simplify alarm service)

looks good; r=dbo

Comment 64

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 357551 [details] [diff] [review]
[checked in] Backend patch 1 (Roundtrip all alarms, provider support, simplify alarm service)

backend patch pushed to comm-central, <http://hg.mozilla.org/comm-central/rev/9d574eae525e>

Comment 65

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 357552 [details] [diff] [review]
[checked in] Frontend Patch 1 (New alarm dialog, gdata frontend)

Frontend patch pushed to comm-central
<http://hg.mozilla.org/comm-central/rev/125c3a73ab46>

Comment 66

[Philipp Kewisch [:Fallen][📱📆]]

(In reply to comment #61)
> You are using three different attributes ("length", "origin" and "unit"), where
> as far as I see one would suffice.
While I generally agree, in this case I'd like to keep it as is. 1 Day doesn't necessarily have to be 24 hours or 86400 seconds, i.e on a DST boundary.

> I believe dialog titles should be capitalized.
Done

> mixed types...
I added a comment, this was done on purpose.

> I found, that this is enough:
> .reminder-icon > .menu-iconic-left > .menu-iconic-icon {
Changed

> Isn't the border 0 by default? 
toolkit styles give it a border I needed to get rid of.

> image should be assigned only once.
Done.

Comment 67

[Philipp Kewisch [:Fallen][📱📆]]

Leaving open for further patches (alarm service, storage calendar)

Comment 68

[Bas van den Bosch]

Which providers have multiple alarms now? For storage and ics I just get a single alarm with the 31-01-2009 build of Lightning.

Comment 69

[Philipp Kewisch [:Fallen][📱📆]]

There are still missing patches, at the moment only 1 alarm is allowed at all. There are further patches needed for the alarm service, the storage calendar and such needed to support multiple alarms.

Comment 70

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Storage Provider support for multiple alarms

This patch takes care of letting the storage provider support multiple alarms.

Comment 71

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Alarm Service/Monitor and random fixes

This should be the last patch from the "complete patch - v7" bundle. It requires the storage patch. With this patch we support multiple alarms for storage, memory, gdata, caldav.

Missing parts for other bugs:
* Multiple alarms for WCAP
* Client support for EMAIL alarms
* Currently, dismissing one DISPLAY alarm from an event also dismisses all
  other DISPLAY alarms from that event, since we save X-MOZ-LASTACK and the
  snooze prop on the item. Back half a year ago I remember that I had problems
  saving those two props directly in the VALARM, but we should investigate
  doing this so we can snooze/dismiss specific alarms.

  A question we should think about in that bug: do we need to show multiple
  entries per event, just because they are different popup alarms?
    Pro: If we want to use the DESCRIPTION property, then we will need to.
    Con: If not, there is no need to show entries more than once.
  If we decide to go with showing only one entry, then we can keep the props
  directly on the item and will not need any migration code.

Comment 72

[Bas van den Bosch]

If you only show one entry, the problem described will stay: dismissing one alarm dismisses all alarms. Either that or you should add code to the alarm-window which asks wether you want to dismiss/snooze all alarms or just this one. So I'd go with showing all for now. Also, as soon as you have email-support you'll need different descriptions/snoozes etc. 

Could you provide a new testpackage for testing like you did in comment 40? 

Are multiple alarms for ics/webdav also included, or do you mean that with memory?

Comment 73

[Daniel Boelzle [:dbo]]

Comment on attachment 360788 [details] [diff] [review]
Storage Provider support for multiple alarms

looks good, r=dbo

Requesting additional review from Stefan.

Comment 74

[Philipp Kewisch [:Fallen][📱📆]]

(In reply to comment #72)
> If you only show one entry, the problem described will stay: dismissing one
> alarm dismisses all alarms.
Yes, I'd like to fix this in a different bug. Its a bit unfortunate, but I think we can live with it.

> Also, as soon as you have
> email-support you'll need different descriptions/snoozes etc. 
I doubt Email support will make it into the next release, unless someone steps up.

> Could you provide a new testpackage for testing like you did in comment 40? 
I'll see what I can do, although due to time restrictions its probably easier to wait for checkins.


> Are multiple alarms for ics/webdav also included, or do you mean that with
> memory?
Yes, multiple alarms should also work for webdav.

Comment 75

[Stefan Sitter [:ssitter]]

Comment on attachment 360788 [details] [diff] [review]
Storage Provider support for multiple alarms

Declining because database update from Sunbird 0.9 to a build with this patch fails if some basic alarms exist in the default storage calendar.

Comment 76

[Stefan Sitter [:ssitter]]

Attachment: Sample alarms from Sunbird 0.9 that break the database update

This ics file contains the sample events/tasks created in Sunbird 0.9. To reproduce the issue import the file in Sunbird 0.9 into a storage calendar and upgrade. The following is reported in the Error Console during update:

[[[
Error: Upgrade to v16 failed! DB Error: User function returned error code ex: [Exception... "'Failure' when calling method: [calIAlarm::offset]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: file:///E:/obj/sb/mozilla/dist/bin/calendar-js/calStorageCalendar.js :: translateAlarm :: line 1334"  data: no]
]]]

[[[
Error: Assert failed: An error was encountered preparing the calendar located at moz-profile-calendar:// for use. It will not be available.
2: [file:///E:/obj/sb/mozilla/dist/bin/calendar-js/calCalendarManager.js:542] cmgr_createCalendar
3: [file:///E:/obj/sb/mozilla/dist/bin/calendar-js/calCalendarManager.js:683] cmgr_assureCache
4: [file:///E:/obj/sb/mozilla/dist/bin/calendar-js/calCalendarManager.js:650] cmgr_getCalendars
5: [null:0] null
6: [file:///E:/obj/sb/mozilla/dist/bin/components/calCompositeCalendar.js:180] anonymous
7: [null:0] null
8: [chrome://calendar/content/calUtils.js:1832] getCompositeCalendar
9: [chrome://calendar/content/calendar-task-tree.xml:154] 

Source File: file:///E:/obj/sb/mozilla/dist/bin/calendar-js/calUtils.js Line: 968
]]]

Comment 77

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Storage Provider support for multiple alarms - v2

Good catch! This patch should fix those issues. If you have an older storage v4 with absolute alarms around, it would be nice if you could test this patch with it.

Comment 78

[Stefan Sitter [:ssitter]]

Comment on attachment 363604 [details] [diff] [review]
Storage Provider support for multiple alarms - v2

Two more findings:

Create an event with an absolute alarm, restart Sunbird - error in console:

[[[
Error: Error getting alarms for item 'New Event' (4e1a38e5-bf5d-44f6-afab-ec0505232ed1)!
[Exception... "'Illegal value' when calling method: [calIEvent::addAlarm]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: file:///E:/sb/calendar-js/calStorageCalendar.js :: anonymous :: line 2279"  data: no]
DB Error: not an error
]]]

Open Sunbird 0.3.1, create event with alarm, upgrade - error in console:

[[[
Error: Upgrade failed! DB Error: table cal_events_v6 has no column named alarm_time
Source File: file:///E:/sb/calendar-js/calStorageCalendar.js
Line: 1180
]]]
[[[
[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [mozIStorageConnection.executeSimpleSQL]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: file:///E:/sb/calendar-js/calStorageCalendar.js :: copyDataOver :: line 1154"  data: no]
]]]

(Note: I did not expected the upgrade from 0.3.1 to work due to bug 470430 but the shown error is new)

Comment 79

[Daniel Boelzle [:dbo]]

Comment on attachment 362089 [details] [diff] [review]
Alarm Service/Monitor and random fixes

>+        try {
>+            notificationbox.removeNotification(setupMaxReminders.notification);
>+        } catch (e) {
>+            // Its ok to swallow this, if the notification element hasn't been
>+            // added then the call will throw a DOM_NOT_FOUND_ERR.
>+            if (e.result != 2152923144 /* NS_ERROR_DOM_NOT_FOUND_ERR */) {
any chance there's a constant/variable available for that?

>+                let related = triggerProp.getParameter("RELATED");
>+                if (related && related == "END") {
IMO a simple |related == "END"| is sufficient

>+                    this.related = ALARM_RELATED_END;
>+                } else {
>+                    this.related = ALARM_RELATED_START;
>+                }
>             }
>-    /* calIAlarmService APIs */
>-    mTimezone: null,
>-    get timezone() {
>+    /**
>+     * calIAlarmService APIs
>+     */
>+    get timezone cAS_get_timezone() {
>+        // TODO Do we really need this? Do we ever set the timezone to something
>+        // different than the default timezone?
>         return this.mTimezone || calendarDefaultTimezone();
>     },

>-    set timezone(aTimezone) {
>+    set timezone cAS_set_timezone(aTimezone) {
>         return (this.mTimezone = aTimezone);
>     },
I don't think we need such an attribute, but I think we need something similar if we want to fix the "change timezone without restart bug" (don't have the id at hand). I can imagine this would require recalculating the timer objects on a timezone change (allday events).
I'd prefer to use an observer notification that the timezone-service fires on timezone changes.

>+    addAlarmsForItem: function cAS_addAlarmsForItem(aItem) {
>+        if (isToDo(aItem) && aItem.isCompleted) {
cal.isToDo ?

>+    removeTimer: function cAS_removeTimers(aItem, aAlarm) {
>+        if (aItem.calendar.id in this.mTimerMap &&
>+            aItem.hashId in this.mTimerMap[aItem.calendar.id] &&
>+            aAlarm.icalString in this.mTimerMap[aItem.calendar.id][aItem.hashId]) {
>+            let timer = this.mTimerMap[aItem.calendar.id][aItem.hashId][aAlarm.icalString];
>+            timer.cancel();
man, this is hard to read

>+
>+            delete this.mTimerMap[aItem.calendar.id][aItem.hashId][aAlarm.icalString];
>+            if (this.mTimerMap[aItem.calendar.id][aItem.hashId].toSource() == "({})") {
>+                delete this.mTimerMap[aItem.calendar.id][aItem.hashId];
>+            }
>+            if (this.mTimerMap[aItem.calendar.id].toSource() == "({})") {
>+                delete this.mTimerMap[aItem.calendar.id];
>             }
>         }
>     },
dto

r=dbo, though untested

Comment 81

[Stefan Sitter [:ssitter]]

Comment on attachment 362089 [details] [diff] [review]
Alarm Service/Monitor and random fixes

I tried to test this patch but it doesn't apply:

> $ hg import --no-commit ../alarms-services.diff
> applying ../alarms-services.diff
> patching file calendar/base/content/calendar-alarm-dialog.js
> Hunk #3 FAILED at 81
> Hunk #5 FAILED at 200
> 2 out of 7 hunks FAILED -- saving rejects to file 
>   calendar/base/content/calendar-alarm-dialog.js.rej
> abort: patch failed to apply

Could you provide an updated version?

Comment 82

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Alarm Service/Monitor and random fixes

Here ya go, updated patch.

Comment 83

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Alarm Service/Monitor and random fixes

(In reply to comment #79)
> (From update of attachment 362089 [details] [diff] [review])
> >+        try {
> >+            notificationbox.removeNotification(setupMaxReminders.notification);
> >+        } catch (e) {
> >+            // Its ok to swallow this, if the notification element hasn't been
> >+            // added then the call will throw a DOM_NOT_FOUND_ERR.
> >+            if (e.result != 2152923144 /* NS_ERROR_DOM_NOT_FOUND_ERR */) {
> any chance there's a constant/variable available for that?
It seems I worked on the patch in the meanwhile, I found a constant for this.


> >+    get timezone cAS_get_timezone() {
> >+        // TODO Do we really need this? Do we ever set the timezone to > I don't think we need such an attribute, but I think we need something similar
> if we want to fix the "change timezone without restart bug" (don't have the id
> at hand). I can imagine this would require recalculating the timer objects on a
> timezone change (allday events).
> I'd prefer to use an observer notification that the timezone-service fires on
> timezone changes.
I think we should fix this in a separate bug, filed bug 484354.


> man, this is hard to read
Added explaining comments to make it simpler to read.

Comment 84

[Stefan Sitter [:ssitter]]

Quick test with attachment 363604 [details] [diff] [review] and attachment 368490 [details] [diff] [review] applied:

[*] Snooze doesn't work anymore. The alarm is handled as dismissed instead. Doesn't matter if I snooze in Sunbird 1.0pre or if I upgrade from Sunbird 0.5/0.7/0.8/0.9 with a snoozed alarm.

[*] The ics shows "DESCRIPTION:Default Mozilla Description" but the alarm dialog does not. Doesn't this violate the iCalendar specification?

<http://tools.ietf.org/html/draft-ietf-calsify-rfc2445bis-09#section-3.6.6>
      When the action is "DISPLAY", the alarm MUST also include a
      "DESCRIPTION" property, which contains the text to be displayed
      when the alarm is triggered.

[*] Upgrade from 0.3.1 fails as mentioned in comment #78. maybe we can ignore the error here and fix it together with bug 470430.

Otherwise it seems to work ok. Only the new custom alarm dialog needs some more usability improvements.

Comment 85

[Philipp Kewisch [:Fallen][📱📆]]

(In reply to comment #78)
> Create an event with an absolute alarm, restart Sunbird - error in console:
> 
I can't reproduce this aprt. Do you have the ics of rhat specific event available?


> Open Sunbird 0.3.1, create event with alarm, upgrade - error in console:
I won't fix this until we fix the update process for old versions as a whole.

Comment 86

[Philipp Kewisch [:Fallen][📱📆]]

(In reply to comment #84)
> [*] Snooze doesn't work anymore. The alarm is handled as dismissed instead.
> Doesn't matter if I snooze in Sunbird 1.0pre or if I upgrade from Sunbird
> 0.5/0.7/0.8/0.9 with a snoozed alarm.
Found the issue, fixed as far as this bug will take it.

> [*] The ics shows "DESCRIPTION:Default Mozilla Description" but the alarm
> dialog does not. Doesn't this violate the iCalendar specification?
The alarm MUST contain this property, but the spec doesn't say anything against not showing it:

 In a DISPLAY alarm, the intended alarm effect is for the text value
   of the "DESCRIPTION" property to be displayed to the user.

Lets leave this to a different bug for thought.

> Otherwise it seems to work ok. Only the new custom alarm dialog needs some more
> usability improvements.
Anything particular you have in mind? Are those issues in bugs yet?

Comment 87

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Storage Provider support for multiple alarms - v3

Alarm issues should be fixed now as noted above. This is the storage patch.

Comment 88

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Alarm Service/Monitor and random fixes - v3

Comment 89

[Stefan Sitter [:ssitter]]

Regarding DISPLAY alarm and DESCRIPTION: 
We do not use it currently, see Bug 327992. But what about other applications that might display ics files created by Sunbird? What about future Sunbird/Lightning releases that might use it? We should revert to create a correct DESCRIPTION property instead of using "Default Mozilla Description". Sunbird 0.9 used the event title in the DESCRIPTION property.

Regarding custom alarm dialog: 
In my opinion the creation of new alarms takes too much clicks: 1st click to open the dialog, 2nd click to add reminder, 3rd and 4th click to set unit from days to minutes or hours, 5th click to change the duration from 0 to another value, 6th click to save the reminder.
Ideally a new reminder should be added automatically when opening the dialog and no custom reminder was set before. The new alarm has suitable default values right after creation. Maybe use the default alarm time from configuration dialog.
No bugs filed yet.

Comment 90

[Stefan Sitter [:ssitter]]

Comment on attachment 369819 [details] [diff] [review]
Storage Provider support for multiple alarms - v3

With this patch I still see the error mentioned in Comment #78:
1. start sunbird with clean profile
2. create new event in default storage calendar and set a custom absolute alarm
3. restart application

After creation the event looks like:
    BEGIN:VEVENT
    CREATED:20090329T175301Z
    LAST-MODIFIED:20090329T175408Z
    DTSTAMP:20090329T175408Z
    UID:41eaf810-7410-4cea-9ea3-77d1133c1446
    SUMMARY:TEST W/ ABSOLUTE ALARM
    DTSTART;TZID=Europe/Berlin:20090401T160000
    DTEND;TZID=Europe/Berlin:20090401T170000
    BEGIN:VALARM
    ACTION:DISPLAY
    TRIGGER;VALUE=DATE-TIME:20090331T120000Z
    DESCRIPTION:Default Mozilla Description
    END:VALARM
    END:VEVENT

After application restart the event looks like:
    BEGIN:VEVENT
    CREATED:20090329T175301Z
    LAST-MODIFIED:20090329T175408Z
    DTSTAMP:20090329T175408Z
    UID:41eaf810-7410-4cea-9ea3-77d1133c1446
    SUMMARY:TEST W/ ABSOLUTE ALARM
    DTSTART;TZID=Europe/Berlin:20090401T160000
    DTEND;TZID=Europe/Berlin:20090401T170000
    END:VEVENT

Console shows:

Error: Error getting alarms for item 'TEST W/ ABSOLUTE ALARM' (41eaf810-7410-4cea-9ea3-77d1133c1446)!
[Exception... "'Illegal value' when calling method: [calIEvent::addAlarm]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: file:///E:/sb/calendar-js/calStorageCalendar.js :: anonymous :: line 2279"  data: no]
DB Error: not an error


Adding some dump() statements at the corresponding error location I get:

Dumping |row.icalString| shows the correct alarm as in the ics above.

Dumping |alarm| fails with:

[Exception... "'Component not initialized' when calling method: [calIAlarm::icalString]"  nsresult: "0xc1f30001 (NS_ERROR_NOT_INITIALIZED)"  location: "JS frame :: file:///E:/sb/calendar-js/calStorageCalendar.js :: anonymous :: line 2278"  data: no]

Dumping |alarm.icalString| fails with: 

[Exception... "'Component not initialized' when calling method: [calIAlarm::icalString]"  nsresult: "0xc1f30001 (NS_ERROR_NOT_INITIALIZED)"  location: "JS frame :: file:///E:/sb/calendar-js/calStorageCalendar.js :: anonymous :: line 2278"  data: no]

Looks like the alarm doesn't get correctly initialized. See Bug Bug 485571 for similar issue.

Comment 91

[Stefan Sitter [:ssitter]]

Correction: Dumping |alarm| fails with:
[Exception... "Not enough arguments [calIAlarm.toString]"  nsresult: "0x80570001 (NS_ERROR_XPC_NOT_ENOUGH_ARGS)"  location: "JS frame :: file:///E:/sb/calendar-js/calStorageCalendar.js :: anonymous :: line 2278"  data: no]

Comment 92

[Stefan Sitter [:ssitter]]

Comment on attachment 369819 [details] [diff] [review]
Storage Provider support for multiple alarms - v3

r=ssitter to get this landed despite the mentioned errors. The upgrade issue can be followed up with bug 470430. The absolute alarm issue can be followed up with bug 485570 / bug 485571 but should be fixed before a upcoming 1.0 beta release I think.

Comment 93

[Philipp Kewisch [:Fallen][📱📆]]

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/cce4879cdf36>
                   and <http://hg.mozilla.org/comm-central/rev/88092e4011eb>
                   and <http://hg.mozilla.org/comm-central/rev/a8efffb40b43>

-> FIXED (woohoo!)

I still could not reproduce the issue from comment 78. Although I doubt it, maybe its a platform issue. Lets hope the number of regressions isn't that high. I fixed the possible exception mentioned in comment 91.

Comment 94

[Philipp Kewisch [:Fallen][📱📆]]

These bugs are likely targeted at Lightning 1.0b1, not Lightning 1.0. If this change was done in error, please adjust the target milestone to its correct value. To filter on this bugspam, you can use "lightning-10-target-move".