Merge pull request #8148 from ckeditor/i/8143

Fix (engine): `model.History#getOperations` was returning incorrect values if history had operations with negative version numbers or version numbers differing by more than one. Closes #8143.

packages/ckeditor5-engine/src/model/history.js

@@ -60,29 +60,37 @@ export default class History {
 	/**
 	 * Returns operations added to the history.
 	 *
-	 * @param {Number} [from=0] Base version from which operations should be returned (inclusive). Defaults to `0`, which means
-	 * that operations from the first one will be returned.
+	 * @param {Number} [from=Number.NEGATIVE_INFINITY] Base version from which operations should be returned (inclusive).
+	 * Defaults to `Number.NEGATIVE_INFINITY`, which means that operations from the first one will be returned.
 	 * @param {Number} [to=Number.POSITIVE_INFINITY] Base version up to which operations should be returned (exclusive).
 	 * Defaults to `Number.POSITIVE_INFINITY` which means that operations up to the last one will be returned.
-	 * @returns {Iterable.<module:engine/model/operation/operation~Operation>} Operations added to the history.
+	 * @returns {Array.<module:engine/model/operation/operation~Operation>} Operations added to the history.
 	 */
-	getOperations( from = 0, to = Number.POSITIVE_INFINITY ) {
-		if ( from < 0 ) {
-			return [];
+	getOperations( from = Number.NEGATIVE_INFINITY, to = Number.POSITIVE_INFINITY ) {
+		const operations = [];
+
+		for ( const operation of this._operations ) {
+			if ( operation.baseVersion >= from && operation.baseVersion < to ) {
+				operations.push( operation );
+			}
 		}
 
-		return this._operations.slice( from, to );
+		return operations;
 	}
 
 	/**
 	 * Returns operation from the history that bases on given `baseVersion`.
 	 *
 	 * @param {Number} baseVersion Base version of the operation to get.
-	 * @returns {module:engine/model/operation/operation~Operation|null} Operation with given base version or `null` if
+	 * @returns {module:engine/model/operation/operation~Operation|undefined} Operation with given base version or `undefined` if
 	 * there is no such operation in history.
 	 */
 	getOperation( baseVersion ) {
-		return this._operations[ baseVersion ];
+		for ( const operation of this._operations ) {
+			if ( operation.baseVersion == baseVersion ) {
+				return operation;
+			}
+		}
 	}
 
 	/**

packages/ckeditor5-engine/tests/model/history.js

@@ -42,7 +42,11 @@ describe( 'History', () => {
 		} );
 
 		it( 'should save multiple operations and keep their order', () => {
-			const ops = getOperations();
+			const ops = [];
+
+			ops.push( new Operation( 0 ) );
+			ops.push( new Operation( 1 ) );
+			ops.push( new Operation( 2 ) );
 
 			for ( const op of ops ) {
 				history.addOperation( op );
@@ -78,31 +82,55 @@ describe( 'History', () => {
 	} );
 
 	describe( 'getOperations', () => {
-		let ops;
+		it( 'should return only operations from given base version', () => {
+			history.addOperation( new Operation( 0 ) );
+			history.addOperation( new Operation( 1 ) );
+			history.addOperation( new Operation( 2 ) );
 
-		beforeEach( () => {
-			ops = getOperations();
+			const ops = history.getOperations( 1 );
 
-			for ( const op of ops ) {
-				history.addOperation( op );
-			}
+			expect( ops.length ).to.equal( 2 );
+			expect( ops[ 0 ].baseVersion ).to.equal( 1 );
+			expect( ops[ 1 ].baseVersion ).to.equal( 2 );
 		} );
 
-		it( 'should return only operations from given base version', () => {
-			const historyOperations = Array.from( history.getOperations( 1 ) );
+		it( 'should return only operations up to given base version', () => {
+			history.addOperation( new Operation( 0 ) );
+			history.addOperation( new Operation( 1 ) );
+			history.addOperation( new Operation( 2 ) );
+
+			const ops = history.getOperations( 1, 2 );
+
+			expect( ops.length ).to.equal( 1 );
+			expect( ops[ 0 ].baseVersion ).to.equal( 1 );
+		} );
 
-			expect( historyOperations ).to.deep.equal( ops.slice( 1 ) );
+		it( 'should return empty array if no operations match', () => {
+			history.addOperation( new Operation( 0 ) );
+			history.addOperation( new Operation( 1 ) );
+
+			expect( history.getOperations( 20 ).length ).to.equal( 0 );
+			expect( history.getOperations( -3, 0 ).length ).to.equal( 0 );
 		} );
 
-		it( 'should return only operations up to given base version', () => {
-			const historyOperations = Array.from( history.getOperations( 1, 2 ) );
+		it( 'should return correct values if history holds operations with negative base version', () => {
+			history.addOperation( new Operation( -2 ) );
+			history.addOperation( new Operation( -1 ) );
+			history.addOperation( new Operation( 0 ) );
+			history.addOperation( new Operation( 1 ) );
+			history.addOperation( new Operation( 2 ) );
 
-			expect( historyOperations ).to.deep.equal( ops.slice( 1, 2 ) );
+			expect( history.getOperations( -1, 2 ).map( op => op.baseVersion ) ).to.deep.equal( [ -1, 0, 1 ] );
 		} );
 
-		it( 'should return empty array if no operations match', () => {
-			expect( Array.from( history.getOperations( 20 ) ).length ).to.equal( 0 );
-			expect( Array.from( history.getOperations( -1 ) ).length ).to.equal( 0 );
+		it( 'should return correct values if history holds operations with base versions that differ by more than one', () => {
+			history.addOperation( new Operation( 0 ) );
+			history.addOperation( new Operation( 4 ) );
+			history.addOperation( new Operation( 6 ) );
+			history.addOperation( new Operation( 9 ) );
+			history.addOperation( new Operation( 13 ) );
+
+			expect( history.getOperations( 2, 11 ).map( op => op.baseVersion ) ).to.deep.equal( [ 4, 6, 9 ] );
 		} );
 	} );
 
@@ -179,13 +207,3 @@ describe( 'History', () => {
 		} );
 	} );
 } );
-
-function getOperations() {
-	const ops = [];
-
-	ops.push( new Operation( 0 ) );
-	ops.push( new Operation( 1 ) );
-	ops.push( new Operation( 2 ) );
-
-	return ops;
-}