Closed Bug 748358 Opened 12 years ago Closed 12 years ago

Change the appearance of the chat button instead of reopening the chat tab when receiving a message

Categories

(Thunderbird :: Instant Messaging, enhancement)

enhancement
Not set
normal

Tracking

(thunderbird15 fixed)

RESOLVED FIXED
Thunderbird 16.0
Tracking Status
thunderbird15 --- fixed

People

(Reporter: florian, Assigned: florian)

References

Details

Attachments

(5 files, 3 obsolete files)

Reopening the chat tab automatically is apparently too obtrusive for email users. It was suggested that we do something with the chat button instead.

I'm closing bug 740358 as I checked-in the fix that's attached there, so I'm opening this new bug to further discuss and implement this proposed additional improvement.

See comment 1 to comment 9 in bug 740358 for more details.
Attached patch WIP (obsolete) — Splinter Review
This patch contains the required code changes to stop reopening the chat tab automatically, and to set an attribute on the chat toolbar button when there are unread messages directed to the user (either private messages, or messages containing the user's nick).

For the chat button, the current patch sets the "checked" attribute to "true" when there are unread messages, as it's the only thing that I found that causes a difference of appearance without any theming changes. The attribute name should probably be changed ("highlight"? "unread"?) once Andreas has some CSS changes to go with this patch. (It can trivially be edited in the _updateChatButtonState method. It would also be trivial to have the number of unread messages in the attribute; if there's a way to display that inside the button and it's wanted).
Assignee: nobody → florian
Attachment #628432 - Flags: ui-review?(bwinton)
Attachment #628432 - Flags: feedback?(nisses.mail)
Comment on attachment 628432 [details] [diff] [review]
WIP

On Mac: 

- I think we want the icon to be highlighted, too.  Currently just the text is.

- How hard would it be to change the label of the button to "Chat (1)" when I had one incoming message?


On Linux:

- The button looks pressed in, which I'm really not a fan of.  I think we need to do something explicitly different here.


On Windows:

- My Windows builds are still broken, so no ui comments from me for this, I'm sorry to say.  :(  Perhaps Andreas will post a screenshot…


So, given the Linux behaviour, I think I'm going to say ui-r-, but I bet we could use a different attribute, and get Andreas to do some fancier styling…  ;)

Thanks,
Blake.
Attachment #628432 - Flags: ui-review?(bwinton) → ui-review-
(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #2)

> - How hard would it be to change the label of the button to "Chat (1)" when
> I had one incoming message?

Relatively trivial, but I'm afraid it would increase the width of the Chat button, and make all the other buttons after it move.

> So, given the Linux behaviour, I think I'm going to say ui-r-, but I bet we
> could use a different attribute, and get Andreas to do some fancier styling…
> ;)

That (getting Andreas to polish the appearance) is my plan! :)
What about doing font-weight bold on the linux label and adding a flare to the icon just like we do for mail folders with new messages in them?
(In reply to Andreas Nilsson (:andreasn) from comment #4)
> What about doing font-weight bold on the linux label and adding a flare to
> the icon just like we do for mail folders with new messages in them?

That idea is OK with me.
Attached patch WIP (v2) (obsolete) — Splinter Review
With new graphics for pinstripe, gnomestripe and qute (non-aero).
Attached patch WIP (v3) (obsolete) — Splinter Review
Sets #button-chat[unreadMessages="true"] in the themes so we can do stuff with the icons. Still need to work out the text styling on aero and pinstripe.
Attachment #631841 - Attachment is obsolete: true
note that this patch needs the patch in #727598 applied first
Attachment #631929 - Flags: ui-review?(bwinton)
Attachment #631929 - Flags: review?(florian)
Comment on attachment 631929 [details] [diff] [review]
patch for linux, windows (xp+aero) and mac

Andreas, I tested this on Mac only. I like the new icon, and your CSS changes seem OK to me, so the r+ is for these parts of the patch.

I found some issues with the JS code changes I proposed before in attachment 628432 [details] [diff] [review] and anyway, I wrote that part so I couldn't r+ it.

I'll attach an updated version of that part of the patch in a separate attachment and request a code review on it.
Attachment #631929 - Flags: review?(florian) → review+
Attachment #628432 - Attachment is obsolete: true
Attachment #628432 - Flags: feedback?(nisses.mail)
Attachment #632746 - Flags: review?(bwinton)
(In reply to Florian Quèze from comment #12)
> Created attachment 632746 [details] [diff] [review]
> Patch (code changes)

This patch removes the 2 more or less broken pieces of code that used to change automatically the selection in the left pane.
I mentioned already in bug 735292 comment 7 that I wasn't fully satisfied with it. The issues became more obvious here as clicking the chat button displayed the chat tab but without selecting the conversation with unread messages; so the unread indicator on the chat button wasn't removed.

For this new patch I wrote a better algorithm to select something that makes when the Chat tab becomes visible. I added lots of comments in the code, so I'm not going to details what it does here.
Attachment #632746 - Flags: ui-review?(bwinton)
Comment on attachment 631929 [details] [diff] [review]
patch for linux, windows (xp+aero) and mac

On Mac:  I like it, but it seemed to remain highlit even after I viewed the email.  How am I supposed to clear out the highlight?

On Windows Aero: The highlit version of the Chat icon seems too dark, particularly in the default theme, since everything is slightly blue.

On Linux: The bold seems a little subtle, but I'm not really sure what else we could do there that would still fit in.  I also like the tiny yellow star, but I think that giving it a bit of a darker edge might help it be more recognizable.  (Since it's on a background of white and light grey…)

Other than those tweaks, I like it, so I guess ui-r=me.
Attachment #631929 - Flags: ui-review?(bwinton) → ui-review+
Comment on attachment 632746 [details] [diff] [review]
Patch (code changes)

(Carrying forward the ui-review from the previous patch, since this is the patch I actually ran with…)

>+++ b/mail/components/im/content/chat-messenger-overlay.js
>@@ -228,36 +223,66 @@ var chatHandler = {
>+  _updateChatButtonState: function() {
>+    delete this._chatButtonUpdatePending;
>+    let chatButton = document.getElementById("button-chat");
>+    if (!chatButton)
>+      return;
>+    if (this.countUnreadMessages())
>+      chatButton.setAttribute("unreadMessages", "true");
>+    else
>+      chatButton.removeAttribute("unreadMessages");

What do you think about doing:
  chatButton.setAttribute("unreadMessages", this.countUnreadMessages());
instead, and getting rid of the "if"?

Other than that (and the things I asked for in the previous patch), I'm pretty happy with it.  r=me!

Thanks,
Blake.
Attachment #632746 - Flags: ui-review?(bwinton)
Attachment #632746 - Flags: ui-review+
Attachment #632746 - Flags: review?(bwinton)
Attachment #632746 - Flags: review+
(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #14)
> Comment on attachment 631929 [details] [diff] [review]
> patch for linux, windows (xp+aero) and mac
> 
> On Mac:  I like it, but it seemed to remain highlit even after I viewed the
> email.  How am I supposed to clear out the highlight?

The email?
The highlight of the chat button goes away as soon as the conversation with unread messages is selected.
Could it be that during your testing you had more than one conversation with unread messages, and missed one? (could easily happen if you had nickserv or chanserv telling you some nonsense for example)

Otherwise, I would need steps to reproduce.

(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #15)

> >+    if (this.countUnreadMessages())
> >+      chatButton.setAttribute("unreadMessages", "true");
> >+    else
> >+      chatButton.removeAttribute("unreadMessages");
> 
> What do you think about doing:
>   chatButton.setAttribute("unreadMessages", this.countUnreadMessages());
> instead, and getting rid of the "if"?

That would force the CSS to use a selector for #chat-button:not([unreadMessages=0]) instead of just #chat-button[unreadMessages] I don't think it's a simplification.
(In reply to Florian Quèze from comment #16)
> (In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #14)
> > On Mac:  I like it, but it seemed to remain highlit even after I viewed the
> > email.  How am I supposed to clear out the highlight?
> The email?

The chat.  :P  Slow brain day, apparently.

> The highlight of the chat button goes away as soon as the conversation with
> unread messages is selected.
> Could it be that during your testing you had more than one conversation with
> unread messages, and missed one? (could easily happen if you had nickserv or
> chanserv telling you some nonsense for example)

I didn't think so.  There might have been twitter messages which remained unread…

> Otherwise, I would need steps to reproduce.

If I notice it happening again, I'll post STR.  Probably as a new bug.
(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #14)

> On Mac:  I like it, but it seemed to remain highlit even after I viewed the
> email.  How am I supposed to clear out the highlight?

I found the steps to reproduce. It happens when the conversation receiving new messages was already selected.
A workaround is to just click the conversation again in the left pane. I'll debug this.
This patch fixes the first issue mentioned in comment 14.
Addressing ui-review remarks.
Note that the above patch don't depend on any external patches.
Landed attachment 636672 [details] [diff] [review] + attachment 636342 [details] [diff] [review] as http://hg.mozilla.org/comm-central/rev/fb66eb0893fc
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 16.0
Attachment #636672 - Flags: approval-comm-aurora?
Attachment #636672 - Flags: approval-comm-aurora? → approval-comm-aurora+
Depends on: 780643
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: