浏览代码

Get rid of duplicated event during marker conversion.

Szymon Kupś 8 年之前
父节点
当前提交
45d47bb8fb

+ 28 - 6
packages/ckeditor5-engine/src/conversion/model-to-view-converters.js

@@ -122,16 +122,27 @@ export function insertUIElement( elementCreator ) {
 			return;
 		}
 
-		if ( !consumable.consume( data.range, evt.name ) ) {
+		const markerRange = data.markerRange;
+		const eventName = evt.name;
+
+		// If marker's range is collapsed - check if it can be consumed.
+		if ( markerRange.isCollapsed && !consumable.consume( markerRange, eventName ) ) {
 			return;
 		}
 
+		// if marker's range is not collapsed - consume all items inside.
+		for ( const value of markerRange ) {
+			if ( !consumable.consume( value.item, eventName ) ) {
+				return;
+			}
+		}
+
 		const mapper = conversionApi.mapper;
 
-		viewWriter.insert( mapper.toViewPosition( data.range.start ), viewStartElement );
+		viewWriter.insert( mapper.toViewPosition( markerRange.start ), viewStartElement );
 
-		if ( !data.range.isCollapsed ) {
-			viewWriter.insert( mapper.toViewPosition( data.range.end ), viewEndElement );
+		if ( !markerRange.isCollapsed ) {
+			viewWriter.insert( mapper.toViewPosition( markerRange.end ), viewEndElement );
 		}
 	};
 }
@@ -489,11 +500,22 @@ export function removeUIElement( elementCreator ) {
 			return;
 		}
 
-		if ( !consumable.consume( data.range, evt.name ) ) {
+		const markerRange = data.markerRange;
+		const eventName = evt.name;
+
+		// If marker's range is collapsed - check if it can be consumed.
+		if ( markerRange.isCollapsed && !consumable.consume( markerRange, eventName ) ) {
 			return;
 		}
 
-		const viewRange = conversionApi.mapper.toViewRange( data.range );
+		// Check if all items in the range can be consumed, and consume them.
+		for ( const value of markerRange ) {
+			if ( !consumable.consume( value.item, eventName ) ) {
+				return;
+			}
+		}
+
+		const viewRange = conversionApi.mapper.toViewRange( markerRange );
 
 		// First remove closing element.
 		viewWriter.clear( viewRange.getEnlarged(), viewEndElement );

+ 45 - 38
packages/ckeditor5-engine/src/conversion/modelconversiondispatcher.js

@@ -382,15 +382,10 @@ export default class ModelConversionDispatcher {
 	}
 
 	/**
-	 * Starts marker-conversion.
+	 * Starts marker conversion.
 	 *
-	 * Fires {@link ~#event:addMarker addMarker event} or {@link ~#event:removeMarker removeMarker event} based on
-	 * given `type` with data based on passed parameters.
-	 *
-	 * Marker conversion is performed in two steps. In the first step, event containing marker's whole range is fired. If
-	 * consumable passed with that event is consumed - no further events are dispatched. In the second step, event for each
-	 * item in the marker's range is fired. This way marker can be converted as a whole or each item in the marker's
-	 * range can be converted separately.
+	 * Fires {@link ~#event:addMarker addMarker} or {@link ~#event:removeMarker removeMarker} events for each item
+	 * in marker's range. If range is collapsed single event is dispatched. See events description for more details.
 	 *
 	 * @fires addMarker
 	 * @fires removeMarker
@@ -407,12 +402,16 @@ export default class ModelConversionDispatcher {
 		// In markers case, event name == consumable name.
 		const eventName = type + ':' + name;
 
-		// Fire event for whole marker. This can be used to convert whole marker to element.
-		const markerConsumable = this._createMarkerConsumable( eventName, range );
-		this.fire( eventName, { name, range }, markerConsumable, this.conversionApi );
+		// When range is collapsed - fire single event with collapsed range in consumable.
+		if ( range.isCollapsed ) {
+			const consumable = new Consumable();
+			consumable.add( range, eventName );
+
+			this.fire( eventName, {
+				markerName: name,
+				markerRange: range
+			}, consumable, this.conversionApi );
 
-		// Return if whole marker was consumed - do not perform conversion of each node in marker's range.
-		if ( !markerConsumable.test( range, eventName ) ) {
 			return;
 		}
 
@@ -422,11 +421,17 @@ export default class ModelConversionDispatcher {
 		// Create separate event for each node in the range.
 		for ( const value of range ) {
 			const item = value.item;
-			const itemRange = Range.createFromPositionAndShift( value.previousPosition, value.length );
+
+			// Do not fire event for already consumed items.
+			if ( !consumable.test( item, eventName ) ) {
+				continue;
+			}
+
 			const data = {
 				item,
-				name,
-				range: itemRange
+				range: Range.createFromPositionAndShift( value.previousPosition, value.length ),
+				markerName: name,
+				markerRange: range
 			};
 
 			this.fire( eventName, data, consumable, this.conversionApi );
@@ -501,22 +506,6 @@ export default class ModelConversionDispatcher {
 	}
 
 	/**
-	 * Creates {@link module:engine/conversion/modelconsumable~ModelConsumable} for adding or removing marker on given `range`.
-	 *
-	 * @private
-	 * @param {'addMarker'|'removeMarker'} type Change type.
-	 * @param {module:engine/model/range~Range} range Range on which marker was added or removed.
-	 * @returns {module:engine/conversion/modelconsumable~ModelConsumable} Values to consume.
-	 */
-	_createMarkerConsumable( type, range ) {
-		const consumable = new Consumable();
-
-		consumable.add( range, type );
-
-		return consumable;
-	}
-
-	/**
 	 * Tests passed `consumable` to check whether given event can be fired and if so, fires it.
 	 *
 	 * @private
@@ -667,6 +656,9 @@ export default class ModelConversionDispatcher {
 
 	/**
 	 * Fired when a new marker is added to the model.
+	 * * If marker's range is not collapsed then event is fired separately for each item contained in that range. Consumable
+	 * contains all items from that range.
+	 * * If marker's range is collapsed then single event is fired. Consumable contains collapsed range.
 	 *
 	 * `addMarker` is a namespace for a class of events. Names of actually called events follow this pattern:
 	 * `addMarker:<markerName>`. By specifying certain marker names, you can make the events even more gradual. For example,
@@ -675,14 +667,23 @@ export default class ModelConversionDispatcher {
 	 *
 	 * @event addMarker
 	 * @param {Object} data Additional information about the change.
-	 * @param {String} data.name Marker name.
-	 * @param {module:engine/model/range~Range} data.range Marker range.
-	 * @param {module:engine/conversion/modelconsumable~ModelConsumable} consumable Values to consume.
+	 * @param {module:engine/model/item~Item} [data.item] Item contained in marker's range. Not present if collapsed range
+	 * is being converted.
+	 * @param {module:engine/model/range~Range} [data.range] Range spanning over item. Not present if collapsed range
+	 * is being converted.
+	 * @param {String} data.markerName Name of the marker.
+	 * @param {module:engine/model/range~Range} data.markerRange Whole marker's range.
+	 * @param {module:engine/conversion/modelconsumable~ModelConsumable} consumable Values to consume. When non-collapsed
+	 * marker is being converted then consumable contains all items in marker's range. For collapsed markers it contains
+	 * only marker's range to consume.
 	 * @param {Object} conversionApi Conversion interface to be used by callback, passed in `ModelConversionDispatcher` constructor.
 	 */
 
 	/**
 	 * Fired when marker is removed from the model.
+	 * * If marker's range is not collapsed then event is fired separately for each item contained in that range. Consumable
+	 * contains all items from that range.
+	 * * If marker's range is collapsed then single event is fired. Consumable contains collapsed range.
 	 *
 	 * `removeMarker` is a namespace for a class of events. Names of actually called events follow this pattern:
 	 * `removeMarker:<markerName>`. By specifying certain marker names, you can make the events even more gradual. For example,
@@ -691,9 +692,15 @@ export default class ModelConversionDispatcher {
 	 *
 	 * @event removeMarker
 	 * @param {Object} data Additional information about the change.
-	 * @param {String} data.name Marker name.
-	 * @param {module:engine/model/range~Range} data.range Marker range.
-	 * @param {module:engine/conversion/modelconsumable~ModelConsumable} consumable Values to consume.
+	 * @param {module:engine/model/item~Item} [data.item] Item contained in marker's range. Not present if collapsed range
+	 * is being converted.
+	 * @param {module:engine/model/range~Range} [data.range] Range spanning over item. Not present if collapsed range
+	 * is being converted.
+	 * @param {String} data.markerName Name of the marker.
+	 * @param {module:engine/model/range~Range} data.markerRange Whole marker's range.
+	 * @param {module:engine/conversion/modelconsumable~ModelConsumable} consumable Values to consume. When non-collapsed
+	 * marker is being converted then consumable contains all items in marker's range. For collapsed markers it contains
+	 * only marker's range to consume.
 	 * @param {Object} conversionApi Conversion interface to be used by callback, passed in `ModelConversionDispatcher` constructor.
 	 */
 }

+ 2 - 2
packages/ckeditor5-engine/tests/conversion/buildmodelconverter.js

@@ -509,7 +509,7 @@ describe( 'Model converter builder', () => {
 
 			it( 'using passed creator function', () => {
 				buildModelConverter().for( dispatcher ).fromMarker( 'search' ).toElement( data => {
-					const className = 'search search-color-' + data.name.split( ':' )[ 1 ];
+					const className = 'search search-color-' + data.markerName.split( ':' )[ 1 ];
 
 					return new ViewUIElement( 'span', { class: className } );
 				} );
@@ -558,7 +558,7 @@ describe( 'Model converter builder', () => {
 
 			it( 'using passed creator function', () => {
 				buildModelConverter().for( dispatcher ).fromMarker( 'search' ).toElement( data => {
-					const className = 'search search-color-' + data.name.split( ':' )[ 1 ];
+					const className = 'search search-color-' + data.markerName.split( ':' )[ 1 ];
 
 					return new ViewUIElement( 'span', { class: className } );
 				} );

+ 12 - 12
packages/ckeditor5-engine/tests/conversion/model-to-view-converters.js

@@ -629,7 +629,7 @@ describe( 'model-to-view-converters', () => {
 		} );
 
 		it( 'should insert and remove ui element - function as a creator', () => {
-			const viewUi = data => new ViewUIElement( 'span', { 'class': data.name } );
+			const viewUi = data => new ViewUIElement( 'span', { 'class': data.markerName } );
 
 			dispatcher.on( 'addMarker:marker', insertUIElement( viewUi ) );
 			dispatcher.on( 'removeMarker:marker', removeUIElement( viewUi ) );
@@ -652,11 +652,11 @@ describe( 'model-to-view-converters', () => {
 			dispatcher.on( 'removeMarker:marker', removeUIElement( viewUi ) );
 
 			dispatcher.on( 'addMarker:marker', ( evt, data, consumable ) => {
-				expect( consumable.test( data.range, 'addMarker:marker' ) ).to.be.true;
+				expect( consumable.test( data.markerRange, 'addMarker:marker' ) ).to.be.true;
 			} );
 
 			dispatcher.on( 'removeMarker:marker', ( evt, data, consumable ) => {
-				expect( consumable.test( data.range, 'removeMarker:marker' ) ).to.be.true;
+				expect( consumable.test( data.markerRange, 'removeMarker:marker' ) ).to.be.true;
 			} );
 
 			dispatcher.convertMarker( 'addMarker', 'marker', range );
@@ -679,11 +679,11 @@ describe( 'model-to-view-converters', () => {
 			dispatcher.on( 'removeMarker:marker', removeUIElement( viewUi ) );
 
 			dispatcher.on( 'addMarker:marker', ( evt, data, consumable ) => {
-				consumable.consume( data.range, 'addMarker:marker' );
+				consumable.consume( data.markerRange, 'addMarker:marker' );
 			}, { priority: 'high' } );
 
 			dispatcher.on( 'removeMarker:marker', ( evt, data, consumable ) => {
-				consumable.consume( data.range, 'removeMarker:marker' );
+				consumable.consume( data.markerRange, 'removeMarker:marker' );
 			}, { priority: 'high' } );
 
 			dispatcher.convertMarker( 'addMarker', 'marker', range );
@@ -706,11 +706,11 @@ describe( 'model-to-view-converters', () => {
 			dispatcher.on( 'removeMarker:marker', removeUIElement( viewUi ) );
 
 			dispatcher.on( 'addMarker:marker', ( evt, data, consumable ) => {
-				expect( consumable.test( data.range, 'addMarker:marker' ) ).to.be.true;
+				expect( consumable.test( data.markerRange, 'addMarker:marker' ) ).to.be.true;
 			} );
 
 			dispatcher.on( 'removeMarker:marker', ( evt, data, consumable ) => {
-				expect( consumable.test( data.range, 'removeMarker:marker' ) ).to.be.true;
+				expect( consumable.test( data.markerRange, 'removeMarker:marker' ) ).to.be.true;
 			} );
 
 			dispatcher.convertMarker( 'addMarker', 'marker', range );
@@ -733,11 +733,11 @@ describe( 'model-to-view-converters', () => {
 			dispatcher.on( 'removeMarker:marker', removeUIElement( viewUi ) );
 
 			dispatcher.on( 'addMarker:marker', ( evt, data, consumable ) => {
-				consumable.consume( data.range, 'addMarker:marker' );
+				consumable.consume( data.markerRange, 'addMarker:marker' );
 			}, { priority: 'high' } );
 
 			dispatcher.on( 'removeMarker:marker', ( evt, data, consumable ) => {
-				consumable.consume( data.range, 'removeMarker:marker' );
+				consumable.consume( data.markerRange, 'removeMarker:marker' );
 			}, { priority: 'high' } );
 
 			dispatcher.convertMarker( 'addMarker', 'marker', range );
@@ -773,7 +773,7 @@ describe( 'model-to-view-converters', () => {
 			} );
 
 			it( 'should insert and remove ui element - function as a creator', () => {
-				const viewUi = data => new ViewUIElement( 'span', { 'class': data.name } );
+				const viewUi = data => new ViewUIElement( 'span', { 'class': data.markerName } );
 
 				dispatcher.on( 'addMarker:marker', insertUIElement( viewUi ) );
 				dispatcher.on( 'removeMarker:marker', removeUIElement( viewUi ) );
@@ -791,10 +791,10 @@ describe( 'model-to-view-converters', () => {
 			it( 'should insert and remove different opening and ending element', () => {
 				function creator( data ) {
 					if ( data.isOpening ) {
-						return new ViewUIElement( 'span', { 'class': data.name, 'data-start': true } );
+						return new ViewUIElement( 'span', { 'class': data.markerName, 'data-start': true } );
 					}
 
-					return new ViewUIElement( 'span', { 'class': data.name, 'data-end': true } );
+					return new ViewUIElement( 'span', { 'class': data.markerName, 'data-end': true } );
 				}
 
 				dispatcher.on( 'addMarker:marker', insertUIElement( creator ) );

+ 69 - 41
packages/ckeditor5-engine/tests/conversion/modelconversiondispatcher.js

@@ -635,79 +635,107 @@ describe( 'ModelConversionDispatcher', () => {
 
 		it( 'should prepare consumable values', () => {
 			dispatcher.on( 'addMarker:name', ( evt, data, consumable ) => {
-				if ( data.item ) {
-					expect( consumable.test( data.item, 'addMarker:name' ) ).to.be.true;
-				} else {
-					expect( consumable.test( data.range, 'addMarker:name' ) ).to.be.true;
-				}
+				expect( consumable.test( data.item, 'addMarker:name' ) ).to.be.true;
 			} );
 
 			dispatcher.on( 'removeMarker:name', ( evt, data, consumable ) => {
-				if ( data.item ) {
-					expect( consumable.test( data.item, 'removeMarker:name' ) ).to.be.true;
-				} else {
-					expect( consumable.test( data.range, 'removeMarker:name' ) ).to.be.true;
-				}
+				expect( consumable.test( data.item, 'removeMarker:name' ) ).to.be.true;
 			} );
 
 			dispatcher.convertMarker( 'addMarker', 'name', range );
 			dispatcher.convertMarker( 'removeMarker', 'name', range );
 		} );
 
-		it( 'should fire conversion for whole range and each item in the range', () => {
+		it( 'should fire conversion for each item in the range', () => {
 			const element = new ModelElement( 'paragraph', null, [ new ModelText( 'foo bar baz' ) ] );
 			root.appendChildren( [ element ] );
 			range = ModelRange.createIn( root );
 
-			const addMarkerItems = [];
-			const removeMarkerItems = [];
+			const addMarkerData = [];
+			const removeMarkerData = [];
 
-			dispatcher.on( 'addMarker:name', ( evt, data ) => {
-				// Called for whole marker.
-				if ( !data.item ) {
-					expect( data.range.isEqual( range ) ).to.be.true;
-				} else {
-					addMarkerItems.push( data.item );
-				}
-			} );
-			dispatcher.on( 'removeMarker:name', ( evt, data ) => {
-				// Called for whole marker.
-				if ( !data.item ) {
-					expect( data.range.isEqual( range ) ).to.be.true;
-				} else {
-					removeMarkerItems.push( data.item );
-				}
-			} );
+			dispatcher.on( 'addMarker:name', ( evt, data ) => addMarkerData.push( data ) );
+			dispatcher.on( 'removeMarker:name', ( evt, data ) => removeMarkerData.push( data ) );
 
 			dispatcher.convertMarker( 'addMarker', 'name', range );
 			dispatcher.convertMarker( 'removeMarker', 'name', range );
 
+			// Check if events for all elements were fired.
 			let i = 0;
 			for ( const val of range ) {
-				const item = val.item;
+				const nodeInRange = val.item;
+				const addData = addMarkerData[ i ];
+				const removeData = removeMarkerData[ i ];
+
+				expect( addData.markerName ).to.equal( 'name' );
+				expect( addData.markerRange ).to.equal( range );
+				expect( addData.range.isEqual( ModelRange.createOn( nodeInRange ) ) );
 
-				if ( item.is( 'textProxy' ) ) {
-					expect( item.data ).to.equal( addMarkerItems[ i ].data );
-					expect( item.data ).to.equal( removeMarkerItems[ i ].data );
+				expect( removeData.markerName ).to.equal( 'name' );
+				expect( removeData.markerRange ).to.equal( range );
+				expect( removeData.range.isEqual( ModelRange.createOn( nodeInRange ) ) );
+
+				if ( nodeInRange.is( 'textProxy' ) ) {
+					expect( nodeInRange.data ).to.equal( addData.item.data );
+					expect( nodeInRange.data ).to.equal( removeData.item.data );
 				} else {
-					expect( removeMarkerItems[ i ] ).to.equal( item );
+					expect( nodeInRange ).to.equal( addData.item );
+					expect( nodeInRange ).to.equal( removeData.item );
 				}
 
 				i++;
 			}
 		} );
 
-		it( 'should not fire conversion for each item in marker\'s range if whole marker conversion was performed', () => {
-			const spy = sinon.spy( ( evt, data, consumable ) => {
-				if ( !data.item ) {
-					expect( consumable.consume( data.range, 'addMarker:name' ) ).to.be.true;
+		it( 'should not fire events for already consumed items', () => {
+			const element = new ModelElement( 'paragraph', null, [ new ModelText( 'foo bar baz' ) ] );
+			root.appendChildren( [ element ] );
+			const range = ModelRange.createIn( root );
+			const addMarkerSpy = sinon.spy( ( evt, data, consumable ) => {
+				// Consume all items in marker range.
+				for ( const value of data.markerRange ) {
+					consumable.consume( value.item, 'addMarker:marker' );
 				}
 			} );
 
-			dispatcher.on( 'addMarker:name', spy );
+			const removeMarkerSpy = sinon.spy( ( evt, data, consumable ) => {
+				// Consume all items in marker range.
+				for ( const value of data.markerRange ) {
+					consumable.consume( value.item, 'removeMarker:marker' );
+				}
+			} );
 
-			dispatcher.convertMarker( 'addMarker', 'name', range );
-			sinon.assert.calledOnce( spy );
+			dispatcher.on( 'addMarker:marker', addMarkerSpy );
+			dispatcher.on( 'addMarker:marker', removeMarkerSpy );
+
+			dispatcher.convertMarker( 'addMarker', 'marker', range );
+			dispatcher.convertMarker( 'removeMarker', 'marker', range );
+
+			sinon.assert.calledOnce( addMarkerSpy );
+			sinon.assert.calledOnce( removeMarkerSpy );
+		} );
+
+		it( 'should fire event for collapsed marker', () => {
+			const range = ModelRange.createFromParentsAndOffsets( root, 1, root, 1 );
+			const addMarkerSpy = sinon.spy( ( evt, data, consumable ) => {
+				expect( data.markerRange ).to.equal( range );
+				expect( data.markerName ).to.equal( 'marker' );
+				expect( consumable.test( data.markerRange, evt.name ) ).to.be.true;
+			} );
+			const removeMarkerSpy = sinon.spy( ( evt, data, consumable ) => {
+				expect( data.markerRange ).to.equal( range );
+				expect( data.markerName ).to.equal( 'marker' );
+				expect( consumable.test( data.markerRange, evt.name ) ).to.be.true;
+			} );
+
+			dispatcher.on( 'addMarker:marker', addMarkerSpy );
+			dispatcher.on( 'addMarker:marker', removeMarkerSpy );
+
+			dispatcher.convertMarker( 'addMarker', 'marker', range );
+			dispatcher.convertMarker( 'removeMarker', 'marker', range );
+
+			sinon.assert.calledOnce( addMarkerSpy );
+			sinon.assert.calledOnce( removeMarkerSpy );
 		} );
 	} );
 } );