[Tab Complete] Does not cycle through inactive nicks

RESOLVED FIXED in 1.2

Status

Instantbird
Conversation
--
enhancement
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: clokep, Assigned: aleth)

Tracking

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

4 years ago
*** Original post on bio 1394 at 2012-04-24 12:14:00 UTC ***

This might be WONTFIX, but I found it strange that when there are active nicks, you can ONLY cycle through active nicks. aleth said that this is to cut down on the number of nicks taken into account for the partial completion and to reduce the cycle length for a channel with many participants. I dislike it because it limits the nicks you can complete to. :(

I think a good middle ground would be to prioritize the active nicks, i.e. put all the active nicks at the front of the completion list, followed by the inactive nicks. I.e. nick1, nick2, nick3 (where nick1 and nick3 are active):
"ni"<tab> --> "nick"<tab> --> "nick1"<tab> --> "nick3"<tab> --> "nick2"<tab> --> "nick1"
(Assignee)

Comment 1

4 years ago
Created attachment 8353146 [details] [diff] [review]
Patch

*** Original post on bio 1394 as attmnt 1393 at 2012-04-24 19:56:00 UTC ***

Turns out it is actually a regression as far as partial completions are concerned to restrict to active nicks - I had forgotten.

Thanks for the good idea of sorting the active nicks to the front - this won't block the others.
Attachment #8353146 - Flags: review?(clokep)
(Assignee)

Updated

4 years ago
Assignee: nobody → aleth
Status: NEW → ASSIGNED
(Assignee)

Comment 2

4 years ago
Created attachment 8353149 [details] [diff] [review]
Patch

*** Original post on bio 1394 as attmnt 1396 at 2012-04-24 21:01:00 UTC ***

Preemptively unbitrotted against bug 954833 (bio 1398).
Attachment #8353149 - Flags: review?(clokep)
(Assignee)

Comment 3

4 years ago
Comment on attachment 8353146 [details] [diff] [review]
Patch

*** Original change on bio 1394 attmnt 1393 at 2012-04-24 21:01:12 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353146 - Attachment is obsolete: true
Attachment #8353146 - Flags: review?(clokep)
(Assignee)

Updated

4 years ago
Depends on: 954833
(Assignee)

Comment 4

4 years ago
Created attachment 8353150 [details] [diff] [review]
Patch

*** Original post on bio 1394 as attmnt 1397 at 2012-04-24 21:03:00 UTC ***

Fix comment.
Attachment #8353150 - Flags: review?(clokep)
(Assignee)

Comment 5

4 years ago
Comment on attachment 8353149 [details] [diff] [review]
Patch

*** Original change on bio 1394 attmnt 1396 at 2012-04-24 21:03:49 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353149 - Attachment is obsolete: true
Attachment #8353149 - Flags: review?(clokep)
(Assignee)

Comment 6

4 years ago
Created attachment 8353152 [details] [diff] [review]
Patch

*** Original post on bio 1394 as attmnt 1399 at 2012-04-24 21:22:00 UTC ***

Remove single space. :-/
Attachment #8353152 - Flags: review?(clokep)
(Assignee)

Comment 7

4 years ago
Comment on attachment 8353150 [details] [diff] [review]
Patch

*** Original change on bio 1394 attmnt 1397 at 2012-04-24 21:22:23 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353150 - Attachment is obsolete: true
Attachment #8353150 - Flags: review?(clokep)
(Reporter)

Comment 8

4 years ago
Comment on attachment 8353152 [details] [diff] [review]
Patch

*** Original change on bio 1394 attmnt 1399 at 2012-04-24 23:23:50 UTC ***

This works and looks OK. I'd prefer flo to look over this as well though.
Attachment #8353152 - Flags: review?(clokep) → review+
(Reporter)

Updated

4 years ago
Attachment #8353152 - Flags: review?
(Reporter)

Comment 9

4 years ago
Comment on attachment 8353152 [details] [diff] [review]
Patch

*** Original change on bio 1394 attmnt 1399 at 2012-04-25 11:57:44 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353152 - Flags: review? → review?(florian)
Comment on attachment 8353152 [details] [diff] [review]
Patch

*** Original change on bio 1394 attmnt 1399 at 2012-04-25 22:09:47 UTC ***

>--- conversation.xml	2012-04-24 22:57:14.000000000 +0200
>+++ conversation.xml	2012-04-24 23:02:38.000000000 +0200

>+          let firstCompletion = this._completions[0];
>+          let lastCompletion = this._completions.slice(-1)[0];

It took me a long time to understand what we are doing with these variables, as they aren't the first and last completions that will be exposed to the user. I think we should add a comment above them explaining what they are. Aleth agreed to this wording I proposed over IRC:

// Save now the first and last completions in alphabetical order, as we will need them to find a common prefix. However they may not be the first and last completions in the list of completions actually exposed to the user, as if there are active nicks they will be moved to the beginning of the list.

>+              activeCompletions.forEach(function(c, i, arr) {

The i and arr parameters aren't used so we can omit them here.

r=me with these 2 nits fixed (and I'm fixing them before pushing the patch, no need for a new attachment).
Attachment #8353152 - Flags: review?(florian) → review+
(Reporter)

Comment 11

4 years ago
*** Original post on bio 1394 at 2012-04-26 00:13:58 UTC ***

Checked in as http://hg.instantbird.org/instantbird/rev/af2a940b0f9e
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.2
You need to log in before you can comment on or make changes to this bug.