Selaa lähdekoodia

Changed: Properly handle marker update with changes to be manged by operations.

Maciej Gołaszewski 7 vuotta sitten
vanhempi
commit
4f29a352f2

+ 29 - 17
packages/ckeditor5-engine/src/model/writer.js

@@ -884,7 +884,6 @@ export default class Writer {
 	 * @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.
 	 */
 	updateMarker( markerOrName, options ) {
 		this._assertWriterUsedCorrectly();
@@ -902,30 +901,43 @@ export default class Writer {
 			throw new CKEditorError( 'writer-updateMarker-marker-not-exists: Marker with provided name does not exists.' );
 		}
 
-		const range = options && options.range;
-
+		const newRange = options && options.range;
 		const hasUsingOperationDefined = !!options && typeof options.usingOperation == 'boolean';
-		const usingOperation = !!options && !!options.usingOperation; // : currentMarker.managedUsingOperations;
 
-		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 ( hasUsingOperationDefined && currentMarker.managedUsingOperations ) {
-				applyMarkerOperation( this, markerName, currentMarker.getRange(), null );
-			}
+		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 ( 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 );
 
-			this.model.markers._set( markerName, range, hasUsingOperationDefined ? usingOperation : currentMarker.managedUsingOperations );
+				// 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 );
+			}
 
 			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() );
+		// 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, currentMarker.getRange(), range );
+			this.model.markers._set( markerName, newRange );
 		}
 	}
 

+ 81 - 29
packages/ckeditor5-engine/tests/model/writer.js

@@ -2049,11 +2049,11 @@ describe( 'Writer', () => {
 			range = Range.createIn( root );
 		} );
 
-		it( 'should accept marker instance', () => {
+		it( 'should update managed marker\'s range by marker instance using operations', () => {
 			const marker = addMarker( 'name', { range, usingOperation: true } );
 			const range2 = Range.createFromParentsAndOffsets( root, 0, root, 0 );
 
-			updateMarker( marker, { range: range2, usingOperation: true } );
+			updateMarker( marker, { range: range2 } );
 
 			expect( batch.deltas.length ).to.equal( 2 );
 
@@ -2064,32 +2064,61 @@ describe( 'Writer', () => {
 			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 );
+		it( 'should update managed marker\'s range by marker name using operations', () => {
+			const marker = addMarker( 'name', { range, usingOperation: true } );
+			const range2 = Range.createFromParentsAndOffsets( root, 0, root, 0 );
 
-			const marker = addMarker( 'name', { range, usingOperation: false } );
+			updateMarker( 'name', { range: range2 } );
+
+			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 update managed marker\'s range by marker instance using operations and usingOperation explicitly passed', () => {
+			const marker = addMarker( 'name', { range, usingOperation: true } );
 			const range2 = Range.createFromParentsAndOffsets( root, 0, root, 0 );
 
-			updateMarker( marker, { range: range2, usingOperation: false } );
+			updateMarker( marker, { range: range2, usingOperation: true } );
 
-			sinon.assert.notCalled( spy );
+			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 accept empty range parameter if marker instance is passed and usingOperation is set to true', () => {
+		it( 'should update managed marker\'s range by marker name using operations and usingOperation explicitly passed', () => {
 			const marker = addMarker( 'name', { range, usingOperation: true } );
-			const spy = sinon.spy();
+			const range2 = Range.createFromParentsAndOffsets( root, 0, root, 0 );
+
+			updateMarker( 'name', { range: 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 );
 
-			updateMarker( marker, { usingOperation: true } );
+			const marker = addMarker( 'name', { range, usingOperation: false } );
+			const range2 = Range.createFromParentsAndOffsets( root, 0, root, 0 );
 
-			const op = batch.deltas[ 0 ].operations[ 0 ];
+			updateMarker( marker, { range: range2 } );
 
-			sinon.assert.calledOnce( spy );
-			expect( spy.firstCall.args[ 1 ][ 0 ].type ).to.equal( 'marker' );
-			expect( op.oldRange ).to.be.null;
-			expect( op.newRange.isEqual( range ) ).to.be.true;
+			sinon.assert.notCalled( spy );
 		} );
 
 		it( 'should create additional operation when marker type changes to not managed using operation', () => {
@@ -2097,7 +2126,7 @@ describe( 'Writer', () => {
 			model.on( 'applyOperation', spy );
 
 			addMarker( 'name', { range, usingOperation: true } );
-			updateMarker( 'name', { range, usingOperation: false } );
+			updateMarker( 'name', { usingOperation: false } );
 
 			const marker = model.markers.get( 'name' );
 
@@ -2117,20 +2146,32 @@ describe( 'Writer', () => {
 			expect( marker.managedUsingOperations ).to.be.false;
 		} );
 
-		it( 'should not create additional operation when using operation is not specified', () => {
+		it( 'should create additional operation when marker type changes to not managed using operation and changing its range', () => {
 			const spy = sinon.spy();
 			model.on( 'applyOperation', spy );
+			const range2 = Range.createFromParentsAndOffsets( root, 0, root, 0 );
 
 			addMarker( 'name', { range, usingOperation: true } );
+			updateMarker( 'name', { range: range2, usingOperation: false } );
 
-			sinon.assert.calledOnce( spy );
+			const marker = model.markers.get( 'name' );
 
-			updateMarker( 'name', { range } );
+			const op1 = batch.deltas[ 0 ].operations[ 0 ];
+			const op2 = batch.deltas[ 1 ].operations[ 0 ];
 
-			const marker = model.markers.get( 'name' );
+			sinon.assert.calledTwice( spy );
+			expect( spy.firstCall.args[ 1 ][ 0 ].type ).to.equal( 'marker' );
+			expect( spy.secondCall.args[ 1 ][ 0 ].type ).to.equal( 'marker' );
 
-			sinon.assert.calledOnce( spy );
-			expect( marker.managedUsingOperations ).to.be.true;
+			expect( op1.oldRange ).to.be.null;
+			expect( op1.newRange.isEqual( range ) ).to.be.true;
+
+			expect( op2.oldRange.isEqual( range ) ).to.be.true;
+			expect( op2.newRange ).to.be.null;
+
+			expect( marker.getRange().isEqual( range2 ) );
+
+			expect( marker.managedUsingOperations ).to.be.false;
 		} );
 
 		it( 'should enable changing marker to be managed using operation', () => {
@@ -2138,7 +2179,7 @@ describe( 'Writer', () => {
 			model.on( 'applyOperation', spy );
 
 			addMarker( 'name', { range, usingOperation: false } );
-			updateMarker( 'name', { range, usingOperation: true } );
+			updateMarker( 'name', { usingOperation: true } );
 
 			const marker = model.markers.get( 'name' );
 
@@ -2148,17 +2189,28 @@ describe( 'Writer', () => {
 			expect( marker.managedUsingOperations ).to.be.true;
 		} );
 
-		it( 'should enable changing marker to be not managed using operation', () => {
+		it( 'should enable changing marker to be managed using operation while changing range', () => {
 			const spy = sinon.spy();
 			model.on( 'applyOperation', spy );
+			const range2 = Range.createFromParentsAndOffsets( root, 0, root, 0 );
 
-			addMarker( 'name', { range, usingOperation: true } );
-			updateMarker( 'name', { range, usingOperation: false } );
+			addMarker( 'name', { range, usingOperation: false } );
+			updateMarker( 'name', { range: range2, usingOperation: true } );
 
 			const marker = model.markers.get( 'name' );
 
-			sinon.assert.calledTwice( spy );
-			expect( marker.managedUsingOperations ).to.be.false;
+			sinon.assert.calledOnce( spy );
+			expect( spy.firstCall.args[ 1 ][ 0 ].type ).to.equal( 'marker' );
+			expect( marker.getRange().isEqual( range2 ) ).to.be.true;
+
+			expect( marker.managedUsingOperations ).to.be.true;
+		} );
+
+		it( 'should throw when range and usingOperations were not provided', () => {
+			expect( () => {
+				addMarker( 'name', { range, usingOperation: false } );
+				updateMarker( 'name' );
+			} ).to.throw( CKEditorError, /^writer-updateMarker-wrong-options/ );
 		} );
 
 		it( 'should throw when marker provided by name does not exists', () => {