Closed
Bug 413214
Opened 17 years ago
Closed 17 years ago
Remove alarm image if suppressAlarms is true
Categories
(Calendar :: Calendar Frontend, defect)
Calendar
Calendar Frontend
Tracking
(Not tracked)
VERIFIED
FIXED
0.8
People
(Reporter: sebo.moz, Assigned: sebo.moz)
Details
Attachments
(2 files, 2 obsolete files)
4.15 KB,
patch
|
sebo.moz
:
review+
|
Details | Diff | Splinter Review |
32.40 KB,
image/png
|
Details |
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 1•17 years ago
|
||
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•17 years ago
|
Flags: wanted-calendar0.8? → wanted-calendar0.8+
Assignee | ||
Comment 2•17 years ago
|
||
This would refresh the views. It does, however, not work in month/multiweek view. Any ideas?
Assignee: nobody → sebo.moz
Status: NEW → ASSIGNED
Comment 3•17 years ago
|
||
+ 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•17 years ago
|
||
(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•17 years ago
|
Keywords: checkin-needed
Updated•17 years ago
|
OS: Linux → All
Hardware: PC → All
Target Milestone: --- → 0.8
Comment 5•17 years ago
|
||
Checked in on HEAD and MOZILLA_1_8_BRANCH => FIXED.
Comment 6•17 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•17 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•17 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•17 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•17 years ago
|
||
Comment 11•17 years ago
|
||
filed bug 415654 for that
Comment 12•17 years ago
|
||
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)
Comment 13•17 years ago
|
||
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.
Description
•