Closed Bug 954682 Opened 10 years ago Closed 10 years ago

Can't accept/add buddies with JS-XMPP (regression for GTalk accounts)

Categories

(Chat Core :: XMPP, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: florian, Assigned: florian)

References

Details

(Whiteboard: [1.2-blocking])

Attachments

(2 files)

*** Original post on bio 1251 at 2012-01-30 11:33:00 UTC ***

- No add request pops up.
- The "Add buddy" dialog doesn't do anything for JS-XMPP accounts.

This doesn't matter for Facebook Chat as the server doesn't support it from XMPP, but it's a serious regression for Gtalk users.
Blocks: 954603
Whiteboard: [1.2-blocking]
*** Original post on bio 1251 as attmnt 1175 at 2012-02-16 18:31:00 UTC ***

This patch doesn't handle showing invitations, as the current code for that (in purplexpcom) is so wrong that I'll need to fully redesign/rewrite it before working on the XMPP part.
Attachment #8352922 - Flags: review?(clokep)
Assignee: nobody → florian
*** Original post on bio 1251 at 2012-02-16 19:48:58 UTC ***

Comment on attachment 8352922 [details] [diff] [review] (bio-attmnt 1175)
Patch, part 1 - add/remove buddies

>diff --git a/chat/protocols/xmpp/xmpp-session.jsm b/chat/protocols/xmpp/xmpp-session.jsm
>@@ -143,19 +143,20 @@ XMPPSession.prototype = {
>   execHandler: function(aId, aStanza) {
>     if (!this._handlers.hasOwnProperty(aId))
>-      return;
>-    this._handlers[aId](aStanza);
>+      return false;
>+    let handler = this._handlers[aId](aStanza);
>     this.removeHandler(aId);
>+    return true;
>   },
Why are we storing handler here?

>diff --git a/chat/protocols/xmpp/xmpp.jsm b/chat/protocols/xmpp/xmpp.jsm
>@@ -354,16 +354,28 @@ const XMPPAccountBuddyPrototype = {
> 
>     return new nsSimpleEnumerator(tooltipInfo);
>   },
> 
>   get normalizedName() this.userName,
>   /* Display name of the buddy */
>   get contactDisplayName() this.buddy.contact.displayName || this.displayName,
> 
>+  remove: function() {
>+    if (!this._account.connected)
>+      return;
>+
>+    let s = Stanza.iq("set", null, null,
>+                      Stanza.node("query", Stanza.NS.roster, null,
>+                                  Stanza.node("item", null,
>+                                              {jid: this.normalizedName,
>+                                               subscription: "remove"})));
>+    this._account._connection.sendStanza(s);
>+  },
In addBuddy you throw if disconnected, here you just return early. That seems a bit strange to me. Does the UI let us remove a buddy if the account is offline? (Of course we should still check, but I'm curious.)

>+  addBuddy: function(aTag, aName) {
[...]
>+    if (this._buddies.hasOwnProperty(jid)) {
>+      DEBUG("not re-adding an existing buddy, should check if the subscription is OK though... :(\n");
>+      return;
>+    }
Should this have an TODO comment?

The rest looks OK at first glance, but I'll look over it again.
*** Original post on bio 1251 at 2012-02-16 20:06:33 UTC ***

(In reply to comment #2)
> Comment on attachment 8352922 [details] [diff] [review] (bio-attmnt 1175) [details]
> Patch, part 1 - add/remove buddies
> 
> >diff --git a/chat/protocols/xmpp/xmpp-session.jsm b/chat/protocols/xmpp/xmpp-session.jsm
> >@@ -143,19 +143,20 @@ XMPPSession.prototype = {
> >   execHandler: function(aId, aStanza) {
> >     if (!this._handlers.hasOwnProperty(aId))
> >-      return;
> >-    this._handlers[aId](aStanza);
> >+      return false;
> >+    let handler = this._handlers[aId](aStanza);
> >     this.removeHandler(aId);
> >+    return true;
> >   },
> Why are we storing handler here?

It's a left-over from something I wrote before. At some point I wanted:
let handler = ...
this.removeHandler(aId);
return handler(aStanza);

But then I decided I didn't want to go change all the handlers to return a boolean.

> >diff --git a/chat/protocols/xmpp/xmpp.jsm b/chat/protocols/xmpp/xmpp.jsm
> >@@ -354,16 +354,28 @@ const XMPPAccountBuddyPrototype = {
> > 
> >     return new nsSimpleEnumerator(tooltipInfo);
> >   },
> > 
> >   get normalizedName() this.userName,
> >   /* Display name of the buddy */
> >   get contactDisplayName() this.buddy.contact.displayName || this.displayName,
> > 
> >+  remove: function() {
> >+    if (!this._account.connected)
> >+      return;
> >+
> >+    let s = Stanza.iq("set", null, null,
> >+                      Stanza.node("query", Stanza.NS.roster, null,
> >+                                  Stanza.node("item", null,
> >+                                              {jid: this.normalizedName,
> >+                                               subscription: "remove"})));
> >+    this._account._connection.sendStanza(s);
> >+  },
> In addBuddy you throw if disconnected, here you just return early. That seems a
> bit strange to me. Does the UI let us remove a buddy if the account is offline?
> (Of course we should still check, but I'm curious.)

I think throwing in addBuddy will prevent the dialog from closing.
In the remove method, I think it's mostly a matter of how much noise we want to make if this isn't done correctly. The "Remove" context menu item isn't disabled for disconnected accounts; I'm not sure what the expected behavior is in that case.

> >+  addBuddy: function(aTag, aName) {
> [...]
> >+    if (this._buddies.hasOwnProperty(jid)) {
> >+      DEBUG("not re-adding an existing buddy, should check if the subscription is OK though... :(\n");
> >+      return;
> >+    }
> Should this have an TODO comment?

Maybe:
DEBUG("not re-adding an existing buddy");
// TODO check if the subscription is ok or if we should resend a presence request.
Comment on attachment 8352922 [details] [diff] [review]
Patch, part 1 - add/remove buddies

*** Original change on bio 1251 attmnt 1175 at 2012-02-16 21:10:14 UTC ***

r+ with the changes from comment 3.
Attachment #8352922 - Flags: review?(clokep) → review+
*** Original post on bio 1251 as attmnt 1178 at 2012-02-20 15:36:00 UTC ***

Second part; this diff applies above part 1 which should be applied first.
Attachment #8352927 - Flags: review?(clokep)
Comment on attachment 8352927 [details] [diff] [review]
Patch, part 2 - authorization requests

*** Original change on bio 1251 attmnt 1178 at 2012-02-20 16:47:55 UTC ***

The JS parts of this all look fine. The only comments I have is whether the changes to blist.js should be else if instead of just if statements.

I didn't look over the libpurple code in depth, but it looks OK on the surface.
Attachment #8352927 - Flags: review?(clokep) → review+
*** Original post on bio 1251 at 2012-02-20 16:56:30 UTC ***

(In reply to comment #6)

> The only comments I have is whether the
> changes to blist.js should be else if instead of just if statements.

All this method looks like:

if (aSubject == "some-value") {
  // Do something...
  return;
}

The only instance that doesn't look like this is:
else if (aTopic == "showing-ui-conversation")
(visible in the context of the patch you reviewed).

To improve consistency we could either remove this |else| (as the code block above it finishes with a |return|) or change the whole method to this style:
if (aSubject == "some-value) {
  // Do something...
else if (aSubject == "some-other-value) {
  // Do something else...
}
else if (aSubject == ...
*** Original post on bio 1251 at 2012-02-20 17:05:22 UTC ***

I think it'd be better if the styled matched, but I won't block this landing based on that. (Removing the else is probably sufficient.)
Status: NEW → ASSIGNED
*** Original post on bio 1251 at 2012-02-21 10:21:57 UTC ***

Fixed:
Part 1: https://hg.instantbird.org/instantbird/rev/bce36aedaa74
Part 2: https://hg.instantbird.org/instantbird/rev/aeeb3eb04a8e
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
OS: Other → All
Hardware: x86 → All
Resolution: --- → FIXED
Target Milestone: --- → 1.2
You need to log in before you can comment on or make changes to this bug.