Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

T/329 Split Selection to Selection and DocumentSelection #505

Merged
merged 8 commits into from Jun 29, 2016
Merged

Conversation

scofalik
Copy link
Contributor

@scofalik scofalik commented Jun 27, 2016

This PR fixes ckeditor/ckeditor5#3644

Notable changes:

  • 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.

New Selection class is meant to be used when you need to cached it or in methods that may/have to modify selection.

@pjasiun pjasiun self-assigned this Jun 28, 2016
@pjasiun
Copy link

pjasiun commented Jun 28, 2016

The only important difference between Selection and DocumentSelection is that DocumentSelection use LiveRange instead of regular ranges. It means that DocumentSelection should be called LiveSelection for consistency.

@scofalik
Copy link
Contributor Author

Done

@@ -162,7 +162,6 @@ export default class Document {
this.version++;

if ( operation.delta ) {
// Right now I can't imagine operations without deltas, but let's be safe.
this.history.addDelta( operation.delta );
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you merge master?

@scofalik
Copy link
Contributor Author

Done.

*
* @returns {engine.model.Selection} Selection instance which ranges and direction is equal to this selection.
*/
getSnapshot() {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have Position.createFromPosition, Range.createFromRange so why not Selection.createFromSelection?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact I do not like these method, I prefer .clone( isDynamic ) but it is more important to have a consistent API now, we can discuss it in the separate issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact, why not Zoidberg?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

…on. Docs: Selection#_ranges is now protected.
@scofalik
Copy link
Contributor Author

Done.

@pjasiun pjasiun merged commit 4f873fb into master Jun 29, 2016
@pjasiun pjasiun deleted the t/329 branch June 29, 2016 12:26
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants