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

Markers removal #4233

Closed
pjasiun opened this issue Jan 8, 2018 · 2 comments · Fixed by ckeditor/ckeditor5-engine#1347
Closed

Markers removal #4233

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

Comments

@pjasiun
Copy link

pjasiun commented Jan 8, 2018

Markers removal is now implemented similarly to the attribute removal: based on marker name/descriptor converters decide what kind of elements should be removed and unwraps similar elements.

The problem is that markers do not work like attributes. For attributes it is not important which instance of an element should be removed: if <b> suppose to be removed from foo it does not matter which <b> is removed as long as foo is unbolded. If a comment or users selection should be removed it is very important to remove the proper element.

Also, markers values change based on data stored outside the document model much more often than attributes do. In fact, this is what markers were created for: to show some external data on the document (like comments or users selection). The problem is that it means that markers descriptors can change asynchronously. For instance, if a comment is selected marker gets a different color. Because of it, the marker to remove is not the same as the marker which was added. It leads to many bugs and hacks.

The third problem is the current way of conversation, introduced together with differ. Because now conversion is delayed it is very hard to find the range of the makers in the view, when model changed.

This is why I think that markers should be removed in a different way. When elements are added to the view they should be stored and exactly these elements should be removed. The only information sent to the converters should be the name of the marker to be removed because the descriptor and range can be invalid at the moment of removal.

After this short introduction, let's move to details.

There are 2 mechanisms to be changed to implement it.

First is view writer. View writer is designed to work with attributes. It splits attribute elements with no information about their connection. The only way to unwrap is by giving range and element template - all similar elements are removed, what is fine for attributes but not for markers. Whenever view writer split view element it should store information about this elements parts. Then it should be possible to unwrap certain element (and all its parts) using a reference to the element instance, not by using range and template. The proper set of view writer methods should be provided.

Second, is the way how converters work. Markers converters should store information what element has been added to the view and what element should be removed. Then it should be enough to say which marker should be removed, based on its name. Range and descriptor should not be needed when a marker is removed.

@scofalik
Copy link
Contributor

scofalik commented Jan 9, 2018

Short note: maybe we could remove HighlightAttributeElement. As far as we remember, it was introduced because of problems with unwrapping attribute elements.

@pjasiun
Copy link
Author

pjasiun commented Feb 19, 2018

See https://github.com/ckeditor/ckeditor5-engine/issues/1303. HighlightAttributeElement need to be removed anyway, because after recent change in the view, subclassing view elements is not allowed.

pjasiun referenced this issue in ckeditor/ckeditor5-engine Mar 16, 2018
Other: Refactored how markers removal is converted from the model to the view. Closes #1226.
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-engine Oct 9, 2019
@mlewand mlewand added this to the iteration 15 milestone Oct 9, 2019
@mlewand mlewand added module:conversion 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
3 participants