Create newCellCreated signal + make block comment keyboard shortcut a setting

[ianhi]

References

Extracts the newCellCreated and block comment changes from #9068

Closes: #4778
Closes: #8882
also see: #7579 (comment)

Code changes

1. Created a newCellCreated signal
   • This is also helpful to 3rd party extensions e.g. the vim keymaps
2. Used that signal to update the extraKeys to allow the keyboard shortcut for blockComment to become a setting
3. set indent and deindent as settings
   • Mentioned in Comment code block command #4778 (comment)
     Is it possible to move that setting into the shortcuts extension? I couldn't figure out how to do that so it is under the Codemirror section of the settings.

User-facing changes

You can now set whatever keys you want to be block comment.

For example, add this to the CodeMirror settings:

    "toggleComment": "Ctrl-5",
    "indent": "Ctrl-8",
    "deindent": "Ctrl-9",

Backwards-incompatible changes

I guess that any extension that uses the jlab codemirror factory to create an editor separate from the notebook or fileeditor will now be responsible for assigning those shortcuts.

[afshin]

For this use case, I would suggest having one single shortcut that using the Accel key instead of either Ctrl or Command. On a Mac, Accel means Command and on Windows and Linux Accel means Ctrl.

[ianhi]

For this use case, I would suggest having one single shortcut that using the Accel key instead of either Ctrl or Command. On a Mac, Accel means Command and on Windows and Linux Accel means Ctrl.

Good point thanks :) just did that

[ianhi]

Should these be removed? They are in the initial commit that I rebased on top of and I don't fully understand this part. I think that this will mean that some of the fileeditor specific commands below will be erroneously enabled by a notebook being present. (To be clear though I'm not sure)

[jasongrout]

I guess that any extension that uses the jlab codemirror factory to create an editor separate from the notebook or fileeditor will now be responsible for assigning those shortcuts.

Yeah, looking through this, we're just working around bad design that was already in the codebase, I think. There's no reason the codemirror factory should be aware of how it's being used in notebook and editors.

I guess ideally the editor factory has an initialization step that happens before the editor is handed off to a consumer, and then there is some signal for when that initialization needs to be updated that the consumer should react to. So, for example, when a notebook creates a new code editor, it should be initialized appropriately already, and then the notebook should listen for changes in that editor configuration.

[jasongrout]

I looked at this more last night and this morning. Here are my notes about what I think the codemirror-extension should ideally do:

• Create its own factory wrapping the codemirror editorServices. This factory would by default add the editors to a focus tracker it maintains, with perhaps an opt-out by the factory consumer. This factory would automatically initialize all codemirrors according to its settings before handing them to the consumer, and would use the focus tracker to automatically update any codemirrors it created when the settings changed.
• Additionally, we would create several keyboard shortcuts in the jlab shortcut system that would use the focus tracker to identify the current active editor and execute the appropriate command. Using the jlab shortcut system makes things a little more consistent with the rest of jlab, and opens up the possibility of having menu items and command palette commands that do these operations (indent/dedent/block comment) though there might be some subtleties dealing with focus.
• (edit) This focus tracker could be exposed to the system for others that wanted a handle on all created codemirrors for whatever other customizations they wanted to do.

What do you think? Is that too big of a change at this point? It means that the codemirror extension doesn't have to depend on the file editor nor the notebook, and means that settings automatically propagate to all codemirrors (for example, they would also propagate to Code Consoles, which isn't happening now with this PR).

Another thought is that we continue with the hacks that you have in place in the VIM extension, without this PR, and we explore this design further for a 3.1 or 4.0.

[jasongrout]

Design question: do you do a lot of customization of the codemirrors in the notebook in your vim extension, or (as far as the codemirror is concerned) do you basically just set the keymap?

[ianhi]

What do you think? Is that too big of a change at this point? It means that the codemirror extension doesn't have to depend on the file editor nor the notebook, and means that settings automatically propagate to all codemirrors (for example, they would also propagate to Code Consoles, which isn't happening now with this PR).

That sounds more sensible than the current situation and definitely a good end goal. Unfortunately it is also almost certainly too big a lift for me to do alone, especially given the 3.0 timeline.

Another thought is that we continue with the hacks that you have in place in the VIM extension, without this PR, and we explore this design further for a 3.1 or 4.0.

That would be fine, the vim extension works satisfactorily as it stands. I was hoping to use the newCellCreated signal in the vimrc extension but I could pick up similar activeCellChanged hacks. The biggest bummer with this would be losing the block comment shortcut setting. Is merging this PR and then doing the better thing for 3.1 a reasonable option? A downside would be that the location of the setting would change from 3.0 -> 3.1.

Design question: do you do a lot of customization of the codemirrors in the notebook in your vim extension, or (as far as the codemirror is concerned) do you basically just set the keymap?

Pretty much just the keymap. The vim extension also plays around with the extrakeys and defines a command that enables moving between cells using j and k https://github.com/axelfahy/jupyterlab-vim/blob/c4e43f940ef4be4c961608b6192412d2f3a33d1f/src/index.ts#L85 but thats pretty minimal.

A few people have also asked for relative line numbers (mostly on jlab-git for some reason???) #15436 which I'd like to add as well. That would also require setting up an event listener on the editor codemirror/codemirror5#4116

function showRelativeLines(cm) {
  const lineNum = cm.getCursor().line + 1;
  if (cm.state.curLineNum === lineNum) {
    return;
  }
  cm.state.curLineNum = lineNum;
  cm.setOption('lineNumberFormatter', l => 
    l === lineNum ? lineNum : Math.abs(lineNum - l));
}
editor.on('cursorActivity', showRelativeLines)

[jasongrout]

The biggest bummer with this would be losing the block comment shortcut setting. Is merging this PR and then doing the better thing for 3.1 a reasonable option? A downside would be that the location of the setting would change from 3.0 -> 3.1.

Yes, I was trying to think of a way to have the block comment shortcut setting at least. You hit the nail on the head with the complication I was weighing - the change the setting if we moved to jlab shortcuts instead of just a normal setting.

However, since at least the block comment would be an additive change, we could just target 3.1 with it.

[jasongrout]

Unfortunately it is also almost certainly too big a lift for me to do alone, especially given the 3.0 timeline.

Moved to 3.1, then. I can work with you after 3.0 is released to get this in.

[krassowski]

It seems that the discussion on improving the dependencies was moved to #10352 and @jasongrout has some recent experiments on this topic in jasongrout@8a6a27d

Especially this one seems relevant:

A much simpler approach would be to keep the existing code for applying defaults (i.e., they do not update in-place, and are only applied at application launch) and just tell the user that they need to refresh if they ever change the codemirror defaults.

[JasonWeill]

There hasn't been any activity on this pull request in over a year. As part of an effort to clean up this repository's pull requests, I'm closing this request. If you would still like to merge this code in, please reopen the request and resolve any outstanding merge conflicts. All of the code and comments will remain in place. Thank you for being part of the JupyterLab community!