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
vim support in the notebook #9068
Conversation
Thanks for making a pull request to JupyterLab! To try out this branch on binder, follow this link: |
Thx @ianhi for this and really sorry to have not been able to move forward my initial attempt by lack of time and other priorities. I am happy to review this PR, probably begin next week. |
Thanks! Also, I forgot to add: this fixes: #4765 |
@ianhi Did a quick review of the changes and look good so far. Tried also to run and see how it was behavings. Maybe an issue on my local env, but with your branch, I can not see the editor keymap menu, |
This is fixed in 3.0rc1. |
Thx a lot @jasongrout - @ianhi, could you please fix the minor conflict, update to latest master, then I will try your branch. thx. |
@echarles rebased on the latest master should be good now! Thanks for looking at it |
Latest push works fine. How can I exit from VIM insert mode? |
Ah good catch. I accidentally removed the activeCell Tracker when I moved to using the new signal. Should be a quick fix |
Ok it should work now. A few thoughts:
|
peeking at the test failures I don't think they are related to this PR - though I stand to be corrected |
I added support for changing the block comment keyboard shortcut so this should fix: #4778 If thats too much scope creep I can remove that commit. |
if (keyMap === 'vim') { | ||
// This setup for vim in the notebook is derived from extension built by |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could the content of this if
statement be mode to a separated ts file that would host static methods for vim, but also for upcoming keymaps. This would allow keeping this index.ts more readable and would allow keypmap implementer to have a place to focus on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good - will do
Thx for this. I see you a lot on the issues in various repos. I have tried the latest push and exiting from insert mode works fine. My next question is how to exit from vim mode to go back to the jupyterlab command mode, e.g. I am in a cell in vim mode with the large green cursor blinking, and I want to delete that cell. In terms of implementation, I have added a comment to create a separated file where the keymap would be hosted, instead of putting it in the index.ts. |
Hi this might not be the best place to ask this, but could you guys please add relative numbers as well? |
@echarles @quantum-booty I think that relative line numbers probably belong in an extension (though I'm no voice of authority on this). Based on codemirror/codemirror5#4116 it should be possible to have relative line numbers for any editor mode, though it is likely most useful in vim mode. I would be happy to add it to the vimrc extension if the newCellSignal from this PR ends up in jlab. If that happens and you want to open a PR to ianhi/vimrc to do that then I'd be happy to walk you through it. |
One more thought: I'm guessing that this is too late to have any chance of landing in 3.0, which is very reasonable. If that is the case what do you think about extracting the fix allowing the block comment key to be a setting? Reading through the issues about that it seems to be an issue primarily for people using non-english keyboards so it seems to go hand in hand with the internationalization efforts. |
@ianhi There is a slot to get this merged in 3.0. We have discussed a bit about this during our last weekly meeting (open to everyone, included you of course, you could do a demo of your work, we love demos 👍 ), and there has been good sponsorship from @telamonian I would prefer not cutting this in smaller PR, so having it merged in one shot would be better. Do you think you could refactor a bit to have the vim related code in a separated file and have a lighter index.ts? |
eb287e7
to
ebab15c
Compare
@ianhi I link here the questions posted by @jasongrout on #8038 (comment) I can help address them, but in first instance, it would be great if you could you reply on this PR to those. Those questions are independent to the release aspects, so we should ensure we correctly address them and get buy-in of all. |
Also, just like a picture is worth a thousand words, a demo is worth a million words. Demonstrating your work during the weekly meeting would be a great facilitator for the vim case. |
Yes. And to be clear, I'm really glad someone is tackling these difficult problems - so thanks so much for the work you've been doing. I'm excited to see a demo! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
THANK YOU @ianhi and @echarles !! ❤️ It is fantastic to see vim-notebook bindings being considered to become part of the default JuptyerLab experience!
I just tried this out briefly and everything works as expected! I think the decision to narrow down the bindings to the essentials and leave out some from the extension (like Ctrl + O
) is great, and it promotes using the JupyterLab bindings in Jupyter command mode which are already really good. The addition of Ctrl + C
to copy to the system clipboard is also really useful (and in the same spirit as that Ctrl + S
works for saving also in vim mode), thank you for that!
I would love for this to land in 3.0, but if that is not possible I am happy to know that I can run this branch until it is merged! And I will keep running it for now an let you know if I run into anything unexpected.
The only question that came up right now is what the preferred approach is for modifying vim key mappings. Say I want to remap Y
to y$
, should I do this in the advanced settings manager in JupyterLab, edit the source file directly, or use the .vimrc extension?
"type": "string", | ||
"title": "Block Comment on MacOS keyboard", | ||
"description": "Key command for block comment with MacOS", | ||
"default": "Cmd-/" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does JupyterLab support using Accel
in this file to avoid having two different shortcuts (if that is a benefit)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so, but I just re-interpreted what jlab was already doing into being settings:
jupyterlab/packages/codemirror/src/factory.ts
Lines 40 to 41 in 14776bb
'Cmd-/': 'toggleComment', | |
'Ctrl-/': 'toggleComment', |
Is accel = ctrl
or alt
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok if this is what JupyterLab has elsewhere then just keep it as is I guess unless someone else with more knowledge chimes in. I believe Accel
is set to Ctrl
on Win/Linux and Cmd
on macOS.
commands; | ||
|
||
let toggleComment = 'Ctrl-/'; | ||
let toggleCommentMac = 'Cmd-/'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment about Accel
here as above.
keymapRemoval = [ | ||
commands.addKeyBinding({ | ||
selector: '.jp-Notebook.jp-mod-editMode', | ||
keys: ['Ctrl J'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is Ctrl
also the expected key to use here for Mac people? Or would they use Cmd
for this, so Accel
might be better? I don't use Mac much so I don't know myself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't have a mac so I never thought about this. This is what the jupyterlab_vim
extension currently does so I just continued that. https://github.com/axelfahy/jupyterlab-vim/blob/c4e43f940ef4be4c961608b6192412d2f3a33d1f/src/index.ts#L466-L470
probably could be improved though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 let's keep the same binding as what the jupyter_lab_vim
extension had since that's what people would be used to.
@echarles and @jasongrout I have thoughts and maybe answers to the questions in the other issue. However, these are all tricky issues so those thoughts will require time I don't have today in order to write them coherently (it's gorgeous out 🌞 and I have grandparents that want me to help them in the garden). So I'll definitely respond, but probably no earlier than tomorrow.
See also https://github.com/ianhi/jupyterlab_vim-system-clipboard-support - lets you yank to the
I had a long discussion with @ejisoo over at ianhi/jupyterlab-vimrc#7 about where vim settings should live. It's tricky because there are both codemirror level settings and jupyter level settings and it's not clear whether people should use the existing shortcuts settings as well as vimrc have one extension manage all vim settings. I think if this PR went in then you would do remappings with Actually, maybe the best course of action would be forming a |
@echarles @jasongrout life got away from me - but here are some thoughts about the issues raised.
As far as I can tell the only other vim extensions in use for jupyterlab are the two that I wrote (though I could be wrong) and both of them should work seamlessly with this PR.
Not sure this was addressed to me but here's my answer regardless. The most basic (truest?) answer is that I saw that there was some movement towards and jumped onto the bandwagon. Beyond that, I think that the main reason I (and other people stumbling across this PR and related issue) got excited is the prospect of increased stability/availability of vim bindings.
My sense is that the extensions have hashed this out over time. So there are now an expected set of keybinds for the notebook. The implementation here also takes care to return the keymap to exactly as it was when switching from
Ya got me on this one. See also my comment above #9068 (comment) about including just the new Alternatively, maybe the switch to
I'm a bit confused - wouldn't the stock experience (default keybinds) still be the same across all jupyter applications? |
Hi, I'm curious about the status of this. I've been learning vim and climbed up the learning curve enough to now want to use it everywhere, including Jupyter Lab. Looking forward to seeing this functionality in a future version! |
Hi @bt- I think the conclusion is that this is waiting till at least 3.1 In the meantime you still can use vim in jlab! The relevant extensions are: The first one is the most important one and you can install that with:
|
@ianhi Not sure where to put this question so putting it here. One small request... currently axelfahy/jupyterlab-vim has the behaviour that on doing Shift+Enter (which calls run-select-next-edit) , the new empty cell created lands in the normal mode. What i prefer is to land in the insert mode (since jlab is primarily an experimental env and i would simply like to code once i am in a new empty cell. I was able to hack the aforementioned function to my use case. Would this be a default behaviour in this pull request.? Or would it be possible to hack this behaviour somehow? If yes, and you have the extension working, would you be kind enouch to let me know how to build your extension with jupyterlab 3.0 itself.( i simply dont want to wait for 3.1 release ) Thanks |
I mentioned this issue in the 3.2 release planning and it was indicated that a significant effort is still required to get this PR merged. I just wanted to check if anyone in this issue thread is working on that currently or has the time and interest to start doing so. |
Closing due to the age and inactivity of this PR. If you would like to try another fix for #8592, please either update this PR and reopen it or open a new PR. Also, see the jupyterlab-vim extension in jupyterlab-contrib. Thanks! |
References
This is a rebase and continuation of #8706
On top of the work by @echarles I have:
newCellCreated
signalIn order to keep this slim I only pulled in the minimal set of commands that I thought are essential for the vim in the notebook experience. I imagine the remainder can be set by users using the shortcut settings or by an extension.
I'd also be happy to add the following:
Thoughts?
I'd also like add better attribution of the source of the vim keymaps https://github.com/jwkvam/jupyterlab-vim as I seem to have lost those comments somewhere along the way.(edit: done)Code changes
newCellCreated
signal to the notebook trackerUser-facing changes
Choosing vim from the settings menu will now affect both the text editor and the notebook . This addresses problem one from #8592
Backwards-incompatible changes
No? I don't think so.