Closed Bug 954531 Opened 10 years ago Closed 10 years ago

Tab complete is case sensitive

Categories

(Instantbird Graveyard :: Conversation, defect)

x86
All
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aleth, Assigned: aleth)

Details

Attachments

(1 file, 4 obsolete files)

*** Original post on bio 1097 at 2011-10-20 19:17:00 UTC ***

and should not be, I think
Attached patch Patch for conversation.xml (obsolete) — Splinter Review
*** 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 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-
Attached patch Patch for conversation.xml (obsolete) — Splinter Review
*** 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?
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
*** 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
*** 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 :-(.
*** 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 :-).
Attached patch Patch for conversation.xml (obsolete) — Splinter Review
*** 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)
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 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-
*** 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).
Attached patch Patch for conversation.xml (obsolete) — Splinter Review
*** 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)
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 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+
*** 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)
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 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+
*** 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.