Desktop: Replace Ace with Code mirror #3284
Conversation
There was a problem hiding this comment.
Thanks for looking info this @CalebJohn. There are a few things that a new text editor should support to really make it worthwhile. I've noted the following in particular:
- Spell checking
- Variable font size
- Search within text (and highlight search results)
I don't know if CodeMirror supports all this but would be worth checking.
I've also made a few comments on the code, but the main thing is that the editor should be a separate component that implements NoteBodyEditorProps and there shouldn't be a need to modify AceEditor files.
It should be easy to get them working side by side and the added benefit is that we can even release the app with CodeMirror even if it's not finished (as beta, like for TinyMCE).
| @@ -5,14 +5,15 @@ import { useState, useEffect, useRef, forwardRef, useCallback, useImperativeHand | |||
| import { EditorCommand, NoteBodyEditorProps } from '../../utils/types'; | |||
There was a problem hiding this comment.
Any reason for the changes on Ace Editor related files? The app can handle multiple text editors, which makes it much easier to migrate to other editors. In NoteEditor.tsx, you could simply import your CodeMirror component, then if props.bodyEditor === "CodeMirror", load it.
So I guess instead of changing the AceEditor files, you'd just copy what you've done so far to NoteBody/CodeMirror and revert the changes on the AceEditor files. If you need some more info on how to get it working let me know.
There was a problem hiding this comment.
Basically just laziness, I didn't know the new aspects of the code very well and I didn't want to waste time figuring out how to support a third editor before I even had one working.
I was initially planning on doing exactly what you suggested before making this public but decided it was better to get some visibility first. I'll definitely make those changes before taking this out of draft.
| export interface EditorProps { | ||
| value: string, | ||
| mode: string, | ||
| style: any, | ||
| theme: any, | ||
| readOnly: boolean, | ||
| autoMatchBraces: boolean, | ||
| keyMap: string, | ||
| cancelledKeys: CancelledKeys, | ||
| onChange: any, | ||
| onScroll: any, | ||
| onEditorContextMenu: any, | ||
| onEditorPaste: any, | ||
| } |
There was a problem hiding this comment.
The editor should support the NoteBodyEditorProps (doesn't have to support all of them, but that should be the input).
There was a problem hiding this comment.
I've restructured the code a bit, so what you're looking at here is basically a react wrapper around codemirror. There is another component that handles the integration with Joplin. Confusingly I've renamed that other one to CodeMirror.tsx and called the wrapper Editor.tsx.
ElectronClient/gui/NoteEditor/NoteBody/AceEditor/CodeMirror.tsx
Outdated
Show resolved
Hide resolved
Move special functions into the utils folder broke up wrapSelectionWithStrings into the utils folder
|
In reference to your 3 items.
Overall we also benefit by having an editor that's easier to interact with and won't require as much hacking to get basic functionality. |
Drag and drop with selected text now works Drag and drop will actually drop at the cursor position When using toolbar buttons (or hotkeys), selections are now intuitive
Eventually, I hope we can use the Atom spell checking package throughout the app to load dictionaries and get spelling suggestions, then each component (CodeMirror, TinyMCE, and title text input) would handle displaying the results. As long as CodeMirror has a well supported way to highlight terms then it's most likely good enough. Last time I've checked this with Ace Editor it was quite complicated.
Ok that's good to know. It might be that we leave it as monospace by default, but it's good if users can change it, and that will also allow us to improve the default editor styling, with larger heading, bold fonts, etc.
Ok that should work then, likewise if there's a well supported way to highlight terms it should work. |
|
This is ready to be reviewed I think. I have all the functionality (that I know of) that was in the original AceEditor and seems to be bug free (from my testing). But I plan to use this branch myself for the next few days to be sure. What I haven't done yet, but would like to do, is add a new file to the documentation that outlines how to extend the codemirror editor component, so standby for that before merging. |
|
Thanks for the update @CalebJohn. Obviously it's a huge PR and not so easy to review. There's a lot of duplicate code between the AceEditor and CodeMirror files so long term we should only have one "Code View" editor to keep things maintainable. If CodeMirror is superior we'll use it and drop Ace Editor. One thing I didn't find is the vim/emacs support? You've imported some files related to it, but I'm not sure they are used yet? Also is there any reason why the editor code is split between CodeMirror.tsx and Editor.ts? I can sort of see why you've done that, but the way I see it CodeMirror.tsx should have direct access to the editor instance, without a wrapper in between. The wrapper means we'll need to have some kind of bridge so that CodeMirror.tsx can access the properties and methods of the editor. I feel that's not necessary and might make the code more difficult to maintain. Also the role of each file is not entirely clear, I guess Editor.tsx is lower level, but I think there's some overlap. For AceEditor.tsx and TinyMCE.tsx, I've put the editor instance directly accessible from the component. Then I've used a bunch of React hooks to lower down the line count and keep things more manageable. What do you think? Would it make sense to put the editor instance in CodeMirror.tsx? By the way we haven't really discussed the various code editors. Obviously there are other ones, like Monaco for instance, and CodeMirror 6 might be released some day. Any reason why you went for CodeMirror in particular? |
👍
This is because of the way codemirror plugins work, during import the plugin registers itself with the codemirror instance, so we just need to pass it a keymap name (default, vim, emacs) and codemiror will handle the rest.
What I was going for was as you've guessed. Separating the Joplin behavior from the codemirror behavior. Which I did for a few reasons.
I actually did end up giving CodeMirror.tsx direct access to the codemirror editor, but most if not all the interactions with it are handled by higher level functions defined in one of the utils modules. Since this is such a large pull request (and I think you'll find it's not quite as large as it seems), I would be open to discussing it in a bit more detail over the discord, that way I can give you a much quicker introduction to why I structured it like this and how everything interacts.
Not a terribly strong reason, but the Jupyter environment uses codemirror to power it's markdown editor (with variable font size) so I have some trust in it. I compared markdown support in Monaco as well and it seemed to be lacking the advanced features that were either already in codemirror (variable font size) or could be added fairly easily (inline images, etc.). I like how easy it is to write plugins for codemirror so even if we have to add support for certain features ourselves it won't be as much of a pain as it was with aceeditor. On the topic of codemirror 6, I just don't trust that it will be ready for use in Joplin for at least a few years, so as long as we keep the codebase manageable we should be able to upgrade then (or stick with the more stable codemirror 5). |
|
Thanks for the info @CalebJohn, I'll go through it again and get back to you. I just thought about over things that ideally should work in a new editor:
All these I expect should be fixed if the editor properly handle unicode and non-monospace fonts, but worth checking anyway. |
|
Thanks @laurent22 I had meant to check these cases but I forgot. I couldn't reproduce #3134 but I reached out to the poster and I hope to check it soon. That one may be difficult to reproduce because I think it might be a font issue, but I'm fairly confident it will be fixed. |
|
Thanks for checking this @CalebJohn. I've just gave a try to the branch and didn't find any issue, and since your changes are quite well isolated I'm keen to merge this so that we can start using it everyday, and find any remaining edge case if any. If you find any issue, feel free to open a new pull request for it, and thanks again for this great improvement! |
|
Thanks Laurent! Fingers crossed there aren't too many issues! |
for reference #3269 #3236 #3054 #3229 #3147 #1762
As mentioned on the forum I've been experimenting with a transition to codemirror rather than ace editor. I still haven't finished the transition yet but so far things are looking really good. There was a lot of code in the old module that was necessary to get around Ace's quirks that has been removed. I still have quite a bit of refactoring to do but I have a functional base here if anyone is keen to test.
Perhaps more importantly codemirror works with non-monospace fonts :) It can also handle varying line heights (adjustable with css), and it supports inline images (will need to be added in a different pull request).
