[search-in-workspace][windows] Fix search in workspace for Windows #2648
Conversation
| import { ILogger } from '@theia/core'; | ||
| import { FileUri } from '@theia/core/lib/node/file-uri'; |
There was a problem hiding this comment.
Please revert, because you're importing a node module into a browser module.
| @@ -112,8 +112,7 @@ export class SearchInWorkspaceService implements SearchInWorkspaceClient { | |||
| throw new Error('Search failed: no workspace root.'); | |||
| } | |||
|
|
|||
| const rootUri = new URI(root.uri); | |||
| const searchId = await this.searchServer.search(what, rootUri.path.toString(), opts); | |||
| const searchId = await this.searchServer.search(what, FileUri.fsPath(root.uri), opts); | |||
There was a problem hiding this comment.
Is there an issue with the path handling in the search service? Should we move this conversion to the service implementation?
Could you point out, what's the real issue with given path on win?
There was a problem hiding this comment.
fsPath has some special handling for windows: https://github.com/Microsoft/vscode-uri/blob/b1d3221579f97f28a839b6f996d76fc45e9964d8/src/index.ts#L955-L976
We can pass a URI instead and call FileUri.fsPath on the backend.
There was a problem hiding this comment.
@AlexTugarev, on Windows the search service passes a path with a leading slash to vscode-ripgrep. So searches always returns empty. FileUri.fsPath() fixes this.
There was a problem hiding this comment.
@gonzalezw, we should not use FileUri in the browser code. We had a very similar issue with the file search (Ctrl/Cmd+P). Please look into that PR. Perhaps this is something we have to fix on the backend.
|
@kittaakos, I've update the PR to make the path conversion in the backend. |
|
@gonzalezw, awesome. Please squash your commits, and I restart the Windows build. The current failure is most likely unrelated to your change. |
gonzalezw
force-pushed
the
fix-search-windows
branch
from
c29877c
to
c72cea8
Compare
|
@kittaakos, I've squashed the commits |
|
I have verified it on Windows (and on OS X too); it works nicely. Thank you! Since we have switched from a path string to a URI string, can you please modify the API to reflect this change. Namely, I would like to ask you to change Edit: and on the implementation side as well. So if someone reads the code, it is clear what |
gonzalezw
force-pushed
the
fix-search-windows
branch
from
c72cea8
to
3ce66db
Compare
| @@ -5,6 +5,10 @@ | |||
| "dependencies": { | |||
| "@theia/core": "^0.3.14", | |||
| "@theia/editor": "^0.3.14", | |||
| "@theia/workspace": "^0.3.14", | |||
There was a problem hiding this comment.
that's very good, thanks for spotting it :)
Please squash again, @gonzalezw. |
Signed-off-by: William Gonzalez <William.Gonzalez@arm.com>
gonzalezw
force-pushed
the
fix-search-windows
branch
from
b172618
to
442345d
Compare
|
@kittaakos, I've squashed. |
There was a problem hiding this comment.
I have verified 442345d on Windows. It works. Thank you for your help, @gonzalezw!
Changes
Fixes Theia issue 2039.