Browse Source

Marked MarkerCollection's `set` and `remove` as protected.

Maciej Bukowski 7 years ago
parent
commit
780d16a6f0

+ 19 - 8
packages/ckeditor5-engine/src/model/markercollection.js

@@ -99,28 +99,30 @@ export default class MarkerCollection {
 	 * 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.
 	 *
+	 * @protected
 	 * @fires module:engine/model/markercollection~MarkerCollection#event:add
 	 * @fires module:engine/model/markercollection~MarkerCollection#event:remove
 	 * @param {String|module:engine/model/markercollection~Marker} markerOrName Name of marker to add 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.
 	 */
-	set( markerOrName, range ) {
+	_set( markerOrName, range, managedUsingOperations ) {
 		const markerName = markerOrName instanceof Marker ? markerOrName.name : markerOrName;
 		const oldMarker = this._markers.get( markerName );
 
 		if ( oldMarker ) {
 			const oldRange = oldMarker.getRange();
 
-			if ( oldRange.isEqual( range ) ) {
+			if ( oldRange.isEqual( range ) && managedUsingOperations === oldMarker.managedUsingOperations ) {
 				return oldMarker;
 			}
 
-			this.remove( markerName );
+			this._remove( markerName );
 		}
 
 		const liveRange = LiveRange.createFromRange( range );
-		const marker = new Marker( markerName, liveRange );
+		const marker = new Marker( markerName, liveRange, managedUsingOperations );
 
 		this._markers.set( markerName, marker );
 		this.fire( 'add:' + markerName, marker );
@@ -131,10 +133,11 @@ export default class MarkerCollection {
 	/**
 	 * Removes given {@link ~Marker marker} or a marker with given name from the `MarkerCollection`.
 	 *
+	 * @protected
 	 * @param {String} markerOrName Marker or name of a marker to remove.
 	 * @returns {Boolean} `true` if marker was found and removed, `false` otherwise.
 	 */
-	remove( markerOrName ) {
+	_remove( markerOrName ) {
 		const markerName = markerOrName instanceof Marker ? markerOrName.name : markerOrName;
 		const oldMarker = this._markers.get( markerName );
 
@@ -281,20 +284,28 @@ class Marker {
 	 * @param {String} name Marker name.
 	 * @param {module:engine/model/liverange~LiveRange} liveRange Range marked by the marker.
 	 */
-	constructor( name, liveRange ) {
+	constructor( name, liveRange, managedUsingOperations ) {
 		/**
 		 * Marker's name.
 		 *
 		 * @readonly
-		 * @member {String} #name
+		 * @type {String}
 		 */
 		this.name = name;
 
 		/**
+		 * Flag indicates if the marker should be managed using operations.
+		 *
+		 * @readonly
+		 * @type {Boolean}
+		 */
+		this.managedUsingOperations = managedUsingOperations;
+
+		/**
 		 * Range marked by the marker.
 		 *
 		 * @protected
-		 * @member {module:engine/model/liverange~LiveRange} #_liveRange
+		 * @type {module:engine/model/liverange~LiveRange}
 		 */
 		this._liveRange = liveRange;
 

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

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

+ 7 - 6
packages/ckeditor5-engine/src/model/writer.js

@@ -791,7 +791,7 @@ export default class Writer {
 				throw new CKEditorError( 'writer-setMarker-no-range: Range parameter is required when adding a new marker.' );
 			}
 
-			return this.model.markers.set( name, newRange );
+			return this.model.markers._set( name, newRange, options.usingOperation );
 		}
 
 		const currentMarker = this.model.markers.get( name );
@@ -827,9 +827,8 @@ export default class Writer {
 	 *
 	 * @param {module:engine/model/markercollection~Marker|String} markerOrName Marker or marker name to remove.
 	 * @param {Object} [options]
-	 * @param {Boolean} [options.usingOperation=false] Flag indicated whether the marker should be removed by MarkerOperation.
 	 */
-	removeMarker( markerOrName, options = { usingOperation: false } ) {
+	removeMarker( markerOrName ) {
 		this._assertWriterUsedCorrectly();
 
 		const name = typeof markerOrName == 'string' ? markerOrName : markerOrName.name;
@@ -843,13 +842,15 @@ export default class Writer {
 			throw new CKEditorError( 'writer-removeMarker-no-marker: Trying to remove marker which does not exist.' );
 		}
 
-		if ( !options.usingOperation ) {
-			this.model.markers.remove( name );
+		const marker = this.model.markers.get( name );
+
+		if ( !marker.managedUsingOperations ) {
+			this.model.markers._remove( name );
 
 			return;
 		}
 
-		const oldRange = this.model.markers.get( name ).getRange();
+		const oldRange = marker.getRange();
 
 		addMarkerOperation( this, name, oldRange, null );
 	}

+ 32 - 32
packages/ckeditor5-engine/tests/model/markercollection.js

@@ -26,8 +26,8 @@ describe( 'MarkerCollection', () => {
 
 	describe( 'iterator', () => {
 		it( 'should return markers added to the marker collection', () => {
-			markers.set( 'a', range );
-			markers.set( 'b', range );
+			markers._set( 'a', range );
+			markers._set( 'b', range );
 
 			const markerA = markers.get( 'a' );
 			const markerB = markers.get( 'b' );
@@ -40,11 +40,11 @@ describe( 'MarkerCollection', () => {
 		} );
 	} );
 
-	describe( 'set', () => {
+	describe( '_set', () => {
 		it( 'should create a marker, fire add:<markerName> event and return true', () => {
 			sinon.spy( markers, 'fire' );
 
-			const result = markers.set( 'name', range );
+			const result = markers._set( 'name', range );
 			const marker = markers.get( 'name' );
 
 			expect( result ).to.equal( marker );
@@ -54,11 +54,11 @@ describe( 'MarkerCollection', () => {
 		} );
 
 		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 );
+			const marker1 = markers._set( 'name', range );
 
 			sinon.spy( markers, 'fire' );
 
-			const marker2 = markers.set( 'name', range2 );
+			const marker2 = markers._set( 'name', range2 );
 
 			expect( markers.fire.calledWithExactly( 'remove:name', marker1 ) ).to.be.true;
 			expect( markers.fire.calledWithExactly( 'add:name', marker2 ) ).to.be.true;
@@ -70,21 +70,21 @@ describe( 'MarkerCollection', () => {
 		} );
 
 		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 );
+			const marker1 = markers._set( 'name', range );
 
 			sinon.spy( markers, 'fire' );
 
-			const marker2 = markers.set( 'name', range );
+			const marker2 = markers._set( 'name', range );
 
 			expect( marker1 ).to.equal( marker2 );
 			expect( markers.fire.notCalled ).to.be.true;
 		} );
 
 		it( 'should accept marker instance instead of name', () => {
-			markers.set( 'name', range );
+			markers._set( 'name', range );
 			const marker1 = markers.get( 'name' );
 
-			const result = markers.set( marker1, range2 );
+			const result = markers._set( marker1, range2 );
 			const marker2 = markers.get( 'name' );
 
 			expect( result ).to.equal( marker2 );
@@ -99,7 +99,7 @@ describe( 'MarkerCollection', () => {
 		} );
 
 		it( 'should return true if marker with given name is in the collection', () => {
-			markers.set( 'name', range );
+			markers._set( 'name', range );
 			expect( markers.has( 'name' ) ).to.be.true;
 		} );
 	} );
@@ -114,13 +114,13 @@ describe( 'MarkerCollection', () => {
 		} );
 	} );
 
-	describe( 'remove', () => {
+	describe( '_remove', () => {
 		it( 'should remove marker, return true and fire remove:<markerName> event', () => {
-			const marker = markers.set( 'name', range );
+			const marker = markers._set( 'name', range );
 
 			sinon.spy( markers, 'fire' );
 
-			const result = markers.remove( 'name' );
+			const result = markers._remove( 'name' );
 
 			expect( result ).to.be.true;
 			expect( markers.fire.calledWithExactly( 'remove:name', marker ) ).to.be.true;
@@ -128,13 +128,13 @@ describe( 'MarkerCollection', () => {
 		} );
 
 		it( 'should destroy marker instance', () => {
-			const marker = markers.set( 'name', range );
+			const marker = markers._set( 'name', range );
 			const liveRange = marker._liveRange;
 
 			sinon.spy( marker, 'stopListening' );
 			sinon.spy( liveRange, 'detach' );
 
-			markers.remove( 'name' );
+			markers._remove( 'name' );
 
 			expect( marker.stopListening.calledOnce ).to.be.true;
 			expect( marker._liveRange ).to.be.null;
@@ -142,22 +142,22 @@ describe( 'MarkerCollection', () => {
 		} );
 
 		it( 'should return false if name has not been found in collection', () => {
-			markers.set( 'name', range );
+			markers._set( 'name', range );
 
 			sinon.spy( markers, 'fire' );
 
-			const result = markers.remove( 'other' );
+			const result = markers._remove( 'other' );
 
 			expect( result ).to.be.false;
 			expect( markers.fire.notCalled ).to.be.true;
 		} );
 
 		it( 'should accept marker instance instead of name', () => {
-			const marker = markers.set( 'name', range );
+			const marker = markers._set( 'name', range );
 
 			sinon.spy( markers, 'fire' );
 
-			const result = markers.remove( marker );
+			const result = markers._remove( marker );
 
 			expect( result ).to.be.true;
 			expect( markers.fire.calledWithExactly( 'remove:name', marker ) ).to.be.true;
@@ -167,10 +167,10 @@ describe( 'MarkerCollection', () => {
 
 	describe( 'getMarkersGroup', () => {
 		it( 'returns all markers which names start on given prefix', () => {
-			const markerFooA = markers.set( 'foo:a', range );
-			const markerFooB = markers.set( 'foo:b', range );
-			markers.set( 'bar:a', range );
-			markers.set( 'foobar:a', range );
+			const markerFooA = markers._set( 'foo:a', range );
+			const markerFooB = markers._set( 'foo:b', range );
+			markers._set( 'bar:a', range );
+			markers._set( 'foobar:a', range );
 
 			expect( Array.from( markers.getMarkersGroup( 'foo' ) ) ).to.deep.equal( [ markerFooA, markerFooB ] );
 			expect( Array.from( markers.getMarkersGroup( 'a' ) ) ).to.deep.equal( [] );
@@ -179,8 +179,8 @@ describe( 'MarkerCollection', () => {
 
 	describe( 'getMarkersAtPosition', () => {
 		it( 'should return iterator iterating over all markers that contains given position', () => {
-			markers.set( 'a', range );
-			const markerB = markers.set( 'b', range2 );
+			markers._set( 'a', range );
+			const markerB = markers._set( 'b', range2 );
 
 			const result = Array.from( markers.getMarkersAtPosition( Position.createAt( root, 1 ) ) );
 
@@ -190,8 +190,8 @@ describe( 'MarkerCollection', () => {
 
 	describe( 'destroy', () => {
 		it( 'should make MarkerCollection stop listening to all events and destroy all markers', () => {
-			const markerA = markers.set( 'a', range );
-			const markerB = markers.set( 'b', range2 );
+			const markerA = markers._set( 'a', range );
+			const markerB = markers._set( 'b', range2 );
 
 			sinon.spy( markers, 'stopListening' );
 			sinon.spy( markerA, 'stopListening' );
@@ -221,7 +221,7 @@ describe( 'Marker', () => {
 		root.appendChildren( new Text( 'foo' ) );
 
 		const range = Range.createFromParentsAndOffsets( root, 1, root, 2 );
-		const marker = model.markers.set( 'name', range );
+		const marker = model.markers._set( 'name', range );
 
 		expect( marker.getRange().isEqual( range ) ).to.be.true;
 		expect( marker.getStart().isEqual( range.start ) ).to.be.true;
@@ -240,9 +240,9 @@ describe( 'Marker', () => {
 
 	it( 'should throw when using the API if marker was removed from markers collection', () => {
 		const range = Range.createFromParentsAndOffsets( root, 1, root, 2 );
-		const marker = model.markers.set( 'name', range );
+		const marker = model.markers._set( 'name', range );
 
-		model.markers.remove( 'name' );
+		model.markers._remove( 'name' );
 
 		expect( () => {
 			marker.getRange();
@@ -259,7 +259,7 @@ describe( 'Marker', () => {
 
 	it( 'should delegate events from live range', () => {
 		const range = Range.createFromParentsAndOffsets( root, 1, root, 2 );
-		const marker = model.markers.set( 'name', range );
+		const marker = model.markers._set( 'name', range );
 
 		const eventRange = sinon.spy();
 		const eventContent = sinon.spy();

+ 7 - 7
packages/ckeditor5-engine/tests/model/operation/markeroperation.js

@@ -30,14 +30,14 @@ describe( 'MarkerOperation', () => {
 	} );
 
 	it( 'should add marker to document marker collection', () => {
-		sinon.spy( model.markers, 'set' );
+		sinon.spy( model.markers, '_set' );
 
 		model.applyOperation( wrapInDelta(
 			new MarkerOperation( 'name', null, range, model.markers, doc.version )
 		) );
 
 		expect( doc.version ).to.equal( 1 );
-		expect( model.markers.set.calledWith( 'name', matchRange( range ) ) );
+		expect( model.markers._set.calledWith( 'name', matchRange( range ) ) );
 		expect( model.markers.get( 'name' ).getRange().isEqual( range ) ).to.be.true;
 	} );
 
@@ -48,14 +48,14 @@ describe( 'MarkerOperation', () => {
 
 		const range2 = Range.createFromParentsAndOffsets( root, 0, root, 3 );
 
-		sinon.spy( model.markers, 'set' );
+		sinon.spy( model.markers, '_set' );
 
 		model.applyOperation( wrapInDelta(
 			new MarkerOperation( 'name', range, range2, model.markers, doc.version )
 		) );
 
 		expect( doc.version ).to.equal( 2 );
-		expect( model.markers.set.calledWith( 'name', matchRange( range2 ) ) );
+		expect( model.markers._set.calledWith( 'name', matchRange( range2 ) ) );
 		expect( model.markers.get( 'name' ).getRange().isEqual( range2 ) ).to.be.true;
 	} );
 
@@ -64,14 +64,14 @@ describe( 'MarkerOperation', () => {
 			new MarkerOperation( 'name', null, range, model.markers, doc.version )
 		) );
 
-		sinon.spy( model.markers, 'remove' );
+		sinon.spy( model.markers, '_remove' );
 
 		model.applyOperation( wrapInDelta(
 			new MarkerOperation( 'name', range, null, model.markers, doc.version )
 		) );
 
 		expect( doc.version ).to.equal( 2 );
-		expect( model.markers.remove.calledWith( 'name' ) );
+		expect( model.markers._remove.calledWith( 'name' ) );
 		expect( model.markers.get( 'name' ) ).to.be.null;
 	} );
 
@@ -87,7 +87,7 @@ describe( 'MarkerOperation', () => {
 
 	it( 'should not fire document markers set event if newRange is same as current marker range', () => {
 		model.change( writer => {
-			writer.setMarker( 'name', range );
+			writer.setMarker( 'name', range, { usingOperation: true } );
 		} );
 
 		sinon.spy( model.markers, 'fire' );

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

@@ -1982,6 +1982,18 @@ describe( 'Writer', () => {
 			expect( model.markers.get( 'name' ) ).to.equal( marker );
 		} );
 
+		it( 'should return marker with properly set managedUsingOperations (to true)', () => {
+			const marker = setMarker( 'name', range, { usingOperation: true } );
+
+			expect( marker.managedUsingOperations ).to.equal( true );
+		} );
+
+		it( 'should return marker with properly set managedUsingOperations (to false)', () => {
+			const marker = setMarker( 'name', range, { usingOperation: false } );
+
+			expect( marker.managedUsingOperations ).to.equal( false );
+		} );
+
 		it( 'should update marker in the document marker collection', () => {
 			setMarker( 'name', range );
 
@@ -2094,15 +2106,15 @@ describe( 'Writer', () => {
 			expect( model.markers.get( 'name' ) ).to.be.null;
 		} );
 
-		it( 'should use MarkerOperation when usingOperation is set to true', () => {
-			setMarker( 'name', range );
+		it( 'should use MarkerOperation when marker was created using operation', () => {
+			setMarker( 'name', range, { usingOperation: true } );
 
 			const marker = model.markers.get( 'name' );
 			const spy = sinon.spy();
 
 			model.on( 'applyOperation', spy );
 
-			removeMarker( marker, { usingOperation: true } );
+			removeMarker( marker );
 
 			expect( spy.calledOnce ).to.be.true;
 			expect( spy.firstCall.args[ 1 ][ 0 ].type ).to.equal( 'marker' );