Closed Bug 955062 Opened 10 years ago Closed 10 years ago

Mark participants as inactive after XXX time

Categories

(Instantbird Graveyard :: Conversation, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: clokep, Assigned: aleth)

Details

Attachments

(1 file, 3 obsolete files)

*** Original post on bio 1632 at 2012-08-09 12:00:00 UTC ***

(In reply to bug 954545 (bio 1112) comment #0)
> Possible improvements/changes might include
> - set nick color back to inactive after a certain timeout and/or if the
> participant sets their status to 'away' (not sure if this info is pushed to IB
> though)

I think a 5 minute timeout of "inactivity" is probably enough time to consider someone now there anymore. What do we think?
*** Original post on bio 1632 at 2012-08-09 12:04:37 UTC ***

5 minutes seems far too short for me. That would be "currently talking" (for which you don't need the colours as it's obvious from the last few messages) as opposed to "recently active". I'd suggest something more like 30 minutes or even longer.
*** Original post on bio 1632 at 2012-08-09 12:19:24 UTC ***

Florian suggested on IRC 1 - 2 hours, not 5 minutes...
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1632 as attmnt 1802 at 2012-08-13 21:22:00 UTC ***

Went for an hour to start with. We can see how that feels in the nightlies...
Attachment #8353561 - Flags: review?(clokep)
Assignee: nobody → aleth
Status: NEW → ASSIGNED
Comment on attachment 8353561 [details] [diff] [review]
Patch

*** Original change on bio 1632 attmnt 1802 at 2012-08-14 01:50:39 UTC ***

(In reply to comment #3)
> Created attachment 8353561 [details] [diff] [review] (bio-attmnt 1802) [details]
> Patch
> 
> Went for an hour to start with. We can see how that feels in the nightlies...

So this looks reasonable, I'm assuming we check the message time because of returning a conversation from hold? (I'd probably want a more extensive comment.) I'm concerned that this will make an (insane) amount of timers so I'd really like flo to look at this closely.

>-        // Ugly hack... :(
Why are we removing this comment?


Nits:
>+            let msgAge = Date.now()/1000 - aMsg.time;
Spaces around the /.

>+            let waitBeforeInactive = returnToInactiveDelay - msgAge;
>+            if (waitBeforeInactive > 0) {
>+              if (buddy.activeTimer)
>+                clearTimeout(buddy.activeTimer);
>+              buddy.activeTimer = setTimeout(function() {
>+                delete buddy.activeTimer;
>+                buddy.setAttribute("inactive", "true");
>+                }, waitBeforeInactive * 1000)
This } should de-dent + it's missing a semi-colon at the end.
Attachment #8353561 - Flags: review?(clokep) → review-
*** Original post on bio 1632 at 2012-08-14 09:34:47 UTC ***

> So this looks reasonable, I'm assuming we check the message time because of
> returning a conversation from hold?

Sure.

> (I'd probably want a more extensive
> comment.) I'm concerned that this will make an (insane) amount of timers so I'd
> really like flo to look at this closely.

I was wondering about that... it only makes timers for active participants, so not _that_ many (more like dozens than hundreds), but I don't know what the overhead is for long-term timers. The alternative is a periodic timer (every minute or so) which then checks through a list of expiry times each time it fires. Unless the timer code is not very optimized, that would boil down to the same thing ;)

> >-        // Ugly hack... :(
> Why are we removing this comment?

Because it's no longer clear what it refers to. The colour handling, I guess...
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1632 as attmnt 1804 at 2012-08-14 22:53:00 UTC ***

Nit and comment fixes.

So I checked and there is indeed no problem with adding this number of long-term timers. The overhead is limited because internally there is only one C++ timer set to the first timeout to occur. See the mMonitor.Wait(nearest_timer_to_expire) in http://dxr.lanedo.com/mozilla-central/xpcom/threads/TimerThread.cpp.html#l217. So we would be duplicating this code or even adding to it (with the regular-interval version) if we tried to be clever.
Attachment #8353563 - Flags: review?(clokep)
Comment on attachment 8353561 [details] [diff] [review]
Patch

*** Original change on bio 1632 attmnt 1802 at 2012-08-14 22:53:36 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353561 - Attachment is obsolete: true
Comment on attachment 8353563 [details] [diff] [review]
Patch

*** Original change on bio 1632 attmnt 1804 at 2012-08-15 00:37:36 UTC ***

This looks good, I just checked it out (with a bit shorter of a time period...) and it seems to work fine. :)
Attachment #8353563 - Flags: review?(clokep) → review+
Whiteboard: [checkin-needed]
*** Original post on bio 1632 at 2012-08-21 13:25:16 UTC ***

Should "const returnToInactiveDelay = 3600;" be a hidden pref, with a special value (either 0 or -1) that would just revert to the current behavior?
(It's a real question, not an r- comment hiding itself as a question.)
*** Original post on bio 1632 at 2012-08-21 13:30:47 UTC ***

(In reply to comment #8)
> Should "const returnToInactiveDelay = 3600;" be a hidden pref, with a special
> value (either 0 or -1) that would just revert to the current behavior?
> (It's a real question, not an r- comment hiding itself as a question.)

I think a hidden pref is reasonable. (I would suggest <= 0 be treated as the current behavior, in that case.)
Attached patch Patch with pref (obsolete) — Splinter Review
*** Original post on bio 1632 as attmnt 1815 at 2012-08-21 15:25:00 UTC ***

Good idea.
Attachment #8353574 - Flags: review?(clokep)
Comment on attachment 8353563 [details] [diff] [review]
Patch

*** Original change on bio 1632 attmnt 1804 at 2012-08-21 15:25:16 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353563 - Attachment is obsolete: true
Whiteboard: [checkin-needed]
Comment on attachment 8353574 [details] [diff] [review]
Patch with pref

*** Original change on bio 1632 attmnt 1815 at 2012-08-26 20:14: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
>+            // the nickActiveTimespan.
Personally I don't like "the" here. But I'm only even mentioning this because I'm commenting somewhere else...

>+              if (waitBeforeInactive > 0) {
>+                // Set timer so the nick returns to inactive
>+                // the specified timespan after the message time.
I think this wants to be "Set a timer so the nick will return to inactive after the specified timespan from when the last message was received." Or something like that, right now the comment makes no sense to me.

The code looks fine though!
Attachment #8353574 - Flags: review?(clokep) → review-
Attached patch PatchSplinter Review
*** Original post on bio 1632 as attmnt 1850 at 2012-08-26 20:22:00 UTC ***

Amended comments.
Attachment #8353609 - Flags: review?(clokep)
Comment on attachment 8353574 [details] [diff] [review]
Patch with pref

*** Original change on bio 1632 attmnt 1815 at 2012-08-26 20:22:14 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353574 - Attachment is obsolete: true
Comment on attachment 8353609 [details] [diff] [review]
Patch

*** Original change on bio 1632 attmnt 1850 at 2012-08-26 20:42:10 UTC ***

That sounds better! Thanks for fixing this! I'm excited to try it out. :)
Attachment #8353609 - Flags: review?(clokep) → review+
Whiteboard: [checkin-needed]
*** Original post on bio 1632 at 2012-08-28 00:51:26 UTC ***

Checked in as http://hg.instantbird.org/instantbird/rev/c09c70153aac! Thanks.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [checkin-needed]
Target Milestone: --- → 1.3
You need to log in before you can comment on or make changes to this bug.