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

Features: Converters #125

Closed
scofalik opened this issue Feb 8, 2016 · 10 comments
Closed

Features: Converters #125

scofalik opened this issue Feb 8, 2016 · 10 comments
Labels

Comments

@scofalik
Copy link

scofalik commented Feb 8, 2016

Features concept

This is a part of Features concept. This issue discusses converters. See also #129.

Converters

Converters are functions that translate Tree Model to Tree View and vice versa. Converters are working on changed part of Model / View. Each converter is looking for specific data in changed elements, recognizes it and executes conversion. It is not yet decided how exactly Feature should publish or register those converters.

When converting from Tree Model to Tree View, converters will mostly look for specific element name or attribute. Conversion from Tree View to Tree Model may be more complicated and depends on the Feature.

(Below needs verification, I might understood it incorrectly.)

One thing that was discussed about converters was how the changes in Tree View will be processed and "consumed". A change in Tree View will mostly be a result of user typing, pasting or dragging something in browser. When that happens, Tree View will be updated with new elements and they will start to be processed by converters. Every element ready to be processed will create a set of "consumables". "Consumables" are everything that is a specific to the element and can be recognized by the creator: element name, element attributes (meaning DOM attributes), element class list, element style properties with values.

Once a converter recognize that the element has something that can be consumed by that converter, it will process that element, apply changes on Tree Model and remove the recognized value from consumables set. This means that each specific property of the node will be recognizes and processed only once. It is important, since converter may try to recognize elements by multiple properties at once (i.e. looking for span element with style font-weight: bold). If font-weight will get consumed first, the first converter won't have a chance to convert that element. This is something that we have to keep an eye on, it may lead to unexpected problems. We will need some prioritization when registering converters. Maybe it should be connected with schema?

(Ideas on this are more than welcome.)

It is important to note that Tree View elements that were marked as changed and then not get recognized, won't appear in Tree Model, which in return won't be then converted back to Tree View. So not recognized parts of Tree View will end up being removed. In same fashion, if b element is converted to bold attribute in Tree Model and then attribute bold is converted to strong we end up with b element substituted by strong element. This is a smart and automatic way to take care of DOM / HTML code quality.

@scofalik
Copy link
Author

We have some new thoughts on converters registration:

  1. Converters might be using editor configuration so they should not be some kind of static functions (class static methods).

  2. We would like to have a bigger control over how we register converters and how many of them we register. It's easy to imagine that some complicated features might have multiple converters, especially from Tree View to Tree Model.

  3. Converters will be registered using a registration function, that will be a method of Editor, Document or maybe different class.

  4. This is a sketch of how it would look like (this is in init() function of Feature):

this.editor.registerModelToViewConverter(
    [
        { attribute: 'bold' }
    ],
    ( changes ) => {
    }
);

this.editor.registerViewToModelConverter(
    [
        { tag: 'b' },
        { tag: 'strong' },
        { style: 'font-weight:bold' }
    ],
    ( viewElement ) => {
    }
);

Both functions take converter callback as second parameter.
registerModelToViewConverter first parameter specifies which attributes / elements or elements with an attribute should be converted.
registerViewToModelConverter first parameter specifies which "consumables" of Tree View element should be recognized and consumed by the converter.

This is only a proposal, of course both registering functions do not exist yet.

@scofalik
Copy link
Author

Another subject for discussion is what exactly converter should take as an argument and what it should do / return. Unfortunately, we will probably have to wait for @pjasiun to come back from leave.

@Reinmar
Copy link
Member

Reinmar commented Feb 18, 2016

https://github.com/ckeditor/ckeditor5-core/compare/t/features?expand=1#diff-a4b9b21ba1c2da39a8a294394a69c5eeR25

Those will need to be moved to some util modules, so you import them only when needed. Otherwise the TreeController class will grow with time because we'll be adding there more and more util functions. In the core we need only a low level super generic API for registering a converter callback.

On the other hand, I don't know how big the implementation of such converter could be. If we're expecting that some of them will be <5LOC, then perhaps splitting the code too much may be an overkill because we'll lose more on the module boilerplate code (such as define(...., function() { ... } ) calls in AMD).

@pjasiun
Copy link

pjasiun commented Feb 18, 2016

And maybe registerAttributeConverter and registerViewToModelConverter could be merged into one helper.

@scofalik
Copy link
Author

Yup. It's up to you how you will "execute" it. I just needed some placeholder to show how BoldFeature will integrate with CKE5 envrionment.

If we're expecting that some of them will be <5LOC

We tried to write example converter for bold feature and belive me it was much much more than 5LOC. Hence we decided it will be best to create kind-of converter factory that will create converter callbacks basing on a few parameters.

@pjasiun
Copy link

pjasiun commented Feb 18, 2016

And maybe registerAttributeConverter and registerViewToModelConverter could be merged into one helper.

On the other hand I like having these two separated, since it is not a lot of code.

@pjasiun
Copy link

pjasiun commented Feb 18, 2016

this.editor.treeController.registerAttributeConverter( 'bold', true, 'strong' );

What is true? Sounds like a philosophical question...

Anyway. I realised that we may need smarter attribute converter which will transform attribute value into a style or something. Or the model attribute value may be an object with multiple values (e.g. link with { src: ... target: ...} as value). Maybe it should be something like this:

registerAttributeConverter( 'bold', ( value ) => new TreeElement( 'strong' ) );
registerAttributeConverter( 'link', ( value ) => new TreeElement( 'a', {
    src: value.src,
    target: value.target
} ) );

@pjasiun
Copy link

pjasiun commented Feb 18, 2016

As a comment to the original message, and after the F2F talk.

First of all, we have a naming collision. The generic mechanisms for transforming tree model into the tree view and the tree view into the tree model are called "converters". And the specific callbacks which tell how to transform bold or paragraph are also called "converters". To solve this problem I propose to call generic conversion mechanism "tree controller" (or tree conversion controller).

Second, we have already tickets for view to model and model to view conversion controllers in the core repository:
ckeditor/ckeditor5-core#179
https://github.com/ckeditor/ckeditor5-core/issues/180

They are under development, they are not a part of the feature and the discussion about the development details, like the list of elements to consume, should be moved there.

This ticket should only focus on the specific converters, some helpers and how they should be defined in features.

@pjasiun
Copy link

pjasiun commented Feb 18, 2016

About the specific converters it will be possible to add a callback, which will looks like this:

// Element conversion:

editor.treeController.registerModelToViewConverter( ( model, view, writer, modelPosition, changes ) => {
    let modelNode = modelPosition.nodeAfter;
    if( modelNode.name == 'img' ) {
        if( changes.consume( [ [ modelNode, TAG ], [ modelNode, 'src' ] ] ) { // Consume both or none.
            const viewPosition = editor.treeController.mapper.toViewPosition( modelPosition );
            const viewNode = new ViewElement( 'img', { 'src': modelNode.getAttribute( 'src' ) } );
            editor.treeController.mapper.bind( modelNode, viewNode );
            writer.insert( viewPosition, viewNode );
        }
    }
} );

editor.treeController.registerViewToModelConverter( ( model, view, batch, viewPosition, changes ) => {
    let viewNode = viewPosition.getNodeAfter();
    if( viewNode.name == 'img' ) {
        if( changes.consume( [ [ viewNode, TAG ], [ viewNode, ATTRIBUTE, 'src'] ] ) ) {
            const modelPosition = editor.treeController.mapper.toModelPosition( viewPosition );
            const modelNode = new ModelElement( 'img', { 'src': viewNode.getAttribute( 'src' ) } );
            editor.treeController.mapper.bind( viewNode, modelNode );
            batch.insert( modelPosition, modelNode );
        }
    } );
} );

Then we need helpers, because 90% of cases will be pretty simple. Beside registerAttributeConverter, described 2 post ago, we could also bring registerElementConverter:

registerElementConverter( 'img', ( modelElement ) => new ViewElement( 'img', { 'src': modelElement.getAttribute( 'src' ) } );

@Reinmar
Copy link
Member

Reinmar commented May 5, 2017

Conversion turned to be one fo the most hardcore topics we are dealing with. The current approach works but it's too complicated. We'll be definitely improving it.

Some tickets to follow:

@Reinmar Reinmar closed this as completed May 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants