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 are all you need #4152

Closed
pjasiun opened this issue Aug 18, 2017 · 13 comments · Fixed by ckeditor/ckeditor5-engine#1268
Closed

Markers are all you need #4152

pjasiun opened this issue Aug 18, 2017 · 13 comments · Fixed by ckeditor/ckeditor5-engine#1268
Assignees
Labels
package:engine status:discussion type:docs This issue reports a task related to documentation (e.g. an idea for a guide).
Milestone

Comments

@pjasiun
Copy link

pjasiun commented Aug 18, 2017

First, let's put apart (static) ranges, positions, and selection. These are used for different cases, for instance, to create and set the document selection.

I think that the number mechanisms we offer developers to handle the same problem is too big: liveposition, liverange and markers. It looks over-complicated and it is more than most users need. The answer to the question: what is a replacement for bookmarks from CKE4 should be simple. And I think it should be: markers.

This is the complete solution, gives you all what you get using live ranges (or live position if the range is collapsed) but also:

  • have a name, what is very useful to get a marker, allows users to use namespaces, create multiple markers of the same "type",
  • markers collection fire event when a marker is created or removed, what is not possible to do with live ranges,
  • let you create converters to the virtual selection easily and show the marker in the DOM.

However, there are 2 problems.

First, we create markers much later than we created live ranges. This is why they are described as live ranges with names. To understand makers you need to understand live ranges first, what is wrong in my opinion. They should be described as an easy and ready to use solution, not overlay using another mechanism.

The second problem is that it should be very clear what is the difference between marker added and not added to the document.

  • Marker added to the document will be synchronized in the collaboration and handled in the undo. Is useful for solutions like spell checking or comments.
  • Marker not added to the document can be used as a bookmark or visual markers. To show results of the find, or select link when the focus is in the input (see https://github.com/ckeditor/ckeditor5-link/issues/13).

They also need to use different API.

We should make it very clear. We could even call these two types of markers differently. What does not help is the fact that the second group is also added to the MarkersCollection which is a property of the document, so they are somehow added to the document. Maybe we should reconsider this, for instance, MarkersCollection could be a property in the editing controller, not the document.

@scofalik
Copy link
Contributor

I have nothing to add, +1.

@Reinmar
Copy link
Member

Reinmar commented Oct 9, 2017

We need to allow anonymous (or uid) markers.

@Reinmar
Copy link
Member

Reinmar commented Oct 9, 2017

And simplify API for creating (standalone edit: they are firing changes...) markers and document markers.

@pjasiun
Copy link
Author

pjasiun commented Dec 22, 2017

All markers should be handled by the writer. There should be additional parameter: type in setMarker with possible values:

  • 'dependent' - marker added to the document, synchronized in the collaboration and handled in the undo; is useful for solutions like spell checking or comments,
  • 'independent' (default) - marker not added to the document, can be used as a bookmark or visual markers, to show results of the find, or select link when the focus is in the input (see ckeditor/ckeditor5-link#13).

Note that dependent markers should be added using operation, while independent should be only added to the markers collection.

Markers collection set and remove should be changed to protected.

There should be also type property in the Marker class to get the type of the marker.

markerOrName should be an optional parameter and if not provided unique ID should be used. Note that ID should be a string which is allowed as an event name.

setMarker should return the created marker.

It should be possible to use setMarker not only to change the range but also to change the type of the markers.

The API should looks like this:

writer.setMarker( markerOrName, newRange, type )
writer.removeMarker( markerOrName )

Docs should be also updated. It should not be needed to understand how live ranges work to learn what markers are, however, there should be @see LiveRange note at the end.

@scofalik
Copy link
Contributor

Of course, I am for this idea, but I don't like the flag name. It's hard to come up with a proper name for this functionality, but the proposed name is not good enough.

@jodator
Copy link
Contributor

jodator commented Dec 26, 2017

Yeah - I also get the feeling that type names are meaningless - I don't have a better names for those yet. The first thought was:

  • dependent - it's a marker that resides in document and is represent in model so for me it's more document type of marker.
  • independent - here is clearly a visual type of marker for me.

The other way is to set a flag rather then type as I don't see other types of markers.

@pjasiun
Copy link
Author

pjasiun commented Jan 3, 2018

I agree that the name of the flag is not perfect. The problem is that it is not easy to define the second type of markers. They are not using operation, are not send to other clients and not tracked by the history. But on the other hand, they use live ranges so they track changes in the document and react to them, and, since all markers have names, then can be observable, tracked.

This is why the best choice seems to be to define them by the mechanism they use.

The boolean flag:

managedByOperations

Methods:

writer.setMarker( markerOrName, newRange, { byOperation: true } );
writer.removeMarker( markerOrName )

Note that in creates a nice sentence "set marker by operation".

@scofalik
Copy link
Contributor

scofalik commented Jan 4, 2018

Please, let it be useOperation or at least usingOperation. It sounds more correct, at least for me.

It reads better: "set marker using operation" instead of "set marker by operation". You are literally using operation to set that marker so that's better IMO.

@pjasiun
Copy link
Author

pjasiun commented Jan 4, 2018

I agree, usingOperation sounds better.

The boolean flag:

managedUsingOperations

Methods:

writer.setMarker( markerOrName, newRange, { usingOperation: true } );
writer.removeMarker( markerOrName )

pjasiun referenced this issue in ckeditor/ckeditor5-engine Feb 6, 2018
Other: Provided one API for two types of markers, improved docs. Closes #1086.

BREAKING CHANGE: Writer should be now used to set or remove markers, instead of MarkerCollection.
@Reinmar
Copy link
Member

Reinmar commented Feb 6, 2018

@AnnaTomanek, isn't "managed by using operations" more correct? That's what I understood from https://english.stackexchange.com/questions/217815/what-is-difference-between-using-and-by-using.

@AnnaTomanek
Copy link
Contributor

It may be, but I'd propose to keep it simple. Not everyone will (1) know there is any difference and (2) understand what this difference is. IMHO managedUsingOperations sounds obvious enough.

@Reinmar
Copy link
Member

Reinmar commented Feb 21, 2018

@pjasiun, you can sleep tight now ;)

@blackening
Copy link

blackening commented Feb 17, 2019

  • have a name, what is very useful to get a marker, allows users to use namespaces, create multiple markers of the same "type",

Sorry, can i check if this has been implemented?

Edit: found it - "Name is used to group and identify markers. Names have to be unique, but markers can be grouped by using common prefixes, separated with :, for example: user:john or search:3"

@mlewand mlewand transferred this issue from ckeditor/ckeditor5-engine Oct 9, 2019
@mlewand mlewand added this to the iteration 14 milestone Oct 9, 2019
@mlewand mlewand added module:model status:discussion type:docs This issue reports a task related to documentation (e.g. an idea for a guide). 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 status:discussion type:docs This issue reports a task related to documentation (e.g. an idea for a guide).
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants