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

RESOLVED FIXED in Thunderbird 17.0

Status

Thunderbird
Instant Messaging
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: florian, Assigned: florian)

Tracking

Trunk
Thunderbird 17.0

Thunderbird Tracking Flags

(thunderbird15 fixed, thunderbird16 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

5 years ago
Created attachment 643376 [details] [diff] [review]
Patch

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?
(Assignee)

Comment 2

5 years ago
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?
(Assignee)

Comment 4

5 years ago
(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+
(Assignee)

Comment 6

5 years ago
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+
(Assignee)

Comment 8

5 years ago
Created attachment 645039 [details] [diff] [review]
Same patch unbitrotted

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
Last Resolved: 5 years ago
status-thunderbird15: --- → fixed
status-thunderbird16: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 17.0
You need to log in before you can comment on or make changes to this bug.