Closed Bug 954829 Opened 10 years ago Closed 10 years ago

[Tab Complete] Does not cycle through inactive nicks

Categories

(Instantbird Graveyard :: Conversation, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: clokep, Assigned: aleth)

References

Details

Attachments

(1 file, 3 obsolete files)

*** 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"
Attached patch Patch (obsolete) — Splinter Review
*** 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: nobody → aleth
Status: NEW → ASSIGNED
Attached patch Patch (obsolete) — Splinter Review
*** 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)
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)
Depends on: 954833
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1394 as attmnt 1397 at 2012-04-24 21:03:00 UTC ***

Fix comment.
Attachment #8353150 - Flags: review?(clokep)
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)
Attached patch PatchSplinter Review
*** Original post on bio 1394 as attmnt 1399 at 2012-04-24 21:22:00 UTC ***

Remove single space. :-/
Attachment #8353152 - Flags: review?(clokep)
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)
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+
Attachment #8353152 - Flags: review?
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+
*** 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
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.2
You need to log in before you can comment on or make changes to this bug.