Fixes #1575: Adds support for searching for strings with newlines #1603
Conversation
src/state/searchState.ts
Outdated
| // Start at the current line and wrap the document if we hit the end. | ||
| outer: | ||
| do { | ||
| const line = TextEditor.getLineAt(new Position(lineIdx, 0)).text; | ||
| let rangeStart = new Position(lineIdx, 0); |
There was a problem hiding this comment.
Minor I know, but can we have a convention of using const when the variable doesn't change?
src/state/searchState.ts
Outdated
| let rangeStart = new Position(lineIdx, 0); | ||
| let rangeEnd = rangeStart.getLineEnd(); | ||
| let lineLengths = []; | ||
| for (var i = 0; i < numNewLines; i++) { |
src/state/searchState.ts
Outdated
| let result = regex.exec(line); | ||
|
|
||
| while (result) { | ||
| if (this._matchRanges.length >= SearchState.MAX_SEARCH_RANGES) { | ||
| break outer; | ||
| } | ||
| let resultToPosition = function(resultIdx: number, lineIdx: number, lineLengths: Array<number>, result: RegExpExecArray){ |
There was a problem hiding this comment.
As a further style suggestion, it's good to prefer => over function in the general case.
src/state/searchState.ts
Outdated
| @@ -92,20 +92,38 @@ export class SearchState { | |||
|
|
|||
| let lineIdx = lineToStartAt; | |||
|
|
|||
| let numNewLines = (searchRE.match(/\\n/g) || []).length; | |||
src/state/searchState.ts
Outdated
| let result = regex.exec(line); | ||
|
|
||
| while (result) { | ||
| if (this._matchRanges.length >= SearchState.MAX_SEARCH_RANGES) { | ||
| break outer; | ||
| } | ||
| let resultToPosition = function(resultIdx: number, lineIdx: number, lineLengths: Array<number>, result: RegExpExecArray){ | ||
| let cur = resultIdx; |
There was a problem hiding this comment.
Could we have a slightly better name than cur, like positionInAggregateLines or something?
|
So there is a definitely a noticeable delay. I'll try to work on the performance. Just wondering, is it a bad idea to load the entire file into one string? Also, would it be viable to find search results as needed? As in, only find the results on the screen, and find more as necessary? Actually, looking around, I think it may be worth trying to utilize vscode's default find functionality. They seem to have already ran into this issue and fixed it. Their solution seems kind of similar to the one I was planning on implementing, but they have the benefit of having direct access to the text model. |
|
Alright, so I implemented a different model that searches the whole text instead of line by line. This is way faster (there's pretty much no drop in speed so far as I can tell, with an arbitrarily high number of newlines). |
|
Alright. I think the current PR is in a mergeable state. The performance is very good, and I've ironed out all the other miscellaneous bugs. I think the failing test is just the test server screwing up. The previous commit passed all tests (only failed due to linting issue), and the final commit shouldn't have changed anything related to that. |
Chillee
changed the title
Basically, the general approach is that we count how many newlines there are in their search string, and expand our search beyond line by line accordingly.
Thus, if their string contains two newline characters, then we check two lines at a time, and so on.
Accordingly, if there are no newline characters, this shouldn't affect performance at all, and performance should then degrade linearly with the amount of newlines (which makes sense).
One thing that it's missing is that Vscode doesn't seem to support highlighting the "newline" character. That makes newlines pretty invisible.
I've also added a test.
PS: You'll notice the commit that made the command line persistent, PR #1601. I accidentally checked it out and don't know how to delete a commit in the past, so I simply reverted it and then squashed the commit. I hope that's not an issue if both PR's are merged...