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

Convert view to model using positions #4225

Closed
pjasiun opened this issue Dec 20, 2017 · 2 comments · Fixed by ckeditor/ckeditor5-engine#1256
Closed

Convert view to model using positions #4225

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

Comments

@pjasiun
Copy link

pjasiun commented Dec 20, 2017

Context: view to model conversion.

Right now converters build nodes which are passed to the parent converters which build parent element with children it gets as an array of elements. There are couple problems with this:

  • the structure is not built until all top-level elements are converted, we can not use live ranges during the conversion because of it,
  • we need to pass "context" object to children which know nothing more about the structure than this context, can not check siblings, also context needs to be updated in every converter.

After closing https://github.com/ckeditor/ckeditor5-engine/issues/858 we should try to fix it.

Parent converter should pass to the child converter position which is the conversion context. Child converter should return the range with the converted fragment. Parent converter can execute its action before or after child converters.

Consider an example: <p><b>foo</b></p>

  • <p> converter creates <paragraph> and pass the position inside it to <b> converter (<paragraph>^</paragraph>),
  • <b> converter call foo converter first and pass the position it gets from <p> converter,
  • foo converter insert text on the position and returns range with inserted element (<paragraph>[foo]</paragraph>),
  • <b> will add attribute to the range.
@Reinmar
Copy link
Member

Reinmar commented Dec 29, 2017

This has a huge 👍 from me.

And the order of text conversion and attribute application which you described is an assumption which I made when implementing the new Schema. Its checkAttribute() method accepts a real node as its context. It doesn't accept anymore a fake context arrays of element names in addition to that node. This fake context arrays were used so far in buildviewconverter.js's setAttributeOn() to check whether this attribute can be applied in this specific context.

Since I removed the support for fake contexts (which are bad because they are fake so they don't carry all the information) I now need to fully depend on the passed node. Unfortunately, right now the conversion first try to apply the attribute (on a DF>$text) and only then insert it to the tree. Which means that checkAttribute( node, attrName ) will not know the whole context.

Once the conversion will first insert the text node to the model and then trigger attribute application, this will work perfectly fine with the new Schema.

But for now, I need to comment this test out because I can't make it pass easily:

https://github.com/ckeditor/ckeditor5-engine/blob/aea6119b12b5e3d009ff8597c3546ab8b488fe88/tests/conversion/buildviewconverter.js#L499-L516

PS. This is only one reason why I like the idea to use positions. The other one is – it's just so much easier to think this way ;)

pjasiun referenced this issue in ckeditor/ckeditor5-engine Jan 31, 2018
Feature: Convert view to model using position. Closes #1213. Closes #1250.

BREAKING CHANGE: `DataController#parse`, `DataController#toModel`, `ViewConversionDispatcher#convert` gets `SchemaContextDefinition` as a contex instead of `String`.
BREAKING CHANGE: `ViewConversionApi#splitToAllowedParent` has been introduced.
BREAKING CHANGE: `ViewConversionApi#storage` has been introduced.
BREAKING CHANGE: `ViewConsumable` has been merged to `ViewConversionApi`.
BREAKING CHANGE: Format od data object passed across conversion callback has been changed.
Feature: `Schema#findAllowedParent` has been introduced.
Feature: `SchemaContext#concat` has been introduced.
pjasiun referenced this issue in ckeditor/ckeditor5-paragraph Jan 31, 2018
Internal: Simplified autoparagraphing after engine view -> model conversion refactoring. Closes https://github.com/ckeditor/ckeditor5-engine/issues/1213.
@Reinmar
Copy link
Member

Reinmar commented Jan 31, 2018

Congrats guys! Amazing job :)

@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
Development

Successfully merging a pull request may close this issue.

4 participants