Przeglądaj źródła

Merge pull request #1288 from ckeditor/t/1284

Other: Keep the same marker instance when marker is updated.
Szymon Cofalik 8 lat temu
rodzic
commit
1779d27832

+ 5 - 5
packages/ckeditor5-engine/src/controller/editingcontroller.js

@@ -131,13 +131,13 @@ export default class EditingController {
 			}
 		}, { priority: 'high' } );
 
-		// If a marker is removed through `model.Model#markers` directly (not through operation), just remove it (if
-		// it was not removed already).
-		this.listenTo( model.markers, 'remove', ( evt, marker ) => {
-			if ( !removedMarkers.has( marker.name ) ) {
+		// If an existing marker is updated through `model.Model#markers` directly (not through operation), just remove it.
+		this.listenTo( model.markers, 'update', ( evt, marker, oldRange ) => {
+			if ( oldRange && !removedMarkers.has( marker.name ) ) {
 				removedMarkers.add( marker.name );
+
 				this.view.change( writer => {
-					this.downcastDispatcher.convertMarkerRemove( marker.name, marker.getRange(), writer );
+					this.downcastDispatcher.convertMarkerRemove( marker.name, oldRange, writer );
 				} );
 			}
 		} );

+ 10 - 15
packages/ckeditor5-engine/src/model/document.js

@@ -161,21 +161,16 @@ export default class Document {
 		// Buffer marker changes.
 		// This is not covered in buffering operations because markers may change outside of them (when they
 		// are modified using `model.markers` collection, not through `MarkerOperation`).
-		this.listenTo( model.markers, 'set', ( evt, marker ) => {
-			// TODO: Should filter out changes of markers that are not in document.
-			// Whenever a new marker is added, buffer that change.
-			this.differ.bufferMarkerChange( marker.name, null, marker.getRange() );
-
-			// Whenever marker changes, buffer that.
-			marker.on( 'change', ( evt, oldRange ) => {
-				this.differ.bufferMarkerChange( marker.name, oldRange, marker.getRange() );
-			} );
-		} );
-
-		this.listenTo( model.markers, 'remove', ( evt, marker ) => {
-			// TODO: Should filter out changes of markers that are not in document.
-			// Whenever marker is removed, buffer that change.
-			this.differ.bufferMarkerChange( marker.name, marker.getRange(), null );
+		this.listenTo( model.markers, 'update', ( evt, marker, oldRange, newRange ) => {
+			// Whenever marker is updated, buffer that change.
+			this.differ.bufferMarkerChange( marker.name, oldRange, newRange );
+
+			if ( !oldRange ) {
+				// Whenever marker changes, buffer that.
+				marker.on( 'change', ( evt, oldRange ) => {
+					this.differ.bufferMarkerChange( marker.name, oldRange, marker.getRange() );
+				} );
+			}
 		} );
 	}
 

+ 88 - 38
packages/ckeditor5-engine/src/model/markercollection.js

@@ -17,8 +17,7 @@ import mix from '@ckeditor/ckeditor5-utils/src/mix';
 /**
  * The collection of all {@link module:engine/model/markercollection~Marker markers} attached to the document.
  * It lets you {@link module:engine/model/markercollection~MarkerCollection#get get} markers or track them using
- * {@link module:engine/model/markercollection~MarkerCollection#event:set} and
- * {@link module:engine/model/markercollection~MarkerCollection#event:remove} events.
+ * {@link module:engine/model/markercollection~MarkerCollection#event:update} event.
  *
  * To create, change or remove makers use {@link module:engine/model/writer~Writer model writers'} methods:
  * {@link module:engine/model/writer~Writer#setMarker} or {@link module:engine/model/writer~Writer#removeMarker}. Since
@@ -79,37 +78,47 @@ export default class MarkerCollection {
 	 * Creates and adds a {@link ~Marker marker} to the `MarkerCollection` with given name on given
 	 * {@link module:engine/model/range~Range range}.
 	 *
-	 * If `MarkerCollection` already had a marker with given name (or {@link ~Marker marker} was passed) and the range to
-	 * set is different, the marker in collection is removed and then new marker is added. If the range was same, nothing
-	 * happens and `false` is returned.
+	 * If `MarkerCollection` already had a marker with given name (or {@link ~Marker marker} was passed), the marker in
+	 * collection is updated and {@link module:engine/model/markercollection~MarkerCollection#event:update} event is fired
+	 * but only if there was a change (marker range or {@link ~Marker#managedUsingOperations} flag has changed.
 	 *
 	 * @protected
-	 * @fires module:engine/model/markercollection~MarkerCollection#event:set
-	 * @fires module:engine/model/markercollection~MarkerCollection#event:remove
+	 * @fires module:engine/model/markercollection~MarkerCollection#event:update
 	 * @param {String|module:engine/model/markercollection~Marker} markerOrName Name of marker to set or marker instance to update.
 	 * @param {module:engine/model/range~Range} range Marker range.
-	 * @param {Boolean} managedUsingOperations Specifies whether the marker is managed using operations.
-	 * @returns {module:engine/model/markercollection~Marker} `Marker` instance added to the collection.
+	 * @param {Boolean} [managedUsingOperations=false] Specifies whether the marker is managed using operations.
+	 * @returns {module:engine/model/markercollection~Marker} `Marker` instance which was added or updated.
 	 */
-	_set( markerOrName, range, managedUsingOperations ) {
+	_set( markerOrName, range, managedUsingOperations = false ) {
 		const markerName = markerOrName instanceof Marker ? markerOrName.name : markerOrName;
 		const oldMarker = this._markers.get( markerName );
 
 		if ( oldMarker ) {
 			const oldRange = oldMarker.getRange();
+			let hasChanged = false;
+
+			if ( !oldRange.isEqual( range ) ) {
+				oldMarker._attachLiveRange( LiveRange.createFromRange( range ) );
+				hasChanged = true;
+			}
 
-			if ( oldRange.isEqual( range ) && managedUsingOperations === oldMarker.managedUsingOperations ) {
-				return oldMarker;
+			if ( managedUsingOperations != oldMarker.managedUsingOperations ) {
+				oldMarker._managedUsingOperations = managedUsingOperations;
+				hasChanged = true;
 			}
 
-			this._remove( markerName );
+			if ( hasChanged ) {
+				this.fire( 'update:' + markerName, oldMarker, oldRange, range );
+			}
+
+			return oldMarker;
 		}
 
 		const liveRange = LiveRange.createFromRange( range );
 		const marker = new Marker( markerName, liveRange, managedUsingOperations );
 
 		this._markers.set( markerName, marker );
-		this.fire( 'set:' + markerName, marker );
+		this.fire( 'update:' + markerName, marker, null, range );
 
 		return marker;
 	}
@@ -118,6 +127,7 @@ export default class MarkerCollection {
 	 * Removes given {@link ~Marker marker} or a marker with given name from the `MarkerCollection`.
 	 *
 	 * @protected
+	 * @fires module:engine/model/markercollection~MarkerCollection#event:update
 	 * @param {String} markerOrName Marker or name of a marker to remove.
 	 * @returns {Boolean} `true` if marker was found and removed, `false` otherwise.
 	 */
@@ -127,7 +137,7 @@ export default class MarkerCollection {
 
 		if ( oldMarker ) {
 			this._markers.delete( markerName );
-			this.fire( 'remove:' + markerName, oldMarker );
+			this.fire( 'update:' + markerName, oldMarker, oldMarker.getRange(), null );
 
 			this._destroyMarker( oldMarker );
 
@@ -193,22 +203,18 @@ export default class MarkerCollection {
 	 */
 	_destroyMarker( marker ) {
 		marker.stopListening();
-		marker._liveRange.detach();
-		marker._liveRange = null;
+		marker._detachLiveRange();
 	}
 
 	/**
-	 * Fired whenever marker is added or updated in `MarkerCollection`.
+	 * Fired whenever marker is added, updated or removed from `MarkerCollection`.
 	 *
-	 * @event set
-	 * @param {module:engine/model/markercollection~Marker} The set marker.
-	 */
-
-	/**
-	 * Fired whenever marker is removed from `MarkerCollection`.
-	 *
-	 * @event remove
-	 * @param {module:engine/model/markercollection~Marker} marker The removed marker.
+	 * @event update
+	 * @param {module:engine/model/markercollection~Marker} Updated Marker.
+	 * @param {module:engine/model/range~Range|null} oldRange Marker range before the update. When is not defined it
+	 * means that marker is just added.
+	 * @param {module:engine/model/range~Range|null} newRange Marker range after update. When is not defined it
+	 * means that marker is just removed.
 	 */
 }
 
@@ -291,6 +297,7 @@ class Marker {
 	 *
 	 * @param {String} name Marker name.
 	 * @param {module:engine/model/liverange~LiveRange} liveRange Range marked by the marker.
+	 * @param {Boolean} managedUsingOperations Specifies whether the marker is managed using operations.
 	 */
 	constructor( name, liveRange, managedUsingOperations ) {
 		/**
@@ -302,25 +309,35 @@ class Marker {
 		this.name = name;
 
 		/**
-		 * Flag indicates if the marker is managed using operations or not. See {@link ~Marker marker class description}
-		 * to learn more about marker types. See {@link module:engine/model/writer~Writer#setMarker}.
+		 * Flag indicates if the marker is managed using operations or not.
 		 *
-		 * @readonly
-		 * @type {Boolean}
+		 * @protected
+		 * @member {Boolean}
 		 */
-		this.managedUsingOperations = managedUsingOperations;
+		this._managedUsingOperations = managedUsingOperations;
 
 		/**
 		 * Range marked by the marker.
 		 *
-		 * @protected
-		 * @type {module:engine/model/liverange~LiveRange}
+		 * @private
+		 * @member {module:engine/model/liverange~LiveRange} #_liveRange
 		 */
-		this._liveRange = liveRange;
+		this._liveRange = this._attachLiveRange( liveRange );
+	}
 
-		// Delegating does not work with namespaces. Alternatively, we could delegate all events (using `*`).
-		this._liveRange.delegate( 'change:range' ).to( this );
-		this._liveRange.delegate( 'change:content' ).to( this );
+	/**
+	 * Returns value of flag indicates if the marker is managed using operations or not.
+	 * See {@link ~Marker marker class description} to learn more about marker types.
+	 * See {@link module:engine/model/writer~Writer#setMarker}.
+	 *
+	 * @returns {Boolean}
+	 */
+	get managedUsingOperations() {
+		if ( !this._liveRange ) {
+			throw new CKEditorError( 'marker-destroyed: Cannot use a destroyed marker instance.' );
+		}
+
+		return this._managedUsingOperations;
 	}
 
 	/**
@@ -369,6 +386,39 @@ class Marker {
 		return Range.createFromRange( this._liveRange );
 	}
 
+	/**
+	 * Binds new live range to marker and detach the old one if is attached.
+	 *
+	 * @protected
+	 * @param {module:engine/model/liverange~LiveRange} liveRange Live range to attach
+	 * @return {module:engine/model/liverange~LiveRange} Attached live range.
+	 */
+	_attachLiveRange( liveRange ) {
+		if ( this._liveRange ) {
+			this._detachLiveRange();
+		}
+
+		// Delegating does not work with namespaces. Alternatively, we could delegate all events (using `*`).
+		liveRange.delegate( 'change:range' ).to( this );
+		liveRange.delegate( 'change:content' ).to( this );
+
+		this._liveRange = liveRange;
+
+		return liveRange;
+	}
+
+	/**
+	 * Unbinds and destroys currently attached live range.
+	 *
+	 * @protected
+	 */
+	_detachLiveRange() {
+		this._liveRange.stopDelegating( 'change:range', this );
+		this._liveRange.stopDelegating( 'change:content', this );
+		this._liveRange.detach();
+		this._liveRange = null;
+	}
+
 	/**
 	 * Fired whenever {@link ~Marker#_liveRange marker range} is changed due to changes on {@link module:engine/model/document~Document}.
 	 * This is a delegated {@link module:engine/model/liverange~LiveRange#event:change:range LiveRange change:range event}.

+ 123 - 30
packages/ckeditor5-engine/tests/model/markercollection.js

@@ -6,6 +6,7 @@
 import MarkerCollection from '../../src/model/markercollection';
 import Position from '../../src/model/position';
 import Range from '../../src/model/range';
+import LiveRange from '../../src/model/liverange';
 import Text from '../../src/model/text';
 import Model from '../../src/model/model';
 import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror';
@@ -41,7 +42,7 @@ describe( 'MarkerCollection', () => {
 	} );
 
 	describe( '_set', () => {
-		it( 'should create a marker, fire set:<markerName> event and return true', () => {
+		it( 'should create a marker and fire update:<markerName>', () => {
 			sinon.spy( markers, 'fire' );
 
 			const result = markers._set( 'name', range );
@@ -49,47 +50,71 @@ describe( 'MarkerCollection', () => {
 
 			expect( result ).to.equal( marker );
 			expect( marker.name ).to.equal( 'name' );
+			expect( marker.managedUsingOperations ).to.false;
 			expect( marker.getRange().isEqual( range ) ).to.be.true;
-			expect( markers.fire.calledWithExactly( 'set:name', marker ) ).to.be.true;
+			sinon.assert.calledWithExactly( markers.fire, 'update:name', result, null, range );
 		} );
 
-		it( 'should fire remove:<markerName> event, and create a new marker if marker with given name was in the collection', () => {
-			const marker1 = markers._set( 'name', range );
+		it( 'should create a marker marked as managed by operations', () => {
+			const marker = markers._set( 'name', range, true );
+
+			expect( marker.managedUsingOperations ).to.true;
+		} );
+
+		it( 'should update marker range and fire update:<markerName> event if marker with given name was in the collection', () => {
+			const marker = markers._set( 'name', range );
 
 			sinon.spy( markers, 'fire' );
+			sinon.spy( marker, '_detachLiveRange' );
+			sinon.spy( marker, '_attachLiveRange' );
 
-			const marker2 = markers._set( 'name', range2 );
+			const result = markers._set( 'name', range2 );
 
-			expect( markers.fire.calledWithExactly( 'remove:name', marker1 ) ).to.be.true;
-			expect( markers.fire.calledWithExactly( 'set:name', marker2 ) ).to.be.true;
+			expect( result ).to.equal( marker );
+			expect( marker.getRange().isEqual( range2 ) ).to.be.true;
+
+			sinon.assert.calledWithExactly( markers.fire, 'update:name', marker, range, range2 );
+			sinon.assert.calledOnce( marker._detachLiveRange );
+			sinon.assert.calledOnce( marker._detachLiveRange );
+		} );
 
-			expect( marker2.name ).to.equal( 'name' );
-			expect( marker2.getRange().isEqual( range2 ) ).to.be.true;
+		it( 'should update marker#managedUsingOperations and fire update:<markerName> event if marker with given name ' +
+			'was in the collection',
+		() => {
+			const marker = markers._set( 'name', range );
 
-			expect( marker1 ).not.to.equal( marker2 );
+			sinon.spy( markers, 'fire' );
+			sinon.spy( marker, '_detachLiveRange' );
+			sinon.spy( marker, '_attachLiveRange' );
+
+			const result = markers._set( 'name', range, true );
+
+			expect( result ).to.equal( marker );
+			expect( marker.managedUsingOperations ).to.true;
+			expect( marker.getRange().isEqual( range ) ).to.be.true;
+
+			sinon.assert.calledWithExactly( markers.fire, 'update:name', marker, range, range );
+			sinon.assert.notCalled( marker._detachLiveRange );
+			sinon.assert.notCalled( marker._attachLiveRange );
 		} );
 
-		it( 'should not fire event and return the same marker if given marker has a range equal to given range', () => {
-			const marker1 = markers._set( 'name', range );
+		it( 'should not fire event if given marker has not changed', () => {
+			const marker = markers._set( 'name', range );
 
 			sinon.spy( markers, 'fire' );
 
-			const marker2 = markers._set( 'name', range );
+			const result = markers._set( 'name', range );
 
-			expect( marker1 ).to.equal( marker2 );
-			expect( markers.fire.notCalled ).to.be.true;
+			expect( marker ).to.equal( result );
+			sinon.assert.notCalled( markers.fire );
 		} );
 
 		it( 'should accept marker instance instead of name', () => {
-			markers._set( 'name', range );
-			const marker1 = markers.get( 'name' );
+			const marker = markers._set( 'name', range );
 
-			const result = markers._set( marker1, range2 );
-			const marker2 = markers.get( 'name' );
+			markers._set( marker, range2 );
 
-			expect( result ).to.equal( marker2 );
-			expect( marker2.getRange().isEqual( range2 ) );
-			expect( marker1 ).not.to.equal( marker2 );
+			expect( marker.getRange().isEqual( range2 ) ).to.true;
 		} );
 	} );
 
@@ -115,7 +140,7 @@ describe( 'MarkerCollection', () => {
 	} );
 
 	describe( '_remove', () => {
-		it( 'should remove marker, return true and fire remove:<markerName> event', () => {
+		it( 'should remove marker, return true and fire update:<markerName> event', () => {
 			const marker = markers._set( 'name', range );
 
 			sinon.spy( markers, 'fire' );
@@ -123,22 +148,20 @@ describe( 'MarkerCollection', () => {
 			const result = markers._remove( 'name' );
 
 			expect( result ).to.be.true;
-			expect( markers.fire.calledWithExactly( 'remove:name', marker ) ).to.be.true;
 			expect( markers.get( 'name' ) ).to.be.null;
+			sinon.assert.calledWithExactly( markers.fire, 'update:name', marker, range, null );
 		} );
 
 		it( 'should destroy marker instance', () => {
 			const marker = markers._set( 'name', range );
-			const liveRange = marker._liveRange;
 
 			sinon.spy( marker, 'stopListening' );
-			sinon.spy( liveRange, 'detach' );
+			sinon.spy( marker, '_detachLiveRange' );
 
 			markers._remove( 'name' );
 
 			expect( marker.stopListening.calledOnce ).to.be.true;
-			expect( marker._liveRange ).to.be.null;
-			expect( liveRange.detach.calledOnce ).to.be.true;
+			expect( marker._detachLiveRange.calledOnce ).to.be.true;
 		} );
 
 		it( 'should return false if name has not been found in collection', () => {
@@ -160,7 +183,7 @@ describe( 'MarkerCollection', () => {
 			const result = markers._remove( marker );
 
 			expect( result ).to.be.true;
-			expect( markers.fire.calledWithExactly( 'remove:name', marker ) ).to.be.true;
+			expect( markers.fire.calledWithExactly( 'update:name', marker, range, null ) ).to.be.true;
 			expect( markers.get( 'name' ) ).to.be.null;
 		} );
 	} );
@@ -255,9 +278,13 @@ describe( 'Marker', () => {
 		expect( () => {
 			marker.getEnd();
 		} ).to.throw( CKEditorError, /^marker-destroyed/ );
+
+		expect( () => {
+			marker.managedUsingOperations;
+		} ).to.throw( CKEditorError, /^marker-destroyed/ );
 	} );
 
-	it( 'should delegate events from live range', () => {
+	it( 'should attach live range to marker', () => {
 		const range = Range.createFromParentsAndOffsets( root, 1, root, 2 );
 		const marker = model.markers._set( 'name', range );
 
@@ -273,4 +300,70 @@ describe( 'Marker', () => {
 		expect( eventRange.calledOnce ).to.be.true;
 		expect( eventContent.calledOnce ).to.be.true;
 	} );
+
+	it( 'should detach live range from marker', () => {
+		const range = Range.createFromParentsAndOffsets( root, 1, root, 2 );
+		const marker = model.markers._set( 'name', range );
+		const liveRange = marker._liveRange;
+
+		const eventRange = sinon.spy();
+		const eventContent = sinon.spy();
+		sinon.spy( liveRange, 'detach' );
+
+		marker.on( 'change:range', eventRange );
+		marker.on( 'change:content', eventContent );
+
+		marker._detachLiveRange();
+
+		liveRange.fire( 'change:range', null, {} );
+		liveRange.fire( 'change:content', null, {} );
+
+		expect( eventRange.notCalled ).to.be.true;
+		expect( eventContent.notCalled ).to.be.true;
+		expect( liveRange.detach.calledOnce ).to.true;
+	} );
+
+	it( 'should reattach live range to marker', () => {
+		const range = Range.createFromParentsAndOffsets( root, 1, root, 2 );
+		const marker = model.markers._set( 'name', range );
+		const oldLiveRange = marker._liveRange;
+		const newLiveRange = LiveRange.createFromParentsAndOffsets( root, 0, root, 1 );
+
+		const eventRange = sinon.spy();
+		const eventContent = sinon.spy();
+		sinon.spy( oldLiveRange, 'detach' );
+
+		marker.on( 'change:range', eventRange );
+		marker.on( 'change:content', eventContent );
+
+		marker._attachLiveRange( newLiveRange );
+
+		oldLiveRange.fire( 'change:range', null, {} );
+		oldLiveRange.fire( 'change:content', null, {} );
+
+		expect( eventRange.notCalled ).to.be.true;
+		expect( eventContent.notCalled ).to.be.true;
+		expect( oldLiveRange.detach.calledOnce ).to.true;
+
+		newLiveRange.fire( 'change:range', null, {} );
+		newLiveRange.fire( 'change:content', null, {} );
+
+		expect( eventRange.calledOnce ).to.be.true;
+		expect( eventContent.calledOnce ).to.be.true;
+	} );
+
+	it( 'should change managedUsingOperations flag', () => {
+		const range = Range.createFromParentsAndOffsets( root, 1, root, 2 );
+		const marker = model.markers._set( 'name', range, false );
+
+		expect( marker.managedUsingOperations ).to.false;
+
+		marker._managedUsingOperations = true;
+
+		expect( marker.managedUsingOperations ).to.true;
+
+		marker._managedUsingOperations = false;
+
+		expect( marker.managedUsingOperations ).to.false;
+	} );
 } );

+ 3 - 3
packages/ckeditor5-engine/tests/model/writer.js

@@ -1995,12 +1995,12 @@ describe( 'Writer', () => {
 		} );
 
 		it( 'should update marker in the document marker collection', () => {
-			setMarker( 'name', range );
+			const marker = setMarker( 'name', range );
 
 			const range2 = Range.createFromParentsAndOffsets( root, 0, root, 0 );
 			setMarker( 'name', range2 );
 
-			expect( model.markers.get( 'name' ).getRange().isEqual( range2 ) ).to.be.true;
+			expect( marker.getRange().isEqual( range2 ) ).to.be.true;
 		} );
 
 		it( 'should accept marker instance', () => {
@@ -2013,7 +2013,7 @@ describe( 'Writer', () => {
 
 			const op = batch.deltas[ 1 ].operations[ 0 ];
 
-			expect( model.markers.get( 'name' ).getRange().isEqual( range2 ) ).to.be.true;
+			expect( marker.getRange().isEqual( range2 ) ).to.be.true;
 			expect( op.oldRange.isEqual( range ) ).to.be.true;
 			expect( op.newRange.isEqual( range2 ) ).to.be.true;
 		} );