Closed Bug 954382 Opened 10 years ago Closed 10 years ago

Finer control over sound notifications (i.e. receiving chat ping vs. instant message)

Categories

(Instantbird Graveyard :: Preferences, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: bugzilla, Assigned: mattdentremont)

Details

Attachments

(1 file, 5 obsolete files)

*** Original post on bio 948 by deOmega <jahkae AT gmail.com> at 2011-07-29 04:30:00 UTC ***

Options for turning off only certain sounds. I use chat for work a LOT
because my company's employees are spread all around the country. I want to be
able to receive chat notifications for jabber group chats, but I don't want
notifications when I send IMs. 

Filed from bug 954380 (bio 946).
https://bugzilla.mozilla.org/show_bug.cgi?id=954380 (bio 946)
*** Original post on bio 948 at 2011-07-29 10:46:51 UTC ***

Updating the topic to be more specific (as I originally thought this was about changing the sounds).

We split out buddy list and chat sounds in version 1.0, but it could probably use a bit further splitting, although it'll add a lot of preferences.

If all you want to do is shut off a particular sound, that can be done in an extension, see http://hg.instantbird.org/addons/file/tip/noblistsounds for an example.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 7 → All
Hardware: x86 → All
Summary: Customize sounds → Finer control over sound notifications (i.e. receiving chat ping vs. instant message)
*** Original post on bio 948 as attmnt 875 by mattdentremont AT gmail.com at 2011-10-12 02:21:00 UTC ***

Could you also set me as the assignee :)
Attachment #8352618 - Flags: review?(clokep)
*** Original post on bio 948 at 2011-10-12 02:32:59 UTC ***

This looks fairly straight forward and I think it'll work OK (I'll try it out tomorrow).

Is there really a reason to have the pref for all sounds? Would that ever get used? (I.e. can aEvent ever be an empty string? I don't think so.)

Also, for some background: I suggested we leave the original prefs (blist and message) for backward compatibility, flo please correct me if this is wrong.
Assignee: nobody → bugzilla
Status: NEW → ASSIGNED
*** Original post on bio 948 by v17al <mattdentremont AT gmail.com> at 2011-10-12 03:48:42 UTC ***

Comment on attachment 8352618 [details] [diff] [review] (bio-attmnt 875)
Patch to add hidden sound preferences

>diff --git a/instantbird/app/profile/all-instantbird.js b/instantbird/app/profile/all-instantbird.js
>--- a/instantbird/app/profile/all-instantbird.js
>+++ b/instantbird/app/profile/all-instantbird.js
>@@ -78,18 +78,30 @@ pref("extensions.mintrayr.alwaysShowTray
> #ifdef XP_UNIX
> // For Linux, use single click.
> pref("extensions.mintrayr.singleClickRestore", true);
> #else
> // For Windows, use double click.
> pref("extensions.mintrayr.singleClickRestore", false);
> #endif
> #endif
>+// For all message related sounds
> pref("messenger.options.playSounds.message", true);
>+// Specific message events, should have same value as ...playSounds.message
>+pref("messenger.options.playSounds.outgoing", true);
>+pref("messenger.options.playSounds.incoming", true);
>+pref("messenger.options.playSounds.alert", true);
>+// For all blist related sounds
> pref("messenger.options.playSounds.blist", false);
>+// Specific blist events, should have same value as ...playSounds.blist
>+pref("messenger.options.playSounds.login", false);
>+pref("messenger.options.playSounds.logout", false);
>+
>+// For all sounds
>+pref("messenger.options.playSounds.", true);
> 
> // this preference changes how we filter incoming messages
> // 0 = no formattings
> // 1 = basic formattings (bold, italic, underlined)
> // 2 = permissive mode (colors, font face, font size, ...)
> pref("messenger.options.filterMode", 2);
> 
> pref("font.default.x-western", "sans-serif");
>diff --git a/instantbird/modules/ibSounds.jsm b/instantbird/modules/ibSounds.jsm
>--- a/instantbird/modules/ibSounds.jsm
>+++ b/instantbird/modules/ibSounds.jsm
>@@ -48,17 +48,18 @@ var Sounds = {
>     outgoing: "chrome://instantbird-sounds/skin/send.wav",
>     login: "chrome://instantbird-sounds/skin/login.wav",
>     logout: "chrome://instantbird-sounds/skin/logout.wav",
>     alert: "chrome://instantbird-sounds/skin/alert.wav"
>   },
> 
>   play: function sh_play(aEvent, aPref, aSubject, aTopic) {
>     if (!Services.prefs.getBoolPref("messenger.options.playSounds." + aPref) ||
>-        !Interruptions.requestInterrupt(aTopic, aSubject, "sound"))
>+        !Services.prefs.getBoolPref("messenger.options.playSounds." + aEvent)
>+        || !Interruptions.requestInterrupt(aTopic, aSubject, "sound"))
>       return;
> 
>     new getHiddenHTMLWindow().Audio(this.soundFiles[aEvent])
>                              .setAttribute("autoplay", "true");
>   },
> 
>   observe: function(aObject, aTopic, aMsg) {
>     switch(aTopic) {
*** Original post on bio 948 as attmnt 876 by mattdentremont AT gmail.com at 2011-10-12 03:52:00 UTC ***

I noticed small mistake in the variable names while coding the add-on. Sorry for comment 4, I was trying to modify the old attachment and discovered that wasn't possible.
Attachment #8352619 - Flags: review?(clokep)
Comment on attachment 8352618 [details] [diff] [review]
Patch to add hidden sound preferences

*** Original change on bio 948 attmnt 875 by mattdentremont AT gmail.com at 2011-10-12 03:52:07 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352618 - Attachment is obsolete: true
Attachment #8352618 - Flags: review?(clokep)
Comment on attachment 8352619 [details] [diff] [review]
Fixed patch (incorrect variable names)

*** Original change on bio 948 attmnt 876 at 2011-10-12 09:14:16 UTC ***

>diff --git a/instantbird/app/profile/all-instantbird.js b/instantbird/app/profile/all-instantbird.js

>+// For all message related sounds
> pref("messenger.options.playSounds.message", true);
>+// Specific message events, should have same value as ...playSounds.message
>+pref("messenger.options.playSounds.outgoing", true);
>+pref("messenger.options.playSounds.incoming", true);
>+pref("messenger.options.playSounds.alert", true);
>+// For all blist related sounds
> pref("messenger.options.playSounds.blist", false);
>+// Specific blist events, should have same value as ...playSounds.blist

With the way you wrote the code, an event-specific pref with a value of false prevents the sound. To let the existing preferences (that are exposed in the UI) be effective, all these new preferences needs to have true as their default value.

>+pref("messenger.options.playSounds.login", false);
>+pref("messenger.options.playSounds.logout", false);


>+// For all sounds
>+pref("messenger.options.playSounds.", true);

As clokep pointed out, this won't be used. (Plus, it doesn't seem correct to have a preference name ending with a .)

>diff --git a/instantbird/modules/ibSounds.jsm b/instantbird/modules/ibSounds.jsm
>--- a/instantbird/modules/ibSounds.jsm
>+++ b/instantbird/modules/ibSounds.jsm
>@@ -48,17 +48,18 @@ var Sounds = {
>     outgoing: "chrome://instantbird-sounds/skin/send.wav",
>     login: "chrome://instantbird-sounds/skin/login.wav",
>     logout: "chrome://instantbird-sounds/skin/logout.wav",
>     alert: "chrome://instantbird-sounds/skin/alert.wav"
>   },
> 
>   play: function sh_play(aEvent, aPref, aSubject, aTopic) {
>     if (!Services.prefs.getBoolPref("messenger.options.playSounds." + aPref) ||
>-        !Interruptions.requestInterrupt(aTopic, aSubject, "sound"))
>+        !Services.prefs.getBoolPref("messenger.options.playSounds." + aEvent)
>+        || !Interruptions.requestInterrupt(aTopic, aSubject, "sound"))

Coding style: Please keep the || at the end of the line.
Attachment #8352619 - Flags: review?(clokep) → review-
Attached patch Post code review patch (obsolete) — Splinter Review
*** Original post on bio 948 as attmnt 878 by mattdentremont AT gmail.com at 2011-10-12 12:25:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352621 - Flags: review?(florian)
Comment on attachment 8352619 [details] [diff] [review]
Fixed patch (incorrect variable names)

*** Original change on bio 948 attmnt 876 by mattdentremont AT gmail.com at 2011-10-12 12:25:21 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352619 - Attachment is obsolete: true
Comment on attachment 8352621 [details] [diff] [review]
Post code review patch

*** Original change on bio 948 attmnt 878 at 2011-10-12 17:41:14 UTC ***

>diff --git a/instantbird/app/profile/all-instantbird.js b/instantbird/app/profile/all-instantbird.js
>--- a/instantbird/app/profile/all-instantbird.js
>+++ b/instantbird/app/profile/all-instantbird.js
>@@ -78,18 +78,27 @@ pref("extensions.mintrayr.alwaysShowTray
> #ifdef XP_UNIX
> // For Linux, use single click.
> pref("extensions.mintrayr.singleClickRestore", true);
> #else
> // For Windows, use double click.
> pref("extensions.mintrayr.singleClickRestore", false);
> #endif
> #endif
>+// For all message related sounds
> pref("messenger.options.playSounds.message", true);
>+// Specific message events
>+pref("messenger.options.playSounds.outgoing", true);
>+pref("messenger.options.playSounds.incoming", true);
>+pref("messenger.options.playSounds.alert", true);
>+// For all blist related sounds
> pref("messenger.options.playSounds.blist", false);
>+// Specific blist events
>+pref("messenger.options.playSounds.login", true);
>+pref("messenger.options.playSounds.logout", true);

The comments should be improved here. Someone reading the file needs to understand the effect of each preference and of their values.

>diff --git a/instantbird/modules/ibSounds.jsm b/instantbird/modules/ibSounds.jsm
>--- a/instantbird/modules/ibSounds.jsm
>+++ b/instantbird/modules/ibSounds.jsm
>@@ -48,16 +48,17 @@ var Sounds = {
>     outgoing: "chrome://instantbird-sounds/skin/send.wav",
>     login: "chrome://instantbird-sounds/skin/login.wav",
>     logout: "chrome://instantbird-sounds/skin/logout.wav",
>     alert: "chrome://instantbird-sounds/skin/alert.wav"
>   },
> 
>   play: function sh_play(aEvent, aPref, aSubject, aTopic) {
>     if (!Services.prefs.getBoolPref("messenger.options.playSounds." + aPref) ||
>+        !Services.prefs.getBoolPref("messenger.options.playSounds." + aEvent) ||

If you are concerned with the length of these lines (I'm myself more concerned with the code duplication), you can add a getBoolPref method:

    getBoolPref: function(aPrefName)
      Services.prefs.getBoolPref("messenger.options.playSounds." + aPrefName),

(If you don't know this short syntax for JS functions, see https://developer.mozilla.org/en/JavaScript/New_in_JavaScript/1.8#Expression_closures_%28Merge_into_own_page.2Fsection%29 )

Then in the play method you can check both prefs in a single < 80 columns line :-).

Looks good otherwise!
Attachment #8352621 - Flags: review?(florian) → review-
Attached patch Revised patch (obsolete) — Splinter Review
*** Original post on bio 948 as attmnt 881 by mattdentremont AT gmail.com at 2011-10-13 00:21:00 UTC ***

Thanks for the great feedback on the reviews :)
Attachment #8352624 - Flags: review?(florian)
Comment on attachment 8352621 [details] [diff] [review]
Post code review patch

*** Original change on bio 948 attmnt 878 by mattdentremont AT gmail.com at 2011-10-13 00:21:18 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352621 - Attachment is obsolete: true
Comment on attachment 8352624 [details] [diff] [review]
Revised patch

*** Original change on bio 948 attmnt 881 at 2011-10-13 00:30:40 UTC ***

>diff --git a/instantbird/app/profile/all-instantbird.js b/instantbird/app/profile/all-instantbird.js

>+// Whether message related sounds should be played at all. If this is enabled
>+// then the more specific prefs are checked as well.

Maybe add an empty line before this comment, as the messenger.options.playSounds block of options is becoming long, it would be more readable to separate it a bit.

> pref("messenger.options.playSounds.message", true);
>+// Specifies whether each message event should trigger a sound for
>+// incoming and outgoing messages, or when your nickname is mentioned in a chat

I wouldn't bother you with this (and would have just fixed it before committing the fix) if I didn't have a more serious comment later, but please finish your sentences with a point.

>+// Specifies whether sounds should be played on login/logout events

Here too.

By the way, your comments are good, very nice improvement! :-)

>diff --git a/instantbird/modules/ibSounds.jsm b/instantbird/modules/ibSounds.jsm
>--- a/instantbird/modules/ibSounds.jsm
>+++ b/instantbird/modules/ibSounds.jsm
>@@ -45,19 +45,24 @@ var Sounds = {
>   soundEvents: ["contact-signed-on", "contact-signed-off", "new-text"],
>   soundFiles: {
>     incoming: "chrome://instantbird-sounds/skin/receive.wav",
>     outgoing: "chrome://instantbird-sounds/skin/send.wav",
>     login: "chrome://instantbird-sounds/skin/login.wav",
>     logout: "chrome://instantbird-sounds/skin/logout.wav",
>     alert: "chrome://instantbird-sounds/skin/alert.wav"
>   },
>+  
>+  getBoolPref: function(aPrefName) {
>+    Services.prefs.getBoolPref("messenger.options.playSounds." + aPrefName);
>+  },

I bet you haven't actually tested this. As it is written, it always "returns" undefined.
Read my example again (and the link I gave). You should not have these curly braces here.

>   play: function sh_play(aEvent, aPref, aSubject, aTopic) {
>-    if (!Services.prefs.getBoolPref("messenger.options.playSounds." + aPref) ||
>+    if (!this.getBoolPref(aPref) ||
>+        !this.getBoolPref(aEvent) ||

Put these 2 tests on the same line, like I said in comment 8.
Attachment #8352624 - Flags: review?(florian) → review-
Attached patch Hopefully the final version :S (obsolete) — Splinter Review
*** Original post on bio 948 as attmnt 882 by mattdentremont AT gmail.com at 2011-10-13 00:36:00 UTC ***

Sorry for the terrible results of my patches, trying to do way too much at once I think :(.
Attachment #8352625 - Flags: review?(clokep)
Comment on attachment 8352624 [details] [diff] [review]
Revised patch

*** Original change on bio 948 attmnt 881 by mattdentremont AT gmail.com at 2011-10-13 00:36:22 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352624 - Attachment is obsolete: true
Comment on attachment 8352625 [details] [diff] [review]
Hopefully the final version :S

*** Original change on bio 948 attmnt 882 at 2011-10-13 00:40:19 UTC ***

Please read again the 3 first points of comment 10.
Attachment #8352625 - Flags: review?(clokep) → review-
*** Original post on bio 948 as attmnt 883 by mattdentremont AT gmail.com at 2011-10-13 00:46:00 UTC ***

Wow, I cannot read at all
Attachment #8352626 - Flags: review?(florian)
Comment on attachment 8352625 [details] [diff] [review]
Hopefully the final version :S

*** Original change on bio 948 attmnt 882 by mattdentremont AT gmail.com at 2011-10-13 00:46:36 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352625 - Attachment is obsolete: true
Comment on attachment 8352626 [details] [diff] [review]
End of the batch of patching mistakes

*** Original change on bio 948 attmnt 883 at 2011-10-13 01:18:40 UTC ***

This looks good, I'm not sure if flo will have more comments about the comments. I tested this briefly and it seems to work as expected.
Attachment #8352626 - Flags: review?(florian) → review+
Attachment #8352626 - Flags: review?(florian)
Comment on attachment 8352626 [details] [diff] [review]
End of the batch of patching mistakes

*** Original change on bio 948 attmnt 883 at 2011-10-15 22:05:21 UTC ***

>diff --git a/instantbird/app/profile/all-instantbird.js b/instantbird/app/profile/all-instantbird.js

Trailing whitespace here:

>+// Whether contact list related sounds should be played at all. If this is 

>diff --git a/instantbird/modules/ibSounds.jsm b/instantbird/modules/ibSounds.jsm

And here:

>+  
>+  getBoolPref: function(aPrefName)
>+    Services.prefs.getBoolPref("messenger.options.playSounds." + aPrefName),

Looks great otherwise, thanks! :-)
Attachment #8352626 - Flags: review?(florian) → review+
*** Original post on bio 948 at 2011-10-20 22:43:53 UTC ***

Congrats on your first check-in! http://hg.instantbird.org/instantbird/rev/00f58c19af0e
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.2
*** Original post on bio 948 by Jake <jake.lauer AT gmail.com> at 2012-10-30 15:56:35 UTC ***

Did this end up making it into the product anywhere? I don't see it in any options in the nightly release. Don't mean to be pushy, just curious. Thanks!
*** Original post on bio 948 at 2012-10-30 16:40:15 UTC ***

(In reply to comment #17)
> Did this end up making it into the product anywhere? I don't see it in any
> options in the nightly release. Don't mean to be pushy, just curious. Thanks!

Yes, it is in Instantbird 1.2 (for reference, we use the Target Milestone field to show what release version of Instantbird a change was made for). There's no UI for it, however. https://addons.instantbird.org/en-US/instantbird/addon/306 does add the options to the UI.

Alternately you could flip them in about:config, filter on messenger.options.playSounds
There was missing email mapping information for this bug during the BIO to BMO merge, manually assigning this bug.
Assignee: bugzilla → mattdentremont
You need to log in before you can comment on or make changes to this bug.