Last Comment Bug 780173 - The "connect" button of the "Show Accounts" dialog stays disabled after disconnecting an IRC account
: The "connect" button of the "Show Accounts" dialog stays disabled after disco...
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Instant Messaging (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Thunderbird 17.0
Assigned To: Florian Quèze [:florian] [:flo]
: instant-messaging
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-03 08:49 PDT by Florian Quèze [:florian] [:flo]
Modified: 2012-08-07 10:26 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
fixed
fixed


Attachments
Patch (6.38 KB, patch)
2012-08-03 08:49 PDT, Florian Quèze [:florian] [:flo]
mconley: review-
Details | Diff | Splinter Review
Patch v2 (6.50 KB, patch)
2012-08-03 13:53 PDT, Florian Quèze [:florian] [:flo]
no flags Details | Diff | Splinter Review
Patch v2.1 (6.44 KB, patch)
2012-08-03 14:05 PDT, Florian Quèze [:florian] [:flo]
bwinton: review+
standard8: approval‑comm‑aurora+
standard8: approval‑comm‑beta+
Details | Diff | Splinter Review

Description Florian Quèze [:florian] [:flo] 2012-08-03 08:49:18 PDT
Created attachment 648729 [details] [diff] [review]
Patch

The code of the "Show Accounts" window has some logic to disable the "connect" and "disconnect" buttons for half a second immediately after they have been clicked to prevent double clicks from triggering a disconnect and a reconnect at the same time.

That code is old and messy but was mostly working. However, the JS-IRC code can sometimes take more than half a second between a click on disconnect and when the account is actually disconnected, and that situation exposes a race in the existing code, that results in the "connect" button staying disabled.

I think it's time to replace this old code with a cleaner version that makes more sense.

Requesting review from aleth who took part in the discussion in the same bug for Instantbird (https://bugzilla.instantbird.org/show_bug.cgi?id=1338), but also from Mike as I'm not sure if aleth can's review is enough to check-in changes to Tb's front-end.
Comment 1 Mike Conley (:mconley) 2012-08-03 12:25:14 PDT
Comment on attachment 648729 [details] [diff] [review]
Patch

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

Two questions - see below.

::: mail/components/im/content/imAccounts.js
@@ +297,5 @@
>        (this.isOffline ||
>         (account.disconnected &&
>          account.connectionErrorReason == Ci.imIAccount.ERROR_UNKNOWN_PRPL));
> +
> +    [["connect", isCommandDisabled],

Is there a good reason why you're using 2 element arrays instead of an object, like you use in onContextMenuShowing?

::: mail/components/im/content/imAccounts.xul
@@ +86,5 @@
>               observes="contextAccountsItems"/>
>     <menuitem id="context_cancelReconnection"
>               command="cmd_cancelReconnection"
>               observes="contextAccountsItems"/>
> +   <menuseparator id="context_accountsItemsSeparator"

Why was this changed?
Comment 2 Florian Quèze [:florian] [:flo] 2012-08-03 13:53:42 PDT
Created attachment 648840 [details] [diff] [review]
Patch v2

(In reply to Mike Conley (:mconley) from comment #1)

> Is there a good reason why you're using 2 element arrays instead of an
> object, like you use in onContextMenuShowing?

No. It's just that not too long ago the first element of each array wasn't a string. Good catch, let's simplify! :)

> ::: mail/components/im/content/imAccounts.xul
> @@ +86,5 @@
> >               observes="contextAccountsItems"/>
> >     <menuitem id="context_cancelReconnection"
> >               command="cmd_cancelReconnection"
> >               observes="contextAccountsItems"/>
> > +   <menuseparator id="context_accountsItemsSeparator"
> 
> Why was this changed?

When an account is in the disconnecting state, both the "Connect" and "Disconnect" buttons are hidden, so I made the behavior of the context menu match, and there was a separator left at the top of the menu. I added an id to the menuseparator so that I could hide it.
Comment 3 Florian Quèze [:florian] [:flo] 2012-08-03 14:05:48 PDT
Created attachment 648846 [details] [diff] [review]
Patch v2.1

Same patch without the dump statements...
Comment 4 Blake Winton (:bwinton) (:☕️) 2012-08-07 08:13:10 PDT
Comment on attachment 648846 [details] [diff] [review]
Patch v2.1

Seems okay.  r=me.

Thanks,
Blake.
Comment 5 Florian Quèze [:florian] [:flo] 2012-08-07 10:18:58 PDT
https://hg.mozilla.org/comm-central/rev/4b4b1339d84e

Note You need to log in before you can comment on or make changes to this bug.