389245 - Clean up undismissed alarms on item modification/deletion

Status: VERIFIED FIXED

(Calendar :: Alarms, defect)

Description

[Daniel Boelzle [:dbo]]

The current alarm monitors (sunbird, lightning) just add new alarms to the open alarm dialog, but never update/remove dangling undismissed ones if an item has been modified/removed. IMO a potential dataloss may be caused:

1. create an item, set an alarm and let it fire
2. modify the item, e.g. changing title => a second alarm will fire
3. dismiss the first alarm
=> dataloss

Comment 1

[Stefan Sitter [:ssitter]]

Seems to be the same alarm dialog issue as in Bug 352024 and Bug 371365.

Comment 2

[Daniel Boelzle [:dbo]]

Attachment: fix + alarm monitor consolidation

- fixes bug: cleanup when refreshing, modifying
- consolidates alarm monitors into one
- encapsulates snooze logic details in alarm service

WFM quite good, further to be tested.

Comment 3

[Daniel Boelzle [:dbo]]

Attachment: fix + alarm monitor consolidation

different approach: now reopening/refilling the alarm window (is user closed it) works, too.

Comment 4

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

Comment on attachment 276203 [details] [diff] [review]
fix + alarm monitor consolidation

Comments are mostly style, r+ with these fixed.


There are two redundant whitespaces here:
mozilla/calendar/base/src/calAlarmMonitor.js:58: Trailing whitespace
+    // alarm is fired in that time-frame, it will actually end up in another window.

mozilla/calendar/base/src/calAlarmService.js:247: Trailing whitespace
+        // We want the parent item, otherwise we're going to accidentally create an



>Index: mozilla/calendar/base/content/calendar-alarm-dialog.js
>+function getAlarmService() {
>+    if (!window.mAlarmService) {
>+        window.mAlarmService = Components.classes["@mozilla.org/calendar/alarm-service;1"]
>+                                         .getService(Components.interfaces.calIAlarmService);
The alarm dialog uses calUtils, shorten to Cc and Ci here and elsewhere.

>+    for (var i = nodes.length; i--;) {
This line may work fine, but something like
for (var i = nodes.length - 1; i > -1; i--)
looks less abusive to my mind

>+                // check again next round since this may only be a refresh:
I dont quite understand what kind of refresh you mean, but I assume you know what you are doing ;)


> }
>+
Remove the blank line at eof.


>Index: mozilla/calendar/base/public/calIAlarmService.idl
While you are at it, please remove the emacs modeline in this file

> [scriptable,uuid(c9c97643-db45-4790-9441-05384ae5c272)]
> [scriptable,uuid(03669cf3-bf4f-4692-97a1-cca891964a1d)]
When changing the interface, please also change the uuid


>Index: mozilla/calendar/base/src/calAlarmMonitor.js
>+/* -*- Mode: javascript; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
No modelines please

>+function peekAlarmWindow() {
>+    var windowMediator = Components.classes["@mozilla.org/appshell/window-mediator;1"]
>+                                   .getService(Components.interfaces.nsIWindowMediator);
>+    return windowMediator.getMostRecentWindow("calendarAlarmWindow");
>+}
Please convert to Cc/Ci here and elsewhere if possible.

>+            this.mWindowOpening = windowWatcher.openWindow(
>+                null,
>+                "chrome://calendar/content/calendar-alarm-dialog.xul",
>+                "_blank",
>+                "chrome,dialog=yes,all,resizable",
>+                null);
>+            var this_ = this;
>+            function window_onLoad() {
>+                for each (var item in this_.mAlarmItems) {
>+                    this_.mWindowOpening.addWidgetFor(item);
>+                }
>+                this_.mWindowOpening = null;
>+            }
>+            this.mWindowOpening.addEventListener("load", window_onLoad, false);
Might this cause race conditions if the window loads faster than the listener is added? Can't we just pass the items to the dialog and let the dialog take care of adding the widgets?

>Index: mozilla/calendar/base/src/calAlarmService.js
>+    this.mObservers = new calListenerBag(Components.interfaces.calIAlarmServiceObserver);
Also use Cc/Ci in this file

>             if (calendar && this.alarmService.mLoadedCalendars[calendar.id]) {
>+                // onLoad signals that changes to the item set are unknown, so purge out all
>+                // alarms belonging to the refreshed/loaded calendar:
>+                this.alarmService.notifyObservers("onRemoveAlarmsByCalendar", [calendar]);
>                 // a refreshed calendar signals that it has been reloaded
>                 // (and cannot notify detailed changes), thus reget all alarms of it:
>                 this.alarmService.initAlarms([calendar]);
What happens if there are no alarms left after the refresh? Please make sure the dialog closes in that case.


>-    snoozeEvent: function(event, duration) {
>+    snoozeAlarm: function(event, duration) {
Please give the function a name.

>-            newEvent.setProperty("X-MOZ-SNOOZE-TIME-"+event.recurrenceId.nativeTime, alarmTime.icalString);
>+            newEvent.setProperty("X-MOZ-SNOOZE-TIME-"+event.recurrenceId.nativeTime,
>+                                 alarmTime.icalString);
Spaces before and after operators.

>+        // We want the parent item, otherwise we're going to accidentally create an 
>+        // exception.  We've relnoted (for 0.1) the slightly odd behavior this can
>+        // cause if you move an event after dismissing an alarm
Do we have a bug# to fix this odd behavior? If you have time to find it, please add it to these comments.

> dump("snooze time is:"+snoozeTime+'\n');
If you feel like it, replace all these dump calls with LOG calls, so we don't have that info in the console unless calendar.debug.log is enabled.



>+EXTRA_COMPONENTS = \
>+	content/calendarService.js \
>+	$(NULL)
Since this is only one component, its sufficient to just put it on the EXTRA_COMPONENTS line and remove the $(NULL) line.

Comment 5

[Stefan Sitter [:ssitter]]

(In reply to comment #4)
> Please convert to Cc/Ci here and elsewhere if possible.
> Also use Cc/Ci in this file

I'd recommend to not use Cc/Ci to avoid adding more code places that needs to be fixed with Bug 390842 later.

Comment 6

[Daniel Boelzle [:dbo]]

(In reply to comment #4)
> >+            this.mWindowOpening = windowWatcher.openWindow(
> >+                null,
> >+                "chrome://calendar/content/calendar-alarm-dialog.xul",
> >+                "_blank",
> >+                "chrome,dialog=yes,all,resizable",
> >+                null);
> >+            var this_ = this;
> >+            function window_onLoad() {
> >+                for each (var item in this_.mAlarmItems) {
> >+                    this_.mWindowOpening.addWidgetFor(item);
> >+                }
> >+                this_.mWindowOpening = null;
> >+            }
> >+            this.mWindowOpening.addEventListener("load", window_onLoad, false);
> Might this cause race conditions if the window loads faster than the listener
> is added? Can't we just pass the items to the dialog and let the dialog take
> care of adding the widgets?
Good point; although the previous code did it the same, I can imagine the handler may not be called, because onload has already be fired. I changed the dialog to call into the alarm monitor in its onload. Passing the items does not work, because more items may be signaled while loading the dialog.

> >+                // onLoad signals that changes to the item set are unknown, so purge out all
> >+                // alarms belonging to the refreshed/loaded calendar:
> >+                this.alarmService.notifyObservers("onRemoveAlarmsByCalendar", [calendar]);
> >                 // a refreshed calendar signals that it has been reloaded
> >                 // (and cannot notify detailed changes), thus reget all alarms of it:
> >                 this.alarmService.initAlarms([calendar]);
> What happens if there are no alarms left after the refresh? Please make sure
> the dialog closes in that case.
The last removed widget will close the dialog in removeWidgetFor().

Comment 7

[Daniel Boelzle [:dbo]]

Attachment: correcting nits, different onload

Comment 8

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

Comment on attachment 276751 [details] [diff] [review]
correcting nits, different onload

>+        onload="window.arguments[0].wrappedJSObject.window_onLoad();"
Is there any way we can get around using wrappedJSObject? We should try to stick to the implementation or change it, but if we use wrappedJSObject then we are working around it.

Comment 9

[Daniel Boelzle [:dbo]]

(In reply to comment #8)
I don't see an option except for defining IDL (which we most probably don't like).

Comment 10

[Daniel Boelzle [:dbo]]

Checked in on HEAD and MOZILLA_1_8_BRANCH. Please verify the bugs mentioned in comment #1, too.

Comment 12

[Damian Szczepanik]

verified with
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.7pre) Gecko/20070908 Calendar/0.7pre