Last Comment Bug 355755 - Alarms are dismissed when Sunbird is closed
: Alarms are dismissed when Sunbird is closed
Status: VERIFIED FIXED
:
Product: Calendar
Classification: Client Software
Component: Alarms (show other bugs)
: unspecified
: All All
: -- normal with 1 vote (vote)
: Sunbird 0.5
Assigned To: Daniel Boelzle [:dbo]
:
Mentors:
: 243144 358347 366049 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-10-06 12:06 PDT by Telmo Amaral
Modified: 2008-04-01 03:15 PDT (History)
9 users (show)
cmtalbert: in‑litmus?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
no dismissAll when unloading alarm dialog (1.11 KB, patch)
2006-10-27 03:00 PDT, Daniel Boelzle [:dbo]
jminta: first‑review+
Details | Diff | Review
IRC discussion on this topic (4.88 KB, text/plain)
2006-10-28 14:26 PDT, Stefan Sitter
no flags Details
no dismissAll when unloading alarm dialog (1.10 KB, patch)
2006-10-31 12:06 PST, Daniel Boelzle [:dbo]
jminta: first‑review+
mvl: ui‑review+
Details | Diff | Review
more irc discussion (20.64 KB, text/html)
2006-12-06 15:13 PST, Dan Mosedale (:dmose)
no flags Details

Description Telmo Amaral 2006-10-06 12:06:11 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.7) Gecko/20060909 Firefox/1.5.0.7
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20061002 Sunbird/0.3

Closing SB while the Calendar Alarms window is open causes all the shown alarms to be dismissed. That is, the next time SB is launched, those alarms won't show up again. This seems consistent with the fact that closing just the Calendar Alarms window (by clicking the X button on the title bar or by pressing Esc) also dismisses all the alarms. These situations are undesirable. Closing Sunbird or the alarms window should snooze all the alarms, not dismiss them.

Reproducible: Always

Steps to Reproduce:
1. Create a couple of events in the past, with an immediate alarm; the Calendar Alarms window shows up, displaying the alarms.
2. Go back to Sunbird's main window and do File | Exit.
3. Launch Sunbird again.
Actual Results:  
The alarms that were being shown before closing Sunbird were dismissed. They're not shown again, even after their snooze times.

Expected Results:  
The alarms should be shown again.
Comment 1 Daniel Boelzle [:dbo] 2006-10-27 02:56:34 PDT
*** Bug 358347 has been marked as a duplicate of this bug. ***
Comment 2 Daniel Boelzle [:dbo] 2006-10-27 03:00:32 PDT
Created attachment 243766 [details] [diff] [review]
no dismissAll when unloading alarm dialog
Comment 3 Telmo Amaral 2006-10-27 06:15:33 PDT
I did the same, to solve the problem on my installation. The only caveat is that, if the user decides to close or escape the dialog without exiting SB, the alarms being shown will not show up again until SB is restarted, even if other alarms come up meanwhile (like a 'snooze until restart'). Ideally, in this situation the alarms should be normally snoozed. Still, it's much better than before.
Comment 4 Joey Minta 2006-10-27 06:44:29 PDT
Comment on attachment 243766 [details] [diff] [review]
no dismissAll when unloading alarm dialog

I have no problems with the code, but you definitely need a UI review on this, because of the changes to the user-experience.  I'm not as convinced either way of what the user expects here. r1/2=jminta
Comment 5 Hristo Stefanov 2006-10-27 07:22:17 PDT
I tried the patch and it works fine, but I think that whenever the Alarms window pup-ups all active alarms should be shown. This probably needs some change in the code that pop-ups the Alarm window - it should ensure that all active alarms are displayed, not only to add the just activated alarm. This behavior ensures that whenever the user look at the Alarm window they will see all active alarms and will not miss an important item just because he/she closed the Alarms window some time before.

Comment 6 Stefan Sitter 2006-10-28 14:26:17 PDT
Created attachment 243922 [details]
IRC discussion on this topic

One proposed solution was to AutoSnooze all alarms for a short time instead, another proposal was to reshow the alarms on next Sunbird startup.
Comment 7 Daniel Boelzle [:dbo] 2006-10-28 17:17:50 PDT
(In reply to comment #6)
> Created an attachment (id=243922) [edit]
> IRC discussion on this topic
> 
> One proposed solution was to AutoSnooze all alarms for a short time instead,

Hmm, I strongly disagree modifying calendar data (writing X-MOZ-SNOOZE...) by just raising and closing the application. IMO most possibly not what users expect.

> another proposal was to reshow the alarms on next Sunbird startup.

+1
If the lastAck stamps don't change, the app will notify again on next startup.
Comment 8 Joey Minta 2006-10-28 17:42:08 PDT
(In reply to comment #7)
> Hmm, I strongly disagree modifying calendar data (writing X-MOZ-SNOOZE...) by
> just raising and closing the application. IMO most possibly not what users
> expect.
I think this is framing it in slightly the wrong way.  The user's expectation about whether or not data is modified isn't what is important.  The user's expectation about whether or not (and when) they will be reminded again is the interesting bit.  (The user may have an intuition about whether data needs to be modified in order to meet their expectation about being reminded, but this is like saying a patient has an intuition about whether surgery is necessary to cure him, it's not binding on the expert.)

With this in mind, I think the best solution is that the alarms that were present in the window when closed would be shown again, the next time the window opens.  (From an implementation standpoint, this just means we need to hold them in memory in the alarm-observer, since on restart they'll be found again, if they're unmodified).  We could even offer a way to open the alarm window when no alarm has fired, revealing the alarms that were closed previously.

Comment 9 Michiel van Leeuwen (email: mvl+moz@) 2006-10-29 05:50:31 PST
That sounds way too complex. Just snooze the alarms on close, and be done with it.
Comment 10 Daniel Boelzle [:dbo] 2006-10-29 07:01:48 PST
(In reply to comment #8)
> (In reply to comment #7)
> > Hmm, I strongly disagree modifying calendar data (writing X-MOZ-SNOOZE...) by
> > just raising and closing the application. IMO most possibly not what users
> > expect.
> I think this is framing it in slightly the wrong way.  The user's expectation
> about whether or not data is modified isn't what is important.  The user's
> expectation about whether or not (and when) they will be reminded again is the
> interesting bit.  (The user may have an intuition about whether data needs to
> be modified in order to meet their expectation about being reminded, but this
> is like saying a patient has an intuition about whether surgery is necessary to
> cure him, it's not binding on the expert.)

Consider a user subscibes to a shared ics file on the intranet. Unless the user has explicitly subscribed the ics file *readOnly*, he will most probably trash other users' changes by just opening/viewing and closing Sunbird. I consider this a bug.
(BTW: If he checked readOnly, uncaught exceptions currently appear on the console.)

Thinking further about this problem (shared calendars and alarms), I think the fundamental problem is that Sunbird's/Lightning's alarm mimic is stored in the shared calendar data. Alarm acknowledgments, snooze times etc. are private to each user, thus they have to be stored separately for each user, e.g. in a local database.

> With this in mind, I think the best solution is that the alarms that were
> present in the window when closed would be shown again, the next time the
> window opens.  (From an implementation standpoint, this just means we need to
> hold them in memory in the alarm-observer, since on restart they'll be found
> again, if they're unmodified).  We could even offer a way to open the alarm
> window when no alarm has fired, revealing the alarms that were closed
> previously.

If I understand you right, then the current (simple) patch fulfills that. No lastAck stamps will change closing the app, thus on next startup the same alarms (if still relevant) will appear again.
Comment 11 Michiel van Leeuwen (email: mvl+moz@) 2006-10-29 07:12:46 PST
But what will the patch do when closing the alarm window, but not the application itself?
Comment 12 Daniel Boelzle [:dbo] 2006-10-29 08:05:30 PST
(In reply to comment #11)
> But what will the patch do when closing the alarm window, but not the
> application itself?

I see your point: with that patch, one needs to restart the app (or wait for a fresh alarm) to get the alarms window again. IMO this is partly ok; I assume the user has had a reason to close it. Of course, snoozing all alarms is a alternative, but when? After 5/10/15 minutes? IMO not very intuitive: I would prefer a straight menu item to get the alarms window again.
Nevertheless also snoozing (writing X-MOZ-SNOOZE-TIME) currently bears the same problems as writing X-MOZ-LASTACK for shared calendars (see comment #10).

Thus as a short term solution, I still vote for the patch, because it potentially avoids dataloss on shared calendars.
Comment 13 Michiel van Leeuwen (email: mvl+moz@) 2006-10-29 09:13:06 PST
At this point, the shared-calendar issue is a non-issue, because shared calendars are not supported. They break in various other ways. And when we fix those issues, this alarm issue will be solved also.
Comment 14 Daniel Boelzle [:dbo] 2006-10-30 00:48:33 PST
(In reply to comment #13)
> At this point, the shared-calendar issue is a non-issue, because shared
> calendars are not supported. They break in various other ways. And when we fix
> those issues, this alarm issue will be solved also.

Well, I see that topic related to <http://wiki.mozilla.org/Calendar:Next_Release>, "Get Data Out (P1)", P2, 0.5: "Share an .ICS file on the network without dataloss".

I can only repeat my point here: Per-user data like dimiss-stamps or snooze-stamps ought to be separated out of the (shared) calendar data. Using X-props is IMO the wrong way.
The current concept puts the burden on provider implementors: some can store user-local data (like WCAP), others can't (like ics).

Looking at code in calAlarmService.js, the author somehow seems to have similar thoughts, though maybe for different reason:

            // This is the *really* hard case where we've snoozed a single
            // instance of a recurring event.  We need to not only know that
            // there was a snooze, but also which occurrence was snoozed.  Part
            // of me just wants to create a local db of snoozes here...
            newEvent.setProperty("X-MOZ-SNOOZE-TIME-"+event.recurrenceId.nativeTime, alarmTime.icalString);
Comment 15 Joey Minta 2006-10-30 05:05:29 PST
(In reply to comment #14)
> Well, I see that topic related to
> <http://wiki.mozilla.org/Calendar:Next_Release>, "Get Data Out (P1)", P2, 0.5:
> "Share an .ICS file on the network without dataloss".
As I understand mvl's point, whether or not there is dataloss with shared calendars is independent of how/when we store snooze.  If we store it locally, there will still be dataloss when making other changes.  If we fix the dataloss for those other changes, then how we store it doesn't make a difference.

> 
> I can only repeat my point here: Per-user data like dimiss-stamps or
> snooze-stamps ought to be separated out of the (shared) calendar data. Using
> X-props is IMO the wrong way.
This ignores the case where one person shares the same calendar on two different computers (home and work).  If I snooze an alarm at work for 3 hours, it could easily be because I want that alarm to pop up an hour after I get home.  Storing the snooze locally won't accomplish that.  (That's also why I opted not to create a local db of snoozes, despite the comment I wrote.)
Comment 16 Daniel Boelzle [:dbo] 2006-10-30 07:53:45 PST
(In reply to comment #15)
> (In reply to comment #14)
> > Well, I see that topic related to
> > <http://wiki.mozilla.org/Calendar:Next_Release>, "Get Data Out (P1)", P2, 0.5:
> > "Share an .ICS file on the network without dataloss".
> As I understand mvl's point, whether or not there is dataloss with shared
> calendars is independent of how/when we store snooze.  If we store it locally,
> there will still be dataloss when making other changes.  If we fix the dataloss
> for those other changes, then how we store it doesn't make a difference.

It would probably help if mvl sums up the problems he is thinking of. Fixing those is good, but I cannot imagine how this relates or solves the sketched problem, i.e. different users writing their dismiss stamp to the very same shared ics file with the current mimic. Please elaborate your thoughts on this.

> > I can only repeat my point here: Per-user data like dimiss-stamps or
> > snooze-stamps ought to be separated out of the (shared) calendar data. Using
> > X-props is IMO the wrong way.
> This ignores the case where one person shares the same calendar on two
> different computers (home and work).  If I snooze an alarm at work for 3 hours,
> it could easily be because I want that alarm to pop up an hour after I get
> home.  Storing the snooze locally won't accomplish that.  (That's also why I
> opted not to create a local db of snoozes, despite the comment I wrote.)

You are putting the burden on provider implementors to handle it correctly: for the shared ics case (again), it can't without storing that data separately or posing dataloss. IMO an expensive feature.
Why don't we keep it simple and go the defensive way first? A local db would also have the benefit that it doesn't care about readOnly calendars.

If we stick to X-props as a universal vehicle for any kind of data, I would greatly appreciate that somebody documents what X-props are to be minded by provider implementors in calIItemBase.idl: which ones are meant to be user-specific and which ones are meant to be shared.
Comment 17 Joey Minta 2006-10-30 08:00:57 PST
(In reply to comment #16)
> If we stick to X-props as a universal vehicle for any kind of data, I would
> greatly appreciate that somebody documents what X-props are to be minded by
> provider implementors in calIItemBase.idl: which ones are meant to be
> user-specific and which ones are meant to be shared.
> 
My understanding of rfc2445 is that x-properties are meant to be unbounded.  It's the property bag in calIItemBase that should be minded by the providers.  ICS happens to do it with x-properties, storage does it with mDBTwo, memory does it with an nsIHashPropertyBag, etc.  Besides, any documentation in calIItemBase would be insufficient to handle additional properties created/used by Lightning/Sunbird extensions.
Comment 18 Daniel Boelzle [:dbo] 2006-10-30 10:01:23 PST
(In reply to comment #17)
> My understanding of rfc2445 is that x-properties are meant to be unbounded. 
> It's the property bag in calIItemBase that should be minded by the providers. 
> ICS happens to do it with x-properties, storage does it with mDBTwo, memory
> does it with an nsIHashPropertyBag, etc.  Besides, any documentation in
> calIItemBase would be insufficient to handle additional properties created/used
> by Lightning/Sunbird extensions.

This all works perfectly in a single-user world. But as I alreasdy pointed out: with respect to shared/ multi-user calendars, providers still *have* to know (at least):
- what X-props are meant to be kept user-local (like the dismiss/snooze stuff)
- what X-props are meant to be spreaded to other users/ attendees

I can only say that this feature puts a lot of stress on the providers. It may not even be implementable:
- some WCAP server versions have bugs writing X-props on occurrences (though this is currently no problem, because all X-MOZ- props are currently set at parents only, future features built on top of X-props will potentially break).
- the shared-ics problem is yet unsolved
- anybody tested whether calDAV support works correctly? i.e. all attendees and organizer(!) can save their dismiss stamps separately?
- I wouldn't expect that all small devices save/tunnel X-props correctly

I see a limited benefit, but lots of problems.
Again:
Why don't we keep it simple and go the defensive way first? A local db would
also have the benefit that you can dismiss alarms from readOnly calendars.
Comment 19 Michiel van Leeuwen (email: mvl+moz@) 2006-10-30 13:26:22 PST
My point about remote calendars was when using an ics calendar to share data. That doesn't work currently. You can thrash all your data in interesting ways. And fixing that (maybe with locking, syncing and all that), we also fix the problem I thought we were talking about here.
But now it seems to be more about general use, where person A will snooze alarms in person B's calendar. That's a completely different story, and i'm not sure what the right solution for that is. The reason for that is that I see two use-cases for remote storage of data: 1) you want to access your data on multiple locations. 2) you share one calendar with other people. in case 1, you do want to store alarm and snooze information in the remote store. in case 2, you do not want to do that. And we have no way of knowing which use case the current user wants. Maybe he even wants a combination of the two cases...

But that sounds like a discussion for the newsgroup or for a different bug. For now, we store alarm data in the remote calendar, through the provider. I think that we should also store the snooze information in there. We don't need to find a solution just for this case where you close the window (not that common), while we still have the big problem of snoozing the alarms with the proper UI. 

So I'm still in favour of making the close button snooze the alarms.
Comment 20 Daniel Boelzle [:dbo] 2006-10-31 12:04:50 PST
(In reply to comment #19)
> But now it seems to be more about general use, where person A will snooze
> alarms in person B's calendar. That's a completely different story, and i'm not
> sure what the right solution for that is. The reason for that is that I see two

Then why do we rely on X-props to store alarm snoozes and dismiss stamps, if that obviously doesn't work?

> use-cases for remote storage of data: 1) you want to access your data on
> multiple locations. 2) you share one calendar with other people. in case 1, you
> do want to store alarm and snooze information in the remote store. in case 2,
> you do not want to do that. And we have no way of knowing which use case the
> current user wants. Maybe he even wants a combination of the two cases...

Yes, that's my point from comment #18.
user-local/specific (1) vs. spreaded to other users/attendees (2)

> But that sounds like a discussion for the newsgroup or for a different bug. 

Yes, I agree we have run off-topic here...

> now, we store alarm data in the remote calendar, through the provider. I think
> that we should also store the snooze information in there. We don't need to
> find a solution just for this case where you close the window (not that
> common), while we still have the big problem of snoozing the alarms with the
> proper UI. 
> 
> So I'm still in favour of making the close button snooze the alarms.

Even if we switch over to snoozing (not dismissing) all alarms, the calendar would be written most likely every time the user just opens and closes the app without asking. :(
Comment 21 Daniel Boelzle [:dbo] 2006-10-31 12:06:15 PST
Created attachment 244223 [details] [diff] [review]
no dismissAll when unloading alarm dialog

correcting the patch.
Comment 22 Telmo Amaral 2006-10-31 18:22:17 PST
(In reply to comment #12)
> (In reply to comment #11)
>> But what will the patch do when closing the alarm window, but not the
>> application itself?
> 
> I see your point: with that patch, one needs to restart the app (or wait for a
> fresh alarm) to get the alarms window again. ...

I think this patch is good, but again let me point out a caveat (which somebody else may later decide to report as a bug): if the user closes the alarms window but not SB itself, the alarms being shown will not pop up again until SB is restarted, /even/ if other alarms pop up meanwhile. That is, for the alarms being shown, the patch works as a 'snooze until restart'. To reproduce, using a fresh profile and the patch:

1. Create event A in the past, with an immediate alarm; the alarm pops up.
2. Close or escape the alarms window, without closing SB.
3. Create event B in the past, with an immediate alarm; the alarm for B pops up, but the alarm for A does not.
4. Restart SB; the alarms for A and B  pop up.
Comment 23 Dan Mosedale (:dmose) 2006-12-06 15:13:03 PST
Created attachment 247739 [details]
more irc discussion

We had some IRC discussion today about the best approach for this, and came to several conclusions.  The gist is this:

a) how we deal with shared vs. personal data in shared calendars over the longer term deserves its own bug; let's spin one off.
b) how we deal with alarms in shared calendars in the shorter term also deserves its own bug; let's spin one off for that too.
c) the right fix for this bug is autosnooze in combination with alarm window state persistence.  Both of those bugs could be fixed here, or it'd be reasonable to start with autosnooze and spin off state persistence into a bug for later.

Details of the thinking are in the attachment.
Comment 24 Matthew (lilmatt) Willis 2006-12-07 15:11:12 PST
Nominating blocking so we can discuss during triage
Comment 25 cmtalbert 2007-01-21 06:41:51 PST
*** Bug 243144 has been marked as a duplicate of this bug. ***
Comment 26 cmtalbert 2007-01-21 06:45:29 PST
(In reply to comment #23)
> Created an attachment (id=247739) [details]
> more irc discussion
> a) how we deal with shared vs. personal data in shared calendars over the
> longer term deserves its own bug; let's spin one off.
> b) how we deal with alarms in shared calendars in the shorter term also
> deserves its own bug; let's spin one off for that too.
> c) the right fix for this bug is autosnooze in combination with alarm window
> state persistence.  Both of those bugs could be fixed here, or it'd be
> reasonable to start with autosnooze and spin off state persistence into a bug
> for later.

I am not finding any of these spinoff bugs, have they been filed? 
 

Comment 27 Martin Schröder [:mschroeder] 2007-01-23 08:29:02 PST
*** Bug 366049 has been marked as a duplicate of this bug. ***
Comment 28 Matthew (lilmatt) Willis 2007-03-12 08:41:53 PDT
The follow-ups need to be filed, and we need input from mvl on whether or not the patch as it stands can land to avoid the dataloss issue, with us following up with the autosnooze, perhaps post 0.5.
Comment 29 Michiel van Leeuwen (email: mvl+moz@) 2007-03-24 09:39:12 PDT
Comment on attachment 244223 [details] [diff] [review]
no dismissAll when unloading alarm dialog

Ok, this might not be the perfect patch (as seen by all the discussion here).
But for now, it's an improvement. Let's just take it.
Comment 30 Matthew (lilmatt) Willis 2007-03-24 11:20:26 PDT
Patch checked in on MOZILLA_1_8_BRANCH and trunk.

-> FIXED
Comment 31 Matthew (lilmatt) Willis 2007-03-24 11:25:48 PDT
Spin offs are bug 375208, bug 375209, and bug 375210
Comment 32 Omar Bajraszewski 2007-06-01 10:15:36 PDT
Verified on Sunbird 0.5 RC1 Mozilla/5.0 (Windows; U; Windows NT 5.1; pl; rv:1.8.1.4pre) Gecko/20070524 Sunbird/0.5

Note You need to log in before you can comment on or make changes to this bug.