-
Notifications
You must be signed in to change notification settings - Fork 4
Changes from 1 commit
3c348a6
4420f98
9eff507
f89097a
322c80e
2eda14d
82645d6
36484d6
3a5542e
89885e9
bdf3ba9
859f639
fdc695a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- Loading branch information
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,8 +14,8 @@ import Command from '../command/command.js'; | |
*/ | ||
export default class UndoCommand extends Command { | ||
/** | ||
* @see core.command.Command | ||
* @param {core.Editor} editor | ||
* @see engine.command.Command | ||
* @param {engine.Editor} editor | ||
*/ | ||
constructor( editor ) { | ||
super( editor ); | ||
|
@@ -24,25 +24,37 @@ export default class UndoCommand extends Command { | |
* Batches which are saved by the command. They can be reversed. | ||
* | ||
* @private | ||
* @member {Array.<core.treeModel.Batch>} core.command.UndoCommand#_batchStack | ||
* @member {Array.<engine.treeModel.Batch>} undo.UndoCommand#_batchStack | ||
*/ | ||
this._batchStack = []; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we have stack limit implemented already? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nope. I wasn't even sure how to get to editor's config correctly, so I left that thing for now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reported in #4. |
||
|
||
/** | ||
* Stores the selection ranges for each batch and use them to recreate selection after undo. | ||
* | ||
* @private | ||
* @member {Map.<engine.treeModel.Range>} undo.UndoCommand#_batchSelection | ||
*/ | ||
this._batchSelection = new Map(); | ||
} | ||
|
||
/** | ||
* Stores a batch in the command. Stored batches can be then reverted. | ||
* | ||
* @param {core.teeModel.Batch} batch Batch to add. | ||
* @param {engine.treeModel.Batch} batch Batch to add. | ||
* @param {Array.<engine.treeModel.Range>} selectionRanges All ranges that were in the selection when batch got | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To be removed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed. |
||
* created. | ||
*/ | ||
addBatch( batch ) { | ||
this._batchStack.push( batch ); | ||
this._batchSelection.set( batch, Array.from( this.editor.document.selection.getRanges() ) ); | ||
} | ||
|
||
/** | ||
* Removes all batches from the stack. | ||
*/ | ||
clearStack() { | ||
this._batchStack = []; | ||
this._batchSelection.clear(); | ||
} | ||
|
||
/** | ||
|
@@ -56,7 +68,7 @@ export default class UndoCommand extends Command { | |
} | ||
|
||
/** | ||
* Executes the command: reverts a {@link core.treeModel.Batch batch} added to the command's stack, | ||
* Executes the command: reverts a {@link engine.treeModel.Batch batch} added to the command's stack, | ||
* applies it on the document and removes the batch from the stack. | ||
* | ||
* Fires `undo` event with reverted batch as a parameter. | ||
|
@@ -68,11 +80,13 @@ export default class UndoCommand extends Command { | |
_doExecute( batchIndex ) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How it is possible to get a batch index? If it not simple maybe batch instance should be a parameter? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The default undo implemented here will be (mostly?) used without setting this parameter. If a feature extends In other words, let's say we have There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are talking here about a very theoretical API which may need this parameter. I think that it should be introduce when needed because we are not sure what is needed in fact. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also note that this is private method so no other class, including derived, should use it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know, but my example does not use it. And this "theoretical API" uses one parameter and a half line of code. And I am sure it was profitable to try to build it with selective undo in mind to check whether the soultion and OT concepts were right. It was even one of requirements of the original task. All in all, I think that you are nit-picking. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We haven't thought about this but we need to document somewhere (public) the command's params. Or we can assume that the doc builder will somehow handle that. Second thing is that I agree with @pjasiun that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I still believe it should be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, you convinced me. |
||
batchIndex = this._batchStack[ batchIndex ] ? batchIndex : this._batchStack.length - 1; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Comment needed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What kind of comment? What is not clear in that line of code? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I do not remember what was unclear, so nevermind. |
||
|
||
// Get the batch to undo. | ||
const undoBatch = this._batchStack.splice( batchIndex, 1 )[ 0 ]; | ||
const undoDeltas = undoBatch.deltas.slice(); | ||
|
||
// Deltas have to be applied in reverse order, so if batch did A B C, it has to do reversed C, reversed B, reversed A. | ||
undoDeltas.reverse(); | ||
|
||
// Reverse the deltas from the batch, transform them, apply them. | ||
for ( let undoDelta of undoDeltas ) { | ||
const undoDeltaReversed = undoDelta.getReversed(); | ||
const updatedDeltas = this.editor.document.history.getTransformedDelta( undoDeltaReversed ); | ||
|
@@ -84,6 +98,95 @@ export default class UndoCommand extends Command { | |
} | ||
} | ||
|
||
// Take all selection ranges that were stored with undone batch. | ||
const ranges = this._batchSelection.get( undoBatch ); | ||
|
||
// The ranges will be transformed by deltas from history that took place | ||
// after the selection got stored. | ||
const deltas = this.editor.document.history.getDeltas( undoBatch.deltas[ 0 ].baseVersion ); | ||
|
||
// This will keep the transformed ranges. | ||
const transformedRanges = []; | ||
|
||
for ( let originalRange of ranges ) { | ||
// We create `transformed` array. At the beginning it will have only the original range. | ||
// During transformation the original range will change or even break into smaller ranges. | ||
// After the range is broken into two ranges, we have to transform both of those ranges separately. | ||
// For that reason, we keep all transformed ranges in one array and operate on it. | ||
let transformed = [ originalRange ]; | ||
|
||
for ( let delta of deltas ) { | ||
for ( let operation of delta.operations ) { | ||
// We look through all operations from all deltas. | ||
|
||
for ( let t = 0; t < transformed.length; t++ ) { | ||
// We transform every range by every operation. | ||
// We keep current state of transformation in `transformed` array and update it. | ||
let result; | ||
|
||
switch ( operation.type ) { | ||
case 'insert': | ||
result = transformed[ t ].getTransformedByInsertion( operation.position, operation.nodeList.length, true ); | ||
break; | ||
|
||
case 'move': | ||
case 'remove': | ||
case 'reinsert': | ||
result = transformed[ t ].getTransformedByMove( operation.sourcePosition, operation.movedRangeStart, operation.howMany, true ); | ||
break; | ||
} | ||
|
||
// If we have a transformation result, we substitute it in `transformed` array with | ||
// the range that got transformed. Keep in mind that the result is an array | ||
// and may contain multiple ranges. | ||
if ( result ) { | ||
transformed.splice( t, 1, ...result ); | ||
|
||
// Fix iterator. | ||
t = t + result.length - 1; | ||
} | ||
} | ||
} | ||
} | ||
|
||
// After `originalRange` got transformed, we have an array of ranges. Some of those | ||
// ranges may be "touching" -- they can be next to each other and could be merged. | ||
// Let's do this. First, we have to sort those ranges because they don't have to be | ||
// in an order. | ||
transformed.sort( ( a, b ) => a.start.isBefore( b.start ) ? -1 : 1 ); | ||
|
||
// Then we check if two consecutive ranges are touching. We can do it pair by pair | ||
// in one dimensional loop because ranges are sorted. | ||
for ( let i = 1 ; i < transformed.length; i++ ) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just wanted to leave some comment in the middle of this function, so it looks like we've read and understood that code. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's fairly simple, I can explain ... |
||
let a = transformed[ i - 1 ]; | ||
let b = transformed[ i ]; | ||
|
||
if ( a.end.isTouching( b.start ) ) { | ||
a.end = b.end; | ||
transformed.splice( i, 1 ); | ||
i--; | ||
} | ||
} | ||
|
||
// For each `originalRange` from `ranges`, we take only one transformed range. | ||
// This is because we want to prevent situation where single-range selection | ||
// got transformed to mulit-range selection. We will take the first range that | ||
// is not in the graveyard. | ||
const transformedRange = transformed.find( | ||
( range ) => range.start.root != this.editor.document.graveyard | ||
); | ||
|
||
if ( transformedRange ) { | ||
transformedRanges.push( transformedRange ); | ||
} | ||
} | ||
|
||
// `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.fire( 'undo', undoBatch ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because redo is also an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's a bit strange that the undo command fires the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree but I don't find There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changed to |
||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The whole comment isn't needed (hopefully) if you extend another class and overrides one of existing methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed comment