780173 - The "connect" button of the "Show Accounts" dialog stays disabled after disconnecting an IRC account

Status: RESOLVED FIXED

(Thunderbird :: Instant Messaging, defect)

Description

[Florian Quèze [:florian]]

Attachment: 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) (:⚙️)]

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]]

Attachment: 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]]

Attachment: Patch v2.1

Same patch without the dump statements...

Comment 4

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 648846 [details] [diff] [review]
Patch v2.1

Seems okay.  r=me.

Thanks,
Blake.

Comment 5

[Florian Quèze [:florian]]

https://hg.mozilla.org/comm-central/rev/4b4b1339d84e

Comment 6

[Florian Quèze [:florian]]

https://hg.mozilla.org/releases/comm-aurora/rev/334e0b2c223d
https://hg.mozilla.org/releases/comm-beta/rev/8cdfef4a5308