`changesDone` we miss you <3

[scofalik]

After recent refactor we rewrote events and changes enqueuing mechanisms. In the end, we got rid of model.Document#event:changesDone in favor of model.Document#event:change.

The new change event was something that was meant to replace changesDone. The thing is that change event is not fired after all registered change blocks are done. It is fired after each change block for given batch is done.

This means that below code:

modelDoc.on( 'change', () => {
    console.log( 'foo' );
} );

model.change( writer => {
    // ... do something that changes the model tree ...

    model.enqueueChange( writer => {
        // ... do something that changes the model tree ...
    } );

    model.enqueueChange( writer => {
        // ... do something that changes the model tree ...
    } );
} );

Will output foo three times. The change event will be fired three times, even though all the changes are, in fact, done in one scope. They are not synchronous (enqueueChange block changes are postponed) but still those changes will be done before we leave the outermost .change() block.

This feels wrong.

It feels wrong because you have callbacks listening to change event. You want to react to those changes, but there are more changes already enqueued and waiting to be executed. Maybe changes from the first change event are somehow affected by the changes from enqueueChange blocks? We end up in a situation that we don't really react to changes when they are done, we react to them somewhere in the middle. Not immediately, like it was earlier, but we have no guarantee that we operate on a final model tree state.

All in all, current situation resulted in a bug described here: https://github.com/ckeditor/ckeditor5-autoformat/issues/56. Since there were multiple change events, there were multiple change batches send to a remote client and Autoformat behaved incorrectly.

I think we should have a discussion how we can make this better. This is not a simple subject, though. Maybe we'll come to a conclusion that we don't want to change anything.

Some questions for starters:

1. Do we keep current change event? How do we use it so we won't break anything?
2. What information we want and need in changesDone? Do we need to refer to a Batch instance in changesDone?
3. What about post-fixers? We wanted them to add their changes to the batch that caused post-fixing, so the post-fixer changes are transparent. How do we handle those? Post-fixers have the same problem - they are fired in-between of changes. But how do we make them work otherwise since they need their batches?
4. Do we need to keep info about batches together with changes in Differ? Can we solve this problem knowing, that the changes are merged together?

[Reinmar]

They are not synchronous (enqueueChange block changes are postponed) but still those changes will be done before we leave the outermost .change() block.

Just to make it clear – they are synchronous but enqueueChange() changes the execution order so these 2 blocks are executed after the change()'s callback is finished. Am I right?

[Reinmar]

I've read https://github.com/ckeditor/ckeditor5-autoformat/issues/56 and, TBH, I don't understand how changesDone would work in that case. Collaborative changes may be intermixed with other changes and how would changesDone be fired for them? I guess... it shouldn't.

So, isn't the point here that the change event is ok, but basically... you need to check the type of the batch? Maybe we need something like change:<type=transparent/not>, so there's a general change event, but you can also listen to specific types easily.

Anyway, let's discuss this.

[scofalik]

Just to make it clear – they are synchronous but enqueueChange() changes the execution order so these 2 blocks are executed after the change()'s callback is finished. Am I right?

Yes, exactly.

I don't understand how changesDone would work in that case.

Assume that we are doing bold by double asterisk sign. If we get both changes: original change and autoformat change together, the Differ would keep information that:

1. Remove two "**" at the beginning.
2. Remove "*" at the end.
3. Bold the text in between.

So, in Differ, the information would already the fixing change, so Autoformat feature won't react to adding second "*" because there would be no information about it in the Differ on changesDone (if both batches are sent together). Did I make it more clear?

[pjasiun]

The question "what should happen if you want to do some changes in the change block" is very, very tricky. I spend a good time on it in the model, and then we spen with @szymonkups a good time solving this again in the view.

What I am sure, is that if I enqueue change in the change listener it is that late that the whole process should start from the beginning and fire change event again.

I also hate the fact that now, we need to check the type in the batch in the autoformat feature and that we will have to do so it all features like this in the future. We may need a mechanism, simmiar to the postfixer, but for changes which creates another batch. These callbacks should be execute after the conversion is done... or maybe @Reinmar is right, and we will not be able to hide this logic.

What I know for sure is that we need to bring is more assertions in this code. For instance we should throw if one use change block (instead on the enqueueChange) in the change listener. It is very easy to make a bug, which will be very hard to debug later.

[pjasiun]

So, in Differ, the information would already the fixing change, so Autoformat feature won't react to adding second "*" because there would be no information about it in the Differ on changesDone (if both batches are sent together). Did I make it more clear?

I think that if differ will start keeping information from multiple batches we will bring more issues than we will solve.

[scofalik]

Another solution could be to throttle sending deltas to a remote client. But this is a solution that is strictly connected to collaboration, a bit offtopic here.

[CKEditorBot]

There has been no activity on this issue for the past year. We've marked it as stale and will close it in 30 days. We understand it may be relevant, so if you're interested in the solution, leave a comment or reaction under this issue.

[CKEditorBot]

We've closed your issue due to inactivity over the last year. We understand that the issue may still be relevant. If so, feel free to open a new one (and link this issue to it).