Selaa lähdekoodia

Provided support for `affectsData` prooperty in `Writer#addMarker()` and `Writer#updateMarker()`.

Maciej Bukowski 7 vuotta sitten
vanhempi
commit
5f550100e9

+ 1 - 1
packages/ckeditor5-engine/src/model/markercollection.js

@@ -109,7 +109,7 @@ export default class MarkerCollection {
 				hasChanged = true;
 			}
 
-			if ( affectsData != oldMarker.affectsData ) {
+			if ( typeof affectsData === 'boolean' && affectsData != oldMarker.affectsData ) {
 				oldMarker._affectsData = affectsData;
 				hasChanged = true;
 			}

+ 1 - 1
packages/ckeditor5-engine/src/model/operation/markeroperation.js

@@ -98,7 +98,7 @@ export default class MarkerOperation extends Operation {
 	_execute() {
 		const type = this.newRange ? '_set' : '_remove';
 
-		this._markers[ type ]( this.name, this.newRange, true );
+		this._markers[ type ]( this.name, this.newRange, true, this.affectsData );
 	}
 
 	/**

+ 50 - 19
packages/ckeditor5-engine/src/model/writer.js

@@ -794,6 +794,10 @@ export default class Writer {
 	 * {@link module:engine/model/markercollection~Marker marker class description} to learn about the difference between
 	 * markers managed by operations and not-managed by operations.
 	 *
+	 * The `options.affectData` parameter, which defaults to true, allows you to create marker, that doesn't affects the data model.
+	 * It impacts on performance, because changes in markers with set `options.affectData:false` don't cause firing the
+	 * {@link module:engine/model/document#change:data `change:data`} event.
+	 *
 	 * Create marker directly base on marker's name:
 	 *
 	 *		addMarker( markerName, { range, usingOperation: false } );
@@ -802,14 +806,19 @@ export default class Writer {
 	 *
 	 *		addMarker( markerName, { range, usingOperation: true } );
 	 *
+	 * Create marker that doesn't affects the data model:
+	 *
+	 *		addMarker( markerName, { range, usingOperation: false, affectData: false } );
+	 *
 	 * 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.
+	 * @param {Boolean} options.usingOperation Flag indicating that 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.
+	 * @param {Boolean} [options.affectsData=true] Flag indicating that the marker changes the data model.
 	 * @returns {module:engine/model/markercollection~Marker} Marker that was set.
 	 */
 	addMarker( name, options ) {
@@ -828,6 +837,7 @@ export default class Writer {
 
 		const usingOperation = options.usingOperation;
 		const range = options.range;
+		const affectsData = options.affectsData === undefined ? true : options.affectsData;
 
 		if ( this.model.markers.has( name ) ) {
 			/**
@@ -848,10 +858,10 @@ export default class Writer {
 		}
 
 		if ( !usingOperation ) {
-			return this.model.markers._set( name, range, usingOperation );
+			return this.model.markers._set( name, range, usingOperation, affectsData );
 		}
 
-		applyMarkerOperation( this, name, null, range );
+		applyMarkerOperation( this, name, null, range, affectsData );
 
 		return this.model.markers.get( name );
 	}
@@ -867,7 +877,10 @@ export default class Writer {
 	 * 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.
+	 *
+	 * The `options.affectData` parameter allows you to inform the engine, that this marker doesn't affects the data model.
+	 * It impacts on performance, because changes in markers with set `options.affectData:false` don't cause firing the
+	 * {@link module:engine/model/document#change:data `change:data`} event.
 	 *
 	 * Update marker directly base on marker's name:
 	 *
@@ -882,18 +895,22 @@ export default class Writer {
 	 *
 	 *		updateMarker( marker, { usingOperation: true } );
 	 *
+	 * Change marker's option (inform engine, that marker don't affect's the data model):
+	 *
+	 *		updateMarker( markerName, { affectData: false } );
+	 *
 	 * @see module:engine/model/markercollection~Marker
 	 * @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}.
+	 * @param {Boolean} [options.affectsData] Flag indicating that the marker changes the data model.
 	 */
 	updateMarker( markerOrName, options ) {
 		this._assertWriterUsedCorrectly();
 
 		const markerName = typeof markerOrName == 'string' ? markerOrName : markerOrName.name;
-
 		const currentMarker = this.model.markers.get( markerName );
 
 		if ( !currentMarker ) {
@@ -905,33 +922,46 @@ export default class Writer {
 			throw new CKEditorError( 'writer-updateMarker-marker-not-exists: Marker with provided name does not exists.' );
 		}
 
-		const newRange = options && options.range;
-		const hasUsingOperationDefined = !!options && typeof options.usingOperation == 'boolean';
+		options = options || {};
 
-		if ( !hasUsingOperationDefined && !newRange ) {
+		const hasUsingOperationDefined = typeof options.usingOperation == 'boolean';
+		const affectsDataDefined = typeof options.affectsData == 'boolean';
+
+		// Use marker's affectsData property if this option is not provided.
+		const affectsData = affectsDataDefined ? options.affectsData : currentMarker.affectsData;
+
+		// if ( affectsData === undefined ) {
+		// 	affectsData = true;
+		// }
+
+		if ( !hasUsingOperationDefined && !options.range && !affectsDataDefined ) {
 			/**
-			 * One of options is required - provide range or usingOperations.
+			 * One of options is required - provide range, usingOperations or affectsData.
 			 *
 			 * @error writer-updateMarker-wrong-options
 			 */
-			throw new CKEditorError( 'writer-updateMarker-wrong-options: One of options is required - provide range or usingOperations.' );
+			throw new CKEditorError(
+				'writer-updateMarker-wrong-options: One of options is required - provide range, usingOperations or affectsData.'
+			);
 		}
 
+		const currentRange = currentMarker.getRange();
+		const updatedRange = options.range ? options.range : currentRange;
+
 		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() );
+				applyMarkerOperation( this, markerName, null, updatedRange, affectsData );
 			} 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 );
+				applyMarkerOperation( this, markerName, currentRange, null, affectsData );
 
 				// 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 );
+				this.model.markers._set( markerName, updatedRange, undefined, affectsData );
 			}
 
 			return;
@@ -939,9 +969,9 @@ export default class Writer {
 
 		// Marker's type doesn't change so update it accordingly.
 		if ( currentMarker.managedUsingOperations ) {
-			applyMarkerOperation( this, markerName, currentMarker.getRange(), newRange );
+			applyMarkerOperation( this, markerName, currentRange, updatedRange, affectsData );
 		} else {
-			this.model.markers._set( markerName, newRange );
+			this.model.markers._set( markerName, updatedRange, undefined, affectsData );
 		}
 	}
 
@@ -976,7 +1006,7 @@ export default class Writer {
 
 		const oldRange = marker.getRange();
 
-		applyMarkerOperation( this, name, oldRange, null );
+		applyMarkerOperation( this, name, oldRange, null, marker.affectsData );
 	}
 
 	/**
@@ -1325,12 +1355,13 @@ function setAttributeOnItem( writer, key, value, item ) {
 // @param {String} name Marker name.
 // @param {module:engine/model/range~Range} oldRange Marker range before the change.
 // @param {module:engine/model/range~Range} newRange Marker range after the change.
-function applyMarkerOperation( writer, name, oldRange, newRange ) {
+// @param {Boolean} affectsData
+function applyMarkerOperation( writer, name, oldRange, newRange, affectsData ) {
 	const model = writer.model;
 	const doc = model.document;
 	const delta = new MarkerDelta();
 
-	const operation = new MarkerOperation( name, oldRange, newRange, model.markers, doc.version );
+	const operation = new MarkerOperation( name, oldRange, newRange, model.markers, doc.version, affectsData );
 
 	writer.batch.addDelta( delta );
 	delta.addOperation( operation );

+ 77 - 2
packages/ckeditor5-engine/tests/model/writer.js

@@ -1975,13 +1975,25 @@ describe( 'Writer', () => {
 		it( 'should return marker with properly set managedUsingOperations (to true)', () => {
 			const marker = addMarker( 'name', { range, usingOperation: true } );
 
-			expect( marker.managedUsingOperations ).to.equal( true );
+			expect( marker.managedUsingOperations ).to.be.true;
 		} );
 
 		it( 'should return marker with properly set managedUsingOperations (to false)', () => {
 			const marker = addMarker( 'name', { range, usingOperation: false } );
 
-			expect( marker.managedUsingOperations ).to.equal( false );
+			expect( marker.managedUsingOperations ).to.be.false;
+		} );
+
+		it( 'should return marker with properly set affectsData (default to true)', () => {
+			const marker = addMarker( 'name', { range, usingOperation: false } );
+
+			expect( marker.affectsData ).to.be.true;
+		} );
+
+		it( 'should return marker with properly set affectsData (to false)', () => {
+			const marker = addMarker( 'name', { range, usingOperation: false, affectsData: false } );
+
+			expect( marker.affectsData ).to.be.false;
 		} );
 
 		it( 'should throw when trying to update existing marker in the document marker collection', () => {
@@ -2196,6 +2208,69 @@ describe( 'Writer', () => {
 			expect( marker.managedUsingOperations ).to.be.true;
 		} );
 
+		it( 'should allow changing affectsData property not using operations', () => {
+			addMarker( 'name', { range, usingOperation: false } );
+			updateMarker( 'name', { affectsData: false } );
+
+			const marker = model.markers.get( 'name' );
+
+			expect( marker.affectsData ).to.be.false;
+		} );
+
+		it( 'should allow changing affectsData property using operations', () => {
+			addMarker( 'name', { range, usingOperation: true } );
+			updateMarker( 'name', { affectsData: false } );
+
+			const op1 = batch.deltas[ 0 ].operations[ 0 ];
+			const op2 = batch.deltas[ 1 ].operations[ 0 ];
+			const marker = model.markers.get( 'name' );
+
+			expect( op1.affectsData ).to.be.true;
+			expect( op2.affectsData ).to.be.false;
+
+			expect( marker.affectsData ).to.be.false;
+		} );
+
+		it( 'should not change affectsData property if not provided', () => {
+			const range2 = Range.createFromParentsAndOffsets( root, 0, root, 0 );
+
+			addMarker( 'name', { range, affectsData: false, usingOperation: false } );
+			updateMarker( 'name', { range: range2 } );
+
+			const marker = model.markers.get( 'name' );
+
+			expect( marker.affectsData ).to.be.false;
+		} );
+
+		it( 'should allow changing affectsData and usingOperation', () => {
+			addMarker( 'name', { range, usingOperation: true } );
+			updateMarker( 'name', { affectsData: false, usingOperation: false } );
+
+			const op1 = batch.deltas[ 0 ].operations[ 0 ];
+			const op2 = batch.deltas[ 1 ].operations[ 0 ];
+			const marker = model.markers.get( 'name' );
+
+			expect( op1.affectsData ).to.be.true;
+			expect( op2.affectsData ).to.be.false;
+
+			expect( marker.affectsData ).to.be.false;
+		} );
+
+		it( 'should allow changing affectsData and usingOperation #2', () => {
+			const spy = sinon.spy();
+			model.on( 'applyOperation', spy );
+
+			addMarker( 'name', { range, usingOperation: false, affectsData: false } );
+			updateMarker( 'name', { usingOperation: true, affectsData: true } );
+
+			const marker = model.markers.get( 'name' );
+
+			sinon.assert.calledOnce( spy );
+			expect( spy.firstCall.args[ 1 ][ 0 ].type ).to.equal( 'marker' );
+
+			expect( marker.affectsData ).to.be.true;
+		} );
+
 		it( 'should throw when range and usingOperations were not provided', () => {
 			expect( () => {
 				addMarker( 'name', { range, usingOperation: false } );