954979 - Check for open conversations when adding a buddy

Status: RESOLVED FIXED

(Instantbird Graveyard :: Conversation, defect)

Description

[aleth [:aleth]]

*** Original post on bio 1547 at 2012-06-26 17:44:00 UTC ***

It is possible for there to be an open conversation with someone who is not yet a buddy (e.g. if they send a message). When we add that person as a buddy, the open conversation is not associated with the buddy, and therefore does not receive presence information etc. 

So, when adding a buddy, any matching open conversations should be associated with the buddy.

Comment 1

[aleth [:aleth]]

Attachment: Patch

*** Original post on bio 1547 as attmnt 1730 at 2012-07-03 23:16:00 UTC ***

This works outside of edge cases (I haven't found any yet, but it should be clear from the code where some might exist). 

The patch consists of a /chat part for addBuddy (reopen any existing conversation of the same name), and a UI part that puts the new conversation into the same tab as the old one.

Trying to add a buddy to an existing prplConversation instead would be very hacky, as you'd also have to change every object up the chain into the UI by hand. You'd also have to change the interface to allow overwriting .buddy on the conversation.

Comment 2

[Patrick Cloke [:clokep]]

Comment on attachment 8353489 [details] [diff] [review]
Patch

*** Original change on bio 1547 attmnt 1730 at 2012-07-17 13:19:44 UTC ***

>diff --git a/chrome/instantbird/content/instantbird/conversation.xml b/chrome/instantbird/content/instantbird/conversation.xml
>+     <method name="replaceConv">
What does this function actually do? Can we get a comment here?

>diff --git a/components/imContacts.js b/components/imContacts.js
>+    // Now check if there is an open conversation for this buddy already.
>+    // getConversationByNameAndAccount(aAccountBuddy.userName, account, false)
>+    // will fail if the case doesn't match, i.e.. when the buddy is
>+    // created with a case that doesnt match the server case.
>+    // accountBuddies have normalizedNames, but we can't call the function
>+    // producing these on the name of the existing conv, so use toLowerCase
>+    // as it should cover most cases.
I dislike what this comment is implying. Something somewhere must know how to open a conversation based on a buddy, shouldn't it be able to find one too? This might require more extensive core changes, but the UI shouldn't have to deal w/ normalizing names, that's the protocol's job.

>+    if (prplConversation && !prplConversation.buddy) {
>+      // It is not possible to simply set the buddy on the existing
>+      // prplConversation. First close existing prplConversation, or else
>+      // creating a new one will just return it again.
>+      Services.conversations.removeConversation(prplConversation);
>+      // This did not close the UI tab, so opening a new conversation should
>+      // fill the existing tab. (If the existing conversation was on hold,
>+      // then this will open a new tab, as the existing conversation had
>+      // no conversation binding.)
>+      aAccountBuddy.createConversation();
>+    }
This sounds like an awful hack, did we look at how easy it would be to add a buddy after the fact?

>diff --git a/modules/imWindows.jsm b/modules/imWindows.jsm
>   showConversation: function(aConv) {
[...]
>+    if (!aConv.isChat) {
>+      // Check if there already exists a conversation tab for the same buddy.
>+      // When this happens, the prplConversation in that tab has already been
>+      // closed by accountBuddyAdded. So we must match against the name
>+      // stored in the UI.
>+      let matches = this._conversations.filter(function(c)
>+        !c.conv.target && aConv.title.toLocaleLowerCase() ==
>+          c.getElt("conv-top-info").getAttribute("displayName").toLocaleLowerCase());
>+      if (matches.length) {
>+        matches[0].replaceConv(aConv);
>+        return;
>+      }
>+    }
What is this change? I'm having trouble following your comment.

Overall, I feel like this patch is hacky and we should be able to do a better job of this...although it might require changing interfaces, etc.

Comment 3

[aleth [:aleth]]

*** Original post on bio 1547 at 2012-07-17 15:37:07 UTC ***

(In reply to comment #2)
> >+     <method name="replaceConv">
> What does this function actually do? Can we get a comment here?

When the target conversation of the conversation binding no longer exists, point the binding at a new conversation.

> >diff --git a/components/imContacts.js b/components/imContacts.js
> >+    // Now check if there is an open conversation for this buddy already.
> >+    // getConversationByNameAndAccount(aAccountBuddy.userName, account, false)
> >+    // will fail if the case doesn't match, i.e.. when the buddy is
> >+    // created with a case that doesnt match the server case.
> >+    // accountBuddies have normalizedNames, but we can't call the function
> >+    // producing these on the name of the existing conv, so use toLowerCase
> >+    // as it should cover most cases.
> I dislike what this comment is implying. Something somewhere must know how to
> open a conversation based on a buddy, shouldn't it be able to find one too?
> This might require more extensive core changes, but the UI shouldn't have to
> deal w/ normalizing names, that's the protocol's job.

We could add a method getConversationByBuddy, but it wouldn't help, as there is no buddy on the conversation we are looking for ;)

There is no sure-fire way to do this I can see, and it's the main flaw of this patch. (There is, in principle, when it is triggered from a context menu on the tab we are looking for, but not when triggered from the main menu Add Buddy entry.) If I am missing something let me know!

Ideally there would be a method for each account that returns any open conversations matching a userName (that could then normalize the userName appropriately). I can't see us adding such a method for each protocol though?

> >+    if (prplConversation && !prplConversation.buddy) {
> >+      // It is not possible to simply set the buddy on the existing
> >+      // prplConversation. First close existing prplConversation, or else
> >+      // creating a new one will just return it again.
> >+      Services.conversations.removeConversation(prplConversation);
> >+      // This did not close the UI tab, so opening a new conversation should
> >+      // fill the existing tab. (If the existing conversation was on hold,
> >+      // then this will open a new tab, as the existing conversation had
> >+      // no conversation binding.)
> >+      aAccountBuddy.createConversation();
> >+    }
> This sounds like an awful hack, did we look at how easy it would be to add a
> buddy after the fact?

Yes. The problem is that the existing structure is not set up to allow this. The buddy property is read-only in the interfaces, but that's not the only issue, I'm not sure what the required modifications to change that in libpurple would be. Then you'd have to get the corresponding uiConv to observe the contact corresponding to the account buddy. And send off a notification to add the buddy to the conversation binding and update the UI. You would have to be _very_ careful as you'd have to close the existing log at the right moment and start a new one (because the naming might change), despite the fact that you have not closed any conversation.  

Is going to be cleaner in the end than closing the existing conversation and starting a new one?

> >   showConversation: function(aConv) {
> [...]
> >+    if (!aConv.isChat) {
> >+      // Check if there already exists a conversation tab for the same buddy.
> >+      // When this happens, the prplConversation in that tab has already been
> >+      // closed by accountBuddyAdded. So we must match against the name
> >+      // stored in the UI.
> >+      let matches = this._conversations.filter(function(c)
> >+        !c.conv.target && aConv.title.toLocaleLowerCase() ==
> >+          c.getElt("conv-top-info").getAttribute("displayName").toLocaleLowerCase());
> >+      if (matches.length) {
> >+        matches[0].replaceConv(aConv);
> >+        return;
> >+      }
> >+    }
> What is this change? I'm having trouble following your comment.

If there is an open tab without an active conversation in it that matches the conversation we are adding, then place the conversation in the existing tab rather than opening a new one.
 
> Overall, I feel like this patch is hacky and we should be able to do a better
> job of this...although it might require changing interfaces, etc.

It certainly is not ideal. Then again it's quite robust, it just may not always work in preventing a new tab from opening, which is the status quo. Suggestions welcome :D

Comment 4

[aleth [:aleth]]

Comment on attachment 8353489 [details] [diff] [review]
Patch

*** Original change on bio 1547 attmnt 1730 at 2012-08-08 22:19:56 UTC ***

has left the conversation"/"user has joined the conversation" pair on the other end. (These are really annoying notifications which other clients send when the user closes a conversation window, but does not go offline.)

Since this would happen exactly once per buddy, I'm wondering whether that is not an acceptable cost, if that is the only issue.

Anyway, I am not sure how/whether to proceed with this.

Comment 5

[aleth [:aleth]]

*** Original post on bio 1547 at 2013-10-18 09:36:43 UTC ***

Looking it this again, it seems like the most hacky part of this would actually be solved by bug 955553 (bio 2115). The remainder is not so different from switching protocols for a contact.

Comment 6

[aleth [:aleth]]

Attachment: 954979.patch

Ignore all the preceding comments.

Comment 7

[Patrick Cloke [:clokep]]

Comment on attachment 8399126 [details] [diff] [review]
954979.patch

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

Just a few nits and please request review from Florian too.

::: chat/components/public/imIConversationsService.idl
@@ +55,4 @@
>    void removeConversation(in prplIConversation aConversation);
>  
> +  // To be called by the Contacts service when an accountbuddy is added.
> +  void addAccountBuddyToConversation(in imIAccountBuddy aAccountBuddy);

Florian will need to look over this API change.

::: chat/components/src/imContacts.js
@@ +1392,5 @@
>  
>      // Fire the notifications.
>      buddy.observe(aAccountBuddy, "account-buddy-added");
> +
> +    // Check to see if a conversation for this buddy already exists.

This isn't really a check, i.e. this comment (to me) implies "If something is true, do something else."

::: chat/components/src/imConversations.js
@@ +39,5 @@
>      return null;
>    },
> +  observeContact: function() {
> +    let contact = this.contact;
> +    if (contact && !this._observedContact) {

It is not immediately obvious to me why this was added. I don't think it's necessary.

@@ +417,5 @@
> +    let uiConv = this.getUIConversation(prplConversation);
> +    let contactId = aAccountBuddy.buddy.contact.id;
> +    if (contactId in this._uiConvByContactId) {
> +      // Trouble! There is an existing uiConv for this contact.
> +      // We must avoid having two uiConvs with the same contact.

Is there some way we "expect" this to happen or just being defensive? (Just curious!)

@@ +419,5 @@
> +    if (contactId in this._uiConvByContactId) {
> +      // Trouble! There is an existing uiConv for this contact.
> +      // We must avoid having two uiConvs with the same contact.
> +      // This is ugly UX, but should almost never happen :(
> +      this.removeConversation(prplConversation);

Nit: Return here and un-indent below.

@@ +484,5 @@
>    },
>    getConversationByNameAndAccount: function(aName, aAccount, aIsChat) {
> +    let normalizedName = aAccount.normalize(aName);
> +    for (let conv of this._prplConversations) {
> +      if (aAccount.normalize(conv.name) == normalizedName &&

I believe this is actually a bug fix that should have been included as part of bug 955553.

::: chat/protocols/irc/irc.js
@@ +607,5 @@
>    this.requestBuddyInfo(aName);
>  }
>  ircConversation.prototype = {
>    __proto__: GenericConvIMPrototype,
> +  get buddy() this._account.getBuddy(this.name),

We need to figure out if this is necessary for Yahoo/Twitter/XMPP in JS. aleth said libpurple does this "correctly" already.

Comment 8

[aleth [:aleth]]

Attachment: 954979.patch 2

(In reply to Patrick Cloke [:clokep] from comment #7)
> > +    let uiConv = this.getUIConversation(prplConversation);
> > +    let contactId = aAccountBuddy.buddy.contact.id;
> > +    if (contactId in this._uiConvByContactId) {
> > +      // Trouble! There is an existing uiConv for this contact.
> > +      // We must avoid having two uiConvs with the same contact.
> 
> Is there some way we "expect" this to happen or just being defensive? (Just
> curious!)

It's remotely possible if you have a conversation open with a contact to which the new accountbuddy is associated, and a second conversation with the account of the accountbuddy.

> >    getConversationByNameAndAccount: function(aName, aAccount, aIsChat) {
> > +    let normalizedName = aAccount.normalize(aName);
> > +    for (let conv of this._prplConversations) {
> > +      if (aAccount.normalize(conv.name) == normalizedName &&
> 
> I believe this is actually a bug fix that should have been included as part
> of bug 955553.

Yes (or in one of the other normalize bugs). It's unrelated to this bug really.

Comment 9

[Patrick Cloke [:clokep]]

Comment on attachment 8399132 [details] [diff] [review]
954979.patch 2

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

From a code perspective I think this is reasonable, let's see what Florian thinks of the API changes.

Comment 10

[Florian Quèze [:florian]]

Comment on attachment 8399132 [details] [diff] [review]
954979.patch 2

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

How is this code going to behave if the for a private IRC conversation the user adds the nick to the buddy list, then removes it, then adds it again?

::: chat/components/src/imContacts.js
@@ +1394,5 @@
>      buddy.observe(aAccountBuddy, "account-buddy-added");
> +
> +    // Let the conversation service know in case a conversation with the
> +    // new buddy already exists.
> +    Services.conversations.addAccountBuddyToConversation(aAccountBuddy);

Why isn't the conversation service just observing account-buddy-added through nsIObserverService?

::: chat/components/src/imConversations.js
@@ +23,5 @@
>    this._messages = [];
>    this.changeTargetTo(aPrplConversation);
>    let iface = Ci["prplIConv" + (aPrplConversation.isChat ? "Chat" : "IM")];
>    this._interfaces = this._interfaces.concat(iface);
> +  this.observeContact();

did you mean to create a setter here? Or maybe just call the method "initContactObserver".
"observe" here keeps making me think of the method that's actually part of the observer object.

@@ -26,5 @@
>    this._interfaces = this._interfaces.concat(iface);
> -  let contact = this.contact;
> -  if (contact) {
> -    // XPConnect will create a wrapper around 'this' here,
> -    // so the list of exposed interfaces shouldn't change anymore.

This comment really needs to stay in the constructor.

@@ +43,5 @@
> +    if (contact) {
> +      // XPConnect will create a wrapper around 'this' here,
> +      // so the list of exposed interfaces shouldn't change anymore.
> +      contact.addObserver(this);
> +      this._observedContact = contact;

What happens if _observedContact was already initialized?

@@ +418,5 @@
> +    let contactId = aAccountBuddy.buddy.contact.id;
> +    if (contactId in this._uiConvByContactId) {
> +      // Trouble! There is an existing uiConv for this contact.
> +      // We must avoid having two uiConvs with the same contact.
> +      // This is ugly UX, but should almost never happen :(

When does this happen?

@@ +425,5 @@
> +    }
> +    // Link the existing uiConv to the contact.
> +    this._uiConvByContactId[contactId] = uiConv;
> +    uiConv.observeContact();
> +    uiConv.notifyObservers(uiConv, "update-buddy-added");

For some reason, I'm confused by this notification name. How would update-conv-buddy feel?

Comment 11

[aleth [:aleth]]

Attachment: 954979.patch 3

>How is this code going to behave if the for a private IRC conversation the
>user adds the nick to the buddy list, then removes it, then adds it again?

Good catch, I had not added any code to handle accountbuddy removal at all. Most of your other comments follow from that.

>Why isn't the conversation service just observing account-buddy-added through
>nsIObserverService?

Because the accountbuddy code calls buddy.observe() directly and I hadn't noticed that the buddy rebroadcast what it was passed.

Comment 12

[aleth [:aleth]]

Attachment: 954979.patch 4

Comment 13

[Florian Quèze [:florian]]

Comment on attachment 8400588 [details] [diff] [review]
954979.patch 4

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

We are getting close, this looks much better than the previous iteration! :-)

::: chat/components/src/imConversations.js
@@ +397,5 @@
> +        // Trouble! There is an existing uiConv for this contact.
> +        // We should avoid having two uiConvs with the same contact.
> +        // This is ugly UX, but at least can only happen if there is
> +        // already an accountBuddy with the same name for the same
> +        // protocol on a different account, which should be rare.

To be sure I understand this correctly, the situation is:
- the user has accounts A1 and A2 on the same protocol.
- nick N is in the blist of A1, but not A2.
- there's an ongoing conversation between A1 and N (so this conversation has a contact associated with it).
- a conversation is opened between N and A2 (status unknown because N isn't in A2's blist).
- the user adds N to A2's blist.

@@ +398,5 @@
> +        // We should avoid having two uiConvs with the same contact.
> +        // This is ugly UX, but at least can only happen if there is
> +        // already an accountBuddy with the same name for the same
> +        // protocol on a different account, which should be rare.
> +        this.removeConversation(prplConversation);

What's the user-visible effect of this? Do we also need to add the prplConv to the other UIConv?

@@ +417,5 @@
> +      // If there is more than one target on the uiConv, close the
> +      // prplConv as we can't dissociate the uiConv from the contact.
> +      if (uiConv.hasMultipleTargets) {
> +        // Can't find the prplConv using the buddy property as the accountBuddy
> +        // has already been removed at this point.

I don't understand the meaning of this comment. Can't you iterate through the prplConv of the uiConv, and compare the account id?

@@ +426,5 @@
> +          this.removeConversation(prplConversation);
> +        return;
> +      }
> +
> +      delete this._uiConvByContactId[contactId];

This doesn't seem correct. Removing the account buddy doesn't necessarily removes the contact. I may be talking to the person on both GTalk and IRC, and removing the IRC nick from my blist while keeping the person in my gtalk roster.

Comment 14

[aleth [:aleth]]

Attachment: 954979.patch 5

>> +        // We should avoid having two uiConvs with the same contact.
>> +        // This is ugly UX, but at least can only happen if there is
>> +        // already an accountBuddy with the same name for the same
>> +        // protocol on a different account, which should be rare.
>> +        this.removeConversation(prplConversation);
>
>What's the user-visible effect of this? Do we also need to add the prplConv to the other UIConv?

Adding the prplConv to the other uiConv would be a bit of work, and it wouldn't help because there would be no way to switch between the conversations for the same protocol but different accounts.

The problem is what happens with the conversation tab with A2 when it receives ui-conv-closed. The tab is then in a broken state (this._conv undefined errors) because the tab isn't automatically closed when that notification is received. I'm not sure exactly what the bug is around this, but there does seem to be one...

>> +        // Can't find the prplConv using the buddy property as the accountBuddy
>> +        // has already been removed at this point.
>
>I don't understand the meaning of this comment. Can't you iterate through the prplConv of the uiConv, and compare the >account id?

I guess that comment was misleading, but I made the change you suggested anyway, it's better that way.


>> +      delete this._uiConvByContactId[contactId];
>
>This doesn't seem correct. Removing the account buddy doesn't necessarily removes the contact. I may be talking to >the person on both GTalk and IRC, and removing the IRC nick from my blist while keeping the person in my gtalk roster.

That's why there is an uiConv.hasMultipleTargets check. The point is that this only happens when no target of the uiconv belongs to the contact any more.

Comment 15

[Florian Quèze [:florian]]

(In reply to aleth from comment #14)
> Created attachment 8401923 [details] [diff] [review]
> 954979.patch 5
> 
> >> +        // We should avoid having two uiConvs with the same contact.
> >> +        // This is ugly UX, but at least can only happen if there is
> >> +        // already an accountBuddy with the same name for the same
> >> +        // protocol on a different account, which should be rare.
> >> +        this.removeConversation(prplConversation);
> >
> >What's the user-visible effect of this?
[...]
> The problem is what happens with the conversation tab with A2 when it
> receives ui-conv-closed. The tab is then in a broken state (this._conv
> undefined errors) because the tab isn't automatically closed when that
> notification is received. I'm not sure exactly what the bug is around this,
> but there does seem to be one...

It's the expected behavior that a tab can't be closed automatically.

Can you be more specific about "The tab is then in a broken state"? How broken is it? When do you get the "this._conv undefined errors" errors? Is it only when attempting to send a message, or do these errors happen without user action? Are they flooding the console, or happening just once?

How difficult would it be for us to display a system message that explains the situation?

Sorry for the delay here, I wrote similar questions last week but apparently my comment never got actually sent :-/.

Comment 16

[aleth [:aleth]]

(In reply to Florian Quèze [:florian] [:flo] from comment #15)
> It's the expected behavior that a tab can't be closed automatically.

Yes, that makes sense. Though I wondered in what other circumstances one would end up with a tab without a uiConv. Deleting the account of the conv in the tab maybe?
 
> Can you be more specific about "The tab is then in a broken state"? How
> broken is it? When do you get the "this._conv undefined errors" errors? Is
> it only when attempting to send a message, or do these errors happen without
> user action? Are they flooding the console, or happening just once?

There is no flooding of the console or anything (or I would not have requested review), these errors occur when interacting with the tab, e.g. by sending a message. By "broken" I meant that there is nothing telling the user what is going on.

> How difficult would it be for us to display a system message that explains
> the situation?

Not difficult at all for the case of this bug (write the system message before closing the conv).

Comment 17

[Florian Quèze [:florian]]

(In reply to aleth from comment #16)

> Not difficult at all for the case of this bug (write the system message
> before closing the conv).

Should we also do it for the other case you mentioned (deleted account) ?

Or should conversation.xml write the message if this._conv isn't initialized when the user attempts to send a message?

Comment 18

[aleth [:aleth]]

(In reply to Florian Quèze [:florian] [:flo] from comment #17)
> Should we also do it for the other case you mentioned (deleted account) ?

I'm not certain it happens in that case, I was just trying to come up with scenarios. There may be others.
 
> Or should conversation.xml write the message if this._conv isn't initialized
> when the user attempts to send a message?

This might be a better idea as it's more general. In which case I should do it in a separate bug.

Comment 19

[aleth [:aleth]]

(In reply to aleth from comment #18)
> This might be a better idea as it's more general. In which case I should do
> it in a separate bug.

Filed as bug 996545.

Comment 20

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/a0340443fdb6

Comment 21

[aleth [:aleth]]

Is it possible in Yahoo to have a conversation with someone who is not a contact? How does JS-Yahoo deal with a contact being removed while a conversation is open?

Comment 22

[Quentin Headen [:qheaden]]

(In reply to aleth from comment #21)
> Is it possible in Yahoo to have a conversation with someone who is not a
> contact? How does JS-Yahoo deal with a contact being removed while a
> conversation is open?

I will have to do some more testing to confirm everything, but I'm guessing that you can in fact have conversations with people not in your contact list. I know you can for sure do this with conferences by inviting people not in your contact list. Also, if you remove someone from your contacts list, but they don't remove you from their end, they can still talk with you. I guess you would have to block that person if necessary.

Protocol-wise, I think it is possible to talk with non-contacts, but I've never seen support for it from the chat clients.

Comment 23

[aleth [:aleth]]

(In reply to Quentin Headen from comment #22)
> (In reply to aleth from comment #21)
> > Is it possible in Yahoo to have a conversation with someone who is not a
> > contact? How does JS-Yahoo deal with a contact being removed while a
> > conversation is open?
> 
> I will have to do some more testing to confirm everything, but I'm guessing
> that you can in fact have conversations with people not in your contact
> list.

Thanks! In that case, please check that adding/removing a contact for someone with whom a Yahoo conversation is open works as expected, and file a new (Yahoo) bug if that is not be the case.