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

RESOLVED FIXED in Thunderbird 17.0

Status

Thunderbird
Instant Messaging
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: florian, Assigned: florian)

Tracking

Trunk
Thunderbird 17.0

Thunderbird Tracking Flags

(thunderbird15 fixed, thunderbird16 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
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.
Attachment #648729 - Flags: review?(mconley)
Attachment #648729 - Flags: review?(aletheia2)
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?
Attachment #648729 - Flags: review?(mconley) → review-
(Assignee)

Comment 2

5 years ago
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.
Attachment #648729 - Attachment is obsolete: true
Attachment #648729 - Flags: review?(aletheia2)
Attachment #648840 - Flags: review?(mconley)
(Assignee)

Comment 3

5 years ago
Created attachment 648846 [details] [diff] [review]
Patch v2.1

Same patch without the dump statements...
Attachment #648840 - Attachment is obsolete: true
Attachment #648840 - Flags: review?(mconley)
Attachment #648846 - Flags: review?(mconley)
(Assignee)

Updated

5 years ago
Attachment #648846 - Flags: review?(mconley) → review?(bwinton)
Comment on attachment 648846 [details] [diff] [review]
Patch v2.1

Seems okay.  r=me.

Thanks,
Blake.
Attachment #648846 - Flags: review?(bwinton) → review+
(Assignee)

Updated

5 years ago
Attachment #648846 - Flags: approval-comm-beta?
Attachment #648846 - Flags: approval-comm-aurora?
Attachment #648846 - Flags: approval-comm-beta?
Attachment #648846 - Flags: approval-comm-beta+
Attachment #648846 - Flags: approval-comm-aurora?
Attachment #648846 - Flags: approval-comm-aurora+
(Assignee)

Comment 5

5 years ago
https://hg.mozilla.org/comm-central/rev/4b4b1339d84e
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 17.0
(Assignee)

Comment 6

5 years ago
https://hg.mozilla.org/releases/comm-aurora/rev/334e0b2c223d
https://hg.mozilla.org/releases/comm-beta/rev/8cdfef4a5308
status-thunderbird15: --- → fixed
status-thunderbird16: --- → fixed
You need to log in before you can comment on or make changes to this bug.