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)
Thunderbird
Instant Messaging
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)
6.91 KB,
patch
|
Bienvenu
:
review+
standard8
:
approval-comm-aurora+
|
Details | Diff | Splinter Review |
When I receive new messages on IRC specifically to me (private messages or by name), there should be an audio notification.
Assignee | ||
Comment 1•12 years ago
|
||
This is similar to bug 742746, but I'm not sure if it's exactly a duplicate.
Comment 2•12 years ago
|
||
(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
Assignee | ||
Comment 3•12 years ago
|
||
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)
Comment 4•12 years ago
|
||
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•12 years ago
|
||
(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?
Assignee | ||
Comment 6•12 years ago
|
||
(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.
Assignee | ||
Comment 7•12 years ago
|
||
(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.
Assignee | ||
Comment 8•12 years ago
|
||
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 9•12 years ago
|
||
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+
Assignee | ||
Comment 10•12 years ago
|
||
(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•12 years ago
|
||
(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•12 years ago
|
||
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 13•12 years ago
|
||
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+
Assignee | ||
Comment 14•12 years ago
|
||
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)
Comment 15•12 years ago
|
||
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.
Assignee | ||
Comment 16•12 years ago
|
||
(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.
Updated•12 years ago
|
Attachment #637052 -
Flags: review?(dbienvenu) → review+
Assignee | ||
Comment 17•12 years ago
|
||
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
Assignee | ||
Updated•12 years ago
|
Attachment #637052 -
Flags: approval-comm-aurora?
Updated•12 years ago
|
Attachment #637052 -
Flags: approval-comm-aurora? → approval-comm-aurora+
Assignee | ||
Comment 18•12 years ago
|
||
https://hg.mozilla.org/releases/comm-aurora/rev/4a79d4a9e15d
status-thunderbird15:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•