CodeMirror element (continued)

[chrschorn]

Continuation of #2775

I was able to make things work reliably by chaining the loading of the js resources in the correct way. The value of the editor is also correctly in sync between client and backend.

Video of codemirror_test.py:

chrome_yc4178WYoT.mp4

This element is using CodeMirror 5, even if some of the URLs say 6.x. I spent a couple hours trying to make this work with CodeMirror 6 (the latest version), but this is not trivial because with v6, the distribution method (and APIs for that matter) have completely changed. Using CodeMirror 6 requires you to use a bundler since it is distributed in many small modules. We don't seem to have a reusable setup for that in the repository and I've tried extending npm.json and npm.py to still make it work (you need like 7+ npm modules) but gave up in the end.

If CodeMirror 5 is okay for everyone, I will continue finishing a first iteration.

Things still missing:

1. Making it explicit which languages and theme names are supported
2. Docstrings, documentation content
3. Extend the test (?)

Is there anything else we should add for a first version?

[rodja]

Wow, @chrschorn that is awesome.

Using CodeMirror 6 requires you to use a bundler since it is distributed in many small modules. We don't seem to have a reusable setup for that in the repository...

Yes, there may be some changes coming this way in 2.0. @falkoschindler has some ideas to improve dependency management. But just for my better understanding: would it not be possible to use the Rollup Bundler by hand and simply commit the resulting js files to the repository?

[chrschorn]

I briefly considered bundling, but this being my first PR here, I didn't want to introduce a new dependency managment method without first getting input from you.

We could follow the example here by creating a script (plus a editor.mjs) that uses a real npm installation ("real" as in "not the npm.py from the repo") to fetch and bundle the parts of codemirror 6 that we want. The js output(s) could go into nicegui/elements/lib/codemirror and then we create the element as usual.

Ideally we would follow this advice to load only the required languages:

When you need to support multiple languages, it can often be useful to dynamically load the language support packages as needed to avoid the amount of code the browser has to load. The Rollup documentation can tell you more about how to do this.

What do you think?

[rodja]

Sounds good to me. @falkoschindler are you ok with having a custom solution to create the delivered JavaScript for the time being? I think it would be good to have the latest version available in NiceGUI. In their version 5 repo they write

NOTE: CodeMirror 6 exists, and is more mobile-friendly, more accessible, better designed, and much more actively maintained.

[falkoschindler]

We could follow the example here by creating a script (plus a editor.mjs) that uses a real npm installation ("real" as in "not the npm.py from the repo") to fetch and bundle the parts of codemirror 6 that we want. The js output(s) could go into nicegui/elements/lib/codemirror and then we create the element as usual.

Yes, that sounds like a reasonable solution until we find a way to support such NPM dependency trees more conveniently in NiceGUI. 👍🏻

[chrschorn]

These two lines caused the source to be copied to /, which gave me recursive import errors when running pre-commit. This fix only installes the dependencies, not nicegui itself. This is done in devcontainer.json now which was previously disabled.

[chrschorn]

There is one too many } here, causing the two lines below to never execute up until now.

[chrschorn]

Hi, this is what I got after digging deep into CodeMirror 6 and the packaging system 🙂

chrome_U6mOBmkveO.mp4

This video is recorded with the codemirror_test.py which I will remove for the final PR (but could be kept as an example?)

Things implemented:

• CodeMirror's "basicSetup", which already includes basic auto complete (it knows builtins and existing variables, but not intellisense level), code folding, undo history, etc. Full list: https://codemirror.net/docs/ref/#codemirror
• The whole thing is bundled using the scripts/codemirror/bundle.bash script, which will do everything in one go (install npm, dependencies, then bundle to nicegui/elements/lib/codemirror/
• Support for 140+ languages. This was as simple as including @codemirror/language-data and writing the code to dynamically change the language. Full list of supported languages: https://github.com/codemirror/language-data/blob/main/src/language-data.ts
• Support for 30+ themes (full list: https://github.com/uiwjs/react-codemirror/tree/master/themes/all)
• Pretty okay javascript size! The unminified code for just the editor (no language packages) is 1.1MB, but the minified version is 440kB, which according to chrome devtools gzips down to just ~140kB while the file is sent to the client.
  • Language packages are dynamically loaded, hence the many new js files included in this PR
• Dynamic language and theme setting
• Editor can be disabled
• Value is sync'd reliably with the server. Initially I had some race-condition type issues where, when typing to fast, an infinite loop or cursor jumping could happen.
• Smaller features such as optional line wrapping, whitespace highlighting, min/max/fixed height
• Supported features/languages can be retreived after the editor has been loaded

Please have a look and let me know if you want me to change things, move files around, clarify some code sections, etc.!

[chrschorn]

Side note: Quite late into implementing this, I found a CodeMirror Vue component: https://github.surmon.me/vue-codemirror However I wasn't sure how to include this into NiceGUI and if its possible at all / would save work.

[rodja]

Wow. That looks great. Thanks for all the hard work you've put in. I only glanced at it and want to share some thoughts:

1. Why have you picked ValueElement? I think it's more like ui.markdown eg. a ContentElement
2. I'm not sure we should use Python and other settings as defaults -- plain text would be more generic in my opinion
3. Each keystroke submits the whole content to the backend. Maybe with @codemirror/collab we can implement changeSets and make the transfer more efficient?

[falkoschindler]

@rodja Like other input elements (ui.input, ui.textarea, ui.editor, ...) I'd also expect the CodeMirror element to be a ValueElement. 🤔

[chrschorn]

1. Should I move the the component to the Controls section of the documentation then? It's a mix between an input element like textarea and a data element like code for sure.
2. Fine by me, I'll set plain text as the default
3. After looking at https://codemirror.net/examples/collab/ it seems doable to send changeSets from client to server (need to implement mainly https://github.com/codemirror/state/blob/main/src/change.ts#L404 in Python). For a server->client value update I'd like to stick to sending a full update for now as it's much more involved to build the changeset and getting the communication working correctly in that direction. That could be part of a future "full collaborative editing" PR.

btw, are there instructions to run the tests in the devcontainer or do you usually do that on your host machine?

[rodja]

1. I think @falkoschindler is right. Codemirror is more about inputting then viewing so the controls section is more appropriate.
2. Great!
3. Sounds like a good plan. This PR is already huge and sending changeset from Backend to the Frontend could be a good addition after we get the basic functionality through the door.
4. the tests should run out of the box in the dev container; have you tried running pytest?

[chrschorn]

Alright, all implemented!

Tests worked now. For some unknown reason they didn't work on the first try so I thought there were some extra steps required in the devcontainer to install selenium etc.

[rodja]

Thanks for updating the branch with latest main and using the new ui.context. But we would prefer you to avoid force-pushing as much as possible because it complicates the review process.

[chrschorn]

Sure thing, sorry about that. It's a GitLab habit (rebase is the default in PRs over there).

[falkoschindler]

Thanks a ton for this amazing pull request, @chrschorn!

Even though I don't yet understand every detail of the JavaScript code interacting with CodeMirror, the implementation looks pretty nice and the communication based on change sets seems to be very efficient. I just made a few changes:

• I think I simplified the height definition by setting a default height of 16rem for the ui.codemirror element like we did for similar elements. Using a height of "100%" for the editor view makes it fill the whole element to reach the desired size. Applying Tailwind classes or CSS style seems to work as expected.

• I pre-generated lists for supported languages and themes. This simplifies the demo quite a bit, because we don't need to call async code. An extra pytests makes sure to stay in sync with the library in case the list grows with the next update.

• I set the default language to None, because I think this is more intuitive - even though I noticed a visual difference between using "plaintext" vs. no language.

• Oh and I switched to "basicLight" as the default theme, because that's NiceGUI's default.

@rodja Can you have a look at the two Docker-related changes? Should we merge them or wait for PR #2964?

[chrschorn]

Hi, thank you for the review!

I think I simplified the height definition by setting a default height of 16rem for the ui.codemirror element like we did for similar elements. Using a height of "100%" for the editor view makes it fill the whole element to reach the desired size. Applying Tailwind classes or CSS style seems to work as expected.

With the new changes I can't get any max or min height tailwind classes to work, have you tested those? Fixed sizes like h-40 seem to work fine, but maybe try putting max-h-40 and add some code or empty lines. On my end the layout breaks in this case. I initially added the min/max height code because what CodeMirror recommendds is to add classes on some of the underlying dom elements, which I wanted to spare the user from. Example with max-h-20:

I'd like to add an easy way to set min/max height back in.

I pre-generated lists for supported languages and themes. This simplifies the demo quite a bit, because we don't need to call async code. An extra pytests makes sure to stay in sync with the library in case the list grows with the next update.

The pre-generation is nice and makes the UI much easier to setup 👍 I had to add plaintext back in to codemirror.js to make the tests pass. I also sorted them nicer now so the ordering makes more logical sense (lower case languages came after upper case, now they don't).

I set the default language to None, because I think this is more intuitive - even though I noticed a visual difference between using "plaintext" vs. no language.

I was torn here:

• I agree None is a nicer default
• "plaintext" provides an entry in supported_languages that can be used in the UI. Without an entry for None/plaintext, I would need to do something like

  languages = {l: l for l in cm.supported_languages}
  languages["plaintext"] = None

  to have a ui.select that allows you to go back to None.
• We could make both work: None and "plaintext" work, which is what I've implemented now

[falkoschindler]

Min/max height
Oh, I didn't check setting min or max heights. I'll look into it and revert my changes in case there is no better way.

Supported languages
Strange. I wonder why the tests passed with my code. But maybe they didn't and I simply forgot to check. Anyway, I think it's less confusing to leave "plaintext" out of the list of supported languages, since it isn't an officially supported language, the name is a bit arbitrary, and None would be kind of a duplication. Using the clearable parameter for ui.select the demo UI is also pretty simple. Now it's even more obvious that there is a "no language" option.

[falkoschindler]

@chrschorn Maybe I'm missing something, but it seems like all height classes are working as expected:

with ui.row(wrap=False).classes('w-full'):
    ui.codemirror('256px (default)')
    ui.codemirror('300px min').classes('min-h-[300px]')
    ui.codemirror('100px fix').classes('h-[100px]')
    ui.codemirror('100px max').classes('max-h-[100px]')

If you try it yourself, make sure to clear the browser cache so the current nicegui.css is loaded.

[chrschorn]

What I can't get to work with just tailwind or css on the main element is an editor that has a dynamic size based on content, e.g. starting at 100px and it can expand up to 300px. The examples you posted are all stuck at exactly one height and don't allow the editor to grow. Maybe you have other ideas to make it work?

with ui.row(wrap=False).classes('w-full'):
    ui.codemirror('256px (default)')
    ui.codemirror('300px min').classes('min-h-[300px]')
    ui.codemirror('100px fix').classes('h-[100px]')
    ui.codemirror('100px max').classes('max-h-[100px]')
    ui.codemirror('500px max').classes('max-h-[500px]')
    ui.codemirror('100px min 300px max' + "\n"*30).classes(replace='w-full min-h-[100px] max-h-[300px]')
    ui.codemirror('100px min 300px max').classes(replace='w-full min-h-[100px] max-h-[300px]')

with ui.row():
    for _ in range(200):
        ui.label("text")

We can also merge is at is though if you're fine with a fixed height for now, I'm happy with the rest 👍

[falkoschindler]

Ah, I think I finally understand! A content-based height doesn't work.

But actually I'd leave it this way for now, because:

• An editor that grows based on content seems to be a rather special use case.
• Extra height arguments might be confusing to use in combination with CSS and Tailwind.
• Right now the behavior is in line with other NiceGUI elements like ui.aggrid, ui.echart, ui.leaflet, ui.log and ui.scroll_area.
• Less code = simple = better.

So let's finish this PR and merge it into main. Great work, @chrschorn! 🚀

[chrschorn]

Sounds good. Happy to contribute - I really like NiceGUI! Looking forward to have this thing released!

[rodja]

@chrschorn would you be interested in creating another pull request to introduce change sets from server to client? As mentioned that would be great for collaborative editing. But it could also be used as powerful alternative to ui.log or to implement AI-auto-completions...

[chrschorn]

@rodja Unfortunately I don't have a lot of time at the moment, but I do see the potential for cool features based on this as well. If you or someone else wants to work on this, please go ahead.