Expose item reconversion for post-fixers

[jodator]

📝 Provide a description of the improvement

After #6510 there's no programatic way to trigger element reconversion. The reconversion can only be started using triggerBy option.

Questions to answer:

• where to put the API. A similar thing is available as Differ.refreshItem() (does insert + remove) but I think this is not a good place.

Additionally, use this API in table cell refresh post-fixer instead of Differ.refreshItem()

---

If you'd like to see this improvement implemented, add a 👍 reaction to this post.

[jodator]

Programmatic reconversion

Technical Story: ckeditor5#8252

Context and Problem Statement

The Declarative reconversion using triggerB PR handled reconversion using triggerBy configuration option for elementToElement() downcast conversion helper. The reconversion mechanism could also be used as a way for certain post-fixers to require model-to-view reconversion.

The go-to mechanism for reconversion should still be the triggerBy configuration. However, for some cases (like refreshing <paragraph> inside <tableCell>) a declarative configuration is hard to achieve.

Decision Drivers

• Simple, drop-in API for Differ.refreshItem() in existing features.
• Transparent for other features.
• Easy to document.

Considered Options

• Handle reconversion by Differ.refreshItem().
• Expose reconversion request on the writer.
• Expose reconversion as a callback for triggerBy configuration option.
• Introduce postFixerApi object.
• ... any other place: model, dispatcher, document

Pros and Cons of the Options

Handle reconversion by Differ.refreshItem().

This approach assumes rewriting Differ.refreshItem() so it can mark the item to be reconverted by the DowncastDispatcher.

document.registerPostFixer( writer => {
	for ( const change of document.differ.getChanges() ) {
		if ( requiresReconversion( change ) ) {
			document.differ.refreshItem( change.item );
		}
	}
} )

• Good, because the API is already in use.
• Good, because the recommended way of working with post-fixers already assumes access to the Differ.
• Bad, because conversion is not a concern of the Differ.
• Bad, because it was already disproven while working on declarative reconversion API PR.

Expose reconversion request on the writer.

Post-fixer callback already receives the writer instance.

document.registerPostFixer( writer => {
	for ( const change of document.differ.getChanges() ) {
		if ( requiresReconversion( change ) ) {
			writer.markForReconversion( change.item );
		}
	}
} )

• Good, because a post-fixer callback already receives the writer instance.
• Bad, because the DowncastDispatcher has no access to the writer instance.
• Bad, because API method is cumbersome.
• Bad, because it would probably require passing additional argument or option to the writer.

Expose reconversion as a callback for triggerBy configuration option.

editor.conversion.for( 'downcast' ).elementToElement( {
	model: 'complex',
	view: convertComplexView,
	triggerBy: {
		callback(  ) { // <- not sure what to pass here but it should be called for every change inside "complex" model element.
			return true;
		}
	}
} )

• Good, because gives greater flexibility for triggering reconversion.
• Good, because reconversion trigger is near the conversion definition.
• Bad, because the API is not clear to me (what to pass and when).
• Bad, because it might not solve all cases anyway - for instance, a change of siblings (not its children) may trigger <complex> element reconversion.

Introduce postFixerApi object

Similar approach as converstionApi parameter used for conversion callbacks.

document.registerPostFixer( postFixerApi => {
	const { writer, differ, reconvertItem } = postFixerApi;

	for ( const change of differ.getChanges() ) {
		if ( requiresReconversion( change ) ) {
			reconvertItem( change.item );
		}
	}
} )

• Good, because post-fixer callback will receive all needed tools at once.
• Good, because simpler to use by end-developers.
• Good, because easy to document.
• Bad, because breaking change.
• Bad, because still requires entangling downcast dispatcher and document.

... any other place: model, dispatcher, document

Exposing a method on any of the above has similar drawbacks and usage

document.registerPostFixer( writer => {
	for ( const change of document.differ.getChanges() ) {
		if ( requiresReconversion( change ) ) {
			document.reconvertItem( change.item ); // ?? weird
			editor.editing.reconvertItem( change.item ); // maybe
			editor.conversion.reconvertModelElement( change.item ); // maybe
			editor.editing.downcastDispatcher.reconvertItem( change.item ); // maybe but unclear side effects
		}
	}
} )

• Good, because if exposed on "related" object (ie conversion, downcast dispatcher) might be easier to find in the docs.
• Bad, because of possible side-effects of using this API outside model.change() or post-fixing process.
• Bad, because the instance of the new method holder must be passed to the post-fixer callback.

[jodator]

During the F2F talk with @Reinmar, we identified that the most desired option would be an option that adds callback() to the triggerBy definition as it could gather code for element conversion in one place.

However, this will not handle the case of paragraph reconversion. This is because how the paragraph is rendered is determined not only by the paragraph's attributes but as well on the number of siblings. This would be very tricky to handle elegantly as it would require to:

• Expose differ to the callback so it could check ALL changes as adding a sibling to a paragraph should trigger reconversion.
• It would have to return elements to be reconverted.

The other approach is to use event + reasonable method.

We can listen to document's change event with proper priority (after model post-fixers but before conversion) to mark which element needs to be reconverted.

The best place for a new method is  editor.editing.downcastDispatcher.reconvertItem( modelElement );.

The next thing to remember is to align this to our _way of doing things_™ we could wrap all this logic into a downcastDispatcher.registerSomeCallback() so the developer could mark elements to reconvert based on differ changes.

The rationale for this is that model post-fixers (an approach used in tables for reconverting model to view) should, well, fix the model.

[Reinmar]

During the F2F talk with @Reinmar, we identified that the most desired option would be an option that adds callback() to the triggerBy definition as it could gather code for element conversion in one place.

However, this will not handle the case of paragraph reconversion. This is because how the paragraph is rendered is determined not only by the paragraph's attributes but as well on the number of siblings. This would be very tricky to handle elegantly as it would require to:

To put this a bit differently – using such a callback in elementToElement() converter would be against SRP and common sense. We'd need to get rid of the model property as the callback would actually control what needs to be converted. That'd not be an element-to-element conversion any more but rather "detect something and then reconvert some things". 

The other approach is to use event + reasonable method.

👍

We can listen to document's change event with proper priority (after model post-fixers but before conversion) to mark which element needs to be reconverted.

If plugging this code at the right stage (it should be, IMO, after post fixers but right before we start conversion) would require using a priority, I think we should have additional addConversionCallback() or similar method so all these callbacks are executed just before the conversion starts. In the end, I treat them as part of the conversion process so they should not be executed somewhere together with post-fixers or in any other uncontrollable moment.

So, while I wasn't sure during our call whether we need a new method, I'd say now that we should have it. To make sure those callbacks are called exactly when needed.

The rationale for this is that model post-fixers (an approach used in tables for reconverting model to view) should, well, fix the model.

👍

[Reinmar]

PS. We will also need to have more useful methods in the Differ to simplify the code detecting whether something needs to happen. This will benefit both – these "preconverters" and "postfixers".

BTW, I like the term "preconverter" :D addPreConverter() would be acceptable to me :D

[niegowski]

DUP of #10360