1024017 - Add ability to choose info shown in the desktop chat notifications

Status: RESOLVED FIXED

(Thunderbird :: Instant Messaging, defect)

Description

[Suyash Agarwal (:sshagarwal)]

This bug adds the pref to show/hide the chat message preview
in the desktop chat notifications.

Comment 1

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v1

As we don't want to complicate code, this is the minimal attempt in making the notification customizable.
Please let me know if we need to make it more customizable to allow removing
sender too. Though that'd be nice, it'll require significant changes. Not sure if that'll be acceptable.

Thanks.

Comment 2

[:aceman]

I vote for this feature. It is a privacy matter. For some users it is enough to know there is a new message but do not want the popup as it could show private text to other people looking at the display.

E.g. the Pidgin application just shows an icon in the systray if there was a new message in any chat. The icon goes away only when the user reads it (exposes pidgin window).

Comment 3

[Florian Quèze [:florian]]

(In reply to :aceman from comment #2)
> I vote for this feature. It is a privacy matter. For some users it is enough
> to know there is a new message but do not want the popup as it could show
> private text to other people looking at the display.

In a popup notification, just displaying the name of the contact talking to you can already be a privacy issue.

> E.g. the Pidgin application just shows an icon in the systray if there was a
> new message in any chat. The icon goes away only when the user reads it
> (exposes pidgin window).

Maybe you want to investigate something that shows only the unread message count? It may be slightly harder on Thunderbird because there's already the unread email count displayed (on the Mac dock at least).

Comment 4

[Suyash Agarwal (:sshagarwal)]

(In reply to Florian Quèze [:florian] [:flo] from comment #3)
> In a popup notification, just displaying the name of the contact talking to
> you can already be a privacy issue.
Indeed. We should hide that too. But as I have addressed this issue in my earlier comment #1,
that'll add complexities to the code. If all agree that this is worth the complexity, we can
go for that.

Thanks.

Comment 5

[Richard Marti (:Paenglab)]

(In reply to Florian Quèze [:florian] [:flo] from comment #3)
> (In reply to :aceman from comment #2)
> > I vote for this feature. It is a privacy matter. For some users it is enough
> > to know there is a new message but do not want the popup as it could show
> > private text to other people looking at the display.
> 
> In a popup notification, just displaying the name of the contact talking to
> you can already be a privacy issue.

Then it would be now the best to only use the sound for notification.

Comment 6

[Suyash Agarwal (:sshagarwal)]

(In reply to Richard Marti (:Paenglab) from comment #5)
> Then it would be now the best to only use the sound for notification.
I don't think we can just do with sound. There are scenarios when one can't have
sound enabled. Rest is what you decide.

Comment 7

[Richard Marti (:Paenglab)]

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

Looks good.

Comment 8

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v2

Okay, so I have made an attempt to cover both the message and buddy name.
Thanks to florian for menuitem idea.

Thanks.

Comment 9

[Florian Quèze [:florian]]

Comment on attachment 8439302 [details] [diff] [review]
Patch v2

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

::: mail/app/profile/all-thunderbird.js
@@ +830,5 @@
>  pref("devtools.debugger.log", false);
>  
>  pref("mail.chat.enabled", true);
>  pref("mail.chat.show_desktop_notifications", true);
> +pref("mail.chat.notification_info", 0);

Please add here a comment listing what the possible values are for this pref, and what they do.

::: mail/components/im/modules/chatNotifications.jsm
@@ +56,5 @@
> +        let bundle = new StringBundle("chrome://messenger/locale/chat.properties");
> +        if (!icon)
> +          icon = "chrome://messenger/skin/userIcon.png";
> +        if (!messageText)
> +          messageText = bundle.getString("messagePreview");

The documentation of getString says "@deprecated use |get| instead".

@@ +58,5 @@
> +          icon = "chrome://messenger/skin/userIcon.png";
> +        if (!messageText)
> +          messageText = bundle.getString("messagePreview");
> +        if (!name)
> +          name = bundle.getString("dummyBuddyName");

I don't think displaying "Buddy" as the sender is useful. Would the notification have an acceptable appearance if we just used an empty string there?

::: mail/locales/en-US/chrome/messenger/chat.properties
@@ +91,5 @@
>  # the last 8-14 days.
>  log.currentWeek=This Week
>  log.previousWeek=Last Week
> +
> +messagePreview=New Message

You may want the notification to specify that it's a chat message, as 'message' in a Thunderbird context is quite ambiguous.

::: mail/locales/en-US/chrome/messenger/preferences/chat.dtd
@@ +25,5 @@
>  
>  <!ENTITY  chatNotifications.label             "When messages directed at you arrive:">
>  <!ENTITY  desktopChatNotifications.label      "Show a notification">
>  <!ENTITY  desktopChatNotifications.accesskey  "c">
> +<!ENTITY  completeNotification.label          "With Buddy name and message preview">

I don't think the word "Buddy" is correct here. How do you feel about 'sender'?

Comment 10

[Richard Marti (:Paenglab)]

Comment on attachment 8439302 [details] [diff] [review]
Patch v2

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

I'm fully with Florian, Buddy as notification title looks good. A empty title would be better. And yes, please exchange in prefs dialog Buddy with sender. Again echoing Florian, "New chat message" is better to show the message's type.

Aside of this, the menulist approach looks simple but has all needed options.

With the string changes it would be ui-r+

Comment 11

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v2.3

Thanks.

Comment 12

[Magnus Melin [:mkmelin]]

Comment on attachment 8439992 [details] [diff] [review]
Patch v2.3

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

::: mail/components/im/modules/chatNotifications.jsm
@@ +52,5 @@
> +        name = aMessage.alias || aMessage.who;
> +        if (messageText && messageText.startsWith("/me "))
> +          messageText = messageText.replace(/^\/me/, name);
> +      case 2:
> +        let bundle = new StringBundle("chrome://messenger/locale/chat.properties");

you could create the bundle only in the case you need it

::: mail/locales/en-US/chrome/messenger/preferences/chat.dtd
@@ +28,2 @@
>  <!ENTITY  desktopChatNotifications.accesskey  "c">
> +<!ENTITY  completeNotification.label          "With Sender's name and message preview">

I don't see any reason to capitalize Sender in these

@@ +28,4 @@
>  <!ENTITY  desktopChatNotifications.accesskey  "c">
> +<!ENTITY  completeNotification.label          "With Sender's name and message preview">
> +<!ENTITY  buddyInfoOnly.label                 "With Sender's name only">
> +<!ENTITY  dummyNotification.label             "Without Sender's name and message preview">

Could the last one be
"Without any info"

As it is it's hard to grasp ("is that without name but with preview...?")

Comment 13

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v3

Thanks.

Comment 14

[Mark Banner (:standard8)]

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