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)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aleth, Assigned: aleth)

References

Details

Attachments

(1 file, 8 obsolete files)

*** 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.
*** 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
Attached patch WIP (obsolete) — Splinter Review
*** 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 ***
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)
*** 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?
*** 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 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+
Assignee: nobody → aleth
Status: NEW → ASSIGNED
*** 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.
Attached patch Patch (obsolete) — Splinter Review
*** 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)
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
Attached patch Patch with lazy color (obsolete) — Splinter Review
*** 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)
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 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-
Attached patch Patch (obsolete) — Splinter Review
*** 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)
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
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)
Attached patch Patch (obsolete) — Splinter Review
*** 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)
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
*** 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).
Attached patch Patch (obsolete) — Splinter Review
*** 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)
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)
*** 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. :)
*** 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 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+
Attachment #8353649 - Flags: review?(florian)
Attached patch Patch (obsolete) — Splinter Review
*** 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)
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 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-
Attached patch Patch without additional timers (obsolete) — Splinter Review
*** 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)
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
*** 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?
*** 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 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-
Attached patch PatchSplinter Review
*** 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)
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 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+
*** 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
Depends on: 955166
You need to log in before you can comment on or make changes to this bug.