Closed Bug 420844 Opened 16 years ago Closed 16 years ago

Only show suppress icon on events if popup alarms are supported

Categories

(Calendar :: Calendar Frontend, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: Fallen, Assigned: Fallen)

References

Details

Attachments

(1 file, 2 obsolete files)

Attached patch Fix - v1 (obsolete) β€” β€” Splinter Review
Currently for WCAP, the suppress icon is always shown since popup alarms should not be fired, since the server does not reliably support X-props.

This gives users the feeling that they will receive no alarms at all which is wrong. Email alarms are supported and the standard transport. To make this clearly visible to the user, I have introduced a capability "capabilities.alarms.popup.supported" that is also being checked. If it is false, then the suppress icon is not shown, but suppressAlarms is true.
Attachment #307202 - Flags: review?(Berend.Cornelius)
Flags: wanted-calendar0.8+
Comment on attachment 307202 [details] [diff] [review]
Fix - v1

When I applied the patch I could see that the icon has been correctly replaced, but when I tested it at a WCAP event where I set a reminder I was confronted with the popup-alarm displaying all events of all loaded WCAP calendars , no matter if they were previously selected or not. Therefor I have to deny the review.
Attachment #307202 - Flags: review?(Berend.Cornelius) → review-
Attached patch patch v. #2 (obsolete) β€” β€” Splinter Review
I gave myself some time to see how it all works and the patch attached might improve the situation. With this patch applied no popups come up. I just added a hunk that refers to my modifications in calAlarmService.js. Probably you find a smarter solution...
Attached patch Fix - v2 β€” β€” Splinter Review
I kind of wonder why this happens, I made the base provider automatically set suppressAlarms = true when capabilities.alarms.popup.supported === false. To make the product more robust, I added additional checks in the alarm service, similar to what you suggested, but I went the route to make the capability an extra check everywhere suppressAlarms is queried/set.

What I came across while looking, is that suppressAlarms only seems to suppress the onAlarm event. I would have expected that alarms of calendars that have suppressAlarms set do not even get an alarm timeout and are just completely ignored. I think it would be better to fix this in a different bug though, since changing this could bring a higher regression risk.

Daniel, Sebo, was there any reason alarms were only stopped shortly before firing, instead of before they are initialized?
Attachment #307202 - Attachment is obsolete: true
Attachment #307701 - Attachment is obsolete: true
Attachment #308390 - Flags: review?(Berend.Cornelius)
(In reply to comment #3)
> What I came across while looking, is that suppressAlarms only seems to suppress
> the onAlarm event. I would have expected that alarms of calendars that have
> suppressAlarms set do not even get an alarm timeout and are just completely
> ignored. I think it would be better to fix this in a different bug though,
> since changing this could bring a higher regression risk.
Kind of strange, because line 576 avoids any items retrieval if suppressAlarms is set.

> Daniel, Sebo, was there any reason alarms were only stopped shortly before
> firing, instead of before they are initialized?
I think that's still a heritage of the former code which hasn't had a refresh logic implemented. If we refresh the alarmed items everytime we switch the mentioned prefs, we IMO could go with the pre-getItem-checks (line 576).
(In reply to comment #4)
> Kind of strange, because line 576 avoids any items retrieval if suppressAlarms
> is set.

Yes, I missed that when writing the comment. You are of course right. Nevertheless, with the patch as is, the alarm service still seems to considers the alarm, even though suppressAlarm is set via base provider and the capability should tell the alarm service to ignore those alarms.
Comment on attachment 308390 [details] [diff] [review]
Fix - v2

Moving review to daniel since he is back and this is a mostly wcap bug.
Attachment #308390 - Flags: review?(Berend.Cornelius) → review?(daniel.boelzle)
Comment on attachment 308390 [details] [diff] [review]
Fix - v2

item.calendar.getProperty("capabilities.alarms.popup.supported") !== false) {
>+                  // Only show the suppressed icon if alarms are suppressed and
>+                  // popup alarms are supported. This keeps us from making the
fix the comment

>+                  // alarm look disabled when the server supports only email
>+                  // alarms (i.e WCAP)
...supports only email alarms

looks good to me; r=dbo, thanks for the patch

BTW: with all those changes, especially the reformatting, this certainly bitrots my "decouple agenda/views etc. into different composite calendars" WIP patch.... hrmpf ;)
Attachment #308390 - Flags: review?(daniel.boelzle) → review+
Philipp asked me to check in on HEAD, MOZILLA_1_8_BRANCH and SUNBIRD_0_8_BRANCH => FIXED.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Checked in nightly build 2008031319 (lightning) and 20080313 (sunbird) -> task is fixed and verified.
Status: RESOLVED → VERIFIED
Depends on: 422898
You need to log in before you can comment on or make changes to this bug.