Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Quick File Open: Support for Search Queries with Whitespaces #8989

Merged

Conversation

seantan22
Copy link
Contributor

@seantan22 seantan22 commented Jan 26, 2021

What it does

Fixes #8747

  • Allows whitespaces to be included in a quick file open search query.
  • Adds support for whitespaces in the scoring of file search results.
  • Adds tests for whitespace queries (considers fuzzy matching and search
    term order).

How to test

  1. ctrl + p to quick file open
  2. Search for a file with a query that includes whitespaces (eg. readme core)
  3. Observe that whitespaces do not affect the search results

Alternatively, run @theia/file-search tests.

Before Changes:

image

After Changes:

image

Review checklist

Reminder for reviewers

Signed-off-by: seantan22 sean.a.tan@ericsson.com

@vince-fugnitto vince-fugnitto added enhancement issues that are enhancements to current functionality - nice to haves file search issues related to the file search labels Jan 26, 2021
@vince-fugnitto vince-fugnitto marked this pull request as draft January 27, 2021 14:26
@seantan22 seantan22 force-pushed the st/whitespace-search branch 5 times, most recently from 227199b to 0339464 Compare January 27, 2021 19:32
@vince-fugnitto vince-fugnitto marked this pull request as ready for review January 28, 2021 14:21
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes work very well for me, and the results are much better than master.
I am now able to search with whitespaces, and the scoring has been properly updated to take whitespaces into consideration and prioritize exact matches.

@seantan22 seantan22 force-pushed the st/whitespace-search branch 2 times, most recently from 13a0c7f to efced8d Compare February 2, 2021 19:10
What it Does

Fixes [eclipse-theia#8747](eclipse-theia#8747)

- Allows whitespaces to be included in a `quick file open` search query.
- Adds support for whitespaces in the scoring of file search results
- Adds tests for whitespace queries (considers fuzzy matching and search
  term order).

How to Test

1. `ctrl + p` to `quick file open`
2. Search for a file with a query that includes whitespaces (eg. `readme core`)
3. Observe that whitespaces do not affect the search results

Alternatively, run `@theia/file-search` tests.

Signed-off-by: seantan22 <sean.a.tan@ericsson.com>
@vince-fugnitto
Copy link
Member

@colin-grant-work I found an issue with the previous approach with fuzzy.match that has been addressed in the latest changes.
Would you mind re-reviewing and if you're happy with the changes please feel free to merge them.

Copy link
Contributor

@DucNgn DucNgn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍
I reviewed and tested the changes on my side, confirmed that the feature is working correctly. Nice to be able to search with whitespaces!

@colin-grant-work
Copy link
Contributor

So the basic functionality is working, but there is one defect when searching with whitespace. With no whitespace, we get highlighting in the results:

image

with whitespace, we get no highlighting:

image

It's still an improvement (I'd rather see the results with no highlighting than have to constantly remember not to put spaces in my queries), but it would be great to get the highlighting functionality working, as well.

@vince-fugnitto
Copy link
Member

So the basic functionality is working, but there is one defect when searching with whitespace. With no whitespace, we get highlighting in the results:

The highlighting is a known issue #4548 and would require some extensive changes on how we compute the matching (#5638 (comment)). For that reason I'm fine with omitting it from the pull-request as it is out of scope.

@colin-grant-work
Copy link
Contributor

The highlighting is a known issue #4548 and would require some extensive changes on how we compute the matching (#5638 (comment)). For that reason I'm fine with omitting it from the pull-request as it is out of scope.

Sounds good. Having touched the highlighting code lightly, I agree it would be a huge lift for this PR. My approval is reaffirmed.

@vince-fugnitto
Copy link
Member

I'll merge tomorrow if there are no objections :)

@vince-fugnitto vince-fugnitto merged commit b74355f into eclipse-theia:master Feb 5, 2021
@paul-marechal paul-marechal added this to the 1.11.0 milestone Feb 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement issues that are enhancements to current functionality - nice to haves file search issues related to the file search
Projects
None yet
Development

Successfully merging this pull request may close these issues.

file-search: support whitespace queries
5 participants