Problems with updating view.Selection and using view.Document#render

[scofalik]

From time to time we have a problem with view.Selection blowing up because of some unfortunate events.

It happened last time, when view selection has not been updated and view.Selection#editableElement was null when it wasn't expected. It made editor crash. Now we have another problem that I described in https://github.com/ckeditor/ckeditor5-upload/issues/55.

I see two ways to go with this.

1. Do not call view.render(). Ever. It is dangerous. You should accept that it is called on changesDone and if you want to re-render view, use model.Document#enqueueChanges.
2. Convert model selection to view selection after each model change (or each model.DocumentSelection change).

First solution feels more like a hack than a solution. We know that rendering happens on changesDone so we use enqueueChanges only to make sure that the view will be re-rendered. On the other hand, we could argue that we use enqueueChanges to make sure that the view will be re-rendered at the appropriate moment. I could buy this. Another awkward thing is that we will need to use model.Document#enqueueChanges to wrap things that are not connected with model at all, just like in ImageUploadProgress:

loader.on( 'change:uploadedPercent', ( evt, name, value ) => {
	progressBar.setStyle( 'width', value + '%' );
	editor.editing.view.render();
} );

In this case, uploadedPercent is a property of an object which is connected with upload mechanisms and it has nothing to do with model. The solution to the linked issue would look like this:

loader.on( 'change:uploadedPercent', ( evt, name, value ) => {
	editor.document.enqueueChanges( () => {
		progressBar.setStyle( 'width', value + '%' );
	} );
} );

However, once again, I could argue, that we should wait with any actions until all other changes that are processed are over. Hell, I could even argue that maybe enqueueChanges should not be model.Document thing but a core.Editor thing? Maybe it should be something that control execution flow not only in model but in the whole editor? 🤔

Second solution at first glance looks more reasonable. The outdated view.Selection was the problem from the beginning, so let's fix the problem and let's not have an outdated view.Selection. I just wonder why we haven't done it at the first place? Maybe there were some bugs or conceptual problems that I don't remember? Or maybe we thought it will be more efficient and we don't have to update view.Selection that often?

Discuss!

[pjasiun]

Do not call view.render(). Ever. It is dangerous. You should accept that it is called on changesDone and if you want to re-render view, use model.Document#enqueueChanges.

👍

So the problem appears when you are after some document change events, but before changesDone, and wants to render, for instance, because of some change in the view. Apparently, it means that the enqueueChanges block should not be limited to the model. We need to ensure that render is called after all applied changes (in the model and in the view).

[Reinmar]

Can we close this now?

[pjasiun]

Yep.