Experience with provideTextSearchResults

[siegebell]

• VSCode Version: 1.21.0-insider (b76a4a7)
• OS Version: Mac 10.13.3

@jrieken @bolinfest @hansonw
I've implemented FileSystemProvider.provideTextSearchResults from the proposed API. Overall it's working very well! For anyone who might play around with this, and to help the API mature, here are my less-nice experiences:

1. Bug: vscode is adding 1 to the line number of each matched range: https://github.com/Microsoft/vscode/blob/42de6cc82f2dcd7866b0b88bccf0984f9d4d9e90/src/vs/workbench/api/node/extHostFileSystem.ts#L148
2. If a file is already open, vscode ignores my provided results and does its own thing. This is fine for now because I'm using ripgrep. But It'll be a problem with engines that have e.g. different regex syntax or are a semantic search.
3. Results are not shown in the search panel until my provider fully resolves 😢
4. The cancellation token is not hooked up: pressing ESC in search does cancel the search in the UI, but the callback I provide to token.onCancellationRequested is never invoked. Same with clicking the "cancel search" button or toggling an option like isRegExp.
5. The Range I report back is almost completely ignored. Columns are instead calculated by the length of the leading and trailing strings:
   https://github.com/Microsoft/vscode/blob/42de6cc82f2dcd7866b0b88bccf0984f9d4d9e90/src/vs/workbench/api/node/extHostFileSystem.ts#L150
6. It took me a while to figure out "leading" and "trailing". At first I thought I could provide an abbreviated snippet, but this screws up the column that vscode highlights. So now I provide the entire line of leading and trailing text around each match. Sure hope there are no outrageously long lines in any of the files I'm searching through. Like jquery...*.min.js...
7. TextSearchQuery defines isRegex: https://github.com/Microsoft/vscode/blob/42de6cc82f2dcd7866b0b88bccf0984f9d4d9e90/src/vs/vscode.proposed.d.ts#L88 but isRegExp is given instead: https://github.com/Microsoft/vscode/blob/42de6cc82f2dcd7866b0b88bccf0984f9d4d9e90/src/vs/platform/search/common/search.ts#L93
8. isSmartCase is also given, but TextSearchQuery does not define it. (Looks even more experimental since it's not exposed in the UI?)
9. Since the UI is unambiguous in setting isRegExp, isCaseSensitive, and isWordMatch, perhaps the fields in TextSearchQuery should not be nullable.
10. I'm uncertain of the semantics of RelativePath and which base directories I should search. For now, my interpretation is this:
    1. Call search for each workspace folder that my FS provider handles.
    2. For each folder, use the plain-string includes and excludes, but ignore the relative patterns who's base is not a subdirectory of the folder.

[roblourens]

If a file is already open, vscode ignores my provided results and does its own thing. This is fine for now because I'm using ripgrep. But It'll be a problem with engines that have e.g. different regex syntax or are a semantic search.

I was experimenting with this API too and wasn't sure what the plan is, will I be able to replace the built in local search provider for local files?

[jrieken]

@siegebell Thanks for trying this and thanks for the feedback. Things are still rough but I pushed a couple of changes that should ease some pain, esp. cancelation and progress should be working now. Also, note that I have pulled the search-functionality out of the file system provider into a SearchProvider. That should allow us to move faster.

The proposal is now this

	export interface SearchProvider {
		provideFileSearchResults?(query: string, progress: Progress<Uri>, token: CancellationToken): Thenable<void>;
		provideTextSearchResults?(query: TextSearchQuery, options: TextSearchOptions, progress: Progress<TextSearchResult>, token: CancellationToken): Thenable<void>;
	}

	export namespace workspace {
		export function registerSearchProvider(scheme: string, provider: SearchProvider): Disposable;
	}

and how it is in action, (progress/cancelation ftw)

[jrieken]

It took me a while to figure out "leading" and "trailing". At first I thought I could provide an abbreviated snippet, but this screws up the column that vscode highlights. So now I provide the entire line of leading and trailing text around each match. Sure hope there are no outrageously long lines in any of the files I'm searching through. Like jquery...*.min.js...

Good point and something that needs to be defined better. I went with the leading/matching/trailing model to prevent the long lines but there are parts of our internal search model I don't know well. I think we need two things: (1) the location of the search match and (2) a preview-string and a pointer inside that preview-string highlighting the actual match. For instance something like this

export interface TextSearchResult {
  location: { uri: Uri, range: Range };
  preview?: { text: string, range: Range };
}

and then a query would also express if and how much preview is desired, e.g. preview just the line of the match or preview the lines above/below (similar to grep -C). Does that make sense?

export interface TextSearchQuery {
  preview: number;
  // ... more ...
}

[jrieken]

@roblourens Decoupling the preview from the actual location (as described above) needs some refactoring I believe.

[roblourens]

Yes it does, there are issues with searching in long lines like #31551 and we should be doing that differently.

[roblourens]

I just pushed a fix for points 7/8. I think all of the other are either handled already or covered by #47058 or #50788. Will close this to continue discussion elsewhere.