Ckeditor5: API simplification – consider merging engine controllers and removing editor.document

Created on 10 Nov 2016  Â·  4Comments  Â·  Source: ckeditor/ckeditor5

Current state

There are two engine controllers – EditingController and DataController. The first introduces the pipeline which renders to the editable and the second the pipeline which reads and writes from/to "data" (editor output). The controller instances are available in editor.editing and editor.data. The DataController has set() and get() methods which make for editor.data.set() and editor.data.get().

There's one "model" available in editor.document and also in editor.editing.model and editor.data.model.

The reasoning behind this was that there's one model with two views (editing and data) handled by two controllers. Those controllers create two pipelines and features use them to register proper converters. This means that one model element may be rendered differently in the editable and in the data (output).

Potential issues

  1. The document is available in 3 places – editor.document, editor.editing.model and editor.data.model. This is confusing as a feature may use any of them. Also the model ?= document problem exists.
  2. The idea was that one can add more controllers, but it's purely theoretical. Features expect to see those two pipelines that we have now and if you add one more you need to add converters for all features to that new one. It's also impossible to create editor without the editing pipeline with the existing features because they will break.
  3. The EditingController has no methods. The only thing which it makes is creating the pipeline. You only operate on the model through the DataController or the on the model itself.

Possible improvement

I was thinking about merging the two controllers into a single DataController. After all, there's one "data" (model) which has multiple views into it (editing, data). If we'd also remove editor.document we'd have only editor.data.model with editor.data.set/get().

However, the pretty obvious downside is that now the two pipelines will land in one class and we'll have a name clash there. E.g. both controllers expose now modelToView dispatcher. We'd need to have:

editor.data.modelToEditingView
editor.data.modelToDataView
editor.data.dataViewToModel

Also, it becomes even clearer now that we have a clash between "data controller" and "data" (as "output"). It's pretty ugly, so we'd need to find a name which doesn't conflict with "editing" (cause there's the editing view) and "data" (cause there are editor output and data processors).

The easy choice would be engine. Cause we have editor.ui which is an instance of UIController, so why not having editor.engine being an instance of EngineController.

However, this gives us editor.engine.set() so it'd need to become editor.engine.setData() (not that bad). I'm also a bit unsure about editor.engine.model.

RFC

I know that the proposal isn't perfect. However, I feel that the current API isn't perfect as well, so we need to think on this still.

engine invalid discussion

Most helpful comment

We've had a long discussion with @pjasiun about how to structure this API because, as usual, it turned out that it's just a topic starter.

We started from analysing how many levels of methods working on the model we have.

  1. High level, extensible: insertContent(), deleteContent(), getSelectedContent(), set|removeTextAttribute(), modifySelection().
  2. Batch level: batch.insert(), batch.remove(), etc.
  3. Deltas (dir: model/delta/).
  4. Operations (dir: model/operation/).
  5. Writer (dir: model/writer/). Writer is used by operations, but it also should be used when doing changes on the model directly (e.g. upon conversion). So it's a bit aside from the previous four layers.

Most of the developers will need only the 1st level and sometimes the 2nd. We should, however, try to make the 1st level methods as wide-range applicable as possible. From my experience, insertion, deletion and setting attributes is what you do 90% of the time. The 5th level is a special case, and will need to be used only for special converters.

The second thing that we understood is that we don't have to make the batch API that nice. It's not a bit overcomplicated for no special reason. You need to be able to create batches easily, of course, because they are needed to work with the 1st level methods, but you don't need to be able to create deltas and apply them so easily. https://github.com/ckeditor/ckeditor5-engine/issues/679 investigates this further.

Now, with the layers defined we were able to define what needs to go where and what a controller is.

So, first of all, there will be a DocumentController available in editor.document. It has a model and 2 views so:

editor.document.model // {engine.model.Document}
editor.document.editingView // {engine.view.Document}
editor.document.dataView // {engine.view.Document}

The 1st level methods will be available directly on the controller so they will be easiest to find. Then 2nd level will less visible, but still it will be visible that you apply deltas to the model. The operations and writer will be the most hidden layers.

All 4 comments

We've had a long discussion with @pjasiun about how to structure this API because, as usual, it turned out that it's just a topic starter.

We started from analysing how many levels of methods working on the model we have.

  1. High level, extensible: insertContent(), deleteContent(), getSelectedContent(), set|removeTextAttribute(), modifySelection().
  2. Batch level: batch.insert(), batch.remove(), etc.
  3. Deltas (dir: model/delta/).
  4. Operations (dir: model/operation/).
  5. Writer (dir: model/writer/). Writer is used by operations, but it also should be used when doing changes on the model directly (e.g. upon conversion). So it's a bit aside from the previous four layers.

Most of the developers will need only the 1st level and sometimes the 2nd. We should, however, try to make the 1st level methods as wide-range applicable as possible. From my experience, insertion, deletion and setting attributes is what you do 90% of the time. The 5th level is a special case, and will need to be used only for special converters.

The second thing that we understood is that we don't have to make the batch API that nice. It's not a bit overcomplicated for no special reason. You need to be able to create batches easily, of course, because they are needed to work with the 1st level methods, but you don't need to be able to create deltas and apply them so easily. https://github.com/ckeditor/ckeditor5-engine/issues/679 investigates this further.

Now, with the layers defined we were able to define what needs to go where and what a controller is.

So, first of all, there will be a DocumentController available in editor.document. It has a model and 2 views so:

editor.document.model // {engine.model.Document}
editor.document.editingView // {engine.view.Document}
editor.document.dataView // {engine.view.Document}

The 1st level methods will be available directly on the controller so they will be easiest to find. Then 2nd level will less visible, but still it will be visible that you apply deltas to the model. The operations and writer will be the most hidden layers.

After another long talk we agreed that this change will not be good. People are scared touching nested properties, and we would have editor.engine.model.document.selection to simply get selection. We need to try to flatten this, not to make it deeper. Also we did not find anything better than engine and it sounds like a property which end-user prefer not touch, so putting there very basic API, whould be bad. At the same time, we do not have any real problem with the current structure with 2 controllers. They reflect very well editors achitecture with 2 pipelines, do not need mutual references and are initialise only in a single place (editor class).

Was this page helpful?
0 / 5 - 0 ratings

Related issues

wwalc picture wwalc  Â·  3Comments

devaptas picture devaptas  Â·  3Comments

msamsel picture msamsel  Â·  3Comments

benjismith picture benjismith  Â·  3Comments

pandora-iuz picture pandora-iuz  Â·  3Comments