307323 - New mail plays both the system beep and the notification sound

Status: RESOLVED FIXED

(Thunderbird :: Preferences, defect)

Description

[Dave Miller [:justdave]]

Today's nightly build picked up the fix to finally play the notification sound
you chose in your preferences when new mail comes in.  The preference gives you
a choice between playing the system alert sound, and playing a custom sound. 
However, if you have the custom sound chosen, it plays both the system alert
sound and the custom one instead of only the custom one.

By experimenting...  if I uncheck the "Animate the Dock Icon" preference, then
it plays only the custom sound.  If I check the "Animate the Dock Icon" and
uncheck the "Play a Sound" then it plays only the system alert sound.  So I
suspect the system alert is a side effect of the way the dock icon is being told
to bounce.

I know it's possible to make the dock icon bounce without having the system play
a sound -- Eudora does it ;)

Comment 1

[Josh Aas]

I can take this

Comment 2

[Josh Aas]

Attachment: fix v1.0

GetAttention() shouldn't play a sound because the API doesn't allow you to
choose whether or not a sound gets played. No callers I found need the sound.
We could change the API, but it isn't necessary. If a caller really wants a
sound, it is easier for them to add the sound that it is for those who don't
want the sound to not have it.

I also fixed some tabbing issues.

Comment 3

[Josh Aas]

On checkin I'll get rid of the comment about playing the default sound that I
accidentally left in.

Comment 4

[Josh Aas]

Attachment: fix v1.1

this just has new comments, not changing review flags even though this is what
I'd check in

Comment 5

[Asaf Romano (gone)]

Comment on attachment 195195 [details] [diff] [review]
fix v1.0

While you're there, please fix the comment on nmMark, especially in cocoa
widget ;). The meaning of it in OS X is "bounce the dock icon".

r=me on v1.1 with that fixed.

Comment 6

[Josh Aas]

nominating for 1.8b5 as this is necessary for new mail alert sounds to work
correctly

Comment 7

[Simon Fraser [no longer active]]

I'd really prefer that we fix cocoa widget to use [NSApp requestAttention:],
since we're in the process of fleshing out cocoa widget.

If you don't want to do that, just check in the widget/src/mac part.

Comment 8

[Josh Aas]

One nice thing about not using NSApp's requestUserAttention: method is that with
the carbon calls we can control whether or not the system beeps. In the case of
mail, we really don't want it to beep. It may or may not beep with the Cocoa
call, but either way that could change in the future and then we're back to this
bug. Just wanted to not that here...

Comment 9

[Josh Aas]

Attachment: fix v2.0

this has an updated Cocoa implementation, which I haven't tested. carbon impl
is the same, just has new comment.

Comment 10

[Asaf Romano (gone)]

So, this creates a behavior difference between Carbon to Cocoa widget - the
cocoa version of GetAttension will only bounce once. I'm not sure what's better...

Simon, Mike, what do you think?

Comment 11

[Josh Aas]

Comment on attachment 195195 [details] [diff] [review]
fix v1.0

Here's my deal: new mail sounds are busted with dock bouncing on. I want that
fixed in the 1.8b5 release. I want a safe patch for the branch that does just
that, and once that is done for the beta I'd be happy to dig into the deeper
issues later on.

Deeper issues might include:
1) what do we want to do in cocoa widgets?
2) should the dock bouncing pref even be there? (Mano raised this point, I
think it should be there)
3) does the GetAttention API need to be adjusted to allow for different alert
levels?

So if this bug gets approved as a 1.8b5 blocker bug, I'd like to land fix v1.1
on the branch. That is my only interest in this bug at the moment.

I am requesting review from Simon again. I want to make sure there is a
fully-reviewed patch ready to go for the branch, though I don't care if that is
v1.1 or v2.0.

Comment 12

[Asaf Romano (gone)]

Comment on attachment 195315 [details] [diff] [review]
fix v2.0

Since we don't want random UE changes when we switch to cocoa widget. r=mano if
you change this to NSCriticalRequest. We can argue about it later.

Comment 13

[Josh Aas]

sounds good to me

Comment 14

[Josh Aas]

fix v2.0 with NSCriticalRequest checked in, leaving this bug open for the branch

Comment 15

[Josh Aas]

v2.0 checked in on branch with NSCriticalRequest for Cocoa