How to handle undo and selection context change in features

[szymonkups]

Some features (heading for example) are listening on model's selection change events to update command's state (and UI). What would be the best way to handle such situation:

• Selection is placed inside paragraph element. UI is updated to show that current block is paragraph:

<paragraph>foo{}bar</paragraph>

• heading command is executed, block type is changed, UI is updated:

<heading1>foo{}bar</heading1>

• undo command is executed, heading1 is converted back to paragraph.

This is fine but there is no selection change event (because there is no selection change, it's more a selection context change), so how command can be notified about this change? Should we listen also to document change event?

[fredck]

What about selection change being fired when the parent tree of the selection has changes?

[scofalik]

This is interesting question.

For sure, listening to change - or maybe - changesDone event is fine. The question is whether we can do it more efficiently. I am afraid it would be hard.

@fredck - this won't work. RenameOperation is done exactly in a way so that it won't do any moving/removing/inserting in the model. The only thing we can do is maybe do something extra when RenameOperation is executed. Like, fire change on all LiveRanges that start/end in renamed node. This looks like a lot of work of unknown gain.

[Reinmar]

What we have right now are low-level selection events: selection#change:range and selection#change:attribute. Let them work like they do now – they notify you whether some selection property has changed. They may be a bit confusing, but I don't think we can change them.

Now, feature (and developers) need some nice API to handle their typical tasks – refreshing when context has changed and when attributes have changed, but including here data changes (which should include initial data load). Let's have a more useful set of Document events for these tasks. Thanks to them developers will be able to quickly and easily listen to what they want and we'll be able to hide some special cases (e.g. we can even fire Document#selectionChange on data load manually in DataController – though, that would be a bit too ugly :D).

[Reinmar]

Note: Thanks to having these events exposed in one place, we'll have just one listener to e.g. changesDone and this listener can e.g. cache the previous selection context. This will be far more performant than doing such checks in every feature. And far nicer API. And this is how we did that in CKE4 :).

[Reinmar]

First thing to change after we introduce a new event: ckeditor/ckeditor5-heading#50

[scofalik]

I am not sure we need a new event for this. When would it be fired? What it will solve?

[Reinmar]

It would be fired when anything about the selection has changed – its context or attributes.

Note that the context may change as a result of content changes while selection's ranges stay untouched.

Rationale – potential performance issues. We're now refreshing all the commands on every single #changesDone which may be fired quite frequently, especially in collab editing cases. A more contextual event would be better here.

This means that this is not a super high priority ticket. However, it needs to be considered by every feature which needs to act on such a change. It can be added later (we'll just change the event) but please keep in mind that this won't be a backward compatible change. By switching from changesDone to selectionChange we may break features which really wanted to listen to the former event (but this assumes that we added the automatic way of plugging command's refresh on change).

Regarding performance – this doesn't seem like a super heavy scenario, but I've seen how CKE4 is affected by too many checks done on "change". The problem here may be that if the content changes the browser needs to re-render the view, so it's pretty overloaded already if the content is long. Adding dozen of checks don't help.

[Reinmar]

We again stumbled upon this. And by the feedback I got from @oleq I can see that it is confusing, but he got it right by himself.

The issue was: https://github.com/ckeditor/ckeditor5-core/issues/50. Intuitively, someone (IIRC @scofalik, who, nota bene, knows the engine extremely well) used change:attribute event to refresh the ToggleAttributeCommand. It was the intuitive way of implementing it and more of us made the same mistake in other commands by relying on change:range.

Unfortunately, just like change:range doesn't consider that a range's paths may not change but the content may, the change:attribute doesn't consider that a content may change (context of the selection) while the attributes are still the same.

In all cases, these were our mistakes. However, we made them. So the question is whether we shouldn't have a better, safer event and warnings in these events' docs that they are limited.

[Reinmar]

I confirmed the issue because we need to do something. The minimum is adding some warnings in the docs.

[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).