T/1556: Expose conversion utilities

[jodator]

Suggested merge commit message (convention)

Enhancement: Expose conversion utilities. Closes ckeditor/ckeditor5#4428.

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.

---

Additional information

• This PR comes with constelation of changes gathered in ckeditor5 branch:
  • ckeditor5-block-quote
  • ckeditor5-ckfinder
  • ckeditor5-core
  • ckeditor5-editor-balloon
  • ckeditor5-editor-inline
  • ckeditor5-enter
  • ckeditor5-heading
  • ckeditor5-image
  • ckeditor5-link
  • ckeditor5-list
  • ckeditor5-media-embed
  • ckeditor5-paragraph
  • ckeditor5-table
  • ckeditor5-typing
  • ckeditor5-ui
  • ckeditor5-undo
  • ckeditor5-widget
• Proposed merge commit message:

   Align code to the latest changes in conversion helpers API.
  

• I think that I've exposed all the required utilities:
  • For downcast (model-to-view conversion), these are:
    • elementToElement
    • attributeToElement
    • attributeToAttribute
    • markerToElement
    • markerToHighlight
  • For upcast (view-to-model conversion), these are:
    • elementToElement
    • elementToAttribute
    • attributeToAttribute
    • elementToMarker
• All above converter methods are marked as @protected and underscored.
• The API is still chainable (the helpers uses this.add() internally to add a protected converter):

   editor.conversion.for( 'upcast' )
   	.add( customConverterA )
   	.elementToElement( { model: 'foo', view: 'bar' } )
   	.add( customConverterB )
   	.elementToAttribute( { model: 'baz', view: 'bez' } );

---

Some questions:

• There are other exported methods in those files - what to do with them? Chceck the code end remove exports if used internally? Methods like convertText(), convertToModelFragment() etc.
• The functions were left in place to not loose git blame immediate history in those functions. Also in this file export and private methods are mixed - should we sort them as public export > protected export > internal/private?
• We might do also: https://github.com/ckeditor/ckeditor5-engine/issues/1335.

[coveralls]

Coverage decreased (-25.7%) to 74.299% when pulling 2f41916 on t/1556 into 7f40831 on master.

[jodator]

ps.: The code works with all repos checked out as in CKEditor5 (the core is crucial as it adds proper helpers to dispatcher groups).

[pjasiun]

• There are other exported methods in those files - what to do with them? Chceck the code end remove exports if used internally? Methods like convertText(), convertToModelFragment() etc.

convertText() is used in the engine, but it a different file. I would do it this way:

• methods which are used in the same file - remove exports,
• methods which are used in other files (in other parts of the engine) - keep public.

• The functions were left in place to not loose git blame immediate history in those functions. Also in this file export and private methods are mixed - should we sort them as public export > protected export > internal/private?

You can sort it, we used to the fact that git blame does not work in the refactored code.

• We might do also: #1335.

Let's do it in a separate ticket.

[pjasiun]

Since methods have the same names now, shouldn't they have the same parameters names too?

[pjasiun]

I mean converterPriority vs priority, in conversion.for( 'downcast' ).attributeToElement vs conversion.attributeToElement.

[jodator]

AFAIR it was due to:
https://ckeditor.com/docs/ckeditor5/latest/api/module_engine_view_elementdefinition-ElementDefinition.html
is used in: https://ckeditor.com/docs/ckeditor5/latest/api/module_engine_conversion_conversion-ConverterDefinition.html

[pjasiun]

It looks more like a bug. There is even an example in the documentation where converterPriority is used. Also docs says that type of definition is ConverterDefinition which uses converterPriority there is nothing about definition.priority property so I believe it should be simply:

Suggested change

	converterPriority: definition.priority
	converterPriority: definition.converterPriority

[jodator]

@pjasiun I like the class idea so to be 100% sure we're on the same page I'll describe how it will be refactored before doing it:

1. There will be base ConversionHelpers class with .add() method.
2. There will be to classed UpcastHelpers (curren upcastHelpers) and DowncastHelpers (current downcastHelpers)
3. Registering conversion pipeline:

this.conversion.register(
    'downcast',
    new DowncastHelpers( [ 
        this.editing.downcastDispatcher,
        this.data.downcastDispatcher
    ]
);
this.conversion.register( 'upcast', new UpcastHelpers( this.data.upcastDispatcher ) );

---

Forget about this: 👇

Also I'd move '*cast-helpers' to *CastHelpers class and leave current *cast-converters in *cast-converters.js files (check how the tests are constructed).

Edit: ☝️ or not... there are many private method used in both helpers and converters.

but the name should be different, IDK... since the conversion.register() "Registers one or more converters" (should be dispatchers?):

this.conversion.register( 'upcast', new UpcastDispatchers( this.data.upcastDispatcher ) );

so UpcastHelpers -> UpcastDispatchers

[pjasiun]

I agree with the first half, up to "Also" :) @jodator said that I should not care about the second half ;)

[jodator]

@pjasiun done

[pjasiun]

Do we need all of these exports in downcast helpers? (see https://github.com/ckeditor/ckeditor5-engine/issues/1306#issuecomment-378870980). It would be great to expose only these methods:

DowncastHelpers
insertText
remove
createViewElementFromHighlightDescriptor

When it comes to testing, the rest of them should have only integrational tests.