Initial VIM support in cell editors #8706

echarles · 2020-07-22T14:04:34Z

References

This PR addresses for Cell Editors Problem 1 - Separated Keymap from Text Editor VIM discussed in #8592

Code changes

code-mirror-extension now tracks also the notebook and update the cell editor options when the Keymap settings is changed.

User-facing changes

Cell editors benefit from VIM mode support. Instead of ESC, user has to use CTRL-[ to exit from insert mode.

Backwards-incompatible changes

None

jupyterlab-dev-mode · 2020-07-22T14:04:44Z

Thanks for making a pull request to JupyterLab!

To try out this branch on binder, follow this link:

ianhi · 2020-07-23T02:21:13Z

!!! very excited to see this moving towards core jupyterlab 😄

Cell editors benefit from VIM mode support. Instead of ESC, user has to use CTRL-[ to exit from insert mode.

Is this restriction on Esc in order to maintain the usage of Esc to enter jupyter command mode? If so I appreciate the desire to maintain jupyterlab shortcuts but think that this change will be extremely disruptive to both terminal vim users and users of the existing jupyterlab-vim extensions. I would instead recommend keeping Esc to exit insert mode and using shift-Esc to get to jupyter command mode.

More generally given the history and user base of https://github.com/jwkvam/jupyterlab-vim I think that any implementation in core should strive to maintain the extension's approach to adapting the vim keymap to jupyter. Not only are the extension's choices sensible, but many people are accustomed to them and there will be a fracturing of the user base if the core implementation does not match them. Personally I could live without some of the shortcuts in the existing jupyterlab-vim extension, but if Esc was is not a valid way to exit insert mode I would not use the core implementation. Instead I would either continue to use the extension or make an extension to bring back the current extensions behavior:

Esc vim-insert -> vim-command
Shift-Esc vim-[insert,command] -> jupyter command mode

I think it is worth taking most (or even all) of the keymaps for maneuvering the notebook from the existing extensions list here: https://github.com/axelfahy/jupyterlab-vim#key-bindings and defined here and here

I'm happy to help with this however would be most useful (feedback, adding coding, meeting to talk about it, other things?). Also CC two other people who may be interested in this: @axelfahy, @jwkvam

ianhi · 2020-07-23T02:24:08Z

I also see that this is marked as "initial" so I totally get not having worked out all the keymap details, but I guess my contention is that a partial vim support would be worse than no support and people instead using the more complete extensions.

jtpio · 2020-07-24T09:46:02Z

Thanks @echarles for this!

Just tried on the Binder and it looks great.

It looks like navigating between cells using k and j when the notebook is in command mode also works. But that seems to be the default in JupyterLab.

I think that any implementation in core should strive to maintain the extension's approach to adapting the vim keymap to jupyter

Indeed, other bindings such as z,z to recenter the scroll position would also be useful (currently it's the keybinding to delete cells). Or d,d and p to cut a cell and paste it somewhere else. But that goes beyond vim support in cell editors since they apply to the notebook command mode, and could be added in a separate change.

blink1073 · 2020-07-26T10:35:15Z

@echarles is this PR ready to review?

echarles · 2020-07-26T10:51:49Z

@blink1073 Nope... (not sure how to put in back in draft) as we have received user feedback on various channels and need to make sure we address them, especially regarding key binding. I need to digest the comments and connect back to have a common agreement on the features before this can be reviewed.

blink1073 · 2020-07-26T10:52:49Z

Convert to draft option is there ^

echarles · 2020-07-27T17:15:07Z

Last commit adds shortcut to move up/down the cell.

I have tried to use ESC keystroke to exit from VIM edit mode. Therefor, the default Keybinding needs to be removed and I wonder if it is possible with the current approach where the VIM settings is applied for all editors and not on editor changes

With all editors, I would need to remove/disable the existing key binding but addKeyBinding(…) does not support disabled and shortcuts-extension is not exposed for consumption.
With action taken on editor change like in jupyterlab-vim extension, the shortcut remains and a commands allows to handle the key on the code editor

https://github.com/axelfahy/jupyterlab-vim/blob/c4e43f940ef4be4c961608b6192412d2f3a33d1f/src/index.ts#L307-L321

ianhi · 2020-07-27T17:29:49Z

packages/notebook-extension/schema/tracker.json

+    {
+      "command": "notebook:move-cell-down",
+      "keys": ["Ctrl J"],
+      "selector": ".jp-Notebook:focus"
+    },
+    {
+      "command": "notebook:move-cell-up",
+      "keys": ["Ctrl K"],
+      "selector": ".jp-Notebook:focus"
+    },


The suggested change below would bring these commands in line with what the extensions currently do. Although having a command to move cells up and down in jupyter command mode would also be awesome. Maybe we could add both?

Suggested change

{

"command": "notebook:move-cell-down",

"keys": ["Ctrl J"],

"selector": ".jp-Notebook:focus"

},

{

"command": "notebook:move-cell-up",

"keys": ["Ctrl K"],

"selector": ".jp-Notebook:focus"

},

{

"command": "notebook:move-cursor-down",

"keys": ["Ctrl J"],

"selector": ".jp-Notebook.jp-mod-editMode"

},

{

"command": "notebook:move-cursor-up",

"keys": ["Ctrl K"],

"selector": ".jp-Notebook.jp-mod-editMode"

},

having a command to move cells up and down in jupyter command mode would also be awesome.

this applies in jupyter command mode. Maybe you mean in VIM command mode?

Oh yeah, my bad for not being more precise with my terminology. Currently the extensions enable you to shift the cursor up or down to adjacent cells using ctrl-j/k in either vim-insert or vim-command modes. Though I think having these keybinds do something in jupyter-command mode is also a valuable addition. Like so:

ianhi · 2020-07-27T17:32:10Z

With all editors, I would need to remove/disable the existing key binding but addKeyBinding(…) does not support disabled and shortcuts-extension is not exposed for consumption.

Huh that's tricky. Would just defining the keymap (using addKeyBinding) as a no-op or return false be equivalent to disabling?

jasongrout · 2020-07-27T17:46:25Z

The "disabled" attribute is part of the setting registry, whereas the addKeyBinding is at a lower level, at the application layer.

I think you can't add a keybinding identical to one that already exists (though you might try?)

echarles · 2020-07-27T18:04:48Z

The "disabled" attribute is part of the setting registry, whereas the addKeyBinding is at a lower level, at the application layer.

Yep, saw that, this why I looked at the shortcuts-extension and saw that it was not exposed to others. Would there be a way to disable a keybindings via the settings extension?

I think you can't add a keybinding identical to one that already exists (though you might try?)

Tried that, and various combination like e.g. what @ianhi suggests with Would just defining the keymap (using addKeyBinding) as a no-op or return false be equivalent to disabling? but it looks like the VIM keybindings has not the priority and is hidden by jupyterelab. I could invoke from the jlab command (therefor, I need to access the selected editor, and use a _onActiveCellChanged approach).

echarles · 2020-07-28T15:54:34Z

The last commit introduces a listener on cell change which allows to override the ESC command to go out of insert mode.

echarles · 2020-07-30T09:54:54Z

This branch is now aligned with master and takes implements what we have discussed:

VIM mode for cell editor (the same way as for file editor).
Esc to exits vim mode
Shift-Esc to go to jupyter command mode
ctrl-j/k to shift up or down a cell

It is ready for review.

ianhi

This is really great! I have one final issue to bring up but otherwise I am a big 👍 on merging this. I've left some comments on places you could optimize the code a bit if you wanted to, but I don't think any of the proposed changes are necessary.

remaining issue:
I just tried this out and found that moving between cells when in vim-command-mode was not possible using j or k and up still works but down no longer works:

The vim extensions work around this by adding a custom codemirror command to j and k in vim mode:
https://github.com/axelfahy/jupyterlab-vim/blob/c4e43f940ef4be4c961608b6192412d2f3a33d1f/src/index.ts#L164-L173

The expected behavior is that pressing j or k in the last or first line will move me to the next cell:

ianhi · 2020-08-09T02:10:04Z

packages/codemirror-extension/src/index.ts

+  let activeCell: Cell | null = null;
+
+  commands.addCommand('codemirror:leave-vim-insert-mode', {
+    label: 'Leave VIM Insert Mode',
+    execute: args => {
+      if (activeCell) {
+        let editor = activeCell.editor as CodeMirrorEditor;


I think this is implicitly depending on onActiveCellChanged having been called in order to have set activeCell.

Suggested change

let activeCell: Cell | null = null;

commands.addCommand('codemirror:leave-vim-insert-mode', {

label: 'Leave VIM Insert Mode',

execute: args => {

if (activeCell) {

let editor = activeCell.editor as CodeMirrorEditor;

commands.addCommand('codemirror:leave-vim-insert-mode', {

label: 'Leave VIM Insert Mode',

execute: args => {

const activeCell = notebookTracker.activeCell;

if (activeCell) {

let editor = activeCell.editor as CodeMirrorEditor;

ianhi · 2020-08-09T02:10:09Z

packages/codemirror-extension/src/index.ts

+  function onActiveCellChanged(): void {
+    if (keyMap === 'vim') {
+      activeCell = notebookTracker.activeCell;
+      if (activeCell) {
+        // From https://github.com/axelfahy/jupyterlab-vim/blob/c4e43f940ef4be4c961608b6192412d2f3a33d1f/src/index.ts
+        CodeMirror.prototype.save = () => {
+          void commands.execute('docmanager:save');
+        };
+        const lvim = (CodeMirror as any).Vim as any;
+        if (lvim) {
+          lvim.defineEx('quit', 'q', function(cm: any) {
+            void commands.execute('notebook:enter-command-mode');
+          });
+          lvim.defineAction('splitCell', (cm: any, actionArgs: any) => {
+            void commands.execute('notebook:split-cell-at-cursor');
+          });
+          lvim.mapCommand('-', 'action', 'splitCell', {}, { extra: 'normal' });
+        }
+      }
+    }
+  }
+


I think all of these act globally and thus only need to be run when the vim keymap is selected rather than everytime the active cell is changed. I'd suggest moving these definitions under the if( keymap == 'vim').

ianhi · 2020-08-09T02:11:36Z

packages/codemirror-extension/src/index.ts

    if (keyMap === 'vim') {
      // @ts-expect-error
      await import('codemirror/keymap/vim.js');
+      commands.addKeyBinding({


Would it be easy to add a signal.emit here with the keymap in it? I could see that being helpful for extensions building around the keymaps. If not easy then it's no big deal.

ianhi · 2020-08-09T03:37:26Z

Aghh sorry I found one more issue. If you create a new cell then that cell will not be in vim mode until you toggle the vim setting on and off.

I think this is why the extensions did all the work of setting the keymap onActiveCellChanged rather than with a tracker. It seems you can only use signals to track the creation of new NotebookPanels there is no tracker for the creation of new cells. Though maybe there ought to be?

ianhi · 2020-09-24T04:49:44Z

Rather than purely finding issues I had a go at solving them. #9068 is a rebase of this PR that fixes all of the issues I raised.

echarles · 2020-09-30T09:55:54Z

Closing as this work is now done in #9068.

github-actions bot added the pkg:codemirror label Jul 22, 2020

tgeorgeux mentioned this pull request Jul 22, 2020

22 July 2020 JupyterLab Weekly Team Meeting jupyterlab/frontends-team-compass#79

Closed

echarles mentioned this pull request Jul 23, 2020

VIM mode in Notebook #8592

Open

echarles marked this pull request as draft July 26, 2020 11:40

Initial VIM support in cell editors

b066666

echarles force-pushed the vim-cell-editor branch from 4e9d4c9 to b066666 Compare July 27, 2020 14:16

Shortcut to move up/down the cell

b801274

github-actions bot added the pkg:notebook label Jul 27, 2020

ianhi reviewed Jul 27, 2020

View reviewed changes

Add onActiveCellChanged to manage ESC to go out of insert mode

b3566c6

echarles added 3 commits July 28, 2020 17:56

Add cells as dependency to codemirror-extension

a02322b

Add cells as dependency to codemirror-extension

7e3ef5d

Merge branch 'master' into vim-cell-editor

dadc2a0

echarles marked this pull request as ready for review July 30, 2020 09:52

echarles mentioned this pull request Jul 31, 2020

Add JupyterLab debugger to core #8747

Merged

jasongrout mentioned this pull request Aug 5, 2020

29 July 2020 Weekly Meeting jupyterlab/frontends-team-compass#83

Closed

ianhi reviewed Aug 9, 2020

View reviewed changes

blink1073 added this to the Future milestone Aug 26, 2020

ianhi mentioned this pull request Sep 24, 2020

vim support in the notebook #9068

Closed

3 tasks

axelfahy mentioned this pull request Sep 28, 2020

Jupyterlab 3 jupyterlab-contrib/jupyterlab-vim#14

Closed

echarles closed this Sep 30, 2020

github-actions bot added the status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. label Mar 30, 2021

github-actions bot locked as resolved and limited conversation to collaborators Mar 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial VIM support in cell editors #8706

Initial VIM support in cell editors #8706

echarles commented Jul 22, 2020

jupyterlab-dev-mode bot commented Jul 22, 2020

ianhi commented Jul 23, 2020

ianhi commented Jul 23, 2020

jtpio commented Jul 24, 2020

blink1073 commented Jul 26, 2020

echarles commented Jul 26, 2020

blink1073 commented Jul 26, 2020

echarles commented Jul 27, 2020 •

edited

ianhi Jul 27, 2020

echarles Jul 27, 2020

ianhi Jul 27, 2020 •

edited

ianhi commented Jul 27, 2020 •

edited

jasongrout commented Jul 27, 2020 •

edited

echarles commented Jul 27, 2020 •

edited

echarles commented Jul 28, 2020

echarles commented Jul 30, 2020

ianhi left a comment

ianhi Aug 9, 2020

ianhi Aug 9, 2020

ianhi Aug 9, 2020

ianhi commented Aug 9, 2020 •

edited

ianhi commented Sep 24, 2020

echarles commented Sep 30, 2020

Initial VIM support in cell editors #8706

Initial VIM support in cell editors #8706

Conversation

echarles commented Jul 22, 2020

References

Code changes

User-facing changes

Backwards-incompatible changes

jupyterlab-dev-mode bot commented Jul 22, 2020

ianhi commented Jul 23, 2020

ianhi commented Jul 23, 2020

jtpio commented Jul 24, 2020

blink1073 commented Jul 26, 2020

echarles commented Jul 26, 2020

blink1073 commented Jul 26, 2020

echarles commented Jul 27, 2020 • edited

ianhi Jul 27, 2020

Choose a reason for hiding this comment

echarles Jul 27, 2020

Choose a reason for hiding this comment

ianhi Jul 27, 2020 • edited

Choose a reason for hiding this comment

ianhi commented Jul 27, 2020 • edited

jasongrout commented Jul 27, 2020 • edited

echarles commented Jul 27, 2020 • edited

echarles commented Jul 28, 2020

echarles commented Jul 30, 2020

ianhi left a comment

Choose a reason for hiding this comment

ianhi Aug 9, 2020

Choose a reason for hiding this comment

ianhi Aug 9, 2020

Choose a reason for hiding this comment

ianhi Aug 9, 2020

Choose a reason for hiding this comment

ianhi commented Aug 9, 2020 • edited

ianhi commented Sep 24, 2020

echarles commented Sep 30, 2020

echarles commented Jul 27, 2020 •

edited

ianhi Jul 27, 2020 •

edited

ianhi commented Jul 27, 2020 •

edited

jasongrout commented Jul 27, 2020 •

edited

echarles commented Jul 27, 2020 •

edited

ianhi commented Aug 9, 2020 •

edited