Closed Bug 955279 Opened 10 years ago Closed 10 years ago

Changing the status to available while an IRC account is in the disconnecting state doesn't reconnect it

Categories

(Chat Core :: IRC, defect)

x86
Other
defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: florian, Assigned: aleth)

References

Details

Attachments

(2 files, 2 obsolete files)

*** Original post on bio 1846 at 2012-12-05 18:45:00 UTC ***

Typically happens when I use /offline to quickly disconnect all accounts and /back immediately after /offline to reconnect them all, when I know my network connections are dead because I've switched to a different wifi network.

In that case, IRC takes a few seconds to timeout; other accounts get marked as disconnected almost immediately, so they are already in the disconnected state when I type /back.
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1846 as attmnt 2152 at 2012-12-05 22:17:00 UTC ***

Set the reconnect timer if the status is changed from OFFLINE while an account is still disconnecting, so it will connect as soon as it has disconnected.
Attachment #8353914 - Flags: review?(florian)
Attached patch PatchSplinter Review
*** Original post on bio 1846 as attmnt 2154 at 2012-12-05 23:01:00 UTC ***

The above patch fails if the disconnection takes longer than 1s (the first reconnection timer length). 

This approach simply connects as soon as the account is disconnected when appropriate.
Attachment #8353916 - Flags: review?(florian)
Comment on attachment 8353914 [details] [diff] [review]
Patch

*** Original change on bio 1846 attmnt 2152 at 2012-12-05 23:01:04 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353914 - Attachment is obsolete: true
Attachment #8353914 - Flags: review?(florian)
Assignee: nobody → aleth
Status: NEW → ASSIGNED
Comment on attachment 8353916 [details] [diff] [review]
Patch

*** Original change on bio 1846 attmnt 2154 at 2012-12-05 23:03:22 UTC ***

Looks fine to me, thanks!
Attachment #8353916 - Flags: review?(florian) → review+
*** Original post on bio 1846 at 2012-12-05 23:36:54 UTC ***

Committed as http://hg.instantbird.org/instantbird/rev/24431396e7ec
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.4
Attached patch Patch to fix bustage (obsolete) — Splinter Review
*** Original post on bio 1846 as attmnt 2163 at 2012-12-11 21:58:00 UTC ***

Check whether the disconnected account was actually connected without errors before attempting to reconnect. (This avoids e.g. immediate reconnection on status change when there was a reconnection timer present, and reconnection on status change when the account is incorrectly configured.)
Attachment #8353925 - Flags: review?(florian)
Comment on attachment 8353925 [details] [diff] [review]
Patch to fix bustage

*** Original change on bio 1846 attmnt 2163 at 2012-12-12 07:17:50 UTC ***

>diff --git a/components/imAccounts.js b/components/imAccounts.js

>     else if (aTopic == "account-disconnected") {
>       this.connectionState = Ci.imIAccount.STATE_DISCONNECTED;
>       if (this._statusObserver &&
>+          this.prplAccount.connectionErrorReason == Ci.prplIAccount.NO_ERROR &&

Why this.prplAccount.connectionErrorReason and not just this.connectionErrorReason?

>@@ -625,17 +626,18 @@ imAccount.prototype = {

>           else if (statusType > Ci.imIStatusInfo.STATUS_OFFLINE &&
>-                   this.disconnected)
>+                   this.disconnected && (!this.prplAccount.connectionErrorReason ||

What's the !connectionErrorReason test for? It looks like a null check, but 0 is actually ERROR_NETWORK_ERROR.
Attachment #8353925 - Flags: review?(florian) → review-
*** Original post on bio 1846 as attmnt 2165 at 2012-12-12 18:45:00 UTC ***

>Why this.prplAccount.connectionErrorReason and not just
>this.connectionErrorReason?
I've now used this.connectionErrorReason in the status observer and kept this.prplAccount.connectionErrorReason in the account observer (for consistency).

>What's the !connectionErrorReason test for? It looks like a null check, but 0
>is actually ERROR_NETWORK_ERROR.
It was intended to check for 'undefined' but this is unnecessary due to the default value in the prototype.
Attachment #8353927 - Flags: review?(florian)
Comment on attachment 8353925 [details] [diff] [review]
Patch to fix bustage

*** Original change on bio 1846 attmnt 2163 at 2012-12-12 18:45:49 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353925 - Attachment is obsolete: true
Comment on attachment 8353927 [details] [diff] [review]
Patch to fix reconnect timer bustage

*** Original change on bio 1846 attmnt 2165 at 2012-12-14 23:03:43 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353927 - Flags: review?(florian) → review+
*** Original post on bio 1846 at 2012-12-14 23:31:47 UTC ***

attachment 8353927 [details] [diff] [review] (bio-attmnt 2165) checked in as http://hg.instantbird.org/instantbird/rev/16136295ea7b
Depends on: 955376
You need to log in before you can comment on or make changes to this bug.