Closed Bug 672816 Opened 8 years ago Closed 8 years ago

Snoozing/Dismissing one event on alarm popup closes entire window

Categories

(Calendar :: Alarms, defect, critical)

Lightning 1.0b4
All
Other
defect
Not set
critical

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: donrhummy, Assigned: donrhummy)

Details

(Whiteboard: [needed beta][no l10n impact])

Attachments

(1 file)

User Agent: Mozilla/5.0 (X11; Linux i686; rv:5.0) Gecko/20100101 Firefox/5.0
Build ID: 20110615151330

Steps to reproduce:

I had three events pop up in the alarm window, I chose the "snooze" next to one of the events only, yet it closed the entire window. The same thing happens if I choose "dismiss" next to one of the events.


Actual results:

It closed the entire window so I could not make choices for the other events.

NOTE: Although it closes the window (and I've found no way to get it back up), the other events are not also snoozed or dismissed as when another alarm comes up later, they still show up. (But again, the same thing will occur then)


Expected results:

That one event should have disappeared and left the window open so I could make choices on all the events.
Severity: normal → critical
Is it possible for you to retest with release candidate of Lightning 1.0b5? It should contain some reminder related fixes but I'm not sure if your problem is covered.
https://ftp.mozilla.org/pub/mozilla.org/calendar/lightning/releases/1.0b5rc1/
(In reply to comment #1)
> Is it possible for you to retest with release candidate of Lightning 1.0b5?
> It should contain some reminder related fixes but I'm not sure if your
> problem is covered.
> https://ftp.mozilla.org/pub/mozilla.org/calendar/lightning/releases/1.0b5rc1/

Sure, I'll give that a try. Just to be sure, is there any way to back up my settings so I can be sure they don't get lost if anything goes wrong?

Also, what's the best way to try this? Do I just go to addons->wrench icon->Install Addons From File? Will that properly update lightning or will it install two of them side by side?
You can backup your profile as a whole, see http://support.mozillamessaging.com/en-US/kb/Profiles for where it is. I doubt anything will go badly wrong, given the bugs that have been fixed in the meanwhile, things can only get better!

Just install it as you described, it will upgrade the previous lightning.
Nope, same issue occurs. I chose "Snooze 5 minutes" for the first item and it clears the window, then for a split second I see the last two pop back into the window and then the window disappears.

Also, the alarm sound is still freezing everything (I use a custom sound) until it's done playing. It needs to be in a different thread.
Just to clarify, "it clears the window, then for a split second I see the last two pop back into the window and then the window disappears" is exactly what happened in 1.0b4. For some reason I neglected to mention the split second "re-showing" of the remaining items before the window disappears. So there's no change from b4 to b5.
> Also, the alarm sound is still freezing everything (I use a custom sound)
> until it's done playing. It needs to be in a different thread.

This is a problem in the Mozilla Toolkit used by Thunderbird, see Bug 110385.
(In reply to comment #6)
> > Also, the alarm sound is still freezing everything (I use a custom sound)
> > until it's done playing. It needs to be in a different thread.
> 
> This is a problem in the Mozilla Toolkit used by Thunderbird, see Bug 110385.

Can you point me to the file where the code is for handling snoozing/dismissing of events in that window? I assume it's javascript + XUL? I'd be happy to take a look.
I fixed it!!!

The FIX:

(File: chrome/calendar/content/calendar/calendar-alarm-dialog.js)

Change line 315 from:
     setTimeout(closer, 0);

To:
    setTimeout(closer, 250);

The problem was that it was setting the close event too quickly. It was checking to see if any events were readded after a refresh but at 0 milliseconds, the dialog didn't have time to re-add the new child nodes, so when the function closer checks for "alarmRichlist.hasChildNodes()" it gets a "false". With the 250ms wait on the setTimeout, it now has time to add them before the child nodes check.
Hmm interesting. The idea of setTimeout(..., 0) is that it postpones the check by processing other javascript events first and putting the current one on the end of the event queue. The calls to add/remove the events happend in direct sequence, so at least in theory the close check shouldn't go inbetween there. I guess we could increase the timeout, but in the long run we need to find a different solution since relying on timeouts wasn't a good idea in the first place.
I'll put up a patch based on this and push it for 1.0b5rc2
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking-calendar1.0+
(In reply to comment #10)
> I'll put up a patch based on this and push it for 1.0b5rc2

Great!

I agree on the timeout. I haven't really looked through all the code but I think there's got to be a better way to handle this. Perhaps have the window do the listening for when items are removed and items are being refreshed:

1. Window listens to "DOMNodeRemoved" to know when child nodes are removed
   a. in that listener it checks for a  boolean set on itself "itemsRefreshing" that is set to true when it's refreshing for items' alarms
   b. If itemsRefreshing == false, then it starts an interval that continually checks until itemsRefreshing == false and then (when it's false) rechecks for whether it has child nodes. If none at that point, it closes.

Again, I haven't looked at the code, but something like that should take care of the issue.
Whiteboard: [needed beta][no l10n impact]
(In reply to comment #11)
> I agree on the timeout. I haven't really looked through all the code but I
> think there's got to be a better way to handle this. Perhaps have the window
> do the listening for when items are removed and items are being refreshed:
> 
> 1. Window listens to "DOMNodeRemoved" to know when child nodes are removed
>    a. in that listener it checks for a  boolean set on itself
> "itemsRefreshing" that is set to true when it's refreshing for items' alarms
>    b. If itemsRefreshing == false, then it starts an interval that
> continually checks until itemsRefreshing == false and then (when it's false)
> rechecks for whether it has child nodes. If none at that point, it closes.
> 
> Again, I haven't looked at the code, but something like that should take
> care of the issue.

There are a few issues with that approach. First of all, we'd like to keep the alarm service and alarm monitor separate entities. The alarm service notifies that alarms are fired and keeps the timers, while the alarm monitor recieves those notifications and shows a dialog specifically for display alarms. The idea behind this is that we could theoretically write an email alarm monitor, that looks for email alarms and uses Thunderbird identities to send an alarm reminder email.

Second, items refreshing is not something with a defined start and end point. If you have more than one calendar, then you would want to set itemsRefreshing=false when the last calendar is done refreshing. If thats a network calendar with 120 seconds timeout, then you might have an empty alarm window for 120 seconds, which is not what the user wants to see.
Attached patch Fix - v1 β€” β€” Splinter Review
This is Don's patch as a diff, r=philipp
Attachment #548115 - Flags: review+
Assignee: nobody → donrhummy
Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/d5ef3936415b>
-> FIXED
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Trunk
Backported to comm-miramar <http://hg.mozilla.org/releases/comm-miramar/rev/54040606dd74>
Target Milestone: Trunk → 1.0b5
(In reply to comment #12)
> (In reply to comment #11)
> > I agree on the timeout. I haven't really looked through all the code but I
> > think there's got to be a better way to handle this. Perhaps have the window
> > do the listening for when items are removed and items are being refreshed:
> > 
> > 1. Window listens to "DOMNodeRemoved" to know when child nodes are removed
> >    a. in that listener it checks for a  boolean set on itself
> > "itemsRefreshing" that is set to true when it's refreshing for items' alarms
> >    b. If itemsRefreshing == false, then it starts an interval that
> > continually checks until itemsRefreshing == false and then (when it's false)
> > rechecks for whether it has child nodes. If none at that point, it closes.
> > 
> > Again, I haven't looked at the code, but something like that should take
> > care of the issue.
> 
> There are a few issues with that approach. First of all, we'd like to keep
> the alarm service and alarm monitor separate entities. The alarm service
> notifies that alarms are fired and keeps the timers, while the alarm monitor
> recieves those notifications and shows a dialog specifically for display
> alarms. The idea behind this is that we could theoretically write an email
> alarm monitor, that looks for email alarms and uses Thunderbird identities
> to send an alarm reminder email.

I'm not sure quite how this pertains as I'm not talking about altering the service or monitor, but the dialog/window's code only.

> Second, items refreshing is not something with a defined start and end
> point. If you have more than one calendar, then you would want to set
> itemsRefreshing=false when the last calendar is done refreshing. If thats a
> network calendar with 120 seconds timeout, then you might have an empty
> alarm window for 120 seconds, which is not what the user wants to see.

I agree with this and it brings me back to my original confusion with the code in general: why is it even refreshing anything? Just because I snoozed Alarm #1, why should Alarm #2, #3 have to be refreshed? Nothing could possibly have changed in such a way from me making a decision on Alarm #1 that requires different rules on the "freshness" of an alarm versus just sitting, looking at the window and reading each alarm. 

In other words, if I don't make a decision for 5 minutes and just leave the window/alarms up there, it doesn't keep refreshing them every 5 seconds. Instead, the alarms will sit there growing "stale" (which I think is the correct action) until I make a decision on them. So why does making a decision on one of the alarms change the rules of "staleness" vs "freshness" such that it forces a refresh on all other alarms?

The whole issue would be resolved if the other alarms simply weren't refreshed.
There's also another problem with having it refresh on every "decision:" it plays an alarm sound for every event in the window. So if you have 6 events, and you deal with them one at a time, you'll have the alarm sound play 21 times!

1. First time pops up: plays 6 alarms
2. You dismiss the first alarm, it refreshes the other 5, plays 5 alarms
3. You snooze the next alarm, it refreshes the other 4, plays 4 alarms
4. You snooze the next alarm, it refreshes the other 3, plays 3 alarms
5. You snooze the next alarm, it refreshes the other 2, plays 2 alarms
6. You snooze the next alarm, it refreshes the other 1, plays 1 alarms
                                                       ---------------
                                              TOTAL:    plays 21 alarms
Unfortunate, but we can't fix this in time for 1.0b5. I'd say this is bug 673562
(In reply to comment #19)
> Unfortunate, but we can't fix this in time for 1.0b5. I'd say this is bug
> 673562

That's also a problem but I think this is different. Mainly that it refreshes unrelated alarms after snoozing/dismissing one alarm. Given that every time an alarm is refreshed it plays the sound, this leads to an insane number of plays.

This bug is what it is: https://bugzilla.mozilla.org/show_bug.cgi?id=674337
You need to log in before you can comment on or make changes to this bug.