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

Embedding Neovim for Ex commands #1725

Merged
merged 32 commits into from
May 24, 2017
Merged

Embedding Neovim for Ex commands #1725

merged 32 commits into from
May 24, 2017

Conversation

Chillee
Copy link
Member

@Chillee Chillee commented May 19, 2017

This is sick. This solves all of our command line related issues! (that edit text at least).

Issues this would fix as is (I tested them):
fixes #737, fixes #828, fixes #991, fixes #1032, fixes #1237, fixes #1401, fixes #1412, fixes #1517, fixes #1524, fixes #1525, fixes #1589, fixes #1611, fixes #1698, fixes #1723, fixes #1732

I added another variable to each BaseCommand, showing whether it's possible to be replaced by Neovim or not. Thus, this should theoretically pass all unit tests right now.

I also made a couple changes to the Readme, one of which fixes #1360 .

@Chillee Chillee changed the title Integrating Neovim for Ex commands WIP: Integrating Neovim for Ex commands May 19, 2017
This was referenced May 19, 2017
@Chillee Chillee changed the title WIP: Integrating Neovim for Ex commands Embedding Neovim for Ex commands May 20, 2017
.travis.yml Outdated
@@ -16,6 +16,9 @@ env:
secure: "tzpL/lQ2L2mVDS0sPY0LnJ3idspKmiOAuKFFfBZbBk4vA6NtJYveXILiskNDWK4p3JIWux/JBxgU3auXU4t7BZy7bMfUpTm0PpsMQfvDV2/rsf3QUQgYvROZkVpeH6b73hxxyTy0wfDHrb0SIUjB9IChUiSDIBjwteVAb/HuIWNKRQ6mbmkZKiQ3xHisFOG9xTkrEO31BfA+nKxUOTtXiZtoHTOWK9+H67QKd19BTRn/vJrwhTUsYqyOBAoFQjDjpAbhRNgs4YxHJz2jn1rmeE8kotf3LsfBrws07Mu9O9CnEcJKsqgJW+ezbHCO/HjLfpul/v0HtF/UQM00v5J2mDFz/ii8OWQ41TjYgkAHhtXIkMDQiM6K1x+gsaq0wm4QDX+Akg3r4sFqzhKsIuSLr1QTAMh9nn9G2asQvVJNNgH1QFvuLMF3bX1xp4l79/xoCEAWqlxx1mScYcmGFtYtQd107U5qPcHrR66vTMV5VqiZ0XapJympE+D5xIiSb4CTFGpl9+PSBk69MjynMEbGsLsudYWp6HZA5CyosxoStXxR1fD+ypqsSyzynSpThZ6IpFJ7Pk95GR3Z78CPhkNZvBvJ62xW/6/hAykWcelRHZy0N5XT2+HP+xcx77Fpqj8ZEAI6ECHNnnZ1KQROodu5LI06oZ2hiM1P1gdBx6xrljg="

before_install:
- sudo add-apt-repository ppa:neovim-ppa/stable;
- sudo apt-get update;
- sudo apt-get install neovim;

Choose a reason for hiding this comment

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

You can get the latest nightly build pre-built for travis ubuntu: https://github.com/neovim/bot-ci#setting-up-integration-builds

That avoids sudo:required.

static async command(vimState: VimState, command: string) {
const nvim = vimState.nvim;
await this.syncVSToVim(vimState);
command = ":" + command + "\n";

Choose a reason for hiding this comment

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

will command already have '<,'> by now? It needs that if there was a visual selection.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes it does. We're already handling it here: https://github.com/Chillee/Vim/blob/24b1a348fa286d8825275fb7ec47b70b8d9c3ee3/src/actions/commands/actions.ts#L1449

We need to handle it before we pass it to neovim so that the user can see it in their command line.


await nvim.callFunction("setpos", [".", [0, vimState.cursorPosition.line + 1, vimState.cursorPosition.character, false]]);
await nvim.callFunction("setpos", ["'>", [0, vimState.cursorPosition.line + 1, vimState.cursorPosition.character, false]]);
await nvim.callFunction("setpos", ["'<", [0, vimState.cursorStartPosition.line + 1, vimState.cursorStartPosition.character, false]]);
Copy link

@justinmk justinmk May 21, 2017

Choose a reason for hiding this comment

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

If the user has a selection in VScode: after setting the '< and '> marks, try gv to "restore" the visual selection in Nvim. Then when : is sent via nvim_input, it will pre-fill :'<,'> in the cmdline.

Copy link
Member Author

@Chillee Chillee May 21, 2017

Choose a reason for hiding this comment

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

As, we already fill in :'<, '>, I don't think this is necessary for command line purposes. But it will be good to recreate our "current state" in vscode as much as possible in neovim.

Also, wouldn't pressing the keys gv pollute the key history for neovim?

Thanks for the code review!

Copy link

@justinmk justinmk May 21, 2017

Choose a reason for hiding this comment

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

There isn't really a key history. It won't affect dot repeat if that's what you mean.

.travis.yml Outdated
@@ -1,7 +1,7 @@
notifications:
email: false

sudo: false
sudo: required

Choose a reason for hiding this comment

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

Shouldn't need this now.

@Chillee
Copy link
Member Author

Chillee commented May 21, 2017

I'm pretty sure the remaining broken tests are due to the testing framework, not actual broken functionality.

There's also a linting issue that's the result of the index.d.ts file for promised-neovim-client being inaccurate. We'll need to declare a custom typings file.

@Chillee Chillee merged commit 08370cf into VSCodeVim:master May 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment