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

CHE-9244 Add VsCode vim extension to the plugin registry #320

Merged
merged 4 commits into from
Dec 18, 2019
Merged

CHE-9244 Add VsCode vim extension to the plugin registry #320

merged 4 commits into from
Dec 18, 2019

Conversation

vinokurig
Copy link
Contributor

What does this PR do?

Add VsCode vim extension to the plugin registry

eclipse-che/che#9244

@vinokurig vinokurig force-pushed the vim_plugin branch 2 times, most recently from e314045 to 55a6b5c Compare December 11, 2019 12:21
@benoitf
Copy link
Contributor

benoitf commented Dec 11, 2019 •
edited
Loading

would be nice to have only lowercase and use already existing ms-code folder
v3/plugins/vsCodeVim --> v3/plugins/ms-vscode/vim (not microsoft extension, read too fast)

would be nice to have only lowercase : v3/plugins/vs-code-vim or vscodevim as it's the publisher id on marketplace

@vinokurig
Copy link
Contributor Author

done

@benoitf
Copy link
Contributor

benoitf commented Dec 11, 2019

@vinokurig sorry I edited my comment, it's not a microsoft one

@vinokurig
Copy link
Contributor Author

vinokurig commented Dec 11, 2019 •
edited
Loading

Yes, the publisher is VsCodeVim, so what folder name should we choose? I see, vscodevim looks good for me

@benoitf
Copy link
Contributor

benoitf commented Dec 11, 2019

let's use like on VS Code marketplace: https://github.com/VSCodeVim/Vim/blob/master/package.json#L7

and then use vscodevim as folder name/publisher

@vinokurig
CHE-9244 Add Vscode vim extension to the plugin registry
60e271b 
Signed-off-by: Igor Vinokur <ivinokur@redhat.com>
@vinokurig
Copy link
Contributor Author

done

amisevsk
amisevsk reviewed Dec 11, 2019
View reviewed changes
Copy link
Contributor

@amisevsk amisevsk left a comment

Choose a reason for hiding this comment

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

I tested the plugin (on Che 7.3.2 -- will test with latest as well), and while the plugin installs and I can see the settings / commands it contributes, I can't seem to activate vim mode. Is there a step I'm missing?

v3/plugins/vscodevim/vim/v1.12.0/meta.yaml Outdated
publisher: vscodevim
apiVersion: v2
name: vim
version: v1.12.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor nitpick/optional request: the other plugins generally avoid using v in version numbers. I'd prefer to stick to a major.minor.bugfix format where possible (in case we need to process version numbers at some later point).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@amisevsk
Copy link
Contributor

Digging in a little deeper, I tried running on nightly, adding the plugin to the default java-maven devfile, and I see the error log

Extension-Host:Error on activation of vim Error: Unable to find .vimrc file
    at VimrcImpl.<anonymous> (/tmp/vscode-unpacked/vscodevim.vim.v1.12.0.wdhjahwzof.vim-1.12.0.vsix/extension/out/src/configuration/vimrc.js:27:23)
    at Generator.next (<anonymous>)
    at /tmp/vscode-unpacked/vscodevim.vim.v1.12.0.wdhjahwzof.vim-1.12.0.vsix/extension/out/src/configuration/vimrc.js:8:71
    at new Promise (<anonymous>)
    at __awaiter (/tmp/vscode-unpacked/vscodevim.vim.v1.12.0.wdhjahwzof.vim-1.12.0.vsix/extension/out/src/configuration/vimrc.js:4:12)
    at VimrcImpl.load (/tmp/vscode-unpacked/vscodevim.vim.v1.12.0.wdhjahwzof.vim-1.12.0.vsix/extension/out/src/configuration/vimrc.js:21:16)
    at Configuration.<anonymous> (/tmp/vscode-unpacked/vscodevim.vim.v1.12.0.wdhjahwzof.vim-1.12.0.vsix/extension/out/src/configuration/configuration.js:172:37)
    at Generator.next (<anonymous>)
    at /tmp/vscode-unpacked/vscodevim.vim.v1.12.0.wdhjahwzof.vim-1.12.0.vsix/extension/out/src/configuration/configuration.js:14:71
    at new Promise (<anonymous>)

@benoitf
Copy link
Contributor

benoitf commented Dec 11, 2019

please note that lot of bugs have only be fixed in theia upstream this morning
eclipse-theia/theia#6687

@vinokurig
fixup! CHE-9244 Add Vscode vim extension to the plugin registry
c790774 
Signed-off-by: Igor Vinokur <ivinokur@redhat.com>
@vinokurig
Copy link
Contributor Author

vinokurig commented Dec 12, 2019 •
edited
Loading

@amisevsk @benoitf
I've updated the plugin source to latest and the error is gone. The latest version which is available in the vscode marketplace is 1.12.2, but the latest version in the Vim's GitHub repository release page is 1.12.0. In order to have a download link for the latest 1.12.2 version I've forked the VsCodeVim/Vim repository to the che-incubator and added the latest 1.12.2release version.

amisevsk
amisevsk reviewed Dec 12, 2019
View reviewed changes
Copy link
Contributor

@amisevsk amisevsk left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Tested again and the plugin installs and works as expected. However:

  • Typing with the plugin enabled is visibly slow -- it's not hard to type faster than letters appear on screen.
  • It doesn't seem to be possible to fully disable the plugin; even after using the Vim: Toggle Vim mode command, the plugin is still intercepting certain keys (e.g. g).

@vinokurig
Copy link
Contributor Author

@amisevsk

Typing with the plugin enabled is visibly slow -- it's not hard to type faster than letters appear on screen.

eclipse-theia/theia#6687 (review)

It doesn't seem to be possible to fully disable the plugin; even after using the Vim: Toggle Vim mode command, the plugin is still intercepting certain keys (e.g. g).

Fixed that in eclipse-theia/theia#6752

@amisevsk
Copy link
Contributor

@vinokurig thanks, ping me once changes are pushed, in case I forget to re-review.

@vinokurig
Copy link
Contributor Author

@amisevsk eclipse-theia/theia#6752 has been merged to Theia's master, but am not sure that they are present in the latest che-theia docker image at the moment

@vinokurig
Copy link
Contributor Author

@amisevsk @benoitf downgraded the plugin to 1.11.3 because of the bug in the latest version: VSCodeVim/Vim#4396

amisevsk
amisevsk approved these changes Dec 17, 2019
View reviewed changes
Copy link
Contributor

@amisevsk amisevsk left a comment

Choose a reason for hiding this comment

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

LGTM, no issues in my basic usage.

@vinokurig vinokurig merged commit cf52f7b into master Dec 18, 2019
@vinokurig vinokurig mentioned this pull request Dec 18, 2019
Closed
monaka referenced this pull request in PizzaFactory/che-plugin-registry Jan 4, 2020
@vinokurig @monaka
CHE-9244 Add VsCode vim extension to the plugin registry (#320)
ec60889 
* CHE-9244 Add Vscode vim extension to the plugin registry

Signed-off-by: Igor Vinokur <ivinokur@redhat.com>

* fixup! CHE-9244 Add Vscode vim extension to the plugin registry

Signed-off-by: Igor Vinokur <ivinokur@redhat.com>

* fixup! CHE-9244 Add Vscode vim extension to the plugin registry
@nickboldt nickboldt deleted the vim_plugin branch February 12, 2020 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benoitf benoitf benoitf approved these changes

@amisevsk amisevsk amisevsk approved these changes

@ibuziuk ibuziuk Awaiting requested review from ibuziuk

@l0rd l0rd Awaiting requested review from l0rd

@nickboldt nickboldt Awaiting requested review from nickboldt

@rhopp rhopp Awaiting requested review from rhopp

@skabashnyuk skabashnyuk Awaiting requested review from skabashnyuk

@vparfonov vparfonov Awaiting requested review from vparfonov

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@vinokurig @benoitf @amisevsk