Browse Source

Changed: UndoCommand now restores selection direction. + fixes after changing to WeakMap.

Szymon Cofalik 9 years ago
parent
commit
4a9bb95383

+ 14 - 6
packages/ckeditor5-undo/src/undocommand.js

@@ -29,10 +29,10 @@ export default class UndoCommand extends Command {
 		this._batchStack = [];
 
 		/**
-		 * Stores the selection ranges for each batch and use them to recreate selection after undo.
+		 * For each batch, it stores the information about selection needed to recreate it after undo.
 		 *
 		 * @private
-		 * @member {WeakMap.<engine.treeModel.Range>} undo.UndoCommand#_batchSelection
+		 * @member {WeakMap} undo.UndoCommand#_batchSelection
 		 */
 		this._batchSelection = new WeakMap();
 	}
@@ -46,7 +46,13 @@ export default class UndoCommand extends Command {
 	 */
 	addBatch( batch ) {
 		this._batchStack.push( batch );
-		this._batchSelection.set( batch, Array.from( this.editor.document.selection.getRanges() ) );
+		this._batchSelection.set(
+			batch,
+			{
+				ranges: Array.from( this.editor.document.selection.getRanges() ),
+				isBackward: this.editor.document.selection.isBackward
+			}
+		);
 	}
 
 	/**
@@ -54,7 +60,6 @@ export default class UndoCommand extends Command {
 	 */
 	clearStack() {
 		this._batchStack = [];
-		this._batchSelection.clear();
 	}
 
 	/**
@@ -98,8 +103,11 @@ export default class UndoCommand extends Command {
 			}
 		}
 
+		// Get the selection state stored with this batch.
+		const selectionState = this._batchSelection.get( undoBatch );
+
 		// Take all selection ranges that were stored with undone batch.
-		const ranges = this._batchSelection.get( undoBatch );
+		const ranges = selectionState.ranges;
 
 		// The ranges will be transformed by deltas from history that took place
 		// after the selection got stored.
@@ -184,7 +192,7 @@ export default class UndoCommand extends Command {
 		// `transformedRanges` may be empty if all ranges ended up in graveyard.
 		// If that is the case, do not restore selection.
 		if ( transformedRanges.length ) {
-			this.editor.document.selection.setRanges( transformedRanges );
+			this.editor.document.selection.setRanges( transformedRanges, selectionState.isBackward );
 		}
 
 		this.fire( 'undo', undoBatch );

+ 19 - 28
packages/ckeditor5-undo/tests/undocommand.js

@@ -37,29 +37,12 @@ describe( 'UndoCommand', () => {
 		} );
 	} );
 
-	describe( 'addBatch', () => {
-		it( 'should add a batch and selection to command stack', () => {
-			root.appendChildren( 'foobar' );
-
-			editor.document.selection.setRanges( Range.createFromElement( root ) );
-
-			const batch = doc.batch();
-			undo.addBatch( batch );
-
-			expect( undo._batchStack.length ).to.equal( 1 );
-			expect( undo._batchStack[ 0 ] ).to.equal( batch );
-
-			expect( undo._batchSelection.get( batch ) ).to.deep.equal( Array.from( editor.document.selection.getRanges() ) );
-		} );
-	} );
-
 	describe( 'clearStack', () => {
 		it( 'should remove all batches from the stack', () => {
 			undo.addBatch( doc.batch() );
 			undo.clearStack();
 
 			expect( undo._batchStack.length ).to.equal( 0 );
-			expect( undo._batchSelection.size ).to.equal( 0 );
 		} );
 	} );
 
@@ -75,7 +58,7 @@ describe( 'UndoCommand', () => {
 		} );
 	} );
 
-	describe( '_doExecute', () => {
+	describe( '_execute', () => {
 		const p = pos => new Position( root, [].concat( pos ) );
 		const r = ( a, b ) => new Range( p( a ), p( b ) );
 
@@ -99,7 +82,8 @@ describe( 'UndoCommand', () => {
 			 - a
 			 - r{}
 			 */
-			editor.document.selection.setRanges( [ r( 2, 4 ) ] );
+			// Let's make things spicy and this time, make a backward selection.
+			editor.document.selection.setRanges( [ r( 2, 4 ) ], true );
 			batch1 = doc.batch();
 			undo.addBatch( batch1 );
 			batch1.setAttr( 'key', 'value', r( 2, 4 ) );
@@ -155,7 +139,7 @@ describe( 'UndoCommand', () => {
 		} );
 
 		it( 'should revert changes done by deltas from the batch that was most recently added to the command stack', () => {
-			undo._doExecute();
+			undo._execute();
 
 			// Selection is restored. Wrap is removed:
 			/*
@@ -173,8 +157,9 @@ describe( 'UndoCommand', () => {
 			expect( root.getChild( 5 ).getAttribute( 'key' ) ).to.equal( 'value' );
 
 			expect( editor.document.selection.getRanges().next().value.isEqual( r( 0, 3 ) ) ).to.be.true;
+			expect( editor.document.selection.isBackward ).to.be.false;
 
-			undo._doExecute();
+			undo._execute();
 
 			// Two moves are removed:
 			/*
@@ -192,8 +177,9 @@ describe( 'UndoCommand', () => {
 			expect( root.getChild( 3 ).getAttribute( 'key' ) ).to.equal( 'value' );
 
 			expect( editor.document.selection.getRanges().next().value.isEqual( r( 1, 3 ) ) ).to.be.true;
+			expect( editor.document.selection.isBackward ).to.be.false;
 
-			undo._doExecute();
+			undo._execute();
 
 			// Set attribute is undone:
 			/*
@@ -211,8 +197,9 @@ describe( 'UndoCommand', () => {
 			expect( root.getChild( 3 ).hasAttribute( 'key' ) ).to.be.false;
 
 			expect( editor.document.selection.getRanges().next().value.isEqual( r( 2, 4 ) ) ).to.be.true;
+			expect( editor.document.selection.isBackward ).to.be.true;
 
-			undo._doExecute();
+			undo._execute();
 
 			// Insert is undone:
 			/*
@@ -226,7 +213,7 @@ describe( 'UndoCommand', () => {
 		it( 'should revert changes done by deltas from given batch, if parameter was passed (test: revert set attribute)', () => {
 			// editor.document.selection.setRanges( [ r( [ 0, 1 ], [ 0, 2 ] ) ] );
 
-			undo._doExecute( 1 );
+			undo._execute( 1 );
 			// Remove attribute:
 			/*
 				[root]
@@ -248,10 +235,11 @@ describe( 'UndoCommand', () => {
 			// Selection is only partially restored because the range got broken.
 			// The selection would have to best on letter "b" and letter "o", but it is set only on letter "b".
 			expect( editor.document.selection.getRanges().next().value.isEqual( r( [ 0, 0 ], [ 0, 1 ] ) ) ).to.be.true;
+			expect( editor.document.selection.isBackward ).to.be.true;
 		} );
 
 		it( 'should revert changes done by deltas from given batch, if parameter was passed (test: revert insert foobar)', () => {
-			undo._doExecute( 0 );
+			undo._execute( 0 );
 			// Remove foobar:
 			/*
 			 [root]
@@ -267,8 +255,9 @@ describe( 'UndoCommand', () => {
 			// Because P element was inserted in the middle of removed text and it was not removed,
 			// the selection is set after it.
 			expect( editor.document.selection.getRanges().next().value.isEqual( r( 1, 1 ) ) ).to.be.true;
+			expect( editor.document.selection.isBackward ).to.be.false;
 
-			undo._doExecute( 0 );
+			undo._execute( 0 );
 			// Remove attributes.
 			// This does nothing in the `root` because attributes were set on nodes that already got removed.
 			// But those nodes should change in they graveyard and we can check them there.
@@ -278,6 +267,7 @@ describe( 'UndoCommand', () => {
 
 			// Operations for undoing that batch were working on graveyard so document selection should not change.
 			expect( editor.document.selection.getRanges().next().value.isEqual( r( 1, 1 ) ) ).to.be.true;
+			expect( editor.document.selection.isBackward ).to.be.false;
 
 			expect( doc.graveyard.getChildCount() ).to.equal( 6 );
 
@@ -286,13 +276,14 @@ describe( 'UndoCommand', () => {
 			}
 
 			// Let's undo wrapping. This should leave us with empty root.
-			undo._doExecute( 1 );
+			undo._execute( 1 );
 			expect( root.getChildCount() ).to.equal( 0 );
 
 			// Once again transformed range ends up in the graveyard.
 			// So we do not restore it. But since Selection is a LiveRange itself it will update
 			// because the node before it (P element) got removed.
 			expect( editor.document.selection.getRanges().next().value.isEqual( r( 0, 0 ) ) ).to.be.true;
+			expect( editor.document.selection.isBackward ).to.be.false;
 		} );
 
 		it( 'should fire undo event with the undone batch', () => {
@@ -301,7 +292,7 @@ describe( 'UndoCommand', () => {
 
 			undo.on( 'undo', spy );
 
-			undo._doExecute();
+			undo._execute();
 
 			expect( spy.calledOnce ).to.be.true;
 			expect( spy.calledWith( batch ) );