Closed Bug 775105 Opened 12 years ago Closed 12 years ago

Presence information in email headers isn't shown correctly when there are multiple email windows

Categories

(Thunderbird :: Instant Messaging, defect)

defect
Not set
normal

Tracking

(thunderbird15 fixed, thunderbird16 fixed)

RESOLVED FIXED
Thunderbird 17.0
Tracking Status
thunderbird15 --- fixed
thunderbird16 --- fixed

People

(Reporter: florian, Assigned: florian)

References

Details

Attachments

(2 files)

Attached patch PatchSplinter Review
The presence information in email headers added in bug 763522 doesn't work at all for standalone message windows, and shows as online only the contact that signed on after the 3pane window was opened, so it's unreliable for windows that haven't been opened at startup.

The patch also fixes another issue related to having multiple 3pane windows: the OAuth dialog to authenticate with twitter was shown once per 3pane window.
Attachment #643376 - Flags: review?(mconley)
So, I'm in the process of reviewing this patch, but I can't seem to get the presence info in mail headers with or without this patch, on Linux.

Is this feature working on Linux?
I don't know any reason why this wouldn't work on Linux.
You need to have a Gtalk account connected with at least one online contact, and look at an email coming from that contact (ie either the email address used to send the email is the gtalk address, or there's an address book card associated with the email address, and that address book card contains the gtalk address of the person).
Comment on attachment 643376 [details] [diff] [review]
Patch

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

Mind you, I have limited chat component experience, so I may have missed something...but this looks pretty reasonable.

Just two points, see below.

::: mail/base/content/msgHdrViewOverlay.js
@@ +1517,5 @@
>    let prplConv = emailAddressNode.chatContact.createConversation();
>    let uiConv = Services.conversations.getUIConversation(prplConv);
> +
> +  let win = window;
> +  if (!("focusConversation" in chatHandler)) {

Can we ever get into a state where this is called before UpdateEmailNodeDetails, such that chatHandler does not exist?

::: mail/components/im/content/chat-messenger-overlay.js
@@ +88,4 @@
>        return;
>  
> +    if (aArgs.convType == "focus") {
> +      chatHandler.focusConversation(aArgs.conv);

Should we be checking for aArgs.conv on lines 86-87 too?
(In reply to Mike Conley (:mconley) from comment #3)
> Comment on attachment 643376 [details] [diff] [review]

> ::: mail/base/content/msgHdrViewOverlay.js
> @@ +1517,5 @@
> >    let prplConv = emailAddressNode.chatContact.createConversation();
> >    let uiConv = Services.conversations.getUIConversation(prplConv);
> > +
> > +  let win = window;
> > +  if (!("focusConversation" in chatHandler)) {
> 
> Can we ever get into a state where this is called before
> UpdateEmailNodeDetails, such that chatHandler does not exist?

This code (onClickEmailPresence) is an event handler on a node that is shown only if UpdateEmailNodeDetails has set the chatStatus attribute, so no.
So I don't mind the code causing a JS error if chatHandler isn't defined; that would tell us that something went horribly wrong.

> ::: mail/components/im/content/chat-messenger-overlay.js
> @@ +88,4 @@
> >        return;
> >  
> > +    if (aArgs.convType == "focus") {
> > +      chatHandler.focusConversation(aArgs.conv);
> 
> Should we be checking for aArgs.conv on lines 86-87 too?

The only reason to check for it would be to throw an exception when it's missing or null. Isn't the current code going to cause a JS error anyway?
Comment on attachment 643376 [details] [diff] [review]
Patch

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

Good points, both. r=me.
Attachment #643376 - Flags: review?(mconley) → review+
Comment on attachment 643376 [details] [diff] [review]
Patch

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: 
Testing completed (on c-c, etc.): 
Risk to taking this patch (and alternatives if risky):
Attachment #643376 - Flags: approval-comm-beta?
Attachment #643376 - Flags: approval-comm-aurora?
Comment on attachment 643376 [details] [diff] [review]
Patch

Yeah, I really think we want to have this on aurora/beta.
Attachment #643376 - Flags: approval-comm-beta?
Attachment #643376 - Flags: approval-comm-beta+
Attachment #643376 - Flags: approval-comm-aurora?
Attachment #643376 - Flags: approval-comm-aurora+
Fix the bitrot from bug 767155.
comm-central: https://hg.mozilla.org/comm-central/rev/02f5462203a5
comm-aurora: https://hg.mozilla.org/releases/comm-aurora/rev/79ec171c040c
comm-beta: https://hg.mozilla.org/releases/comm-beta/rev/021bfff32743
Assignee: nobody → florian
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 17.0
You need to log in before you can comment on or make changes to this bug.