Alarms are dismissed when Sunbird is closed

VERIFIED FIXED in Sunbird 0.5

Status

Calendar
Alarms
VERIFIED FIXED
11 years ago
9 years ago

People

(Reporter: Telmo Amaral, Assigned: dbo)

Tracking

unspecified
Sunbird 0.5
Bug Flags:
in-litmus ?

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

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

Updated

11 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Updated

11 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

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

Updated

11 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 1

11 years ago
*** Bug 358347 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 2

11 years ago
Created attachment 243766 [details] [diff] [review]
no dismissAll when unloading alarm dialog
Attachment #243766 - Flags: first-review?(jminta)
(Reporter)

Comment 3

11 years ago
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

11 years ago
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
Attachment #243766 - Flags: ui-review?
Attachment #243766 - Flags: first-review?(jminta)
Attachment #243766 - Flags: first-review+

Comment 5

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

(Assignee)

Updated

11 years ago
Attachment #243766 - Flags: ui-review? → ui-review?(mvl)

Comment 6

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

Comment 7

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

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

That sounds way too complex. Just snooze the alarms on close, and be done with it.
(Assignee)

Comment 10

11 years ago
(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.
But what will the patch do when closing the alarm window, but not the application itself?
(Assignee)

Comment 12

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

Comment 14

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

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

Comment 16

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

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

Comment 18

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

Comment 20

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

Comment 21

11 years ago
Created attachment 244223 [details] [diff] [review]
no dismissAll when unloading alarm dialog

correcting the patch.
Attachment #243766 - Attachment is obsolete: true
Attachment #244223 - Flags: ui-review?(mvl)
Attachment #244223 - Flags: first-review?(jminta)
Attachment #243766 - Flags: ui-review?(mvl)
(Reporter)

Comment 22

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

Updated

11 years ago
Attachment #244223 - Flags: first-review?(jminta) → first-review+

Updated

11 years ago
Whiteboard: [litmus testcase wanted]
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.
Nominating blocking so we can discuss during triage
Flags: blocking-calendar0.5?
OS: Windows XP → All
Hardware: PC → All
Target Milestone: --- → Sunbird 0.5
Flags: blocking-calendar0.5? → blocking-calendar0.5+

Updated

11 years ago
Duplicate of this bug: 243144

Comment 26

11 years ago
(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? 
 

Duplicate of this bug: 366049
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.
Whiteboard: [litmus testcase wanted] → [cal-ui-review needed][litmus testcase wanted]
Whiteboard: [cal-ui-review needed][litmus testcase wanted] → [cal-ui-review needed] [no l10n impact] [litmus testcase wanted]
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.
Attachment #244223 - Flags: ui-review?(mvl) → ui-review+
Patch checked in on MOZILLA_1_8_BRANCH and trunk.

-> FIXED
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Spin offs are bug 375208, bug 375209, and bug 375210
Whiteboard: [cal-ui-review needed] [no l10n impact] [litmus testcase wanted] → [no l10n impact] [litmus testcase wanted]

Comment 32

10 years ago
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
Status: RESOLVED → VERIFIED

Updated

10 years ago
Flags: in-litmus?
Whiteboard: [no l10n impact] [litmus testcase wanted] → [no l10n impact]

Updated

9 years ago
Flags: blocking-calendar0.5+
Whiteboard: [no l10n impact]
You need to log in before you can comment on or make changes to this bug.