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

Revamp editor settings #3441

Merged
merged 5 commits into from Dec 21, 2017
Merged

Conversation

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Dec 20, 2017

Fixes #3358.

This restructures settings for the code editor instances, for both the notebook and the text editor.

  1. The editor settings JSON schema now exactly matches the CodeEditor.IConfig interface, so all settings exposed in that interface may be set using the settings registry.
  2. The notebook has an IEditorConfig object, which has three CodeEditor.IConfigs, one for each of code, markdown, and raw cells. These config objects may be customized individually through the settings system, so each cell type can have their own settings.
  3. The toggle all line numbers command for the notebook is retained for feature parity, though it fits less well within this framework. It is considered not toggled if any of the three cell types have line numbers turned off, and toggled if they all have line numbers turned on.
@ian-r-rose ian-r-rose changed the title [WIP] Revamp editor settings Revamp editor settings Dec 20, 2017
@blink1073
Copy link
Member

@blink1073 blink1073 commented Dec 20, 2017

Bellisimo! The classic notebook allows you to toggle the line numbers for all types of cells with one command, I think that is useful.

@blink1073 blink1073 added this to the Beta milestone Dec 20, 2017
@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Dec 20, 2017

Okay, I will restore a command to do that.

@blink1073
Copy link
Member

@blink1073 blink1073 commented Dec 20, 2017

I'd say base the toggle on the state of the active cell.

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Dec 20, 2017

Roger. This will mean mostly reverting 8c482f2

@blink1073
Copy link
Member

@blink1073 blink1073 commented Dec 20, 2017

Ah, sweet, it was already doing that.

@ian-r-rose ian-r-rose force-pushed the revamp_editor_settings branch from 4ff41ca to cc6e3e3 Dec 20, 2017
@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented Dec 21, 2017

I played with it locally and have a few questions:

  • Do we really think users need to configure the editing experience separately for code/markdown/raw cells? What is the usage case for that?
  • I see there is still a top-level CodeMirror config section. How does that interplay and relate to the others?
  • Should indent mode be on a per file basis as well as a global setting? Most of us keep tabs=spaces enabled, but some file types require tabs (Makefiles). What is the best logic for that?
  • How far does this go in getting rid of the more abstract editor ideas? I know we have talked about stopping trying to abstract out all possible editors. Wondering how this PR relates to that.

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Dec 21, 2017

To your questions/notes:

  1. The reasoning was as follows: we had been hard-coding editor settings for the three types of notebook cells. For instance, auto-closing brackets were turned on by default for code cells, but not markdown cells. This made sense for a lot of use-cases, but were not necessarily the best fit for everyone (cf. #3358). At the same time, we have an abstracted CodeEditor.IConfig interface that, at the very least, represent a reasonable subset of settings implemented by a text editor. Rather than hard-coding those settings for different cell types, this provides those same settings as sensible defaults, but allows them to be configurable. Essentially, this takes the IConfig interface and exposes it to users through the settings (rather than a subset of it that had been exposed previously). I do not know of specific cases where people would want different settings for raw cells, but I can easily think of cases where I would want to be able to configure the editor behavior of code or markdown cells to my personal preference. And at that point, it seemed odd to leave raw cells out.
  2. The CodeMirror config is a superset of those CodeEditor.IConfig settings, and is mostly ignored by the notebook (though look at the nascent jupyterlab_vim plugin for an example of an extension that uses them). Those settings (keymap, indentation, theme) are applied to text editors, and are available through the settings editor or the Settings menu.
  3. This is something that we probably want to think through a bit more (though maybe in another PR?). The previous behavior for the indentation was only on a per-editor-instance basis, overriding the hard-coded behavior (and not for notebooks at all). With this PR, those settings are configurable and persisted in the settings, so it is a bit more sticky. This is not necessarily better for the case of Makefiles, but I don't think it is worse, either. Furthermore, it is strictly more flexible in the case of notebooks.
  4. This does not really push forwards on getting rid of the abstract editor (and, as you have noted, uses the abstract CodeEditor.IConfig as the interface for the persisted settings). However, I would suggest that, even if we do dispose of the abstract editor (in favor of something like Monaco), we will still want to define a subset of the settings for that editor to be persisted as settings. So we might still want something that looks similar to this.

@blink1073
Copy link
Member

@blink1073 blink1073 commented Dec 21, 2017

I'm coming around to the idea that at the level below an extension we should continue to use the abstract editor API. We can use the concrete editor at the extension level and take advantage of settings specific to that type of editor. I can't think of anything at the notebook widget level or lower that would need to differentiate between editor types. We have other tools like the completer and tooltip that similarly don't need a concrete editor API.

I also agree that per-cell type settings are useful, and they exist in the classic notebook.

Sublime allows you to set indentation rules globally, per file type, and per buffer. I think we should allow for a similar pattern.

@afshin
Copy link
Member

@afshin afshin commented Dec 21, 2017

@ian-r-rose, this is awesome! The new schemas you wrote are the most complex settings schemas we have in the codebase. Since they're more complex than the ones that the settings system was built on, it's a good test. Did everything work okay for you?

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Dec 21, 2017

@afshin Yeah, the settings system worked great! I am not too familiar with JSON schemas, so there was some fumbling around on my part, but the setting editor+registry did just fine with custom definitions, a more nested structure, etc.

@ian-r-rose
Copy link
Member Author

@ian-r-rose ian-r-rose commented Dec 21, 2017

@blink1073 Yes, I agree that it would be useful to have global (as this PR does), per-extension, and per-buffer settings, especially for indentation. I see no reason why we couldn't extend the schema to do that, though perhaps we should wait on that until post-beta?

@blink1073
Copy link
Member

@blink1073 blink1073 commented Dec 21, 2017

We agreed in the meeting today to defer per file settings. Thanks!

@blink1073 blink1073 merged commit b9c1fb8 into jupyterlab:master Dec 21, 2017
2 checks passed
@blink1073 blink1073 mentioned this pull request Dec 23, 2017
1 task
@blink1073 blink1073 mentioned this pull request Dec 29, 2017
@lock lock bot locked as resolved and limited conversation to collaborators Aug 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants