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

Split Selection into (Static)Selection and LiveSelection #3644

Closed
Reinmar opened this issue Apr 11, 2016 · 12 comments · Fixed by ckeditor/ckeditor5-engine#505
Closed

Split Selection into (Static)Selection and LiveSelection #3644

Reinmar opened this issue Apr 11, 2016 · 12 comments · Fixed by ckeditor/ckeditor5-engine#505
Assignees
Labels
package:engine type:feature This issue reports a feature request (an idea for a new functionality or a missing option).
Milestone

Comments

@Reinmar
Copy link
Member

Reinmar commented Apr 11, 2016

The request comes from the question "what are we going to pass to methods like deleteContents()?". In CKEditor 4 we usually had a command that automatically works on the real selection and helper methods which accept ranges. The command of course call one of the helper methods, so they exist mostly for code reusability reasons. Other algorithms can use those in order to build some more complex behaviours.

In CKEditor 5 the 4's scheme with util methods working on ranges does not fully make sense, because:

  1. Simple array of ranges doesn't specify a direction. So it would need to be accompanied with another attribute.
  2. Selection defines also its attributes, so the array of ranges and direction would need also this.
  3. A simple set of ranges do not have selection's method from which some may be useful.

In other words, it's clearly visible that algorithms need to work on selection. Theoretically we could now do this:

const sel = new Selection( doc );

sel.addRange( someRange );

deleteUtils.delete( sel );

However, the current implementation of the Selection operates on live ranges, what mean that the selection needs to be detached once not needed. Otherwise, the editor will quickly become sluggish due to huge number of attached live positions.

Therefore, let's split the selection into two classes. Basic, static selection that can be used as a feed for utils, and live selection for the document's main selection (and those who know what they are doing).

The split should be pretty simple to do as live ranges are created in just one place.

PS. For now we can freely live without the static selection. Simply, in the tests (which, for a time being, will be the only code that uses the feature utils) we can use live selection and detach it when needed.

@pjasiun
Copy link

pjasiun commented May 24, 2016

https://github.com/ckeditor/ckeditor5-engine/issues/441 prove that we need static selection.

@scofalik
Copy link
Contributor

scofalik commented Jun 2, 2016

My question is whether we really need a Selection object. Do we need attributes interface and other methods of Selection? Maybe what we need is something that we could call SelectionState which would be an array of ranges + direction. I already use something similar in undo:

const selection = {
    ranges: Array.from( this.editor.document.selection.getRanges() ),
    isBackward: this.editor.document.selection.isBackward
};

this._items.push( { batch, selection } );

@pjasiun
Copy link

pjasiun commented Jun 2, 2016

It is not very object oriented ;) I think that some methods might be useful. Also we may want to pass this object somewhere, for instance set model selection to the values from the static selection.

@scofalik
Copy link
Contributor

scofalik commented Jun 2, 2016

You could retrive this kind of object by using, let's say .getState() and use it by .setState(). What I mean is that I don't belive that we need much more than those values and creating another kind of selection seems confusing.

@scofalik
Copy link
Contributor

scofalik commented Jun 2, 2016

I am not saying t hat SelectionState should not be a class. It can be a class, no problem. I just mean that we probably need only "selection snapshot". At least for two use cases that come to my mind: undo and deleteContents, all we really need is ranges + direction. No need for attributes, adding/removing, etc.

@pjasiun
Copy link

pjasiun commented Jun 2, 2016

https://github.com/ckeditor/ckeditor5-engine/issues/441 also do not need attributes. I agree that SelectionState/StaticSelection/SelectionSnapshot may have no attribute API. At least as long we will not find a case for them.

@Reinmar
Copy link
Member Author

Reinmar commented Jun 2, 2016

Do we need attributes interface and other methods of Selection?

I presume that we need attributes to represent empty inline styles.

And we definitely need other selection methods.

Selection is a real thing and has a behaviour (== methods), state and all things which make it a clear nominee for a class (== an object with properties and with methods to work with that object). Plus, we may have multiple selections – one real, many collaborators' selections, selections in the undo, selections used temporarily in your code to represent some selection state (like arguments for deleteContents()), etc.

@Reinmar
Copy link
Member Author

Reinmar commented Jun 23, 2016

This feature is needed in 0.1.0 because right now undoing deleting gives you a non-collapsed selection. That happens because right before delete happens the selection is extended to the left/right to contain the content that delete should remove.

If we had static selection we could use it to tell deleteContents() what should be deleted, instead of modifying the real selection.

@scofalik scofalik self-assigned this Jun 27, 2016
@scofalik
Copy link
Contributor

My question was whether we need attributes API in StaticSelection. From what I and PJ discussed the answer is no.

@pjasiun
Copy link

pjasiun commented Jun 27, 2016

I think we can add attribute support at any moment. We should add it as soon as we found usage for it.

@scofalik
Copy link
Contributor

  • Introduced DocumentSelection which has same features as "old" Selection.
  • Selection is now static, meaning that it does not create LiveRanges and does not update when document changes.
  • Selection has no attributes API.
  • Selection has no "default range".
  • DocumentSelection has getSnapshot method that returns Selection instance with same ranges and direction as that DocumentSelection.

@scofalik
Copy link
Contributor

Changed getSnapshot to Selection.createFromSelection.

@mlewand mlewand transferred this issue from ckeditor/ckeditor5-engine Oct 9, 2019
@mlewand mlewand added this to the v0.1.0 milestone Oct 9, 2019
@mlewand mlewand added 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.

4 participants