quick palette fixes #4549

akosyakov · 2019-03-13T09:47:26Z

By default quick palette won't do separate fuzzy matching anymore. For most clients it produces bogus result.
Fixed race condition in file search.
Matching based on relative paths for file search instead of absolute, see [open file][find command] Unexpected search results #4537 (comment)
Use a root URI as a CWD for rigrep to make it reliable and limit the search scope, see quick palette fixes #4549 (comment)

See #4549 (comment) for how to reproduce.

elaihau · 2019-03-13T12:08:20Z

This is what i get from the Feb 26 build

And this patch:

As you can tell from the GIFs, when I searched work I wasn't getting as many results from workspace and search-in-workspace folder, and some results, such as README.md, package.json, and those from .git (which should be ignored), didn't make sense to me.

Not sure if it is caused by the change in this PR.

vince-fugnitto · 2019-03-13T12:47:38Z

And this patch:

As you can tell from the GIFs, when I searched work I wasn't getting as many results from workspace and search-in-workspace folder, and some results, such as README.md, package.json, and those from .git (which should be ignored), didn't make sense to me.

Not sure if it is caused by the change in this PR.

I can confirm, I noticed the same thing @elaihau mentioned.
I also a slight delay when searching, let's say tasks. The file-results only appear after a short delay, but not sure it's a major issue.

akosyakov · 2019-03-13T12:53:03Z

@elaihau @vince-fugnitto could you double check, i get on master:

with this PR:

@elaihau Could be that you confused one with another?

Your search also looks different, it seems that in one case you pressed quick file twice to enable ignored files and another once. Please press once.

akosyakov · 2019-03-13T13:00:12Z

and those from .git (which should be ignored), didn't make sense to me.

It's the same on master, please open a separate issue.

elaihau · 2019-03-13T13:07:33Z

I pulled from master and ran one more test from my local env:

I don't see any entries from .git, or package.json / README.md. So master branch seems fine. I will also test this change from my local env.

akosyakov · 2019-03-13T13:12:28Z

@elaihau strange and you don't have any custom user settings?

elaihau · 2019-03-13T13:13:28Z

you don't have any custom user settings?

not sure. could you please give me a couple of examples of "user settings" ?

akosyakov · 2019-03-13T13:15:37Z

I meant just in home/youruser/.theia/setting.json, maybe you customize some settings. Files from .gitignore should not appear in search result by default, only if you trigger search twice. But you have them.

elaihau · 2019-03-13T13:23:43Z

I don't have search-related prefs in my settings.json:

   "editor.autoSave": "off",
   "editor.lineNumbers": "relative",
   "workspace.supportMultiRootWorkspace": true,
   "typescript.trace.server": "verbose",
   "editor.renderWhitespace": "all",
   "editor.renderIndentGuides": true,
   "editor.fontSize": 14,

I just tested this change in my local env. and everything looks working.
weird thing was I saw different search results from gitpod.

akosyakov · 2019-03-13T13:27:50Z

I just tested this change in my local env. and everything looks working.

If you trigger file search once you should not see files filtered by .gitignore but you have them on your gifs. It does not look working.

elaihau · 2019-03-13T13:36:51Z

If you trigger file search once you should not see files filtered by .gitignore

if i am not mistaken, the .gitignore thing has broken for a long time #1588
i also see similar issues reported on the board of ripgrep. if you search "ignore" from https://github.com/BurntSushi/ripgrep/issues you will see at least a couple.

AlexTugarev · 2019-03-13T13:41:26Z

Works nicely for commands now! 💯

File search works with the .gitignore exception.
a) good cases

b) should have been filtered, as node_modules is ignored

I checked with current master state and it's the very same issue:

vince-fugnitto · 2019-03-13T13:44:48Z

@akosyakov I see the problems from master now, I don't what caused it as it was working quite nicely :( your PR does address a lot of the issues and improves it.

I noticed one thing however, do you believe this is a bug from your PR?

Searching for workspace:

Searching for workspace-:

As you can see when searching for workspace we don't get the complete set of results, we start getting results which don't truly make sense based on the query. When the search query is workspace- we get the more complete set.

akosyakov · 2019-03-13T13:46:46Z

@elaihau ok, it seems with Gitpod OS, it works a bit better and manages to filter out some ignored files

Anyway, i did not change code around it. It's fixing following issues reproduciable on master:
(1) race condition: try typing shell very fast and you get results like LICENSE which does not match at all and sometimes shell.ts won't match at all when it should
(2) using absolute paths for matching, type shell this time very slow, make sure that shell.ts is among results, but there are also completely bogus results, like logo/theia-logo.svg in Gitpod

The issue with (2) is that we match against /workSpace/tHEia/Logo/theia-Logo.svg, but should only against logo/theia-logo.svg which does not have shell.

With this PR you should not be able to reproduce these issues.

akosyakov · 2019-03-13T13:50:26Z

@vince-fugnitto i think it is all because of #1588. you should not see anything from node_modules and .git and then results should be cleaned. It is for sure something worth to look into.

akosyakov · 2019-03-13T13:52:35Z

And I also have feeling that it got broken at some point, or we just all so get used to it.

elaihau

what i am getting from my local env (ubuntu 16 + chrome + example browser) looks good.

however when i tested on gitpod it gives me some irrelavent stuffs (e.g., package.json and readme.md) when i search "work". Also, like @vince-fugnitto described, some results from searching workspace- do not show up on the search of workspace.

akosyakov · 2019-03-13T14:11:48Z

Ok, will give one more round maybe can improve it a bit more.

AlexTugarev · 2019-03-13T14:13:35Z

packages/file-search/src/node/file-search-service-impl.spec.ts

+    describe('irrelevant absolute results', () => {
+        const rootUri = FileUri.create(path.resolve(__dirname, '../../../../..'));
+
+        it('not fuzzy', async () => {


here is a repro for the bogus workspace search results.

const rootUri = FileUri.create(path.resolve(__dirname, '../../../..')); it.only('not fuzzy', async () => { const searchPattern = rootUri.path.dir.base; const matches = await service.find(searchPattern, { rootUris: [rootUri.toString()], fuzzyMatch: false, useGitIgnore: true, limit: 200 }); for (const match of matches) { const relativUri = rootUri.relative(new URI(match)); if (relativUri) { const relativMatch = relativUri.toString(); assert.notEqual(relativMatch.indexOf(searchPattern), -1, relativMatch); } } });

@elaihau, ☝️ the workspace search pattern is in the path of the bogus search results.

it.only nice feature, did not know it :)

just don't forget to remove it ;-)

It's because of glob search: https://github.com/theia-ide/theia/blob/8235400c78af8eb46ef5523ae1a635ec9761a0c3/packages/file-search/src/node/file-search-service-impl.ts#L93-L94

@elaihau @vince-fugnitto @AlexTugarev do you have an idea why we do it in addition to fuzzy search?

ah, I see it is for vscode extension, i don't think API was modeled properly, there should be a new explicit option for included globs and it should not affect quick file

@akosyakov this is one example: https://github.com/Microsoft/vscode/blob/master/extensions/npm/src/tasks.ts#L101

npm uses the service to find all packages.json, excluding those from node_modules.

there are a few examples tested here as well:

https://github.com/Microsoft/vscode/blob/92629f3f5f4c662a26cc193b5208c3ff3a335243/extensions/vscode-api-tests/src/singlefolder-tests/workspace.test.ts#L490-L522

there should be a new explicit option for included globs and it should not affect quick file

i was thinking about the same thing. but after seeing this interface i chose to support the string match & glob match from the same function without having that option. https://github.com/Microsoft/vscode/blob/master/src/vs/workbench/services/search/common/search.ts#L79

export interface IFileQueryProps<U extends UriComponents> extends ICommonQueryProps<U> { type: QueryType.File; filePattern?: string; /** * If true no results will be returned. Instead `limitHit` will indicate if at least one result exists or not. * Currently does not work with queries including a 'siblings clause'. */ exists?: boolean; sortByScore?: boolean; cacheKey?: string; }

maybe you are right, with the new option defined we would have finer grained control over what the service does.

@elaihau yes, searchPattern and includePatterns are not the same, i've separated them

akosyakov · 2019-03-13T21:28:22Z

@elaihau @vince-fugnitto @AlexTugarev please try again

VS Code API client also should be tested, what do you expect from npm extension?

vince-fugnitto · 2019-03-13T21:51:14Z

@elaihau @vince-fugnitto @AlexTugarev please try again

VS Code API client also should be tested, what do you expect from npm extension?

the builds are failing, and so is the prebuilt gitpod workspace 😞

akosyakov · 2019-03-14T05:43:32Z

@vince-fugnitto should be good now

Signed-off-by: Anton Kosyakov <[email protected]>

…4537 Signed-off-by: Anton Kosyakov <[email protected]>

Signed-off-by: Anton Kosyakov <[email protected]>

AlexTugarev

Thanks so much for fixing this! 💯

vince-fugnitto · 2019-03-14T11:04:23Z

@akosyakov sorry I just got a chance to check it now, its still early morning in Montreal 😄

On Gitpod I noticed that everything was fine, however locally I noticed irregularities.
I had many recently opened results, and when searching all these recently opened results persisted, which felt like an error to me. I don't know why it occurs locally and not on Gitpod however.

It might be something to look into.

akosyakov · 2019-03-14T11:16:53Z

@vince-fugnitto please file an issue, this PR does not touch recently opened at all, but you are right it is worth to look into as well

vince-fugnitto · 2019-03-14T11:39:05Z

@vince-fugnitto please file an issue, this PR does not touch recently opened at all, but you are right it is worth to look into as well

I was able to reproduce in Gitpod as well.
I'm completely fine in merging the PR, the changes look great and works great as well !
If anything we can always fix the recently opened in another PR after we track it in an issue.

akosyakov · 2019-03-14T12:26:53Z

VS Code API client also should be tested, what do you expect from npm extension?

@elaihau is it good for VS Code extension? Could you verify please?

elaihau · 2019-03-14T12:30:43Z

VS Code API client also should be tested, what do you expect from npm extension?

@elaihau is it good for VS Code extension? Could you verify please?

testing right now

elaihau · 2019-03-14T12:42:47Z

i am getting the following errors in browser:

Uncaught (in promise) TypeError: theiaToken.onCancellationRequested is not a function
    at WorkspaceMainImpl.<anonymous> (:3000/82.bundle.js:14076)
    at step (:3000/82.bundle.js:13938)
    at Object.next (:3000/82.bundle.js:13919)
    at :3000/82.bundle.js:13913
    at new Promise (<anonymous>)
    at push.../../packages/plugin-ext/lib/main/browser/workspace-main.js.__awaiter (:3000/82.bundle.js:13909)
    at WorkspaceMainImpl.push.../../packages/plugin-ext/lib/main/browser/workspace-main.js.WorkspaceMainImpl.$startFileSearch (:3000/82.bundle.js:14053)
    at RPCProtocolImpl.push.../../packages/plugin-ext/lib/api/rpc-protocol.js.RPCProtocolImpl.doInvokeHandler (:3000/82.bundle.js:835)
    at RPCProtocolImpl.push.../../packages/plugin-ext/lib/api/rpc-protocol.js.RPCProtocolImpl.invokeHandler (:3000/82.bundle.js:820)
    at RPCProtocolImpl.push.../../packages/plugin-ext/lib/api/rpc-protocol.js.RPCProtocolImpl.receiveRequest (:3000/82.bundle.js:784)

i will see if i can find where it went wrong

elaihau · 2019-03-14T12:51:53Z

line 174 of workspace-main.ts is where it failed. the function call didn't reach the file-search-service, and stopped in the plugin-ext

        if (theiaToken) {
            theiaToken.onCancellationRequested(() => source.cancel());
        }

command.ts:15 Uncaught (in promise) TypeError: theiaToken.onCancellationRequested is not a function
    at WorkspaceMainImpl.<anonymous> (workspace-main.ts:174)
    at step (workspace-main.ts:15)
    at Object.next (workspace-main.ts:15)
    at workspace-main.ts:15
    at new Promise (<anonymous>)
    at push.../../packages/plugin-ext/lib/main/browser/workspace-main.js.__awaiter (workspace-main.ts:15)
    at WorkspaceMainImpl.push.../../packages/plugin-ext/lib/main/browser/workspace-main.js.WorkspaceMainImpl.$startFileSearch (workspace-main.ts:156)
    at RPCProtocolImpl.push.../../packages/plugin-ext/lib/api/rpc-protocol.js.RPCProtocolImpl.doInvokeHandler (rpc-protocol.ts:216)
    at RPCProtocolImpl.push.../../packages/plugin-ext/lib/api/rpc-protocol.js.RPCProtocolImpl.invokeHandler (rpc-protocol.ts:201)
    at RPCProtocolImpl.push.../../packages/plugin-ext/lib/api/rpc-protocol.js.RPCProtocolImpl.receiveRequest (rpc-protocol.ts:157)

akosyakov · 2019-03-14T12:55:31Z

@elaihau good, please describe which extension do you install and what you do

akosyakov · 2019-03-14T13:07:00Z

packages/plugin-ext/src/main/browser/workspace-main.ts

-        maxResults?: number, token?: theia.CancellationToken): Promise<UriComponents[]> {
-        const opts: FileSearchService.Options = { rootUris: this.roots.map(r => r.uri) };
+    async $startFileSearch(includePattern: string, includeFolderUri: string | undefined, excludePatternOrDisregardExcludes?: string | false,
+        maxResults?: number, theiaToken?: theia.CancellationToken): Promise<UriComponents[]> {


@benoitf @evidolob Is cancellation hooked into plugin RCP? from #4549 (comment) it looks like not. Is there another way to cancel?

fyi, this is what vscode does from the extension host:

if (token && token.isCancellationRequested) { return Promise.resolve([]); } return this._proxy.$startFileSearch(includePattern, includeFolder, excludePatternOrDisregardExcludes, maxResults, token) .then(data => Array.isArray(data) ? data.map(URI.revive) : []);

code is from extHostWorkspace.ts

i know, but it does not seem to work in Theia, i would need to check

…h root Signed-off-by: Anton Kosyakov <[email protected]>

akosyakov · 2019-03-14T13:34:57Z

@elaihau i've removed cancellation, plugin RCP does not seem to support it, could you try again please

elaihau · 2019-03-14T14:10:14Z

describe which extension do you install and what you do

i installed grunt and npm. what i did was run command "run tasks" from the single root workspace where gulp tasks and npm scripts are defined.

i tested again. with the latest change everything from my side worked fine. Thank you !

akosyakov requested review from vince-fugnitto, elaihau and svenefftinge and removed request for elaihau March 13, 2019 09:48

akosyakov force-pushed the GH-4537 branch from 9bb5937 to 6aac747 Compare March 13, 2019 09:48

akosyakov assigned AlexTugarev Mar 13, 2019

akosyakov mentioned this pull request Mar 13, 2019

file search does not highlight properly #4548

Open

akosyakov unassigned AlexTugarev Mar 13, 2019

akosyakov requested review from elaihau and AlexTugarev March 13, 2019 09:50

elaihau approved these changes Mar 13, 2019

View reviewed changes

AlexTugarev reviewed Mar 13, 2019

View reviewed changes

akosyakov force-pushed the GH-4537 branch from 6aac747 to f7830f1 Compare March 13, 2019 21:23

akosyakov requested review from elaihau and AlexTugarev March 13, 2019 21:24

akosyakov force-pushed the GH-4537 branch from f7830f1 to 6b24526 Compare March 14, 2019 05:40

akosyakov added 3 commits March 14, 2019 05:55

[quick-open] disable separate fuzzy substring matching by default #4537

89b2f6b

Signed-off-by: Anton Kosyakov <[email protected]>

[file-search] handle cancellation properly to avoid race conditions #…

265e12f

…4537 Signed-off-by: Anton Kosyakov <[email protected]>

[file-search] match against relative paths (not absolute) #4537

e93d45a

Signed-off-by: Anton Kosyakov <[email protected]>

akosyakov force-pushed the GH-4537 branch from 6b24526 to fe10de1 Compare March 14, 2019 05:56

AlexTugarev approved these changes Mar 14, 2019

View reviewed changes

vince-fugnitto approved these changes Mar 14, 2019

View reviewed changes

akosyakov mentioned this pull request Mar 14, 2019

recently opened files are not filtered in quick file #4563

Closed

akosyakov commented Mar 14, 2019

View reviewed changes

[file-search] spawn rigrep with root URI as cwd and only once for eac…

5a25354

…h root Signed-off-by: Anton Kosyakov <[email protected]>

akosyakov force-pushed the GH-4537 branch from fe10de1 to 5a25354 Compare March 14, 2019 13:34

elaihau approved these changes Mar 14, 2019

View reviewed changes

benoitf approved these changes Mar 14, 2019

View reviewed changes

akosyakov merged commit b21023e into master Mar 15, 2019

akosyakov deleted the GH-4537 branch March 15, 2019 07:00

quick palette fixes #4549

quick palette fixes #4549

Conversation

akosyakov commented Mar 13, 2019 • edited Loading

elaihau commented Mar 13, 2019 • edited Loading

vince-fugnitto commented Mar 13, 2019

akosyakov commented Mar 13, 2019 • edited Loading

akosyakov commented Mar 13, 2019

elaihau commented Mar 13, 2019 • edited Loading

akosyakov commented Mar 13, 2019

elaihau commented Mar 13, 2019

akosyakov commented Mar 13, 2019

elaihau commented Mar 13, 2019

akosyakov commented Mar 13, 2019

elaihau commented Mar 13, 2019

AlexTugarev commented Mar 13, 2019

vince-fugnitto commented Mar 13, 2019

akosyakov commented Mar 13, 2019 • edited Loading

akosyakov commented Mar 13, 2019 • edited Loading

akosyakov commented Mar 13, 2019

elaihau left a comment • edited Loading

Choose a reason for hiding this comment

akosyakov commented Mar 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elaihau Mar 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akosyakov commented Mar 13, 2019

vince-fugnitto commented Mar 13, 2019

akosyakov commented Mar 14, 2019

AlexTugarev left a comment

Choose a reason for hiding this comment

vince-fugnitto commented Mar 14, 2019

akosyakov commented Mar 14, 2019

vince-fugnitto commented Mar 14, 2019

akosyakov commented Mar 14, 2019

elaihau commented Mar 14, 2019

elaihau commented Mar 14, 2019

elaihau commented Mar 14, 2019 • edited Loading

akosyakov commented Mar 14, 2019

Choose a reason for hiding this comment

elaihau Mar 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akosyakov commented Mar 14, 2019 • edited Loading

elaihau commented Mar 14, 2019

akosyakov commented Mar 13, 2019 •

edited

Loading

elaihau commented Mar 13, 2019 •

edited

Loading

akosyakov commented Mar 13, 2019 •

edited

Loading

elaihau commented Mar 13, 2019 •

edited

Loading

akosyakov commented Mar 13, 2019 •

edited

Loading

akosyakov commented Mar 13, 2019 •

edited

Loading

elaihau left a comment •

edited

Loading

elaihau Mar 13, 2019 •

edited

Loading

elaihau commented Mar 14, 2019 •

edited

Loading

elaihau Mar 14, 2019 •

edited

Loading

akosyakov commented Mar 14, 2019 •

edited

Loading