Declarative reconversion using triggerBy #8099

jodator · 2020-09-17T11:00:21Z

Suggested merge commit message (convention)

Feature (engine): Introduce automatic model-to-view reconversion by defining triggerBy option for elementToElement() conversion helper. Closes #7956.

Other (table): Table cell contents refreshing for the editing view will now make less view updates.

Additional information

This PR:

introduces the triggerBy configuration option for downcast elementToElement() conversion helper config.
Handles element re-conversion and view momoization on DowncastDispatcher level.
- The model element children that have exiting view are not reconverted (memoization).
- Newly inserted children are converted by corresponding converters.
- Converter for reconverted element can converts also its children (especially in case of layered view)
Has fixes for table package table-cell-refresh-post-fixer to only re-insert paragraphs not entire table cells.

~~This PR:~~

~~Introduces 'refresh' type to differ changes (internally represented as 'x' in differ processing algorithms).~~
~~This is needed to properly handle refreshing items by the dispatcher. Assumptions:~~
1. ~~The refreshing newly inserted element should be treated as 'insert'~~
2. ~~Attribute changes on refreshed item must be preserved - allows to have atomic converters for attributes working alongside refreshing.~~
~~Introduces automatic refreshing of an item in elementToElement downcast by a new configuration option triggerBy.~~
It defines which inner events should trigger refreshing. This has limited support for now (follow-up). You can trigger reconversion of the element on attribute change or by insert/remove of a child. This process does not cover three-way-binding of a layerd view elements (follow-up, case of image -> figure>img conversion).
~~Introduces DowncastDispatcher.convertRefresh() that handles refreshing elements with~~ ~~memoization~~.
The memoization process is done by re-using existing child view elements. For instance refreshing <paragraph> will trigger 'insert' + 'attribute' conversion events for the main element and will use existing view elements for child text nodes and elements. It will also trigger proper events for newly added child elements.
~~Renames Differ.refreshItem() to Differ.reinsertItem() and will deprecate it (follow-up).~~
~~The name is needed for the new 'refresh' change handling. The reinsertItem() is still needed as triggerBy doesn't handle table case (missing three-way-binding).~~
~~Re-introduces Differ.refreshItem() that now uses 'refresh' change instead of 'remove' + 'insert' combo (reserved for Differ.reinsertItem().~~
~~The table's table-cell-refresh-post-fixer logic was changed to refresh only <paragraph> elements inside a table cell.~~
~~It uses new Differ.refreshItem() in order to make smaller changes in the view. The logic behind was also changed as I found a different approach to it.~~
~~For this matter, I've changed how <paragraph> inside <tableCell> is converted.~~
There's new converter for a <paragraph> with a 'high' priority that overrides default conversion for table case (so it can be rendered as <span> in the editing view. This converter is run on refreshItem() as well.

~~Internals of the differ has been~~ ~~slightly~~ refactored so parts of the logic of inserting an item could be re-used in refreshing mechanism. However, I did not review all the changes there and I feel that some might be removed. For instance, I've added refresh x insert|remove|attribute changes handling in the Differ._handleChange() later than some of the earlier changes.

TODO before merge:

review the code with @niegowski
- extract paragraph in table cell converter to downcast.js
- check todos
- fix artilce.js manual test
- create re-conversion manual test (current article.js)
- check if mapper.bindSlots()is needed
- refreshing table cell paragraph might cause infinite loops (Writing post-fixers is annoying #1936 (comment))
review differ / conversion changes with @scofalik after the initial review.
check if the memoization is done properly (it looks like I have this on two levels)

# Conflicts: # packages/ckeditor5-engine/src/conversion/downcastdispatcher.js

…atcher.

…sionTriggerEvent() as it is a protected method.

jodator · 2020-10-12T14:59:32Z

@scofalik I've refactored this PR. I think that it is much straightforward as it does not use differ at all to reconvert an element.

scofalik

I didn't go through it as thoroughly as the last time. AFAICS, the things that we discussed are changed and obviously, the code looks simpler. I didn't try to analyze if all methods are correctly written. From my POV, I approve this PR, though I left some suggestions in the review.

scofalik · 2020-10-12T15:25:56Z

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

+		 * @type {Map<String, String>}
+		 * @private
+		 */
+		this._reconversionTriggerEventToElementNameMapping = new Map();


German people would approve this name.

OK after a second thought I've renamed that a bit since it is used in a context anyway.

scofalik · 2020-10-12T17:34:16Z

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

+	 * @returns {Boolean}
+	 */
+	_isReconvertTriggerEvent( eventName, elementName ) {
+		return this._reconversionTriggerEventToElementNameMapping.has( eventName ) &&


Hehe, once again, I think that .has() here is not needed.

Cause if it is not there it will be undefined === elementName, should return false, right.

Yep, you're right.

Actually not - here you can have either element or text. I'll clean things up a bit here to make that clear.

scofalik · 2020-10-12T17:55:33Z

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

+	return `${ type }:${ name }`;
+}
+
+function getParentElementFromChange( entry ) {


I've seen this function used and it was expected to return falsy value and it was supposed to be a text node. But does it really happen? I think it will always return something, doesn't it?

scofalik · 2020-10-12T17:58:20Z

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

+
+function elementOrTextProxyToView( item, mapper ) {
+	if ( item.is( 'textProxy' ) ) {
+		const mappedPosition = mapper.toViewPosition( Position._createAt( item.parent, item.startOffset ) );


You can use _createBefore()

At least I think so.

scofalik · 2020-10-12T18:00:58Z

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

+	if ( item.is( 'textProxy' ) ) {
+		const mappedPosition = mapper.toViewPosition( Position._createAt( item.parent, item.startOffset ) );
+
+		return mappedPosition.parent.is( '$text' ) ? mappedPosition.parent : null;


Are you sure this will always work? I didn't analyze it but it looks suspicious. Does mapper always prefer a position inside a text node? I wonder when it returns null (cause AFAICS you expect it) and if you are ready to handle it.

scofalik · 2020-10-12T18:02:07Z

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

@@ -1349,6 +1366,21 @@ function downcastElementToElement( config ) {

 	return dispatcher => {
 		dispatcher.on( 'insert:' + config.model, insertElement( config.view ), { priority: config.converterPriority || 'normal' } );
+
+		if ( config.triggerBy ) {
+			if ( Array.isArray( config.triggerBy.attributes ) ) {


Why Array.isArray and not simply !config.triggerBy.attributes ?

Because I wanted to be sure it is an array. But we do not test for this after all so an early Exception on weird config might be better.

jodator · 2020-10-13T04:50:23Z

I didn't go through it as thoroughly as the last time. AFAICS, the things that we discussed are changed and obviously, the code looks simpler. I didn't try to analyze if all methods are correctly written. From my POV, I approve this PR, though I left some suggestions in the review.

Big thanks! Will take it from here with @niegowski :)

…rsion actions.

An early Exception will tell developer that the config provided is wrong.

packages/ckeditor5-engine/tests/manual/slotconversion.js

niegowski

Only some very minor things. Other than that now this code is much more readable than during my previous review 🙂

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

niegowski · 2020-10-13T12:46:13Z

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

+	 * Get changes without those that needs to be converted using {@link #reconvertElement} defined by a `triggerBy` configuration for
+	 * {@link module:engine/conversion/downcasthelpers~DowncastHelpers#elementToElement `elementToElement()`} conversion helper.


This description seems outdated.

Updated, dunno if clearer 😞

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

Co-authored-by: Kuba Niegowski <1232187+niegowski@users.noreply.github.com>

jodator added 30 commits August 11, 2020 12:59

PoC: Create complex re-render with slot conversion example.

0b1e50e

PoC: Document box view output in JSX-ish notation.

f467fce

PoC: Initial 'refresh' downcast action.

7d85e32

PoC: Refresh conversion - retain "slot" children.

7be4b06

PoC: Add render id to the view element itself.

9bd831f

PoC: Implement triggerBy handling.

aa55061

Merge branch 'master' into i/7956

120af23

# Conflicts: # packages/ckeditor5-engine/src/conversion/downcastdispatcher.js

Revert POC code.

a8f35ad

Fix tests.

dbc1ed6

Add base scenarios for re-converting using triggerBy.

1a4245b

Make triggerBy test example to use a more complex view.

cfe4658

Refactor tests to simple and complex scenarios.

5f77aae

Add tests for complex view structure without children.

2c95c02

Unify test cases names.

89e6afa

Add tests for slot conversion.

dff6f2f

Update test cases for slot conversion.

703ac9e

Add more cases for memoization in downcast converter.

a30c086

Fix case of re-converting main element on child add.

c2609ac

Add test for removing a slot element.

28bd1cf

Typo fix.

16d18d4

Merge branch 'master' into i/7956-reconversion-trigger-by

213cb9d

Move poc code to downcastDispatcher.convertChanges().

ffc29a6

Remove redundant slice and move it to return statements.

7e7e4b7

Inline poc code into DowncastDispatcher.convertChanges().

bd192b5

Refactor downcasthelpers tests.

9164e3c

Use re-render whole element with triggerBy in table downcast conversion.

2b9bd51

Add test cases for various slot conversion scenarios.

5da1570

Update links to issues.

93dedac

Handle child removal on parent refresh in the differ.

5b3a700

Handle attribute change on refreshed node.

9fa12db

jodator added 10 commits October 8, 2020 17:14

Add more tests for the re-conversion scenarios.

7e359d8

Fix view elements memoization.

467ebcc

Remove slot binding after changes in memoization done by DowncastDisp…

9704c97

…atcher.

Change triggerBy configuration API.

4abbf4a

Rename conversion refresh to reconversion.

39592eb

Refactor DowncastDispatcher#_mapChangesWithAutomaticReconversion().

7c85c64

Add comments to DowncastDispatcher#reconvertElement().

019066e

Merge branch 'master' into i/7956-reconversion-trigger-by

dc85a1e

Rename DowncastDispatcher#mapReconversionTriggerEvent to _mapReconver…

e8921b6

…sionTriggerEvent() as it is a protected method.

Update jsdocs of the reconvertElement methods.

2a5e54a

jodator requested a review from scofalik October 12, 2020 14:59

scofalik approved these changes Oct 12, 2020

View reviewed changes

jodator added 2 commits October 13, 2020 09:36

Update logic for ignoring text changes when mapping events to reconve…

e31c77a

…rsion actions.

Remove Array.isArray() check for configuration option.

b8c5703

An early Exception will tell developer that the config provided is wrong.

jodator requested a review from niegowski October 13, 2020 07:39

jodator commented Oct 13, 2020

View reviewed changes

packages/ckeditor5-engine/tests/manual/slotconversion.js Outdated Show resolved Hide resolved

Fix manual test.

73c2fb8

niegowski reviewed Oct 13, 2020

View reviewed changes

jodator and others added 2 commits October 13, 2020 15:40

Apply suggestions from code review

857343b

Co-authored-by: Kuba Niegowski <1232187+niegowski@users.noreply.github.com>

Update DowncastDispatcher#_mapChangesWithAutomaticReconversion() docs.

d193903

jodator requested a review from niegowski October 13, 2020 13:56

niegowski approved these changes Oct 13, 2020

View reviewed changes

niegowski merged commit a7c9973 into master Oct 13, 2020

niegowski deleted the i/7956 branch October 13, 2020 14:28

jodator changed the title ~~7956: Differ.refreshItem() + triggerBy~~ Declarative reconversion using triggerBy Oct 14, 2020

This was referenced Oct 14, 2020

Expose item reconversion for post-fixers #8252

Closed

Text node with attribute is duplicated in slot conversion demo #8203

Closed

Table downcast in one step #7580

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Declarative reconversion using triggerBy #8099

Declarative reconversion using triggerBy #8099

jodator commented Sep 17, 2020 •

edited

Loading

jodator commented Oct 12, 2020

scofalik left a comment

scofalik Oct 12, 2020

jodator Oct 13, 2020

scofalik Oct 12, 2020

scofalik Oct 12, 2020

jodator Oct 13, 2020

jodator Oct 13, 2020

scofalik Oct 12, 2020

scofalik Oct 12, 2020

scofalik Oct 12, 2020

scofalik Oct 12, 2020

scofalik Oct 12, 2020

jodator Oct 13, 2020

jodator commented Oct 13, 2020

niegowski left a comment

niegowski Oct 13, 2020

jodator Oct 13, 2020

		* Get changes without those that needs to be converted using {@link #reconvertElement} defined by a `triggerBy` configuration for
		* {@link module:engine/conversion/downcasthelpers~DowncastHelpers#elementToElement `elementToElement()`} conversion helper.

Declarative reconversion using triggerBy #8099

Declarative reconversion using triggerBy #8099

Conversation

jodator commented Sep 17, 2020 • edited Loading

Suggested merge commit message (convention)

Additional information

jodator commented Oct 12, 2020

scofalik left a 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

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

jodator commented Oct 13, 2020

niegowski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jodator commented Sep 17, 2020 •

edited

Loading