Inconsistent "upcast writing"

[Reinmar]

We've just introduced UpcastWriter and when I've stumbled upon this piece of code (in the documentation) I thought that something's wrong:

editor.plugins.get( 'Clipboard' ).on( 'inputTransformation', ( evt, data ) => {
	if ( data.content.childCount == 1 && isUrlText( data.content.getChild( 0 ) ) ) {
		const linkUrl = data.content.getChild( 0 ).data;

		data.content = new ViewDocumentFragment( [
			ViewElement(
				'a',
				{ href: linkUrl },
				[ new ViewText( linkUrl ) ]
			)
		] );
	}
} );

Why aren't we using UpcastWriter here? If, instead of creating a fresh new document fragment, we'd like to modify the one in data.content, we'd need to use the upcast writer. So I actually thought that this piece of code is wrong... and it is – constructors of these node classes are protected so we can't use them like this. But they are not exposed in UpcastWriter.

Also, View#change() doesn't return the return value of its callback just like Model#change() does which would be highly inconvenient.

[Reinmar]

cc @pjasiun @f1ames

[f1ames]

Why aren't we using UpcastWriter here?

Probably because UpcastWriter was introduced after the above code was created and no one took time to refactor it yet.

constructors of these node classes are protected so we can't use them like this. But they are not exposed in UpcastWriter.

That's the second issue that UpcastWriter don't expose this, so probably it should be added if needed.

---

We already have one task to utilize UpcastWriter in DOMConverter (https://github.com/ckeditor/ckeditor5-engine/issues/1520), but yes, there are more places in the code where it can be used.

[pjasiun]

Agree that upcast writer should be used there. We can even create an instance in the clipboard plugin and put next to content in the event, so one should import nothing and should not need to think which writer should be used.

Also, View#change() doesn't return the return value of its callback just like Model#change() does which would be highly inconvenient.

This is a good idea too. However, we need to ensure that it is possible. It might be impossible if the view#change block would be executed asynchronously in some cases. But AFAIR all change blocks are now executed synchronously, so it should be possible.

[Reinmar]

This is a good idea too. However, we need to ensure that it is possible. It might be impossible if the view#change block would be executed asynchronously in some cases. But AFAIR all change blocks are now executed synchronously, so it should be possible.

AFAICS there should be no problem. Cool.

Which create*() methods are we going to need? Text, element (no subclasses), docfrag? Something more?

[pjasiun]

Yep, we should introduce createDocumentFragment, createElement and createText and is should be enough.

BTW, there is no createDocumentFragment in the DowncastWriter, but I think we should not introduce it as long as we do not have a case for it. I am not sure if DocumentFragment makes much sense in downcast.

[f1ames]

we should introduce createDocumentFragment, createElement and createText

So this issue is for introducing those 3 above methods to UpcastWriter? (and eventually correcting the clipboard docs).

What about:

View#change() doesn't return the return value of its callback just like Model#change() does...

Do we know that this change is needed? Should it be also covered here or in a separate issue?

[Reinmar]

Without returning from change() creating nodes is just sadly complicated. You create them inside the callback and need to assign them to a variable defined outside. So to completely clean the code I posted above we need that. I’d do both things at once.

…

Sent from my iPhone

On 17 Sep 2018, at 15:57, Krzysztof Krztoń ***@***.***> wrote: we should introduce createDocumentFragment, createElement and createText So this issue is for introducing those 3 above methods to UpcastWriter? (and eventually correcting the clipboard docs). What about: View#change() doesn't return the return value of its callback just like Model#change() does... Do we know that this change is needed? Should it be also covered here or in a separate issue? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.