861594 - Allow alarm acknowledgement status to be stored locally

Status: NEW

(Calendar :: Alarms, defect)

Description

[Matthew Mecca [:mmecca]]

Providing an option to track alarm dismissal and snooze status in a local database will allow alarms to function normally in read-only calendars and with providers/servers that don't fully support X-MOZ props.

Comment 1

[Matthew Mecca [:mmecca]]

Attachment: WIP v1

WIP patch that creates a local database to track alarm acks. I made this a per-calendar pref that is off by default, but I wonder if it would be better to track alarms locally by default and offer a pref for shared alarms. What do you think?

Comment 2

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

Comment on attachment 737211 [details] [diff] [review]
WIP v1

Review of attachment 737211 [details] [diff] [review]:
-----------------------------------------------------------------

Great stuff, you will make a lot of users happy with this patch. I think you are going in the right direction with this patch, but I think you might need to do some more deep integration in calItemBase. We especially need to check code that uses X-MOZ-LASTACK and the snooze times if it makes use of the local alarms correctly. Ideally, this happens as transparent as possible, making it harder for new code to wrongfully use the remote properties.

Also, what happens when the checkbox is enabled? There should be some sort of migration path, otherwise I believe all alarms will be fired again because they are not yet in the local database? Again we need to find a good solution for migration, I see possibilities for confusion. If we assume the X-MOZ properties are authoritative and the calendar is shared, someone else may dismiss the alarm and the current user will miss the alarm because he assumes he is using local reminders.

There may be additional implications, but I think its a good idea to use local alarms by default. Maybe you can make it so that local alarms are used for all new calendars, but not for existing calendars. This way we don't surprise any users, but still give them the possibility to switch.

f++

::: calendar/base/public/calILocalAlarms.idl
@@ +30,5 @@
> +     *                         or null if an alarm has not been acknowledged.
> +     * @param aSnoozeTime      Returns the date an alarm was snoozed to, or null
> +     *                         if an alarm has not been snoozed.
> +     */
> +    void getAcks(in calIItemBase aItem, out calIDateTime aLastAck, out calIDateTime aSnoozeTime);

Do you think we should handle acks per alarm instead of per item? If we at some point start displaying the description of each alarm, then one might want to dismiss only the one or other alarm, but not all of them at once.

I understand why you're going this direction though, its been like that before too. Just something to consider and keep in mind.

::: calendar/base/src/calAlarmService.js
@@ +194,5 @@
>      set timezone(aTimezone) {
>          return (this.mTimezone = aTimezone);
>      },
>  
> +    localAlarmManager: function() Components.classes["@mozilla.org/calendar/local-alarms-manager;1"]

Consider moving this to calUtils instead.

@@ +212,5 @@
> +        alarmTime.addDuration(aDuration);
> +
> +        if (this.itemAlarmsLocal(aItem)) {
> +            this.localAlarmManager().setAcks(aItem, ackTime, alarmTime);
> +            return {};

If there is no async operation to cancel, then I think you can just return null.

@@ +238,5 @@
>          let now = nowUTC();
> +
> +        if (this.itemAlarmsLocal(aItem)) {
> +            this.localAlarmManager().setAcks(aItem, now, null);
> +            return {};

Same here

::: calendar/base/src/calLocalAlarms.js
@@ +16,5 @@
> +
> +    classID: Components.ID("{3b4b321e-a2a7-11e2-8d1c-000cf1d75a68}"),
> +    QueryInterface: XPCOMUtils.generateQI([Components.interfaces.calILocalAlarms,
> +                                           Components.interfaces.nsIObserver,
> +                                           Components.interfaces.calIObserver]),

Consider moving interfaces and classid out, i.e.

function calLocalAlarms() {
}

const calLocalAlarmsInterfaces = [ ... ];
const calLocalAlarmsClassID = Components.ID(...);
calLocalAlarms.prototype = {
...

@@ +24,5 @@
> +        classDescription: "Database containing local alarm acknowledgement",
> +        interfaces: [Components.interfaces.calILocalAlarms,
> +                     Components.interfaces.nsIObserver,
> +                     Components.interfaces.calIObserver],
> +        flags: Components.interfaces.nsIClassInfo.SINGLETON,

Remove comma at end of last line.

@@ +91,5 @@
> +        let listener = {
> +            handleCompletion: function() {
> +                // The item data isn't actually being modified here, but some observers expect
> +                // to be notified that the alarm status has changed.
> +                aItem.calendar.wrappedJSObject.mObservers.notify("onModifyItem", [aItem, aItem]);

Hmm, using wrappedJSObject doesn't look so clean. Unfortunately I don't have a good alternative to offer at the moment.

Maybe if you integrate into calItemBase, this line will not be needed because the provider modifyItem will be called anyway.

::: calendar/locales/en-US/chrome/calendar/calendar.dtd
@@ +307,5 @@
>  <!ENTITY calendarproperties.refreshInterval.manual.label   "Manually">
>  <!ENTITY calendarproperties.name.label                     "Name:">
>  <!ENTITY calendarproperties.readonly.label                 "Read Only">
>  <!ENTITY calendarproperties.firealarms.label               "Show Reminders">
> +<!ENTITY calendarproperties.localalarms.label              "Local Reminders">

I suggest "Keep Reminders Local"

Comment 3

[Peter Lairo]

I'm glad to hear this will be an option (unfortunately ON by default). I wonder how this will impact the many users who access their CalDAV calendars from their PC, their Smartphone, and their tablet. They will have to dismiss every alarm at least twice (PC and Smartphone) (maybe more, if they have a work PC and a home PC).

If the user turns the local alarms OFF, will he then be affected by the current Google bug (bug 451821)?

Comment 4

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

Turning this on by default will really only impact those who use two copies of Lightning. Smartphones do not make use of X-MOZ- properties, so alarms will have to be dismissed there anwyay.

Also, if my suggestion is followed to only turn it on by default for newly created calendars, it will not impact existing users at all until they create a new calendar.

Obviously if local alarms are off (or more explicitly, remote alarms are on) then they will be affected by the Google bug. I think that bug will be fixed until TB24 though.

Comment 5

[Michel Verbraak]

1. Turning this feature on by default will impact a lot of users who share calendars but with different rights to the calendar. For example the following case in an Exchange calendar:

A calendar where different people have author rights to. Author rights means, in Exchange, they have read access to all items in a calendar but are only allowed to change/remove items they have created. For example: an assistant who is allowed to add items in a calendar of her/his boss. She/he has added the calendar of her/his boss to Lightning and the assistant maintains work related items in the calendar of the boss and she/he would also like to modify the alarms on these items but her/his boss also puts private items in his own calendar with alarms which his assistant does not have rights to change and even does not have to see an alarm signal for. The assistant needs to be reminded of the alarms she/he put into the boss calendar to prepare stuff for the meetings the boss has.

This fix stores all alarms for all items in a calendar in the localAlarmDatabase or not. It cannot discriminate between r/w items and read-only items.

2. As an extension to comment 3 and 4. How are changes done to remotely stored alarms going to be followed in the localAlarmsDB? I see it is a TODO but this needs to be added.

Comment 6

[Matthew Mecca [:mmecca]]

(In reply to Michel Verbraak from comment #5)

> A calendar where different people have author rights to. Author rights
> means, in Exchange, they have read access to all items in a calendar but are
> only allowed to change/remove items they have created. For example: an
> assistant who is allowed to add items in a calendar of her/his boss. She/he
> has added the calendar of her/his boss to Lightning and the assistant
> maintains work related items in the calendar of the boss and she/he would
> also like to modify the alarms on these items but her/his boss also puts
> private items in his own calendar with alarms which his assistant does not
> have rights to change and even does not have to see an alarm signal for. The
> assistant needs to be reminded of the alarms she/he put into the boss
> calendar to prepare stuff for the meetings the boss has.

Is that how alarms are handled in other clients?

> This fix stores all alarms for all items in a calendar in the
> localAlarmDatabase or not. It cannot discriminate between r/w items and
> read-only items.

It wouldn't be too hard to do a check at snooze/dismiss time to test write access to the item and use the local alarm db accordingly, maybe even regardless of the calendar pref.

> 2. As an extension to comment 3 and 4. How are changes done to remotely
> stored alarms going to be followed in the localAlarmsDB? I see it is a TODO
> but this needs to be added.

Agreed.

Comment 7

[Michel Verbraak]

(In reply to Matthew Mecca [:mmecca] from comment #6)
> (In reply to Michel Verbraak from comment #5)
> 
> > A calendar where different people have author rights to. Author rights
> > means, in Exchange, they have read access to all items in a calendar but are
> > only allowed to change/remove items they have created. For example: an
> > assistant who is allowed to add items in a calendar of her/his boss. She/he
> > has added the calendar of her/his boss to Lightning and the assistant
> > maintains work related items in the calendar of the boss and she/he would
> > also like to modify the alarms on these items but her/his boss also puts
> > private items in his own calendar with alarms which his assistant does not
> > have rights to change and even does not have to see an alarm signal for. The
> > assistant needs to be reminded of the alarms she/he put into the boss
> > calendar to prepare stuff for the meetings the boss has.
> 
> Is that how alarms are handled in other clients?
> 
Just tested in outlook 2010. It acts in the following way:
- Assistant adds a new item to the calendar of the boss with an alarm.
- The alarm only goes of in the outlook the boss has running and not in the outlook session of the assistant. This behaviour is the same after a restart of outlook for both. 
- The assistant can still change the alarm setting on the item.
- So by default outlook does not show alarms in shared calendars. This is different than I explained in comment 5 but that might be a desired solution.

As an extra: Maybe the property also needs to be made available during calendar creation on the same wizard page where you can turn on/off show reminders?

Comment 8

[Charles]

So... is this still being worked on? It sounds like it was very close to being ready, and this would be very handy for our new SOGo setup when dealing with Shared Calendars that users have read-only access to.

Comment 9

[Matthew Mecca [:mmecca]]

I think we should resolve bug 702206 before moving forward with this bug. If the intent of the emerging specs [1] is to improve alarm interop across clients and servers, I'm not sure it makes sense to move toward local-only alarm handling here (especially not by default). We could still make use of local alarm handling for read-only calendars though.

[1]: https://tools.ietf.org/html/draft-daboo-valarm-extensions-04

Comment 10

[Charles]

Sounds good to me, but... it looks like maybe progress has stalled for some reason? :(

Comment 11

[Lukas Barth]

Is there any chance of this getting in? As far as I can see it, this stalls #356002 ( see https://bugzilla.mozilla.org/show_bug.cgi?id=356002#c87 ), and that in turn makes Lightning unusable with a read-only ICS calendar. Those notifications fire every 10 minutes or so without any way of dismissing them.. :(