Closed Bug 1024017 Opened 6 years ago Closed 6 years ago

Add ability to choose info shown in the desktop chat notifications

Categories

(Thunderbird :: Instant Messaging, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 33.0

People

(Reporter: sshagarwal, Assigned: sshagarwal)

Details

(Keywords: privacy)

Attachments

(1 file, 3 obsolete files)

This bug adds the pref to show/hide the chat message preview
in the desktop chat notifications.
Attached patch Patch v1 (obsolete) — Splinter Review
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.
Attachment #8438601 - Flags: ui-review?(richard.marti)
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).
Keywords: privacy
Version: unspecified → Trunk
(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).
(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.
(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.
(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 on attachment 8438601 [details] [diff] [review]
Patch v1

Looks good.
Attachment #8438601 - Flags: ui-review?(richard.marti) → ui-review+
Attached patch Patch v2 (obsolete) — Splinter Review
Okay, so I have made an attempt to cover both the message and buddy name.
Thanks to florian for menuitem idea.

Thanks.
Attachment #8438601 - Attachment is obsolete: true
Attachment #8439302 - Flags: ui-review?(richard.marti)
Attachment #8439302 - Flags: review?(mkmelin+mozilla)
Attachment #8439302 - Flags: review?(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'?
Attachment #8439302 - Flags: review?(florian) → feedback+
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+
Attachment #8439302 - Flags: ui-review?(richard.marti)
Summary: Allow disabling chat message preview in desktop notifications → Add ability to choose info shown in the desktop chat notifications
Attached patch Patch v2.3 (obsolete) — Splinter Review
Thanks.
Attachment #8439302 - Attachment is obsolete: true
Attachment #8439302 - Flags: review?(mkmelin+mozilla)
Attachment #8439992 - Flags: ui-review+
Attachment #8439992 - Flags: review?(mkmelin+mozilla)
Attachment #8439992 - Flags: feedback+
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...?")
Attachment #8439992 - Flags: review?(mkmelin+mozilla) → review+
Attached patch Patch v3Splinter Review
Thanks.
Attachment #8439992 - Attachment is obsolete: true
Attachment #8443423 - Flags: ui-review+
Attachment #8443423 - Flags: review+
Attachment #8443423 - Flags: feedback+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/b9c94299932d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 33.0
You need to log in before you can comment on or make changes to this bug.