776609 - Instant Messaging button and menuitem should integrate with new IM component.

Status: RESOLVED FIXED

(Thunderbird :: Address Book, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

There's an Instant Messaging button and menuitem available in our address book. That button / menuitem should integrate with our new IM component.

From Florian:

"If we know that the contact is available, we should do the same thing as what happens when the user clicks the presence icon in an email header. If we don't know/the contact isn't available, we should just fallback to the old behavior (ie starting the default AIM client of the system if there's an AIM screenname in the address book card)"

Comment 1

[Mike Conley (:mconley) (:⚙️)]

So, I've started hacking on this, and I've noticed that the IM command also works when we have multiple contacts selected.

3 cases, with my question being in the 3rd:

Case 1:

User has selected all contacts that are online via the IM component. In this case, conversations are opened up with each contact, and the chat tab is opened to focus on the conversation with the last contact in the selection.

Case 2:

User has selected no contacts that are online via the IM component. In this case, we revert to the old behaviour, and attempt to communicate with all of those users via an AIM client.

Case 3:

User has selected some contacts that are online via the IM component. In this case...do we try to communicate with the available contacts via IM, and *also* open up the AIM client?

Any ideas, preferences, suggestions on that?

Comment 2

[Florian Quèze [:florian]]

My suggestion would be to disable the IM command when more than one contact is selected.

Rationale: I think the default installed AIM client is unlikely to handle a goim command with multiple contacts correctly, and our chat code could only start several private conversations.
I think the behavior the user would expect is to start a group chat and invite all the selected contacts to that group chat. I don't think AIM handles that; our chat code doesn't; and even if we tried, it's very very difficult to get it right if all the contacts aren't on the same network (although currently I think we only use presence info from XMPP servers that are likely - but not guaranteed - to be able to talk to each other).

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Blake:

How do you feel about that?

-Mike

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Attachment: WIP Patch 1

Currently busted, but checkpointing.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1

So here's how this functions:

1) The Instant Message button / menuitem is only enabled iff there is one contact selected.

2) When the IM button / menuitem is selected, if the contact has online presence for either its GTalk or Jabber properties, we open/focus the chat tab with that discussion (or, if no 3pane exists, open a new 3pane and then open a chat tab with that conversation).

3) When the IM button / menuitem is selected, if the contact has *no* presence with either GTalk or Jabber, we default to trying to open AIM using the contact's AIM username property. If the user does not have an AIM username, we still try to open AIM.

Does this sound right?

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 645796 [details] [diff] [review]
Patch v1

Withdrawing ui-r, since I want to make a few changes.

In particular, I want to make it so that if the user has a contact selected with no GTalk, Jabber, or AIM fields available, the command is disabled.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v2

Ok, this one disables the command if the contact does not have either a GTalk, AIM, or Jabber user id.

Comment 8

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 645857 [details] [diff] [review]
Patch v2

ui-r- based on the opinion that opening AIM when there are no AIM links in the contact isn't really useful.  Also, it looks like we can talk to offline GTalk Buddies, so perhaps we should do that, too.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v3

Ok, with this patch, we only enable the Instant Messaging command iff the user has an AIM account OR (the user has a GTalk / Jabber account AND is in our list of IM contacts).

The user does not need to be online in order to open a conversation with them.

If the user only has an AIM account, we try to open the AIM client - otherwise, we prefer trying the GTalk / Jabber accounts first.

Comment 10

[Florian Quèze [:florian]]

Comment on attachment 646243 [details] [diff] [review]
Patch v3

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

::: mail/components/addrbook/content/addressbook.js
@@ +36,5 @@
> +// These chat properties are the ones that our IM component supports. If a
> +// contact has a value for one of these properties, we can communicate with
> +// that contact (assuming that the user has added that value to their list
> +// of IM contacts).
> +const kChatProperties = ["_GoogleTalk", "_JabberId"];

Should we also use the email address, assuming that if we find it in the chat contacts, it's because the email address is also the jabber id of the contact?
The code showing presence info in email headers does that, to make it more likely that we display presence information even when the user hasn't filled the address book cards correctly/completely/at all.
In the context of the address book I'm not sure if we should also treat the email address as a Jabber ID by default, but I thought I would mention it so that wether or not we do it is a conscious decision rather than something we have forgotten ;).

@@ +636,2 @@
>  
> +  if (cards.length < 1) {

Any reason why you are checking for < 1 rather than != 1 here?

Can we ever be in this situation anyway? (Wouldn't the command be disabled if we don't have only one card selected?)

@@ +651,5 @@
> +      let prplConv = chatContact.createConversation();
> +      let uiConv = Services.conversations.getUIConversation(prplConv);
> +
> +      let win = window;
> +      if (!("focusConversation" in chatHandler)) {

Any reason to expect that "focusConversation" could exist in the addressbook window? Is addressbook.js ever included in a 3pane window?

::: mail/components/im/content/chat-messenger-overlay.js
@@ +919,5 @@
>  
>      imServices.tags.getTags().forEach(function (aTag) {
>        aTag.getContacts().forEach(function(aContact) {
> +        let name = aContact.preferredBuddy.normalizedName;
> +        chatHandler.allContacts[name] = aContact;

Is it possible to open the address book before any 3pane window gets loaded?
If it happens the "IM" command of the address book will be disabled for all non-AIM IM contacts.

I'm not sure if we care. If we do, the solution is to build allContacts from the init method of chatHandler.jsm, and to just loop over chatHandler.allContacts here instead of looping over the contacts of each tag.

::: mail/components/im/modules/chatHandler.jsm
@@ +18,5 @@
>      this._initializing = true;
>  
>      Components.utils.import("resource:///modules/index_im.js");
> +    Components.utils.import("resource://gre/modules/Services.jsm");
> +    Components.utils.import("resource:///modules/imServices.jsm");

resource:///modules/imServices.jsm already imports and re-exports resource://gre/modules/Services.jsm, so you can remove the first of these 2 lines.

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Thanks for the review, Florian!

(In reply to Florian Quèze from comment #10)
> In the context of the address book I'm not sure if we should also treat the
> email address as a Jabber ID by default, but I thought I would mention it so
> that wether or not we do it is a conscious decision rather than something we
> have forgotten ;).

Hm, for this, I wonder where the line is between "magical" vs "confusing". As a user, I might get confused if I'm able to chat with a contact that I haven't set IM properties to... it might seem convenient, but it also might bewilder me.

What do you think, Blake?

> 
> @@ +636,2 @@
> >  
> > +  if (cards.length < 1) {
> 
> Any reason why you are checking for < 1 rather than != 1 here?

Whoops - wow, yeah, good catch. +1 for code review!

> 
> Can we ever be in this situation anyway? (Wouldn't the command be disabled
> if we don't have only one card selected?)

Fired from cmd_chatWithCard, yeah, we shouldn't be able to get into the case where the selected cards is not 1, but I'm just being defensive of our assertion here.

> 
> @@ +651,5 @@
> > +      let prplConv = chatContact.createConversation();
> > +      let uiConv = Services.conversations.getUIConversation(prplConv);
> > +
> > +      let win = window;
> > +      if (!("focusConversation" in chatHandler)) {
> 
> Any reason to expect that "focusConversation" could exist in the addressbook
> window? Is addressbook.js ever included in a 3pane window?

I think I'm basically cargo-culting here. I copied this code from the online-presence in message-header stuff.

No, addressbook.js should never be included in the 3pane window. I'll get rid of this.

> 
> ::: mail/components/im/content/chat-messenger-overlay.js
> @@ +919,5 @@
> >  
> >      imServices.tags.getTags().forEach(function (aTag) {
> >        aTag.getContacts().forEach(function(aContact) {
> > +        let name = aContact.preferredBuddy.normalizedName;
> > +        chatHandler.allContacts[name] = aContact;
> 
> Is it possible to open the address book before any 3pane window gets loaded?

Yes, if the user passes a -addressbook argument from the command line.

> If it happens the "IM" command of the address book will be disabled for all
> non-AIM IM contacts.
> 
> I'm not sure if we care. If we do, the solution is to build allContacts from
> the init method of chatHandler.jsm, and to just loop over
> chatHandler.allContacts here instead of looping over the contacts of each
> tag.
> 

Ok, I'll investigate that route.

> ::: mail/components/im/modules/chatHandler.jsm
> @@ +18,5 @@
> >      this._initializing = true;
> >  
> >      Components.utils.import("resource:///modules/index_im.js");
> > +    Components.utils.import("resource://gre/modules/Services.jsm");
> > +    Components.utils.import("resource:///modules/imServices.jsm");
> 
> resource:///modules/imServices.jsm already imports and re-exports
> resource://gre/modules/Services.jsm, so you can remove the first of these 2
> lines.

Good catch.

Comment 12

[Blake Winton (:bwinton) (:☕️)]

(In reply to Mike Conley (:mconley) from comment #11)
> (In reply to Florian Quèze from comment #10)
> > In the context of the address book I'm not sure if we should also treat the
> > email address as a Jabber ID by default, but I thought I would mention it so
> > that wether or not we do it is a conscious decision rather than something we
> > have forgotten ;).
> Hm, for this, I wonder where the line is between "magical" vs "confusing".
> As a user, I might get confused if I'm able to chat with a contact that I
> haven't set IM properties to... it might seem convenient, but it also might
> bewilder me.
> 
> What do you think, Blake?

That feels too confusing to me.  I would expect chat to only work on peopel I've told it to chat with.

Later,
Blake.

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v4

Fixed review comments.

We now handle the case where the addressbook is opened on its own. Note that if you're testing that on a debug build, you're going to get an (unrelated) assertion failure + crash on shutdown. I've filed and posted a patch for this (bug 778791). Apply that first if you don't want the shutdown crashes.

Comment 14

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 647236 [details] [diff] [review]
Patch v4

Okay, I've played with this for a while, and it seems pretty good to me.  ui-r=me!

Thanks,
Blake.

Comment 15

[Florian Quèze [:florian]]

Comment on attachment 647236 [details] [diff] [review]
Patch v4

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

::: mail/components/addrbook/content/addressbook.js
@@ +651,5 @@
> +    let chatID = card.getProperty(chatProperty, "");
> +
> +    if (chatID && (chatID in chatHandler.allContacts)) {
> +      let chatContact = chatHandler.allContacts[chatID];
> +      let prplConv = chatContact.createConversation();

Sorry for not catching this during the previous review, but I think you really want to check that chatContact.canSendMessage is true before attempting to create a conversation. (canSendMessage will be false for contacts of offline accounts, and when the contact is offline for protocols that don't support sending messages to offline contacts).

Also, when we have several possible chat username, shouldn't we fallback to starting a conversation with an offline contact only after verifying that they are all offline?

::: mail/components/im/modules/chatHandler.jsm
@@ -6,2 @@
>  
> -Components.utils.import("resource:///modules/imServices.jsm");

Why are you moving this from here...

@@ +17,5 @@
>        return;
>      this._initializing = true;
>  
>      Components.utils.import("resource:///modules/index_im.js");
> +    Components.utils.import("resource:///modules/imServices.jsm");

...to here?

Importing imServices just defines a bunch of lazy service getters, it's not expensive.

Importing index_im.js on the other hand starts the IM indexers.

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v5 - (carrying forward ui-r+ from bwinton)

This patch tries to open a conversation with the first online chat contact associated with an address book card. If there are no online chat contacts, it goes for the first offline but chat-able contact. Failing that, it tries AIM. Failing that, it gives up and throws.

Comment 17

[Florian Quèze [:florian]]

Comment on attachment 647542 [details] [diff] [review]
Patch v5 - (carrying forward ui-r+ from bwinton)

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

Looks ok, just one detail is confusing in comments. r=me with that addressed.

::: mail/components/addrbook/content/addressbook.js
@@ +649,2 @@
>  
> +  // We want to open a conversation with the first online account that we can

The word account here,

@@ +650,5 @@
> +  // We want to open a conversation with the first online account that we can
> +  // find. Failing that, we'll take the first offline (but still chat-able)
> +  // account we can find.
> +  //
> +  // First, sort the IM accounts into two groups - online contacts go into

and here is confusing, as you aren't referring to IM accounts.
Maybe use "username" instead?

Comment 18

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v6 (r+'d by florian, ui-r+'d by bwinton)

Thanks Florian! Updated commentary to refer to "usernames" as opposed to "accounts".

Comment 19

[Mike Conley (:mconley) (:⚙️)]

comm-central: https://hg.mozilla.org/comm-central/rev/6d71aefd4936
comm-aurora: https://hg.mozilla.org/releases/comm-aurora/rev/f1e2c093a5b9
comm-beta: https://hg.mozilla.org/releases/comm-beta/rev/cecb35e420ed