Closed Bug 1009504 Opened 7 years ago Closed 7 years ago

Throbber persists when disconnected while joining

Categories

(Instantbird :: Conversation, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aleth, Assigned: aleth)

References

Details

Attachments

(1 file, 1 obsolete file)

I thought I'd taken care of this in the original patch, but I missed something.
Blocks: 1003200
Attached patch 1009504.patch (obsolete) — Splinter Review
Fixes the existing code in jsProtoHelper, and modifies getDisconnected() which didn't work when disconnected during joining (because left=true is the default state for IRC).
Attachment #8423161 - Flags: review?(clokep)
Comment on attachment 8423161 [details] [diff] [review]
1009504.patch

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

::: chat/modules/jsProtoHelper.jsm
@@ +577,5 @@
>  
>    _left: false,
>    get left() this._left,
>    set left(aLeft) {
> +    if (aLeft == this._left && !this.joining)

We're talking about this on IRC, but I'm halfway through my review so I'm just going to hit save: This scares me, I don't see !this.joining as a valid statement to return early. Can you please include some rationale (both in the bug and in a comment in the code) for why this is easy?

::: chat/protocols/irc/irc.js
@@ +1668,5 @@
>  
>      // Clean up each conversation: mark as left and remove participant.
>      for each (let conversation in this._conversations) {
> +      if (conversation.isChat) {
> +        conversation.joining = false;

Please add a comment above this saying something about "In case the channel was never fully joined" or something.
Attached patch 1009504.patch 2Splinter Review
Looking at this again, I think we can do without the extra safety of the logic in the left setter altogether.
Attachment #8423161 - Attachment is obsolete: true
Attachment #8423161 - Flags: review?(clokep)
Attachment #8423234 - Flags: review?(clokep)
Comment on attachment 8423234 [details] [diff] [review]
1009504.patch 2

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

Thanks!
Attachment #8423234 - Flags: review?(clokep) → review+
https://hg.mozilla.org/comm-central/rev/9f5b1f06702d
Status: NEW → RESOLVED
Closed: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 1.6
You need to log in before you can comment on or make changes to this bug.