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: data convertors should create simpler structures #3961

Closed
fredck opened this issue Jan 27, 2017 · 8 comments · Fixed by ckeditor/ckeditor5-engine#845
Closed

Markers: data convertors should create simpler structures #3961

fredck opened this issue Jan 27, 2017 · 8 comments · Fixed by ckeditor/ckeditor5-engine#845
Assignees
Labels
package:engine type:feature This issue reports a feature request (an idea for a new functionality or a missing option).
Milestone

Comments

@fredck
Copy link
Contributor

fredck commented Jan 27, 2017

Markers should be converted differently in the data pipeline.

Right now, they're converted in a way that satisfies the view pipeline purpose by creating spans with content inside. For example, if a marker spans across several paragraphs, this is a (simplified) output of the DOM:

<p><span marker1>...</span></p>
<p><span marker1>...</span></p>
<p><span marker1>...</span></p>

When it comes to the data pipeline instead, outputting markers like above is semantically wrong and senseless as it unnecessarily bloats up the data.

Markers are simply ranges within the document with no visual representation (which can be added on top of markers). Therefore it's data representation should be much simpler, just describing the start and end boundaries of the range.

For example, for the above case, the following data output makes much more sense:

<p><span marker1-start />...</p>
<p>...</p>
<p>...<span marker1-end /></p>
@Reinmar
Copy link
Member

Reinmar commented Jan 27, 2017

Are markers a single range always?

Asking because I'm curious what if I'd like to e.g. comment on an entire table column? If that case was somehow considered in markers, then the output would need to consider that too.

@pjasiun
Copy link

pjasiun commented Jan 27, 2017

Are markers a single range always?

Yep. That's the point. At least for now.

@scofalik
Copy link
Contributor

scofalik commented Jan 27, 2017

Interesting topic, however I wonder how we should approach it. At the moment we haven't touched view-to-model-marker conversion at all. We assumed that data about markers is kept somewhere and will be used to init some things in editor after "real" data is loaded. This made sense, at least for some time.

The thing is, you will not always want to have the data output you proposed. This depends on a feature.

  • Sometimes markers are just "editing helpers" and are important only during editing,
  • sometimes markers may be acutally something that you would like to keep in content (because it makes important visible changes in content) - unless we consider this as wrong markers usage.

To be perfectly honest, proposed format in data pipeline is semantically wrong too. If something is just for editing purposes, it should not be kept in data. These are empty, invisible spans, something we have in CKE4 and what we want to avoid in output data. But if we don't want them in the output data, we would need a separate way to be able to save them.

So, the question is whether we are okay with a'la CKE4 bookmarks? AFAIR we were trying to make as clean data output as possible.

@scofalik
Copy link
Contributor

Maybe we could store markers in a comment (invisible part of data format). For example, for HTML:

<p>Foo bar.</p>
<!-- ckeditor5-markers: {<markerName>,<rootName>,<startPath>,<endPath>}, {...}, ... !-->

But then how we solve it for markdown?

@pjasiun
Copy link

pjasiun commented Jan 27, 2017

I like the idea, but I'm not sure how generic it is. I mean that every feature which uses markers may want to serialize them in the different way (or do not serialize them at all). I'm not sure if we should implement a generic solution at this stage or focus on cases we have and make them generic later, if needed.

@Reinmar
Copy link
Member

Reinmar commented Jan 27, 2017

Maybe we could store markers in a comment (invisible part of data format).

It doesn't need to be a comment. It could be a custom element, but rather not a <span> because sometimes you may need to locate it in places where spans are not allowed.

@scofalik
Copy link
Contributor

scofalik commented Jan 27, 2017

I'm not sure how generic it is

I am not sure either, but I am sure that only some features will like to "save"/"serialize" their markers. We may need to provide a way to create special converters in buildModelConverter to convert markers to the output that @fredck proposed. And also the sam goes for buildViewConverter.

@fredck
Copy link
Contributor Author

fredck commented Jan 27, 2017

I agree that markers will have several use cases that will not require conversion into the data pipeline at all. Others will not have any visibility in the view. Others will be just local and not shared with other users. Etc. It seems to be a feature of markers to be flagged in a way that they behave differently.

What I wanted to say on this issue is that, IF a marker is to be serialized in the data pipeline, it should be in a different way, not in the way it happens in the view.

And in fact, I don't see other ways for a marker to be serialized, if it is supposed to be serialized.

So our approach could be either having a default implementation for this converter in engine or wait for the first feature that needs it to implement it and later on move it to engine, if other features will pop up.

@oskarwrobel oskarwrobel self-assigned this Feb 26, 2017
oskarwrobel referenced this issue in ckeditor/ckeditor5-engine Mar 7, 2017
Feature: Introduced markers serialization. Closes #787. Closes #846.

BREAKING CHANGES: `BuildModelConverter#fromMarkerCollapsed` is removed. Use `BuildModelConverter#fromMarker` instead.

NOTE: `insertUIElement` model to view converter now supports collapsed and non-collapsed ranges.
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-engine Oct 9, 2019
@mlewand mlewand added this to the iteration 9 milestone Oct 9, 2019
@mlewand mlewand added status:confirmed type:feature This issue reports a feature request (an idea for a new functionality or a missing option). 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:feature This issue reports a feature request (an idea for a new functionality or a missing option).
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants