Last Comment Bug 779136 - Make Chat button turn blue if any new unread messages exist
: Make Chat button turn blue if any new unread messages exist
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Instant Messaging (show other bugs)
: 15 Branch
: x86 All
: -- normal (vote)
: Thunderbird 17.0
Assigned To: Mike Conley (:mconley) - (needinfo me!)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-31 08:03 PDT by Mike Conley (:mconley) - (needinfo me!)
Modified: 2012-08-03 06:59 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
+
fixed
+
fixed


Attachments
WIP Patch 1 (4.16 KB, patch)
2012-07-31 11:41 PDT, Mike Conley (:mconley) - (needinfo me!)
no flags Details | Diff | Splinter Review
Patch v1 (4.16 KB, patch)
2012-08-01 07:52 PDT, Mike Conley (:mconley) - (needinfo me!)
no flags Details | Diff | Splinter Review
Patch v1 (4.17 KB, patch)
2012-08-01 08:05 PDT, Mike Conley (:mconley) - (needinfo me!)
florian: review-
Details | Diff | Splinter Review
Patch v2 (4.16 KB, patch)
2012-08-01 09:17 PDT, Mike Conley (:mconley) - (needinfo me!)
florian: review+
bwinton: ui‑review+
standard8: approval‑comm‑aurora+
standard8: approval‑comm‑beta+
Details | Diff | Splinter Review

Description Mike Conley (:mconley) - (needinfo me!) 2012-07-31 08:03:22 PDT
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] [:flo] 2012-07-31 08:11:48 PDT
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] [:flo] 2012-07-31 08:17:35 PDT
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) - (needinfo me!) 2012-07-31 11:41:06 PDT
Created attachment 647617 [details] [diff] [review]
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) - (needinfo me!) 2012-07-31 11:48:04 PDT
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] [:flo] 2012-08-01 04:46:03 PDT
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) - (needinfo me!) 2012-08-01 07:27:53 PDT
(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) - (needinfo me!) 2012-08-01 07:52:48 PDT
Created attachment 647952 [details] [diff] [review]
Patch v1
Comment 8 Mike Conley (:mconley) - (needinfo me!) 2012-08-01 08:05:43 PDT
Created attachment 647956 [details] [diff] [review]
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] [:flo] 2012-08-01 08:42:31 PDT
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) - (needinfo me!) 2012-08-01 09:17:01 PDT
Created attachment 647979 [details] [diff] [review]
Patch v2
Comment 11 Florian Quèze [:florian] [:flo] 2012-08-01 09:24:23 PDT
Comment on attachment 647979 [details] [diff] [review]
Patch v2

Looks good (note: I haven't tested this). Thanks.
Comment 12 Blake Winton (:bwinton) (:☕️) 2012-08-02 11:02:47 PDT
Comment on attachment 647979 [details] [diff] [review]
Patch v2

Yes, this.  ui-r=me.
Comment 13 Mike Conley (:mconley) - (needinfo me!) 2012-08-02 11:07:41 PDT
comm-central: https://hg.mozilla.org/comm-central/rev/2af3a435cade
Comment 14 Mike Conley (:mconley) - (needinfo me!) 2012-08-03 06:59:03 PDT
comm-aurora: https://hg.mozilla.org/releases/comm-aurora/rev/f19a2f8a86cb
comm-beta: https://hg.mozilla.org/releases/comm-beta/rev/36db44dc2aca

Note You need to log in before you can comment on or make changes to this bug.