Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

Commit

Permalink
Merge pull request #1497 from ckeditor/t/ot-followups
Browse files Browse the repository at this point in the history
Other: Moved error logging from debug tools to OT code.
Other: Aligned `Schema#getValidRanges` results to changes in `AttributeOperation`.
Other: Unified `RemoveOperation` and `ReinsertOperation` to have just one `MoveOperation`.
Other: Simplified `LiveRange#event:change` second parameter which is now an object containing `Position` not an `Operation`.
Internal: Operational transformations refactor followups. 
Internal: Additional improvements for the new OT algorithms.
Internal: Added relations to better solve cases in undo.
Internal: Improved cloning mechanism inside `WrapOperation`.
Internal: Improved `model.Range#getTransformedByMergeOperation`.
Internal: Other improvements in OT algorithms.
Internal: Improvements in docs.

Closes #1468. Closes #1464. Closes #1467. Closes #1460. Closes #1461. Closes #1462. Closes #1463. Closes #1475. Closes #1479.

BREAKING CHANGE: `LiveRange#event:change` second parameter is now an object containing property `deletionPosition`. It can be `model.Position` instance, if the range was moved to the graveyard root. The position is equal to the position from which nodes were removed. Otherwise, it is set to `null`.

BREAKING CHANGE: `Schema#getValidRanges` will return only flat ranges now. If an attribute is allowed on some nodes and in those nodes children, multiple "nested" ranges will be returned.

BREAKING CHANGE: `Schema#getValidRanges` is a generator now.
  • Loading branch information
Piotr Jasiun committed Aug 9, 2018
2 parents f7f2837 + 3d9af07 commit 74f00e9
Show file tree
Hide file tree
Showing 42 changed files with 1,203 additions and 1,135 deletions.
19 changes: 0 additions & 19 deletions src/dev-utils/enableenginedebug.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ import MoveOperation from '../model/operation/moveoperation';
import NoOperation from '../model/operation/nooperation';
import RenameOperation from '../model/operation/renameoperation';
import RootAttributeOperation from '../model/operation/rootattributeoperation';
import transform from '../model/operation/transform';
import Model from '../model/model';
import ModelDocument from '../model/document';
import ModelDocumentFragment from '../model/documentfragment';
Expand Down Expand Up @@ -349,24 +348,6 @@ function enableLoggingTools() {
`"${ this.key }": ${ JSON.stringify( this.oldValue ) } -> ${ JSON.stringify( this.newValue ) }, ${ this.root.rootName }`;
} );

const _transformTransform = transform.transform;

sandbox.mock( transform, 'transform', function( a, b, context ) {
let results;

try {
results = _transformTransform( a, b, context );
} catch ( e ) {
logger.error( 'Error during operation transformation!' );
logger.error( a.toString() + ( context.isStrong ? ' (important)' : '' ) );
logger.error( b.toString() + ( context.isStrong ? '' : ' (important)' ) );

throw e;
}

return results;
} );

sandbox.mock( ViewText.prototype, 'toString', function() {
return `#${ this.data }`;
} );
Expand Down
4 changes: 2 additions & 2 deletions src/model/documentselection.js
Original file line number Diff line number Diff line change
Expand Up @@ -750,14 +750,14 @@ class LiveSelection extends Selection {

const liveRange = LiveRange.createFromRange( range );

liveRange.on( 'change:range', ( evt, oldRange, operation ) => {
liveRange.on( 'change:range', ( evt, oldRange, data ) => {
this._hasChangedRange = true;

// If `LiveRange` is in whole moved to the graveyard, save necessary data. It will be fixed on `Model#applyOperation` event.
if ( liveRange.root == this._document.graveyard ) {
this._fixGraveyardRangesData.push( {
liveRange,
sourcePosition: operation.sourcePosition
sourcePosition: data.deletionPosition
} );
}
} );
Expand Down
18 changes: 13 additions & 5 deletions src/model/liverange.js
Original file line number Diff line number Diff line change
Expand Up @@ -86,9 +86,7 @@ export default class LiveRange extends Range {
* @param {Object} data Object with additional information about the change. Those parameters are passed from
* {@link module:engine/model/document~Document#event:change document change event}.
* @param {String} data.type Change type.
* @param {module:engine/model/batch~Batch} data.batch Batch which changed the live range.
* @param {module:engine/model/range~Range} data.range Range containing the result of applied change.
* @param {module:engine/model/position~Position} data.sourcePosition Source position for move, remove and reinsert change types.
* @param {module:engine/model/position~Position|null} deletionPosition Source position for move, remove and reinsert change types.
*/

/**
Expand Down Expand Up @@ -145,18 +143,28 @@ function transform( operation ) {
const result = Range.createFromRanges( ranges );
const boundariesChanged = !result.isEqual( this );
const contentChanged = doesOperationChangeRangeContent( this, operation );
let deletionPosition = null;

if ( boundariesChanged ) {
if ( result.root.rootName == '$graveyard' ) {
if ( operation.type == 'remove' ) {
deletionPosition = operation.sourcePosition;
} else {
// Merge operation.
deletionPosition = operation.deletionPosition;
}
}

// If range boundaries have changed, fire `change:range` event.
const oldRange = Range.createFromRange( this );

this.start = result.start;
this.end = result.end;

this.fire( 'change:range', oldRange, operation );
this.fire( 'change:range', oldRange, { deletionPosition } );
} else if ( contentChanged ) {
// If range boundaries have not changed, but there was change inside the range, fire `change:content` event.
this.fire( 'change:content', Range.createFromRange( this ), operation );
this.fire( 'change:content', Range.createFromRange( this ), { deletionPosition } );
}
}

Expand Down
1 change: 0 additions & 1 deletion src/model/operation/detachoperation.js
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,6 @@ export default class DetachOperation extends Operation {
if ( this.sourcePosition.root.document ) {
/**
* Cannot detach document node.
* Use {@link module:engine/model/operation/removeoperation~RemoveOperation remove operation} instead.
*
* @error detach-operation-on-document-node
*/
Expand Down
6 changes: 3 additions & 3 deletions src/model/operation/insertoperation.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
import Operation from './operation';
import Position from '../position';
import NodeList from '../nodelist';
import RemoveOperation from './removeoperation';
import MoveOperation from './moveoperation';
import { _insert, _normalizeNodes } from './utils';
import Text from '../text';
import Element from '../element';
Expand Down Expand Up @@ -87,13 +87,13 @@ export default class InsertOperation extends Operation {
/**
* See {@link module:engine/model/operation/operation~Operation#getReversed `Operation#getReversed()`}.
*
* @returns {module:engine/model/operation/removeoperation~RemoveOperation}
* @returns {module:engine/model/operation/moveoperation~MoveOperation}
*/
getReversed() {
const graveyard = this.position.root.document.graveyard;
const gyPosition = new Position( graveyard, [ 0 ] );

return new RemoveOperation( this.position, this.nodes.maxOffset, gyPosition, this.baseVersion + 1 );
return new MoveOperation( this.position, this.nodes.maxOffset, gyPosition, this.baseVersion + 1 );
}

/**
Expand Down
6 changes: 6 additions & 0 deletions src/model/operation/moveoperation.js
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,12 @@ export default class MoveOperation extends Operation {
* @inheritDoc
*/
get type() {
if ( this.targetPosition.root.rootName == '$graveyard' ) {
return 'remove';
} else if ( this.sourcePosition.root.rootName == '$graveyard' ) {
return 'reinsert';
}

return 'move';
}

Expand Down
4 changes: 0 additions & 4 deletions src/model/operation/operationfactory.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,6 @@ import MarkerOperation from '../operation/markeroperation';
import MoveOperation from '../operation/moveoperation';
import NoOperation from '../operation/nooperation';
import Operation from '../operation/operation';
import ReinsertOperation from '../operation/reinsertoperation';
import RemoveOperation from '../operation/removeoperation';
import RenameOperation from '../operation/renameoperation';
import RootAttributeOperation from '../operation/rootattributeoperation';
import SplitOperation from '../operation/splitoperation';
Expand All @@ -29,8 +27,6 @@ operations[ MarkerOperation.className ] = MarkerOperation;
operations[ MoveOperation.className ] = MoveOperation;
operations[ NoOperation.className ] = NoOperation;
operations[ Operation.className ] = Operation;
operations[ ReinsertOperation.className ] = ReinsertOperation;
operations[ RemoveOperation.className ] = RemoveOperation;
operations[ RenameOperation.className ] = RenameOperation;
operations[ RootAttributeOperation.className ] = RootAttributeOperation;
operations[ SplitOperation.className ] = SplitOperation;
Expand Down
76 changes: 0 additions & 76 deletions src/model/operation/reinsertoperation.js

This file was deleted.

60 changes: 0 additions & 60 deletions src/model/operation/removeoperation.js

This file was deleted.

4 changes: 4 additions & 0 deletions src/model/operation/splitoperation.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,10 @@ export default class SplitOperation extends Operation {
this.position.stickiness = 'toNext';

this.graveyardPosition = graveyardPosition ? Position.createFromPosition( graveyardPosition ) : null;

if ( this.graveyardPosition ) {
this.graveyardPosition.stickiness = 'toNext';
}
}

/**
Expand Down

0 comments on commit 74f00e9

Please sign in to comment.