Conversion refactor

[scofalik]

I am opening this ticket to have a place for discussion about upcoming conversion refactor.

Goals

There are a few goals that we want to achieve with this.

The first goal is to stop converting an incorrect model state to the view. This happens now because we fire model-to-view conversion after each atomic change on the model.

There is also a problem with mapping during conversion. Sometimes we need to map non-existing model position/range to the view (a case for remove). Sometimes we want to map an element from the model to the view, but the element was already mapped to another element (a case for rename and move AFAIR). It would be perfect if those problems would vanish away.

The second goal is to make it easier to write converters. At the moment, converters are the most complicated part of developing a feature. For a new developer, it may be not clear what exactly they have to do to write a correct converter.

This is also connected with providing more / different / better / easier to use conversion helpers. Now we have more knowledge about how the converters actually look, so we know what kind of helpers we can introduce.

Additionally, markers conversion isn't pretty right now. It'd be nice to revisit it.

The bonus goal is to prepare an environment where we can easily hook callbacks / fixers that should react to changes in the model.

Keep in mind, that developers have their own experiences and are accustomed to some patterns. If the solution is easier for the user, it is probably better.

Think if there is anything that can be improved in view-to-model conversion. Since this topic all began from talking about "converting model changes to view on changesDone" it was natural, that we mostly focused on model-to-view conversion. In my opinion, view-to-model conversion is fine, but I might be missing something.

Related tickets

https://github.com/ckeditor/ckeditor5-engine/issues/829 - Simplify model to view conversion
https://github.com/ckeditor/ckeditor5-engine/issues/736 - Add more conversion helpers
https://github.com/ckeditor/ckeditor5-engine/issues/778 - Change-based conversion can be really confusing
https://github.com/ckeditor/ckeditor5-engine/issues/779 - It shouldn't be needed to listen on 3 events to convert an attribute
https://github.com/ckeditor/ckeditor5-engine/issues/945 - Consider simplifying attribute conversion

Thoughts and ideas

Thinking out of the box

I'd like to make it clear, that we should try to forget about how conversion looks now. Of course, we have experience, we know what worked and what did not work. However, maybe we could come up with a totally different idea for conversion mechanism.

The only assumptions to make are:

1. There is a model element which is composed of name and attributes. We need to convert that model element to a view element.
2. The model element might change. We need to reflect that change in the view.
3. The model might be in incorrect state after some changes are applied. We need a good solution how to fix the model if that happens.

Limit amount of change types

This already has been tweaked a bit earlier. Anyway, This is a minimal set of changes that we need to support:

• insert node,
• remove node,
• attribute changes.

Insert and remove changes handle, of course, removing and inserting elements, but also moving or renaming them.

Attribute changes are a bit more complicated. I am not sure what we need here. Unfortunately "add attribute" + "remove attribute" does not seem to be the correct way to change an attribute. Mostly because some elements require some attributes so removing attribute is forbidden. Take listItem for example. Changing it's type attribute should be converted "in one step", especially because it doesn't make sense to have a listItem without type attribute.

Maybe the solution is to have just one "attribute change" change type, and the converter should handle it all: inserting, removing and changing the attribute. The question is what we gain here - other than having to attach only one callback (instead of three, like now).

Another idea is to remove the element which attributes got changed and then insert it with the changed attribute. I am not sure how this will impact view tree and then DOM rendering. I am not sure how inefficient such solution would be. Thankfully, the biggest efficiency bottleneck is typing and attribute conversion is not connected with it.

Conversion on changesDone

This is the original idea from which it all started. It is simple - do not convert model changes right away. Instead, buffer them. When all changes are applied, all callbacks and fixers did their changes, convert the model to the view. This should happen on changesDone event. At this moment, all fixers should already have fixed the model.

What do we gain from this solution? Mostly, the converters will be easier to write because they won't need to convert incorrect model states.

How to enable conversion on changesDone? There are a couple of ideas here:

1. Mark changed elements. Have a data structure that will keep changes done to the model: which elements were removed, which were added and which attributes changed. This is a bit difficult for Text instances. Text changes might need to be kept as LivePositions, especially when whole text node was removed. On changesDone iterate through this data structure and apply changes to the view one by one. This is the simplest and original proposal. I'd treat this as a "fallback solution". It does not change a lot in the conversion itself, it just postpones the whole conversion to a later stage. It solves some of our problems. If we won't come up with something better, this should be okay.
2. Copy model or parts of the model to diff them later. Instead of tracking down each change, we could copy the whole model when first change is applied and then diff old (saved) model with the new one. Unfortunately, I fear that this would be too inefficient for typing.
3. Save which model elements have changed. Then convert them to the view and diff them with the actual view. This idea is similar to what we have right now in view->DOM rendering. We only change what really changed. I am not sure, but AFAICS this solution could also resolve some problems with mapping.

Do not forget about markers! Markers are also a part of the model and need to be converted on changesDone. We would have to store which markers have changed, and how. The exact implementation will probably depend on the chosen solution for model tree conversion.

Those ideas do not touch change callbacks, though. Those callbacks / fixers, similarly to converters, are now a bit too complicated. At the end of this post I've included a list of all callbacks listening to change event.

Decouple building and inserting elements

We came from an idea that we are converting changes, not model state. This is why we'd rather say "convert image insertion" rather than "convert image element". On the other hand, it is much easier to think that "listItem model element should be represented as <li></li> in view". People are familiar with this kind of thinking. We should take it into consideration.

Hence, I propose to think about decouple inserting elements and building elements. Inserting is same anyway for most features. For now, only lists have their own needs for inserting -- when you insert an element between listItems, in view you need to break <ul>/<ol> element. Similarly, when you remove an element, it may happen that two lists need to be merged.

The reason for this proposal is that maybe if we think about "building" elements, it will be easier to refactor conversion mechanisms and propose new conversion utilities.

This idea is strongly connected with rebuilding items a lot of time (for example, rebuild the whole item when its attribute changed). We need to think how elements change and what impact does rebuilding have on creating a view structure and then DOM.

Firing event before the change is applied to the model

This is a wild idea and I don't know what it would achieve expect of giving us more possibilities but I just wanted to throw it here and see if maybe you will find it useful.

The idea is simple: instead of applying a change to the model right away, fire an event. Listeners would be able to modify or cancel the change.

Do we need consumables?

This was brought up during talks and is not originally my idea. The question was whether we need consumables and how do we use them at the moment. The doubt was that consumables have the same role as event canceling. The bottom line was that we maybe need them in view-to-model conversion but probably are able to remove them from model-to-view conversion.

Of course, if we will go with "building" items, the model-to-view conversion will get a bit similar to the view-to-model conversion. Then we might need consumables to build elements from the model in a right way.

Use cases for change event other than ModelConversionDispatcher

CKE5 uses change event for following callbacks / fixing:

• autoformatting (typing of certain characters are detected on change event),
• image caption (adding image element is detected and if the image does not have caption element, the element is added),
• lists fixer (a lot of incorrect scenarios related with type and indent attributes are detected and fixed),
• auto paragraphing (when an empty root is detected, paragraph element is inserted),
• change buffer is updated after each change event (typing),
• undo (undo stack is refreshed after each change on the model),
• image upload.

Additionally, these are Letters use cases:

• document structure is fixed to always have title and body,
• comments management heavily depends on detecting marker changes,
• saving changes to send to other clients,
• comments position in the sidebar is refreshed after each change.

I think that in overall, there are two types of callbacks:

• ones that care about model state or how it was changed and then fix the model or refresh some other states,
• others that have to react to every change, mostly to track it.

What helpers do we need?

This is to bootstrap a work that has to be done with conversion helpers. There was an idea to drop converter builders and instead provide a set of utility functions that will be simpler to use but will produce more focused converters.

We need to study how do we use converter builders. Which cases need to be ported to utility functions. Then, we need to see what was impossible to do using builders.

A short list of helpers that we need. Was not researched much yet:

• model element <-> view element (by element name),
• model attribute <-> view element (by names),
• model attribute <-> view attribute (by names),
• markers conversion,
• collapsed selection conversion (it breaks attributes and inserts empty attribute elements),
• new view style converter (make it easy to add and remove a part of style attribute),
• new view class converter (make it easy to add and remove a part of class attribute),
• new wrapper converters (case where generated view structure is wrapped in an element, for example <figure> element in image feature)

Tasks

A list of tasks that we need to do no matter how we proceed with this refactoring.

1. Clean view.writer. For example, it has rename method and I am not sure if it is used at all.
2. Check how UIElements are created and added to the view tree. Try to come up with an API to manage UIElements better.

[Reinmar]

Some notes from today's meeting:

• Can we remove consumables from m->v converters? Can they be replaced by simple event cancelling (in at least 99% of cases)? Or could we perhaps figure out some other solution?
• Can we remove conversion builder? Can it be replaced with more conversion helpers?
  • Thanks to that we'll use plugin.listenTo( ... ) to plug converters which will ensure proper cleanup on plugin destruction. However, we don't support destroying plugins (without destroying the whole editor) so it doesn't fix any issue... :D
  • Packages like ckeditor5-basic-styles will be able to expose their own, more contextual conversion utils. It's easier to write many contextual utils than one big generic conversion builder.
• Can we somehow use evt.return? It makes fire() return a value.

[scofalik]

Idea: currently ModelConversionDispatcher controls dispatching conversion events both for the converted node and its attributes and children. The idea is to move this responsibility to the converters. If a converter would convert attributes and/or children in a custom way, it would not fire the events. In another case, there could be just a helper function that would handle firing proper events.

[scofalik]

I was thinking about this concept of conversion on changesDone, after all the fixers did their changes. I am not sure how much we gain if we just postpone the changes, but then convert them one-by-one anyway. This is especially true for attributes conversion. To be honest, it may be even worse.

Consider this example:

<listItem indent=0>A</listItem>
<listItem indent=1>B</listItem>
<listItem indent=1>C</listItem>
<listItem indent=2>D</listItem>

If you indent item C, you change its indent to 2, and the next items indent to 3. If you will process then those changes one-by-one, you might get confusing state in view, because in view item D will look like its indent is 2 but in model it will be 3. So we won't have incorrect states in model, but we will have view that does not correspond to the model. Lists converters are complicated in a way that list items need to be joined with elements before and after them when they change or are inserted or when something between them is removed. Which makes it all tricky.

This is why I proposed remove+insert approach. But I have to think whether it will resolve our problems.

[scofalik]

BTW. I know that you (@pjasiun) already wrote about the problem with mismatched model and view, but that was different case. Inserting and removing is doable, as long as you don't have to do a lot of magic connected with inserting itself. Lists converters do a lot of magic, unfortunately, that's why it got complicated.

[scofalik]

A quick summary of pros and cons of different approaches:

---

Postponed changes

Conception:

• mark elements that got removed, inserted or had attribute change
• on changesDone apply changes from left to right (not sure about that, possibly removes would have to be before inserts etc.)

Pros:

• probably easiest to implement and we can predict the outcome and possible issues
• does not mess up with UIElements because it is not rebuilding part of view tree

Cons:

• during conversion, the model is much different than the view
• will not simplify converters much
• does not seem like a big improvement, won't solve all the problems (like problems with mapping)

---

Remove + insert

Conception:

• mark elements that got removed, inserted or had attribute change
• remove all removed and changed elements, then insert all inserted and changed elements
• attribute change is treated as remove + insert

Pros:

• theoretically converters might get simpler if view is "cleaned" of removed and changed elements, but it is hard to say how much simpler they would be
• still a manageable scope of changes

Cons:

• during conversion, the model is much different than the view
• UIElements will have to be somehow refreshed if they are in the changed element
• possibly still some problems with mapping

---

Generate new view and diff

Conception:

• similar to view-to-DOM rendering
• mark elements which children changed
• convert children of those elements from the model to the view
• diff converted children with actual view and apply changes

Pros:

• this way we really get all the benefits of postponing conversion to changesDone
• possibly, all the known problems are solved

Cons:

• big change, also conceptually, hard to tell all the outcomes of the change
• possibly negative impact on the view and DOM rendering, we would have to take care that no extra elements are created and the bound ones are used
• UIElements in changed range would have to be refreshed

[pjasiun]

First of all, I don't think we should go with the totally new concept, because of 2 reasons.

First, the current concept works pretty well for many of cases, we don't think at this point because they just work. The most complex thing is to design conversion pluggable. It needs to be possible to convert element in one plugin and its attribute in another or convert the same attribute or element by various plugins, depending on the context. It works well with the current approach. We also handle selection attributes conversion and changes done directly in the view, without conversion, which should not be removed. Also, note that the current concept is very elastic and let you convert changes in any way, not necessarily element->element. If, for instance, we will have to transform paragraphs to <br>s one day we may need something like this. I'm pretty sure that, even it a new approach will fix problems we have, a lot of new problems will appear.

Second, it would be far more time-consuming than it seems to be. Note that there are whole mechanisms, like view writers or selection with attributes designed to work with the current approach. If we will bring a totally new concept, we would have to rewrite more and more.

This is why I think we should focus on fixing what is the problem. And I see a few problems here.

First is the fact that we execute converters before all changes in the model are applied and before post-fixer are called. I agree that it might be tricky, but I believe that we should try to fix it, we may get a better result.

Second is that writing converters without the builder is far more complicated than it should be. We see here a big space for improvements.

Third, is that we (I) focused on making converter pluggable so much, that forget about cases when the whole big part of the document should be converted together (for instance image with attributes and children).

Another problem is the fact, that it's very hard to debug converters. We may rethink this too bringing a betters structure or some tools.

I think that what we do: pluggable conversion is a very complicated task. I don't think we will bring a perfect solution. We should do our best to fix problems we know about.

[pjasiun]

This already has been tweaked a bit earlier. Anyway, This is a minimal set of changes that we need to support:

insert node,
remove node,
attribute changes.

What about selection attributes and markers?

Thankfully, the biggest efficiency bottleneck is typing and attribute conversion is not connected with it.

Unfortunately, it is. Typing in the bolded text is the case.

Consider this example:

<listItem indent=0>A</listItem>
<listItem indent=1>B</listItem>
<listItem indent=1>C</listItem>
<listItem indent=2>D</listItem>

If you indent item C, you change its indent to 2, and the next items indent to 3. If you will process then those changes one-by-one, you might get confusing state in view, because in view item D will look like its indent is 2 but in model it will be 3. So we won't have incorrect states in model, but we will have view that does not correspond to the model. Lists converters are complicated in a way that list items need to be joined with elements before and after them when they change or are inserted or when something between them is removed. Which makes it all tricky.

Not really. If the model will be changed when the conversion starts we will be able to see that it's list indentation what changed and fix it for the whole list at once.

Do we need consumables?

I know that I'm the one who suggested it, but more and more I think that some form of consumables is needed. You need to be able to mark that you converted more than a single change, for instance, you converted also attributes, children or the whole list. However, we may rethink then it makes them simpler to use. Maybe, it's enough to cancel all event.

[scofalik]

The most complex thing is to design conversion pluggable.

I want to make it clear that I think that neither of suggested ideas stands against this concept. If some feature inserts, for example, and indented list item, or a blockquote with multiple paragraphs, etc., we are able to convert it now. So whatever we are able to do now, we will be if we decide for any of those approaches.

We also handle selection attributes conversion and changes done directly in the view, without conversion, which should not be removed.

I don't understand. I guess you mean that rebuilding destroys all the changes that we do outside of conversion? Yup, well, this might be a problem. I'll return to this later at the end of post.

Also, note that the current concept is very elastic and let you convert changes in any way, not necessarily element->element.

AFAICS nothing changes. At least this is my hope.

Note that there are whole mechanisms, like view writers or selection with attributes designed to work with the current approach.

This is true. I just think that since we already have to support just inserting complex structures, we can do it for any conversion.

This is why I think we should focus on fixing what is the problem.

This is the point of this discussion and my more-than-enough opening post. There are two problems: problems are vaguely defined and easy solutions don't fix them.

What about selection attributes and markers?

AFAIR, they happen at different times (not in ModelConversionDispatcher#convertChange). So this is a different story.

Unfortunately, it is. Typing in the bolded text is the case.

Typing in the bolded text is, AFAIR, inserting text already with the attribute? Anyway, this doubt concerned when an attribute is changed on an element so that the whole element is re-converted. AFAICS, your case is unrelated.

Not really. If the model will be changed when the conversion starts we will be able to see that it's list indentation what changed and fix it for the whole list at once.

I am not sure you understood this correctly. Anyway, I will just reply to the last part: "fix it for the whole list at once". See, this is the problem. We cannot "fix something for the whole list at once". Even if we postpone changes to changesDone event, we still will have to process them one-by-one. It's true, that the model will be correct. But it won't be in sync with the view, which will make it confusing when coding converters (to say it simply, you will be able only to "look left" in the model because only "left side" of the model will be already converted to the view, in fact, it would best if you'd "look left" only also in the view, which is not possible for lists converters which have to merge lists with previous and next siblings).

All in all, the bottom line is: I have doubts whether we should change anything if we want to just postpone changes to the later. We won't achieve much. The whole point of this refactor was to make writing converters simpler, I am not sure we are achieving this with easy solutions.

[scofalik]

Sorry, I was supposed to return to something at the end of the previous post.

So, there is a problem with changes done directly on the view outside of conversion. It's true. If we rebuild part of a tree, we might lose those changes. First, however, we should answer two questions:

1. Should we work directly on the view tree outside of conversion?
2. In what cases exactly will we lose some view content?

My answers:

1. I am not sure. It would be best if all is done in conversion, and I believe that in most cases we are able to do things through conversion. The same way we don't mess with DOM outside of view rendering (we don't, do we?).
2. The view element will be lost only when:
   a. it was inserted outside of conversion,
   b. it was inserted between Text nodes or AttributeElement nodes,
   c. it is in changed range.

Do we have any cases for such elements right now, be it in CKE5 or Letters?

EDIT: Actually, answer for point 2 is not entirely correct. In fact, the UIElement will be lost each time a new view element is created during conversion. Now it depends on implementation but UIElements might be also lost during move or rename operations conversions (if they are remove+insert and bindings are not kept).

[scofalik]

Okay, so I've been thinking more about this UIElements because there is a function view.writer#rename. I stumbled upon it when reading the code and was confused, cause I was sure that the rename conversion at the moment is done through remove + insert. And it is. So now I wonder, wouldn't you, in some scenarios, lose your UIElements already? For example, move is also already converted as remove + insert. I'll have to check it to be sure, but I think you'll be losing UIElements already. If that is the case we have "a problem" already. That's why I think that UIElements should be also inserted upon conversion. Or we need some way of adding them so they are somehow registered and refreshed, IDK.

[scofalik]

Actually, answer for point 2 is not entirely correct. In fact, the UIElement will be lost each time a new view element is created during conversion. Now it depends on implementation but UIElements might be also lost during move or rename operations conversions (if they are remove+insert and bindings are not kept).

So I checked it and I was right. I've added List feature in UIElement manual test in engine http://localhost:8125/ckeditor5-engine/tests/view/manual/uielement.html. I renamed paragraph with UIElements added by hand and they were lost. Also, this example shows how to properly add UIElements -- those added through callbacks are correctly managed.

[scofalik]

We discussed with @pjasiun that diffing will be difficult to do efficiently. Consider this example:

<root>
  <paragraph> ... </paragraph>
  <listItem indent=0> ... </listItem>
  <paragrah> ... </paragraph>
  <listItem indent=0> ... </listItem>
</root>

Now, the second paragraph is removed. To make the conversion and diffing more efficient, we would like to leave unchanged elements and just use bound view elements. This would be important not only for fast conversion but also for fast diffing. We'd like to diff elements by reference. But it is not possible because:

1. listItems are bound to <li> elements.
2. Lists in view have to be merged, so we can't leave them as they are.

This means that we would have to reconvert whole parent (root in the worst case) and then diff deeply and by properties (not reference).

I am not saying that this kind of solution would have to be slow. Especially typing - usually typing occurs in leaves of the model tree.

[pjasiun]

We talked and (more-less) agreed that @scofalik proposal was focused on lists conversion.

When we were thinking about bolding text in the paragraph what we have now is:

• wrap text.

What we would have after the change is:

• remove all paragraph's content,
• rebuild all text nodes in the paragraph,
• rebuild all links, other inline elements,
• rebuild all styles, wrap texts,
• insert new content.

It would not, for sure, make debugging simpler and this is why I'm against such change.

Also, we realized that this concept will not work well for list merging. We would have to rebuild the whole root and do the deep diff, what would be terribly complicated.

[scofalik]

There was a proposal that we might enable a way to have it easier to deal with complex conversions like list conversions. When list's structure changes we are fine with rebuilding big chunks of it. However now, we have to do it step by step, and conversion on changes done will not be much help. It might be nice to have a mechanism to handle multiple changes at once.

[scofalik]

Okay, I came to the conclusion that I am fine with refactoring this in a way that changes are buffered until changesDone event.

Unfortunately, we will have a limitation, that we should only look at previous elements in the model. To be safe, we should assume the same for the view. We can do something with lists later if we want (add some post-fixers that would trigger list rebuilding or something. IDK. But this can be a step 2).

[scofalik]

https://stackoverflow.com/questions/46970216/can-not-input-to-editableelement-of-ckeditor5

Some feedback on conversion. Or rather a case that a developer has, how they approached it and what are their expectations. I've added a very long answer. It was mostly an exercise for me too. This case might be valuable for us when thinking about more complex CKE uses and what conversion utilities we want to introduce.

BTW. I think that "widgetizing" part of the view is a difficult concept at the moment. It seems like magic and I am not sure what to really expect. Maybe a guide could change it.

[jodator]

As @scofalik suggest: "don't read whole just add your case".

Case 1: Conditionally output buildModelConverter().toAttribute(). It might be invalid as proper way to do thing which I describe below was to not add an attribute to model in the first place.

I wanted to conditionally convert attribute. Lets say that alignment attribute has one value that is "defaut" and should not be rendered thus returning 'undefined' - which does not work. But I'm not sure if this is a valid case as the default value should not be put to the model in the first place.

Case 2: toAtrribute() as @scofalik told me is not working the way I think it works.
I was suprised to hear that toAttribute will overwrite whole style.

Maybe adding toStyle( name, value ) that would merge style values defined by other converters?
So given converters:

buildModelConverter()
    .fromAttribute( 'foo' )
    .toStyle( 'font-color', 'blue' );

buildModelConverter()
    .fromAttribute( 'bar' )
    .toStyle( 'background-color', 'white' );

should output for example div block:

<div style="background-color: white;font-color: blue;"></div>