Closed Bug 413214 Opened 16 years ago Closed 16 years ago

Remove alarm image if suppressAlarms is true

Categories

(Calendar :: Calendar Frontend, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: sebo.moz, Assigned: sebo.moz)

Details

Attachments

(2 files, 2 obsolete files)

Attached patch patch v1 (obsolete) — Splinter Review
Follow-up from bug 257428.

The patch does not listen to onPrefChanged and onPrefDeleted. A view refresh is necessary in that case.
Attachment #298099 - Flags: review?(philipp)
Flags: wanted-calendar0.8?
Comment on attachment 298099 [details] [diff] [review]
patch v1

If you like, go ahead and implement onPrefChanged/Deleted. All you need to do for now is refresh the view. We aren't to smart about such things, but that should be done in a separate bug. You need to do this in each view, see calendar-multiday-view line 2503 and calendar-month-view line 1669.

Otherwise r+
Attachment #298099 - Flags: review?(philipp) → review+
Flags: wanted-calendar0.8? → wanted-calendar0.8+
This would refresh the views. It does, however, not work in month/multiweek view. Any ideas?
Assignee: nobody → sebo.moz
Status: NEW → ASSIGNED
+            if (aValue == "suppressAlarms" &&

You mean aName == "suppressAlarms". Apart from that everything looks fine, r+ if you decide to use that as a final version.
Attached patch patch v2Splinter Review
(In reply to comment #3)
> You mean aName == "suppressAlarms". 
Oh yes, that makes sense. And moreover, it works! :-)

Carrying over r+
Attachment #298099 - Attachment is obsolete: true
Attachment #298129 - Attachment is obsolete: true
Attachment #298282 - Flags: review+
Keywords: checkin-needed
OS: Linux → All
Hardware: PC → All
Target Milestone: --- → 0.8
Checked in on HEAD and MOZILLA_1_8_BRANCH => FIXED.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Comment on attachment 298282 [details] [diff] [review]
patch v2

>           var alarmImage = document.getAnonymousElementByAttribute(this, "anonid", "alarm-image");
>-          if ((item.alarmOffset || item.parentItem.alarmOffset) && getPrefSafe("calendar.alarms.indicator.show", true)) {
>+          if ((item.alarmOffset || item.parentItem.alarmOffset) &&
>+              getPrefSafe("calendar.alarms.indicator.show", true) &&
>+              !item.calendar.getProperty("suppressAlarms")) {
>               alarmImage.removeAttribute("hidden");
>           }

We should remove the check for suppressAlarms since this will remove the alarm icon even though I might have switched off popup alarms for the calendar. Since the item's definition still has the alarm set, I'd vote for having the icon.
Comment on attachment 298282 [details] [diff] [review]
patch v2

(In reply to comment #6)
> We should remove the check for suppressAlarms since this will remove the alarm
> icon even though I might have switched off popup alarms for the calendar. Since
> the item's definition still has the alarm set, I'd vote for having the icon.
> 
Thats all this bug is about. Just for clarification: you want the whole patch to be backed out, not just this one line?

I'm not sure I agree with the argument. But then again, I don't understand what exactly the icon is for. If it is supposed to show the user that he doesn't need to worry about the event atm because there is an alarm coming up anyway, then it should be removed. If the icon is there for another reason, that I currently don't see, then I would like to learn it.

Adding Christian and asking for a post ui-review. Sorry for not asking you first.
Attachment #298282 - Flags: ui-review?(christian.jansen)
Sebo, it's just because users need to know whether an alarm has been specified on an item, even if you've switched off popup alarms for the whole calendar (suppressAlarms). We (Christian, Philipp and I) agreed that we want a different icon for that case, e.g. something like a muted alarm bell.
I suggest to change the icon instead of removing it. The situation is comparable with adjusting the loudspeaker volume. If the loudspeaker is muted the icon indicates that state. In addition I suggest to inform users in the event dialog the alarms are suppressed. See attachment (the wording of the Reminder string is far from being perfect ;-)
Attached image Suppressed Alarms
filed bug 415654 for that
Comment on attachment 298282 [details] [diff] [review]
patch v2

Removing ui-review request. Christian has commented already, and there is also a follow-up bug (with patch attached).
Attachment #298282 - Flags: ui-review?(christian.jansen)
VERIFIED
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.12pre) Gecko/20080204
Thunderbird/2.0.0.12pre Lightning/0.8pre (2008020318)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.