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.

Sign up for GitHub

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

Jump to bottom

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
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
47367f1
Fix #438 - Limit the number of matches,
roblourens Jul 21, 2016
e4de85e
Change to take an options object,
roblourens Jul 26, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 5 additions & 5 deletions src/actions/actions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { VimSpecialCommands, VimState, SearchState } from './../mode/modeHandler';
import { VimSpecialCommands, VimState, SearchState, SearchDirection } from './../mode/modeHandler';
import { ModeName } from './../mode/mode';
import { TextEditor } from './../textEditor';
import { Register, RegisterMode } from './../register/register';
Expand Down Expand Up @@ -490,7 +490,7 @@ class CommandStar extends BaseCommand {
public async exec(position: Position, vimState: VimState): Promise<VimState> {
const currentWord = CommandStar.GetWordAtPosition(position);

vimState.searchState = new SearchState(+1, vimState.cursorPosition, currentWord);
vimState.searchState = new SearchState(SearchDirection.Forward, vimState.cursorPosition, currentWord);

do {
vimState.cursorPosition = vimState.searchState.getNextSearchMatchPosition(vimState.cursorPosition).pos;
Expand All @@ -517,7 +517,7 @@ class CommandHash extends BaseCommand {
public async exec(position: Position, vimState: VimState): Promise<VimState> {
const currentWord = CommandStar.GetWordAtPosition(position);

vimState.searchState = new SearchState(-1, vimState.cursorPosition, currentWord);
vimState.searchState = new SearchState(SearchDirection.Backward, vimState.cursorPosition, currentWord);

do {
// use getWordLeft() on position to start at the beginning of the word.
Expand Down Expand Up @@ -606,7 +606,7 @@ export class CommandSearchForwards extends BaseCommand {
isMotion = true;

public async exec(position: Position, vimState: VimState): Promise<VimState> {
vimState.searchState = new SearchState(+1, vimState.cursorPosition);
vimState.searchState = new SearchState(SearchDirection.Forward, vimState.cursorPosition);
vimState.currentMode = ModeName.SearchInProgressMode;

return vimState;
Expand All @@ -620,7 +620,7 @@ export class CommandSearchBackwards extends BaseCommand {
isMotion = true;

public async exec(position: Position, vimState: VimState): Promise<VimState> {
vimState.searchState = new SearchState(-1, vimState.cursorPosition);
vimState.searchState = new SearchState(SearchDirection.Backward, vimState.cursorPosition);
vimState.currentMode = ModeName.SearchInProgressMode;

return vimState;
Expand Down
87 changes: 52 additions & 35 deletions src/mode/modeHandler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,50 +128,68 @@ 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.

Forward = 1,
Backward = -1
}

export class SearchState {
private static readonly MAX_SEARCH_RANGES = 1000;

/**
* Every range in the document that matches the search string.
*/
public matchRanges: vscode.Range[] = [];
private _matchRanges: vscode.Range[] = [];
public get matchRanges(): vscode.Range[] {
Copy link
Member

Choose a reason for hiding this comment

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

You refactored this into a getter but I don't think you ever used the getter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The public prop was used elsewhere, but only to read, so I changed it to a public getter. This and some of the other changes are just opinionated cleanup and totally not necessary, so I can revert them if you folks prefer the original way! I just like to restrict public access as much as possible.

Copy link
Member

Choose a reason for hiding this comment

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

I do like the idea of accessibilty restriction and per microsoft/TypeScript#2845, accessors for the same member name must specify the same accessibility, we can't have a public getter and a private setter at the same time, so accessing the private member to do modification is the best way.

Copy link
Member

Choose a reason for hiding this comment

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

Above comment only works for https://github.com/VSCodeVim/Vim/pull/494/files#diff-a7723f32fe6b05e135838f84e5c23c0fR176 . Even though matchRanges is readonly, you can't re-initialize it but you can always change its content, like pushing more elements to it.

Copy link
Member

@johnfn johnfn Jul 23, 2016 •
edited

Choose a reason for hiding this comment

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

@roblourens I would suggest to use the readonly modifier in that case - it's a new modifier introduced in Typescript 2.0 😄

(Haven't gotten around to retrofitting it into the rest of the code base - we only upgraded very recently.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, we still reinitialize it internally

Copy link
Member

Choose a reason for hiding this comment

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

Ah, my bad, I assumed readonly was getter without setter.

return this._matchRanges;
}

private _searchCursorStartPosition: Position;
public get searchCursorStartPosition(): Position {
return this._searchCursorStartPosition;
}

private _matchesDocVersion: number;
private _searchDirection: SearchDirection = SearchDirection.Forward;

private _searchString = "";
public get searchString(): string {
return this._searchString;
}

public searchCursorStartPosition: Position;

/**
* 1 === forward
* -1 === backward
*/
public searchDirection = 1;


public set searchString(search: string){
this._searchString = search;

this._recalculateSearchRanges();
if (this._searchString !== search) {
this._searchString = search;
this._recalculateSearchRanges({ forceRecalc: true });
}
}

private _recalculateSearchRanges(): void {
private _recalculateSearchRanges({ forceRecalc }: { forceRecalc?: boolean } = {}): void {
const search = this.searchString;

if (search === "") { return; }

// Calculate and store all matching ranges
this.matchRanges = [];
if (this._matchesDocVersion !== TextEditor.getDocumentVersion() || forceRecalc) {
// Calculate and store all matching ranges
this._matchesDocVersion = TextEditor.getDocumentVersion();
this._matchRanges = [];

for (let lineIdx = 0; lineIdx < TextEditor.getLineCount(); lineIdx++) {
const line = TextEditor.getLineAt(new Position(lineIdx, 0)).text;
outer:
for (let lineIdx = 0; lineIdx < TextEditor.getLineCount(); lineIdx++) {
const line = TextEditor.getLineAt(new Position(lineIdx, 0)).text;

let i = line.indexOf(search);
let i = line.indexOf(search);

for (; i !== -1; i = line.indexOf(search, i + search.length)) {
this.matchRanges.push(new vscode.Range(
new Position(lineIdx, i),
new Position(lineIdx, i + search.length)
));
for (; i !== -1; i = line.indexOf(search, i + search.length)) {
if (this._matchRanges.length >= SearchState.MAX_SEARCH_RANGES) {
break outer;
}

this._matchRanges.push(new vscode.Range(
new Position(lineIdx, i),
new Position(lineIdx, i + search.length)
));
}
}
}
}
Expand All @@ -184,41 +202,41 @@ export class SearchState {
public getNextSearchMatchPosition(startPosition: Position, direction = 1): { pos: Position, match: boolean} {
this._recalculateSearchRanges();

if (this.matchRanges.length === 0) {
if (this._matchRanges.length === 0) {
// TODO(bell)
return { pos: startPosition, match: false };
}

const effectiveDirection = direction * this.searchDirection;
const effectiveDirection = direction * this._searchDirection;

if (effectiveDirection === 1) {
for (let matchRange of this.matchRanges) {
if (effectiveDirection === SearchDirection.Forward) {
for (let matchRange of this._matchRanges) {
if (matchRange.start.compareTo(startPosition) > 0) {
return { pos: Position.FromVSCodePosition(matchRange.start), match: true };
}
}

// Wrap around
// TODO(bell)
return { pos: Position.FromVSCodePosition(this.matchRanges[0].start), match: true };
return { pos: Position.FromVSCodePosition(this._matchRanges[0].start), match: true };
} 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).

return {
pos: Position.FromVSCodePosition(this.matchRanges[this.matchRanges.length - 1].start),
pos: Position.FromVSCodePosition(this._matchRanges[this._matchRanges.length - 1].start),
match: true
};
}
}

constructor(direction: number, startPosition: Position, searchString = "") {
this.searchDirection = direction;
this.searchCursorStartPosition = startPosition;
constructor(direction: SearchDirection, startPosition: Position, searchString = "") {
this._searchDirection = direction;
this._searchCursorStartPosition = startPosition;
this.searchString = searchString;
}
}
Expand Down Expand Up @@ -576,7 +594,6 @@ export class ModeHandler implements vscode.Disposable {
}

// Update view

await this.updateView(vimState);

return vimState;
Expand Down
4 changes: 4 additions & 0 deletions src/textEditor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,10 @@ export class TextEditor {
});
}

static getDocumentVersion(): number {
return vscode.window.activeTextEditor.document.version;
}

/**
* Removes all text in the entire document.
*/
Expand Down