alarm dialog plays a sound for each alarm that is displayed

RESOLVED FIXED in 1.0b7

Status

Calendar
Alarms
RESOLVED FIXED
9 years ago
6 years ago

People

(Reporter: Wolfgang Sourdeau, Assigned: Wolfgang Sourdeau)

Tracking

(Blocks: 1 bug)

unspecified
1.0b7
Bug Flags:
wanted-calendar1.0 +

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

9 years ago
User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; fr; rv:1.9.0.10) Gecko/2009042805 Iceweasel/3.0.9 (Debian-3.0.9-1)
Build Identifier: 

The title says it all. With the current behaviour of Lightning, that displays all occurences of alarms in the future and the past, this really is annoying. Also, this slows down the display of the dialog.

A patch is attached with a proposed fix.

Reproducible: Always

Steps to Reproduce:
1. create a recurring event for 5 days and set a reminder

Actual Results:  
the alarm dialog will display once but the sound will play 5 times.


Expected Results:  
The sound should play only once per display or once per batch of alarms.
We should definitely look into this. It shouldn't be too hard to fix, just add a small timeout for the alarm sound and don't make the sound play if another alarm is added in that time. A different solution might be to only make one sound when alarms are added via onLoad, but to still make one sound per items added via onAddItem (i.e when a new event is created with an alarm).

Also, we should find out how the progress is on making the nsISound asynchronous. I believe the thread blocks when a sound is played. Maybe we can use the new thread api to fire the sound in a separate thread.

Markus, maybe this bug interests you?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: wanted-calendar1.0+
Whiteboard: [good first bug]
(Assignee)

Comment 2

9 years ago
Created attachment 382124 [details] [diff] [review]
suggested fix, play sound once per alarm monitor

Since I am not yet very familiar with the alarm code, I am not sure whether this would enable the once/batch solution mentionned in the ticket rather than the once/dialog.
Attachment #382124 - Flags: review?
Comment on attachment 382124 [details] [diff] [review]
suggested fix, play sound once per alarm monitor

Oh, sorry. I overread that you were about to attach a patch. I'll review this later on.
Attachment #382124 - Flags: review? → review?(philipp)

Comment 4

9 years ago
I don't think the proposed solution is usable. If someone keeps the reminder window open and two hours later another alarm fires no sound will be played.

In addition keep in mind this is a Linux only issue due to Bug 110385. Other platforms don't have this issue. Previous reports like Bug 390365 were resolved against this one.
(Assignee)

Comment 5

9 years ago
Even if the problem on Linux is that the playing of sound files is asynchronous, I doubt this would be appealling acoustically to users of other systems.
However, I agree that this could probably be handled another way, for example following the idea submitted by Philipp.
(Assignee)

Comment 6

9 years ago
It would seem reasonable to me to have a constraint of 1 alarm/minute since this would give time to Lightning to normally process all the alarms that should be taken into account and also because events and alarms are generally set at even intervals of 5 or mores minutes. What do you think?
Although related to import, see also bug 276585.

I don't think we should limit on timed basis. Say you have default alarms set and acciddentally create an event in the past. The alarm will sound. You ignore the window because you know why the alarm sounded. 40 seconds later, you get a reminder for your meeting. The alarm will not sound again and you may miss the meeting.
Comment on attachment 382124 [details] [diff] [review]
suggested fix, play sound once per alarm monitor

This is obviously not the right solution. I think the best thing to do would be to fix the sound being async bug. As you can see, we already have a limitation of 5 alarm sounds per minute. so the maximum time the application is held is 5 * length of alarm sound. I think this is reasonable for now.
Attachment #382124 - Flags: review?(philipp) → review-
By the way, I experimented with using cal.execWorker() to execute the alarm sound on a different thread, but either I got the application to crash due to mutex lock assertations, or I didn't get more than one sound to fire.

Daniel, maybe you can give it a try? You proably know better what can be done with execWorker() and what can't.
Whiteboard: [good first bug]
(Assignee)

Comment 10

9 years ago
(In reply to comment #7)
> Although related to import, see also bug 276585.
> 
> I don't think we should limit on timed basis. Say you have default alarms set
> and acciddentally create an event in the past. The alarm will sound. You ignore
> the window because you know why the alarm sounded. 40 seconds later, you get a
> reminder for your meeting. The alarm will not sound again and you may miss the
> meeting.

I doubt this would ever happen at all, because most events and alarms are set to round timestamps. So there is one chance on X billions of billions that this may one day happen to somebody. The sounding and display of 500 alarms for events occuring in the past is much more inconvenient and will affect far more users.
(Assignee)

Comment 11

9 years ago
Another way of looking at it is to take into account that most of those alarms ring at the same second. Why, because they occured 5 minutes before or for other different reasons. This batch of alarm thus appears at the same time, specifically when the calendar is opened. Therefore, it would probably be conceivable to have batches of alarms meant to sound at the same specific timestamp assembled in javascript arrays. If you make those alarms sound once per array rather than once per event, the problem is solved.
(Assignee)

Comment 12

9 years ago
(In reply to comment #11)
> Another way of looking at it is to take into account that most of those alarms
> ring at the same second. Why, because they occured 5 minutes before or for
> other different reasons. This batch of alarm thus appears at the same time,
> specifically when the calendar is opened. Therefore, it would probably be
> conceivable to have batches of alarms meant to sound at the same specific
> timestamp assembled in javascript arrays. If you make those alarms sound once
> per array rather than once per event, the problem is solved.

(to finish my idea:)
For example... rather than having each alarm objects sound for itself. The alarm monitor could sort those alarms by timestamp and make them ring together.
(Assignee)

Comment 13

9 years ago
Created attachment 383926 [details] [diff] [review]
restoration of the showmissed preference

This new patch restores the showmissed preference. It doesn't lightning from reparsing the alarms in the past but at least they don't bother the users anymore. I will open a new ticket for the reparsing issue.
Attachment #382124 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Attachment #383926 - Flags: review?(philipp)
Assignee: nobody → WSourdeau
OS: Linux → All
Hardware: x86 → All
Blocks: 496889
Status: NEW → ASSIGNED
Comment on attachment 383926 [details] [diff] [review]
restoration of the showmissed preference

>+                let showMissed = true;
>+                try {
>+                    showMissed = this.mPrefService.getBoolPref("calendar.alarms.showmissed");
Instead of keeping a pref service reference, I think its fine to use cal.getPrefSafe().

Otherwise it looks fine from a code review point. Could you attach a new patch with getPrefSafe for checkin?
Attachment #383926 - Flags: review?(philipp) → review+
Created attachment 396422 [details] [diff] [review]
[checked in] restoration of the showmissed preference v2

Wolfgang, I hope you don't mind I rewrote your patch to account for Philipp's suggestions regarding the PrefService, to get the pref before entering the 'for each' loop, and to merge the 'if (showMissed)' statement with the 'else' clause.
Attachment #396422 - Flags: review?(philipp)
Comment on attachment 396422 [details] [diff] [review]
[checked in] restoration of the showmissed preference v2

Looks good, thanks. r=philipp
Attachment #396422 - Flags: review?(philipp) → review+
(Assignee)

Comment 17

8 years ago
> Wolfgang, I hope you don't mind I rewrote your patch to account for Philipp's
> suggestions regarding the PrefService, to get the pref before entering the 'for
> each' loop, and to merge the 'if (showMissed)' statement with the 'else'
> clause.

Hi Martin,

Not at all. I am rather glad you did. Thanks!
Comment on attachment 396422 [details] [diff] [review]
[checked in] restoration of the showmissed preference v2

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/60a433653c6c>

This bug is not fixed because this patch is for bug 496889.
Attachment #396422 - Attachment description: restoration of the showmissed preference v2 → [checked in] restoration of the showmissed preference v2
Attachment #383926 - Attachment is obsolete: true
Created attachment 517137 [details] [diff] [review]
Make sound instance global

Whats left to do for this bug? I've found that we might save some cycles if we use a global nsISound instance for the monitor and just play the sounds with that instance.

We already have a limit to the alarm sounds per time unit, so I think most of this bug is fixed?

Wolfgang, I know we haven't decided on a final process yet, but maybe you want to review this patch?
Attachment #517137 - Flags: review?(WSourdeau)
Attachment #517137 - Flags: review?(WSourdeau) → review?(matthew.mecca)
Comment on attachment 517137 [details] [diff] [review]
Make sound instance global

Review of attachment 517137 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, r=mmecca

::: calendar/base/src/calAlarmMonitor.js
@@ +53,5 @@
>      this.wrappedJSObject = this;
>      this.mAlarms = [];
> +
> +    this.mSound = Components.classes["@mozilla.org/sound;1"]
> +                            .createInstance(Components.interfaces.nsISound);

Should we add this.mSound.init() here?
Attachment #517137 - Flags: review?(matthew.mecca) → review+
I was thinking the same back then, but this means extra load on startup. If init() is not called, it is automatically called on the first sound, which is a better point in time to my mind.
Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/989daa36e011>
-> FIXED
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Trunk
This bug was also pushed to comm-beta and comm-aurora, likely during the last merge.
Target Milestone: Trunk → 1.0b6
You need to log in before you can comment on or make changes to this bug.