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

Commit

Permalink
Merge pull request #1325 from ckeditor/t/1308
Browse files Browse the repository at this point in the history
Internal: Improved `Writer#setSelection()` and `Selection#setTo()` methods. Closes #1308.
  • Loading branch information
Piotr Jasiun authored Mar 6, 2018
2 parents 24b97f5 + cf27c1c commit 3d25385
Show file tree
Hide file tree
Showing 29 changed files with 559 additions and 384 deletions.
4 changes: 1 addition & 3 deletions src/controller/editingcontroller.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,7 @@ import { convertSelectionChange } from '../conversion/upcast-selection-converter
import {
convertRangeSelection,
convertCollapsedSelection,
clearAttributes,
clearFakeSelection
clearAttributes
} from '../conversion/downcast-selection-converters';

import ObservableMixin from '@ckeditor/ckeditor5-utils/src/observablemixin';
Expand Down Expand Up @@ -95,7 +94,6 @@ export default class EditingController {

// Attach default model selection converters.
this.downcastDispatcher.on( 'selection', clearAttributes(), { priority: 'low' } );
this.downcastDispatcher.on( 'selection', clearFakeSelection(), { priority: 'low' } );
this.downcastDispatcher.on( 'selection', convertRangeSelection(), { priority: 'low' } );
this.downcastDispatcher.on( 'selection', convertCollapsedSelection(), { priority: 'low' } );

Expand Down
10 changes: 1 addition & 9 deletions src/conversion/downcast-selection-converters.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ export function convertRangeSelection() {
viewRanges.push( viewRange );
}

conversionApi.writer.setSelection( viewRanges, selection.isBackward );
conversionApi.writer.setSelection( viewRanges, { backward: selection.isBackward } );
};
}

Expand Down Expand Up @@ -127,11 +127,3 @@ export function clearAttributes() {
viewWriter.setSelection( null );
};
}

/**
* Function factory, creates a converter that clears fake selection marking after the previous
* {@link module:engine/model/selection~Selection model selection} conversion.
*/
export function clearFakeSelection() {
return ( evt, data, conversionApi ) => conversionApi.writer.setFakeSelection( false );
}
2 changes: 1 addition & 1 deletion src/conversion/upcast-selection-converters.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ export function convertSelectionChange( model, mapper ) {
ranges.push( mapper.toModelRange( viewRange ) );
}

modelSelection.setTo( ranges, viewSelection.isBackward );
modelSelection.setTo( ranges, { backward: viewSelection.isBackward } );

if ( !modelSelection.isEqual( model.document.selection ) ) {
model.change( writer => {
Expand Down
4 changes: 2 additions & 2 deletions src/dev-utils/model.js
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ export function setData( model, data, options = {} ) {
ranges.push( new ModelRange( start, end ) );
}

writer.setSelection( ranges, selection.isBackward );
writer.setSelection( ranges, { backward: selection.isBackward } );

if ( options.selectionAttributes ) {
writer.setSelectionAttribute( selection.getAttributes() );
Expand Down Expand Up @@ -326,7 +326,7 @@ export function parse( data, schema, options = {} ) {
}

// Create new selection.
selection = new ModelSelection( ranges, viewSelection.isBackward );
selection = new ModelSelection( ranges, { backward: viewSelection.isBackward } );

// Set attributes to selection if specified.
for ( const [ key, value ] of toMap( options.selectionAttributes || [] ) ) {
Expand Down
2 changes: 1 addition & 1 deletion src/dev-utils/view.js
Original file line number Diff line number Diff line change
Expand Up @@ -340,7 +340,7 @@ export function parse( data, options = {} ) {

// When ranges are present - return object containing view, and selection.
if ( ranges.length ) {
const selection = new Selection( ranges, !!options.lastRangeBackward );
const selection = new Selection( ranges, { backward: !!options.lastRangeBackward } );

return {
view,
Expand Down
16 changes: 9 additions & 7 deletions src/model/documentselection.js
Original file line number Diff line number Diff line change
Expand Up @@ -348,19 +348,21 @@ export default class DocumentSelection {
/**
* Sets this selection's ranges and direction to the specified location based on the given
* {@link module:engine/model/selection~Selection selection}, {@link module:engine/model/position~Position position},
* {@link module:engine/model/element~Element element}, {@link module:engine/model/position~Position position},
* {@link module:engine/model/element~Node node}, {@link module:engine/model/position~Position position},
* {@link module:engine/model/range~Range range}, an iterable of {@link module:engine/model/range~Range ranges} or null.
* Should be used only within the {@link module:engine/model/writer~Writer#setSelection} method.
*
* @see module:engine/model/writer~Writer#setSelection
* @protected
* @param {module:engine/model/selection~Selection|module:engine/model/documentselection~DocumentSelection|
* module:engine/model/position~Position|module:engine/model/element~Element|
* module:engine/model/position~Position|module:engine/model/node~Node|
* Iterable.<module:engine/model/range~Range>|module:engine/model/range~Range|null} selectable
* @param {Boolean|Number|'before'|'end'|'after'} [backwardSelectionOrOffset]
* @param {Number|'before'|'end'|'after'|'on'|'in'} [placeOrOffset] Sets place or offset of the selection.
* @param {Object} [options]
* @param {Boolean} [options.backward] Sets this selection instance to be backward.
*/
_setTo( selectable, backwardSelectionOrOffset ) {
this._selection.setTo( selectable, backwardSelectionOrOffset );
_setTo( selectable, placeOrOffset, options ) {
this._selection.setTo( selectable, placeOrOffset, options );
}

/**
Expand Down Expand Up @@ -631,8 +633,8 @@ class LiveSelection extends Selection {
return super.getLastRange() || this._document._getDefaultRange();
}

setTo( selectable, backwardSelectionOrOffset ) {
super.setTo( selectable, backwardSelectionOrOffset );
setTo( selectable, optionsOrPlaceOrOffset, options ) {
super.setTo( selectable, optionsOrPlaceOrOffset, options );
this._refreshAttributes();
}

Expand Down
103 changes: 77 additions & 26 deletions src/model/selection.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@

import Position from './position';
import Element from './element';
import Node from './node';
import Range from './range';
import EmitterMixin from '@ckeditor/ckeditor5-utils/src/emittermixin';
import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror';
Expand All @@ -35,11 +36,11 @@ export default class Selection {
*
* // Creates selection at the given range.
* const range = new Range( start, end );
* const selection = new Selection( range, isBackwardSelection );
* const selection = new Selection( range );
*
* // Creates selection at the given ranges
* const ranges = [ new Range( start1, end2 ), new Range( star2, end2 ) ];
* const selection = new Selection( ranges, isBackwardSelection );
* const selection = new Selection( ranges );
*
* // Creates selection from the other selection.
* // Note: It doesn't copies selection attributes.
Expand All @@ -55,16 +56,31 @@ export default class Selection {
* const position = new Position( root, path );
* const selection = new Selection( position );
*
* // Creates selection at the start position of given element.
* // Creates selection at the start position of the given element.
* const paragraph = writer.createElement( 'paragraph' );
* const selection = new Selection( paragraph, offset );
*
* // Creates a range inside an {@link module:engine/model/element~Element element} which starts before the
* // first child of that element and ends after the last child of that element.
* const selection = new Selection( paragraph, 'in' );
*
* // Creates a range on an {@link module:engine/model/item~Item item} which starts before the item and ends
* // just after the item.
* const selection = new Selection( paragraph, 'on' );
*
* `Selection`'s constructor allow passing additional options (`backward`) as the last argument.
*
* // Creates backward selection.
* const selection = new Selection( range, { backward: true } );
*
* @param {module:engine/model/selection~Selection|module:engine/model/documentselection~DocumentSelection|
* module:engine/model/position~Position|module:engine/model/element~Element|
* Iterable.<module:engine/model/range~Range>|module:engine/model/range~Range} [selectable]
* @param {Boolean|Number|'before'|'end'|'after'} [backwardSelectionOrOffset]
* Iterable.<module:engine/model/range~Range>|module:engine/model/range~Range|null} selectable
* @param {Number|'before'|'end'|'after'|'on'|'in'} [placeOrOffset] Sets place or offset of the selection.
* @param {Object} [options]
* @param {Boolean} [options.backward] Sets this selection instance to be backward.
*/
constructor( selectable, backwardSelectionOrOffset ) {
constructor( selectable, placeOrOffset, options ) {
/**
* Specifies whether the last added range was added as a backward or forward range.
*
Expand All @@ -90,7 +106,7 @@ export default class Selection {
this._attrs = new Map();

if ( selectable ) {
this.setTo( selectable, backwardSelectionOrOffset );
this.setTo( selectable, placeOrOffset, options );
}
}

Expand Down Expand Up @@ -300,58 +316,93 @@ export default class Selection {
* {@link module:engine/model/element~Element element}, {@link module:engine/model/position~Position position},
* {@link module:engine/model/range~Range range}, an iterable of {@link module:engine/model/range~Range ranges} or null.
*
* // Sets ranges from the given range.
* // Removes all selection's ranges.
* selection.setTo( null );
*
* // Sets selection to the given range.
* const range = new Range( start, end );
* selection.setTo( range, isBackwardSelection );
* selection.setTo( range );
*
* // Sets ranges from the iterable of ranges.
* // Sets selection to given ranges.
* const ranges = [ new Range( start1, end2 ), new Range( star2, end2 ) ];
* selection.setTo( ranges, isBackwardSelection );
* selection.setTo( ranges );
*
* // Sets ranges from the other selection.
* // Sets selection to other selection.
* // Note: It doesn't copies selection attributes.
* const otherSelection = new Selection();
* selection.setTo( otherSelection );
*
* // Sets ranges from the given document selection's ranges.
* // Sets selection to the given document selection.
* // Note: It doesn't copies selection attributes.
* const documentSelection = new DocumentSelection( doc );
* selection.setTo( documentSelection );
*
* // Sets range at the given position.
* // Sets collapsed selection at the given position.
* const position = new Position( root, path );
* selection.setTo( position );
*
* // Sets range at the position of given element and optional offset.
* const paragraph = writer.createElement( 'paragraph' );
* // Sets collapsed selection at the position of the given node and an offset.
* selection.setTo( paragraph, offset );
*
* // Removes all ranges.
* selection.setTo( null );
* Creates a range inside an {@link module:engine/model/element~Element element} which starts before the first child of
* that element and ends after the last child of that element.
*
* selection.setTo( paragraph, 'in' );
*
* Creates a range on an {@link module:engine/model/item~Item item} which starts before the item and ends just after the item.
*
* selection.setTo( paragraph, 'on' );
*
* `Selection#setTo()`' method allow passing additional options (`backward`) as the last argument.
*
* // Sets backward selection.
* const selection = new Selection( range, { backward: true } );
*
* @param {module:engine/model/selection~Selection|module:engine/model/documentselection~DocumentSelection|
* module:engine/model/position~Position|module:engine/model/element~Element|
* module:engine/model/position~Position|module:engine/model/node~Node|
* Iterable.<module:engine/model/range~Range>|module:engine/model/range~Range|null} selectable
* @param {Boolean|Number|'before'|'end'|'after'} [backwardSelectionOrOffset]
* @param {Number|'before'|'end'|'after'|'on'|'in'} [placeOrOffset] Sets place or offset of the selection.
* @param {Object} [options]
* @param {Boolean} [options.backward] Sets this selection instance to be backward.
*/
setTo( selectable, backwardSelectionOrOffset ) {
setTo( selectable, placeOrOffset, options ) {
if ( selectable === null ) {
this._setRanges( [] );
} else if ( selectable instanceof Selection ) {
this._setRanges( selectable.getRanges(), selectable.isBackward );
} else if ( selectable && selectable._selection instanceof Selection ) {
} else if ( selectable && typeof selectable.getRanges == 'function' ) {
// We assume that the selectable is a DocumentSelection.
// It can't be imported here, because it would lead to circular imports.
this._setRanges( selectable.getRanges(), selectable.isBackward );
} else if ( selectable instanceof Range ) {
this._setRanges( [ selectable ], backwardSelectionOrOffset );
this._setRanges( [ selectable ], !!placeOrOffset && !!placeOrOffset.backward );
} else if ( selectable instanceof Position ) {
this._setRanges( [ new Range( selectable ) ] );
} else if ( selectable instanceof Element ) {
this._setRanges( [ Range.createCollapsedAt( selectable, backwardSelectionOrOffset ) ] );
} else if ( selectable instanceof Node ) {
const backward = !!options && !!options.backward;
let range;

if ( placeOrOffset == 'in' ) {
range = Range.createIn( selectable );
} else if ( placeOrOffset == 'on' ) {
range = Range.createOn( selectable );
} else if ( placeOrOffset !== undefined ) {
range = Range.createCollapsedAt( selectable, placeOrOffset );
} else {
/**
* selection.setTo requires the second parameter when the first parameter is a node.
*
* @error model-selection-setTo-required-second-parameter
*/
throw new CKEditorError(
'model-selection-setTo-required-second-parameter: ' +
'selection.setTo requires the second parameter when the first parameter is a node.' );
}

this._setRanges( [ range ], backward );
} else if ( isIterable( selectable ) ) {
// We assume that the selectable is an iterable of ranges.
this._setRanges( selectable, backwardSelectionOrOffset );
this._setRanges( selectable, placeOrOffset && !!placeOrOffset.backward );
} else {
/**
* Cannot set selection to given place.
Expand Down
4 changes: 2 additions & 2 deletions src/model/utils/deletecontent.js
Original file line number Diff line number Diff line change
Expand Up @@ -194,9 +194,9 @@ function insertParagraph( writer, position, selection ) {
writer.insert( paragraph, position );

if ( selection instanceof DocumentSelection ) {
writer.setSelection( paragraph );
writer.setSelection( paragraph, 0 );
} else {
selection.setTo( paragraph );
selection.setTo( paragraph, 0 );
}
}

Expand Down
1 change: 1 addition & 0 deletions src/model/utils/modifyselection.js
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ export default function modifySelection( model, selection, options = {} ) {
const unit = options.unit ? options.unit : 'character';

const focus = selection.focus;

const walker = new TreeWalker( {
boundaries: getSearchRange( focus, isForward ),
singleCharacters: true,
Expand Down
45 changes: 30 additions & 15 deletions src/model/writer.js
Original file line number Diff line number Diff line change
Expand Up @@ -911,47 +911,62 @@ export default class Writer {
/**
* Sets this selection's ranges and direction to the specified location based on the given
* {@link module:engine/model/selection~Selection selection}, {@link module:engine/model/position~Position position},
* {@link module:engine/model/element~Element element}, {@link module:engine/model/position~Position position},
* {@link module:engine/model/element~Node node}, {@link module:engine/model/position~Position position},
* {@link module:engine/model/range~Range range}, an iterable of {@link module:engine/model/range~Range ranges} or null.
*
* // Sets ranges from the given range.
* // Sets selection to the given range.
* const range = new Range( start, end );
* writer.setSelection( range, isBackwardSelection );
* writer.setSelection( range );
*
* // Sets ranges from the iterable of ranges.
* // Sets selection to given ranges.
* const ranges = [ new Range( start1, end2 ), new Range( star2, end2 ) ];
* writer.setSelection( range, isBackwardSelection );
* writer.setSelection( range );
*
* // Sets ranges from the other selection.
* // Sets selection to other selection.
* const otherSelection = new Selection();
* writer.setSelection( otherSelection );
*
* // Sets ranges from the given document selection's ranges.
* // Sets selection to the given document selection.
* const documentSelection = new DocumentSelection( doc );
* writer.setSelection( documentSelection );
*
* // Sets collapsed range at the given position.
* // Sets collapsed selection at the given position.
* const position = new Position( root, path );
* writer.setSelection( position );
*
* // Sets collapsed range at the given offset in element.
* const paragraph = writer.createElement( 'paragraph' );
* // Sets collapsed selection at the position of the given node and an offset.
* writer.setSelection( paragraph, offset );
*
* // Removes all ranges.
* Creates a range inside an {@link module:engine/model/element~Element element} which starts before the first child of
* that element and ends after the last child of that element.
*
* writer.setSelection( paragraph, 'in' );
*
* Creates a range on an {@link module:engine/model/item~Item item} which starts before the item and ends just after the item.
*
* writer.setSelection( paragraph, 'on' );
*
* // Removes all selection's ranges.
* writer.setSelection( null );
*
* `Writer#setSelection()` allow passing additional options (`backward`) as the last argument.
*
* // Sets selection as backward.
* writer.setSelection( range, { backward: true } );
*
* Throws `writer-incorrect-use` error when the writer is used outside the `change()` block.
*
* @param {module:engine/model/selection~Selection|module:engine/model/documentselection~DocumentSelection|
* module:engine/model/position~Position|module:engine/model/element~Element|
* module:engine/model/position~Position|module:engine/model/node~Node|
* Iterable.<module:engine/model/range~Range>|module:engine/model/range~Range|null} selectable
* @param {Boolean|Number|'before'|'end'|'after'} [backwardSelectionOrOffset]
* @param {Number|'before'|'end'|'after'|'on'|'in'} [placeOrOffset] Sets place or offset of the selection.
* @param {Object} [options]
* @param {Boolean} [options.backward] Sets this selection instance to be backward.
*/
setSelection( selectable, backwardSelectionOrOffset ) {
setSelection( selectable, placeOrOffset, options ) {
this._assertWriterUsedCorrectly();

this.model.document.selection._setTo( selectable, backwardSelectionOrOffset );
this.model.document.selection._setTo( selectable, placeOrOffset, options );
}

/**
Expand Down
2 changes: 1 addition & 1 deletion src/view/domconverter.js
Original file line number Diff line number Diff line change
Expand Up @@ -486,7 +486,7 @@ export default class DomConverter {
}
}

return new ViewSelection( viewRanges, isBackward );
return new ViewSelection( viewRanges, { backward: isBackward } );
}

/**
Expand Down
Loading

0 comments on commit 3d25385

Please sign in to comment.