Limit inheritance of Editor and EditorUI classes

[Reinmar]

In order to create your custom editor you need to understand currently the following paths of classes (assuming that you're creating your editor based on the classic one):

• Editor -> StandardEditor -> ClassicEditor
• (Controller) -> EditorUI -> BoxedEditorUI -> ClassicEditorUI
• (View) -> EditorUIView -> BoxedEditorUIView

Both chains are long and smell bad (cause you should favor composition over inheritance). We also noticed that the editor interfaces must be exposed (#303) for the feature authors to understand whether a proper editor was used.

OTOH, this chain of inheritance let us limit the length of the ClassicEditor to the real minimum. Also, there's nothing wrongly placed at the moment – each class indeed defines the behaviours that it introduces. If you know the base classes that you want to use for your editor (e.g. StandardEditor, BoxedEditorUI and BoxedEditorUIView, you really don't need to code or care much.

So, I'm curious whether we could change much in this picture. Some ideas that I have:

• Merge Editor into StandardEditor and document what part comes from where by defining interfaces (the StandardEditor will implement a couple of them).
  • Pros: most editors will base on the StandardEditor and their implementers will need to know only this one.
  • Cons: There's a lot of initialisation going on in the Editor class and all this will have to be moved to the StandardEditor. It can also be moved to a helper function (kinda mixin which could e.g. be applied in the create() method – in fact, the initPlugins method is already used there, so it could also be moved out of the class).
• EditorUIView doesn't make much sense (at least at the moment) – we could get rid of it by extracting the icon logic to a helper. It'd also be nice if we could symmetrically remove EditorUI, but this class makes more sense.
• BoxedEditorUI – it's similar to EditorUIView – it doesn't contain any interesting behaviour. It only defines things.

The resulting architecture would be:

• StandardEditor (see ad1) -> ClassicEditor
• (Controller) -> EditorUI -> ClassicEditorUI (see ad2)
• (View) -> BoxedEditorUIView (see ad3)

(ad1) implements sth like EditorInterface, StandardEditorInterface.
(ad2) needs to contain the minimum code which BoxedEditorUI contains now.
(ad3) defines everything that this specific view has.

[pjasiun]

In fact, even for me, who knows part of these classes very well, it is unclear what class I should use and reading the code is hard because of the long chains.

EditorUIView doesn't make much sense (at least at the moment) – we could get rid of it by extracting the icon logic to a helper. It'd also be nice if we could symmetrically remove EditorUI, but this class makes more sense.

I agree. I believe it is much simpler to understand code when it uses helpers instead of base classes.

BoxedEditorUI – it's similar to EditorUIView – it doesn't contain any interesting behaviour. It only defines things.

In fact, the class was the most unclear part for me: should I use it? When?

I think even if we merge part of the logic and user does not need it all, it is still simpler for him to copy the class and modify it. Now I need to merge classes first to change them later.

Also, I think it is partially the matter of the documentation. We do not have a good docs overview about the UI library, what does not help. For instance, I have no idea what this code do: https://github.com/ckeditor/ckeditor5-ui-default/blob/d4a6ee75e5ef50317fd0637c86c6b79a61522eeb/src/editorui/editorui.js#L48 and this doc does not help: https://github.com/ckeditor/ckeditor5-ui/blob/eca89ac35ad4b14896a05a6b1c2c8d67ceb52086/src/controller.js#L156

At the same time, our way of building ui does not help when you want to find a class by its import path.

[pjasiun]

Merge Editor into StandardEditor...

And when I was thinking about this, I realised that StandardEditor should be spilt. setData and getData methods are not a part of the editors API for features, so the should not be merged into the Editor class. They should be a part of the specific implementation (in our case ClassicEditor), or not if the implementation wants to have a different API. About the rest, I agree that it's a good idea to merge it into the Editor.

[Reinmar]

It only depends on what StandardEditor is meant to represent. How specific it's meant to be. If we leave such things like "whether an editor has set/getData() methods" to implementers of end-editor classes, then the super basic API may different between similar ones. So this is more of a recommendation here...

[pjasiun]

I understand StandardEditor as the features API, so the set of methods and properties features can expect that editor passed to them will have. I do not think that getData and setData should be part of such API. And I already have a case for the editor where I do not want to have getData and setData. I have title and body and getTitle, setTitle, getBody and setBody makes more sense for me. I do not know why I should be forced to have getData and setData in my API too.

[Reinmar]

Dunno if that was mentioned already, but we definitely discussed this – one of the main goals here should be to create such interfaces which the features will be able to rely on. Right now, most features require big part of StandardEditor, but with the exception of setData() and getData() and similar methods. Let's clarify what most features need and what has a bigger chance to be editor's implementation specific.

EDIT: I think that it was covered by #303.

[Reinmar]

I think that we need to answer here one question – what kind of editors we use really.

I can see two types:

• Normal editors.
• DOM-independent virtual editors (for tests which only require the model to be working).

I suggested merging Editor and StandardEditor into one class, but this would mean that it would accept element as its first param. Right now we have Editor( config ) and StandardEditor( element, config ), so we can use the first one as a base for that DOM-independent virtual editor.

So, either we'll keep this split and layer this code horizontally, or try a different approach.

Who said that we can have just one base Editor class? Well... perhaps I might've said that 3 years ago when working on this API for the first time. Anyway, we can have many of them. So we can have our Editor and VirtualEditor classes. They can implement some common interfaces and some special ones. And that's it. You want your own editor with two roots and some other custom behaviour – implement it (from scratch). Of course, you can use utils and mixins that we use but don't expect that there will be some base class which implements the most generic editor in the world (which doesn't exist anyway).

[pjasiun]

On the one hand, I agree, on the other, we need to have well-defined plugins API anyway, the set of properties every plugin can expect that every editor has. It might be better to have such API defined as base Editor class, not only as a documentation. If it will be a base class we will remember to update it. It might be also more readable this way. We will be also able to test if plugins really work with this base editor, if we do not miss something.

[Reinmar]

Base Editor class may simply be the BaseEditor interface. But then, we have more freedom of how this interface will be split. E.g. we can document (through an interface) which properties are required for the "editing" part of a feature and which by the "full" feature. The first interface will be then implemented by the VirtualTestEditor and the Editor class will simply implement two interfaces. Or something like that.

The problem with the base class is deciding what should be there and then handling situations when you want to have an editor which doesn't implement these properties (like VirtualTestEditor).

[Reinmar]

This was heavily reviewed and changed over the last months, so I think we can close this ticket.