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

Explore notebook keymap support #106679

Closed
rebornix opened this issue Sep 14, 2020 · 17 comments
Closed

Explore notebook keymap support #106679

rebornix opened this issue Sep 14, 2020 · 17 comments
Assignees
Labels
Milestone

Comments

@rebornix
Copy link
Member

Notebook actions we ship in the core have VS Code inspired keybindings bound to them, with which existing users can manipulate the notebook content with the keyboard shortcuts they are already with in the text editors.

However Jupyter users may prefer the Vim-like keybindings from traditional Jupyter web apps. Those keybindings may have conflicts with the builtin ones. To avoid conflicts, we can turn them into keymaps (builtin nb keymap and a Jupyter keymap) but they should be only scoped to Notebook (meaning you can install a sublime keymap for the whole workbench and also a notebook keymap).

@rebornix rebornix added feature-request Request for new features or functionality notebook labels Sep 14, 2020
@rebornix rebornix added this to the September 2020 milestone Sep 14, 2020
@rebornix rebornix modified the milestones: September 2020, October 2020 Sep 30, 2020
@rebornix rebornix modified the milestones: October 2020, On Deck Oct 26, 2020
@rebornix
Copy link
Member Author

Now that Jupyter is a standalone extension (no longer part of the Python extension), it makes perfect sense that the Jupyter extension contributes the Jupyter style keybindings. It would be weird that if we publish a dedicated keymap extension and make it a dependency of the Jupyter extension.

With that being said, the requirements are still there. Users may still have following requests (not that users have Jupyter extension installed)

  • Users may want to only use VS Code style keybindings
    • In other words, they want to disable the Jupyter keybindings from the Jupyter extension but they can't disable the Jupyter extension
  • Users may want to use the Jupyter style keybindings
    • Keybindings from extensions have higher priority than the builtin ones (as they are registered later), so they will take effect by default
  • Users want to switch "keymap", e.g., Jupyter users will use Jupyter style keybindings when they try VS Code the very first time but later on might switch to VS Code ones

Allowing the Jupyter ones to be disabled and switching between keybinding set easily is the major problem to solve. A domain specific keymap is kind of appealing:

  • Users can install a Vim keymap and the Jupyter extension at the same time. The Jupyter extension declares it as a keymap for Notebook so they can be enabled at the same time
  • Users can disable the keybindings from the Jupyter extension someway

But it comes with too many open questions: how do users disable the sub keymap? how do extensions contribute a sub keymap? how do extension declare which keybinding contributions belong to this domain level keymap? If we take one step back and ask ourselves, is this generic enough and deserving such a fancy solution?

Considering the complexity of introducing a new concept of sub keymap, we can now go with a pragmatic solution: adding a setting useJupyterKeybinding to the extension and then put it into the when clause to relevant keybindings. Enabling/disabling the keybindings is only a matter of flipping the setting.

cc @sandy081 , FYI just in case we have similar ideas brought up else where or sometime in the future.
cc @DonJayamanne , I can send a PR for Jupyter extension to allow enabling/disabling the Jupyter style keybindings.

@sandy081
Copy link
Member

  • Users may want to only use VS Code style keybindings
  • In other words, they want to disable the Jupyter keybindings from the Jupyter extension but they can't disable the Jupyter extension

This can be solved by creating a Jupyter extension pack - Jupyter notebook extension + Jupyter keymap extension. Users can disable the keymap extension independent of notebook extension. There are also discussions going on about introducing optional dependencies, but I think extension pack is an appropriate solution here as there is no any code dependencies here.

CCing @alexdima for switching keymaps.

IMO it would be cool to allow user to switch keymaps. For eg., Show the list of keymap extensions in the quick pick (like themes) and allow user to switch. Internally, we can disable all other keymap extensions.

@alexdima
Copy link
Member

👍 on having multiple extensions.

Personally, I don't think keybindings should be shipped together with an extension that provides important functionality because keybindings are so personal to people, and the main selling point of the extension is the important functionality, not the keybindings of the established software.

In other words, IMHO people would want to install the Jupyter extension for the Jupyter Notebooks functionality, and can then optionally install Jupyter keybindings if they really want those keybindings.

@DonJayamanne
Copy link
Contributor

Thanks, so the recommendation is to create a separate extension for the keybindings, sounds simple.
We'll (owners of the Jupyter extension) create one, unless you guys were planning on creating this.

@rebornix
Copy link
Member Author

rebornix commented Nov 25, 2020

@sandy081 and me had offline discussions about this and talked about why we call this "sub"-keymap support in the issue title. Basically I thought we can only have one activated keymap extension in VS Code workspace, while Jupyter keybindings will only work in notebook component, so we may want to support a case where users have a global keymap (e.g. Sublime) and a domain specific keymap (Jupyter). But that turns out to be false assumption. You might see a prompt for asking to you to disable other keymaps when installing from the Welcome page but you can always install keymap extensions from Extension View without any issue. They can work together but there might be some conflicts. For Jupyter keymap, it's not a big deal as existing keymaps don't cover the notebook component, so they can work together seamlessly.

Agree with you all that putting Jupyter keybindings into a separate extension is the best approach here. The Jupyter core extension will contribute some Jupyter specific commands with default keybindings, when users install the Jupyter core extensions, both the notebook commands implemented by VS Code and Jupyter core extensions have proper default keybindings. If users want to use Jupyter style keybindings, then they install the Jupyter keymap extension.

Note that to avoid any prompt when installing other real keymaps, the Jupyter keymap can leave the category blank instead of putting it under keymaps.

@rebornix rebornix modified the milestones: On Deck, November 2020 Nov 25, 2020
@rebornix
Copy link
Member Author

Closing since we have recommendation posted above.

@DonJayamanne
Copy link
Contributor

@rebornix Thanks for the suggestion.
Unfortunately creating a separate extension doesn't work for us right now.
We would like to ship such an extension as a dependency of the Jupyter extension.
However VS Code doesn't allow on to disable dependant extensions (e.g. try installing Python extenison & disabling Jupyter extension, this doesn't work).

@rebornix
Copy link
Member Author

Unfortunately creating a separate extension doesn't work for us right now.
We would like to ship such an extension as a dependency of the Jupyter extension.
However VS Code doesn't allow on to disable dependant extensions

@sandy081 can you help confirm that currently it's not possible to archive this? If so, it left us the option of using a setting in Jupyter extension to disable the jupyter style keybindings.

@alexdima
Copy link
Member

We would like to ship such an extension as a dependency of the Jupyter extension.

If extension B depends on extension A:

  • then extension A is installed when installing extension B (this is mostly done for the next point)
  • and also extension B activates only if extension A is installed, enabled, and (in case it has code) activated successfully.

So an extension dependency has both an install-time consequence, but most importantly, a runtime consequence. I think the alternative to an extension dependency is an extension pack, since that has only install-time effects. cc @sandy081 for opinions

@sandy081
Copy link
Member

Yeah extension pack has the flexibility to disable or uninstall extension A independent of the runtime. IMO from the concept perspective, using extensionPack is not appropriate. Coincidentally I am exploring an option for such requirements where a feature rich extension wants to optionally include additional features those are bundled as other extensions - #6384. I am assuming the solution we achieve there will also suffice yours.

@github-actions github-actions bot locked and limited conversation to collaborators Jan 18, 2021
@rebornix rebornix removed this from the November 2020 milestone Aug 9, 2021
@rebornix rebornix added this to the August 2021 milestone Aug 9, 2021
@rebornix
Copy link
Member Author

rebornix commented Aug 9, 2021

Reopening as current approach doesn't solve the problem fully and lead to user confusion of using the notebook editor the first time.

@rebornix rebornix reopened this Aug 9, 2021
@rebornix rebornix assigned joyceerhl and unassigned roblourens Aug 9, 2021
@rebornix rebornix changed the title Explore notebook "sub"-keymap support Explore notebook keymap support Aug 9, 2021
@rebornix
Copy link
Member Author

rebornix commented Aug 13, 2021

@joyceerhl and I did some analysis of the complaints we received from users before/after adopting the suggestions we proposed above and revisited the keyboard shortcut conflicts review Joyce did last month. We found the complains are mostly from inconsistency between the behaviors of VS Code and Jupyter Classic/Lab, an experienced Jupyter users would find it broken even if the behavior changes slightly (or in some's opinions, correctly).

Inconsistency/overlap

  • Shift+Enter doesn't quit editing markdown jupyter#6037, jupyter#7042
  • Ctrl+Enter should go to control mode jupyter#6198, jupyter#6582
  • Ctrl+Enter insert lines instead of running jupyter#4377
  • Ctrl+Enter doesn't insert cell below jupyter#4420
    • this is majorly about which keybindings we recommend by default
  • Insert Cell Above/Below should stay in control mode.
    • This probably makes good sense since in Jupyter it's driven by A/B and focusing the cell editor will immediately quit control mode.
  • S doesn't save notebook

Now that we have ipynb serializer in the core, users are able to view Jupyter notebook files oob in VS Code, we can implement the Jupyter style commands in the core then users can always interact with the Jupyter notebooks in the way they prefer (no matter if it's Jupyter's keybindings or VS Code builtin ones, or a mix of both) through Keybindings Editor/keybindings.json, without installing any additional keymap/extensions.

The next step would be figuring out how to help users choose the "keymap" to use in Jupyter notebook. One thing to note here is no matter what solution we choose, it won't change the fact that:

  • Users defined keybindings have higher priority vscode-jupyter#517
  • Keybindings contributed by other extensions can still override the Jupyter ones vscode-jupyter#328 (otherwise Vim won't work)

Proposals

A: Jupyter extension contributes all keybindings

This is the same as before but Jupyter won't have their own command implementations. To run a Jupyter notebook (ctrl+enter, shift+enter), users need to install Jupyter extension (finding kernels).

Pro

  • No confusion from users perspective: we get Jupyter keybindings after installing Jupyter extension

Con

  • If an user finds Jupyter keybindings ineffecient, they don't have simple switch to disable them (though this can be solved by keeping a setting for Jupyter keybindings)
  • The Run button is bind to Run Cell command, which might not be an expected behavior

B: Jupyter Keymap extension, shipped together with Jupyter but users can disable it

We ship Jupyter keybindings as a separate extension, but it's not a real exclusive "Keymap" extension. Jupyter will ship together with this extension (like the renderer extension) and users can disable it if they want

Pro

  • It comes with Jupyter
  • Users can disable it through extension page

Con

  • Same as Proposal A, this won't solve the problem of "Run button"

C: Keybindings in the core, with a setting controlling which keymap to use

We will have all the keybindings defined in the core, but introduce a setting notebook.keybinding.mode: "default" | "jupyter" to control which set of keybindings will take effect. The setting will potentially control the behavior of the Run button.

Pro

  • It works OOB
  • We can control the behaviors of execution related buttons
    • Note this can also be resolved in a different way

Con

  • We won't have the flexibility to control if keybindings should just work for Jupyter or all notebooks.

@tanhakabir
Copy link
Contributor

My understanding from todays discussion was that keymaps for Notebooks that are universal for all notebooks, Jupyter and custom alike.

For now we'll ship the keymapping with the extension and push users to install the Jupyter extension (which contains everything they need from kernel detection to renderers) when they're trying to use a keybinding in the Notebook. Before having the Jupyter extension keymappings Notebooks will follow the VS Code built-in ones, after the extension all Notebooks will follow the Jupyter contributed ones. Users can disable the Jupyter keybindings if they want to.

Currently all of the bugs about keybindings are of users who have the Jupyter extension but still have unexpected behaviour. We will fix these first and then be on look out for telemetry or user filed issues for not having the Jupyter keybindings out of the box.

Custom Notebooks are in its infancy and we don't know enough yet about what the community of custom notebooks will expect so we can't yet conclusively state what their preferences and expectations of keybindings will be. Will they have Jupyter experience or only VS Code experience? We only have at most 100 engaged custom notebook users at the moment.

I did a bit more exploring and asked Maria from the .NET team what their process of thinking behind keymaps were when they were making .dib Notebook. They kept the same keybindings as VS Code and had no filed issues around keybindings.

@rebornix
Copy link
Member Author

We have good discussions offline. The default behavior of Run button is a problem which should be solved individually, as even with Proposal C, if users don't want the Run button to "Run + Insert" (default behavior of run button on Jupyter), we still need a solution to allow users to pick their preferred "Run" command to use. Thus it's not necessarily an advantage of Proposal C. Proposal B is more preferable as it functions the same as proposal A and it's easy for users to disable the whole keymap. In the meantime, it gives users a chance to use Jupyter keybindings with custom notebooks.

When users have Jupyter extension installed, the notebook keybindings behave the same as Jupyter Classic/Lab (follow their muscle memory). For users who don't have Jupyter installed yet but they start to edit and run jupyter notebook, we can provide extension recommendation to them, like what we do for Kernel and Renderers, based on hueristics:

  • users press shift+enter or ctrl+enter
  • users press A/B when focusing on cell container

The recommendation can be presented through getting started (a section for Jupyter notebook, which mentions Kernel detection and Jupyter Renderer).

@rebornix
Copy link
Member Author

rebornix commented Aug 13, 2021

Another topic we covered in the meeting was whether the Jupyter keymap should be shipped in the core and turned on all all notebooks. Having the keymap in the core helps with consistency and smoother transition from custom notebook to Jupyter, but it's arguable if the keybindings shipped with Jupyter Classic/Lab are the ones we want to offer to users as default ones, especially when the other notebook leaders/innovators behavior differently:

  • Ctrl+Enter
    • Jupyter Lab, run and focus container
    • Google Colab, just run
    • Deepnote, just run
    • Observablehq, run and insert cell below
  • A/B
    • Jupyter Lab, insert cell and focus container
    • Google Colab, insert cell and focus editor
    • Deepnote, Observablehq, not supported

Recommending/find the right keymap to use will then be similar to notebook layouts, which depend on users' background and prior experience. Jupyter extension can ship keymaps which is the same as Jupyter Lab but users can install other keymaps.

@tanhakabir
Copy link
Contributor

Thinking again about what would be good compromises, I think it does make sense to take the non-contentious keybindings and have them supported OOB and the other keybindings which don't fit well with VS Code's editing model can be contributed by the Jupyter extension.

I'm thinking about these to be brought into core:

Command

  • K: select cell above
  • J: select cell below
  • A: insert cell above
  • B: insert cell below
  • D,D: delete selected cells
  • L: toggle line numbers
  • O: toggle output of selected cells
  • Shift-L: toggles line numbers in all cells (I believe we support in core but added it here since I saw on the table it said otherwise)

Edit

  • Ctrl-Shift-Minus: split cell at cursor(s)

@rebornix
Copy link
Member Author

We will continue our work once we get #123581 (comment).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

7 participants