Executing command inside enqueueChanges() leads to confusing behavior

[Reinmar]

Let's image:

enqeueuChange( () => {
  sel.setRanges( someRanges );
  editor.execute( 'bold' );
  sel.collapse( somewhere );
} );

You expect that someRanges will be bolded and selection will be restored in somewhere, right?

Wrong!

This code will be executed in this order:

1. set ranges,
2. enqueue bold's callback,
3. collapse selection,
4. bold's callback is executed... on a collpased selection.

The solution here is to use 2 enqueueChanges() blocks for selection changes and leave bolding unwrapped (cause it executes its own enqueueChanges()). Unfortunately, this is super confusing ;/

[scofalik]

This is confusing because enqueueChanges works a bit like setTimeout. It would be perfectly clear how this code will be executed:

setTimeout( () => {
    selection.setRanges( [ range ] );

    setTimeout( () => {
        document.execute( 'bold' );
    }, 1 );

    selection.collapse( range.start );
}, 1 );

Unfortunately:

1. We forget sometimes how enqueueChanges work. If we do, I suspect that users will forget it too.
2. enqueueChanges are hidden in different places.

It's nice that enqueueChanges is already used in the command because user don't have to write unneeded code... but this leads to situations like this.

BTW. remember that there is a ticket somewhere to make all commands create enqueueChanges blocks automatically (so it won't be even declared in _doExecute).

[Reinmar]

I've again tried to do this:

                editor.document.selection.setRanges( ranges );
                editor.execute( commandOrCallback, { batch, forceValue: true } );
                editor.document.selection.setTo( initialSelection );

Just to understand that it's wrong.

So I changed it to:

                editor.document.enqueueChanges( () => {
                    editor.document.selection.setRanges( ranges );
                } );

                editor.execute( commandOrCallback, { batch, forceValue: true } );

                editor.document.enqueueChanges( () => {
                    editor.document.selection.setTo( initialSelection );
                } );

To again understand that this is wrong because it will render the content 3 times, while I want the selection changes to stay virtual.

So, the solution seems to be:

		editor.document.enqueueChanges( () => {
			editor.document.selection.setRanges( ranges );

			editor.execute( commandOrCallback, { batch, forceValue: true } );

			editor.document.enqueueChanges( () => {
				editor.document.selection.setTo( initialSelection );
				initialSelection.destroy();
			} );
		} );

Which even makes sense if you think about it but it's far from straightforward.

[scofalik]

This is faaaar from straightforward and I don't see how it makes more sense than other things that you've tried (even after thinking about it). We should research how we can make nqChanges better.

BTW. nqChanges in nqChanges is very confusing and leads to flow problems too. This is was one of the reasons why we did a refactor in model.Selection so it won't use nqChanges inside model.Selection methods.

[Reinmar]

I know that it's bad. But it makes some sense that in order to prevent rendering between enqChanges you wrap them in another one.

Unfortunately, how the rest of the code works (with that one more enqChanges inside) is just terrible. I totally agree.

Perhaps we should just give up this idea and remove this function completely. We need to understand whether we have any other option to ensure immutability during one's algorithm execution or not.

[pjasiun]

I was thinking about it for the while. The problem is not the enqChanges block, we can easily get rid of it. Then problem is to tell in the proper moment that changes are done.

We need to have an event which tells that all changes in the model are done and the rendering can start.

This event should be fired when the user calls any API method, like editor.execute( 'command' );.

At the same time, we want to be able to do something after such method, in an intuitive way, but have the "changes done" event postponed.

We could have simple fire( 'changesDone' ); at the end of editor.execute( 'command' ); and stop using enqChanges block. But, unfortunately, it would not make this code nicer, the last line need to be in the changesDone listener.

At the same time, there was an idea that new changes should go after all already enqueued change. Despite that, I sill can find cases for it (postfixers), these are edge cases. The default behavior should be to postpone only the changeDone event and execute in the normal order. In means that this code will be executed in the expected order:

editor.document.changes( () => { //these changes are not enqueued anymore
	editor.document.selection.setRanges( ranges );

	editor.execute( commandOrCallback, { batch, forceValue: true } );

	editor.document.selection.setTo( initialSelection );
	initialSelection.destroy();
} );

function foo() {
	editor.document.changes( () => {
		console.log( 'foo' );
	} );
}

function bar() {
	editor.document.changes( () => {
		console.log( 'bar1' );
		foo();
		console.log( 'bar2');
	} );
}

editor.on( 'changesDone', () => console.log( 'changesDone' );

foo();
// foo
// changesDone

bar();
// bar1
// foo
// bar2
// changesDone

At the same time it should be possible to enqueue changes:

editor.document.changes( () => {
	console.log( 'bar1' );

	editor.document.changes( () => console.log( 'postfixer' ), { enqueue: true } );

	console.log( 'bar2');
} );

// bar1
// bar2
// postfixer
// changesDone

In fact, it could be done by the listener in the changesDone event, but then we will have the problem back when you will try to call in that postfixer function which has changes block.

TL;DR: enqueueChanges() should be changed to changes() and execute changes immediately instead of enqueuing them.

[Reinmar]

TBH, it was hard to read the code now (I didn't notice the enqueue param) which means that it may be equally hard to read them in the future.

But more importantly, if we're talking about making changes() not enqueuing by default it existence stops being justified. Using a callback is always harder than e.g. calling some document.done() method or even calling const transaction = document.start() and later transaction.commit().

I'd look for a solution which is plain simple for the 99% of cases and just think on how to implement the remaining 1% and what are its requirements (what use cases precisely we want to support). For example, I can easily imagine that one could simply block changesDone for a bit or just listen on this event to perform more changes (postfixers), etc. So, let's just clarify what we need.

[scofalik]

Please remember, that the first and most important problem that enqueueChanges is solving, was to prevent listener/fixer to step into currently resolved changes block. So, if changes() or enqueueChanges() without parameter does not postpone the callback it is useless. Well maybe it is not useless, because it leads to firing changesDone event... But I am afraid that if we have two ways to "enqueue changes" it may be confusing.

[Reinmar]

On the occasion of #506 I've been thinking about the API a bit and perhaps I'm completely wrong here but I want to write this down just in case I forget.

We introduced enqueueChanges() the way we did (so it enqs at the end) so to secure your code from other code which may break it in the meantime (by listening to #change). However, there's very little code which actually listen to #change (and in fact, we prefer #changesDone cause it's safer ??).

Anyway, we see right now that it doesn't make any sense to complicate >90% of the code just to solve some edgy cases. So we agree that whatever API we'll come up with should execute the code linearly.

However, we may still consider securing us a bit. Any code which does changes will create some kind of transaction (either as .changes( () => ... ) or by .start(); ... .commit();) and we don't expect other code to make changes during that. So what do you think to log warnings if there's another transaction started during yours?

At the same time, you may expect this to happen (e.g. when executing one command inside another) and then you can explicitly tell that by e.g. start( { allowIntermediateChanges: true } );.

Since we'll be doing post-fixers using the #changesDone event (I've just discussed it briefly with @scofalik and it may be doable to do this if we'll ensure that additional changes are included in the currently fired event) then this situation with intermediate changes will only apply to those explicit cases where you execute one command within another. So perhaps we don't even need that allowIntermediateChanges option.

[pjasiun]

Please remember, that the first and most important problem that enqueueChanges is solving, was to prevent listener/fixer to step into currently resolved changes block. So, if changes() or enqueueChanges() without parameter does not postpone the callback it is useless.

No, it's not. The main task is to tell when all changes are done, so, for instance, render can be called.

[scofalik]

No, it's not. The main task is to tell when all changes are done, so, for instance, render can be called.

We might use something different for this (something simpler, one method to trigger renderer) so it is not the most important reason, but I already fixed my answer :).

[scofalik]

However, there's very little code which actually listen to #change (and in fact, we prefer #changesDone cause it's safer ??).

This is not true at the moment. In fact, it is completely opposite. changesDone brings very little information so we are unable to do any fixing on it.

At the same time, you may expect this to happen (e.g. when executing one command inside another) and then you can explicitly tell that by e.g. start( { allowIntermediateChanges: true } );

I don't like additional flags, etc. Let's make it simple, shall we?

[Reinmar]

This is not true at the moment. In fact, it is completely opposite. changesDone brings very little information so we are unable to do any fixing on it.

I know it's not true ATM, but it may be true one day, right? :)

I don't like additional flags, etc. Let's make it simple, shall we?

Yeah, that what my post lead to – we don't need that flag if we do all postfixing on changesDone. And this flag won't actully make any sense if we will keep postfixing on change because you never know when it may happen.

[pjasiun]

This topic is bigger, but I wanted to leave here one note. I agree that we can replace synchronous .changes( () => ... ) by pair of methods .start(); and .commit();. However, I still prefer the first way, because it forces developers to keep start and end of the "transaction" in the same block. If they will be separate functions, I'm sure that at some point in some blocks we will move .start(); or .commit(); to a different function which will be called from multiple places or to a condition. With .changes( () => ... ) it won't be possible to use it in the dangerous way.

[pjasiun]

Reading https://github.com/ckeditor/ckeditor5-engine/issues/1148 I had a thought that instead of .changes( () => ... ) maybe it should be called .render( () => ... ). Rendering is pretty much the only reason why you need this wrapping.

However, I'm not sure, if it's not too abstract to make changes in the "render" callback.

Also, I'm not sure on what level it should be then. changes are called on the model, and there is no "rendering" in the model. We can assume that firing changes is kind of rendering but it's a slippery explanation.

[Reinmar]

However, I'm not sure, if it's not too abstract to make changes in the "render" callback.

It is ;D

Also, I'm not sure on what level it should be then. changes are called on the model, and there is no "rendering" in the model. We can assume that firing changes is kind of rendering but it's a slippery explanation.

Let's not overthink this. You do/apply changes to the model. That's it. No firing, no rendering. The only other terms which came to my mind and which fit here are those related to transactions (such as "commit").

[pjasiun]

Let's not overthink this. You do/apply changes to the model.

Well, not exactly. Reading https://github.com/ckeditor/ckeditor5-engine/issues/1148#issuecomment-332213851 I realized that this should not be limited to the model. Changes in the view should also be enqueued. .changes should be part of the editing controller, not the model.

[Reinmar]

Changes in the view should also be enqueued. .changes should be part of the editing controller, not the model.

Still, we should not be talking about rendering or firing stuff. I meant only that. Note that terms such as committing or applying changes are fine in both the model and the view.

[pjasiun]

Fixed via https://github.com/ckeditor/ckeditor5-engine/issues/1186.