Allow for completions filter to be case insensitive #2249

bschulte · 2018-05-26T00:26:41Z

I created an issue about this #2230. This change will just allow users to change the completion filter to not be case sensitive.

The setting "editor.completions.caseSensitive" can be set to true or false (true by default) and will tell the completion filter to be case sensitive or not.

CrossR · 2018-05-26T15:17:23Z

browser/src/Services/Configuration/IConfigurationValues.ts

@@ -150,6 +150,9 @@ export interface IConfigurationValues {
 // a custom init.vim, as that may cause problematic behavior
 "editor.completions.mode": string

+ // Decide whether or not the completion matching should be case sensitive
+ "editor.completions.caseSensitive": boolean


Thanks for the PR @bschulte!

It may make sense to make this like the other case sensitive option, ie string | boolean such that it can be smart/true/false.

Then the check becomes something like this:

oni/browser/src/Services/Menu/MenuFilter.ts

Lines 25 to 39 in 262098f

if (caseSensitivitySetting === false) {

return false

} else if (caseSensitivitySetting === true) {

return true

} else {

// "Smart" casing strategy

// If the string is all lower-case, not case sensitive..

if (searchString === searchString.toLowerCase()) {

return false

// Otherwise, it is case sensitive..

} else {

return true

}

}

}

That should also let you simplify the regex check like we have over here:

oni/browser/src/Services/QuickOpen/RegExFilter.ts

Lines 34 to 38 in afab2be

const isCaseSensitive = shouldFilterbeCaseSensitive(searchString)

if (!isCaseSensitive) {

searchString = searchString.toLowerCase()

}

That lets us use the same regex for all three cases then.

bschulte · 2018-05-26T17:27:31Z

Thanks for the feedback! I've added a commit to allow for the config value of true/false/smart. I also tried to follow the same pattern that you linked to elsewhere in the code.

I did have to also perform the check if the search should be case sensitive when it comes to creating the RegEx, otherwise it wouldn't work. From the code elsewhere in the project, it looks like you didn't have to do that so maybe I'm missing something silly?

Edit: Scratch that, I was just thinking about it too much and confused myself. I see where you perform the check both on the search text and the potential matches. I'll modify my code to follow a similar pattern and keep things uniform.

bschulte · 2018-05-26T17:42:24Z

OK, I modified things to conform to the already exists patterns a bit more.

One thing I noticed in testing was that the underlining wasn't correct when the search was case insensitive (which makes sense). I was having a bit of trouble tracking down where that code was to handle that, would you mind pointing me in the right direction?

bryphe · 2018-05-29T23:40:54Z

Thanks for making the changes, @bschulte ! Changes overall look good to me.

It looks like there is a test failure with the AutoCompletionTest-HTML integration test:

oni/test/ci/AutoCompletionTest-HTML.ts

Line 18 in d180879

oni.automation.sendKeys("<lt>body s")

That test sends these keys:
<body s

And waits for completion to appear (it expects that style should be in the completion). It looks like perhaps that behavior changed with this - it might be that style isn't one of the top-ten completions we show with case-insensitive. A quick fix for this would be to add a configuration setting, like:

oni/test/ci/AutoClosingPairsTest.ts

Line 63 in d180879

export const settings = {

export const settings = {
   config: {
        "editor.completions.caseSensitive": true,
    }
}

So for now, we could just set that configuration setting specific for that test. Alternatively, we could pick a new item to test to validate.

Speaking of tests - it'd be great to have a unit test documenting this behavior here:

oni/browser/test/Services/Completion/CompletionTests.ts

Line 173 in d180879

 it("if there is a completion matching the base, it should be the first shown", async () => { 

We have a bunch of tests exercising various parts of completion there - it'd be cool to have a case that exercises the new behavior as well. That will help protect it from regressing. Let me know if you have questions about adding a test there.

Thanks again for taking this on @bschulte - I know users have asked for this before, will be a great addition to our completion story 👍

Allow for completions filter to be case insensitive

a1c6db9

CrossR reviewed May 26, 2018

View reviewed changes

Refactored to allow for smart case completion

21a3044

Small refactor to match code pattern elsewhere in the project

9881538

nghialm269 mentioned this pull request Oct 7, 2018

Autocomplete case insensitivity #2230

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow for completions filter to be case insensitive #2249

Allow for completions filter to be case insensitive #2249

bschulte commented May 26, 2018

CrossR May 26, 2018

bschulte commented May 26, 2018 •

edited

bschulte commented May 26, 2018

bryphe commented May 29, 2018

	if (caseSensitivitySetting === false) {
	return false
	} else if (caseSensitivitySetting === true) {
	return true
	} else {
	// "Smart" casing strategy
	// If the string is all lower-case, not case sensitive..
	if (searchString === searchString.toLowerCase()) {
	return false
	// Otherwise, it is case sensitive..
	} else {
	return true
	}
	}
	}

	const isCaseSensitive = shouldFilterbeCaseSensitive(searchString)

	if (!isCaseSensitive) {
	searchString = searchString.toLowerCase()
	}

Allow for completions filter to be case insensitive #2249

Are you sure you want to change the base?

Allow for completions filter to be case insensitive #2249

Conversation

bschulte commented May 26, 2018

CrossR May 26, 2018

Choose a reason for hiding this comment

bschulte commented May 26, 2018 • edited

bschulte commented May 26, 2018

bryphe commented May 29, 2018

bschulte commented May 26, 2018 •

edited