If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Remove alarm image if suppressAlarms is true

VERIFIED FIXED in 0.8

Status

Calendar
Calendar Views
VERIFIED FIXED
10 years ago
10 years ago

People

(Reporter: Sebastian Schwieger, Assigned: Sebastian Schwieger)

Tracking

unspecified
Bug Flags:
wanted-calendar0.8 +

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

10 years ago
Created attachment 298099 [details] [diff] [review]
patch v1

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+

Updated

10 years ago
Flags: wanted-calendar0.8? → wanted-calendar0.8+
(Assignee)

Comment 2

10 years ago
Created attachment 298129 [details] [diff] [review]
work in progress (incl. onPrefChanged)

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

Comment 4

10 years ago
Created attachment 298282 [details] [diff] [review]
patch v2

(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+
(Assignee)

Updated

10 years ago
Keywords: checkin-needed
OS: Linux → All
Hardware: PC → All
Target Milestone: --- → 0.8

Comment 5

10 years ago
Checked in on HEAD and MOZILLA_1_8_BRANCH => FIXED.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED

Comment 6

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

Comment 7

10 years ago
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)

Comment 8

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

Comment 9

10 years ago
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 ;-)

Comment 10

10 years ago
Created attachment 299970 [details]
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.