The default bug view has changed. See this FAQ.

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.