Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Make Chat button turn blue if any new unread messages exist

RESOLVED FIXED in Thunderbird 17.0

Status

Thunderbird
Instant Messaging
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mconley, Assigned: mconley)

Tracking

15 Branch
Thunderbird 17.0
x86
All

Thunderbird Tracking Flags

(thunderbird15+ fixed, thunderbird16+ fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

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.
Assignee: nobody → mconley
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.
This is somehow related to bug 745369 that requested a notification for new messages that aren't directed to the user.
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 on attachment 647617 [details] [diff] [review]
WIP Patch 1

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

Is this the right idea?
Attachment #647617 - Flags: ui-review?(bwinton)
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?
(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!
Created attachment 647952 [details] [diff] [review]
Patch v1
Attachment #647617 - Attachment is obsolete: true
Attachment #647617 - Flags: ui-review?(bwinton)
Attachment #647952 - Flags: ui-review?(bwinton)
Attachment #647952 - Flags: review?(florian)
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.
Attachment #647952 - Attachment is obsolete: true
Attachment #647952 - Flags: ui-review?(bwinton)
Attachment #647952 - Flags: review?(florian)
Attachment #647956 - Flags: ui-review?(bwinton)
Attachment #647956 - Flags: review?(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();
Attachment #647956 - Flags: review?(florian) → review-
Created attachment 647979 [details] [diff] [review]
Patch v2
Attachment #647956 - Attachment is obsolete: true
Attachment #647956 - Flags: ui-review?(bwinton)
Attachment #647979 - Flags: ui-review?(bwinton)
Attachment #647979 - Flags: review?(florian)
Comment on attachment 647979 [details] [diff] [review]
Patch v2

Looks good (note: I haven't tested this). Thanks.
Attachment #647979 - Flags: review?(florian) → review+
Comment on attachment 647979 [details] [diff] [review]
Patch v2

Yes, this.  ui-r=me.
Attachment #647979 - Flags: ui-review?(bwinton) → ui-review+
Attachment #647979 - Flags: approval-comm-beta?
Attachment #647979 - Flags: approval-comm-aurora?
comm-central: https://hg.mozilla.org/comm-central/rev/2af3a435cade
Status: NEW → RESOLVED
Last Resolved: 5 years ago
tracking-thunderbird15: --- → ?
tracking-thunderbird16: --- → ?
OS: Windows 7 → All
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 17.0
tracking-thunderbird15: ? → +
tracking-thunderbird16: ? → +
Attachment #647979 - Flags: approval-comm-beta?
Attachment #647979 - Flags: approval-comm-beta+
Attachment #647979 - Flags: approval-comm-aurora?
Attachment #647979 - Flags: approval-comm-aurora+
comm-aurora: https://hg.mozilla.org/releases/comm-aurora/rev/f19a2f8a86cb
comm-beta: https://hg.mozilla.org/releases/comm-beta/rev/36db44dc2aca
status-thunderbird15: --- → fixed
status-thunderbird16: --- → fixed
You need to log in before you can comment on or make changes to this bug.