Add more conversion helpers

[Reinmar]

This piece is tricky to remember:

https://github.com/ckeditor/ckeditor5-engine/blob/85dce7e914a16f065b1ac153821afc8205e4333d/src/conversion/buildviewconverter.js#L287-L298

The context on the fly modification was a total surprise, so let's create a helper.

The other thing which I noticed too late when working on ckeditor/ckeditor5-paragraph#12 is that I have to use the writer when merging paragraphs. And perhaps more.

[Reinmar]

Actually, ckeditor/ckeditor5-paragraph#12 is a very good test, because it's the 1st or 2nd time when we're implementing non-standard converters. Let's work on refactoring this code once it lands.

[Reinmar]

Also, perhaps this is a good question whether we shouldn't always clone the conversion data object before modifying it. Making it immutable would smell much better.

[Reinmar]

There's another case here: https://github.com/ckeditor/ckeditor5-image/pull/23/files. There's also a case for model-to-view converters. Either the existing ones aren't very useful or @szymonkups didn't know how to use the in a nice way. I don't know too, so it means that something's missing here.

In other words – the image style feature should be couple of lines long.

[szymonkups]

Cases from ImageStyle plugin:

View to model helpers

We can pass a ViewMatcher instance to the .from() method of buildViewConverter, it would be nice if we could also have parameterized consuming() method. Something like this:

const viewMatcher = new ViewMatcher( [
	{ name: 'figure', class: [ 'image', 'image-align-left' ] },
	{ name: 'figure', class: [ 'image', 'image-align-right' ] }
] );
		
buildViewConverter()
	.for( data.viewToMode )
	.from( viewMatcher )
	.consuming( match => /*create object with values to consume based on the match*/ );
	.toAttribute( viewElement => /*create key-value pair*/ );

Model to view helpers

• It would be nice to be able to have something similar to ViewMatcher in model, so we can for example convert attributes of particular element: only style attribute of image element.
• Helpers for converting to view style and class attributes. Now toAttribute will just replace all styles and classes inside view element. It could be even implemented as separate helpers: toStyle and toCssClass methods.

[Reinmar]

Another case: https://github.com/ckeditor/ckeditor5-image/pull/48/files#diff-181a641e45476f54a783a7ad41ed6f5bR62

[pjasiun]

Yea, when I was reading this code I understood why it needs to be improved. In fact, we could try to get rid off model.writer and let users modify model elements directly. It might make the internal code uglier, but it's not the internal code what people are using, but API. No element.appendChildren is pretty useless.

[pjasiun]

Also, I don't like the push/pop there. I agree that we need some abstraction on top of them.

[scofalik]

In fact, we could try to get rid off model.writer and let users modify model elements directly.

Woah... This escalated quickly...

You realize that what you complain about is a product of the big refactoring that took place in engine some time ago? There was a time when model.writer code/tasks were spread across model nodes.

This led to:

• complicated code inside nodes,
• circular dependencies,
• no parity between model and view,
• magic happening inside methods,
• and maybe more things that I don't remember now.

Then we introduced model.writer:

• so there is parity between model and view (no WTF -> why I can use model API directly, but view through writer?),
• to have a clean and direct API and then tools over it to automate and simplify working with the API.

I understand that this was some time ago and sometimes you have to review ideas from pasts, but that was a big and long refactoring and we wouldn't make that decision "because so". Of course the main goal of refactoring was removing CharacterProxy but I remember how happy you were that model.writer fixed so many problems and made code clean and magic-less.

BTW. AFAIR you proposed appendChildren to make model resemble DOM.
BTW2. People do not use model.Node API, people will use Batch API. Unless you are preparing something big, using model.writer should not be a big deal for you. And I expect this will be a very rare case. In some circumstance I feel that model.writer API may be even more convenient than nodes API.

[Reinmar]

When working on the message quote feature I had to convert:

<messageQuote>
  <paragraph>x</paragraph>
  <paragraph>y</paragraph>
</messageQuote>

to:

<figure class="quote">
  <blockquote>
    <p>x</p>
    <p>y</p>
  </blockquote>
</figure>

And it turned out that I can't do this using the converter builder.

I think that this may be a quite popular conversion pattern (e.g. the image feature does it similarly), so either we should add support for creating bigger structures to the converter builder or consider this case for the conversion utils.

[szymonkups]

I was working on this for couple of days, trying to find a solution for convenient conversion helpers. I really like conversion builder approach - it's easy to understand and "hides" the fact that when converting from model to view we convert changes:

buildModelConverter()
    .for( dispatcher )
    .fromElement( 'paragraph' )
    .toElement( 'p' );

At the time when builder was created it was impossible to predict what kind of utilities will be needed, and implement them inside the builder from the start. This way, some features implement own converters with tweaks matching their needs. It's not that bad, but many times it took some time to create and test them and it would be great if other features could easily re-use them. For example: image feature has caption converter, it would be great if for example table feature could use it.

Why didn't we extend the builder in the first place? I think because current builder approach with .from*() and to*() methods, couldn't be easily extended. It requires to add some more parameters to already existing methods or create new methods working well together with other ones, for example: you need to be sure that your new toSomething method will work without problems with all previous from* methods .

I would like to propose a change to a way how conversion builder is used. It should contain only the logic to apply "conversion utilities" to given dispatchers. All the conversion logic should be moved to those utilities. This way each utility will be separated from others, it could have it's own configuration and adding new utilities would be much easier. Consider this example:

buildModelConverter()
    .for( dispatcher )
    .use( elementToElement( 'paragraph', 'p' ) )
    .use( attributeToElement( 'bold', 'strong' ) );

We keep elementToElement utility in total separation from attributeToElement. This way each utility can cover it's own use cases and be extended separately.
Let's say we want to create a converter that converts capiton elements that are placed inside image. We can introduce filter option to elementToElement utility without touching other utilities:

buildModelConverter()
    .for( dispatcher )
    .use( elementToElement( 'caption', 'figcaption', {
        // Convert only captions of images.
        filter: element => isImage( element.parent )
} ) );

It would be still hard to predict all use cases, but I think this approach will encourage us to look for more general approach to conversion problems and improve our utilities instead of directly writing converters that solves only per-feature problems.

I've created POC for image feature: https://github.com/ckeditor/ckeditor5-image/compare/t/ckeditor5-engine/736. It introduces the new approach and couple of utilities:

• model element to view element conversion,
• model attribute to view CSS class conversion,
• model attribute to view child element attribute.

It allowed to replace almost all model to view custom converters (excluding some fixers) without tests changes.

WDYT?

[scofalik]

If we go with such sophisticated helper functions, I don't know there's much reason in keeping ModelConverterBuilder and ViewConverterBuilder classes. So I am either for dropping those or for not moving so much logic to the util functions.

[szymonkups]

If we go with such sophisticated helper functions, I don't know there's much reason in keeping ModelConverterBuilder and ViewConverterBuilder classes. So I am either for dropping those or for not moving so much logic to the util functions.

Yeah, it looks like in this approach builder is just a little helper to passing dispatchers to utils functions. It can be easily skipped.

[Reinmar]

I can see that we all, independently come to the same conclusions that we need helpers instead of one converter builder. That's good :)

[scofalik]

It wasn't really independent :P.

[pjasiun]

I think that the mistake we (I) did, was that we focused so much on making the conversion with builder simple that we forget that conversion without the builder, for more tricky cases, need to be rather simple too. The builder is very nice, recently, @jodator notice that he really like using it because he doesn't need to get deeper and learn what does it do. It just works. Now, focusing on advance converters, we should keep in mind that is should be still very simple and intuitive to create a simple converter.

Ok, enough introduction. ;)

We should improve listenTo method, so you can add a callback to multiple objects or multiple events. This is something we miss pretty often. Now when you want to add a listener to multiple events you need to save it in the variable:

const converter = () => { /*...*/ };

this.listenTo( data.modelToView, 'addAttribute', converter );
this.listenTo( data.modelToView, 'removeAttribute', converter );

Now it should be possible to do:

this.listenTo( data.modelToView, [ 'addAttribute', 'removeAttribute' ], () => { /*...*/ } );

However, converter added to multiple objects and events, even if short, is still not readable:

this.listenTo( [ data.modelToView, editing.modelToView ], [ 'addAttribute', 'removeAttribute' ], () => { /*...*/ } );

At this point, I like what @szymonkups proposed very much. I think it's how the API could look, on the top of improved event listeners. Good job.

In fact, we could go even further, to have just utils:

convertElementToElement( {
	for: [ data.modelToView, editing.modelToView ],
	from: 'paragraph',
	to: 'p'
} );

Or even, assuming that usually, one wants to have converters to both pipelines, both ways:

convertElementToElement( editor, { viewElement: 'paragraph', modelElement: 'p' } );

But it might be too far, too magic.

---

At the same time notice that there is still a big room for improvements in how to write such conversion utils.

One thing we should improve is the fact that events names and consumables are not compatible. This is why we need code like this: https://github.com/ckeditor/ckeditor5-image/compare/t/ckeditor5-engine/736#diff-d90b5bb92fa682c8d8e05fc292e94643R19 or use helper no one remembers about: https://github.com/ckeditor/ckeditor5-engine/blob/369d636488bc75d9832c27ff28b9eaf38d8a5735/src/conversion/model-to-view-converters.js#L564-L568 It should be possible to pass event name to the consume method and this method should know what part of it should be consumed. At the end of the day, we don't use 'consume' function anywhere else and there is no good reason to keep in incompatible with its only usage.

I think that we could also change attribute events names:
addAttribute -> attribute:add
removeAttribute -> attribute:remove
changeAttribute -> attribute:change
So it will be possible to have just listened to the attribute event. Or we could just use changeAttribute always (with null as an old or new value in case of add or remove). It might be even simpler and semantically better.

Also, note that we already think about the view to model conversion simplification together with "one API" change (https://github.com/ckeditor/ckeditor5-engine/issues/858). This topic is very related because it's view to model conversion where, at this point, we use API very different from the regular model API, so we need to change view converters together with this unification. I will create a separate ticket for this change soon.

[jodator]

But it might be too far, too magic.

A bit too magical for me. Also with convertElementToElement() is a bit too specific as you would also need:

• convertAttributeToElement()
• convertElementToAttribute()
• ... others?

Current fluent API for converters reads nice and is very intuitive also with helpers like to/fromClass() or to/fromStyle() which gives you already what you need without breaking down classes or reading styles.

[pjasiun]

Fixed by ckeditor/ckeditor5-engine#1274.