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

Multi-Cursor Mode v 2.0 #811

Merged
merged 80 commits into from
Sep 30, 2016
Merged

Multi-Cursor Mode v 2.0 #811

merged 80 commits into from
Sep 30, 2016

Conversation

johnfn
Copy link
Member

@johnfn johnfn commented Sep 28, 2016

The other one got way out of control because I was trying to refactor the entire codebase. Undoing all that insanity and cutting it down to basics!

Copy link
Member

@jpoon jpoon left a comment

Choose a reason for hiding this comment

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

Got through only a couple of the changes, but posting them first.

@@ -13,8 +13,12 @@
}
],

"vim.hlsearch": false,
Copy link
Member

@jpoon jpoon Sep 28, 2016

Choose a reason for hiding this comment

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

should we be checking in these settings? They are very much user-specific.

Copy link
Member Author

Choose a reason for hiding this comment

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

My settings.json is objectively the best though... 😉

Okay, okay, we should probably add it to .gitignore. :P

@@ -254,6 +258,17 @@ export abstract class BaseCommand extends BaseAction {
*/
isCompleteAction = true;

supportsMultiCursor = true;

multicursorIndex: number | undefined = undefined;
Copy link
Member

Choose a reason for hiding this comment

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

Nitpicking: I feel like you should capitalize the 'c'

Copy link
Member Author

Choose a reason for hiding this comment

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

hmm, i am being inconsistent here...

/**
* In multi-cursor mode, do we run this command for every cursor, or just once?
*/
public runsOnceForEveryCursor(): boolean {
Copy link
Member

Choose a reason for hiding this comment

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

Can this be a property instead?

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 why you did it this way.


for (let i = 0; i < timesToRepeat; i++) {
vimState = await this.exec(position, vimState);
let timesToRepeat = this.canBePrefixedWithCount ? vimState.recordedState.count || 1 : 1;
Copy link
Member

Choose a reason for hiding this comment

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

This is the same as line 288? Can we execute this once outside the if block?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, good point.

let resultingCursors: Range[] = [];
let i = 0;

const cursorsToIterateOver = vimState.allCursors
Copy link
Member

@jpoon jpoon Sep 28, 2016

Choose a reason for hiding this comment

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

Mind leaving a comment here about what's happening? This looks super magical to me :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I actually explained what was going on over here:

https://github.com/VSCodeVim/Vim/pull/811/files#diff-a7723f32fe6b05e135838f84e5c23c0fR965

Unfortunately I had to have similar logic in 2 places but I think the alternative is even worse. :P

@johnfn johnfn merged commit 70c2c07 into master Sep 30, 2016
@johnfn johnfn mentioned this pull request Sep 30, 2016
40 tasks
@jpoon jpoon deleted the multi-cursor-mode-simple branch October 5, 2016 08:10
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.

None yet

3 participants