Closed Bug 954820 Opened 10 years ago Closed 10 years ago

[Tab complete] Smart Undo and Cycle through alternative completions

Categories

(Instantbird Graveyard :: Conversation, defect)

x86
All
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aleth, Assigned: aleth)

References

Details

Attachments

(1 file, 3 obsolete files)

*** Original post on bio 1385 at 2012-04-19 17:17:00 UTC ***

*** Due to BzAPI limitations, the initial description is in comment 1 ***
Blocks: 954805
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1385 as attmnt 1372 at 2012-04-19 17:17:00 UTC ***

- Pressing TAB multiple times now cycles through the alternative completions.
- This means the existence of a preferred nick (due to a ping) no longer blocks alternative completions.
- If some of the possible completions are active nicks, restrict the alternative completions to those too.
- BACKSPACE after tab complete acts as undo for the completion.
- BACKSPACE after a multiple tab completion (nicks separated by commas) will first undo the ": " -> ", " as suggested in bug 954805 (bio 1371).
Attachment #8353125 - Flags: review?(clokep)
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1385 as attmnt 1373 at 2012-04-19 18:23:00 UTC ***

Before I forget, Mook mentioned the constant for the backslash key.
Attachment #8353126 - Flags: review?(florian)
Comment on attachment 8353125 [details] [diff] [review]
Patch

*** Original change on bio 1385 attmnt 1372 at 2012-04-19 18:23:51 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353125 - Attachment is obsolete: true
Attachment #8353125 - Flags: review?(clokep)
Comment on attachment 8353126 [details] [diff] [review]
Patch

*** Original change on bio 1385 attmnt 1373 at 2012-04-19 21:42:48 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353126 - Flags: review?(florian) → review-
*** Original post on bio 1385 at 2012-04-19 21:42:48 UTC ***

Comment on attachment 8353126 [details] [diff] [review] (bio-attmnt 1373)
Patch

Review comments given over IRC:

22:20:21 - flo: aleth: the intended behavior when there are multiple completions is still to complete the common prefix, and then completing the first solution is when pressing tab a second time, right?
22:20:37 - aleth: Yes
22:21:01 - aleth: (unless there is a preferred nick)
22:22:51 - flo: "if (pos > 2 && text.substring(pos - 2, pos) == ": ") {" I understand that 2 is to avoid pos-2 being negative, but don't you need way more characters to have a chance of undoing the : ->, change?
22:23:08 - flo: Isn't 6 ("a, b: ") the minimum?
22:23:48 - aleth: Yes, I could change that. But the code won't fail as is.
22:28:47 - flo: it seems |preceding[preceding.length - 1]| could be just |preceding.pop()|
22:28:58 - flo: as you don't seem to be using that array again
22:29:22 - aleth: Nice :) I didn't know pop.
22:30:41 - clokep_work: (And if you ever want the last element...I like preceding.slice(-1).)
22:31:01 - flo: the code changing ": " to " " in each item of this._altCompletions looks like it really wants to be a call to .map
22:31:28 - flo: clokep_work: pop is the last element
22:31:43 - aleth: flo: Yes that would be better
22:32:34 - clokep_work: flo: Yes, of course.
22:32:49 - clokep_work: Sorry. I meant "if you ever want the last element but don't want to modify the array".
22:36:37 - flo: aleth: |completions = matchingCompletions.sort();| why the .sort?
22:36:57 - aleth: It's just moved from further down in the existing code.
22:37:17 - flo: ok
22:37:31 - flo: |completions.slice(0);| is to copy the array?
22:37:47 - aleth: Moved so that the alternative completions end up sorted too.
22:37:47 - aleth: Yes.
22:38:29 - flo: I think I prefer |completions.concat();| for that (no particular reason, except maybe that when there's no argument I don't have to lookup the method to know the meaning of its arguments)
22:38:40 - aleth: np
22:41:48 - flo: https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Array/filter takes a second parameter for the value of this
22:42:07 - aleth: oh, so I don't need bind? how convenient
22:42:08 - flo: so you don't need the confusing .bind call
22:44:05 - flo: there's another array copy at |activeCompletions.slice(0);|
22:45:07 - flo: why do you need a copy by the way? The activeCompletions array isn't touched after that line
22:45:28 - aleth: but it's scoped to the if block
22:46:06 - aleth: Does that not cause problems?
22:46:20 - flo: leaving the scope will remove the reference held by the activeCompletions variable
22:46:31 - flo: but if you have stored it somewhere else, there's still a reference
22:46:41 - flo: so no reason for it to be garbage collected
22:47:16 - aleth: OK. I wasn't sure about that.
22:47:45 - flo: I see what you mean though; that would be right in C
22:49:50 - clokep_work: Bye! :)
22:49:56 - clokep_work has left the room (Quit: Frisbee!).
22:50:50 - flo: you initialize preferredNick to -1 so later I would test != -1 rather than >= 0
22:54:49 - flo: the code after |// Add suffix to alternative completions.| should use a |let suffix = isFirstWord ? firstWordSuffix : " ";| variable rather than testing this in the loop
22:54:58 - flo: and it sounds like another good candidate for a .map call
22:55:31 - aleth: Yes, it's basically the same code.
22:57:54 - flo: when there's a preferredNick, wouldn't it make more sense once you have found its index in the this._altCompletions array to remove it from that index, and .unshift it?
22:58:19 - flo: then the next tab press would complete what's the first completion in alphabetical order
22:58:31 - aleth: No, so you can still cycle through the alternatives later.
22:59:09 - flo: uh?
22:59:16 - aleth: I didn't want to change the ordering.
22:59:28 - flo: why?
22:59:57 - aleth: Imagine there are 3 active nicks beginning with m and mic has pinged you. Then you will get mic and further tab presses cycle through the 3 possibilities.
23:01:08 - flo: I still don't understand
23:01:08 - aleth: Since it's cyclic, why break the alphabetic order of the set?
23:01:31 - flo: to complete in alphabetical order rather than starting in the middle?
23:01:46 - aleth: You start at the preferred nick.
23:02:30 - flo: instead of |let completion = completions[0];| you would have |let completion = this._altCompletions[0];|
23:02:55 - flo: and you could drop the duplicated |(isFirstWord ? firstWordSuffix : " ")| which is already included in the string contained by this._altCompletions
23:04:05 - aleth: Sorry, are you trying to tidy the code, or to change the behaviour? I'm confused.
23:04:21 - flo: both at once
23:05:09 - aleth: Why would you want a different behaviour though?
23:05:21 - flo: if you have 3 nicks mia mib and mic, and mib has pinged you, I think the "mib mia mic" cycle makes as much sense as "mib mic mia"
23:06:25 - aleth: I don't really agree - as soon as you've pressed tab once you are cycling through a list that is no longer correctly ordered. Confusing.
23:08:14 - flo: ok... I'll pretend I'm convinced, as it's really not important :)
23:08:26 - aleth: It's not really ;)
23:08:30 - flo: you can still replace |(isFirstWord ? firstWordSuffix : " ")| by reusing the |suffix| variable
23:08:31 - aleth: For the tidying the code aspect, I guess it would be possible to cycle the array the correct number of times instead of what the current code does.
23:08:36 - aleth: Yes
23:08:42 - aleth: That's a good idea
23:12:31 - flo: "it would be possible to cycle the array the correct number of times instead of what the current code does." I don't understand this sentence
23:13:11 - flo: I've now finished reading the code
23:14:07 - flo: it generally seems quite good, although I have a feeling that if the completions and this._altCompletions arrays could be the same, things would be simplified (not sure if there's a really good reason for these 2 arrays to coexist or not)
23:15:14 - flo: also, _altCompletions doesn't seem a good name. I assume "alt" means 'alternative' here, which implies there are at least 2 possibilities. But you have a case where the length of that array is 1. The array seems to contain possible completions rather than alternative completions
23:15:46 - aleth: True, I should change that.
23:16:24 - aleth: flo: It's possible I can make the two arrays coincide, it was easier while coding not to.
23:18:13 - aleth: I'll take another look later.
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 1385 as attmnt 1374 at 2012-04-19 23:37:00 UTC ***

Implemented review comments and tidied up a bit.
Attachment #8353127 - Flags: review?(florian)
Comment on attachment 8353126 [details] [diff] [review]
Patch

*** Original change on bio 1385 attmnt 1373 at 2012-04-19 23:37:21 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353126 - Attachment is obsolete: true
Comment on attachment 8353127 [details] [diff] [review]
Patch

*** Original change on bio 1385 attmnt 1374 at 2012-04-20 09:59:37 UTC ***

r- but only because of coding style comments, this looks quite good now! (Note: I still haven't tested it.)

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

>+              if (this._completions[0].slice(-2) == ": ")
>+                this._completions = this._completions.map(
>+                  function(c) c.slice(0, -2) + " ");

Coding style: {} as the instruction is on more than 1 line.

If that doesn't make a >80 columns line, I would prefer the line break after the = rather than after the (.

>+            this.addString(this._completions[this._completionsIndex++]);
>+            if (this._completionsIndex == this._completions.length)
>+              this._completionsIndex = 0;

Would this be more readable?
           this.addString(this._completions[this._completionsIndex]);
           this._completionsIndex = (this._completionsIndex + 1) % this._completions.length;
(may need a line break after the =)


>+            let activeCompletions = this._completions.filter(
>+              function(c) this._hasBuddy(c)
>+                          && !this.buddies[c].hasAttribute("inactive"), this);

Coding style: The && at the end of the line rather than at the beginning.

Maybe keep "function(c)" on the same line as "filter("?
Attachment #8353127 - Flags: review?(florian) → review-
Attached patch PatchSplinter Review
*** Original post on bio 1385 as attmnt 1375 at 2012-04-20 10:14:00 UTC ***

% is a good idea for this!
Attachment #8353128 - Flags: review?(florian)
Comment on attachment 8353127 [details] [diff] [review]
Patch

*** Original change on bio 1385 attmnt 1374 at 2012-04-20 10:14:46 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353127 - Attachment is obsolete: true
Comment on attachment 8353128 [details] [diff] [review]
Patch

*** Original change on bio 1385 attmnt 1375 at 2012-04-23 22:16:15 UTC ***

r=me, let's try this on nightlies! :-)
Attachment #8353128 - Flags: review?(florian) → review+
*** Original post on bio 1385 at 2012-04-23 23:00:25 UTC ***

Checked in as http://hg.instantbird.org/instantbird/rev/9196673434de
Status: NEW → 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.