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

[WIP] Support Vim mode in notebook #1109

Closed
wants to merge 29 commits into
base: master
from

Conversation

6 participants
@parleur
Contributor

parleur commented Feb 19, 2016

This pull request is functional but not usable right now.

It switches the keybindings of all codemirror instance (inspired by the editor). It obviously raises issue in UI interface I aim to discuss here.

  • Issues (just here to be exhautive):
    • Double modal editor: when vim mode is activated in CM, it add a modal editor inside the Jupyter's modal management... Yes press Enter then press a to edit ... not usable
    • Escape (managed by Jupyter) unfocus CM even in Vim edit mode.
  • Solutions proposed. Obviously vim mode should modify the Jupyter keymap also. I propose following changes:
    • Down (Jup managed ) --> Ctrl+Down : switch cell
    • Esc ( Jup managed ) --> Esc (Jup managed): not even ADD a modal state but track CM modal state and synchronize Jupyter modal and CM modal
    • 'a' and 'i' (CM vim mod) --> Same thing, escape from vim edit mode AND from Jupyter edit mode

That's it. Feel free to discuss, that's the main purpose of this PR

@parleur parleur force-pushed the parleur:keybindings branch from 3f7b291 to 137d1b4 Feb 19, 2016

@minrk minrk added this to the 5.0 milestone Feb 21, 2016

@parleur parleur changed the title from Support Vim and other keybinding in notebook to Support Vim mode in notebook Feb 21, 2016

@minrk minrk changed the title from Support Vim mode in notebook to [WIP] Support Vim mode in notebook Feb 22, 2016

@Carreau

This comment has been minimized.

Contributor

Carreau commented Feb 22, 2016

Thanks @parleur I'll try to look at that soon !

@Carreau

This comment has been minimized.

Contributor

Carreau commented Feb 23, 2016

High level comments:

Could we have a tick marker on the menu for the currently use keymap ?

}
codemirror.setOption(opt, value);
});
var that = this;

This comment has been minimized.

@Carreau

Carreau Feb 23, 2016

Contributor

forgotten var that=this ?

delete shortcut['down'];
shortcut['ctrl-down'] = 'jupyter-notebook:select-next-cell';
delete shortcut['up'];
shortcut['ctrl-up'] = 'jupyter-notebook:select-previous-cell';

This comment has been minimized.

@Carreau

Carreau Feb 23, 2016

Contributor

I dont' think we want to bind to ctrl-something, it will screw up with windows manager on some linux/osx

@Carreau

This comment has been minimized.

Contributor

Carreau commented Feb 23, 2016

OK, I think that we want to avoid vim shortcut for now, at it seem to be complex. Get the logic right for default/emacs/sublime, and deals with vim later.

@Carreau

This comment has been minimized.

Contributor

Carreau commented Feb 23, 2016

Regardless of that, pretty sweet, and seem to mostly work for me.

vimMode = true;
this.keyboard_manager.mode = 'vim-command';
} else if ( keymap === 'sublime' || keymap === 'default') {
vimMode = false;

This comment has been minimized.

@Carreau

Carreau Feb 23, 2016

Contributor

if you set vimMode to null, the update option will remove the key from the config file, which is cleaner I think.

vimMode = false;
this.keyboard_manager.mode = 'command';
} else {
console.log('No valid keymap specified. Valid keymaps are in {"vim", "sublime", "default"}');

This comment has been minimized.

@Carreau

Carreau Feb 23, 2016

Contributor

and emacs ?

@minrk

This comment has been minimized.

Member

minrk commented Feb 23, 2016

Awesome, people are going to be really happy about this.

@jdfreder

This comment has been minimized.

Contributor

jdfreder commented Feb 25, 2016

Sweet! Have you had a chance to try @ivanov 's vimception?

@hack-c

This comment has been minimized.

hack-c commented Feb 26, 2016

This looks sweet. @parleur Can you provide some context on what changes were made and where review is most helpful? THanks!!

@parleur parleur closed this Mar 2, 2016

@jdfreder

This comment has been minimized.

Contributor

jdfreder commented Mar 2, 2016

Closed? Did you find an alternative way to implement it?

@Carreau

This comment has been minimized.

Contributor

Carreau commented Mar 2, 2016

Closed? Did you find an alternative way to implement it?

I think @parleur want a bit less attention, and yes, we discussed an alternative for vim keybinding, and had a long conversation on how key handling works in general in the notebook.

@hack-c

This comment has been minimized.

hack-c commented Mar 2, 2016

ah great, thanks @Carreau. I'm interested in helping out with "emacs mode" if/when there's an opening.

@Carreau

This comment has been minimized.

Contributor

Carreau commented Mar 2, 2016

The emacs key bindings are not that hard, the issue is with Vim that already have a dual command/edit mode.

We (@parleur and I) think it should be possible to "never" be in jupyter command mode, and have extra keybindings for codemirror that trigger actions at notebook level.

So you can do things like <esc>:run all, or :<esc>:above , or alike.
It needs some refactor, like replacing

if (this.mode === 'edit') {
    return this.edit_shortcuts.call_handler(event);
} else if (this.mode === 'command') {
  return this.command_shortcuts.call_handler(event);
}

by something like:

new_mode = this.mode[this.current_mode].call_handler(event)
this.current_mode = new_mode

To gain some flexibility, and some patching of markdown cell to switch the rendering on focus.

@jdfreder

This comment has been minimized.

Contributor

jdfreder commented Mar 3, 2016

^ @Carreau I like that idea

@parleur

This comment has been minimized.

Contributor

parleur commented Mar 9, 2016

Here a working cell configuration. Mainly, I modified Cell to pass anything from Cell.class_config to subclasses. Of course if subclasses ( namely CodeCell, MarkdownCell, ...) overwrite config if defined. Also initialise the notebook in the correct mode.

@parleur parleur force-pushed the parleur:keybindings branch 2 times, most recently from bb3eac0 to d252ec0 Mar 9, 2016

@Carreau

This comment has been minimized.

Contributor

Carreau commented Mar 9, 2016

I think you might need to rebase.

@parleur

This comment has been minimized.

Contributor

parleur commented Mar 10, 2016

A stupid question: you suggest to rebase on top on current master:HEAD, I am used to merge new HEAD into my dev branch ( I ve read somewhere, it is dangerous to rebase any published commit ). I am wrong? Or is it equivalent ?

@Carreau

This comment has been minimized.

Contributor

Carreau commented Mar 10, 2016

Usually we avoid merging with HEAD.
It is not dangerous to rebase, it can just be a bit more complicated. And anyway locally your commits are still in the reflog, so you cannot lose work.

I can show you how to rebase next Tuesday on Skype if you like.

@Carreau

This comment has been minimized.

Contributor

Carreau commented Mar 10, 2016

Also current commit is parleur@2832c13

So now that it is linked here, whatever you do, we can get it back from GitHub.

@parleur

This comment has been minimized.

Contributor

parleur commented Mar 24, 2016

A little comment about dynamic Codemirror keymap switching.
CodeMirror have a bug/strange behavior with handlers so that when a handler have been registered before a keymap switch then it is treated after the newly declared codemirror core handler. The solution is to unbind the jupyter handler then switch keymap and rebind it.

@Carreau

This comment has been minimized.

Contributor

Carreau commented Mar 24, 2016

A little comment about dynamic Codemirror keymap switching.
CodeMirror have a bug/strange behavior with handlers so that when a handler have been registered before a keymap switch then it is treated after the newly declared codemirror core handler. The solution is to unbind the jupyter handler then switch keymap and rebind it.

Another solution would be forcing to reload page after switching keymap..

@parleur

This comment has been minimized.

Contributor

parleur commented Mar 24, 2016

Hmm it would solve the Codemirror bug ... but likely raise other issues. At minimum auto saving before reloading and handle interrupt computation due to keymap switching
Unbind and rebind is proper and simpler (it is needed only about the keydown hander). The cleanest way would be to patch codemirror but it is out of my scope.

@Carreau

This comment has been minimized.

Contributor

Carreau commented Mar 24, 2016

We can likely assume people won't switch keymaps too many time. So I think reloading (with a warning) is acceptable if we want to go this route.

@parleur parleur force-pushed the parleur:keybindings branch 2 times, most recently from f81ac18 to 93947c1 Mar 24, 2016

@parleur parleur force-pushed the parleur:keybindings branch from 3919fc0 to ac461c4 Mar 25, 2016

@parleur

This comment has been minimized.

Contributor

parleur commented Mar 25, 2016

The vim keymap works and going back to default mode. It's quick and dirty but it is an example of pleasant notebook navigation using only the keyboard.

Note that it work using a patched codemirror (already merged) that can be backported onto the current codemirror version used by Jupyter.

@Carreau

This comment has been minimized.

Contributor

Carreau commented Mar 25, 2016

THanks for fixing things upstream. In general CodeMirror is released monthly, so we never have to wait too much between releases.

@Carreau

This comment has been minimized.

Contributor

Carreau commented Mar 30, 2016

@parleur

This comment has been minimized.

Contributor

parleur commented Apr 1, 2016

This PR in now close. After many time spent on in it appears that dealing a keymap layer need a proper keyevent handling, and it is not a refacto that can be done under the hood here.

@parleur parleur closed this Apr 1, 2016

@alok

This comment has been minimized.

alok commented May 29, 2017

I wonder if Neovim's embedding functionality could be used for this, like how ActualVim works, rather than just reimplementing vim-mode like every other project with a vim emulation mode.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment