Introduced additional conversion options for downcast conversion. #7773

oskarwrobel · 2020-08-04T11:25:59Z

Suggested merge commit message (convention)

Feature: Introduced additional conversion options for downcast conversion. Closes #7628.

Additional information

For example – encountered issues, assumptions you had to make, other affected tickets, etc.

scofalik · 2020-08-05T11:50:19Z

packages/ckeditor5-engine/src/controller/datacontroller.js

@@ -151,10 +151,11 @@ export default class DataController {
 	 * @param {String} [options.trim='empty'] Whether returned data should be trimmed. This option is set to `empty` by default,
 	 * which means whenever editor content is considered empty, an empty string will be returned. To turn off trimming completely
 	 * use `'none'`. In such cases exact content will be returned (for example `<p>&nbsp;</p>` for an empty editor).
+	 * @param {Object} [options.conversionOptions] Additional, custom configuration passed to the conversion process.


Okay, let's talk about this. We can have one of three:

The way you implemented it (and we think about the name).

Having just one flat object ({ root: 'main', showSuggestions: true, foo: 'bar' }).

Having two parameters (and we think about names).

What I don't like about current solution is that you need to specify additional "level" of object if you want to pass "conversion option":

editor.getData( { conversionOptions: { showSuggestions: true } } )

I don't like it. Let's talk about other options.

Having two parameters isn't perfect either:

editor.getData( {}, { showSuggestions: true } )

Having it flat is a bit messy and may be difficult to document, but would be the easiest to use:

editor.getData( { showSuggestions: true } )
editor.getData( { root: 'other' } )

And when it comes to name, it should make sense from the user's perspective. conversionOptions doesn't make a lot of sense, especially if someone is not a programmer, does not know CKE5 internals and simply integrates editor and wants to use .getData().

resultOptions, resultConfiguration, dataOptions, dataConfiguration is the direction I'd go.

Or just having it all in a one, flat options object 🤷

I think that having one object would be best. For example, an option like options.trim also make sense to be available during conversion. From the integrator's perspective, one will not be able to understand why showSuggestions is a conversion option and trim is not. This is an implementation detail that should be abstracted.

That's why one options object should be enough.

scofalik · 2020-08-05T12:09:48Z

packages/ckeditor5-engine/src/conversion/downcastdispatcher.js

 	 */
-	convertInsert( range, writer ) {
+	convertInsert( range, writer, options = {} ) {


I am not sure I like this parameter here -- because you have to then document it the way you did. Maybe we could handle it somehow differently?

For example, set a property of conversionApi at the beginning of .get() and clear at the end? Hmmm.

Or maybe it is fine. But I'd change the documentation. In this place of API docs noone is interested where this parameter is used. This is kind of integration detail. We can write in the tutorial that options passed in .get() are later available in conversionApi but what you described is too much.

Correct me if I'm wrong, but in the current form, we'd need to have an additional param in every event and method.

In that case, conversionApi would be better as it's already widely available.

we'd need to have an additional param in every event and method

Not in every, but in convertInsert() and convertMarkerAdd(). To those methods that are used in DataController#toView().

For me, it sounds good to set options directly to the conversionApi (at the beginning of DataController#toView()) and remove when the conversion is done (at the end of DataController#toView())

scofalik · 2020-08-05T12:10:54Z

I left my notes. @Reinmar could you take a look at the API?

scofalik · 2020-08-07T08:23:42Z

After thinking for a while I think that I like the "flat object" solution the most.

scofalik · 2020-08-07T08:34:28Z

I'd document the options object this way:

* @param {Object} [options] Additional configuration for the retrived data. `DataController` provides two optional properties: `root` and `trim`. Other properties of this object are specified by various editor features.

I hope above makes sense and is understandable for users which are not familiar with CKE5 internals.

Reinmar · 2020-08-07T11:25:04Z

Quoting myself:

I think that having one object would be best. For example, an option like options.trim also makes sense to be available during conversion. From the integrator's perspective, one will not be able to understand why showSuggestions is a conversion option and trim is not. This is an implementation detail that should be abstracted.

That's why one options object should be enough.

So, I think we agree with each other @scofalik.

scofalik · 2020-08-10T11:43:45Z

packages/ckeditor5-core/src/editor/utils/dataapimixin.js

@@ -76,5 +76,6 @@ export default DataApiMixin;
 * @param {String} [options.trim='empty'] Whether returned data should be trimmed. This option is set to `'empty'` by default,
 * which means that whenever editor content is considered empty, an empty string is returned. To turn off trimming
 * use `'none'`. In such cases exact content will be returned (for example `'<p>&nbsp;</p>'` for an empty editor).
+ * @param {Object} [options.conversionOptions] Additional, custom configuration passed to the conversion process.


This is no longer valid.

scofalik · 2020-08-10T11:45:43Z

packages/ckeditor5-engine/src/controller/datacontroller.js

@@ -146,15 +146,16 @@ export default class DataController {
 	 * Returns the model's data converted by downcast dispatchers attached to {@link #downcastDispatcher} and
 	 * formatted by the {@link #processor data processor}.
 	 *
-	 * @param {Object} [options]
+	 * @param {Object} [options] Additional configuration for the retrieved data. `DataController` provides two optional
+	 * properties: `root` and `trim`. Other properties of this object are specified by various editor features.


scofalik · 2020-08-10T11:49:04Z

packages/ckeditor5-engine/src/controller/datacontroller.js

@@ -220,6 +224,9 @@ export default class DataController {

 		this.mapper.bindElements( modelElementOrFragment, viewDocumentFragment );

+		// Make additional options available during conversion process through conversionSpi.


Suggested change

// Make additional options available during conversion process through conversionSpi.

// Make additional options available during conversion process through `conversionApi`.

scofalik · 2020-08-10T11:49:21Z

packages/ckeditor5-engine/src/controller/datacontroller.js

@@ -233,6 +240,9 @@ export default class DataController {
 			}
 		}

+		// Remove options from conversionApi.


Suggested change

// Remove options from conversionApi.

// Clean `conversionApi`.

scofalik · 2020-08-10T11:49:48Z

packages/ckeditor5-engine/src/conversion/downcastdispatcher.js

@@ -166,6 +165,7 @@ export default class DowncastDispatcher {
 	 * @fires attribute
 	 * @param {module:engine/model/range~Range} range The inserted range.
 	 * @param {module:engine/view/downcastwriter~DowncastWriter} writer The view writer that should be used to modify the view document.
+	 * {module:engine/conversion/downcastdispatcher~DowncastConversionApi conversion api}. Used only with `dataDowncast` group.


scofalik · 2020-08-10T11:49:58Z

packages/ckeditor5-engine/src/conversion/downcastdispatcher.js

@@ -319,6 +319,7 @@ export default class DowncastDispatcher {
 	 * @param {String} markerName Marker name.
 	 * @param {module:engine/model/range~Range} markerRange Marker range.
 	 * @param {module:engine/view/downcastwriter~DowncastWriter} writer View writer that should be used to modify view document.
+	 * {module:engine/conversion/downcastdispatcher~DowncastConversionApi conversion api}. Used only with `dataDowncast` group.


scofalik · 2020-08-10T12:10:22Z

packages/ckeditor5-core/src/editor/utils/dataapimixin.js

@@ -71,7 +71,8 @@ export default DataApiMixin;
 * the right format for you.
 *
 * @method #getData
- * @param {Object} [options]
+ * @param {Object} [options] Additional configuration for the retrieved data. By default two optional


How about ...

Additional configuration for the retrieved data. Editor features may introduce more configuration options that can be set through this parameter.

I think I'd leave DataController description as it is now. editor.getData() is a very top-level method, its description needs to be as simple as it can.

packages/ckeditor5-engine/src/conversion/downcastdispatcher.js

oskarwrobel requested a review from scofalik August 4, 2020 11:25

Introduced additional conversion options for downcast conversion.

0b2044e

oskarwrobel force-pushed the t/7628 branch from 827cb06 to 0b2044e Compare August 4, 2020 11:31

scofalik reviewed Aug 5, 2020

View reviewed changes

scofalik self-requested a review August 5, 2020 14:15

Made getData options flat.

5b8a7b1

oskarwrobel added 3 commits August 7, 2020 14:06

Merge branch 'master' into t/7628

81b6d11

Do not remove basic options from DataController#get config.

ef301bb

Changed the way of passing options to the downcast conversion process.

719a13c

scofalik reviewed Aug 10, 2020

View reviewed changes

Imporved docs.

134ff46

scofalik reviewed Aug 10, 2020

View reviewed changes

scofalik requested review from Reinmar and removed request for Reinmar August 10, 2020 12:12

More docs improvements.

5c08d2f

scofalik reviewed Aug 10, 2020

View reviewed changes

packages/ckeditor5-engine/src/conversion/downcastdispatcher.js Outdated Show resolved Hide resolved

scofalik and others added 3 commits August 10, 2020 14:28

Update packages/ckeditor5-engine/src/conversion/downcastdispatcher.js

d099834

More and more docs.

27453f6

Merge branch 't/7628' of github.com:ckeditor/ckeditor5 into t/7628

790c50d

scofalik merged commit 0a5d07e into master Aug 10, 2020

scofalik deleted the t/7628 branch August 10, 2020 12:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduced additional conversion options for downcast conversion. #7773

Introduced additional conversion options for downcast conversion. #7773

oskarwrobel commented Aug 4, 2020

scofalik Aug 5, 2020

scofalik Aug 5, 2020

Reinmar Aug 7, 2020

scofalik Aug 5, 2020

Reinmar Aug 7, 2020

oskarwrobel Aug 10, 2020 •

edited

Loading

oskarwrobel Aug 10, 2020 •

edited

Loading

scofalik commented Aug 5, 2020

scofalik commented Aug 7, 2020

scofalik commented Aug 7, 2020 •

edited

Loading

Reinmar commented Aug 7, 2020

scofalik Aug 10, 2020

scofalik Aug 10, 2020

scofalik Aug 10, 2020

scofalik Aug 10, 2020

scofalik Aug 10, 2020

scofalik Aug 10, 2020

scofalik Aug 10, 2020

scofalik Aug 10, 2020

		@@ -220,6 +224,9 @@ export default class DataController {

		this.mapper.bindElements( modelElementOrFragment, viewDocumentFragment );

		// Make additional options available during conversion process through conversionSpi.

	// Remove options from conversionApi.
	// Clean `conversionApi`.

Introduced additional conversion options for downcast conversion. #7773

Introduced additional conversion options for downcast conversion. #7773

Conversation

oskarwrobel commented Aug 4, 2020

Suggested merge commit message (convention)

Additional information

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oskarwrobel Aug 10, 2020 • edited Loading

Choose a reason for hiding this comment

oskarwrobel Aug 10, 2020 • edited Loading

Choose a reason for hiding this comment

scofalik commented Aug 5, 2020

scofalik commented Aug 7, 2020

scofalik commented Aug 7, 2020 • edited Loading

Reinmar commented Aug 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oskarwrobel Aug 10, 2020 •

edited

Loading

oskarwrobel Aug 10, 2020 •

edited

Loading

scofalik commented Aug 7, 2020 •

edited

Loading