742746 - Directed messages should be able to hook into system prompts/give notifications (similar style to new mail alerts)

Status: RESOLVED FIXED

(Thunderbird :: Instant Messaging, defect)

Description

[Mark Banner (:standard8)]

At the moment if someone mentions my name in irc, I get a bolded message, however if I'm not using Thunderbird or it isn't visible, then I've got no other indication available to me.

Chat should integrate in a similar way as the new mail notification - an option for an alert sound, plus a sliding alert, or growl notification as appropriate to the platform the user is on.

Comment 1

[Florian Quèze [:florian]]

Bug 735819 is related, but not sure if it's exactly a dup.

Comment 2

[Florian Quèze [:florian]]

Irving, I think you have some significant experience hacking the new message notifications, do you have some idea of how this can be done? The last time I looked at the code of the current notifications, it seemed to rely on receiving notifications directly from the folder objects, which seemed a bit scary.

Comment 3

[:Irving Reid (No longer working on Firefox)]

At this point we don't expose APIs that go directly to the underlying OS's notification system. It would be worth looking at what the browser/platform layer have for that, and use it both for IM and mail notification.

Comment 4

[Wladimir Palant]

Attachment: Stopgap solution

This is a very simple extension that will show a notification via nsIAlertsService for each directed message. Obviously, this is not a perfect solution: messages appear even if you are currently in the chat window and they disappear faster than you can read them. Also, clicking the message doesn't do anything. But it is at least a notification.

Comment 6

[Brad]

From my dup:


Actual results:

Nothing. I didn't notice it.  When I opened Thunderbird again, the Chat tab did have a red (1).


Expected results:

Like in Instantbird and Thunderbird email, there should be an option for when new messages arrive, to:
-Show an alert - Windows popup summary of the message, Growl OSX, etc
-Play a sound

Since it's entirely possible that users would not want either of these for email but would want them for chats, the options should just be duplicated from the Options General tab into the Options Chat tab.


I'd really love to poke around the code to be able to help some day, but the code base is a bit daunting - is there good documentation somewhere that I could start from?

Comment 7

[Wayne Mery (:wsmwk)]

https://getsatisfaction.com/mozilla_messaging/tags/bug_742746 currently has about 70 interested parties. That's a pretty high number. (After I merged a bunch of topics)  However, some of these people where specifically interested sound - which got some fixes in TB16 and TB17

Comment 8

[Landry Breuil (:gaston)]

the stopgap solution in comment 4 is a good POC, works here (okay, it dumps the whole xml of the message in the popup).

I've deployed an xmpp server for my coworkers and we use it via thunderbird, so it'd be nice to have visual notifications in addition to the sound added in bug 755718.

Comment 10

[Richard Marti (:Paenglab)]

A up to Daily working extension which also provides a different notification sound can be found here: https://addons.mozilla.org/thunderbird/addon/thunderbird-chat-notificati/

Comment 11

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v1

Possible fix.
Please let me know if this isn't correct.

Thanks.

Comment 12

[aleth [:aleth]]

Comment on attachment 8381429 [details] [diff] [review]
Patch v1

Review of attachment 8381429 [details] [diff] [review]:
-----------------------------------------------------------------

This desperately needs a pref to allow turning the notifications off. It should probably be exposed in Preferences. Maybe there is already a notification pref that can be used for this, to avoid adding a new one?

Please, as I already recommended on IRC, take a look at ibNotifications.jsm to avoid reintroducing edge cases and missing details which have already been solved there. Examples (in addition to the pref) include action messages and the ellipsis.

FYI, you should r?florian for the final review as he knows the surrounding code better.

::: mail/components/im/content/chat-messenger-overlay.js
@@ +273,5 @@
> +
> +  showChatNotification: function(aSubject, aData) {
> +   if (!aSubject.incoming || aSubject.system ||
> +       (aSubject.conversation.isChat && !aSubject.containsNick))
> +     return;

This smells like duplication. Why not observe "new-directed-incoming-message" given that you are making this a TB-specific implementation?

Comment 13

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v2

Okay, so this is another approach as suggested by aleth.
We can remove ibNotifications.jsm altogether but then the
differenced between imNotifications.jsm ibNotifications.jsm
need to be handled somewhere. E.g. the ibInterruptions.jsm
use and imWindows.jsm use and the prefName.

Thanks.

Comment 14

[Suyash Agarwal (:sshagarwal)]

Also, this chat sound pref is hidden (maybe this is the same mail sound pref that is visible). But for notifications, do we need another visible pref? I think yes, rest is what you say.

Comment 15

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v2.5

Updated patch with pref.

Comment 16

[Suyash Agarwal (:sshagarwal)]

This patch shows the desktop notifications even if the TB chat window is open. I think that is undesired and I'll turn if off if agreed upon.
And, as :Paenglab suggested, we should have a different sound for chat messages. So, should I separate the sound pref too?

Comment 17

[aleth [:aleth]]

I think it would be a good idea, before reviewing this further, to spec out precisely the expected behaviour for TB. In particular

* Is there an existing pref to turn mail notification messages on/off? Should it apply to chat notifications?
* Is there an existing pref to turn mail notification sounds on/off? Should it apply to chat notifications?
* Are these prefs exposed in the Preference pane so a user can turn these potential annoyances off?
* What are the defaults going to be?
* Under what circumstances should a notification happen? Only when TB does not have focus? Only for the conversation not currently displayed in the chat pane? What's the existing story for mail notifications (let's be consistent if possible)?

This probably needs a UI-review.

Comment 18

[Suyash Agarwal (:sshagarwal)]

(In reply to aleth from comment #17)
> I think it would be a good idea, before reviewing this further, to spec out
> precisely the expected behaviour for TB. In particular

Thanks for helping me in setting up the description for ui-review.

> * Is there an existing pref to turn mail notification messages on/off?
Yes, there is one under the "General" tab (Options dialog box).
> Should it apply to chat notifications?
I don't think it should. It is sometimes required that one has mail notifications
turned on but not chat notifications and vice-versa. Moreover, as we already have
a "Chat" tab available, I think it will be good if we can have it separately
under its own tab.

> * Is there an existing pref to turn mail notification sounds on/off?
Yes there is. Custom sounds are also allowed.
> Should it apply to chat notifications?
Maybe/maybe not. :Paenglab suggested on the channel that we should have separate
sound alert for Chat (different from mail). Also, that would be good to distinguish
between chat notification and new mail notification.

> * Are these prefs exposed in the Preference pane so a user can turn these
> potential annoyances off?
Yes they are. One can even customize the new mail notification. So as to choose
what all details are to be displayed in the notification. I don't think we need
this customization. We are showing the chat buddy name and the message (truncated
to the available width).

> * What are the defaults going to be?
Defaults would be yes, as for mail notifications.

> * Under what circumstances should a notification happen? Only when TB does
> not have focus? Only for the conversation not currently displayed in the
> chat pane? What's the existing story for mail notifications (let's be
> consistent if possible)?
TB new mail notifications don't appear when the window has focus.
For chat notifications, I would suggest the notification to show up when the
window doesn't have focus. When the window has focus but the conversation isn't
open, I don't think we need a notification. The bold unread message count would
suffice. Moreover, when we will have multiple chats in the same tab soon, I think
this behavior will continue to work.

Also for an existing implementation, it would be nice if we can have the Instantbird
behavior available for us to decide. And please let me know if I have missed some
points. And do make the corrections.

Thanks.

Comment 19

[Suyash Agarwal (:sshagarwal)]

Comment on attachment 8384098 [details] [diff] [review]
Patch v2.5

Patch expected behavior - Comment 18.

Comment 20

[aleth [:aleth]]

(In reply to Suyash Agarwal (:sshagarwal) away till 7th of March from comment #18)

So to summarize (part of) what you said, your proposal is to add two new checkboxes to the TB Preferences.

> Also for an existing implementation, it would be nice if we can have the
> Instantbird behavior available for us to decide.

Instantbird has separate, exposed checkboxes to turn message notifications and messaging event sounds on/off, and does not show new messsage notifications if Instantbird has focus.

Comment 21

[Patrick Cloke [:clokep]]

Comment on attachment 8384098 [details] [diff] [review]
Patch v2.5

Review of attachment 8384098 [details] [diff] [review]:
-----------------------------------------------------------------

My understanding is that imNotifications is supposed to replace ibNotifications, this patch doesn't seem to remove ibNotifications, but it does stop building it. This doesn't make any sense. Additionally, you'd have to replace usage of ibNotifications with imNotifications.

Is imNotifications simple a rename of ibNotifications? If no, I'm unsure how these are supposed to relate (given the above).

Comment 22

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v2.7

Okay, so I forked ibNotifications.jsm as discussed on the channel.
This patch provides separate pref for enabling/disabling desktop chat notifications (exposed in the UI under Chat tab in Options dialog window).
True, by default.
Clicking on the notification takes you to the chat tab.
Please let me know the changes to be made.

Thanks.

Comment 23

[aleth [:aleth]]

Comment on attachment 8384735 [details] [diff] [review]
Patch v2.7

Review of attachment 8384735 [details] [diff] [review]:
-----------------------------------------------------------------

::: chat/modules/imNotifications.jsm
@@ +1,4 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +

This file is not shared with Instantbird and so should live in mail/.

@@ +45,5 @@
> +      if (buddy)
> +        icon = buddy.buddyIconFilename;
> +    }
> +    if (!icon)
> +      icon = "chrome://instantbird/skin/newMessage.png";

I'm pretty sure this won't work in TB.

@@ +55,5 @@
> +        if (aTopic == "alertclickcallback") {
> +          let win = Services.wm.getMostRecentWindow("mail:3pane");
> +          if (win) {
> +            win.focus();
> +            win.showChatTab();

But does this focus the right conversation?

@@ +83,5 @@
> +
> +  _notificationPrefName: "mail.chat.show_desktop_notifications",
> +  observe: function(aSubject, aTopic, aData) {
> +    if (aTopic != "new-text")
> +      return;

Can we save the following if clause by using new-directed-... or is there some difference?

::: mail/components/preferences/chat.xul
@@ +29,5 @@
>        <preference id="purple.logging.log_chats"            name="purple.logging.log_chats"            type="bool"/>
>        <preference id="purple.logging.log_ims"              name="purple.logging.log_ims"              type="bool"/>
>        <preference id="purple.logging.log_system"           name="purple.logging.log_system"           type="bool"/>
>        <preference id="pref.privacy.disable_button.view_passwords" name="pref.privacy.disable_button.view_passwords" type="bool"/>
> +	  <preference id="mail.chat.show_desktop_notifications" name="mail.chat.show_desktop_notifications" type="bool"/>

The indentation looks wrong?

::: mail/locales/en-US/chrome/messenger/preferences/chat.dtd
@@ +21,5 @@
>  <!ENTITY  andSetStatusToAway.accesskey  "A">
>  
>  <!ENTITY  sendTyping.label              "Send typing notifications in conversations">
>  <!ENTITY  sendTyping.accesskey          "t">
> +<!ENTITY  desktopChatNotifications.label "Show desktop notifications for incoming chat messages">

In Instantbird, the equivalent string is "When receiving a new message, notify of messages received in inactive windows". I'll leave it up to the UI reviewer to decide whether the string here is understandable enough.

Comment 24

[aleth [:aleth]]

(In reply to aleth from comment #23)
> The indentation looks wrong?
Maybe just a BMO display artifact.

Comment 25

[Florian Quèze [:florian]]

(In reply to aleth from comment #24)
> (In reply to aleth from comment #23)
> > The indentation looks wrong?
> Maybe just a BMO display artifact.

Not BMO's fault :-). It's because there are mixed tabs and spaces for the indentation of that line.

Comment 26

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v3

(In reply to aleth from comment #23)
> > +            win.focus();
> > +            win.showChatTab();
> 
> But does this focus the right conversation?
It is the same function call that "cmd_chat" command executes
so this should work. Currently this is broken due to some other
issue not related to my patch.

Please let me know if some other nits are to be addressed before I can request review.

Thanks.

Comment 27

[aleth [:aleth]]

(In reply to Suyash Agarwal (:sshagarwal) away till 7th of March from comment #26)
> Created attachment 8388224 [details] [diff] [review]
> Patch v3
> 
> (In reply to aleth from comment #23)
> > > +            win.focus();
> > > +            win.showChatTab();
> > 
> > But does this focus the right conversation?
> It is the same function call that "cmd_chat" command executes
> so this should work. Currently this is broken due to some other
> issue not related to my patch.

Can you be a little bit more specific? If there is another bug, please link to it or file it.

Comment 28

[Suyash Agarwal (:sshagarwal)]

(In reply to aleth from comment #27)
> Can you be a little bit more specific? If there is another bug, please link
> to it or file it.

Ya, it was from Bug 980089. The patch solved the issue.
Thanks.

Comment 29

[aleth [:aleth]]

Comment on attachment 8388224 [details] [diff] [review]
Patch v3

This looks ready for review, r?florian.

Comment 30

[Suyash Agarwal (:sshagarwal)]

Comment on attachment 8388224 [details] [diff] [review]
Patch v3

(In reply to aleth from comment #29)
> Comment on attachment 8388224 [details] [diff] [review]
> Patch v3
> 
> This looks ready for review, r?florian.

Thanks.

Comment 31

[Florian Quèze [:florian]]

Comment on attachment 8388224 [details] [diff] [review]
Patch v3

Review of attachment 8388224 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/components/im/modules/imNotifications.jsm
@@ +7,5 @@
> +Components.utils.import("resource:///modules/imServices.jsm");
> +Components.utils.import("resource:///modules/hiddenWindow.jsm");
> +
> +var Notifications = {
> +  mainWindow: Services.wm.getMostRecentWindow("mail:3pane"),

I don't know what this is trying to do, but it's probably not doing what's intended. This line will keep a reference to the most recent mail 3 pane window that was open at the time this .jsm file is first loaded. This is likely to keep the window alive even after it's closed.

@@ +24,5 @@
> +  _showMessageNotification: function (aMessage) {
> +    // Put the message content into a div node of the hidden HTML window.
> +    let doc = getHiddenHTMLWindow().document;
> +    let xhtmlElement = doc.createElementNS("http://www.w3.org/1999/xhtml", "div");
> +    xhtmlElement.innerHTML = aMessage.message.replace(/<br>/gi, "<br/>");

I wonder if the hidden window + innerHTML trick is still required, or if the DOM parser could now be used instead. See http://mxr.mozilla.org/comm-beta/source/chat/modules/imContentSink.jsm#345 for an example of DOM parser usage.

@@ +48,5 @@
> +        icon = buddy.buddyIconFilename;
> +    }
> +    if (!icon) {
> +      icon = "chrome://chat/skin/newMessage.png";
> +	  }

Wrong indent.

@@ +53,5 @@
> +
> +    // Prepare an observer to focus the conversation if the
> +    // notification is clicked.
> +    let observer = {
> +      observe: function(aSubject, aTopic, aData) {

You can directly use a function as an nsIObserver, you don't need the {observe: ...} wrapper object.

@@ +55,5 @@
> +    // notification is clicked.
> +    let observer = {
> +      observe: function(aSubject, aTopic, aData) {
> +        if (aTopic == "alertclickcallback") {
> +          this.mainWindow = Services.wm.getMostRecentWindow("mail:3pane");

Did you mean |let mainWindow =| here?

@@ +81,5 @@
> +                .showAlertNotification(icon, name, messageText, true, "", observer);
> +  },
> +
> +  init: function() {
> +	Services.obs.addObserver(Notifications, "new-directed-incoming-message", false);

Please avoid tabs when indenting.

::: mail/components/im/moz.build
@@ +10,5 @@
>  ]
>  
>  EXTRA_JS_MODULES += [
>      'modules/chatHandler.jsm',
> +    'modules/imNotifications.jsm',

The 'im' prefix is usually used for files from chat/. Maybe we could instead call this file "chatNotifications.jsm"?

Comment 32

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v3.5

Made the suggested changes. Though, I am not sure about the DOM parser code.

Comment 33

[Florian Quèze [:florian]]

Comment on attachment 8394317 [details] [diff] [review]
Patch v3.5

Review of attachment 8394317 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/components/im/modules/chatNotifications.jsm
@@ +21,5 @@
> +
> +  _showMessageNotification: function (aMessage) {
> +    let parser = Components.classes["@mozilla.org/xmlextras/domparser;1"]
> +                           .createInstance(Components.interfaces.nsIDOMParser);
> +    let doc = parser.parseFromString(aMessage.message.replace(/<br>/gi, "<br/>"), "text/html");

I don't think we need this |.replace(/<br>/gi, "<br/>")| here. Or if we do, I don't know why.

@@ +22,5 @@
> +  _showMessageNotification: function (aMessage) {
> +    let parser = Components.classes["@mozilla.org/xmlextras/domparser;1"]
> +                           .createInstance(Components.interfaces.nsIDOMParser);
> +    let doc = parser.parseFromString(aMessage.message.replace(/<br>/gi, "<br/>"), "text/html");
> +    let br = doc.querySelector("br");

There's no guarantee that the document contains any <br> node. I think you want to look for the body node.

Comment 34

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v3.7

Okay, made the suggested changes and the patch works for me.

Thanks.

Comment 35

[Florian Quèze [:florian]]

Comment on attachment 8394773 [details] [diff] [review]
Patch v3.7

Review of attachment 8394773 [details] [diff] [review]:
-----------------------------------------------------------------

Starting to look good! :-)

::: chat/themes/jar.mn
@@ +11,5 @@
>  	skin/classic/chat/idle-16.png
>  	skin/classic/chat/idle.png
>  	skin/classic/chat/mobile-16.png
>  	skin/classic/chat/mobile.png
> +	skin/classic/chat/newMessage.png

If this icon is intended to only be used by Thunderbird, it should go in mail/themes, not in chat/

::: mail/components/im/modules/chatNotifications.jsm
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +const EXPORTED_SYMBOLS = ["Notifications"];
> +

const {classes: Cc, interfaces: Ci, utils: Cu} = Components;

and then you can shorten things on several lines of the file :-)

@@ +51,5 @@
> +    if (messageText.startsWith("/me "))
> +      messageText = messageText.replace(/^\/me/, name);
> +
> +    // If the TB window doesn't have the focus, show the notification!
> +    if (!Services.wm.getMostRecentWindow("mail:3pane").document.hasFocus())

Use {} when the instruction after an if is wrapped over multiple lines.

@@ +68,5 @@
> +    if (aTopic == "new-directed-incoming-message") {
> +      if (Services.prefs.getBoolPref(this._notificationPrefName))
> +        this._showMessageNotification(aSubject);
> +    } else if (aTopic == "alertclickcallback") {
> +      // Prepare an observer to focus the conversation if the

Remove the "Prepare an observer to" part here, the sentence will make more sense without it.

::: mail/locales/en-US/chrome/messenger/preferences/chat.dtd
@@ +22,5 @@
>  
>  <!ENTITY  sendTyping.label              "Send typing notifications in conversations">
>  <!ENTITY  sendTyping.accesskey          "t">
> +
> +<!ENTITY  desktopChatNotifications.label "Show desktop notifications for incoming chat messages">

Isn't this wording misleading? I wonder if people would expect notifications for all incoming messages (including in IRC channels).

@@ +23,5 @@
>  <!ENTITY  sendTyping.label              "Send typing notifications in conversations">
>  <!ENTITY  sendTyping.accesskey          "t">
> +
> +<!ENTITY  desktopChatNotifications.label "Show desktop notifications for incoming chat messages">
> +<!ENTITY  desktopChatNotifications.accesskey "C">

I think the case of the accesskey usually matches the case of the letter in the string; I don't see any capital C in the desktopChatNotifications.label string.

Comment 36

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v3.9

Made the suggested changes.
(In reply to Florian Quèze [:florian] [:flo] from comment #35)
> > +<!ENTITY  desktopChatNotifications.label "Show desktop notifications for incoming chat messages">
> 
> Isn't this wording misleading? I wonder if people would expect notifications
> for all incoming messages (including in IRC channels).

By incoming messages from IRC channels, I assume directed messages. If yes, then yes we show the notification. If this misleads to all the IRC messages, then we probably need to re-phrase this. So, please let me what the case is.

Thanks.

Comment 37

[Florian Quèze [:florian]]

Comment on attachment 8401562 [details] [diff] [review]
Patch v3.9

The code looks good to me. I haven't tested this myself, but I assume you have before attaching the patch :-).

I think the UI reviewers can decide if the string for the preference window is correct or needs to be reworded.

Comment 38

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v4

Added custom sound option to the chat preferences.

Comment 39

[Patrick Cloke [:clokep]]

Comment on attachment 8401930 [details] [diff] [review]
Patch v4

Review of attachment 8401930 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/app/profile/all-thunderbird.js
@@ +833,5 @@
>  pref("mail.chat.play_notification_sound", true);
> +pref("mail.chat.show_desktop_notifications", true);
> +pref("mail.chat.biff.play_sound", true);
> +pref("mail.chat.biff.play_sound.type", 0);
> +pref("mail.chat.biff.play_sound.url", "");

Please add comments around these values like https://mxr.mozilla.org/comm-central/source/mailnews/mailnews.js#623

Comment 40

[Richard Marti (:Paenglab)]

I can change the notification sound, but used is always the mail sound no matter if mail sound is on default or custom. I checked "mail.chat.biff.play_sound.type" and it changes from 0 to 1.

Comment 41

[Suyash Agarwal (:sshagarwal)]

Comment on attachment 8401930 [details] [diff] [review]
Patch v4

Okay so this functionality can be implemented in a followup and we can push the functionality implemented so far. So, marking this patch as obsolete.

Comment 42

[Richard Marti (:Paenglab)]

Comment on attachment 8401562 [details] [diff] [review]
Patch v3.9

Review of attachment 8401562 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. ui-r+ with using "Show notifications for messages directed at you" in the preferences.

Comment 43

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v4

Done.

Thanks.

Comment 44

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/fce1dc971d43