Last Comment Bug 755718 - There should be an audible notification when new messages are received on IRC
: There should be an audible notification when new messages are received on IRC
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Instant Messaging (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Thunderbird 16.0
Assigned To: Florian Quèze [:florian] [:flo]
:
:
Mentors:
: 735819 (view as bug list)
Depends on: 742746
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-16 06:34 PDT by Mike Kaply [:mkaply]
Modified: 2012-08-15 01:27 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed


Attachments
Patch (5.42 KB, patch)
2012-06-21 10:18 PDT, Florian Quèze [:florian] [:flo]
no flags Details | Diff | Splinter Review
Patch v2 (5.71 KB, patch)
2012-06-21 14:53 PDT, Florian Quèze [:florian] [:flo]
mozilla: review+
clokep: review+
bwinton: ui‑review+
Details | Diff | Splinter Review
Patch v3 (6.91 KB, patch)
2012-06-27 02:56 PDT, Florian Quèze [:florian] [:flo]
mozilla: review+
standard8: approval‑comm‑aurora+
Details | Diff | Splinter Review

Description Mike Kaply [:mkaply] 2012-05-16 06:34:22 PDT
When I receive new messages on IRC specifically to me (private messages or by name), there should be an audio notification.
Comment 1 Florian Quèze [:florian] [:flo] 2012-05-16 06:40:27 PDT
This is similar to bug 742746, but I'm not sure if it's exactly a duplicate.
Comment 2 Ludovic Hirlimann [:Usul] 2012-06-05 06:44:39 PDT
(In reply to Florian Quèze from comment #1)
> This is similar to bug 742746, but I'm not sure if it's exactly a duplicate.

if it's not it kinds of depend on some notification system for chat.
Comment 3 Florian Quèze [:florian] [:flo] 2012-06-21 10:18:58 PDT
Created attachment 635360 [details] [diff] [review]
Patch

The sound part was easier than the other part of the "we need notifications for new IMs like for new emails" because the sound is handled by the same file for all OSes.

Blake, this plays the same sounds as when new emails are received; each time an IM directed to the user (so either a private message, or a message containing the user's nick in an IRC channel or on Twitter) arrives.
Comment 4 :Irving Reid (No longer working on Firefox) 2012-06-21 12:15:29 PDT
Florian, I've been working on pulling the "when should we notify" logic for mail messages into a new module (bug 715799), with the thought that eventually it might also handle non-email notifications.

I'd rather see the "what IM messages are specifically interesting" logic somewhere other than in the layer of code that actually posts the notification to the user, but I must admit that I don't have a clear design in mind yet for how to organize it.
Comment 5 David :Bienvenu 2012-06-21 13:20:38 PDT
(In reply to Irving Reid (:irving) from comment #4)

> 
> I'd rather see the "what IM messages are specifically interesting" logic
> somewhere other than in the layer of code that actually posts the
> notification to the user, but I must admit that I don't have a clear design
> in mind yet for how to organize it.

yeah, I agree with Irving here. Could the code generating the notification only do so for things that should cause a sound to play?
Comment 6 Florian Quèze [:florian] [:flo] 2012-06-21 13:24:20 PDT
(In reply to Irving Reid (:irving) from comment #4)
> Florian, I've been working on pulling the "when should we notify" logic for
> mail messages into a new module (bug 715799), with the thought that
> eventually it might also handle non-email notifications.

Mark showed me the bug, and I look forward these improvements, but my understanding is that the patch from bug 715799 is still under ongoing development, and impossible to take to aurora because it changes interfaces.

Here I'm looking for a short time solution that could land on aurora to be part of Tb15 (the first release with IM pref'ed on by default).
I should have mentioned in comment 3 that I don't expect this code to stay in the tree for a long time. I actually look forward to it being removed, as I dislike having that logic hard coded in C++ in a way that's unreachable to add-ons.

> I'd rather see the "what IM messages are specifically interesting" logic
> somewhere other than in the layer of code that actually posts the
> notification to the user, but I must admit that I don't have a clear design
> in mind yet for how to organize it.

I would really prefer to have it in JS somewhere else, but currently in that file, the code that decides if email notifications are needed is the OnItemIntPropertyChanged method, and the code actually posting the notification is PlayBiffSound that isn't scriptable, so I'm afraid there's no other possible place.

The only other option I saw was to fully duplicate the logic of the PlayBiffSound method in a JS component handling sounds for IMs. That seems almost as bad. The reason why I decided against doing this is that it can work for sounds, but wouldn't for the systray icon, for which no scriptable interface exist in the platform, and I prefer to handle all kinds of notifications consistently.
Comment 7 Florian Quèze [:florian] [:flo] 2012-06-21 13:25:46 PDT
(In reply to David :Bienvenu from comment #5)

> yeah, I agree with Irving here. Could the code generating the notification
> only do so for things that should cause a sound to play?

Hmm, yeah, we could generate another different notification when a new message directed to the user arrives.
Comment 8 Florian Quèze [:florian] [:flo] 2012-06-21 14:53:00 PDT
Created attachment 635492 [details] [diff] [review]
Patch v2

Now adding a new-directed-incoming-message notification fired from imConversations.js. I couldn't really decide if it's useful to send that notification to the conversation in addition to sending it to the observers service, so I did it, as it probably won't hurt.
Comment 9 David :Bienvenu 2012-06-21 15:10:30 PDT
Comment on attachment 635492 [details] [diff] [review]
Patch v2

that all looks fine, except for missing spaces between rv,rv and between if and (

+  NS_ENSURE_SUCCESS(rv,rv);
+
+  bool chatEnabled = false;
+  if(NS_SUCCEEDED(rv))
+    rv = pref->GetBoolPref(PREF_CHAT_ENABLED, &chatEnabled);
+  if(NS_SUCCEEDED(rv) && chatEnabled) {
Comment 10 Florian Quèze [:florian] [:flo] 2012-06-21 15:23:02 PDT
(In reply to David :Bienvenu from comment #9)
> Comment on attachment 635492 [details] [diff] [review]
> Patch v2
> 
> that all looks fine, except for missing spaces between rv,rv and between if
> and (

I dislike it too, but it seemed to be the dominant style in the existing file.
http://mxr.mozilla.org/comm-central/source/mailnews/base/src/nsStatusBarBiffManager.cpp#59
Should I fix lines 59 70 74 85 and 96 too? ;)
Comment 11 David :Bienvenu 2012-06-21 15:39:43 PDT
(In reply to Florian Quèze from comment #10)
> (In reply to David :Bienvenu from comment #9)

> Should I fix lines 59 70 74 85 and 96 too? ;)

you can if you want, but I mostly care about new code.

prevalent style is only used for a few things. Braces is the only thing I can think of that...
Comment 12 Patrick Cloke [:clokep] 2012-06-22 03:56:28 PDT
Comment on attachment 635492 [details] [diff] [review]
Patch v2

r+ on the chat/ part. Please file a bug with Instantbird to update for this change (as you said on IRC, it affects at least: ibSounds.jsm, ibNotifications.jsm and (maybe) conversation.xml).
Comment 13 Blake Winton (:bwinton) (:☕️) 2012-06-26 12:34:16 PDT
Comment on attachment 635492 [details] [diff] [review]
Patch v2

I think we want a "mail.chat.play_sound" (hidden) pref, but other than that, it seems good.  ui-r=me!

Thanks,
Blake.
Comment 14 Florian Quèze [:florian] [:flo] 2012-06-27 02:56:18 PDT
Created attachment 637052 [details] [diff] [review]
Patch v3

This adds a "mail.chat.play_biff_sound" hidden pref to turn of IM sounds, as requested in comment 13.

I'm not really sure if this needs a new review or not from David, this new patch adds 10 lines of code, so requesting it just in case.
Comment 15 :Irving Reid (No longer working on Firefox) 2012-06-27 06:17:56 PDT
1) does this need to be controlled separately from the existing mail.biff.play_sound pref?

2) Can we stop using the word "biff" for notifications? It's not meaningful to anyone not steeped in UNIX lore. "mail.chat.play_notification_sound" or play_alert_sound would be better.
Comment 16 Florian Quèze [:florian] [:flo] 2012-06-27 06:38:54 PDT
(In reply to Irving Reid (:irving) from comment #15)
> 1) does this need to be controlled separately from the existing
> mail.biff.play_sound pref?

Yes. The reason for adding this pref is that we expect the sound to be played way more frequently for new IM than for new emails, and it may be annoying to some users. The pref is to let users disable the sound for IM but not for email.

And in the future, I think we will want to let the user have a different sound for new email and new IM.

> 2) Can we stop using the word "biff" for notifications? It's not meaningful
> to anyone not steeped in UNIX lore. "mail.chat.play_notification_sound" or
> play_alert_sound would be better.

I used "biff" in the name to make it clear that the sounds that's going to be played is the one controlled by the mail.biff.play_sound* preferences.

When using different sounds for email and IM, I don't think the pref for the IM sound will still contain the "biff" word.
Comment 17 Florian Quèze [:florian] [:flo] 2012-06-28 09:04:46 PDT
I've checked this in as https://hg.mozilla.org/comm-central/rev/a960462c9a18 with "mail.chat.play_notification_sound" as the preference name, as David agreed with Irving (see bug 767987 comment 10).
Comment 18 Florian Quèze [:florian] [:flo] 2012-07-16 05:39:31 PDT
https://hg.mozilla.org/releases/comm-aurora/rev/4a79d4a9e15d
Comment 19 Ludovic Hirlimann [:Usul] 2012-08-15 01:27:14 PDT
*** Bug 735819 has been marked as a duplicate of this bug. ***

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