Closed Bug 954735 Opened 10 years ago Closed 10 years ago

IRC contacts don't get their status updated

Categories

(Chat Core :: IRC, defect)

x86
All
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aleth, Assigned: clokep)

References

Details

(Whiteboard: [1.2-blocking])

Attachments

(3 files, 5 obsolete files)

*** Original post on bio 1303 at 2012-03-01 01:10:00 UTC ***

- Nicks that go offline aren't marked as offline in the buddy list. As they leave and even when the account is disconnected.

- Nicks that come online don't appear in the buddy list (even after >10 minutes)

On reconnect, these status changes aren't updated even after a couple of minutes. No error messages.
Blocks: 953944
Whiteboard: [1.2-blocking]
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1303 as attmnt 1221 at 2012-03-05 01:39:00 UTC ***

This refills the queue before checking if it's empty...since if you have a short queue it will be empty after the first request.
Attachment #8352974 - Flags: review?(florian)
Assignee: nobody → clokep
Status: NEW → ASSIGNED
Comment on attachment 8352974 [details] [diff] [review]
Patch

*** Original change on bio 1303 attmnt 1221 at 2012-03-05 10:12:00 UTC ***

This fixes the problem for contacts that come online and go offline while the account is connected. :)

However, if the user himself disconnects, IRC contacts on that account do not go offline.
Attachment #8352974 - Flags: review?(florian) → review-
Attached patch Patch v2 (obsolete) — Splinter Review
*** Original post on bio 1303 as attmnt 1223 at 2012-03-05 11:41:00 UTC ***

Good catch, I had forgotten about that since it wasn't in the bug description!
Attachment #8352976 - Flags: review?(bugzilla)
Comment on attachment 8352974 [details] [diff] [review]
Patch

*** Original change on bio 1303 attmnt 1221 at 2012-03-05 11:41:35 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352974 - Attachment is obsolete: true
Comment on attachment 8352976 [details] [diff] [review]
Patch v2

*** Original change on bio 1303 attmnt 1223 at 2012-03-05 11:48:34 UTC ***

> Good catch, I had forgotten about that since it wasn't in the bug description!
It was in comment #0 ;)

Thanks for fixing this!
Attachment #8352976 - Flags: review?(bugzilla) → review+
*** Original post on bio 1303 at 2012-03-05 13:09:32 UTC ***

flo wants a comment explaining why we check if buddy exists (and in the other place this is used in reply 303).
Attached patch Patch v2.1 (obsolete) — Splinter Review
*** Original post on bio 1303 as attmnt 1224 at 2012-03-06 01:06:00 UTC ***

I'm carrying this review forward as the only difference is a comment. The other usage of getBuddy already has a comment explaining why we're checking if the buddy exists.
Attachment #8352977 - Flags: review+
Comment on attachment 8352976 [details] [diff] [review]
Patch v2

*** Original change on bio 1303 attmnt 1223 at 2012-03-06 01:06:42 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352976 - Attachment is obsolete: true
Attached patch Patch v3Splinter Review
*** Original post on bio 1303 as attmnt 1228 at 2012-03-06 23:53:00 UTC ***

Now using the _buddies directly...
Attachment #8352981 - Flags: review?(florian)
Comment on attachment 8352977 [details] [diff] [review]
Patch v2.1

*** Original change on bio 1303 attmnt 1224 at 2012-03-06 23:53:04 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352977 - Attachment is obsolete: true
Comment on attachment 8352981 [details] [diff] [review]
Patch v3

*** Original change on bio 1303 attmnt 1228 at 2012-03-06 23:56:04 UTC ***

Looks OK.

I have a feeling that we should eventually find a way to handle setting all the buddies of a disconnecting account to STATUS_UNKNOWN somewhere in the core, but that's very clearly out of the scope of this bug :).
Attachment #8352981 - Flags: review?(florian) → review+
*** Original post on bio 1303 at 2012-03-07 00:30:57 UTC ***

Checked in as http://hg.instantbird.org/instantbird/rev/ee75fc196d4c
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.2
*** Original post on bio 1303 at 2012-03-14 23:42:59 UTC ***

This is not entirely fixed in the latest nightly :(

I still occasionally see buddies that have gone offline (with quit message and everything) not disappear from the contact list.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
*** Original post on bio 1303 at 2012-03-15 00:11:08 UTC ***

(In reply to comment #10)
> This is not entirely fixed in the latest nightly :(
> 
> I still occasionally see buddies that have gone offline (with quit message and
> everything) not disappear from the contact list.

Do they continually persist online even after a few minutes?
*** Original post on bio 1303 at 2012-03-15 10:15:27 UTC ***

(In reply to comment #11)

> Do they continually persist online even after a few minutes?

Yes, still shown online several hours later. I forgot to mention it as I was in the middle of the craziness surrounding IM-in-Tb's landing! :)
*** Original post on bio 1303 at 2012-04-05 22:15:35 UTC ***

Running two instances of IB, I just saw one instance had noticed flo had left, while the other had not. The difference was that the second instance had a channel open in which flo was a participant.

Here's half an idea: removeParticipant sends a "chat-buddy-remove" event when a nick quits. After this happens, maybe the ISON handler no longer sets that nick to offline.
*** Original post on bio 1303 at 2012-04-05 22:24:10 UTC ***

(In reply to comment #13)
> Here's half an idea: removeParticipant sends a "chat-buddy-remove" event when a
> nick quits. After this happens, maybe the ISON handler no longer sets that nick
> to offline.
I don't see a mechanism for this to happen though.

Some further info: The client which hasn't noticed flo has left sends out ISON messages "ISON :flo clokep clokep_work ..." and receives ":concrete.mozilla.org 303 aleth :". Which explains why it hasn't noticed, at least the proximate cause.
*** Original post on bio 1303 at 2012-04-05 22:29:42 UTC ***

(In reply to comment #14)
> Some further info: The client which hasn't noticed flo has left sends out ISON
> messages "ISON :flo clokep clokep_work ..." and receives ":concrete.mozilla.org
> 303 aleth :". Which explains why it hasn't noticed, at least the proximate
> cause.

Should one conclude from this it's a server-side issue, and the only thing IB can do is add explicit buddy status updates to the JOIN/QUIT message handler?
*** Original post on bio 1303 at 2012-04-06 00:52:32 UTC ***

(In reply to comment #14)
(In reply to comment #15)
Clearly I suffered a momentary lapse of reason when I wrote those comments :( Please ignore.
*** Original post on bio 1303 at 2012-04-06 00:55:58 UTC ***

Instead I suspect the problem is that when 303 returns a blank list, nobody gets marked offline in that case.
Attached patch Handle an empty array (obsolete) — Splinter Review
*** Original post on bio 1303 as attmnt 1305 at 2012-04-06 01:13:00 UTC ***

Handle the empty array case.
Attachment #8353058 - Flags: review?(bugzilla)
Comment on attachment 8353058 [details] [diff] [review]
Handle an empty array

*** Original change on bio 1303 attmnt 1305 at 2012-04-06 09:45:33 UTC ***

This looks good!

Maybe one should clarify the comment
> // If there are any buddies online, mark them as online.
to something like "Set the status of the buddies in the last ISON message", to clarify that they will be marked either online or offline?
Attachment #8353058 - Flags: review?(bugzilla) → review+
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1303 as attmnt 1307 at 2012-04-06 09:47:00 UTC ***

Sets the buddy status immediately on receiving a JOIN/QUIT message when appropriate, to avoid user confusion due to a possible delay.
Attachment #8353060 - Flags: review?(clokep)
Comment on attachment 8353060 [details] [diff] [review]
Patch

*** Original change on bio 1303 attmnt 1307 at 2012-04-06 10:29:00 UTC ***

For both situations you have the test inside of a loop over the conversations...it should be outside this loop since this only needs to be done once. :)
Attachment #8353060 - Flags: review?(clokep) → review-
*** Original post on bio 1303 as attmnt 1310 at 2012-04-06 10:37:00 UTC ***

This is the same as attachment 8353058 [details] [diff] [review] (bio-attmnt 1305) with a changed comment so I'm carrying the review forward.
Attachment #8353063 - Flags: review+
Comment on attachment 8353058 [details] [diff] [review]
Handle an empty array

*** Original change on bio 1303 attmnt 1305 at 2012-04-06 10:37:11 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353058 - Attachment is obsolete: true
Attached patch Handle join/quitSplinter Review
*** Original post on bio 1303 as attmnt 1311 at 2012-04-06 10:39:00 UTC ***

Ah yes.
Attachment #8353064 - Flags: review?(clokep)
Comment on attachment 8353060 [details] [diff] [review]
Patch

*** Original change on bio 1303 attmnt 1307 at 2012-04-06 10:39:14 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353060 - Attachment is obsolete: true
Comment on attachment 8353064 [details] [diff] [review]
Handle join/quit

*** Original change on bio 1303 attmnt 1311 at 2012-04-06 10:45:10 UTC ***

Looks good! Thanks for fixing this! Please add yourself to the license header on our next patches to irc.js and ircBase.jsm. :)
Attachment #8353064 - Flags: review?(clokep) → review+
Whiteboard: [1.2-blocking] → [1.2-blocking][checkin-needed]
*** Original post on bio 1303 at 2012-04-06 23:32:02 UTC ***

Attachment 8353063 [details] [diff] (bio-attmnt 1310) checked in as http://hg.instantbird.org/instantbird/rev/b1cea431c4a4 and attachment 8353064 [details] [diff] [review] (bio-attmnt 1311) as http://hg.instantbird.org/instantbird/rev/54fccd250e03

Please file new bugs for any further issues.
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Whiteboard: [1.2-blocking][checkin-needed] → [1.2-blocking]
You need to log in before you can comment on or make changes to this bug.