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)
Tracking
(Not tracked)
RESOLVED
FIXED
1.2
People
(Reporter: aleth, Assigned: aleth)
References
Details
Attachments
(1 file, 3 obsolete files)
9.07 KB,
patch
|
florian
:
review+
|
Details | Diff | Splinter Review |
*** Original post on bio 1385 at 2012-04-19 17:17:00 UTC *** *** Due to BzAPI limitations, the initial description is in comment 1 ***
Assignee | ||
Comment 1•10 years ago
|
||
*** 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)
Assignee | ||
Comment 2•10 years ago
|
||
*** 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)
Assignee | ||
Comment 3•10 years ago
|
||
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 4•10 years ago
|
||
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-
Comment 5•10 years ago
|
||
*** 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.
Assignee | ||
Comment 6•10 years ago
|
||
*** 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)
Assignee | ||
Comment 7•10 years ago
|
||
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 8•10 years ago
|
||
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-
Assignee | ||
Comment 9•10 years ago
|
||
*** 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)
Assignee | ||
Comment 10•10 years ago
|
||
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 11•10 years ago
|
||
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+
Assignee | ||
Comment 12•10 years ago
|
||
*** 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
Assignee | ||
Updated•10 years ago
|
Target Milestone: --- → 1.2
You need to log in
before you can comment on or make changes to this bug.
Description
•