Settings for enable/disable

[mlucool]

Fixes #24

Please finish the following when submitting a pull request:

• Add a line to History.md briefly documenting the change.

If this is a release, additionally do the following:

• Update the dependencies in package.json
• Run jlpm install to update yarn.lock

Thanks!

[github-actions]

👈 Launch a binder notebook on branch mlucool/jupyterlab-vim/disable

[ianhi]

Hi @mlucool thanks for doing this! (also nice to see you here). Overall I'm 👍 to this and thanks for taking initiative to clean up the structure a bit.

What do you think of adding an on/off toggle to the Settings menu, similar to how jupyterlab git allows for toggling Simple Staging via a menu?

[ianhi]

Also I think if you add FEAT or feature to the commit message then the automated version bump machinery will give us a bigger version bump (though @axelfahy would know better than I)

[mlucool]

Hi,

This is in the advanced settings menu:

If you mean the the top level settings menu, near "Text Editor Keymap", I'm a bit hesitant to until it's built into lab. The git plugin has it's own menu making easy access to changing options cleaner.

[ianhi]

If you mean the the top level settings menu, near "Text Editor Keymap", I'm a bit hesitant to until it's built into lab. The git plugin has it's own menu making easy access to changing options cleaner.

Fair enough. Although, fair warning I was one of the drivers of the "built into lab" and I haven't/won't have time for that for awhile.

[ianhi]

This is a clear step in the right direction but I think that we shouldn't merge this until we add code to clean up after ourselves when a user disables vim mode. There are currently two issues:

1. Cells that the user has already entered will remain in vim mode
2. All of our commands will remain. So the user won't get back the behavior of Esc taking them to the notebook command mode.

I think the fixes for both of these problems can be found in my (now stalled) PR adding vim support to core: jupyterlab/jupyterlab#9068 please feel free to extract them into this PR.

When the settings flip us from enabled to disabled we need to

1. Use the notebook tracker to loop over all existing cells and ensure that they are no longer in vim mode
2. Save the return statements from when we assign the commands and then call the lumino dispose method on them to get rid of them

[ianhi]

Expanding a little bit more.

For 1:
Essentially we need to take: https://github.com/jupyterlab/jupyterlab/blob/83a21d729d1c00d87e72674dbbd4f4287fe18f45/packages/codemirror-extension/src/index.ts#L293

For 2:
In that PR I pushed all throwing all the keymap setup into a single function: https://github.com/jupyterlab/jupyterlab/blob/83a21d729d1c00d87e72674dbbd4f4287fe18f45/packages/codemirror-extension/src/index.ts#L205 which would accept the previous keymap commands in order to remove them (this makes it easy to extend to supporting emacs etc) I think it may make sense to do something similar here.

[mlucool]

Thanks for the feedback @ianhi. Would you be able to pull those changes on top of this or would you like me to?

[ianhi]

Would you be able to pull those changes on top of this

I can do this at some point but can't guarantee any timeline as Jlab-vim is mostly a weekends only thing for me right now. (There are just too many interesting things in the world to learn about and to do 😞 )

[asford]

Would it be possible to merge this without the "clean disable" flow above as long as the toggle is buried in the settings menu?

I totally agree with @ianhi that a proper cleanup should be done if the setting is exposed as a drop-down in the UI, but a stopgap that requires changing the json settings document then refreshing page seems reasonable-ish in the time being?

[ianhi]

Would it be possible to merge this without the "clean disable" flow above as long as the toggle is buried in the settings menu?

That's a good idea. Prioritize the good over the perfect. I think we should add a comment to the settings file mentioning the need to refresh, but otherwise 👍 from me.

[ianhi]

I think we should add a comment to the settings file mentioning the need to refresh, but otherwise +1 from me.

oh I see that's already been done! I'll look into actually merging this tonight or tomorrow

[ianhi]

What's the purpose of adding rc.2? Won't that make it so that pip install doesn't pick up this version?

[mlucool]

We can remove that. I accidently left that during my test of publishing this to an internal registry

[mlucool]

oh I see that's already been done! I'll look into actually merging this tonight or tomorrow

Anything still holding up the merge?

[ianhi]

Anything still holding up the merge?

Apparently only my medium term memory. Sorry about that!

[ianhi]

I'm giving the binder one final test to be sure - but merging and subsequent automated release should be imminent

[ianhi]

Thanks @mlucool !

[ianhi]

pypi publish action here: https://github.com/axelfahy/jupyterlab-vim/actions/runs/797794893

[ianhi]

ack! it failed due to the same error as jupyter-widgets/widget-ts-cookiecutter#101
I think we need to pull in the changes from jupyterlab/extension-cookiecutter-ts#143 to get this to build properly

[mlucool]

I ended up needing to do that change a few times already today (which felt very silly to be forced into, although clearly an improvement in code), so one more was somewhat fast: #34