Closed
Bug 954735
Opened 11 years ago
Closed 11 years ago
IRC contacts don't get their status updated
Categories
(Chat Core :: IRC, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
1.2
People
(Reporter: aleth, Assigned: clokep)
References
Details
(Whiteboard: [1.2-blocking])
Attachments
(3 files, 5 obsolete files)
(deleted),
patch
|
florian
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
clokep
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
clokep
:
review+
|
Details | Diff | Splinter Review |
*** 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.
Assignee | ||
Comment 1•11 years ago
|
||
*** 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 | ||
Updated•11 years ago
|
Assignee: nobody → clokep
Status: NEW → ASSIGNED
Reporter | ||
Comment 2•11 years ago
|
||
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-
Assignee | ||
Comment 3•11 years ago
|
||
*** 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)
Assignee | ||
Comment 4•11 years ago
|
||
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
Reporter | ||
Comment 5•11 years ago
|
||
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+
Assignee | ||
Comment 6•11 years ago
|
||
*** 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).
Assignee | ||
Comment 7•11 years ago
|
||
*** 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+
Assignee | ||
Comment 8•11 years ago
|
||
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
Assignee | ||
Comment 9•11 years ago
|
||
*** 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)
Assignee | ||
Comment 10•11 years ago
|
||
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 11•11 years ago
|
||
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+
Assignee | ||
Comment 12•11 years ago
|
||
*** 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: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.2
Reporter | ||
Comment 13•11 years ago
|
||
*** 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 → ---
Assignee | ||
Comment 14•11 years ago
|
||
*** 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?
Comment 15•11 years ago
|
||
*** 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! :)
Reporter | ||
Comment 16•11 years ago
|
||
*** 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.
Reporter | ||
Comment 17•11 years ago
|
||
*** 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.
Reporter | ||
Comment 18•11 years ago
|
||
*** 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?
Reporter | ||
Comment 19•11 years ago
|
||
*** 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.
Reporter | ||
Comment 20•11 years ago
|
||
*** 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.
Assignee | ||
Comment 21•11 years ago
|
||
*** 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)
Reporter | ||
Comment 22•11 years ago
|
||
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+
Reporter | ||
Comment 23•11 years ago
|
||
*** 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)
Assignee | ||
Comment 24•11 years ago
|
||
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-
Assignee | ||
Comment 25•11 years ago
|
||
*** 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+
Assignee | ||
Comment 26•11 years ago
|
||
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
Reporter | ||
Comment 27•11 years ago
|
||
*** Original post on bio 1303 as attmnt 1311 at 2012-04-06 10:39:00 UTC ***
Ah yes.
Attachment #8353064 -
Flags: review?(clokep)
Reporter | ||
Comment 28•11 years ago
|
||
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
Assignee | ||
Comment 29•11 years ago
|
||
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+
Reporter | ||
Updated•11 years ago
|
Whiteboard: [1.2-blocking] → [1.2-blocking][checkin-needed]
Assignee | ||
Comment 30•11 years ago
|
||
*** 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: 11 years ago → 11 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.
Description
•