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

Fix #438 - Limit the number of matches, and try to only recalculate when the searchString changes, or the document changes #494

Merged
merged 2 commits into from
Jul 27, 2016

Conversation

roblourens
Copy link
Contributor

document.version is 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.

@@ -129,50 +129,63 @@ export class VimSettings {
useCtrlKeys = false;
}

export enum SearchDirection {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Big thumbs up on making this an enum. There's no need to retain my old 1 and -1. The default values are fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@johnfn
Copy link
Member

johnfn commented Jul 21, 2016

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.

@rebornix
Copy link
Member

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)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what any of these TODO's mean :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This means that Vim rings a bell here (or flashes the screen or otherwise indicates an error has occurred).

@johnfn
Copy link
Member

johnfn commented Jul 26, 2016

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)
@johnfn johnfn merged commit 543b6c1 into VSCodeVim:master Jul 27, 2016
@johnfn
Copy link
Member

johnfn commented Jul 27, 2016

@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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants