Closed Bug 755718 Opened 12 years ago Closed 12 years ago

There should be an audible notification when new messages are received on IRC

Categories

(Thunderbird :: Instant Messaging, defect)

defect
Not set
normal

Tracking

(thunderbird15 fixed)

RESOLVED FIXED
Thunderbird 16.0
Tracking Status
thunderbird15 --- fixed

People

(Reporter: mkaply, Assigned: florian)

References

Details

Attachments

(1 file, 2 obsolete files)

When I receive new messages on IRC specifically to me (private messages or by name), there should be an audio notification.
This is similar to bug 742746, but I'm not sure if it's exactly a duplicate.
(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.
Depends on: 742746
Attached patch Patch (obsolete) — Splinter Review
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.
Assignee: nobody → florian
Attachment #635360 - Flags: ui-review?(bwinton)
Attachment #635360 - Flags: review?(mbanner)
Attachment #635360 - Flags: review?(dbienvenu)
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.
(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?
(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.
(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.
Attached patch Patch v2 (obsolete) — Splinter Review
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.
Attachment #635360 - Attachment is obsolete: true
Attachment #635360 - Flags: ui-review?(bwinton)
Attachment #635360 - Flags: review?(mbanner)
Attachment #635360 - Flags: review?(dbienvenu)
Attachment #635492 - Flags: ui-review?(bwinton)
Attachment #635492 - Flags: review?(dbienvenu)
Attachment #635492 - Flags: review?(clokep)
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) {
Attachment #635492 - Flags: review?(dbienvenu) → review+
(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? ;)
(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 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).
Attachment #635492 - Flags: review?(clokep) → review+
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.
Attachment #635492 - Flags: ui-review?(bwinton) → ui-review+
Attached patch Patch v3Splinter Review
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.
Attachment #635492 - Attachment is obsolete: true
Attachment #637052 - Flags: review?(dbienvenu)
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.
(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.
Attachment #637052 - Flags: review?(dbienvenu) → review+
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).
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 16.0
Attachment #637052 - Flags: approval-comm-aurora?
Attachment #637052 - Flags: approval-comm-aurora? → approval-comm-aurora+
You need to log in before you can comment on or make changes to this bug.