Merge pull request #1388 from ckeditor/t/1299

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.

src/model/delta/markerdelta.js

@@ -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

src/model/markercollection.js

@@ -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.
  *
@@ -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 } );
  *
  * // ...
  *
@@ -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}
  */

src/model/writer.js

@@ -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
@@ -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 } );
  }
  }
  }
@@ -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
@@ -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 );
  }
 
  /**

tests/controller/datacontroller.js

@@ -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 );
@@ -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 );

tests/controller/editingcontroller.js

@@ -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 } ) )
@@ -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 => {
@@ -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 } ) )
@@ -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 ] ) );
  } );
 
@@ -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 => {
@@ -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 => {
@@ -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 => {