浏览代码

Changes made to engine.treeView.Selection: getRanges method returns iterator, removed getRangeAt and removeRangeAt methods, added docs.

Szymon Kupś 9 年之前
父节点
当前提交
356eb335f2
共有 2 个文件被更改,包括 29 次插入92 次删除
  1. 17 36
      packages/ckeditor5-engine/src/treeview/selection.js
  2. 12 56
      packages/ckeditor5-engine/tests/treeview/selection.js

+ 17 - 36
packages/ckeditor5-engine/src/treeview/selection.js

@@ -14,6 +14,17 @@ import EmitterMixin from '../../utils/emittermixin.js';
 /**
  * Class representing selection in tree view.
  *
+ * Selection can consist of {@link engine.treeView.Range ranges} that can be added using
+ * {@link engine.treeView.Selection#addRange addRange} and {@link engine.treeView.Selection#setRanges setRanges} methods.
+ * Both methods create copies of provided ranges and store those copies internally. Further modifications to passed
+ * ranges will not change selection's state.
+ * Selection's ranges can be obtained via {@link engine.treeView.Selection#getRanges getRanges},
+ * {@link engine.treeView.Selection#getFirstRange getFirstRange} and {@link engine.treeView.Selection#getLastRange getLastRange}
+ * methods, which return copies of ranges stored inside selection. Modifications made on these copies will not change
+ * selection's state. Similar situation occurs when getting {@link engine.treeView.Selection#anchor anchor},
+ * {@link engine.treeView.Selection#focus focus}, {@link engine.treeView.Selection#getFirstPosition first} and
+ * {@link engine.treeView.Selection#getLastPosition last} positions - all will return copies of requested positions.
+ *
  * @memberOf engine.treeView
  */
 export default class Selection {
@@ -120,25 +131,14 @@ export default class Selection {
 	}
 
 	/**
-	 * Returns an array of ranges added to the selection. The method returns a copy of internal array, so
-	 * it will not change when ranges get added or removed from selection.
+	 * Returns an iterator that contains copies of all ranges added to the selection.
 	 *
-	 * @returns {Array.<engine.treeView.Range>}
+	 * @returns {Iterator.<engine.treeView.Range>}
 	 */
-	getRanges() {
-		return this._ranges.slice();
-	}
-
-	/**
-	 * Returns copy of a range at specified index, or `null` if there is no range under that index.
-	 *
-	 * @param {Number} index Index of range in selection.
-	 * @returns {engine.treeView.Range|null}
-	 */
-	getRangeAt( index ) {
-		const range = this._ranges[ index ];
-
-		return range ? Range.createFromRange( range ) : null;
+	*getRanges() {
+		for ( let range of this._ranges ) {
+			yield Range.createFromRange( range );
+		}
 	}
 
 	/**
@@ -207,25 +207,6 @@ export default class Selection {
 	}
 
 	/**
-	 * Removes range at given index.
-	 *
-	 * @fires engine.treeView.Selection#change
-	 * @param {Number} index
-	 * @returns {engine.treeView.Range|null} Returns removed range or `null` if there is no range under given index.
-	 */
-	removeRangeAt( index ) {
-		const removed = this._ranges.splice( index, 1 );
-
-		if ( removed.length ) {
-			this.fire( 'change' );
-
-			return removed[ 0 ];
-		}
-
-		return null;
-	}
-
-	/**
 	 * Removes all ranges that were added to the selection.
 	 *
 	 * @fires engine.treeView.Selection#change

+ 12 - 56
packages/ckeditor5-engine/tests/treeview/selection.js

@@ -115,12 +115,12 @@ describe( 'Selection', () => {
 	describe( 'addRange', () => {
 		it( 'should add range to selection ranges', () => {
 			selection.addRange( range1 );
-			expect( selection.getRangeAt( 0 ).isEqual( range1 ) ).to.be.true;
+			expect( selection._ranges[ 0 ].isEqual( range1 ) ).to.be.true;
 		} );
 
 		it( 'should fire change event', ( done ) => {
 			selection.once( 'change', () => {
-				expect( selection.getRangeAt( 0 ).isEqual( range1 ) ).to.be.true;
+				expect( selection._ranges[ 0 ].isEqual( range1 ) ).to.be.true;
 				done();
 			} );
 
@@ -141,11 +141,13 @@ describe( 'Selection', () => {
 	} );
 
 	describe( 'getRanges', () => {
-		it( 'should return copy array of all ranges', () => {
+		it( 'should return iterator with copies of all ranges', () => {
 			selection.addRange( range1 );
 			selection.addRange( range2 );
 
-			const ranges = selection.getRanges();
+			const iterable = selection.getRanges();
+			const ranges = Array.from( iterable );
+
 			expect( ranges.length ).to.equal( 2 );
 			expect( ranges[ 0 ].isEqual( range1 ) ).to.be.true;
 			expect( ranges[ 0 ] ).to.not.equal( range1 );
@@ -154,25 +156,6 @@ describe( 'Selection', () => {
 		} );
 	} );
 
-	describe( 'getRangeAt', () => {
-		it( 'should return copy of range at specified index', () => {
-			selection.addRange( range1 );
-			selection.addRange( range2 );
-			selection.addRange( range3 );
-
-			expect( selection.getRangeAt( 0 ).isEqual( range1 ) ).to.be.true;
-			expect( selection.getRangeAt( 1 ).isEqual( range2 ) ).to.be.true;
-			expect( selection.getRangeAt( 2 ).isEqual( range3 ) ).to.be.true;
-			expect( selection.getRangeAt( 0 ) ).to.not.equal( range1 );
-			expect( selection.getRangeAt( 1 ) ).to.not.equal( range2 );
-			expect( selection.getRangeAt( 2 ) ).to.not.equal( range3 );
-		} );
-
-		it( 'should return null if no range at given index', () => {
-			expect( selection.getRangeAt( 0 ) ).to.be.null;
-		} );
-	} );
-
 	describe( 'getFirstRange', () => {
 		it( 'should return copy of range with first position', () => {
 			selection.addRange( range1 );
@@ -241,33 +224,6 @@ describe( 'Selection', () => {
 		} );
 	} );
 
-	describe( 'removeRangeAt', () => {
-		it( 'should remove range at given index', () => {
-			selection.addRange( range1 );
-			selection.addRange( range2 );
-			selection.addRange( range3 );
-
-			const range = selection.removeRangeAt( 1 );
-			expect( range.isEqual( range2 ) ).to.be.true;
-			expect( selection.rangeCount ).to.equal( 2 );
-		} );
-
-		it( 'should return null if no range at specified index', () => {
-			expect( selection.removeRangeAt( 2 ) ).to.be.null;
-		} );
-
-		it( 'should fire change event', ( done ) => {
-			selection.addRange( range1 );
-
-			selection.once( 'change', () => {
-				expect( selection.rangeCount ).to.equal( 0 );
-				done();
-			} );
-
-			selection.removeRangeAt( 0 );
-		} );
-	} );
-
 	describe( 'removeAllRanges', () => {
 		it( 'should remove all ranges and fire change event', ( done ) => {
 			selection.addRange( range1 );
@@ -296,10 +252,10 @@ describe( 'Selection', () => {
 
 			selection.once( 'change', () => {
 				expect( selection.rangeCount ).to.equal( 2 );
-				expect( selection.getRangeAt( 0 ).isEqual( range2 ) ).to.be.true;
-				expect( selection.getRangeAt( 0 ) ).is.not.equal( range2 );
-				expect( selection.getRangeAt( 1 ).isEqual( range3 ) ).to.be.true;
-				expect( selection.getRangeAt( 1 ) ).is.not.equal( range3 );
+				expect( selection._ranges[ 0 ].isEqual( range2 ) ).to.be.true;
+				expect( selection._ranges[ 0 ] ).is.not.equal( range2 );
+				expect( selection._ranges[ 1 ].isEqual( range3 ) ).to.be.true;
+				expect( selection._ranges[ 1 ] ).is.not.equal( range3 );
 				done();
 			} );
 
@@ -313,7 +269,7 @@ describe( 'Selection', () => {
 			selection.once( 'change', () => {
 				expect( selection.rangeCount ).to.equal( 1 );
 				expect( selection.isCollapsed ).to.be.true;
-				expect( selection.getRangeAt( 0 ).start.isEqual( range2.start ) ).to.be.true;
+				expect( selection._ranges[ 0 ].start.isEqual( range2.start ) ).to.be.true;
 				done();
 			} );
 
@@ -336,7 +292,7 @@ describe( 'Selection', () => {
 			selection.once( 'change', () => {
 				expect( selection.rangeCount ).to.equal( 1 );
 				expect( selection.isCollapsed ).to.be.true;
-				expect( selection.getRangeAt( 0 ).end.isEqual( range3.end ) ).to.be.true;
+				expect( selection._ranges[ 0 ].end.isEqual( range3.end ) ).to.be.true;
 				done();
 			} );