Closed Bug 954631 Opened 10 years ago Closed 10 years ago

Only notify prplIAccounts of status changes when enabled

Categories

(Chat Core :: General, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: clokep, Assigned: clokep)

Details

Attachments

(1 file, 5 obsolete files)

*** Original post on bio 1199 at 2011-12-09 03:08:00 UTC ***

*** Due to BzAPI limitations, the initial description is in comment 1 ***
Attached patch WIP 1 (obsolete) — Splinter Review
*** Original post on bio 1199 as attmnt 1067 at 2011-12-09 03:08:00 UTC ***

prplIAccounts should not receive status changes from their imIAccounts unless they've been enabled (either by auto-sign on or by connecting from the account manager). They should also stop receiving status changes once they're disconnected from the account manager.


This should avoid bug 954085 (bio 650) (for Twitter) and http://hg.instantbird.org/instantbird/rev/7cee3360eda5 (for XMPP).

The patch connected here works for Twitter, but does not work for XMPP (it seems that connect() never get's called on the xmpp.jsm object?)
*** Original post on bio 1199 at 2011-12-09 10:27:08 UTC ***

I don't understand what you are doing with this patch.

For example, why are you removing the this.connect call from xmpp.jsm but not from twitter.js when the status is back online?

It seems to me the .connect and .disconnect calls that result from the status changes should be done by imAccounts.js, not by each prpl. Wasn't this the point of this change?

(In reply to comment #0)
> (it seems that connect() never get's called on the xmpp.jsm object?)

As I said yesterday on IRC:
11:20:22 <flo> (in JS-XMPP it's difficult to set a variable in the connect method as that method changes for gtalk/facebook/whatever)
Attached patch Patch v1 (obsolete) — Splinter Review
*** Original post on bio 1199 as attmnt 1068 at 2011-12-09 11:40:00 UTC ***

(In reply to comment #1)
> I don't understand what you are doing with this patch.
I'm trying to only call the observe method of the prplIAccount when it should be notified of a status change, which is once it's been auto-logged in or connected via a user (but not disconnected via a user).

> For example, why are you removing the this.connect call from xmpp.jsm but not
> from twitter.js when the status is back online?
This was a misunderstanding of what the previous changeset was doing to reconnect XMPP. I've added it back.

> It seems to me the .connect and .disconnect calls that result from the status
> changes should be done by imAccounts.js, not by each prpl. Wasn't this the
> point of this change?
The point of this change was to have prplIAccounts have no knowledge of status changes when in a "disabled" state (disconnected by the user). Calling connect/disconnect from imIAccount should be as simple as calling them from that observe method I'm already modifying. This can be added if you'd like.
Attachment #8352810 - Flags: review?(florian)
Comment on attachment 8352809 [details] [diff] [review]
WIP 1

*** Original change on bio 1199 attmnt 1067 at 2011-12-09 11:40:11 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352809 - Attachment is obsolete: true
Assignee: nobody → clokep
Status: NEW → ASSIGNED
*** Original post on bio 1199 at 2011-12-09 11:45:28 UTC ***

I would prefer removing the connect and disconnect calls from the protocol specific code, yes :).
Attached patch Patch v2 (obsolete) — Splinter Review
*** Original post on bio 1199 as attmnt 1071 at 2011-12-10 15:15:00 UTC ***

This patch directly calls the connect and disconnect methods in imIAccount, so prplIAccounts no longer need to worry about this code. The observe methods is still called in this situation.
Attachment #8352813 - Flags: review?(florian)
Comment on attachment 8352810 [details] [diff] [review]
Patch v1

*** Original change on bio 1199 attmnt 1068 at 2011-12-10 15:15:24 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352810 - Attachment is obsolete: true
Attachment #8352810 - Flags: review?(florian)
Comment on attachment 8352813 [details] [diff] [review]
Patch v2

*** Original change on bio 1199 attmnt 1071 at 2011-12-11 23:33:51 UTC ***

The logic seems good, my comments are only about details.

>diff --git a/chat/components/src/imAccounts.js b/chat/components/src/imAccounts.js
>--- a/chat/components/src/imAccounts.js
>+++ b/chat/components/src/imAccounts.js
>@@ -181,16 +181,18 @@ imAccount.prototype = {
>   id: "",
>   numericId: 0,
>   protocol: null,
>   prplAccount: null,
>   connectionState: Ci.imIAccount.STATE_DISCONNECTED,
>   connectionStateMsg: "",
>   connectionErrorMessage: "",
>   _connectionErrorReason: Ci.prplIAccount.NO_ERROR,
>+  // Used to know if the prplIAccount should receive changes in status.
>+  _receiveStatusInformation: false,
>   get connectionErrorReason() {

It's sometimes difficult to tell if somewhere is a good place to add something, but here it's obvious that it's a bad one, _receiveStatusInformation should really not be between the connectionErrorReason getter and its default value.

Maybe put it above the method where it's value is set? (so above the connect method in this case)

>@@ -249,18 +251,34 @@ imAccount.prototype = {
>   set observedStatusInfo(aUserStatusInfo) {
>     if (!this.prplAccount)
>       return;
>     if (this._statusObserver)
>       this.statusInfo.removeObserver(this._statusObserver);
>     this._observedStatusInfo = aUserStatusInfo;
>     if (!this._statusObserver) {
>       let prplAccount = this.prplAccount;
>+      let imIAccount = this;

im*I*Account isn't a good name here. It's not an interface, but an instance of imAccount.

Do we really need to give access to 2 different variables for this function closure?
Maybe we could replace the prplAccount usage with imIAccount.prplAccount?

Or maybe we can just use .bind(this) at the end of the function definition, so that inside the observer we use this and this.prplAccount? That seems cleaner.

>       this._statusObserver = {
>         observe: function(aSubject, aTopic, aData) {
>+          // Only (dis)connect and notify the prplIAccount of status changes if
>+          // the account has already been connected by the user (either via the
>+          // account manager or via auto-login).
>+          if (!imIAccount._receiveStatusInformation)
>+            return;
>+          
>+          // Disconnect or reconnect the account
>+          let statusType = aSubject.statusType;
>+          if (statusType == Ci.imIStatusInfo.STATUS_OFFLINE &&
>+              imIAccount.connected)
>+            prplAccount.disconnect();
>+          else if (statusType > Ci.imIStatusInfo.STATUS_OFFLINE &&
>+                   imIAccount.disconnected)
>+            prplAccount.connect();
>+          
>           prplAccount.observe(aSubject, aTopic, aData);

I don't think the prpl code of the account has anything to do with the notification if it's not connected yet or if it's being disconnected, so add an else before this instruction.


>diff --git a/chat/protocols/twitter/twitter.js b/chat/protocols/twitter/twitter.js

>@@ -372,34 +368,19 @@ Account.prototype = {

>+  // Currently only used for "status-changed" notification, but Twitter does not
>+  // have different statuses.
>+  observe: function(aSubject, aTopic, aMsg) { },

I don't see how an empty function could be only using the "status-changed" notification ;).

What about:
Twitter doesn't broadcast the user's availability, so we can ignore imIUserStatusInfo's status notifications.
Attachment #8352813 - Flags: review?(florian) → review-
Attached patch Patch v3 (obsolete) — Splinter Review
*** Original post on bio 1199 as attmnt 1075 at 2011-12-12 00:00:00 UTC ***

This takes into account the review comments:
 - Moves the definition above the section of code that uses it.
 - Uses bind instead of scoped variables for closures.
 - Stop notifying prplAccount of connect/disconnect (they're "notified" via the connect()/disconnect() methods anyway).
 - Fix the Twitter comment.
Attachment #8352817 - Flags: review?(florian)
Comment on attachment 8352813 [details] [diff] [review]
Patch v2

*** Original change on bio 1199 attmnt 1071 at 2011-12-12 00:00:08 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352813 - Attachment is obsolete: true
Attached patch Patch v4 (obsolete) — Splinter Review
*** Original post on bio 1199 as attmnt 1076 at 2011-12-12 01:18:00 UTC ***

This removes the extra variable and just checks whether _statusObserver exists when connecting/disconnecting. It seems to work in my simple test cases I've been running, but I'd appreciate feedback.
Attachment #8352818 - Flags: review?(florian)
Comment on attachment 8352817 [details] [diff] [review]
Patch v3

*** Original change on bio 1199 attmnt 1075 at 2011-12-12 01:18:08 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352817 - Attachment is obsolete: true
Attachment #8352817 - Flags: review?(florian)
Comment on attachment 8352818 [details] [diff] [review]
Patch v4

*** Original change on bio 1199 attmnt 1076 at 2011-12-12 10:30:10 UTC ***

I'm not confident that the code in this patch wouldn't add the observer several times if the user clicks on "connect" in the account manager several times after the account has been disconnected with an error.

Wouldn't it be more reliable to create the statusObserver object and add it in observe (when observing the "account-connected" notification) and to remove and delete it when observing "account-disconnecting"?
Then the observedStatusInfo setter would remove and readd it only if it existed?
Attachment #8352818 - Flags: review?(florian) → review-
Attached patch Patch v5Splinter Review
*** Original post on bio 1199 as attmnt 1085 at 2011-12-16 19:33:00 UTC ***

After some more discussion on IRC:
 - Observer gets added when "account-connected" is observed.
 - Observer gets removed when "disconnect" method is run.
Attachment #8352827 - Flags: review?(florian)
Comment on attachment 8352818 [details] [diff] [review]
Patch v4

*** Original change on bio 1199 attmnt 1076 at 2011-12-16 19:33:53 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352818 - Attachment is obsolete: true
Comment on attachment 8352827 [details] [diff] [review]
Patch v5

*** Original change on bio 1199 attmnt 1085 at 2011-12-21 13:21:04 UTC ***

Looks ready to land this time! Thanks for working on this :)
Attachment #8352827 - Flags: review?(florian) → review+
*** Original post on bio 1199 at 2011-12-21 19:10:46 UTC ***

https://hg.instantbird.org/instantbird/rev/59b6d45b646e
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.2
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: