chat sound not playing if mail notification sound disabled

RESOLVED FIXED in Thunderbird 17.0

Status

Thunderbird
Instant Messaging
--
trivial
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: realRaven, Assigned: realRaven)

Tracking

15 Branch
Thunderbird 17.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

5 years ago
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
(Assignee)

Updated

5 years ago
Severity: normal → trivial
(Assignee)

Updated

5 years ago
Component: General → Instant Messaging
(Assignee)

Comment 1

5 years ago
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...
(Assignee)

Comment 2

5 years ago
Created attachment 646910 [details] [diff] [review]
moved check out of PlayBiffSound into folder listener
Assignee: nobody → axelg
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
OS: Windows XP → All
Hardware: x86 → All
(Assignee)

Comment 3

5 years ago
Created attachment 646913 [details] [diff] [review]
Same patch as before w/o whitespace mods
Attachment #646910 - Attachment is obsolete: true
Attachment #646913 - Flags: review?
(Assignee)

Comment 4

5 years ago
Created attachment 646915 [details] [diff] [review]
Samne patch without duplicate comment

removed duplicate comment
Attachment #646913 - Attachment is obsolete: true
Attachment #646913 - Flags: review?
Attachment #646915 - Flags: review?(irving)
(Assignee)

Comment 5

5 years ago
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.
Attachment #646915 - Attachment is obsolete: true
Attachment #646915 - Flags: review?(irving)
(Assignee)

Comment 6

5 years ago
Created attachment 646925 [details] [diff] [review]
moved a comment

moved a comment to a more appropriate place
Attachment #646921 - Attachment is obsolete: true
Axel, is it intentional that there's no review request on the patch attached in comment 6?
(Assignee)

Comment 8

5 years ago
probably not, who should I add for this one?
Attachment #646925 - Flags: review?(irving)
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.
Attachment #646925 - Flags: review?(irving) → review+
Pushed to comm-central trunk:

https://hg.mozilla.org/comm-central/rev/6bbaa1a0d296
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Comment 11

5 years ago
Thanks Florian for taking this over. My build system defunct for the last fortnight :(
Target Milestone: --- → Thunderbird 17.0
You need to log in before you can comment on or make changes to this bug.