-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 multiple issues with expand selection commands and pair/block movement #2921
Conversation
…name symbols for compatibility with `matcher` changes
…ly expands selection to enclosing pairs.
…n `tagMatcher.ts`. This fixes expanding selection over multiple tags. Maintain selection start position in `MoveTagMatch` of `motion.ts` on failure.
…iling to enter surround mode.
…ntically accurate in in `surround.ts`.
…by passing `vimState` to `findPairedChar` method of `PairMatcher`
… selection in `tagMatcher.ts`
…t for changes in `tagMatcher.ts` and `matcher.ts`
…mmutable as possible when dry-running multiple block selection movements
src/actions/motion.ts
Outdated
let recordedState = vimState.recordedState; | ||
let result: Position | IMovement = new Position(0, 0); // bogus init to satisfy typechecker | ||
|
||
if (count < 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know we've had this pattern but we should really move this sort of check somewhere outside this function (on a different PR).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking of abstracting more of the code in this function to private methods. This would probably allow us to add a couple of classes between BaseMovement
and its subclasses and reduce duplication. That might also help in the future to refactor how count-prefixes are handled before reaching BaseMovement
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Were you thinking of doing that in this PR or sometime later?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll try to cleanup some of the changes in this PR and sprinkle in some more comments. If you have any other concerns or suggestions I'd like to organize what I need to do so I can plan some time to get to it. I think the refactor I proposed fits well with this PR since I ended up duplicating execActionWithCount
a couple times where I needed to override it.
@@ -3817,7 +3817,7 @@ class ActionChangeChar extends BaseCommand { | |||
|
|||
public couldActionApply(vimState: VimState, keysPressed: string[]): boolean { | |||
return ( | |||
super.doesActionApply(vimState, keysPressed) && | |||
super.couldActionApply(vimState, keysPressed) && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch.
Travis tests have failedHey @xmbhasin, Node.js: 8npm test --silent
|
Travis tests have failedHey @xmbhasin, Node.js: 8npm test --silent
|
Travis tests have failedHey @xmbhasin, Node.js: 8npm test --silent
|
Travis tests have failedHey @xmbhasin, Node.js: 8npm test --silent
|
I think I'm ready for a final review of this PR. I would have liked to fix count-prefixed actions with inner tag ( |
Travis tests have failedHey @xmbhasin, Node.js: 8npm test --silent
|
I'll merge when build finishes. |
What this PR does / why we need it:
Fixes multiple issues with bracket, tag, and other pair movement and selection commands. Adds backtick and angle brackets support to
af
command. Fixesaf
jumping to nonenclosing blocks. Addsaf
expanding over multiple types of blocks in succession. Fixesafd
commands deleting and then returning cursor to the wrong position. Fixes expanding repeatedly over the same type of block ( eg usingv3a}
over{{{|}}}
). Adds a number of new tests around the behaviour of these changes.Refactors
matcher.ts
and improves its performance.Should get us closer to implementing more of the wildfire plugin as requested in #2834 in the form of
af
.Which issue(s) this PR fixes
Should fix #2907 and fix #2506.
Special notes for your reviewer:
You can test out the difference these commits make in a file such as this: