Browse Source

Improved docs, added tests for setMarker.

Maciej Bukowski 7 years ago
parent
commit
467f7c09a0

+ 20 - 6
packages/ckeditor5-engine/src/model/writer.js

@@ -774,11 +774,24 @@ export default class Writer {
 	 *
 	 * @param {module:engine/model/markercollection~Marker|String} markerOrName Marker or marker name to add or update.
 	 * @param {module:engine/model/range~Range} [newRange] Marker range.
+	 * @param {Object} [options]
+	 * @param {Boolean} [options.usingOperation=false] Flag indicated whether the marker should be added by MarkerOperation.
+	 * @returns {module:engine/model/markercollection~Marker} Marker that was set.
 	 */
 	setMarker( markerOrName, newRange, options = { usingOperation: false } ) {
 		this._assertWriterUsedCorrectly();
 
 		const name = typeof markerOrName == 'string' ? markerOrName : markerOrName.name;
+
+		if ( !options.usingOperation ) {
+			// Marker set without using operation should always have range.
+			if ( !newRange ) {
+				throw new CKEditorError( 'writer-setMarker-no-range: Range parameter is required when adding a new marker.' );
+			}
+
+			return this.model.markers.set( name, newRange );
+		}
+
 		const currentMarker = this.model.markers.get( name );
 
 		if ( !newRange && !currentMarker ) {
@@ -790,11 +803,6 @@ export default class Writer {
 			throw new CKEditorError( 'writer-setMarker-no-range: Range parameter is required when adding a new marker.' );
 		}
 
-		if ( !options.usingOperation ) {
-			const marker = this.model.markers.set( name, newRange );
-			return marker;
-		}
-
 		const currentRange = currentMarker ? currentMarker.getRange() : null;
 
 		if ( !newRange ) {
@@ -805,12 +813,16 @@ export default class Writer {
 			// Just change marker range.
 			addMarkerOperation( this, name, currentRange, newRange );
 		}
+
+		return this.model.markers.get( name );
 	}
 
 	/**
 	 * Removes given {@link module:engine/model/markercollection~Marker marker} or marker with given name.
 	 *
 	 * @param {module:engine/model/markercollection~Marker|String} markerOrName Marker or marker name to remove.
+	 * @param {Object} [options]
+	 * @param {Boolean} [options.usingOperation=false] Flag indicated whether the marker should be removed by MarkerOperation.
 	 */
 	removeMarker( markerOrName, options = { usingOperation: false } ) {
 		this._assertWriterUsedCorrectly();
@@ -827,7 +839,9 @@ export default class Writer {
 		}
 
 		if ( !options.usingOperation ) {
-			return this.model.markers.remove( name );
+			this.model.markers.remove( name );
+
+			return;
 		}
 
 		const oldRange = this.model.markers.get( name ).getRange();

+ 37 - 4
packages/ckeditor5-engine/tests/model/writer.js

@@ -1965,16 +1965,28 @@ describe( 'Writer', () => {
 		} );
 
 		it( 'should add marker to the document marker collection', () => {
-			setMarker( 'name', range, { usingOperation: true } );
+			setMarker( 'name', range );
 
 			expect( model.markers.get( 'name' ).getRange().isEqual( range ) ).to.be.true;
 		} );
 
+		it( 'should return marker if that was set directly', () => {
+			const marker = setMarker( 'name', range );
+
+			expect( model.markers.get( 'name' ) ).to.equal( marker );
+		} );
+
+		it( 'should return marker if that was set using operation API', () => {
+			const marker = setMarker( 'name', range, { usingOperation: true } );
+
+			expect( model.markers.get( 'name' ) ).to.equal( marker );
+		} );
+
 		it( 'should update marker in the document marker collection', () => {
-			setMarker( 'name', range, { usingOperation: true } );
+			setMarker( 'name', range );
 
 			const range2 = Range.createFromParentsAndOffsets( root, 0, root, 0 );
-			setMarker( 'name', range2, { usingOperation: true } );
+			setMarker( 'name', range2 );
 
 			expect( model.markers.get( 'name' ).getRange().isEqual( range2 ) ).to.be.true;
 		} );
@@ -1991,13 +2003,28 @@ describe( 'Writer', () => {
 			expect( op.newRange.isEqual( range2 ) ).to.be.true;
 		} );
 
-		it( 'should accept empty range parameter if marker instance is passed', () => {
+		it( 'should accept empty range parameter if marker instance is passed and usingOperation is set to true', () => {
 			const marker = setMarker( 'name', range );
 			const spy = sinon.spy();
 
 			model.on( 'applyOperation', spy );
 
 			setMarker( marker, null, { usingOperation: true } );
+
+			const op = batch.deltas[ 0 ].operations[ 0 ];
+
+			expect( spy.calledOnce ).to.be.true;
+			expect( spy.firstCall.args[ 1 ][ 0 ].type ).to.equal( 'marker' );
+			expect( op.oldRange ).to.be.null;
+			expect( op.newRange.isEqual( range ) ).to.be.true;
+		} );
+
+		it( 'should use operations when having set usingOperations to true', () => {
+			const spy = sinon.spy();
+
+			model.on( 'applyOperation', spy );
+
+			setMarker( 'name', range, { usingOperation: true } );
 			const op = batch.deltas[ 0 ].operations[ 0 ];
 
 			expect( spy.calledOnce ).to.be.true;
@@ -2008,6 +2035,12 @@ describe( 'Writer', () => {
 
 		it( 'should throw if marker with given name does not exist and range is not passed', () => {
 			expect( () => {
+				setMarker( 'name', null, { usingOperation: true } );
+			} ).to.throw( CKEditorError, /^writer-setMarker-no-range/ );
+		} );
+
+		it( 'should throw if marker is set directly and range is not passed', () => {
+			expect( () => {
 				setMarker( 'name' );
 			} ).to.throw( CKEditorError, /^writer-setMarker-no-range/ );
 		} );