779136 - Make Chat button turn blue if any new unread messages exist

Status: RESOLVED FIXED

(Thunderbird :: Instant Messaging, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

The current behaviour of the Chat button in the 3pane is that it turns blue and shows a numeric badge when there are any "mentions" in any chat conversation.

What we'd like is for the icon to turn blue if there is any new activity in any conversation, and show the numeric badge if there are any mentions.

Comment 1

[Florian Quèze [:florian]]

I think you just need to change http://hg.mozilla.org/comm-central/annotate/b3a09c3b06c9/mail/components/im/content/chat-messenger-overlay.js#l283 to return 2 values instead of one, and use one value for whether the icon turn blue, and the other (existing) one for the badge count.

Comment 2

[Florian Quèze [:florian]]

This is somehow related to bug 745369 that requested a notification for new messages that aren't directed to the user.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Attachment: WIP Patch 1

Checkpointing while I transfer to Windows machine, but I think I more or less have this done.

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 647617 [details] [diff] [review]
WIP Patch 1

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

Is this the right idea?

Comment 5

[Florian Quèze [:florian]]

Comment on attachment 647617 [details] [diff] [review]
WIP Patch 1

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

Drive by...

::: mail/components/im/content/chat-messenger-overlay.js
@@ +275,5 @@
> +
> +    if (unreadTotalCount)
> +      chatButton.setAttribute('unreadMessages', 'true');
> +    else
> +      chatButton.removeAttribute('unreadMessages');

Any reason for using single quotes rather than double quotes like everywhere else?

@@ +294,3 @@
>      for each (let conv in convs) {
> +        unreadTargettedCount += conv.unreadTargetedMessageCount;
> +        unreadTotalCount += conv.unreadIncomingMessageCount;

Fix the indent here.

@@ +305,5 @@
>      let title =
>        document.getElementById("chatBundle").getString("chatTabTitle");
> +    let [unreadTargettedCount, unreadTotalCount] = this.countUnreadMessages();
> +    if (unreadTotalCount)
> +      title += " (" + unreadTotalCount + ")";

Is there a reason for displaying the total count instead of the targetted count in the tab title? Wouldn't it be confusing to have a different count displayed in the icon badge and in the tab title?

Comment 6

[Mike Conley (:mconley) (:⚙️)]

(In reply to Florian Quèze from comment #5)
> Comment on attachment 647617 [details] [diff] [review]
> WIP Patch 1
> 
> Review of attachment 647617 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Drive by...
> 
> ::: mail/components/im/content/chat-messenger-overlay.js
> @@ +275,5 @@
> > +
> > +    if (unreadTotalCount)
> > +      chatButton.setAttribute('unreadMessages', 'true');
> > +    else
> > +      chatButton.removeAttribute('unreadMessages');
> 
> Any reason for using single quotes rather than double quotes like everywhere
> else?

Hm - that's a habit I picked up when submitting patches to Gaia (https://wiki.mozilla.org/Gaia#Coding_Style), but having refreshed myself of the Mozilla JS style guide on literals (https://developer.mozilla.org/en/Developer_Guide/Coding_Style#Literals), I'll switch them back to double-quotes.

Good catch!

> 
> @@ +294,3 @@
> >      for each (let conv in convs) {
> > +        unreadTargettedCount += conv.unreadTargetedMessageCount;
> > +        unreadTotalCount += conv.unreadIncomingMessageCount;
> 
> Fix the indent here.
> 
> @@ +305,5 @@
> >      let title =
> >        document.getElementById("chatBundle").getString("chatTabTitle");
> > +    let [unreadTargettedCount, unreadTotalCount] = this.countUnreadMessages();
> > +    if (unreadTotalCount)
> > +      title += " (" + unreadTotalCount + ")";
> 
> Is there a reason for displaying the total count instead of the targetted
> count in the tab title? Wouldn't it be confusing to have a different count
> displayed in the icon badge and in the tab title?

Whoops - you're right, thanks!

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1

As Florian pointed out in IRC, I attached the same WIP patch before. Here's the actual Patch v1.

Comment 9

[Florian Quèze [:florian]]

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

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

::: mail/components/im/content/chat-messenger-overlay.js
@@ +282,5 @@
>        let unreadInt = Components.classes["@mozilla.org/supports-PRInt32;1"]
>                                  .createInstance(Ci.nsISupportsPRInt32);
> +      unreadInt.data = unreadTotalCount;
> +      Services.obs.notifyObservers(unreadInt, "unread-im-count-changed", unreadTotalCount);
> +      this._notifiedUnreadCount = unreadTotalCount;

I think you also wanted to use the unreadTargettedCount value for these 6 lines of code related to the unread-im-count-changed notification (the value sent by that notification is directly used by nsMessengerOSXIntegration.mm to badge the dock on Mac).

@@ +303,5 @@
>        return;
>  
>      let title =
>        document.getElementById("chatBundle").getString("chatTabTitle");
> +    let [unreadTargettedCount, unreadTotalCount] = this.countUnreadMessages();

The unreadTotalCount variable seems unused. I think when using a destructuring assignment you only need to name what you use, so this would also work:
let [unreadTargettedCount] = this.countUnreadMessages();

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v2

Comment 11

[Florian Quèze [:florian]]

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

Looks good (note: I haven't tested this). Thanks.

Comment 12

[Blake Winton (:bwinton) (:☕️)]

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

Yes, this.  ui-r=me.

Comment 13

[Mike Conley (:mconley) (:⚙️)]

comm-central: https://hg.mozilla.org/comm-central/rev/2af3a435cade

Comment 14

[Mike Conley (:mconley) (:⚙️)]

comm-aurora: https://hg.mozilla.org/releases/comm-aurora/rev/f19a2f8a86cb
comm-beta: https://hg.mozilla.org/releases/comm-beta/rev/36db44dc2aca