Closed
Bug 955111
Opened 10 years ago
Closed 10 years ago
Restore participants' active status if still appropriate after a reconnect
Categories
(Instantbird Graveyard :: Conversation, enhancement)
Instantbird Graveyard
Conversation
Tracking
(Not tracked)
RESOLVED
FIXED
1.3
People
(Reporter: aleth, Assigned: aleth)
References
Details
Attachments
(1 file, 8 obsolete files)
7.27 KB,
patch
|
florian
:
review+
|
Details | Diff | Splinter Review |
*** Original post on bio 1683 at 2012-08-28 23:29:00 UTC *** When the user, or the participant, briefly disconnects/reconnects, their active/inactive status should be restored to what it was before this happens. This is both for consistency with what we see for the same conversation on restoring from hold, and because generally the disconnect/reconnect will have been automatic.
Comment 1•10 years ago
|
||
*** Original post on bio 1683 at 2012-08-29 07:17:04 UTC *** That means that the (in)active status of a participant should only depend on how long it is since his last message to the channel? Quitting, parting, closing the conversation (or even crashing?) shouldn't affect the status then? Sounds like a good idea in my opinion.
Severity: normal → enhancement
Assignee | ||
Comment 2•10 years ago
|
||
*** Original post on bio 1683 as attmnt 1875 at 2012-08-30 16:26:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Assignee | ||
Comment 3•10 years ago
|
||
Comment on attachment 8353633 [details] [diff] [review] WIP *** Original change on bio 1683 attmnt 1875 at 2012-08-31 11:06:31 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353633 -
Flags: feedback?(clokep)
Assignee | ||
Comment 4•10 years ago
|
||
*** Original post on bio 1683 at 2012-09-01 16:07:43 UTC *** (In reply to comment #2) > WIP Do you see a more elegant way to do this?
Comment 5•10 years ago
|
||
*** Original post on bio 1683 at 2012-09-01 16:45:55 UTC *** Well, my first thought is that you don't want to use the same time for "active" as keeping a buddy active through a part/quit. I.e. if I say something (therefore I'm active) leave work and drive home and sign back online, 30 - 40 minutes might have passed...but I'd still be active? That doesn't seem to make sense to me. (I also think it will look like a bug to users, some buddies sign on and are active and others aren't.) I think we should only try to handle the case where a buddy signs off and signs back on within a few minutes. Besides that, a more elegant way doesn't really come to me, but maybe Florian has some ideas.
Comment 6•10 years ago
|
||
Comment on attachment 8353633 [details] [diff] [review] WIP *** Original change on bio 1683 attmnt 1875 at 2012-09-01 16:46:26 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353633 -
Flags: feedback?(clokep) → feedback+
Updated•10 years ago
|
Assignee: nobody → aleth
Status: NEW → ASSIGNED
Assignee | ||
Comment 7•10 years ago
|
||
*** Original post on bio 1683 at 2012-09-01 17:45:29 UTC *** (In reply to comment #4) > I think we should only try to handle the case where a buddy signs off and signs > back on within a few minutes. Yes, I agree with this. The WIP was experimenting with "who cares if you disconnected", but I don't think we should completely ignore the fact someone left if it seems intentional.
Assignee | ||
Comment 8•10 years ago
|
||
*** Original post on bio 1683 as attmnt 1883 at 2012-09-01 20:40:00 UTC *** How about this? Keeps them active for around three minutes, which should be long enough for e.g. a reboot but not too much longer. I don't think this timeout needs to be a pref really, it should simply have a sensible value. (Is this sensible?)
Attachment #8353641 -
Flags: review?(clokep)
Assignee | ||
Comment 9•10 years ago
|
||
Comment on attachment 8353633 [details] [diff] [review] WIP *** Original change on bio 1683 attmnt 1875 at 2012-09-01 20:40:03 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353633 -
Attachment is obsolete: true
Assignee | ||
Comment 10•10 years ago
|
||
*** Original post on bio 1683 as attmnt 1884 at 2012-09-01 21:15:00 UTC *** Alternative patch that also sets the nick colours lazily.
Attachment #8353642 -
Flags: review?(clokep)
Assignee | ||
Comment 11•10 years ago
|
||
Comment on attachment 8353642 [details] [diff] [review] Patch with lazy color *** Original change on bio 1683 attmnt 1884 at 2012-09-01 21:56:15 UTC *** moved to separate bug
Attachment #8353642 -
Attachment is obsolete: true
Attachment #8353642 -
Flags: review?(clokep)
Comment 12•10 years ago
|
||
Comment on attachment 8353641 [details] [diff] [review] Patch *** Original change on bio 1683 attmnt 1883 at 2012-09-02 13:02:51 UTC *** >diff --git a/chrome/instantbird/content/instantbird/conversation.xml b/chrome/instantbird/content/instantbird/conversation.xml > // Set nick to active if the message is not older than > // nickActiveTimespan. > let nickActiveTimespan = > Services.prefs.getIntPref("messenger.conversations.nickActiveTimespan"); > if (nickActiveTimespan > 0) { >- let msgAge = Date.now() / 1000 - aMsg.time; >- let waitBeforeInactive = nickActiveTimespan - msgAge; >+ let expireTime = aMsg.time + nickActiveTimespan; >+ let waitBeforeInactive = expireTime - Date.now() / 1000; Would it make more sense here to do waitBeforeInactive = expireTime * 1000 - Date.now(); And then not re-multiply to get milliseconds later in the function? Or even just have expireTime in milliseconds, as you again multiple that by 1000 later. >+ <field name="_partedActiveBuddies">[]</field> Seems like you're actually using this as a hash later, which would imply to me a {}. > <method name="removeBuddy"> [...] >- if (item.activeTimer) >+ if (item.activeTimer) { >+ // If the buddy is absent for less than this timespan, >+ // it will retain its active participant status on rejoining. >+ const kRestoreActiveTimespan = 200000; 200000...what? Seconds? Milliseconds? > clearTimeout(item.activeTimer); >+ this._partedActiveBuddies[aName] = { >+ expireTime: item.expireTime, >+ timer: setTimeout(function() { >+ delete this._partedActiveBuddies[aName]; >+ }.bind(this), kRestoreActiveTimespan) I did have a thought of whether we care to have this timer to remove things from the list or just let them stay in there and ignore times that are < 0 if people rejoin. Any thoughts on this?
Attachment #8353641 -
Flags: review?(clokep) → review-
Assignee | ||
Comment 13•10 years ago
|
||
*** Original post on bio 1683 as attmnt 1889 at 2012-09-02 13:28:00 UTC *** (In reply to comment #9) > Would it make more sense here to do > waitBeforeInactive = expireTime * 1000 - Date.now(); > And then not re-multiply to get milliseconds later in the function? Or even > just have expireTime in milliseconds, as you again multiple that by 1000 later. In my mind I was doing it this way to round to seconds, but of course JS always does everything with floats, so... yeah. > >+ this._partedActiveBuddies[aName] = { > >+ expireTime: item.expireTime, > >+ timer: setTimeout(function() { > >+ delete this._partedActiveBuddies[aName]; > >+ }.bind(this), kRestoreActiveTimespan) > I did have a thought of whether we care to have this timer to remove things > from the list or just let them stay in there and ignore times that are < 0 if > people rejoin. Any thoughts on this? If we are connected to a busy channel with lots of people coming and going, and keep IB running for days, the list will just grow and grow without bound that way.
Attachment #8353647 -
Flags: review?(clokep)
Assignee | ||
Comment 14•10 years ago
|
||
Comment on attachment 8353641 [details] [diff] [review] Patch *** Original change on bio 1683 attmnt 1883 at 2012-09-02 13:28:06 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353641 -
Attachment is obsolete: true
Assignee | ||
Comment 15•10 years ago
|
||
Comment on attachment 8353647 [details] [diff] [review] Patch *** Original change on bio 1683 attmnt 1889 at 2012-09-02 13:29:01 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353647 -
Flags: review?(clokep)
Assignee | ||
Comment 16•10 years ago
|
||
*** Original post on bio 1683 as attmnt 1890 at 2012-09-02 13:44:00 UTC ***
>+ <field name="_partedActiveBuddies">[]</field>
>Seems like you're actually using this as a hash later, which would imply to me
>a {}.
You're right of course, but strangely <field name="_partedActiveBuddies">{}</field> leads to an error (_partedActiveBuddies is undefined). I've now done the same thing we do for this.buddies itself.
Attachment #8353648 -
Flags: review?(clokep)
Assignee | ||
Comment 17•10 years ago
|
||
Comment on attachment 8353647 [details] [diff] [review] Patch *** Original change on bio 1683 attmnt 1889 at 2012-09-02 13:44:19 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353647 -
Attachment is obsolete: true
Comment 18•10 years ago
|
||
*** Original post on bio 1683 at 2012-09-02 20:20:12 UTC *** (In reply to comment #11) > >+ <field name="_partedActiveBuddies">[]</field> > >Seems like you're actually using this as a hash later, which would imply to me > >a {}. > You're right of course, but strangely <field > name="_partedActiveBuddies">{}</field> leads to an error (_partedActiveBuddies > is undefined). So [] works but not {} ? Maybe try ({}) instead of {}. (In reply to comment #6) > How about this? Keeps them active for around three minutes, which should be > long enough for e.g. a reboot but not too much longer. I don't think this > timeout needs to be a pref really, it should simply have a sensible value. (Is > this sensible?) Even though I don't have a solid rationale for it as it's not exactly related, I would suggest using the same value as http://lxr.instantbird.org/instantbird/source/chat/chat-prefs.js#60: pref("messenger.options.messagesStyle.combineConsecutiveInterval", 300); // 5 minutes (not necessarily fetching it from the pref though; having the const in the code is fine with me).
Assignee | ||
Comment 19•10 years ago
|
||
*** Original post on bio 1683 as attmnt 1891 at 2012-09-02 21:00:00 UTC ***
Seems reasonable, 5 minutes is fine with me.
>So [] works but not {} ? Maybe try ({}) instead of {}.
I'm sticking with the <field>-free definition, it makes sense to follow this.buddies for this and (re)initialize them simultaneously.
Attachment #8353649 -
Flags: review?(clokep)
Assignee | ||
Comment 20•10 years ago
|
||
Comment on attachment 8353648 [details] [diff] [review] Patch *** Original change on bio 1683 attmnt 1890 at 2012-09-02 21:00:17 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353648 -
Attachment is obsolete: true
Attachment #8353648 -
Flags: review?(clokep)
Comment 21•10 years ago
|
||
*** Original post on bio 1683 at 2012-09-04 03:27:07 UTC *** Comment on attachment 8353649 [details] [diff] [review] (bio-attmnt 1891) Patch >diff --git a/chrome/instantbird/content/instantbird/conversation.xml b/chrome/instantbird/content/instantbird/conversation.xml >@@ -188,26 +188,25 @@ > if (nickActiveTimespan > 0) { >- let msgAge = Date.now() / 1000 - aMsg.time; >- let waitBeforeInactive = nickActiveTimespan - msgAge; >- if (waitBeforeInactive > 0) { >+ let expireTime = (aMsg.time + nickActiveTimespan) * 1000; >+ let waitBeforeInactive = expireTime - Date.now(); >+ if (Math.floor(waitBeforeInactive / 1000) > 0) { Why is this floor(waitBeforeInactive / 1000), are you just trying to optimize the case to not create a timer if it's < 1 second? Besides that, I think it's OK. But I'll try it out after you answer my question. :)
Assignee | ||
Comment 22•10 years ago
|
||
*** Original post on bio 1683 at 2012-09-04 09:09:41 UTC *** (In reply to comment #14) > Why is this floor(waitBeforeInactive / 1000), are you just trying to optimize > the case to not create a timer if it's < 1 second? Yes, we want to avoid blinking nicks ;)
Comment 23•10 years ago
|
||
Comment on attachment 8353649 [details] [diff] [review] Patch *** Original change on bio 1683 attmnt 1891 at 2012-09-05 03:29:56 UTC *** Can we just get a comment here saying that we want to avoid blinking of nicks? >+ if (Math.floor(waitBeforeInactive / 1000) > 0) { r+ with that change, but I'd like flo to look at this too.
Attachment #8353649 -
Flags: review?(clokep) → review+
Updated•10 years ago
|
Attachment #8353649 -
Flags: review?(florian)
Assignee | ||
Comment 24•10 years ago
|
||
*** Original post on bio 1683 as attmnt 1901 at 2012-09-05 20:20:00 UTC *** Add comment as requested.
Attachment #8353659 -
Flags: review?(florian)
Assignee | ||
Comment 25•10 years ago
|
||
Comment on attachment 8353649 [details] [diff] [review] Patch *** Original change on bio 1683 attmnt 1891 at 2012-09-05 20:20:56 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353649 -
Attachment is obsolete: true
Attachment #8353649 -
Flags: review?(florian)
Comment 26•10 years ago
|
||
Comment on attachment 8353659 [details] [diff] [review] Patch *** Original change on bio 1683 attmnt 1901 at 2012-09-23 22:51:48 UTC *** >diff --git a/chrome/instantbird/content/instantbird/conversation.xml b/chrome/instantbird/content/instantbird/conversation.xml > if (nickActiveTimespan > 0) { >- let msgAge = Date.now() / 1000 - aMsg.time; >- let waitBeforeInactive = nickActiveTimespan - msgAge; >- if (waitBeforeInactive > 0) { >+ let expireTime = (aMsg.time + nickActiveTimespan) * 1000; It's not obvious to me what "expireTime" means. (When reading the variable name; when reading the code, after a while it becomes clear of course.) >+ buddy.activeTimer = setTimeout(this._inactivateBuddyFunction(buddy), >+ waitBeforeInactive); Should we cancel all these timers when the conversation binding is destroyed? (Wouldn't the timer keep it alive and in memory otherwise? Or cause JS errors when they are executed if the nodes no longer exist?) >+ buddy.expireTime = expireTime; I would suggest storing buddy.lastMessageTime instead of the time when the buddy should no longer be marked as active. I think this information is more likely to be useful to potential add-ons. >+ <!-- Returns a function that resets a buddy to inactive --> >+ <method name="_inactivateBuddyFunction"> >+ <parameter name="aBuddy"/> >+ <body> >+ <![CDATA[ >+ return function() { >+ delete aBuddy.activeTimer; >+ aBuddy.setAttribute("inactive", "true"); >+ } >+ ]]> >+ </body> >+ </method> This is complicated. Any reason why you can't just use a regular method, and pass aBuddy as a parameter? (using the setTimeout(function, time, param1, param2, ...) syntax) > item.color = color; >+ if (this._partedActiveBuddies[name]) { If name is "hasOwnProperty" you are in trouble, because this test will always return true, and you will have a JS error later. > <method name="removeBuddy"> > <parameter name="aName"/> > <body> > <![CDATA[ > if (!this._hasBuddy(aName)) > throw "Cannot remove a buddy that was not in the room"; > var item = this.buddies[aName]; >- if (item.activeTimer) >+ if (item.activeTimer) { >+ // If the buddy is absent for less than this timespan (in ms), >+ // it will retain its active participant status on rejoining. >+ const kRestoreActiveTimespan = 300000; > clearTimeout(item.activeTimer); >+ this._partedActiveBuddies[aName] = { >+ expireTime: item.expireTime, I would suggest storing lastMessageTime and partTime instead. I think it's more valuable for add-on developers. >+ timer: setTimeout(function() { >+ delete this._partedActiveBuddies[aName]; >+ }.bind(this), kRestoreActiveTimespan) I dislike these timers (and like for the other timers, I wonder if we would have to cancel them when destroying the binding). I saw your point in a previous comment about not wanting that list to grow very large for a busy channel where the user idles for days, but I don't think it would use an awful lot of memory to keep them. How much memory would it take to store 10k nicks here? Probably not much compared to the messages in the conversation. I plan to integrate Show Nick by default relatively soon, and I would like it to be able to display the parted nicks too (although they probably wouldn't be colored, just displayed with a gray background), so I think having a list of parted nicks around would be quite useful. I would suggest having this._partedBuddies instead of this._partedActiveBuddies, and stop using timers to clean-up the list.
Attachment #8353659 -
Flags: review?(florian) → review-
Assignee | ||
Comment 27•10 years ago
|
||
*** Original post on bio 1683 as attmnt 1928 at 2012-09-28 12:02:00 UTC ***
>Any reason why you can't just use a regular method, and pass aBuddy as a
>parameter? (using the setTimeout(function, time, param1, param2, ...) syntax)
The reason for that method was to avoid duplication. I've done it slightly differently in this patch.
Attachment #8353684 -
Flags: review?(florian)
Assignee | ||
Comment 28•10 years ago
|
||
Comment on attachment 8353659 [details] [diff] [review] Patch *** Original change on bio 1683 attmnt 1901 at 2012-09-28 12:02:27 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353659 -
Attachment is obsolete: true
Assignee | ||
Comment 29•10 years ago
|
||
*** Original post on bio 1683 at 2012-09-28 12:06:01 UTC ***
>I would suggest having this._partedBuddies instead of this._partedActiveBuddies
If we're going to "support" add-ons using this, should it really be this.partedBuddies without the underscore?
Comment 30•10 years ago
|
||
*** Original post on bio 1683 at 2012-09-28 13:29:41 UTC *** Comment on attachment 8353684 [details] [diff] [review] (bio-attmnt 1928) Patch without additional timers >diff --git a/chrome/instantbird/content/instantbird/conversation.xml b/chrome/instantbird/content/instantbird/conversation.xml >+ aBuddy.activeTimer = setTimeout(function() { >+ delete aBuddy.activeTimer; >+ aBuddy.setAttribute("inactive", "true"); >+ }, waitBeforeInactive); Drive-by nit: the } should align with aBuddy.
Comment 31•10 years ago
|
||
Comment on attachment 8353684 [details] [diff] [review] Patch without additional timers *** Original change on bio 1683 attmnt 1928 at 2012-10-02 21:14:34 UTC *** >diff --git a/chrome/instantbird/content/instantbird/conversation.xml b/chrome/instantbird/content/instantbird/conversation.xml >index cdc4918..d369c76 100644 >--- a/chrome/instantbird/content/instantbird/conversation.xml >+++ b/chrome/instantbird/content/instantbird/conversation.xml >@@ -92,16 +92,21 @@ > if (this._conv) > this._forgetConv(); > > if ("MessageFormat" in window) { > let textbox = this.editor; > MessageFormat.unregisterTextbox(textbox); > TextboxSpellChecker.unregisterTextbox(textbox); > } >+ >+ Object.keys(this.buddies).forEach(function(name) { >+ if (this.buddies[name].activeTimer) >+ clearTimeout(this.buddies[name].activeTimer); >+ }, this); - It seems this block would need to be executed only for chat conversations, otherwise this.buddies doesn't exist. - Why the complicated Object.keys(...).forEach logic? It seems you never use the 'name' variable anyway. Why not: for each (let chatBuddy in this.buddies) { if (chatBuddy.activeTimer) clearTimeout(chatBuddy.activeTimer); } - If there was a good reason for that complicated syntax, I think you should only indent the content of the function by 2 spaces, and align the } on the "O" of "Object", like clokep said in comment 21. >+ <method name="_activateBuddy"> >+ aBuddy.activeTimer = setTimeout(function() { >+ delete aBuddy.activeTimer; >+ aBuddy.setAttribute("inactive", "true"); >+ }, waitBeforeInactive); You can reduce the indentation on these last 3 lines. > <!-- Add a buddy in the visible list of participants --> > <method name="addBuddy"> >+ item.setAttribute("inactive", "true"); >+ if (Object.prototype.hasOwnProperty.call(this._partedBuddies, name)) { >+ let parted = this._partedBuddies[name]; >+ // If the buddy was absent for less than this timespan (in ms), >+ // it retains its active participant status on rejoining. >+ const kRestoreActiveTimespan = 300000; >+ if (parted.lastMsgTime && >+ (parted.partTime - Date.now()) < kRestoreActiveTimespan) Isn't (parted.partTime - Date.now()) always a negative value? Looks OK otherwise. If you want to change this._partedBuddies to this.partedBuddies (comment 20), that's OK with me.
Attachment #8353684 -
Flags: review?(florian) → review-
Assignee | ||
Comment 32•10 years ago
|
||
*** Original post on bio 1683 as attmnt 1938 at 2012-10-07 17:45:00 UTC *** >It seems this block would need to be executed only for chat conversations, >otherwise this.buddies doesn't exist. Good catch, I thought it was always initialized, but it's (rightly) not. - Why the complicated Object.keys(...).forEach logic? There was more code there in an earlier, more complicated version of the patch. >>+ aBuddy.activeTimer = setTimeout(function() { >You can reduce the indentation on these last 3 lines. I'm never sure of the indentation after setTimeout... >Isn't (parted.partTime - Date.now()) always a negative value? Absolutely. Thanks!
Attachment #8353694 -
Flags: review?(florian)
Assignee | ||
Comment 33•10 years ago
|
||
Comment on attachment 8353684 [details] [diff] [review] Patch without additional timers *** Original change on bio 1683 attmnt 1928 at 2012-10-07 17:45:43 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353684 -
Attachment is obsolete: true
Comment 34•10 years ago
|
||
Comment on attachment 8353694 [details] [diff] [review] Patch *** Original change on bio 1683 attmnt 1938 at 2012-10-07 17:52:42 UTC *** Thanks!
Attachment #8353694 -
Flags: review?(florian) → review+
Comment 35•10 years ago
|
||
*** Original post on bio 1683 at 2012-10-07 23:03:32 UTC *** Checked in as http://hg.instantbird.org/instantbird/rev/291416fe603d This should be great to try out! :)
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.3
You need to log in
before you can comment on or make changes to this bug.
Description
•