Implement a builtin Position, Range and Selection factories

[Reinmar]

Strongly related to #667.

1. We need to check what do we import from @ckeditor/ckeditor5-engine/src in plugins (source, not tests). I used this regexp import [\w, {},_]+ from '@ckeditor/ckeditor5-engine/src. From my quick check, 90% of that are Position, Range and Selection from the model and the view. There are also some converters from src/conversion/downcast-converters and src/conversion/upcast-converters, but one thing at a time (https://github.com/ckeditor/ckeditor5-engine/issues/1556).
2. Design a set of factories like createRange(), createRangeOn(), etc.
3. Review this list based on the actual use. Decide which static methods of Range and Position may be removed since the factories will be created.
4. Decide where to expose those factories. On the Document classes or on View&Model?
5. Implement those methods.
6. Update all existing code. It may turn out that the dependency on @ckeditor/ckeditor5-engine can be removed in a couple of plugins.
7. Remove static methods if we'll decide in step 3. to remove them.

cc @jodator @pjasiun

---

Edit (by @jodator):

• model methods usage review
  
• model API proposal

[Reinmar]

We've been planning this for a long time but I realised that it will significantly affect some of the documentation. This is currently one of the biggest blockers to write plugins without dependency on the engine.

[pjasiun]

Wow, that's huge :/ However, I agree that this is important.

[Reinmar]

I actually don't think that it's so big. It may even turn out that we'll simply move the static factory methods to some other place and implement a simple pass-through method for constructors. We've got most of the code and tests. I hope it's mostly about moving it around.

[jodator]

I hope it's mostly about moving it around.

I also think that is just that. Maybe huge in terms of lines changed but doable right now.

[pjasiun]

I also think that is just that. Maybe huge in terms of lines changed but doable right now.

That might be true.

I was checking the API and realized that we have also TreeWalker to be exposed. However, it can be get using range methods. For instance:

const walker = new TreeWalker( { boundaries: urlRange, ignoreElementEnd: true } );

(https://github.com/ckeditor/ckeditor5-media-embed/blob/7e75a681aa070ebdd4f5fddf04bdf8e1887903e2/src/automediaembed.js#L117)
Can be changed to:

const walker = urlRange.getWalker( { ignoreElementEnd: true } );

Also, I was checking how often do we use Selection class, but it is used often enough to need to be exposed. On the other hand, DocumentSelection class should not be exposed.

I think that we need to simply count the number of usage of all constructors and static methods of model.selection, model.range, model.position, view.selection, view.range and view.position. This way we will know which methods are popular and which are not used at all. We should count only usage in src of plugins (excluding engine internal usage, core, and tests). @jodator can you handle this?

[jodator]

@pjasiun with pleasure ;) Already on it. Some conclusion are already there, like Position.createFromParentsAndOffset() might be private and not exposed as it is used only by table and might be replaced by other factory methods. Anyway I'll prepare full report on this.

[jodator]

Current usage of model imports

Imports:

• import Range from '@ckeditor/ckeditor5-engine/src/model/range'; (15)
• import Position from '@ckeditor/ckeditor5-engine/src/model/position'; (12)
• import Selection from '@ckeditor/ckeditor5-engine/src/model/selection'; (6)
• import Element from '@ckeditor/ckeditor5-engine/src/model/element'; (6)
• import Batch from '@ckeditor/ckeditor5-engine/src/model/batch'; (3)
• import TreeWalker from '@ckeditor/ckeditor5-engine/src/model/treewalker'; (1)
• import { SchemaContext } from '@ckeditor/ckeditor5-engine/src/model/schema'; (1)
• import LivePosition from '@ckeditor/ckeditor5-engine/src/model/liveposition'; (1)
• import LiveRange from '@ckeditor/ckeditor5-engine/src/model/liverange'; (1)
• import { transformSets } from '@ckeditor/ckeditor5-engine/src/model/operation/transform'; (1)

As you can see it's not much (probably test would have hundreds...) - also there are some minimal usages of other things like SchemaContext from model.

Position

• new Position() 0

• Position.createFromPosition() 0

• Position.fromJSON() 0

• Position.createAt() (20 usages found)

  packages/ckeditor5-image/src/image  (1 usage found)
  packages/ckeditor5-link/src  (1 usage found)
  packages/ckeditor5-list/src  (4 usages found)
  packages/ckeditor5-paragraph/src  (2 usages found)
  packages/ckeditor5-table/src  (11 usages found)
  packages/ckeditor5-widget/src  (1 usage found)
  

  • mostly used as:
    • Position.createAt( node )
    • Position.createAt( node, 'end' )
    • one Position.createAt( node, foo ? 'after' : 'before' )
    • one Position.createAt( node, foo ? 'end' : 0 )

• Position.createAfter() (11 usages found)

  packages/ckeditor5-block-quote/src  (3 usages found)
  packages/ckeditor5-list/src  (1 usage found)
  packages/ckeditor5-table/src  (5 usages found)
  packages/ckeditor5-widget/src  (2 usages found)
  

• Position.createBefore() (11 usages found)

  packages/ckeditor5-block-quote/src  (2 usages found)
  packages/ckeditor5-list/src  (5 usages found)
  packages/ckeditor5-paragraph/src  (1 usage found)
  packages/ckeditor5-table/src/converters  (2 usages found)
  packages/ckeditor5-widget/src  (1 usage found)
  

• Position.createFromParentAndOffset() (3 usages found)

  packages/ckeditor5-table/src  (3 usages found)
  

Range

• Range.createCollapsedAt() 0

• Range.createFromRanges() 0

• Range.createFromRange() 0

• Range.createFromParentsAndOffsets (3 usages found)

  packages/ckeditor5-autoformat/src  (2 usages found)
  packages/ckeditor5-typing/src/  (1 usage found)
  

• Range.createFromPositionAndShift() (1 usage found)

  packages/ckeditor5-typing/src/utils  (1 usage found)
  

• Range.createIn() (8 usages found)

  packages/ckeditor5-table/src  (5 usages found)
  packages/ckeditor5-typing/src  (1 usage found)
  packages/ckeditor5-widget/src  (2 usages found)
  

  Mostly used as: writer.setSelection (5)

• Range.createOn() (3 usages found)

  packages/ckeditor5-link/src  (1 usage found)
  packages/ckeditor5-table/src/commands  (1 usage found)
  packages/ckeditor5-widget/src  (1 usage found)
  

  • writer.setSelection (2)

• new Range() (9 usages found)

  packages/ckeditor5-block-quote/src  (1 usage found)
  packages/ckeditor5-highlight/src  (1 usage found)
  packages/ckeditor5-image/src  (1 usage found)
  packages/ckeditor5-link/src  (1 usage found)
  packages/ckeditor5-list/src  (1 usage found)
  packages/ckeditor5-paragraph/src  (1 usage found)
  packages/ckeditor5-table/src  (2 usages found)
  packages/ckeditor5-typing/src  (1 usage found)
  

Live Range & Live Position

• LivePosition.createFromPosition (2 usages in AutoMediaEmbed)
• new LiveRange() (1 usage in AutoMediaEmbed)

Selection

• new Selection() (8 usages found)

  packages/ckeditor5-image/src  (2 usages found)
  packages/ckeditor5-list/src  (1 usage found)
  packages/ckeditor5-typing/src  (4 usage found)
  packages/ckeditor5-widget/src  (1 usage found)
  

Element

• new Element() (2 usages found)

  packages/ckeditor5-block-quote/src/blockquotecommand.js  (1 usage found)
          161 quote = new Element( 'blockQuote' );
  packages/ckeditor5-typing/src/deletecommand.js  (1 usage found)
          176 const paragraph = new Element( 'paragraph' );
  

• intanceof Element() (6 usages found)

  packages/ckeditor5-image/src/image  (1 usage found)
      utils.js  (1 usage found)
          68 return modelElement instanceof ModelElement && modelElement.name == 'image';
  packages/ckeditor5-image/src/imagecaption  (1 usage found)
      utils.js  (1 usage found)
          51 if ( node instanceof ModelElement && node.name == 'caption' ) {
  packages/ckeditor5-list/src  (2 usages found)
      converters.js  (2 usages found)
          827 const indent = modelItemOrPosition instanceof ModelElement ? modelItemOrPosition.getAttribute( 'listIndent' ) : options.listIndent;
          828 let item = modelItemOrPosition instanceof ModelElement ? modelItemOrPosition.previousSibling : modelItemOrPosition.nodeBefore;
  packages/ckeditor5-widget/src  (2 usages found)
      widget.js  (2 usages found)
          251 if ( objectElement2 instanceof ModelElement && schema.isObject( objectElement2 ) ) {
          372 if ( objectElement instanceof ModelElement && schema.isObject( objectElement ) ) {
  

[jodator]

New model API proposal

Where to expose new API

Initially I thought that it should go on Model as it is engine/model/position module but I think that we should place the methods on the document:

• it's Position in document
• it is a Range in the document
• there's editor.model.document.selection
• position is relative to the document's root (you need root to create position)
• web API for range document.createRangeFromPoint() exists

So I'm for editor.model.document.createFooBarBaz() methods.

Position

1. The basic factory methods is Position.createAt() as an entry point and Position.createFromParentAndOffset() combo.
   This method is used as:

• Position.createAt( node, 'after' ) === Position.createAfter()
• Position.createAt( node, 'before' ) === Position.createBefore()
• Position.createAt( node, 'end' )
• Position.createAt( node ) === Position.createAt( node, 0 ) (also should be explicit)
• Position.createAt( node, offset )
  Looks useful, so: doc.createPositionAt().

2. Shortcut methods (are they really useful?):

   • Postion.createAfter() -> doc.createPostionAfter()
   • Postion.createBefore() -> doc.createPostionBefore()

3. (not used outside engine) The clones: Positon.createFromPosition() IMO should be: position.clone().

4. (not used outside engine) new Position() - mostly used in tests doc.createPosition():

doc.createPosition( [ 0, 1, 2 ] ); // creates position in default root
doc.createPosition( [ 0, 1, 2 ], '$graveyard' );
doc.createPosition( [ 0, 1 ], doc.getRoot() );

Summary:

const doc = editor.model.document;

const positionAfter = doc.createPositionAt( node, 'after' );
const positionBefore = doc.createPositionAt( node, 'before' );
const positionAt = doc.createPositionAt( node, 7 );
const positionAtEnd = doc.createPositionAt( node, 'end' );

const cloned = positionAt.clone();

const positionInTest = doc.createPosition( [ 0, 2, 3 ], '$graveyard' );

Range

1. Mostly used Range.createOn() & Range.createIn() - looks helpful so

• doc.createRangeIn()
• doc.createRangeOn()

2. Range.createFromParentsAndOffsets() might be replaced by doc.createRange( doc.createPositionAt(), ... )
3. Range.createFromPositionAndShift() has only one usage in typing and with positive shift (deletion count) so it can be replaced with doc.createRange( postion, position.getShiftedBy( shift ) )
4. The clones: Range.createFomRange() IMO should be: range.clone().
5. Creating range: new Range: doc.createRange( start, end ). But I have troubles on it's usages ib some places (other are OK):

• conversion (list, table) - specialised converters creates ranges
• new Range().getWalker() in paragraph also might be improved (?)

6. I'd remove not used:

• createCollapsedAt() the same as doc.createRange( position )
• createFromRanges() internal use - make helper method

Summary:

const doc = editor.model.document;

const range = doc.createRange( position, position.getShiftedBy( 7 ) );

const cloned = range.clone();

const rangeOnNode = doc.createRangeOn( node );
const rangeInsideNode = doc.createRangeIn( tableCell );

Selection

The selection factory to be exposed:

const selection = doc.createSelection( doc.createRange( start, end ) );

doc.selection.setSelection( selection )

Other APIs (might be follow ups):

1. Remove new Element() - use writer.createElement().
2. Remove element instanceof Element - most of the time its just !!element checks but to be sure we have also !!element && element.is( 'element' ).
3. LiveRange & LivePosition - @pjasiun said we can do a follow up. But I can see that we can createFoo or initFoot for them:

const liveRange = doc.createLiveRange( range ); 
// or:
doc.createLiveRange( start, end ); // or both
const livePostion = doc.createLivePosition( position );

4. The Batch - most of the times () it might be replaced by model.enqueueChange( 'default', () => {} ) as new Batch() creation is redundant there.
   The other use in Undo AFAIR.
5. SchemaContext - also can be replaced by a factory method as schema.createContext( position ) (?).
6. import { transformSets } - Undo (generally Undo is a special case where we might leave engine dependency)
7. The TreeWalker usage in MediaEmbed - AFAICS can be replaced by: doc.createLiveRange( start, stop ).getWalker( { ignoreElementEnd: true } )

[pjasiun]

Great research. Can't wait to see the new API in action.

Some notes from me.

So I'm for editor.model.document.createFooBarBaz() methods.

That's not true. You can have a position on the detached document fragment which is not a document. editor.model.document.selection is instance of DocumentSelection, you can have Selection instance outside the document. This is why I am for model.*.

const positionInTest = doc.createPosition( [ 0, 2, 3 ], '$graveyard' );

doc.createPosition -> model.createPositionFromPath since this it the more "advanced" API than doc.createPositionAt.

The clones: Range.createFomRange() IMO should be: range.clone().

I'm for it. Especially if we will hide LivePosition and LiveRange and suggest users to use markers.

LiveRange & LivePosition - @pjasiun said we can do a follow-up. But I can see that we can createFoo or initFoot for them:

See https://github.com/ckeditor/ckeditor5-engine/issues/1086.

The Batch - most of the times () it might be replaced by model.enqueueChange( 'default', () => {} ) as new Batch() creation is redundant there.
The other use in Undo AFAIR.

I am for model.createBatch. Typing case prove that model.enqueueChange( 'default' ... ) is not enought in some cases.

[jodator]

So we agreed that:

1. The main goal is to remove imports from code (not the tests) of other plugins to remove that dependency.
2. Put the factory methods on writer
3. Implement LivePosition#toPosition(), LivePosition.fromPosition() and .clone() instead of Postion.createFromPosition() - and similar for Range.
4. The create position will be: model.createPositionFromPath()
5. remove batch dependency by chaning to model.enqueChange and model.createBatch()

[Reinmar]

I just found this:

		const endPos = LivePosition.createFromPosition( selRange.end );
		endPos.stickiness = 'toNext';

Perhaps worth considering while taking care of createFromPosition().