Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mark model changes which do not affect data #4348

Closed
pjasiun opened this issue May 21, 2018 · 6 comments · Fixed by ckeditor/ckeditor5-engine#1421
Closed

Mark model changes which do not affect data #4348

pjasiun opened this issue May 21, 2018 · 6 comments · Fixed by ckeditor/ckeditor5-engine#1421
Assignees
Labels
package:engine type:improvement This issue reports a possible enhancement of an existing feature.
Milestone

Comments

@pjasiun
Copy link

pjasiun commented May 21, 2018

There are model changes which do not affect data: selection change, some markers. At the moment, there is no easy way to learn if the change touches data what makes integrations much harder.

First, we should introduce marker API to tell if that marker change touches data. I propose to call it affectsData (true by default). It should be a property in options parameter in addMarker and updateMarker. If the value is set it should be the same for all further operations, as long as it is not changed.

Then, we need to have an easy way to get only these change events which affect data. I propose to introduce change:data event. It will be called for all changes which affect data. If the change will contain only selection change, or markers which do not affect data, then change event should be fired. In all other cases, when data can change change:data should be fired.

@pjasiun
Copy link
Author

pjasiun commented May 21, 2018

This is "Solution 2" from https://github.com/ckeditor/ckeditor5-autosave/issues/1.

@scofalik
Copy link
Contributor

I thought that change:data is not backward compatible but it actually is. I am leaving this as a note if others would wonder.

I wanted to argue that I like a flag in Differ or evt more, but actually I don't care that much :). It ca be change:data too.

@ma2ciek
Copy link
Contributor

ma2ciek commented May 22, 2018

While adding affectsData to markers is easy and backward compatible, I'm a bit stuck providing the change:data event.

Should I add the affectsData property to the MarkerOperation too? I'm thinking about listening to the model#_change event (or document#change event with the high priority) and checking whether at least one operation from writer->batch->deltas->operations changes the data, so I can emit the event.

@pjasiun
Copy link
Author

pjasiun commented May 22, 2018

Should I add the affectsData property to the MarkerOperation too?

Yep. It seems to be the way to go.

@ma2ciek
Copy link
Contributor

ma2ciek commented May 29, 2018

Am I right and it only makes sense to fire data:change event on markers changes for markers that are managed using operations?

@ma2ciek
Copy link
Contributor

ma2ciek commented May 30, 2018

Answering my own question, no, I'm not. A marker that exists only in the model.markers can still have a dataDowncast converter as @scofalik pointed it out.

pjasiun referenced this issue in ckeditor/ckeditor5-engine Jun 13, 2018
Feature: Introduced `ModelDocument#change:data` event. Closes #1418.
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-engine Oct 9, 2019
@mlewand mlewand added this to the iteration 18 milestone Oct 9, 2019
@mlewand mlewand added module:model type:improvement This issue reports a possible enhancement of an existing feature. package:engine labels Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:engine type:improvement This issue reports a possible enhancement of an existing feature.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants