Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rethink consumables #4226

Closed
pjasiun opened this issue Dec 21, 2017 · 1 comment · Fixed by ckeditor/ckeditor5-engine#1241
Closed

Rethink consumables #4226

pjasiun opened this issue Dec 21, 2017 · 1 comment · Fixed by ckeditor/ckeditor5-engine#1241
Assignees
Labels
package:engine type:improvement This issue reports a possible enhancement of an existing feature.
Milestone

Comments

@pjasiun
Copy link

pjasiun commented Dec 21, 2017

Consumables in conversion are useful in complex cases, but very annoying when you are writing a simple converter. It's not a natural part of converters, and it's not clear if one need to consume or prevent an event.

In fact, it makes sense to consume consumables only if you are consuming future changes (at least in the model to view conversion). The current event could be automatically consumed after conversion is done. Also, the dispatcher could not fire event for consumed elements, so it would not be needed to check if an element is not already consumed. This way we could get rid of consuming in simple converters.

Also, after introducing differ, consumables are very similar to differ results (both are a list of changes to be handled). We should consider unification.

I focused here mostly on the model to view consumables. View to model consumables possibly could be simplified too, but this is a little more tricky because the view is messier, there are more conflicts and overwritten converters (i.e. auto paragraphing). View to model conversion needs to handle data filtering what makes consumables far more useful there.

@pjasiun
Copy link
Author

pjasiun commented Jan 15, 2018

After a lot of thinking, I realized that we need consumables. In the model to view conversion, they are needed to consume children if the parent will handle the conversion, or to overwrite some behavior. In the view to model conversion, consumables are needed for features like auto-paragraphing, which will be very tricky to implement without them. Even if, we would be able to simplify the mechanism in the model to view conversion and make consumables optional, we still need it in the view to model conversion and if we will make consumables obligated in one case and optional in the other, we will have even more bugs than we have now.

The only simplification we should do is to make the API simpler. Now, you need to transform event name to the consumable type using eventNameToConsumableType which apparently is very easy to forget about (I many converters event names are manually translated to consumable types). This is why this should be built in the consume and test methods, as a helper:

consumable.consume( data.item, evt.name ) // eventNameToConsumableType done internally

And:

consumable.test( data.item, evt.name )

Also, it could be exposed as consumable method:

consumable.toType( evt.name );

pjasiun referenced this issue in ckeditor/ckeditor5-engine Jan 19, 2018
Feature: Consumable type name is now normalized inside `conversion.ModelConsumable` methods. Closes #1214.
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-engine Oct 9, 2019
@mlewand mlewand added this to the iteration 14 milestone Oct 9, 2019
@mlewand mlewand added module:conversion type:improvement This issue reports a possible enhancement of an existing feature. package:engine labels Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:engine type:improvement This issue reports a possible enhancement of an existing feature.
Projects
None yet
3 participants