Clean up undismissed alarms on item modification/deletion

VERIFIED FIXED in 0.7

Status

VERIFIED FIXED
11 years ago
11 years ago

People

(Reporter: dbo, Assigned: dbo)

Tracking

({dataloss})

unspecified
dataloss
Dependency tree / graph
Bug Flags:
blocking-calendar0.7 +
in-litmus ?

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

11 years ago
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
Flags: blocking-calendar0.7+
Seems to be the same alarm dialog issue as in Bug 352024 and Bug 371365.
(Assignee)

Updated

11 years ago
Assignee: nobody → daniel.boelzle
(Assignee)

Comment 2

11 years ago
Created attachment 276136 [details] [diff] [review]
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.
(Assignee)

Updated

11 years ago
Status: NEW → ASSIGNED
Whiteboard: [patch in hand]
Target Milestone: --- → 0.7
(Assignee)

Comment 3

11 years ago
Created attachment 276203 [details] [diff] [review]
fix + alarm monitor consolidation

different approach: now reopening/refilling the alarm window (is user closed it) works, too.
Attachment #276136 - Attachment is obsolete: true
(Assignee)

Updated

11 years ago
Attachment #276203 - Flags: review?(philipp)
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.
Attachment #276203 - Flags: review?(philipp) → review+
(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.
(Assignee)

Comment 6

11 years ago
(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().
(Assignee)

Comment 7

11 years ago
Created attachment 276751 [details] [diff] [review]
correcting nits, different onload
Attachment #276751 - Flags: review?(philipp)
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.
(Assignee)

Comment 9

11 years ago
(In reply to comment #8)
I don't see an option except for defining IDL (which we most probably don't like).
Attachment #276751 - Flags: review?(philipp) → review+
(Assignee)

Comment 10

11 years ago
Checked in on HEAD and MOZILLA_1_8_BRANCH. Please verify the bugs mentioned in comment #1, too.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Whiteboard: [patch in hand]
Duplicate of this bug: 352024
Depends on: 395437
Blocks: 371365

Comment 12

11 years ago
verified with
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.7pre) Gecko/20070908 Calendar/0.7pre
Status: RESOLVED → VERIFIED
Flags: in-litmus?
You need to log in before you can comment on or make changes to this bug.