778114 - chat sound not playing if mail notification sound disabled

Status: RESOLVED FIXED

(Thunderbird :: Instant Messaging, defect)

Description

[Axel Grude [:realRaven]]

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:14.0) Gecko/20100101 Firefox/14.0.1
Build ID: 20120713134347

Steps to reproduce:

enabled mail.chat.play_notification_sound while having mail.biff.play_sound disabled


Actual results:

no notification sounds played back when my name was mentioned in IRC


Expected results:

chat notification sounds should play even if mail notification sounds are disabled

Comment 1

[Axel Grude [:realRaven]]

Tech Notes for patching:

Pref check within PlayBiffSound() needs to move outside and be checked before calling PlayBiffSound() instead; code:

http://mxr.mozilla.org/comm-central/source/mailnews/base/src/nsStatusBarBiffManager.cpp#229 


http://mxr.mozilla.org/comm-central/source/mailnews/base/src/nsStatusBarBiffManager.cpp#89

http://mxr.mozilla.org/comm-central/source/mailnews/base/src/nsStatusBarBiffManager.cpp#180

I will see if I can get around to writing that one soon...

Comment 2

[Axel Grude [:realRaven]]

Attachment: moved check out of PlayBiffSound into folder listener

Comment 3

[Axel Grude [:realRaven]]

Attachment: Same patch as before w/o whitespace mods

Comment 4

[Axel Grude [:realRaven]]

Attachment: Samne patch without duplicate comment

removed duplicate comment

Comment 5

[Axel Grude [:realRaven]]

Attachment: This one even compiles :P

Ok, it was pointed out that previous code was incomplete, and how to compile a single file. This one compiles and should also work as designed. I have yet to make a full build for testing.

Comment 6

[Axel Grude [:realRaven]]

Attachment: moved a comment

moved a comment to a more appropriate place

Comment 7

[Florian Quèze [:florian]]

Axel, is it intentional that there's no review request on the patch attached in comment 6?

Comment 8

[Axel Grude [:realRaven]]

probably not, who should I add for this one?

Comment 9

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 646925 [details] [diff] [review]
moved a comment

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

Looks fine, works in my quick test.

::: mailnews/base/src/nsStatusBarBiffManager.cpp
@@ +170,5 @@
>      // if we fail along the way, don't return.
>      // we still need to update the UI.    
>      if (newValue == nsIMsgFolder::nsMsgBiffState_NewMail) {
> +      nsresult rv;
> +      nsCOMPtr<nsIPrefBranch> pref(do_GetService(NS_PREFSERVICE_CONTRACTID, &rv));

This retrieves the pref service twice per notification (once here, and once inside the call to PlayBiffSound()) but I can't see a simple way to only get it once, and it's not a huge performance hit anyway.

Comment 10

[:Irving Reid (No longer working on Firefox)]

Pushed to comm-central trunk:

https://hg.mozilla.org/comm-central/rev/6bbaa1a0d296

Comment 11

[Axel Grude [:realRaven]]

Thanks Florian for taking this over. My build system defunct for the last fortnight :(