search

jupyterlab-contrib / jupyterlab-vim

Vim notebook cell bindings for JupyterLab
https://jupyterlab-contrib.github.io/jupyterlab-vim.html
MIT License
661 stars 43 forks source link

enable/disable without refreshing page + add menu button + restructure #58

Closed ianhi closed 2 years ago

ianhi commented 2 years ago

This PR implements the "clean disable" discussed in https://github.com/jupyterlab-contrib/jupyterlab-vim/pull/32 (attn @mlucool + @asford) so that when the extension is enabled or disabled user's do not need to refresh the browser.

The commits do the following:

1 - Most of clean disable. Also reorganize the package to separate jlab setup commands and codemirror setup commands. In the moving to files I also cleaned up a few small things and added better typing. 2 - Switch the last active cell when (dis)enabled. Otherwise you would need to switch cells to get the full effect. I looked at a few other ways to do this but this seemed to be the best. I couldn't find a signal that would work, and iterating over the cells from INotebookPanel doesn't give you deep enough access to get to the editor.

3 - Added a menu button! To start I put it next to the Text Editor Keymap menu item, but open to putting it elsewhere if people feel strongly. ~4 - change plugin name to reflect new organization.~ i realized this would have implications for the npm package so I removed that change

clean-disable

github-actions[bot] commented 2 years ago

Binder :point_left: Launch a binder notebook on branch ianhi/jupyterlab-vim/clean-disable

ianhi commented 2 years ago

Still need to update the readme with information about this. In particular the ability to set the keybindings

ianhi commented 2 years ago

In https://github.com/jupyterlab-contrib/jupyterlab-vim/pull/58/commits/af50e68afa8c42b3eb8df55109153e00ba6c303b I took the opportunity to add vim: to the commands that we add so it's clear where they are coming from and to reduce odds of collision. Thoughts on this name? I think after we release this we won't be able to the change it again at risk of breaking people's set ups.

fcollonval commented 2 years ago

In af50e68 I took the opportunity to add vim: to the commands that we add so it's clear where they are coming from and to reduce odds of collision. Thoughts on this name? I think after we release this we won't be able to the change it again at risk of breaking people's set ups.

I would recommend to use the full package name to unsure uniqueness. But that may look too much :wink:

ianhi commented 2 years ago

I would recommend to use the full package name to unsure uniqueness. But that may look too much wink

This was exactly my thinking and in the end for me conciseness won out. This is currently (and also likely to continue to be) the only vim plugin (other then vimrc)

Also @fcollonval I noticed that the package name in the code doesn't match up with the repo name: https://github.com/jupyterlab-contrib/jupyterlab-vim/blob/12f4541290d6c94c7a5516cbe255b5f05386cb0c/package.json#L2

is it best not to change that in order to keep npm package continuity?

ianhi commented 2 years ago

I did some real life user research (i.e. asking people I know who use this) and the strong consensus was a preference for prefixing the commands with vim: so for now lets go with that. If it really is a problem (which I don't expect) I think it would be possible to have a deprecation cycle where using the old command pops up a warning dialog.

I still need to add docs tho

fcollonval commented 2 years ago

is it best not to change that in order to keep npm package continuity?

Yes

asford commented 2 years ago

This looks very useful. Anything I can do to assist in the merge?

ianhi commented 2 years ago

This looks very useful. Anything I can do to assist in the merge?

Bumping this was a good first step :) - scanning over it looks like the only remaining piece this needs is documentation. I didn't merge because if I don't force myself to add it here then I don't think i ever will.

Suggestions of language on documenting (or a real guarantee that you'd open a PR with documentation after this one) would be helpful! Otherwise I will try to do this this weekend (feel free to ping)

ianhi commented 2 years ago

ok! first pass at docs written. I'm reasonably happy with it but haven't properly proofread as it's late. Will try to look it over again tomorrow - but if anyone else can have glance that'd be great.

ianhi commented 2 years ago

Thanks @asford and @fcollonval ! Will work towards a release now

ianhi commented 2 years ago

somehow these got left out: https://github.com/jupyterlab-contrib/jupyterlab-vim/commit/51b305aaaf34795ec5a83dbc067883b1ef8aea12