Sneak plugin #2307
Merged
Sneak plugin #2307
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…nd lastCommaRepeatableMovement to enable extensibility beyond t and f movements.
|
@jpotterm is this ready for review? |
|
@jpoon Yep! Was I supposed to indicate that before somehow? |
jpoon
reviewed
Jan 30, 2018
src/actions/commands/actions.ts
Outdated
| return super.doesActionApply(vimState, keysPressed) && !vimState.recordedState.operator; | ||
| return ( | ||
| super.doesActionApply(vimState, keysPressed) && | ||
| !Configuration.sneak && |
There was a problem hiding this comment.
Interesting, are we missing a !Configuration.surround check here?
There was a problem hiding this comment.
I don't think so. Surround seems to be working fine without it.
| public doesActionApply(vimState: VimState, keysPressed: string[]): boolean { | ||
| // Don't run if there's an operator because the Sneak plugin uses <operator>z | ||
| return ( | ||
| super.doesActionApply(vimState, keysPressed) && vimState.recordedState.operator === undefined |
There was a problem hiding this comment.
Do we need a !Configuration.sneak check here?
There was a problem hiding this comment.
Regardless of Sneak, I don't think the fold command works with an operator.
There was a problem hiding this comment.
If that's the case, then why did you need to add this check?
There was a problem hiding this comment.
Because currently the fold command runs even when there is an operator (but it shouldn't). So this is essentially fixing a bug, but one whose only effect is to conflict with Sneak.
src/actions/motion.ts
Outdated
| isRepeat: boolean; | ||
| } | ||
|
|
||
| export function createRepeatMovement( |
There was a problem hiding this comment.
Can you make this a class with constructor instead?
There was a problem hiding this comment.
Do you mean that instead of extending BaseMovement, MoveFindForward (for example) should extend a new class RepeatableMovement whose constructor takes two optional parameters (isRepeat and keysPressed)?
There was a problem hiding this comment.
Actually, can you add isRepeat to BaseMovement? No need to create a new class.
There was a problem hiding this comment.
Ok. Currently BaseMovement has no constructor, so should I create one, or keep the createRepeatMovement helper function?
There was a problem hiding this comment.
Constructor. Never been a fan of factory methods when that's the purpose of a constructor.
src/actions/plugins/sneak.ts
Outdated
| return ( | ||
| super.couldActionApply(vimState, keysPressed) && | ||
| Configuration.sneak && | ||
| keysPressed[0] === startingLetter |
There was a problem hiding this comment.
Do you need this check: keysPressed[0] === startingLetter? Should be taken care of in the base class.
There was a problem hiding this comment.
Yes, because startingLetter changes based on whether or not there is an operator.
src/actions/plugins/sneak.ts
Outdated
|
|
||
| return ( | ||
| super.couldActionApply(vimState, keysPressed) && | ||
| Configuration.sneak && |
There was a problem hiding this comment.
Can you reorder such that the Configuration.sneak check comes first?
test/plugins/sneak.test.ts
Outdated
| setup(async () => { | ||
| await setupWorkspace(); | ||
| modeHandler = await getAndUpdateModeHandler(); | ||
| Configuration.sneak = true; |
There was a problem hiding this comment.
FYI, i've made some changes so we can more easily change configurations in the UTs -- #2335. PR hasn't yet been merged yet.
There was a problem hiding this comment.
Please resolve merge conflicts and refer to https://github.com/VSCodeVim/Vim/blob/master/test/mode/modeVisual.test.ts#L689 for new pattern of changing configurations in the UTs.
src/actions/plugins/sneak.ts
Outdated
|
|
||
| @RegisterAction | ||
| class SneakForward extends BaseMovement { | ||
| keys = [['s', '<character>', '<character>'], ['z', '<character>', '<character>']]; |
There was a problem hiding this comment.
Will ['s', '<character>', '<character>'] not conflict with surround?
There was a problem hiding this comment.
No, because that only matches when there is no operator (see couldActionApply). With an operator it uses z instead. Surround uses ys without an operator and s with an operator.
There was a problem hiding this comment.
I'm not familiar with the sneak plugin. Can we use only z? Might be confusing for folks that it is sometimes s and other times z.
There was a problem hiding this comment.
No, we can't use z because that's used for folds. I think the main audience for this plugin are people who have used vim-sneak and they'll be used to it. I think it would be more confusing for people if the Sneak plugin didn't act like vim-sneak.
Admittedly it's not a perfect design, but I can't think of a better one (at least without changing a bunch of other vim keybindings). It's always tricky to find unused keys in vim. Surround's ys vs <operator>s vs S is also confusing, but you can get used to it.
jpoon
requested changes
Jan 30, 2018
Travis tests have failedHey Jonathan Potter, Node.js: 8gulp forceprettiernpm run buildnpm test --silent; |
|
@jpoon I think I've made all the changes you requested! I'm not using VSCodeVim anymore so it's up to you if you still want to merge this pull request. Hopefully this will be useful to other people and I'm happy to make any changes required to get it merged, but I probably won't be available for any bug fixes that need to happen further down the road. |
|
@jpotterm LGTM. Can you fix the merge conflicts? |
|
@jpoon I don't see any merge conflicts. Github says "This branch has no conflicts with the base branch". Am I looking in the wrong place? |
|
@jpoon For w.e. reason, we can't rebase, but merging/squashing the PR should work fine. Since it looks like you're ok with the PR, I'm just gonna merge it. |
|
I can't find "vim.sneak" options in settings.json |
|
@muhajirframe It just got merged, but we haven't released a new update yet. |
|
When will you release it? |
|
Weird, I don't recall GH giving me that option. Thanks @Chillee for merging. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Implements the sneak plugin (https://github.com/justinmk/vim-sneak).
Supports
s,S,<operator>z,<operator>Z,;, and,.Does not support preventing additions to the jump list or highlighting matches because of limitations in VSCodeVim and because those features aren't very important.
Closes #1268
As part of this pull request I also changed the way
lastRepeatableMovementis handled. I made it more extensible so that the Sneak plugin could use it too. The way it worked before it was really only useful forfandt.