Skip to content

Commit

Permalink
Merge pull request #17296 from ckeditor/cc/performance-offsets
Browse files Browse the repository at this point in the history
Feature (engine): Introduced `getChildAtOffset()` method for `model.Element` and `model.DocumentFragment`.

Feature (engine): Introduced `Position#isValid()` to check whether the position exists in current model tree.

Other (engine): Node index and offset related values are now cached in model `Node` and `NodeList` to improve performance.

Internal (engine): Used `Position#isValid()` to better validate selection ranges.

Tests (engine): Changed error messages expected in some tests. Now different errors may be thrown than earlier because internal execution logic changed a bit. New error codes are more precise.
  • Loading branch information
Dumluregn authored Oct 23, 2024
2 parents 56cce90 + 055753f commit d874050
Show file tree
Hide file tree
Showing 13 changed files with 350 additions and 143 deletions.
3 changes: 2 additions & 1 deletion packages/ckeditor5-engine/src/model/document.ts
Original file line number Diff line number Diff line change
Expand Up @@ -433,7 +433,8 @@ export default class Document extends /* #__PURE__ */ EmitterMixin() {
* @returns `true` if `range` is valid, `false` otherwise.
*/
public _validateSelectionRange( range: Range ): boolean {
return validateTextNodePosition( range.start ) && validateTextNodePosition( range.end );
return range.start.isValid() && range.end.isValid() &&
validateTextNodePosition( range.start ) && validateTextNodePosition( range.end );
}

/**
Expand Down
16 changes: 13 additions & 3 deletions packages/ckeditor5-engine/src/model/documentfragment.ts
Original file line number Diff line number Diff line change
Expand Up @@ -149,13 +149,23 @@ export default class DocumentFragment extends TypeCheckable implements Iterable<
/**
* Gets the child at the given index. Returns `null` if incorrect index was passed.
*
* @param index Index of child.
* @param index Index in this document fragment.
* @returns Child node.
*/
public getChild( index: number ): Node | null {
return this._children.getNode( index );
}

/**
* Gets the child at the given offset. Returns `null` if incorrect index was passed.
*
* @param offset Offset in this document fragment.
* @returns Child node.
*/
public getChildAtOffset( offset: number ): Node | null {
return this._children.getNodeAtOffset( offset );
}

/**
* Returns an iterator that iterates over all of this document fragment's children.
*/
Expand Down Expand Up @@ -208,8 +218,8 @@ export default class DocumentFragment extends TypeCheckable implements Iterable<
// eslint-disable-next-line @typescript-eslint/no-this-alias, consistent-this
let node: Node | DocumentFragment = this;

for ( const index of relativePath ) {
node = ( node as Element | DocumentFragment ).getChild( ( node as Element | DocumentFragment ).offsetToIndex( index ) )!;
for ( const offset of relativePath ) {
node = ( node as Element | DocumentFragment ).getChildAtOffset( offset )!;
}

return node;
Expand Down
19 changes: 16 additions & 3 deletions packages/ckeditor5-engine/src/model/element.ts
Original file line number Diff line number Diff line change
Expand Up @@ -82,12 +82,25 @@ export default class Element extends Node {
}

/**
* Gets the child at the given index.
* Gets the child at the given index. Returns `null` if incorrect index was passed.
*
* @param index Index in this element.
* @returns Child node.
*/
public getChild( index: number ): Node | null {
return this._children.getNode( index );
}

/**
* Gets the child at the given offset. Returns `null` if incorrect index was passed.
*
* @param offset Offset in this element.
* @returns Child node.
*/
public getChildAtOffset( offset: number ): Node | null {
return this._children.getNodeAtOffset( offset );
}

/**
* Returns an iterator that iterates over all of this element's children.
*/
Expand Down Expand Up @@ -153,8 +166,8 @@ export default class Element extends Node {
// eslint-disable-next-line @typescript-eslint/no-this-alias, consistent-this
let node: Node = this;

for ( const index of relativePath ) {
node = ( node as Element ).getChild( ( node as Element ).offsetToIndex( index ) )!;
for ( const offset of relativePath ) {
node = ( node as Element ).getChildAtOffset( offset )!;
}

return node;
Expand Down
68 changes: 26 additions & 42 deletions packages/ckeditor5-engine/src/model/node.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,20 @@ export default abstract class Node extends TypeCheckable {
*/
private _attrs: Map<string, unknown>;

/**
* Index of this node in its parent or `null` if the node has no parent.
*
* @internal
*/
public _index: number | null = null;

/**
* Offset at which this node starts in its parent or `null` if the node has no parent.
*
* @internal
*/
public _startOffset: number | null = null;

/**
* Creates a model node.
*
Expand All @@ -85,66 +99,42 @@ export default abstract class Node extends TypeCheckable {

/**
* Index of this node in its parent or `null` if the node has no parent.
*
* Accessing this property throws an error if this node's parent element does not contain it.
* This means that model tree got broken.
*/
public get index(): number | null {
let pos;

if ( !this.parent ) {
return null;
}

if ( ( pos = this.parent.getChildIndex( this ) ) === null ) {
throw new CKEditorError( 'model-node-not-found-in-parent', this );
}

return pos;
return this._index;
}

/**
* Offset at which this node starts in its parent. It is equal to the sum of {@link #offsetSize offsetSize}
* of all its previous siblings. Equals to `null` if node has no parent.
*
* Accessing this property throws an error if this node's parent element does not contain it.
* This means that model tree got broken.
*/
public get startOffset(): number | null {
let pos;

if ( !this.parent ) {
return null;
}

if ( ( pos = this.parent.getChildStartOffset( this ) ) === null ) {
throw new CKEditorError( 'model-node-not-found-in-parent', this );
}

return pos;
return this._startOffset;
}

/**
* Offset size of this node. Represents how much "offset space" is occupied by the node in it's parent.
* It is important for {@link module:engine/model/position~Position position}. When node has `offsetSize` greater than `1`, position
* can be placed between that node start and end. `offsetSize` greater than `1` is for nodes that represents more
* than one entity, i.e. {@link module:engine/model/text~Text text node}.
* Offset size of this node.
*
* Represents how much "offset space" is occupied by the node in its parent. It is important for
* {@link module:engine/model/position~Position position}. When node has `offsetSize` greater than `1`, position can be placed between
* that node start and end. `offsetSize` greater than `1` is for nodes that represents more than one entity, i.e.
* a {@link module:engine/model/text~Text text node}.
*/
public get offsetSize(): number {
return 1;
}

/**
* Offset at which this node ends in it's parent. It is equal to the sum of this node's
* Offset at which this node ends in its parent. It is equal to the sum of this node's
* {@link module:engine/model/node~Node#startOffset start offset} and {@link #offsetSize offset size}.
* Equals to `null` if the node has no parent.
*/
public get endOffset(): number | null {
if ( !this.parent ) {
if ( this.startOffset === null ) {
return null;
}

return this.startOffset! + this.offsetSize;
return this.startOffset + this.offsetSize;
}

/**
Expand Down Expand Up @@ -387,7 +377,7 @@ export default abstract class Node extends TypeCheckable {
}

/**
* Removes this node from it's parent.
* Removes this node from its parent.
*
* @internal
* @see module:engine/model/writer~Writer#remove
Expand Down Expand Up @@ -448,12 +438,6 @@ Node.prototype.is = function( type: string ): boolean {
return type === 'node' || type === 'model:node';
};

/**
* The node's parent does not contain this node.
*
* @error model-node-not-found-in-parent
*/

/**
* Node's attributes. See {@link module:utils/tomap~toMap} for a list of accepted values.
*/
Expand Down
Loading

0 comments on commit d874050

Please sign in to comment.