Fix #438 - Limit the number of matches, and try to only recalculate when the searchString changes, or the document changes #494
Conversation
| @@ -129,50 +129,63 @@ export class VimSettings { | |||
| useCtrlKeys = false; | |||
| } | |||
|
|
|||
| export enum SearchDirection { | |||
There was a problem hiding this comment.
Big thumbs up on making this an enum. There's no need to retain my old 1 and -1. The default values are fine.
There was a problem hiding this comment.
Thanks! The -1 and +1 are nice because elsewhere you're multiplying two directions to compute the 'effective direction', and that makes it easy. I like that pattern personally.
There was a problem hiding this comment.
Ah, I see. Hmm, if we're using the values of an enum directly, I'm not sure if this change actually improves readability, since that is not really standard. What do you think?
There was a problem hiding this comment.
I think it improves readability from the outside, more than having to determine the magic numbers 1 and -1. The particular values are only used internally.
|
This is a really awesome change! I'll get to looking over it more seriously and merging it when I get back from vacation early next week. |
|
It looks perfect! |
| } else { | ||
| for (let matchRange of this.matchRanges.slice(0).reverse()) { | ||
| for (let matchRange of this._matchRanges.slice(0).reverse()) { | ||
| if (matchRange.start.compareTo(startPosition) < 0) { | ||
| return { pos: Position.FromVSCodePosition(matchRange.start), match: true }; | ||
| } | ||
| } | ||
|
|
||
| // TODO(bell) |
There was a problem hiding this comment.
I'm not sure what any of these TODO's mean :)
There was a problem hiding this comment.
This means that Vim rings a bell here (or flashes the screen or otherwise indicates an error has occurred).
|
Sorry for the slower responses, I was on vacation. This change is awesome. Thanks a bunch for taking it on. I have a few minor suggestions, and after that we should be good to go here. |
and try to only recalculate when the searchString changes, or the document changes
and move MAX_SEARCH_RANGES check (PR feedback)
|
@roblourens Again, thanks for the awesome work on this PR. This is definitely one of the things that has been nagging at me - this is a huge improvement! |
document.versionis incremented each time the document is changed. Using a regex might be faster too, and since/is supposed to be a regex search, that's another thing that could be done. But you'll need to handle invalid regexes and stuff and I didn't want to do it in this change.