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 #1388 from ckeditor/t/1299
Browse files Browse the repository at this point in the history 
Feature: Introduce `writer#updateMarker()` method. Closes #1299.

BREAKING CHANGE: The `writer#setMarker()` method is used only to create a new marker and it does not accept a `marker` instance as a parameter. To update existing marker use `writer#updateMarker()` method.
BREAKING CHANGE: The `options.usingOperation` option in `writer#setMarker()` is now a required one.
BREAKING CHANGE: The `range` parameter was removed. Use `options.range` instead.
  • Loading branch information
Piotr Jasiun authored Apr 6, 2018
2 parents 26bf582 + 49b99b4 commit bed647f
Show file tree
Hide file tree
Showing 13 changed files with 398 additions and 193 deletions.
4 changes: 2 additions & 2 deletions src/model/delta/markerdelta.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@ import Delta from './delta';
import DeltaFactory from './deltafactory';

/**
* To provide specific OT behavior and better collisions solving, the {@link module:engine/model/writer~Writer#setMarker Batch#setMarker}
* and {@link module:engine/model/writer~Writer#removeMarker Batch#removeMarker} methods use the `MarkerDelta` class which inherits
* To provide specific OT behavior and better collisions solving, the {@link module:engine/model/writer~Writer#addMarker Writer#addMarker}
* and {@link module:engine/model/writer~Writer#removeMarker Writer#removeMarker} methods use the `MarkerDelta` class which inherits
* from the `Delta` class and may overwrite some methods.
*
* @extends module:engine/model/delta/delta~Delta
Expand Down
8 changes: 4 additions & 4 deletions src/model/markercollection.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import mix from '@ckeditor/ckeditor5-utils/src/mix';
* {@link module:engine/model/markercollection~MarkerCollection#event:update} event.
*
* To create, change or remove makers use {@link module:engine/model/writer~Writer model writers'} methods:
* {@link module:engine/model/writer~Writer#setMarker} or {@link module:engine/model/writer~Writer#removeMarker}. Since
* {@link module:engine/model/writer~Writer#addMarker} or {@link module:engine/model/writer~Writer#removeMarker}. Since
* the writer is the only proper way to change the data model it is not possible to change markers directly using this
* collection. All markers created by the writer will be automatically added to this collection.
*
Expand Down Expand Up @@ -275,11 +275,11 @@ mix( MarkerCollection, EmitterMixin );
* Therefore, they are handled in the undo stack and synchronized between clients if the collaboration plugin is enabled.
* This type of markers is useful for solutions like spell checking or comments.
*
* Both type of them should be added / updated by {@link module:engine/model/writer~Writer#setMarker}
* Both type of them should be added / updated by {@link module:engine/model/writer~Writer#addMarker}
* and removed by {@link module:engine/model/writer~Writer#removeMarker} methods.
*
* model.change( ( writer ) => {
* const marker = writer.setMarker( name, range, { usingOperation: true } );
* const marker = writer.addMarker( name, { range, usingOperation: true } );
*
* // ...
*
Expand Down Expand Up @@ -343,7 +343,7 @@ class Marker {
/**
* Returns value of flag indicates if the marker is managed using operations or not.
* See {@link ~Marker marker class description} to learn more about marker types.
* See {@link module:engine/model/writer~Writer#setMarker}.
* See {@link module:engine/model/writer~Writer#addMarker}.
*
* @returns {Boolean}
*/
Expand Down
191 changes: 123 additions & 68 deletions src/model/writer.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@ import DocumentSelection from './documentselection';
import toMap from '@ckeditor/ckeditor5-utils/src/tomap';

import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror';
import uid from '@ckeditor/ckeditor5-utils/src/uid';

/**
* The model can only be modified by using the writer. It should be used whenever you want to create a node, modify
Expand Down Expand Up @@ -206,7 +205,7 @@ export default class Writer {
markerRange.end._getCombined( rangeRootPosition, position )
);

this.setMarker( markerName, range, { usingOperation: true } );
this.addMarker( markerName, { range, usingOperation: true } );
}
}
}
Expand Down Expand Up @@ -771,6 +770,78 @@ export default class Writer {
applyRemoveOperation( Position.createBefore( element ), 1, delta, this.model );
}

/**
* Adds a {@link module:engine/model/markercollection~Marker marker}. Marker is a named range, which tracks
* changes in the document and updates its range automatically, when model tree changes.
*
* As the first parameter you can set marker name.
*
* The required `options.usingOperation` parameter lets you decide if the marker should be managed by operations or not. See
* {@link module:engine/model/markercollection~Marker marker class description} to learn about the difference between
* markers managed by operations and not-managed by operations.
*
* Create marker directly base on marker's name:
*
* addMarker( markerName, { range, usingOperation: false } );
*
* Create marker using operation:
*
* addMarker( markerName, { range, usingOperation: true } );
*
* Note: For efficiency reasons, it's best to create and keep as little markers as possible.
*
* @see module:engine/model/markercollection~Marker
* @param {String} name Name of a marker to create - must be unique.
* @param {Object} options
* @param {Boolean} options.usingOperation Flag indicated whether the marker should be added by MarkerOperation.
* See {@link module:engine/model/markercollection~Marker#managedUsingOperations}.
* @param {module:engine/model/range~Range} options.range Marker range.
* @returns {module:engine/model/markercollection~Marker} Marker that was set.
*/
addMarker( name, options ) {
this._assertWriterUsedCorrectly();

if ( !options || typeof options.usingOperation != 'boolean' ) {
/**
* The options.usingOperations parameter is required when adding a new marker.
*
* @error writer-addMarker-no-usingOperations
*/
throw new CKEditorError(
'writer-addMarker-no-usingOperations: The options.usingOperations parameter is required when adding a new marker.'
);
}

const usingOperation = options.usingOperation;
const range = options.range;

if ( this.model.markers.has( name ) ) {
/**
* Marker with provided name already exists.
*
* @error writer-addMarker-marker-exists
*/
throw new CKEditorError( 'writer-addMarker-marker-exists: Marker with provided name already exists.' );
}

if ( !range ) {
/**
* Range parameter is required when adding a new marker.
*
* @error writer-addMarker-no-range
*/
throw new CKEditorError( 'writer-addMarker-no-range: Range parameter is required when adding a new marker.' );
}

if ( !usingOperation ) {
return this.model.markers._set( name, range, usingOperation );
}

applyMarkerOperation( this, name, null, range );

return this.model.markers.get( name );
}

/**
* Adds or updates a {@link module:engine/model/markercollection~Marker marker}. Marker is a named range, which tracks
* changes in the document and updates its range automatically, when model tree changes. Still, it is possible to change the
Expand All @@ -779,101 +850,85 @@ export default class Writer {
* As the first parameter you can set marker name or instance. If none of them is provided, new marker, with a unique
* name is created and returned.
*
* The `options.usingOperation` parameter lets you decide if the marker should be managed by operations or not. See
* The `options.usingOperation` parameter lets you change if the marker should be managed by operations or not. See
* {@link module:engine/model/markercollection~Marker marker class description} to learn about the difference between
* markers managed by operations and not-managed by operations. It is possible to change this option for an existing marker.
* This is useful when a marker have been created earlier and then later, it needs to be added to the document history.
*
* Create/update marker directly base on marker's name:
* Update marker directly base on marker's name:
*
* setMarker( markerName, range );
* updateMarker( markerName, { range } );
*
* Update marker using operation:
*
* setMarker( marker, range, { usingOperation: true } );
* setMarker( markerName, range, { usingOperation: true } );
*
* Create marker with a unique id using operation:
*
* setMarker( range, { usingOperation: true } );
*
* Create marker directly without using operations:
*
* setMarker( range )
* updateMarker( marker, { range, usingOperation: true } );
* updateMarker( markerName, { range, usingOperation: true } );
*
* Change marker's option (start using operations to manage it):
*
* setMarker( marker, { usingOperation: true } );
*
* Note: For efficiency reasons, it's best to create and keep as little markers as possible.
* updateMarker( marker, { usingOperation: true } );
*
* @see module:engine/model/markercollection~Marker
* @param {module:engine/model/markercollection~Marker|String} [markerOrName]
* Name of a marker to create or update, or `Marker` instance to update, or range for the marker with a unique name.
* @param {module:engine/model/range~Range} [range] Marker range.
* @param {Object} [options]
* @param {Boolean} [options.usingOperation=false] Flag indicated whether the marker should be added by MarkerOperation.
* @param {String} markerOrName Name of a marker to update, or a marker instance.
* @param {Object} options
* @param {module:engine/model/range~Range} [options.range] Marker range to update.
* @param {Boolean} [options.usingOperation] Flag indicated whether the marker should be added by MarkerOperation.
* See {@link module:engine/model/markercollection~Marker#managedUsingOperations}.
* @returns {module:engine/model/markercollection~Marker} Marker that was set.
*/
setMarker( markerOrNameOrRange, rangeOrOptions, options ) {
updateMarker( markerOrName, options ) {
this._assertWriterUsedCorrectly();

let markerName, newRange, usingOperation;
const markerName = typeof markerOrName == 'string' ? markerOrName : markerOrName.name;

if ( markerOrNameOrRange instanceof Range ) {
markerName = uid();
newRange = markerOrNameOrRange;
usingOperation = !!rangeOrOptions && !!rangeOrOptions.usingOperation;
} else {
markerName = typeof markerOrNameOrRange === 'string' ? markerOrNameOrRange : markerOrNameOrRange.name;
const currentMarker = this.model.markers.get( markerName );

if ( rangeOrOptions instanceof Range ) {
newRange = rangeOrOptions;
usingOperation = !!options && !!options.usingOperation;
} else {
newRange = null;
usingOperation = !!rangeOrOptions && !!rangeOrOptions.usingOperation;
}
if ( !currentMarker ) {
/**
* Marker with provided name does not exists.
*
* @error writer-updateMarker-marker-not-exists
*/
throw new CKEditorError( 'writer-updateMarker-marker-not-exists: Marker with provided name does not exists.' );
}

const currentMarker = this.model.markers.get( markerName );
const newRange = options && options.range;
const hasUsingOperationDefined = !!options && typeof options.usingOperation == 'boolean';

if ( !usingOperation ) {
if ( !newRange ) {
/**
* Range parameter is required when adding a new marker.
*
* @error writer-setMarker-no-range
*/
throw new CKEditorError( 'writer-setMarker-no-range: Range parameter is required when adding a new marker.' );
}
if ( !hasUsingOperationDefined && !newRange ) {
/**
* One of options is required - provide range or usingOperations.
*
* @error writer-updateMarker-wrong-options
*/
throw new CKEditorError( 'writer-updateMarker-wrong-options: One of options is required - provide range or usingOperations.' );
}

// If marker changes to marker that do not use operations then we need to create additional operation
// that removes that marker first.
if ( currentMarker && currentMarker.managedUsingOperations && !usingOperation ) {
applyMarkerOperation( this, markerName, currentMarker.getRange(), null );
}
if ( hasUsingOperationDefined && options.usingOperation !== currentMarker.managedUsingOperations ) {
// The marker type is changed so it's necessary to create proper operations.
if ( options.usingOperation ) {
// If marker changes to a managed one treat this as synchronizing existing marker.
// If marker changes to a managed one treat this as synchronizing existing marker.
// Create `MarkerOperation` with `oldRange` set to `null`, so reverse operation will remove the marker.
applyMarkerOperation( this, markerName, null, newRange ? newRange : currentMarker.getRange() );
} else {
// If marker changes to a marker that do not use operations then we need to create additional operation
// that removes that marker first.
const currentRange = currentMarker.getRange();
applyMarkerOperation( this, markerName, currentRange, null );

return this.model.markers._set( markerName, newRange, usingOperation );
}
// Although not managed the marker itself should stay in model and its range should be preserver or changed to passed range.
this.model.markers._set( markerName, newRange ? newRange : currentRange );
}

if ( !newRange && !currentMarker ) {
throw new CKEditorError( 'writer-setMarker-no-range: Range parameter is required when adding a new marker.' );
return;
}

const currentRange = currentMarker ? currentMarker.getRange() : null;

if ( !newRange ) {
// If `newRange` is not given, treat this as synchronizing existing marker.
// Create `MarkerOperation` with `oldRange` set to `null`, so reverse operation will remove the marker.
applyMarkerOperation( this, markerName, null, currentRange );
// Marker's type doesn't change so update it accordingly.
if ( currentMarker.managedUsingOperations ) {
applyMarkerOperation( this, markerName, currentMarker.getRange(), newRange );
} else {
// Just change marker range.
applyMarkerOperation( this, markerName, currentRange, newRange );
this.model.markers._set( markerName, newRange );
}

return this.model.markers.get( markerName );
}

/**
Expand Down
10 changes: 7 additions & 3 deletions tests/controller/datacontroller.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -406,7 +406,8 @@ describe( 'DataController', () => {

model.change( writer => {
writer.insert( modelElement, modelRoot, 0 );
writer.setMarker( 'marker:a', Range.createFromParentsAndOffsets( modelRoot, 0, modelRoot, 1 ), { usingOperation: true } );
const range = Range.createFromParentsAndOffsets( modelRoot, 0, modelRoot, 1 );
writer.addMarker( 'marker:a', { range, usingOperation: true } );
} );

const viewDocumentFragment = data.toView( modelElement );
Expand All @@ -428,8 +429,11 @@ describe( 'DataController', () => {
model.change( writer => {
writer.insert( modelElement, modelRoot, 0 );

writer.setMarker( 'marker:a', Range.createFromParentsAndOffsets( modelP1, 1, modelP1, 3 ), { usingOperation: true } );
writer.setMarker( 'marker:b', Range.createFromParentsAndOffsets( modelP2, 0, modelP2, 2 ), { usingOperation: true } );
const rangeA = Range.createFromParentsAndOffsets( modelP1, 1, modelP1, 3 );
const rangeB = Range.createFromParentsAndOffsets( modelP2, 0, modelP2, 2 );

writer.addMarker( 'marker:a', { range: rangeA, usingOperation: true } );
writer.addMarker( 'marker:b', { range: rangeB, usingOperation: true } );
} );

const viewDocumentFragment = data.toView( modelP1 );
Expand Down
16 changes: 8 additions & 8 deletions tests/controller/editingcontroller.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,7 @@ describe( 'EditingController', () => {
const range = new ModelRange( new ModelPosition( modelRoot, [ 0, 1 ] ), new ModelPosition( modelRoot, [ 2, 2 ] ) );

model.change( writer => {
writer.setMarker( 'marker', range );
writer.addMarker( 'marker', { range, usingOperation: false } );
} );

expect( getViewData( editing.view, { withoutSelection: true } ) )
Expand All @@ -246,7 +246,7 @@ describe( 'EditingController', () => {
const range = new ModelRange( new ModelPosition( modelRoot, [ 0, 1 ] ), new ModelPosition( modelRoot, [ 2, 2 ] ) );

model.change( writer => {
writer.setMarker( 'marker', range );
writer.addMarker( 'marker', { range, usingOperation: false } );
} );

model.change( writer => {
Expand All @@ -261,13 +261,13 @@ describe( 'EditingController', () => {
const range = new ModelRange( new ModelPosition( modelRoot, [ 0, 1 ] ), new ModelPosition( modelRoot, [ 2, 2 ] ) );

model.change( writer => {
writer.setMarker( 'marker', range );
writer.addMarker( 'marker', { range, usingOperation: false } );
} );

const range2 = new ModelRange( new ModelPosition( modelRoot, [ 0, 0 ] ), new ModelPosition( modelRoot, [ 0, 2 ] ) );

model.change( writer => {
writer.setMarker( 'marker', range2 );
writer.updateMarker( 'marker', { range: range2 } );
} );

expect( getViewData( editing.view, { withoutSelection: true } ) )
Expand All @@ -278,7 +278,7 @@ describe( 'EditingController', () => {
const range = new ModelRange( new ModelPosition( modelRoot, [ 0, 1 ] ), new ModelPosition( modelRoot, [ 2, 2 ] ) );

model.change( writer => {
writer.setMarker( 'marker', range );
writer.addMarker( 'marker', { range, usingOperation: false } );
writer.insertText( 'xyz', new ModelPosition( modelRoot, [ 1, 0 ] ) );
} );

Expand All @@ -290,7 +290,7 @@ describe( 'EditingController', () => {
const range = new ModelRange( new ModelPosition( modelRoot, [ 0, 1 ] ), new ModelPosition( modelRoot, [ 2, 2 ] ) );

model.change( writer => {
writer.setMarker( 'marker', range );
writer.addMarker( 'marker', { range, usingOperation: false } );
} );

model.change( writer => {
Expand All @@ -308,7 +308,7 @@ describe( 'EditingController', () => {
const range = new ModelRange( new ModelPosition( modelRoot, [ 0, 1 ] ), new ModelPosition( modelRoot, [ 2, 2 ] ) );

model.change( writer => {
writer.setMarker( 'marker', range );
writer.addMarker( 'marker', { range, usingOperation: false } );
} );

model.change( writer => {
Expand All @@ -326,7 +326,7 @@ describe( 'EditingController', () => {
const range = new ModelRange( new ModelPosition( modelRoot, [ 0, 1 ] ), new ModelPosition( modelRoot, [ 0, 3 ] ) );

model.change( writer => {
writer.setMarker( 'marker', range );
writer.addMarker( 'marker', { range, usingOperation: false } );
} );

model.change( writer => {
Expand Down
Loading

0 comments on commit bed647f

Please sign in to comment.