954085 - Twitter accounts should reconnect automatically when going back from offline

Status: RESOLVED FIXED

(Chat Core :: Twitter, defect)

Description

[Patrick Cloke [:clokep]]

*** Original post on bio 650 at 2011-01-07 05:04:00 UTC ***

*** Due to BzAPI limitations, the initial description is in comment 1 ***

Comment 1

[Patrick Cloke [:clokep]]

Attachment: Patch v0.1 for API review

*** Original post on bio 650 as attmnt 471 at 2011-01-07 05:04:00 UTC ***

There doesn't seem to be an API for libpurple accounts to be notified from the libpurple core of changes in user status (i.e. setting myself to away should cause IRC to set itself to away).

The patch I'm attaching is really just for API review (and might not be patchable since my file contains bits of bug 953932 (bio 495), bug 954082 (bio 647), bug 954083 (bio 648) and bug 954084 (bio 649) at the moment).

Pretty much we observe for the "status-changed" action and then call a special function.

Comment 2

[Florian Quèze [:florian]]

Comment on attachment 8352214 [details] [diff] [review]
Patch v0.1 for API review

*** Original change on bio 650 attmnt 471 at 2011-01-14 13:16:06 UTC ***

>diff -r ba17c12c2206 purple/purplexpcom/src/jsProtoHelper.jsm
>--- a/purple/purplexpcom/src/jsProtoHelper.jsm	Thu Jan 06 16:48:25 2011 +0100
>+++ b/purple/purplexpcom/src/jsProtoHelper.jsm	Fri Jan 07 00:02:41 2011 -0500
>@@ -126,9 +126,27 @@
>     this._base = new AccountBase();
>     this._base.concreteAccount = this;
>     this._base.init(aKey, aName, aProtoInstance);
>+
>+    Services.obs.addObserver(this, "status-changed", false);

If you add an observer, it should be removed when the account object is no longer needed, or it will leak. You can also use a weak reference (put "true" as the last parameter, and add nsIWeakReference in the list of interfaces the QueryInterface function will accept).

>+  observe: function(aSubject, aTopic, aMsg) {
Are we sure the account implementation from the protocol specific code will never need to implement nsIObserver?

>+    let statusType = aSubject.currentStatusType;
>+    if (statusType == Ci.purpleICoreService.STATUS_OFFLINE)
>+      this.disconnect();
>+    else
>+      this.statusChanged(statusType, aMsg);
Hmm, is there a reason for handling calls to disconnect automatically but not to connect?

>+  },
>+  statusChanged: function(aStatusType, aMsg) {
>+    throw Components.results.NS_ERROR_NOT_IMPLEMENTED;
>+  },

Can't we have accounts for protocols that don't care if there's a status change? A protocol plugin to send text messages to mobile phones may not care for example what the current user status is. And offline in the sense "no internet connection usable from the computer" may not even be relevant if the message is sent to the phone through a wifi or bluetooth connection.

On the other hand, throwing from an observer wouldn't cause much trouble...


More generally, about the approach taken here: it's a bit sad to have one observer per account. I think the "good" implementation would be something looping over the list of accounts (both JS-implemented and libpurple-implemented) and calling a method on the account to request the status change is sent over the network. Probably quite a bit of crappy code to rewrite in purpleCoreService.cpp and some confusing code to remove from libpurple to achieve this.

If this bug is blocking you, I think it's OK to accept a patch working around this from jsProtoHelper as you did in attachment 8352214 [details] [diff] [review] (bio-attmnt 471) (the above review comments about the patch itself would still need to be addressed of course).

Comment 3

[Patrick Cloke [:clokep]]

*** Original post on bio 650 at 2011-01-25 04:32:26 UTC ***

> More generally, about the approach taken here: it's a bit sad to have one
> observer per account. I think the "good" implementation would be something
> looping over the list of accounts (both JS-implemented and
> libpurple-implemented) and calling a method on the account to request the
> status change is sent over the network. Probably quite a bit of crappy code to
> rewrite in purpleCoreService.cpp and some confusing code to remove from
> libpurple to achieve this.

I think the code to change in purpleCoreService.cpp is the two functions at http://lxr.instantbird.org/instantbird/source/purple/purplexpcom/src/purpleCoreService.cpp#938 Does this seem correct flo?

I'm not sure where to even begin looking in libpurple for the related code.

Comment 4

[Patrick Cloke [:clokep]]

*** Original post on bio 650 at 2011-02-26 22:35:45 UTC ***

This is not on my plate at the moment, I might come back to it later.

Comment 5

[Florian Quèze [:florian]]

*** Original post on bio 650 at 2011-05-17 15:20:09 UTC ***

Marking wanted in the status whiteboard as this blocks "[Twitter] Reconnect automatically when going back from offline" on our roadmap.

The 0.3 solution may turn out to be just to observe the notification in each account that cares about status changes.

Comment 6

[Patrick Cloke [:clokep]]

Attachment: v1.0

*** Original post on bio 650 as attmnt 724 at 2011-06-17 20:39:00 UTC ***

This works (logic wise), but probably leaks since I'm not deleting the observer, except I'm not sure where to add the removeObserver call. The logic is kind of confusing though, I'm not sure if you can think of a better way to do this flo?

Comment 7

[Patrick Cloke [:clokep]]

Comment on attachment 8352214 [details] [diff] [review]
Patch v0.1 for API review

*** Original change on bio 650 attmnt 471 at 2011-06-17 20:39:04 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 8

[Florian Quèze [:florian]]

Comment on attachment 8352466 [details] [diff] [review]
v1.0

*** Original change on bio 650 attmnt 724 at 2011-06-17 21:21:54 UTC ***

>diff --git a/purple/purplexpcom/src/twitter.js b/purple/purplexpcom/src/twitter.js

>@@ -149,18 +154,34 @@ Account.prototype = {
>     // Get a new token if needed...
>     if (!this.token || !this.tokenSecret) {
>       this.requestToken();
>       return;
>     }
> 
>     LOG("Connecting using existing token");
>     this.getTimelines();
>+    this._connected = true;
>   },

You are setting this to true only if the account is connected using the previously stored oauth token. I don't think this is what you want.

Do you want this to be set to true if a connection is attempted but we failed to get the oauth authentication?
If so you should set this to true sooner, juste after the this.base.connecting call.
If not, then you should move that line to the beginning of the getTimelines method.

> 
>+  // Currently only used for "status-changed" notification.
>+  observe: function(aSubject, aTopic, aMsg) {
>+    let connected = this._connected;
>+    if (!connected)
>+      return;

You don't need this variable.

>+
>+    if (aSubject.currentStatusType == Ci.imIStatusInfo.STATUS_OFFLINE)
>+      this.disconnect();

set this._connected to true here.

>+    else if (aSubject.currentStatusType == Ci.imIStatusInfo.STATUS_AVAILABLE)
>+      this.connect();

If the status is "Unavailable" you should reconnect too.
You probably want to check |currentStatusType > STATUS_OFFLINE| (even the "INVISIBLE" status should cause a reconnection).

>+
>+    // If we disconnect from a status change, keep the original connection
>+    // status.
>+    this._connected = connected;

Not needed in the connect case :).

>@@ -513,16 +534,17 @@ Account.prototype = {
>     this.base.disconnected();
>   },
>   UnInit: function() {
>     this.cleanUp();

<here> is the perfect place for your removeObserver call.

>     this._base.UnInit();
>   },
>   disconnect: function() {
>     this.gotDisconnected();
>+    this._connected = false;

Nit: I would prefer |delete this._connected;| (to remove the value and fallback to the value of the prototype.

Comment 9

[Patrick Cloke [:clokep]]

Attachment: v1.1

*** Original post on bio 650 as attmnt 725 at 2011-06-17 21:43:00 UTC ***

Hopefully an r+ this time.

As discussed on IRC I changed the variable name from _connected to _enabled (and fixed the other issues).

Comment 10

[Patrick Cloke [:clokep]]

Comment on attachment 8352466 [details] [diff] [review]
v1.0

*** Original change on bio 650 attmnt 724 at 2011-06-17 21:43:03 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 11

[Florian Quèze [:florian]]

Comment on attachment 8352467 [details] [diff] [review]
v1.1

*** Original change on bio 650 attmnt 725 at 2011-06-17 22:47:33 UTC ***

Looks great! :)

Comment 12

[Florian Quèze [:florian]]

*** Original post on bio 650 at 2011-06-17 23:03:50 UTC ***

https://hg.instantbird.org/instantbird/rev/081800f32b6c

Comment 13

[Florian Quèze [:florian]]

*** Original post on bio 650 at 2011-06-21 15:29:10 UTC ***

https://hg.instantbird.org/instantbird/rev/d30455356be8 - follow-up to avoid connecting several times the same account (and receiving 'Error 420' HTTP errors).

Comment 14

[Benedikt Pfeifer]

*** Original post on bio 650 at 2013-03-25 15:51:22 UTC ***

Dependency was added before bug was morphed into a Twitter bug:

Taken from my bug mail (bug 953944 (bio 507)), 2011-01-07:

https://bugzilla.mozilla.org/show_bug.cgi?id=953944 (bio 507)
Patrick Cloke (:clokep) <[e-mail]> changed:
          What    |Removed                    |Added
----------------------------------------------------------------------------
        Depends on|                            |650                       |650


Taken from bug history:

[e-mail] 	2011-05-27 12:33:22 CEST 	Summary 	JavaScript accounts must be notified of status changes 	Twitter accounts should reconnect automatically when going back from offline