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

Text search should support context #59458

Closed
jrieken opened this issue Sep 26, 2018 · 7 comments
Closed

Text search should support context #59458

jrieken opened this issue Sep 26, 2018 · 7 comments
Assignees
Labels
api-proposal feature-request Request for new features or functionality verification-needed Verification of issue is requested verified Verification succeeded
Milestone

Comments

@jrieken
Copy link
Member

jrieken commented Sep 26, 2018

We should consider something like the --context option (https://github.com/BurntSushi/ripgrep/blob/master/GUIDE.md#common-options) instead of the current preview options. They currently don't allow to show a multi line context around a match and make implementing a textual representation of search results very hard...

@roblourens roblourens added api-proposal feature-request Request for new features or functionality labels Sep 26, 2018
@roblourens
Copy link
Member

I would add

export interface TextSearchPreviewOptions {
    contextLines?: number;
}

@jrieken
Copy link
Member Author

jrieken commented Sep 27, 2018

Yeah, that would be nice but might also be problematic. When having context: boolean the search engine can decide, e.g. show 5 lines (two above & below) for normal matches but only show parts of a line for monster lines

@roblourens
Copy link
Member

roblourens commented Sep 27, 2018

Sometimes it's up to the user how much context should be shown. In Atom I can change leading and trailing context via the UI independently.

If you are showing results as text with context, is it annoying that multiple results on one line are given as multiple result objects, possibly with overlapping previews? Should the API keep the one-result-per-line model instead?

One difficulty here is when you have a long line with one match at the beginning and one at the end.

But to extend the current model to that I think would only require us to change the two ranges to Range|Range[]

@jrieken
Copy link
Member Author

jrieken commented Sep 28, 2018

Should the API keep the one-result-per-line model instead?

That means to show more context (line above/below) the file needs to opened and I don't that feasible...

@roblourens
Copy link
Member

roblourens commented Sep 28, 2018

I'm referring to something else, not context lines. When you have two matches on the same line, the API will send two result objects. For your extension, it might be better if you can get a single result object that indicates the two matches. That's what I mean by one result per line.

Kai had the suggestion of just using exactly ripgrep's format: https://burntsushi.net/stuff/libripgrep/doc/grep_printer/struct.JSON.html

This is all kind of tangential but related because it probably doesn't make sense to show the same context lines twice when there are two matches on one line.

@roblourens roblourens added this to the October 2018 milestone Oct 3, 2018
@roblourens roblourens changed the title Make previewOptions more like -C option Text search should support context Oct 3, 2018
@roblourens
Copy link
Member

roblourens commented Oct 3, 2018

Opened #59919 for the other topic, the API change for context would look like this:

export interface TextSearchOptions extends SearchOptions {
        // ...

	/**
	 * Lines of context after the match
	 */
	afterContext?: number;

	/**
	 * Lines of context before the match
	 */
	beforeContext?: number;
}

export interface TextSearchContext {
        /**
	 * The uri for the matching document.
         */
	uri: Uri;

	/**
	 * One or more lines of text.
	 * previewOptions.charsPerLine applies to this
	 */
	text: string;

	/**
	 * The range of this context within the document.
	 */
	range: Range;
}

export type TextSearchResult = TextSearchLineMatch | TextSearchContext;

I think we would sort all of these results on the frontend.

@roblourens
Copy link
Member

Please verify by calling the findTextInFiles API from an extension with the context options, check that you get something reasonable.

@RMacfarlane RMacfarlane added the verified Verification succeeded label Nov 1, 2018
@vscodebot vscodebot bot locked and limited conversation to collaborators Dec 11, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-proposal feature-request Request for new features or functionality verification-needed Verification of issue is requested verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

3 participants