Closed
Bug 954531
Opened 10 years ago
Closed 10 years ago
Tab complete is case sensitive
Categories
(Instantbird Graveyard :: Conversation, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
1.2
People
(Reporter: aleth, Assigned: aleth)
Details
Attachments
(1 file, 4 obsolete files)
1.99 KB,
patch
|
florian
:
review+
|
Details | Diff | Splinter Review |
*** Original post on bio 1097 at 2011-10-20 19:17:00 UTC *** and should not be, I think
Assignee | ||
Comment 1•10 years ago
|
||
*** Original post on bio 1097 as attmnt 923 at 2011-10-23 16:31:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352665 -
Flags: review?
Comment 2•10 years ago
|
||
Comment on attachment 8352665 [details] [diff] [review] Patch for conversation.xml *** Original change on bio 1097 attmnt 923 at 2011-10-23 16:35:49 UTC *** >--- conversation.xml 2011-10-23 16:49:31.000000000 +0200 >+++ conversation.xml 2011-10-23 18:26:29.000000000 +0200 >@@ -532,14 +535,14 @@ > } > > // Keep only the completions that share |word| as a prefix. >- completions = completions.filter(function(c) c.indexOf(word) == 0); >+ completions = completions.filter(function(c) c.toLowerCase().indexOf(word.toLowerCase())==0); Please keep the spaces around the ==. Also I wonder if toLocaleLowerCase should be used here? https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/String/toLocaleLowerCase > // Only one possible completion? Apply it! :-) > if (completions.length == 1) { >- this.addString(completions[0].substring(word.length) + >- (isFirstWord ? firstWordSuffix : " ")); >+ inputBox.selectionStart -= word.length; >+ this.addString(completions[0] + (isFirstWord ? firstWordSuffix : " ")); What is this change for?
Attachment #8352665 -
Flags: review? → review-
Assignee | ||
Comment 3•10 years ago
|
||
*** Original post on bio 1097 as attmnt 924 at 2011-10-23 16:46:00 UTC *** > Also I wonder if toLocaleLowerCase should > be used here? Wow, I didn't know that existed, but it makes sense of course. In this case it shouldn't make any difference however, as it's applied "on both sides of the equation". Should I change it anyway? > > // Only one possible completion? Apply it! :-) > > if (completions.length == 1) { > >- this.addString(completions[0].substring(word.length) + > >- (isFirstWord ? firstWordSuffix : " ")); > >+ inputBox.selectionStart -= word.length; > >+ this.addString(completions[0] + (isFirstWord ? firstWordSuffix : " ")); > What is this change for? To replace the (possibly all lowercase) first few letters you typed with the correct case from the nick, e.g. geeks -> GeekShadow.
Attachment #8352666 -
Flags: review?
Assignee | ||
Comment 4•10 years ago
|
||
Comment on attachment 8352665 [details] [diff] [review] Patch for conversation.xml *** Original change on bio 1097 attmnt 923 at 2011-10-23 16:46:35 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352665 -
Attachment is obsolete: true
Comment 5•10 years ago
|
||
*** Original post on bio 1097 at 2011-10-23 16:56:55 UTC *** (In reply to comment #3) > Created an attachment (id=924) [details] > Patch for conversation.xml > > > Also I wonder if toLocaleLowerCase should > > be used here? > > Wow, I didn't know that existed, but it makes sense of course. In this case it > shouldn't make any difference however, as it's applied "on both sides of the > equation". Should I change it anyway? Yeah, I guess that's right. (I misunderstood how toLocaleLowerCase was used slightly, so in this case it shouldn't really matter, you're right). > > > > // Only one possible completion? Apply it! :-) > > > if (completions.length == 1) { > > >- this.addString(completions[0].substring(word.length) + > > >- (isFirstWord ? firstWordSuffix : " ")); > > >+ inputBox.selectionStart -= word.length; > > >+ this.addString(completions[0] + (isFirstWord ? firstWordSuffix : " ")); > > What is this change for? > > To replace the (possibly all lowercase) first few letters you typed with the > correct case from the nick, e.g. geeks -> GeekShadow. Right, that makes sense. I'll test this in a minute.
Assignee: nobody → aleth
Status: NEW → ASSIGNED
Comment 6•10 years ago
|
||
*** Original post on bio 1097 at 2011-10-23 17:16:39 UTC *** (In reply to comment #0) > and should not be, I think I like it case sensitive :-(.
Comment 7•10 years ago
|
||
*** Original post on bio 1097 at 2011-10-23 18:00:45 UTC *** (In reply to comment #5) > (In reply to comment #0) > > and should not be, I think > > I like it case sensitive :-(. So, let me elaborate on this. What really frustrated me with the case insensitive behavior of Tab Complete is when I typed "Mi" and it couldn't complete to "Mic" because there was a "mi<something>" nick in the room. I'm not exactly sure of why you prefer the insensitive behavior, but I suspect it is because you sometimes don't know the case of the letters of the nick you are typing. If this is right, then maybe we can get the best of both behaviors by being case sensitive only if what's already been typed has a mixed case? (or has some upper case letters, so that we can catch the "single letter in upper case" situation? That latter solution wouldn't fix the case automatically when someone has accidentally enabled CAPS lock). I hope this helps :-).
Assignee | ||
Comment 8•10 years ago
|
||
*** Original post on bio 1097 as attmnt 925 at 2011-10-23 20:26:00 UTC *** I like that idea, it seems to make everyone happy :) How's this look?
Attachment #8352667 -
Flags: review?(florian)
Assignee | ||
Comment 9•10 years ago
|
||
Comment on attachment 8352666 [details] [diff] [review] Patch for conversation.xml *** Original change on bio 1097 attmnt 924 at 2011-10-23 20:26:04 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352666 -
Attachment is obsolete: true
Attachment #8352666 -
Flags: review?
Comment 10•10 years ago
|
||
Comment on attachment 8352667 [details] [diff] [review] Patch for conversation.xml *** Original change on bio 1097 attmnt 925 at 2011-10-25 14:55:57 UTC *** General comment: don't use tabs to indent (except in Makefiles and jar.mn files). >--- conversation.xml 2011-10-23 22:18:15.000000000 +0200 >+++ conversation.xml 2011-10-23 22:20:24.000000000 +0200 >@@ -532,29 +535,37 @@ > } > > // Keep only the completions that share |word| as a prefix. >- completions = completions.filter(function(c) c.indexOf(word) == 0); >+ // Be case insensitive only if |word| is entirely lower case Add a period at the end of the sentence. >+ if (word.toLocaleLowerCase() == word) >+ completions = completions.filter(function(c) c.toLocaleLowerCase().indexOf(word) == 0); >+ else >+ completions = completions.filter(function(c) c.indexOf(word) == 0); Put the filter function in a variable, and don't duplicate the completions = completions.filter(filter) code :-). > // Only one possible completion? Apply it! :-) > if (completions.length == 1) { >- this.addString(completions[0].substring(word.length) + >- (isFirstWord ? firstWordSuffix : " ")); >+ inputBox.selectionStart -= word.length; >+ this.addString(completions[0] + (isFirstWord ? firstWordSuffix : " ")); I can't figure out the effect of this change, can you explain? > // We have several possible completions, attempt to find a common prefix. [removed lines] >+ // Check if possible completions match across |word|, if not, give up >+ if (completions.every(function(c) c.substring(0,word.length) >+ == completions[0].substring(0,word.length))) { I don't understand this either. Maybe to explain give an example of a case where this test is needed?
Attachment #8352667 -
Flags: review?(florian) → review-
Comment 11•10 years ago
|
||
*** Original post on bio 1097 at 2011-10-25 15:09:04 UTC *** (In reply to comment #8) > > // Only one possible completion? Apply it! :-) > > if (completions.length == 1) { > >- this.addString(completions[0].substring(word.length) + > >- (isFirstWord ? firstWordSuffix : " ")); > >+ inputBox.selectionStart -= word.length; > >+ this.addString(completions[0] + (isFirstWord ? firstWordSuffix : " ")); > > I can't figure out the effect of this change, can you explain? I think I understand a bit better now. You probably want to add a comment there explaining that as the case of some letters that the user typed may not be the same in the completion, we need to replace what the user has typed. > > // We have several possible completions, attempt to find a common prefix. > [removed lines] > >+ // Check if possible completions match across |word|, if not, give up > >+ if (completions.every(function(c) c.substring(0,word.length) > >+ == completions[0].substring(0,word.length))) { > > I don't understand this either. Maybe to explain give an example of a case > where this test is needed? It seems you could have the same effect by just initializing the variable i to 0 instead of word.length. Also, you may want to fix the case of what the user has typed (so that if I have "Mic" and "Mic|web" in my nicklist, when typing mi<tab>, I get "Mic" rather than "mic" as a (partial) completion).
Assignee | ||
Comment 12•10 years ago
|
||
*** Original post on bio 1097 as attmnt 938 at 2011-10-25 20:01:00 UTC *** Thanks for your comments.
Attachment #8352680 -
Flags: review?(florian)
Assignee | ||
Comment 13•10 years ago
|
||
Comment on attachment 8352667 [details] [diff] [review] Patch for conversation.xml *** Original change on bio 1097 attmnt 925 at 2011-10-25 20:01:58 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352667 -
Attachment is obsolete: true
Comment 14•10 years ago
|
||
Comment on attachment 8352680 [details] [diff] [review] Patch for conversation.xml *** Original change on bio 1097 attmnt 938 at 2011-10-26 10:50:43 UTC *** Looks good! :-) (note: I haven't tested this yet.) The only details I would change are: >--- conversation.xml 2011-10-25 21:58:17.000000000 +0200 >+++ conversation.xml 2011-10-25 22:00:03.000000000 +0200 >+ var condition; var -> let. > // We have several possible completions, attempt to find a common prefix. >+ // Possible completions must match from the beginning. The added sentence here isn't helpful (a prefix is always from the beginning). Either remove it, or rephrase it to indicate that even though the completion may be case insensitive, a partial completion is only possible if the beginning of all possible completions is identical (including the case).
Attachment #8352680 -
Flags: review?(florian) → review+
Assignee | ||
Comment 15•10 years ago
|
||
*** Original post on bio 1097 as attmnt 945 at 2011-10-26 18:18:00 UTC *** (In reply to comment #11) > Looks good! :-) (note: I haven't tested this yet.) There might still be edge cases I haven't thought of ;) But I suppose that's what nightlies are for. > >+ var condition; > > var -> let. I'd like to understand this change. Aren't they identical here because we are not inside a {...} block? > The added sentence here isn't helpful (a prefix is always from the beginning). > Either remove it, or You're right, it's unnecessary. It's simply working as it should.
Attachment #8352687 -
Flags: review?(florian)
Assignee | ||
Comment 16•10 years ago
|
||
Comment on attachment 8352680 [details] [diff] [review] Patch for conversation.xml *** Original change on bio 1097 attmnt 938 at 2011-10-26 18:18:19 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352680 -
Attachment is obsolete: true
Comment 17•10 years ago
|
||
Comment on attachment 8352687 [details] [diff] [review] Patch for conversation.xml *** Original change on bio 1097 attmnt 945 at 2011-10-26 23:32:02 UTC *** Looks good! :) (In reply to comment #12) > > var -> let. > > I'd like to understand this change. Aren't they identical here because we are > not inside a {...} block? We discussed this on IRC. Summary: when they are identical, we prefer let, especially in newer code.
Attachment #8352687 -
Flags: review?(florian) → review+
Comment 18•10 years ago
|
||
*** Original post on bio 1097 at 2011-10-27 12:04:54 UTC *** Fixed: https://hg.instantbird.org/instantbird/rev/9ff5edcb1619
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.
Description
•