ソースを参照

Added: Create writer.updateMarker() method.

Maciej Gołaszewski 7 年 前
コミット
d47e215024

+ 51 - 27
packages/ckeditor5-engine/src/model/writer.js

@@ -827,35 +827,31 @@ export default class Writer {
 	 * @returns {module:engine/model/markercollection~Marker} Marker that was set.
 	 */
 	setMarker( nameOrRange, rangeOrOptions, options ) {
-		// TODO: change method signature - optional is first???
 		this._assertWriterUsedCorrectly();
 
-		let markerName, newRange, usingOperation;
+		let markerName, range, usingOperation;
 
-		// TODO: method me ?
 		if ( nameOrRange instanceof Range ) {
 			markerName = uid();
-			newRange = nameOrRange;
+			range = nameOrRange;
 			usingOperation = _checkUsingOptionsIsDefined( rangeOrOptions );
 		} else {
 			markerName = nameOrRange;
 
 			if ( rangeOrOptions instanceof Range ) {
-				newRange = rangeOrOptions;
+				range = rangeOrOptions;
 				usingOperation = _checkUsingOptionsIsDefined( options );
 			} else {
-				newRange = null;
+				range = null;
 				usingOperation = _checkUsingOptionsIsDefined( rangeOrOptions );
 			}
 		}
 
-		const currentMarker = this.model.markers.get( markerName );
-
-		if ( currentMarker ) {
+		if ( this.model.markers.has( markerName ) ) {
 			throw new CKEditorError( 'writer-setMarker-marker-exists: Marker with provided name already exists.' );
 		}
 
-		if ( !newRange ) {
+		if ( !range ) {
 			/**
 			 * Range parameter is required when adding a new marker.
 			 *
@@ -865,25 +861,10 @@ export default class Writer {
 		}
 
 		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 ) {
-				applyMarkerOperation( this, markerName, currentMarker.getRange(), null );
-			}
-
-			return this.model.markers._set( markerName, newRange, usingOperation );
+			return this.model.markers._set( markerName, range, usingOperation );
 		}
 
-		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 );
-		} else {
-			// Just change marker range.
-			applyMarkerOperation( this, markerName, currentRange, newRange );
-		}
+		applyMarkerOperation( this, markerName, null, range );
 
 		return this.model.markers.get( markerName );
 
@@ -903,6 +884,49 @@ export default class Writer {
 		}
 	}
 
+	updateMarker( markerOrName, rangeOrOptions, options ) {
+		this._assertWriterUsedCorrectly();
+
+		const markerName = typeof markerOrName == 'string' ? markerOrName : markerOrName.name;
+
+		const currentMarker = this.model.markers.get( markerName );
+
+		if ( !currentMarker ) {
+			throw new CKEditorError( 'writer-updateMarker-marker-not-exists: Marker with provided name does not exists.' );
+		}
+
+		let range, usingOperation;
+
+		if ( rangeOrOptions instanceof Range ) {
+			range = rangeOrOptions;
+			usingOperation = !!options && !!options.usingOperation;
+		} else {
+			range = null;
+			usingOperation = !!rangeOrOptions && !!rangeOrOptions.usingOperation;
+		}
+
+		if ( !usingOperation ) {
+			// If marker changes to a marker that do not use operations then we need to create additional operation
+			// that removes that marker first.
+			if ( currentMarker.managedUsingOperations ) {
+				applyMarkerOperation( this, markerName, currentMarker.getRange(), null );
+			}
+
+			this.model.markers._set( markerName, range, usingOperation );
+
+			return;
+		}
+
+		if ( !range ) {
+			// If `range` 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, currentMarker.getRange() );
+		} else {
+			// Just change marker range.
+			applyMarkerOperation( this, markerName, currentMarker.getRange(), range );
+		}
+	}
+
 	/**
 	 * Removes given {@link module:engine/model/markercollection~Marker marker} or marker with given name.
 	 * The marker is removed accordingly to how it has been created, so if the marker was created using operation,

+ 97 - 40
packages/ckeditor5-engine/tests/model/writer.js

@@ -2010,39 +2010,6 @@ describe( 'Writer', () => {
 			} ).to.throw( CKEditorError, /^writer-setMarker-marker-exists/ );
 		} );
 
-		// 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 );
-
-			setMarker( marker, range2, { usingOperation: true } );
-
-			expect( batch.deltas.length ).to.equal( 2 );
-
-			const op = batch.deltas[ 1 ].operations[ 0 ];
-
-			expect( marker.getRange().isEqual( range2 ) ).to.be.true;
-			expect( op.oldRange.isEqual( range ) ).to.be.true;
-			expect( op.newRange.isEqual( range2 ) ).to.be.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();
-
-			model.on( 'applyOperation', spy );
-
-			setMarker( marker, { 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 create a unique id if the first param is type of range', () => {
 			const marker = setMarker( range, { usingOperation: false } );
 
@@ -2085,13 +2052,77 @@ describe( 'Writer', () => {
 			} ).to.throw( CKEditorError, /^writer-setMarker-no-range/ );
 		} );
 
-		// TODO: move to updateMarker
-		it.skip( 'should create additional operation when marker type changes to not managed using operation', () => {
+		it( 'should throw when trying to use detached writer', () => {
+			const marker = setMarker( 'name', range, { usingOperation: false } );
+			const writer = new Writer( model, batch );
+
+			expect( () => {
+				writer.setMarker( marker, null, { usingOperation: true } );
+			} ).to.throw( CKEditorError, /^writer-incorrect-use/ );
+		} );
+	} );
+
+	describe( 'updateMarker()', () => {
+		let root, range;
+
+		beforeEach( () => {
+			root = doc.createRoot();
+			root._appendChildren( new Text( 'foo' ) );
+			range = Range.createIn( root );
+		} );
+
+		it( 'should accept marker instance', () => {
+			const marker = setMarker( 'name', range, { usingOperation: true } );
+			const range2 = Range.createFromParentsAndOffsets( root, 0, root, 0 );
+
+			updateMarker( marker, range2, { usingOperation: true } );
+
+			expect( batch.deltas.length ).to.equal( 2 );
+
+			const op = batch.deltas[ 1 ].operations[ 0 ];
+
+			expect( marker.getRange().isEqual( range2 ) ).to.be.true;
+			expect( op.oldRange.isEqual( range ) ).to.be.true;
+			expect( op.newRange.isEqual( range2 ) ).to.be.true;
+		} );
+
+		it( 'should not use operations when updating marker which does not use operations', () => {
 			const spy = sinon.spy();
 			model.on( 'applyOperation', spy );
 
-			setMarker( 'name', range, { usingOperation: true } );
 			const marker = setMarker( 'name', range, { usingOperation: false } );
+			const range2 = Range.createFromParentsAndOffsets( root, 0, root, 0 );
+
+			updateMarker( marker, range2, { usingOperation: false } );
+
+			sinon.assert.notCalled( spy );
+		} );
+
+		it( '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();
+
+			model.on( 'applyOperation', spy );
+
+			updateMarker( marker, { 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 create additional operation when marker type changes to not managed using operation', () => {
+			const spy = sinon.spy();
+			model.on( 'applyOperation', spy );
+
+			setMarker( 'name', range, { usingOperation: true } );
+			updateMarker( 'name', range );
+
+			const marker = model.markers.get( 'name' );
+
 			const op1 = batch.deltas[ 0 ].operations[ 0 ];
 			const op2 = batch.deltas[ 1 ].operations[ 0 ];
 
@@ -2108,13 +2139,14 @@ describe( 'Writer', () => {
 			expect( marker.managedUsingOperations ).to.be.false;
 		} );
 
-		// TODO: move to updateMarker
-		it.skip( 'should enable changing marker to be not managed using operation', () => {
+		it( 'should enable changing marker to be managed using operation', () => {
 			const spy = sinon.spy();
 			model.on( 'applyOperation', spy );
 
 			setMarker( 'name', range, { usingOperation: false } );
-			const marker = setMarker( 'name', range, { usingOperation: true } );
+			updateMarker( 'name', range, { usingOperation: true } );
+
+			const marker = model.markers.get( 'name' );
 
 			expect( spy.calledOnce ).to.be.true;
 			expect( spy.firstCall.args[ 1 ][ 0 ].type ).to.equal( 'marker' );
@@ -2122,12 +2154,31 @@ describe( 'Writer', () => {
 			expect( marker.managedUsingOperations ).to.be.true;
 		} );
 
+		it( 'should enable changing marker to be not managed using operation', () => {
+			const spy = sinon.spy();
+			model.on( 'applyOperation', spy );
+
+			setMarker( 'name', range, { usingOperation: true } );
+			updateMarker( 'name', range, { usingOperation: false } );
+
+			const marker = model.markers.get( 'name' );
+
+			sinon.assert.calledTwice( spy );
+			expect( marker.managedUsingOperations ).to.be.false;
+		} );
+
+		it( 'should throw when marker provided by name does not exists', () => {
+			expect( () => {
+				updateMarker( 'name', { usingOperation: false } );
+			} ).to.throw( CKEditorError, /^writer-updateMarker-marker-not-exists/ );
+		} );
+
 		it( 'should throw when trying to use detached writer', () => {
 			const marker = setMarker( 'name', range, { usingOperation: false } );
 			const writer = new Writer( model, batch );
 
 			expect( () => {
-				writer.setMarker( marker, null, { usingOperation: true } );
+				writer.updateMarker( marker, { usingOperation: true } );
 			} ).to.throw( CKEditorError, /^writer-incorrect-use/ );
 		} );
 	} );
@@ -2577,6 +2628,12 @@ describe( 'Writer', () => {
 		return marker;
 	}
 
+	function updateMarker( markerOrNameOrRange, rangeOrManagedUsingOperations, options ) {
+		model.enqueueChange( batch, writer => {
+			writer.updateMarker( markerOrNameOrRange, rangeOrManagedUsingOperations, options );
+		} );
+	}
+
 	function removeMarker( markerOrName, options ) {
 		model.enqueueChange( batch, writer => {
 			writer.removeMarker( markerOrName, options );