955581 - Allow tab completion of nicks which have left the room

Status: RESOLVED FIXED

(Instantbird Graveyard :: Conversation, enhancement)

Description

[aleth [:aleth]]

*** Original post on bio 2142 at 2013-08-30 11:34:00 UTC ***

At least for a short amount of time.

I often find myself completing to the wrong nick, because someone has temporarily or recently left the room. But would this change make things better or worse overall?

Comment 1

[Patrick Cloke [:clokep]]

*** Original post on bio 2142 at 2013-08-30 12:03:23 UTC ***

Seems like Florian has had this issue, I've also had this issue.

Comment 2

[Florian Quèze [:florian]]

*** Original post on bio 2142 at 2013-08-30 12:20:48 UTC ***

I had the issue too. I'm not sure if completing left nicks is the right fix though. (As I suspect I sometimes use tab completion to check if someone is around.)

But yeah, maybe for a short amount of time :-).

Comment 3

[aleth [:aleth]]

Attachment: Patch

*** Original post on bio 2142 as attmnt 2810 at 2013-08-31 17:26:00 UTC ***

We could try this...

Random thought: in the long run, it might be nice to recognise when a message is addressed to a parted nick, and memoserv it automatically.

Comment 4

[Florian Quèze [:florian]]

*** Original post on bio 2142 at 2013-09-01 21:56:17 UTC ***

(In reply to comment #3)

> Random thought: in the long run, it might be nice to recognise when a message
> is addressed to a parted nick, and memoserv it automatically.

This may be a little bit too magical (and half the time if you say something to someone just before he leaves and end up sending the message after he has left, it's not necessarily something worth showing the next day. Eg 'Good night!').

Could be a nice add-on though :).

Comment 5

[Florian Quèze [:florian]]

Comment on attachment 8354580 [details] [diff] [review]
Patch

*** Original change on bio 2142 attmnt 2810 at 2013-09-04 07:33:04 UTC ***

>diff --git a/instantbird/content/conversation.xml b/instantbird/content/conversation.xml

>+              // Add recently parted nicks.
>+              const kIncludeNickTimespan = 300000;
>+              for (let nick of Object.keys(this.partedBuddies))

Are the keys in this.partedBuddies already sorted by part time? I suspect they are; in which case you may want to consider a binary search, and you can concat a slice of the array, instead of pushing each recently parted nick.

>+                if (Date.now() - this.partedBuddies[nick].partTime < kIncludeNickTimespan)

What about putting |Date.now() - kIncludeNickTimespan| in a variable before the loop?


I'm not sure how important these performance concerns are as I'm not sure how many nicks we will have in the partedBuddies object, but I figure just after a netsplit it may be in the hundreds.

Comment 6

[aleth [:aleth]]

Attachment: Patch

*** Original post on bio 2142 as attmnt 2817 at 2013-09-04 17:37:00 UTC ***

>Are the keys in this.partedBuddies already sorted by part time? I suspect they
>are; in which case you may want to consider a binary search, and you can concat
>a slice of the array, instead of pushing each recently parted nick.

They appear to be sorted by part time, but do we really want this code to depend on the implementation interna of objects? Even looping over a thousand parted buddies should not take too long in this context I think.

Comment 7

[aleth [:aleth]]

Comment on attachment 8354580 [details] [diff] [review]
Patch

*** Original change on bio 2142 attmnt 2810 at 2013-09-04 17:37:21 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 8

[Patrick Cloke [:clokep]]

Comment on attachment 8354587 [details] [diff] [review]
Patch

*** Original change on bio 2142 attmnt 2817 at 2013-09-04 18:01:23 UTC ***

>diff --git a/instantbird/content/conversation.xml b/instantbird/content/conversation.xml
>@@ -634,16 +634,24 @@
>+
>+              // Add recently parted nicks.
>+              const kIncludeNickTimespan = 300000;
>+              let cutoffTime = Date.now() - kIncludeNickTimespan;
>+              for (let nick of Object.keys(this.partedBuddies))
>+                if (this.partedBuddies[nick].partTime > cutoffTime)
>+                  completions.push(nick);
Nit: { } around the if statement, we only drop the { } if the inner statement is a single line, not a single statement.

Comment 9

[Florian Quèze [:florian]]

*** Original post on bio 2142 at 2013-09-04 21:26:03 UTC ***

(In reply to comment #7)

> >+              for (let nick of Object.keys(this.partedBuddies))
> >+                if (this.partedBuddies[nick].partTime > cutoffTime)
> >+                  completions.push(nick);
> Nit: { } around the if statement, we only drop the { } if the inner statement
> is a single line, not a single statement.

I assume you meant 'for statement'.

(In reply to comment #6)

> >Are the keys in this.partedBuddies already sorted by part time? I suspect they
> >are; in which case you may want to consider a binary search, and you can concat
> >a slice of the array, instead of pushing each recently parted nick.
> 
> They appear to be sorted by part time, but do we really want this code to
> depend on the implementation interna of objects?

I would be really surprised if this 'implementation detail' changed without a lot of prior notices.

> Even looping over a thousand
> parted buddies should not take too long in this context I think.

A binary search may be an excessive complication as yes, it's just js objects without traversing xpconnect wrappers. But looping from the end to search for the oldest nick we are interested in seems reasonable to me.


Random thought: would we want to remove from the parted list nicks that have been there for too long?

Comment 10

[aleth [:aleth]]

Attachment: Patch

*** Original post on bio 2142 as attmnt 2824 at 2013-09-05 13:21:00 UTC ***

(In reply to comment #8)
> Random thought: would we want to remove from the parted list nicks that have
> been there for too long?

We'd need an interval timer that cleans up the list every hour or day or so. The same would apply to the show nick list. I suppose it might be a good idea if people are lurking on #ubuntu without restarting for weeks. Definitely a separate bug though...

Comment 11

[aleth [:aleth]]

Comment on attachment 8354587 [details] [diff] [review]
Patch

*** Original change on bio 2142 attmnt 2817 at 2013-09-05 13:21:30 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 12

[aleth [:aleth]]

Comment on attachment 8354594 [details] [diff] [review]
Patch

*** Original change on bio 2142 attmnt 2824 at 2013-09-05 13:28:18 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 13

[aleth [:aleth]]

Attachment: Patch

*** Original post on bio 2142 as attmnt 2825 at 2013-09-05 15:19:00 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 14

[aleth [:aleth]]

Comment on attachment 8354594 [details] [diff] [review]
Patch

*** Original change on bio 2142 attmnt 2824 at 2013-09-05 15:19:58 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 15

[aleth [:aleth]]

Attachment: Patch

*** Original post on bio 2142 as attmnt 2826 at 2013-09-05 15:42:00 UTC ***

for -> while.

Comment 16

[aleth [:aleth]]

Comment on attachment 8354595 [details] [diff] [review]
Patch

*** Original change on bio 2142 attmnt 2825 at 2013-09-05 15:42:43 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 17

[Florian Quèze [:florian]]

Comment on attachment 8354596 [details] [diff] [review]
Patch

*** Original change on bio 2142 attmnt 2826 at 2013-09-05 15:51:13 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 18

[Patrick Cloke [:clokep]]

*** Original post on bio 2142 at 2013-09-06 00:18:36 UTC ***

http://hg.instantbird.org/instantbird/rev/a17743c6cca1