Changed: The writer.setMarker() method should not update the existing…

… marker.
ckeditor · Mar 28, 2018 · c71470c · c71470c
1 parent e143b41
commit c71470c
Show file tree

Hide file tree

Showing 2 changed files with 42 additions and 30 deletions.
diff --git a/src/model/writer.js b/src/model/writer.js
@@ -818,25 +818,27 @@ export default class Writer {
 	 * Note: For efficiency reasons, it's best to create and keep as little markers as possible.
 	 *
 	 * @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} [name]
+	 * Name of a marker to create or 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 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 ) {
+	setMarker( nameOrRange, rangeOrOptions, options ) {
+		// TODO: change method signature - optional is first???
 		this._assertWriterUsedCorrectly();
 
 		let markerName, newRange, usingOperation;
 
-		if ( markerOrNameOrRange instanceof Range ) {
+		// TODO: method me ?
+		if ( nameOrRange instanceof Range ) {
 			markerName = uid();
-			newRange = markerOrNameOrRange;
+			newRange = nameOrRange;
 			usingOperation = _checkUsingOptionsIsDefined( rangeOrOptions );
 		} else {
-			markerName = typeof markerOrNameOrRange === 'string' ? markerOrNameOrRange : markerOrNameOrRange.name;
+			markerName = nameOrRange;
 
 			if ( rangeOrOptions instanceof Range ) {
 				newRange = rangeOrOptions;
@@ -849,16 +851,20 @@ export default class Writer {
 
 		const currentMarker = this.model.markers.get( markerName );
 
-		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 ( currentMarker ) {
+			throw new CKEditorError( 'writer-setMarker-marker-exists: Marker with provided name already exists.' );
+		}
 
+		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 ( !usingOperation ) {
 			// 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 ) {
@@ -868,10 +874,6 @@ export default class Writer {
 			return this.model.markers._set( markerName, newRange, usingOperation );
 		}
 
-		if ( !newRange && !currentMarker ) {
-			throw new CKEditorError( 'writer-setMarker-no-range: Range parameter is required when adding a new marker.' );
-		}
-
 		const currentRange = currentMarker ? currentMarker.getRange() : null;
 
 		if ( !newRange ) {
@@ -887,6 +889,11 @@ export default class Writer {
 
 		function _checkUsingOptionsIsDefined( options ) {
 			if ( !options || typeof options.usingOperation != 'boolean' ) {
+				/**
+				 * The options.usingOperations parameter is required when adding a new marker.
+				 *
+				 * @error writer-setMarker-no-usingOperations
+				 */
 				throw new CKEditorError(
 					'writer-setMarker-no-usingOperations: The options.usingOperations parameter is required when adding a new marker.'
 				);

diff --git a/tests/model/writer.js b/tests/model/writer.js
@@ -2000,16 +2000,18 @@ describe( 'Writer', () => {
 			expect( marker.managedUsingOperations ).to.equal( false );
 		} );
 
-		it( 'should update marker in the document marker collection', () => {
-			const marker = setMarker( 'name', range, { usingOperation: false } );
+		it( 'should throw when trying to update existing marker in the document marker collection', () => {
+			setMarker( 'name', range, { usingOperation: false } );
 
 			const range2 = Range.createFromParentsAndOffsets( root, 0, root, 0 );
-			setMarker( 'name', range2, { usingOperation: false } );
 
-			expect( marker.getRange().isEqual( range2 ) ).to.be.true;
+			expect( () => {
+				setMarker( 'name', range2, { usingOperation: false } );
+			} ).to.throw( CKEditorError, /^writer-setMarker-marker-exists/ );
 		} );
 
-		it( 'should accept marker instance', () => {
+		// TODO: move to updateMarker
+		it.skip( 'should accept marker instance', () => {
 			const marker = setMarker( 'name', range, { usingOperation: true } );
 			const range2 = Range.createFromParentsAndOffsets( root, 0, root, 0 );
 
@@ -2024,7 +2026,8 @@ describe( 'Writer', () => {
 			expect( op.newRange.isEqual( range2 ) ).to.be.true;
 		} );
 
-		it( 'should accept empty range parameter if marker instance is passed and usingOperation is set to true', () => {
+		// TODO: move to updateMarker
+		it.skip( 'should accept empty range parameter if marker instance is passed and usingOperation is set to true', () => {
 			const marker = setMarker( 'name', range, { usingOperation: true } );
 			const spy = sinon.spy();
 
@@ -2082,7 +2085,8 @@ describe( 'Writer', () => {
 			} ).to.throw( CKEditorError, /^writer-setMarker-no-range/ );
 		} );
 
-		it( 'should create additional operation when marker type changes to not managed using operation', () => {
+		// TODO: move to updateMarker
+		it.skip( 'should create additional operation when marker type changes to not managed using operation', () => {
 			const spy = sinon.spy();
 			model.on( 'applyOperation', spy );
 
@@ -2104,7 +2108,8 @@ describe( 'Writer', () => {
 			expect( marker.managedUsingOperations ).to.be.false;
 		} );
 
-		it( 'should enable changing marker to be not managed using operation', () => {
+		// TODO: move to updateMarker
+		it.skip( 'should enable changing marker to be not managed using operation', () => {
 			const spy = sinon.spy();
 			model.on( 'applyOperation', spy );