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

Expose all conversion utilities #4428

Closed
Reinmar opened this issue Sep 20, 2018 · 18 comments · Fixed by ckeditor/ckeditor5-engine#1613
Closed

Expose all conversion utilities #4428

Reinmar opened this issue Sep 20, 2018 · 18 comments · Fixed by ckeditor/ckeditor5-engine#1613
Assignees
Labels
package:engine type:improvement This issue reports a possible enhancement of an existing feature.
Milestone

Comments

@Reinmar
Copy link
Member

Reinmar commented Sep 20, 2018

A sibling ticket of https://github.com/ckeditor/ckeditor5-engine/issues/1555.

We exposed half of conversion utilities as editor.conversion's methods. What we miss are the methods from:

Some of those functions are needed quite frequently which we experienced when answering to various questions. I think that in most questions about conversion we needed some of these lower-level functions rather than those big factories we already have exposed.

I can see that there are quite a lot of downcast-converters but I'd expose all of them anyway, without a research which of them are used and how often (half of them are used in quite advanced cases with markers, but we'll want to make markers more popular anyway).

I'd expose this functions as e.g.:

editor.conversion.downcastConverters.downcastElementToElement()
editor.conversion.upcastConverters.upcastElementToElement()
@Reinmar Reinmar self-assigned this Sep 20, 2018
@pjasiun
Copy link

pjasiun commented Sep 20, 2018

Maybe?

editor.conversion.downcast.elementToElement()
editor.conversion.upcast.elementToElement()

@pjasiun
Copy link

pjasiun commented Sep 20, 2018

Or just:

editor.conversion.downcastElementToElement()
editor.conversion.upcastElementToElement()
```

@pjasiun
Copy link

pjasiun commented Sep 20, 2018

I can see that there are quite a lot of downcast-converters

Note that this is because we still do not close https://github.com/ckeditor/ckeditor5-engine/issues/1306.

@Reinmar
Copy link
Member Author

Reinmar commented Sep 20, 2018

Note that this is because we still do not close #4281.

I'm actually unsure now about hiding them :D With time, I got the feeling that most questions about conversion are questions about some odd cases. Like, extending existing converters, overriding some things, etc. I can't tell from my experience which of the functions we need, but if these helpers are used in our converters, there's a high chance that they can help in people's scenarios.

@pjasiun
Copy link

pjasiun commented Sep 21, 2018

if these helpers are used in our converters, there's a high chance that they can help in people's scenarios.

It's not exactly this that, that we have helpers and then use them in converters. It is more like: we have converters and then part of these converters is exported besides these converters as the whole. It is not like wrap is a flexible until and there are are converters using it. It is just a part of downcastAttributeToElement designed as a listener attached by this function. Note that there is a 1-1 connection between converters and "utils", they are not reusable between converters.

@pjasiun
Copy link

pjasiun commented Sep 21, 2018

I do not mean that these parts of converters will be useless, but they might be very hard to document them in a clear way. I think that you can more-less easily explain what converters and writer are, but it will be hard to tell how to use this "helpers" properly (btw we used to call them converters in the past). It was easier before the previous refactoring, but then we introduce one more level (called "converters" today).

@Reinmar
Copy link
Member Author

Reinmar commented Sep 21, 2018

Hm, ok. I'll use your list from https://github.com/ckeditor/ckeditor5-engine/issues/1306#issuecomment-378870980.

The public ones will be available via conversion.upcast/downcast. The protected ones will be still exported but documented as protected (I should also prepend their names with _). To avoid confusion and duplication in the API docs, I'd stop exporting the public ones – they'd be only available in conversion.upcast/downcast. WDYT?

@pjasiun
Copy link

pjasiun commented Sep 21, 2018

👍

You can also skip insertText and remove since they have a very specific, hardcoded usage. The rest should be useful.

@Reinmar
Copy link
Member Author

Reinmar commented Sep 24, 2018

Hm... there's a problem with the naming. Look at the code I wrote in https://github.com/ckeditor/ckeditor5-table/issues/122#issuecomment-424100223.

After the API change I'd write this:

editor.conversion.upcast.attributeToAttribute( {
    // ..
} );

And this wouldn't work as well ;| The inconsistency in this looks very bad:

// You need to use for().add()...
editor.conversion.for( 'upcast' ).add(
	editor.conversion.upcast.attributeToAttribute( {
		// ...
	} )
);

// But this time you don't...
editor.conversion.elementToElement( {
	// ...
} );

Do you have any ideas how we could straighten this up?

@Reinmar
Copy link
Member Author

Reinmar commented Sep 24, 2018

One thing would be to be careful in the documentation and always use editor.conversion.for().add() when necessary. But this still will look bad in the code. We may try with a bit extended naming like:

editor.conversion.upcast.getAttributeToAttributeConverter()
// or...
editor.conversion.upcast.createAttributeToAttributeConverter()

Which indicatest that this is a getter/factory. But this way we'll make the names awfully long so it'd be good to find a better option.

The second problem I have is that editor.conversion.upcast... sounds like "let's do something" cause I read "upcast" as a verb. We wanted this to be a namespace, but I think this is risky.

@Reinmar
Copy link
Member Author

Reinmar commented Sep 24, 2018

One thing I'm sure is that we shouldn't mix those converter factories (which require to be used in for().add() in the editor.conversion.* object. That's because we have those two-way converters there already so mixing them together would be confusing too.

Another thing that I didn't like in the code samples I show above is the length of them. This looks sad:

editor.conversion.for( 'upcast' ).add(
	editor.conversion.upcast.attributeToAttribute( {
		// ...
	} )
);

What if... for() would return an object with more methods than just add()?

editor.conversion.for( 'upcast' ).attributeToAttribute( {
    // ...
} );

This way:

  • we keep this maximally short,
  • you cannot use this method in a wrong way simply because you don't need to do anything more than this.

@jodator
Copy link
Contributor

jodator commented Sep 25, 2018

What with dataDowncast & editingDowncast in the downcast pipeline?

I like the editor.conversion.downcast.elementToElement() format more then

editor.conversion.for( 'upcast' ).add(
	editor.conversion.upcast.attributeToAttribute( {
		// ... too much and confusing :/
	} )
);

as it is consistent with foo.bar.conversion.elementToElement() helpers..

So then we should also expose them:

editor.conversion.downcast.elementToElement()
editor.conversion.downcast.data.elementToElement()
editor.conversion.downcast.editing.elementToElement()
// or
editor.conversion.downcast.elementToElement()
editor.conversion.dataDowncast.elementToElement()
editor.conversion.editingDowncast.elementToElement()

@Reinmar Reinmar removed their assignment Dec 5, 2018
@jodator jodator self-assigned this Dec 11, 2018
@jodator
Copy link
Contributor

jodator commented Dec 12, 2018

All below proposition will take the simples custom converter for image - using 'dataDowncast' pipeline:

import {
	downcastElementToElement
} from '@ckeditor/ckeditor5-engine/src/conversion/downcast-converters';

editor.conversion.for( 'dataDowncast' ).add( downcastElementToElement( {
    model: 'image',
    view: ( modelElement, viewWriter ) => createImageViewElement( viewWriter )
} ) );

I: initial proposition (updated)

// ver I a.
editor.conversion.for( 'dataDowncast' )
    .add( editor.conversion.createDowncastElementToElement( {
        model: 'image',
        view: ( modelElement, viewWriter ) => createImageViewElement( viewWriter )
    } ) );

// ver I b.
editor.conversion.for( 'dataDowncast' )
    .add( editor.conversion.downcast.createElementToElement( {
        model: 'image',
        view: ( modelElement, viewWriter ) => createImageViewElement( viewWriter )
    } ) );

Comments:

  • lengthy method names & code bloat

II: (PK's proposal) expose all converters in .for()

editor.conversion.for( 'dataDowncast' ).downcastElementToElement( {
    model: 'image',
    view: ( modelElement, viewWriter ) => createImageViewElement( viewWriter )
} );

// Problem: nothing prevents this ¯\_(ツ)_/¯
editor.conversion.for( 'upcast' ).downcastElementToElement( {
    model: 'image',
    view: ( modelElement, viewWriter ) => createImageViewElement( viewWriter )
} );

Comments:

  • either exposes all (:-1:) or requires conversion group to explicitly add helpers (check below)
  • Slightly bloated docs? (or not - check below)
  • Dunno if we wanna keep them to be chainable (as with .add()?

III: Prop: explicit helper chain (kinda meh but I'll post it anyway)

editor.conversion.for( 'dataDowncast' )
    .addDowncastHelper()
    .elementToElement( {
        model: 'image',
        view: ( modelElement, viewWriter ) => createImageViewElement( viewWriter )
    } );

editor.conversion.for( 'upcast' )
    .addUpcastHelper()
    .elementToElement( {
        model: 'image',
        view: ( modelElement, viewWriter ) => createImageViewElement( viewWriter )
    } );

// But also:
editor.conversion.for( 'downcast' )
    .addUpcastHelper()
    .elementToElement( {
        model: 'image',
        view: ( modelElement, viewWriter ) => createImageViewElement( viewWriter )
    } );

Comments:

  • not chainable - or it will be fugly
  • too explicit?

I would go with PK's (II) proposal as it looks the best but I'd limit available helpers returned from .for() for upcast/downcast only.
It would require defining them in different way though:

this.conversion.register( 'downcast',
    [ this.editing.downcastDispatcher, this.data.downcastDispatcher ] );
this.conversion.register( 'editingDowncast', [ this.editing.downcastDispatcher ] );
this.conversion.register( 'dataDowncast', [ this.data.downcastDispatcher ] );
this.conversion.register( 'upcast', [ this.data.upcastDispatcher ] );

// Will require to expose API for proper upcast/downcast
this.conversion.register(
    'downcast',
    [ this.editing.downcastDispatcher, this.data.downcastDispatcher ],
    { useDowncastHelpers: true }
);
this.conversion.register(
    'upcast',
    [ this.data.upcastDispatcher ],
    { useUpcastHelpers: true }
);

// Or
this.conversion.register(
    'upcast',
    [ this.data.upcastDispatcher ],
    { helpers: UpcastHelpers }  // or new UpcastHelpers...
);

@scofalik
Copy link
Contributor

scofalik commented Dec 12, 2018

I am for PKs proposal. Three things.

  1. I don't mind that it is possible to add an upcast converter to the downcast pipeline. It is possible at the moment, so this would not be any kind of regression. You can do a lot of things incorrectly when coding and we cannot and should not secure them all. 🤷‍♂️
  2. Remember to still keep .add() method as it can be used for custom converters.
  3. I think chaining is a tertiary feature so I wouldn't care about it.

@jodator
Copy link
Contributor

jodator commented Dec 12, 2018

After F2F talks @pjasiun & @scofalik we agreed on PKs proposal with defining which helpers goes for which pipeline. Also due to fact of increased numbers of parameters the conversion.register() will accept an object:

this.conversion.register( {
    name: 'downcast',
    dispatchers: [ this.editing.downcastDispatcher, this.data.downcastDispatcher ],
    helpers: DowncastHelpers
} );

There will be two objects (interfaces) which will group proper helpers (DowncastHelpers & UpcastHelpers) - the name can be discussed during PR as now I don't have final name proposition.

The chaining is not required.

@pjasiun
Copy link

pjasiun commented Dec 12, 2018

Agree with @jodator and @scofalik (and PK) that proposal II, with registering helpers for each pipeline, looks the best. I would go with:

this.conversion.register( {
    name: 'upcast',
    dispatcher: this.data.upcastDispatcher, //acepts also array dispatcher: [ this.editing.downcastDispatcher, this.data.downcastDispatcher ]
    helpers: UpcastHelpers // or upcastHelpers
);

Then this.conversion.for( 'upcast' ) returns upcastHelpers so you can do:

this.conversion.for( 'upcast' ).elementToElement

@jodator
Copy link
Contributor

jodator commented Dec 12, 2018

🏆 first

@pjasiun
Copy link

pjasiun commented Dec 12, 2018

By the way it will be nice that you will be able to do:

this.conversion.elementToElement
this.conversion.for( 'dataDowncast' ).elementToElement
this.conversion.for( 'upcast' ).elementToElement

And each of these will execute the proper method.

pjasiun referenced this issue in ckeditor/ckeditor5-engine Dec 21, 2018
Enhancement: Expose conversion utilities. Closes #1556.

BREAKING CHANGE: The conversion.register() method now accepts single options object as a parameter.
BREAKING CHANGE: The downcastElementToElement() helper was removed from public API. Use conversion.for( 'downcast' ).elementToElement() instead.
BREAKING CHANGE: The downcastAttributeToElement() helper was removed from public API. Use conversion.for( 'downcast' ).attributeToElement() instead.
BREAKING CHANGE: The downcastAttributeToAttribute() helper was removed from public API. Use conversion.for( 'downcast' ).attributeToAttribute() instead.
BREAKING CHANGE: The downcastMarkerToElement() helper was removed from public API. Use conversion.for( 'downcast' ).markerToElement() instead.
BREAKING CHANGE: The downcastMarkerToHighlight() helper was removed from public API. Use conversion.for( 'downcast' ).markerToHighlight() instead.
BREAKING CHANGE: The upcastElementToElement() helper was removed from public API. Use conversion.for( 'upcast' ).elementToElement() instead.
BREAKING CHANGE: The upcastElementToAttribute() helper was removed from public API. Use conversion.for( 'upcast' ).elementToAttribute() instead.
BREAKING CHANGE: The upcastAttributeToAttribute() helper was removed from public API. Use conversion.for( 'upcast' ).attributeToAttribute() instead.
BREAKING CHANGE: The upcastElementToMarker() helper was removed from public API. Use conversion.for( 'upcast' ).elementToMarker() instead.
BREAKING CHANGE: The insertUIElement() and removeUIElement() downcast converters were removed from public API. Use conversion.for( 'downcast' ).markerToElement() instead.
BREAKING CHANGE: The highlightText(), highlightElement() and removeHighlight() downcast converters were removed from public API. Use conversion.for( 'downcast' ).markerToHighlight() instead.
BREAKING CHANGE: The insertElement() downcast converter was removed from public API. Use conversion.for( 'downcast' ).elementToElement() instead.
BREAKING CHANGE: The changeAttribute() downcast converter was removed from public API. Use conversion.for( 'downcast' ).attributeToAttribute() instead.
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-engine Oct 9, 2019
@mlewand mlewand added this to the iteration 22 milestone Oct 9, 2019
@mlewand mlewand added status:confirmed 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.

5 participants