Browse Source

Feature: Keep the same marker instance when marker is updated.

Oskar Wróbel 7 years ago
parent
commit
4ffb96d604

+ 86 - 36
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,17 +78,16 @@ 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 {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.
+	 * @returns {module:engine/model/markercollection~Marker} Added or updated `Marker` instance.
 	 */
 	_set( markerOrName, range, managedUsingOperations ) {
 		const markerName = markerOrName instanceof Marker ? markerOrName.name : markerOrName;
@@ -97,19 +95,30 @@ export default class MarkerCollection {
 
 		if ( oldMarker ) {
 			const oldRange = oldMarker.getRange();
+			let hasChanged = false;
 
-			if ( oldRange.isEqual( range ) && managedUsingOperations === oldMarker.managedUsingOperations ) {
-				return oldMarker;
+			if ( !oldRange.isEqual( range ) ) {
+				oldMarker._attachLiveRange( LiveRange.createFromRange( range ) );
+				hasChanged = true;
 			}
 
-			this._remove( markerName );
+			if ( !!managedUsingOperations !== oldMarker.managedUsingOperations ) {
+				oldMarker._managedUsingOperations = managedUsingOperations;
+				hasChanged = true;
+			}
+
+			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;
 	}
 
 	/**
@@ -370,6 +387,39 @@ class Marker {
 	}
 
 	/**
+	 * 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;
+	} );
 } );