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

Change-based conversion can be really confusing #3954

Closed
Reinmar opened this issue Jan 24, 2017 · 1 comment
Closed

Change-based conversion can be really confusing #3954

Reinmar opened this issue Jan 24, 2017 · 1 comment
Labels
package:engine resolution:duplicate This issue is a duplicate of another issue and was merged into it. status:discussion type:question This issue asks a question (how to...).

Comments

@Reinmar
Copy link
Member

Reinmar commented Jan 24, 2017

If things like https://github.com/ckeditor/ckeditor5-image/issues/31 are required, then we have a problem, either with the concept itself or how we sell it.

As we know, the concept of converting a change has its merits and it's unlikely that we'll be able to change it. It allows for a better composition of features and reduces overhead on the changes coming from the collaboration. If we'd like to have dirty fragments conversion, we'd need to translate applied changes to regions to re-convert, including things like selection and markers. Taking feature composition into consideration, it is very hard to imagine such a solution and, very likely, it wouldn't be a clean and clear concept either.

Anyway, apparently we need to work on how we sell the conversion. E.g. if each model change requires its own converter, then converter builders should not propose consuming more than either an element or an attribute. Which means that perhaps:

  • We could have some warning that something wasn't consumed (IIRC, this was requested a couple of times already :D). In this specific case, AFAIU, the image src attr wasn't consumed because the builder didn't do that.
  • There should be no generic converterBuilder.from() method (in fact, I see that it's been just removed, now, only the specialised ones are public).

And finally, we need to be very clear about this in the documentation, other places in the API (like – if you have a converter function, the name should always state whether it converts an element or attribute), etc.

@Reinmar Reinmar changed the title Document change-based conversion can be really confusing Change-based conversion can be really confusing Jan 24, 2017
@Reinmar
Copy link
Member Author

Reinmar commented Oct 11, 2017

@Reinmar Reinmar closed this as completed Oct 11, 2017
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-engine Oct 9, 2019
@mlewand mlewand added resolution:duplicate This issue is a duplicate of another issue and was merged into it. status:discussion type:question This issue asks a question (how to...). 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 resolution:duplicate This issue is a duplicate of another issue and was merged into it. status:discussion type:question This issue asks a question (how to...).
Projects
None yet
Development

No branches or pull requests

2 participants