Last Comment Bug 778114 - chat sound not playing if mail notification sound disabled
: chat sound not playing if mail notification sound disabled
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Instant Messaging (show other bugs)
: 15 Branch
: All All
: -- trivial (vote)
: Thunderbird 17.0
Assigned To: Axel Grude [:realRaven]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-27 06:07 PDT by Axel Grude [:realRaven]
Modified: 2012-09-26 04:01 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
moved check out of PlayBiffSound into folder listener (5.05 KB, patch)
2012-07-28 15:16 PDT, Axel Grude [:realRaven]
no flags Details | Diff | Review
Same patch as before w/o whitespace mods (1.89 KB, patch)
2012-07-28 15:36 PDT, Axel Grude [:realRaven]
no flags Details | Diff | Review
Samne patch without duplicate comment (1.83 KB, patch)
2012-07-28 15:43 PDT, Axel Grude [:realRaven]
no flags Details | Diff | Review
This one even compiles :P (2.01 KB, patch)
2012-07-28 16:17 PDT, Axel Grude [:realRaven]
no flags Details | Diff | Review
moved a comment (2.06 KB, patch)
2012-07-28 16:36 PDT, Axel Grude [:realRaven]
irving: review+
Details | Diff | Review

Description Axel Grude [:realRaven] 2012-07-27 06:07:41 PDT
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] 2012-07-27 06:13:19 PDT
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] 2012-07-28 15:16:46 PDT
Created attachment 646910 [details] [diff] [review]
moved check out of PlayBiffSound into folder listener
Comment 3 Axel Grude [:realRaven] 2012-07-28 15:36:55 PDT
Created attachment 646913 [details] [diff] [review]
Same patch as before w/o whitespace mods
Comment 4 Axel Grude [:realRaven] 2012-07-28 15:43:10 PDT
Created attachment 646915 [details] [diff] [review]
Samne patch without duplicate comment

removed duplicate comment
Comment 5 Axel Grude [:realRaven] 2012-07-28 16:17:55 PDT
Created attachment 646921 [details] [diff] [review]
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] 2012-07-28 16:36:26 PDT
Created attachment 646925 [details] [diff] [review]
moved a comment

moved a comment to a more appropriate place
Comment 7 Florian Quèze [:florian] [:flo] 2012-08-03 06:53:05 PDT
Axel, is it intentional that there's no review request on the patch attached in comment 6?
Comment 8 Axel Grude [:realRaven] 2012-08-03 06:57:56 PDT
probably not, who should I add for this one?
Comment 9 :Irving Reid (No longer working on Firefox) 2012-08-08 06:50:19 PDT
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) 2012-08-20 12:55:26 PDT
Pushed to comm-central trunk:

https://hg.mozilla.org/comm-central/rev/6bbaa1a0d296
Comment 11 Axel Grude [:realRaven] 2012-08-20 13:05:38 PDT
Thanks Florian for taking this over. My build system defunct for the last fortnight :(

Note You need to log in before you can comment on or make changes to this bug.