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

Drop the CodeEditor abstraction #10380

Open
jtpio opened this issue Jun 10, 2021 · 5 comments
Open

Drop the CodeEditor abstraction #10380

jtpio opened this issue Jun 10, 2021 · 5 comments
Labels
api-change A change that should be accompanied by a major version increase enhancement
Milestone

Comments

@jtpio
Copy link
Member

jtpio commented Jun 10, 2021

In the weekly dev meeting yesterday, we discussed whether we should drop the CodeEditor abstraction. The overall sentiment was positive, with ~15 people in the call.

This would be a 4.0 change.

Some core packages already rely on editors being Codemirror editors, for example the debugger extension.

Focusing on a single code editor (Codemirror) would make it easier to fully leverage all the available APIs.

Related: #10370

@jupyterlab/committers, extension authors and anyone else: feel free to leave a 👍 or 👎 to get a clear idea of the general sentiment. Thanks!

@jtpio jtpio added this to the 4.0 milestone Jun 10, 2021
@jasongrout
Copy link
Contributor

Thanks for opening this. Several people in the meeting also said we should move directly to CodeMirror 6 in 4.0, and that it is pretty stable at this point.

@fcollonval
Copy link
Member

fcollonval commented Jun 10, 2021

move directly to CodeMirror 6

👍

@vidartf I know that CM author decided to not port the merge view feature of CodeMirror 5 to 6: codemirror/dev#327

This will presumably require some efforts to get nbdime and jupyterlab-git compatible with 4.0. But this is not a blocker.

@bollwyvl
Copy link
Contributor

Definitely a big 👍 on CM6. I think if we fully engage with it, there will be some good opportunities for using things like DecorationSet to make huge challenges of jupyterlab-lsp just... disappear. And the accessibility impact cannot be overstated.

However: while we're taking a breath...

I would posit that ProseMirror document editor (esp. the code editor and track changes examples) show depth of integration (and composability) on top of a more-broadly-familiar UI (e.g. word processing) that we might really not want to try to re-build. Indeed, this Document-first view might end up being what a lot of different audiences might want first, and by default, even for notebooks that were authored in a traditional cell view.

If these could be made to fit the existing API, then CodeEditor might still be useful to keep.

So you'd basically know, "i have a line/column-focused source editor", but not necessarily know whether that editor is rich text or plain text (or currently be rendered as both for e.g. math.

My thinking is:

  • eschew marked or markdown-it for prosemirror as the markdown renderer
    • the solutions (and ports) have not proven to be particularly extensible at scale, in practice
  • offer an alternate, yet first-party, notebook frontend which uses all the existing notebook model stuff, delivered by ProseMirror e.g.
    • 1 PM on the page, with "magic" mapping to CellModels
    • n prosemirrors (one per CellModel) on the page
    • some mix of the two, e.g.
      • a "master" Markdown cell which...
        • ...transcludes cell (parts) by local cell id
  • extend appropriately so it can embed all the fixings
    • CM input areas (that "just work" with e.g. jupyter-lsp)
    • output areas
      • esp widgets
    • attachments
  • backfill the mostly-compatible observable aspects currently offered by CodeMirror (e.g. selections, etc). and work with the Yjs stuff

@dmonad
Copy link
Member

dmonad commented Jun 15, 2021

ProseMirror indeed provides a very nice abstraction over rich-text content. If you want to express complex content like tables and if you want to make sure that your content conforms to some schema, then ProseMirror is awesome.

But it also has a very steep learning curve. Currently, Jupyter Notebooks only consist of a list of markdown, code, and raw cells + output. It doesn't require rich-text support (markdown != rich-text) and we don't necessarily need a complex abstraction over the different cell types. Actually, we just created a very handy abstraction that extension authors can work agains: the SharedNotebook API in the shared-models repository. If an extension author wants to access content, render selections, or manipulate cells, then they should use the SharedNotebook abstraction. It is very easy to use and allows us to change the renderer (e.g. from CodeMirror 5 to 6) without breaking extensions.

With the SharedModels package I was actually hoping that we could simplify the project by throwing out ModelDB (in 4.0) and just having a simple renderer for the few different kinds of cells using a single editor. This would make it easier for newcomers to understand and advance the codebase.

@fcollonval fcollonval added enhancement api-change A change that should be accompanied by a major version increase labels Jul 7, 2021
@jtpio jtpio mentioned this issue Dec 16, 2021
4 tasks
@JohanMabille
Copy link
Member

A few thoughts on removing the codeeditor package: while I think we should definitely drop the IEditor interface, we may not want to remove all the abstractions and propagate all the CM6 types everywhere:

  • having a mapping of basic values <-> CM extensions is very convenient for configuration and serialization.
  • the parsing is very different and does not provide a list of tokens anymore but rather an iterator over the tokens. Keeping the IToken interface and a function to create an array of such tokens from the iterator may be convenient (even if suboptimal)
  • even if CM6 internally works with an offset, having a Position type (with line and column members) has proven to be convenient, and the jupyterlab codemirror package already provides conversion functions. As long as we don't go back and forth it should be fine to keep a Position type. However, we should adopt the CM6 convention regarding the first line number (1 in CM6 vs 0 in CM5).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-change A change that should be accompanied by a major version increase enhancement
Projects
None yet
Development

No branches or pull requests

8 participants