Skip to content
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

navigation-hotkeys: please remove Esc toggle #1334

Closed
stas00 opened this issue Aug 19, 2018 · 16 comments

Comments

Projects
None yet
3 participants
@stas00
Copy link

commented Aug 19, 2018

It took me a while to hunt this one down. This really useful navigation-hotkeys extension adds a problematic hotkey: esc - toggle to edit mode, which is already assigned to switch to command mode. This breaks a normal use flow where one would hit Esc to switch from any mode to Command mode followed by a custom command mode keyboard shortcut.

Once this extension is enabled Esc behavior becomes non-deterministic and if the current cell is not visible one can't tell the current mode - should I hit Esc or not? So for example I end up typing ii when I'm trying to interrupt the kernel, since hitting Esc-ii happened to be from an already 'command mode', and moving me into edit mode and entering 'ii' to the cell.

Please consider removing this shortcut or making it configurable on the user side. At the moment I have to disable this extension and manually re-insert all but one shortcut from it.

Thank you.

@JSpenced

This comment has been minimized.

Copy link
Contributor

commented Dec 20, 2018

You might want to checkout my pull request fyi. I've fixed this issue as I removed it in my pull since it can be set in the notebook.json file with the default command. Also, I updated it so the command palette works right. Let me if it doesn't work on your pc.

@stas00

This comment has been minimized.

Copy link
Author

commented Dec 20, 2018

Thank you, @JSpenced.

OK, it proved to somehow be tricky to make sure i'm testing the right thing, since more than install is required. This seemed to have done the trick:

pip3 install git+https://github.com/JSpenced/jupyter_contrib_nbextensions.git@update-for-jupyter-and-remove-redundant-keys
jupyter contrib nbextension install --user
+ restart jupyter server

So, your PR fixes the Esc toggle issue - now it only switched it to Cmd Mode.

But it broke these keys:

Needs to be:

Alt- + - Split cell and keep cursor position
Alt- - - Combine cell and keep cursor position

After installing your PR branch these 2 got merge into one, rendering both invalid:

Alt- + - Combine cell and keep cursor position
@JSpenced

This comment has been minimized.

Copy link
Contributor

commented Dec 21, 2018

For installation, you could just copy over the modified main.js file into your nbextensions/navigation-keys folder. Or copy the navigation keys to nav-keys-modified and replace the main.js. Activate that plug-in. I can't see why those would be combined into one binding as I completely removed the command from the file. Jupyter just overwrites bindings so at least one should be set I think.

Yeah, I was confused about the split cell and combine keybindings as they were written in a non-standard way in the javascript file. I wasn't really sure what they were originally because I wasn't using the plug-in before. I got rid of split-cell-at-cursor as that is redundant with the jupyter binding just as enter edit mode was binded to escape.

Add the edit part below into your notebook.json file to get that binding combination back and the escape one is included (optional) to get the original behavior for Esc back. I didn't manually add these into the plug-in because I thought it was unnecessary. Maybe I should just make my own plug-in because I primarily wanted emacs style bindings for navigating cells. This one should probably be directly backward compatible.

"keys": {
   "edit": {
     "bind": {
"Alt-Shift-=": "jupyter-notebook:split-cell-at-cursor",
"Alt-minus": "navigation_hotkeys:merge-cell-with-previous-cell",
   }
   },
"keys": {
   "command": {
     "bind": {
`"Esc": "jupyter-notebook:enter-edit-mode",`
}
   }
}

@JSpenced

This comment has been minimized.

Copy link
Contributor

commented Dec 21, 2018

I can create a commit that fixes these problems and adds the bindings in, but I really don't see the point of including a command already in Jupyter core.

@JSpenced

This comment has been minimized.

Copy link
Contributor

commented Dec 21, 2018

FYI, you should be able to do this with the old navigation-hotkeys but this is untested so not sure if it will work or not:

"keys": {
   "command": {
     "unbind": {
`"Esc"
}
   }
@stas00

This comment has been minimized.

Copy link
Author

commented Dec 21, 2018

Well, I'm just showing the results of installing your version, @JSpenced. I already chose not to use this extension, so I have my own bindings.

Your current implementation not only removes one binding, but it wrongly assigns a different function to an existing binding, as I shared in the pasted from config screen Alt-add started doing what Alt-remove was doing before.

This is unrelated to the issue, which was specifically about leaving Esc functionality matching the core, but surely this proposed removal of several bindings leads to not-backward compatible changes, since now anybody who's used to have the bindings of navigation-hotkeys will suddenly have them gone on update. I'm not saying this is wrong, but this is just not a great practice where a plugin update leads to different bindings. Normally it should go through at least a cycle of deprecation, where perhaps if someone uses the old key you tell them the new key and/or how to rebind back to the old one, or a legacy bindings option in the config. Perhaps there are other options.

@stas00

This comment has been minimized.

Copy link
Author

commented Dec 21, 2018

Maybe I should just make my own plug-in because I primarily wanted emacs style bindings for navigating cells.

May be instead of hardcoded bindings the extension could let users override any and provide pre-made sets of those? (emacs, vi, etc.?)

@JSpenced

This comment has been minimized.

Copy link
Contributor

commented Dec 21, 2018

Yeah, I was originally just updating it for me so I didn't have the annoying autogenerated() messages in my command palette, but I think you're right. I'll make a commit to make it backwards compatible now that I know the proper keys for split and merge cell. Also, that means adding back in Esc functionality for edit mode.

I need to look up how to create options to turn on and off keybindings in the settings file, so if I get time I'll do that later. Yeah, ideally you could turn on and off the bindings separately.

@JSpenced

This comment has been minimized.

Copy link
Contributor

commented Dec 21, 2018

Just to be clear the binding for split cell is alt - + so it's alt-shift-=, right?

@stas00

This comment has been minimized.

Copy link
Author

commented Dec 21, 2018

Neither. I'm not sure why the doc displayed in the extension manager for this extension shows +/- - the way it's currently coded - they are actually add/subtract keys in the numpad. Which was probably not the best choice in first place, since I don't think those keys even exist on some limited keyboards like on chromebooks.

        'alt-add' : {
            help    : 'split cell',
            help_index : 'eb',
            handler : function() {
                IPython.notebook.split_cell();
                IPython.notebook.edit_mode();
                return false;
            }
        },
        'alt-subtract' : {
            help    : 'merge cell',
            help_index : 'eb',
            handler : function() {
                var i = IPython.notebook.get_selected_index();
                if (i > 0) {
                    var l = IPython.notebook.get_cell(i-1).code_mirror.lineCount();
                    IPython.notebook.merge_cell_above();
                    IPython.notebook.get_selected_cell().code_mirror.setCursor(l,0);
                    }
            }
        },
@JSpenced

This comment has been minimized.

Copy link
Contributor

commented Dec 21, 2018

Ahhh, now I get it. Yeah, I don't have those keys on my macbook so was confused. Okay, coding it up and I'll put in a params for escape. Push soon.

@JSpenced

This comment has been minimized.

Copy link
Contributor

commented Dec 21, 2018

Should be completely backwards compatible and has the function requested here.

Let me know if you get any issues.

Cheers

@stas00

This comment has been minimized.

Copy link
Author

commented Dec 21, 2018

For some reason updating is a problem, Tried reinstalling with pip several times, eventually it worked after I did:

jupyter contrib nbextension uninstall --user
jupyter contrib nbextension install --user

and only then it updated .local/share/jupyter/nbextensions/ - but it was a bad choice since I lost the previous config of the enabled extensions :(

The latest incarnation of your version of navigation-hotkeys is missing the alt-add / split entry - it's in the readme, but not in the code. alt-subtract and the rest works.

@JSpenced

This comment has been minimized.

Copy link
Contributor

commented Dec 21, 2018

Sorry, I can't test that key and forgot to change it back after testing. Just pushed up to change it to alt-add so all should be good now. It was there just the wrong binding. Also, I force pushed fyi.

Yeah, I have the repo in a different folder and just paste over nav-keys or just the main.js and hotkeys.yaml.

@stas00

This comment has been minimized.

Copy link
Author

commented Dec 21, 2018

It's all good now, @JSpenced. Thank you.

@juhasch

This comment has been minimized.

Copy link
Member

commented Dec 29, 2018

Merged in PR #1378.

@juhasch juhasch closed this Dec 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.