Merge pull request #1216 from ckeditor/t/1207

Other: Refactored events fired by model classes. Closes #1207.

Author: Piotr Jasiun

src/controller/editingcontroller.js

@@ -8,7 +8,6 @@
  */
 
 import RootEditableElement from '../view/rooteditableelement';
-import ModelDiffer from '../model/differ';
 import ViewDocument from '../view/document';
 import Mapper from '../conversion/mapper';
 import ModelConversionDispatcher from '../conversion/modelconversiondispatcher';
@@ -84,47 +83,15 @@ export default class EditingController {
 			viewSelection: this.view.selection
 		} );
 
-		// Model differ object. It's role is to buffer changes done on model and then calculates a diff of those changes.
-		// The diff is then passed to model conversion dispatcher which generates proper events and kicks-off conversion.
-		const modelDiffer = new ModelDiffer();
-
-		// Before an operation is applied on model, buffer the change in differ.
-		this.listenTo( this.model, 'applyOperation', ( evt, args ) => {
-			const operation = args[ 0 ];
-
-			if ( operation.isDocumentOperation ) {
-				modelDiffer.bufferOperation( operation );
-			}
-		}, { priority: 'high' } );
-
-		// Buffer marker changes.
-		// This is not covered in buffering operations because markers may change outside of them (when they
-		// are modified using `model.document.markers` collection, not through `MarkerOperation`).
-		this.listenTo( this.model.markers, 'add', ( evt, marker ) => {
-			// Whenever a new marker is added, buffer that change.
-			modelDiffer.bufferMarkerChange( marker.name, null, marker.getRange() );
-
-			// Whenever marker changes, buffer that.
-			marker.on( 'change', ( evt, oldRange ) => {
-				modelDiffer.bufferMarkerChange( marker.name, oldRange, marker.getRange() );
-			} );
-		} );
-
-		this.listenTo( this.model.markers, 'remove', ( evt, marker ) => {
-			// Whenever marker is removed, buffer that change.
-			modelDiffer.bufferMarkerChange( marker.name, marker.getRange(), null );
-		} );
+		const doc = this.model.document;
 
 		// When all changes are done, get the model diff containing all the changes and convert them to view and then render to DOM.
-		this.listenTo( this.model, 'changesDone', () => {
+		this.listenTo( doc, 'change', () => {
 			// Convert changes stored in `modelDiffer`.
-			this.modelToView.convertChanges( modelDiffer );
-
-			// Reset model diff object. When next operation is applied, new diff will be created.
-			modelDiffer.reset();
+			this.modelToView.convertChanges( doc.differ );
 
 			// After the view is ready, convert selection from model to view.
-			this.modelToView.convertSelection( this.model.document.selection );
+			this.modelToView.convertSelection( doc.selection );
 
 			// When everything is converted to the view, render it to DOM.
 			this.view.render();

src/conversion/modelconversiondispatcher.js

@@ -135,11 +135,6 @@ export default class ModelConversionDispatcher {
 
 		// Convert changes that happened on model tree.
 		for ( const entry of differ.getChanges() ) {
-			// Skip all the changes that happens in graveyard. These are not converted.
-			if ( _isInGraveyard( entry ) ) {
-				continue;
-			}
-
 			if ( entry.type == 'insert' ) {
 				this.convertInsert( Range.createFromPositionAndShift( entry.position, entry.length ) );
 			} else if ( entry.type == 'remove' ) {
@@ -596,9 +591,3 @@ function shouldMarkerChangeBeConverted( modelPosition, marker, mapper ) {
 
 	return !hasCustomHandling;
 }
-
-// Checks whether entry change describes changes that happen in graveyard.
-function _isInGraveyard( entry ) {
-	return ( entry.position && entry.position.root.rootName == '$graveyard' ) ||
-		( entry.range && entry.range.root.rootName == '$graveyard' );
-}

src/dev-utils/enableenginedebug.js

@@ -656,7 +656,8 @@ class DebugPlugin extends Plugin {
 	constructor( editor ) {
 		super( editor );
 
-		const modelDocument = this.editor.model.document;
+		const model = this.editor.model;
+		const modelDocument = model.document;
 		const viewDocument = this.editor.editing.view;
 
 		modelDocument[ treeDump ] = [];
@@ -665,11 +666,11 @@ class DebugPlugin extends Plugin {
 		dumpTrees( modelDocument, modelDocument.version );
 		dumpTrees( viewDocument, modelDocument.version );
 
-		modelDocument.on( 'change', () => {
+		model.on( 'applyOperation', () => {
 			dumpTrees( modelDocument, modelDocument.version );
 		}, { priority: 'lowest' } );
 
-		modelDocument.on( 'changesDone', () => {
+		model.document.on( 'change', () => {
 			dumpTrees( viewDocument, modelDocument.version );
 		}, { priority: 'lowest' } );
 	}

src/model/differ.js

@@ -61,6 +61,40 @@ export default class Differ {
 		 * @type {Number}
 		 */
 		this._changeCount = 0;
+
+		/**
+		 * For efficiency purposes, `Differ` stores the change set returned by the differ after {@link #getChanges} call.
+		 * Cache is reset each time a new operation is buffered. If the cache has not been reset, {@link #getChanges} will
+		 * return the cached value instead of calculating it again.
+		 *
+		 * This property stores those changes that did not take place in graveyard root.
+		 *
+		 * @private
+		 * @type {Array.<Object>|null}
+		 */
+		this._cachedChanges = null;
+
+		/**
+		 * For efficiency purposes, `Differ` stores the change set returned by the differ after {@link #getChanges} call.
+		 * Cache is reset each time a new operation is buffered. If the cache has not been reset, {@link #getChanges} will
+		 * return the cached value instead of calculating it again.
+		 *
+		 * This property stores all changes evaluated by `Differ`, also those that took place in graveyard.
+		 *
+		 * @private
+		 * @type {Array.<Object>|null}
+		 */
+		this._cachedChangesWithGraveyard = null;
+	}
+
+	/**
+	 * Informs whether there are any changes buffered in `Differ`.
+	 *
+	 * @readonly
+	 * @type {Boolean}
+	 */
+	get isEmpty() {
+		return this._changesInElement.size == 0 && this._changedMarkers.size == 0;
 	}
 
 	/**
@@ -98,6 +132,9 @@ export default class Differ {
 
 				break;
 		}
+
+		// Clear cache after each buffered operation as it is no longer valid.
+		this._cachedChanges = null;
 	}
 
 	/**
@@ -168,9 +205,24 @@ export default class Differ {
 	 * the position on which the change happened. If a position {@link module:engine/model/position~Position#isBefore is before}
 	 * another one, it will be on an earlier index in the diff set.
 	 *
+	 * Because calculating diff is a costly operation, the result is cached. If no new operation was buffered since the
+	 * previous {@link #getChanges} call, the next call with return the cached value.
+	 *
+	 * @param {Object} options Additional options.
+	 * @param {Boolean} [options.includeChangesInGraveyard=false] If set to `true`, also changes that happened
+	 * in graveyard root will be returned. By default, changes in graveyard root are not returned.
 	 * @returns {Array.<Object>} Diff between old and new model tree state.
 	 */
-	getChanges() {
+	getChanges( options = { includeChangesInGraveyard: false } ) {
+		// If there are cached changes, just return them instead of calculating changes again.
+		if ( this._cachedChanges ) {
+			if ( options.includeChangesInGraveyard ) {
+				return this._cachedChangesWithGraveyard.slice();
+			} else {
+				return this._cachedChanges.slice();
+			}
+		}
+
 		// Will contain returned results.
 		const diffSet = [];
 
@@ -320,7 +372,15 @@ export default class Differ {
 
 		this._changeCount = 0;
 
-		return diffSet;
+		// Cache changes.
+		this._cachedChangesWithGraveyard = diffSet.slice();
+		this._cachedChanges = diffSet.slice().filter( _changesInGraveyardFilter );
+
+		if ( options.includeChangesInGraveyard ) {
+			return this._cachedChangesWithGraveyard;
+		} else {
+			return this._cachedChanges;
+		}
 	}
 
 	/**
@@ -330,6 +390,7 @@ export default class Differ {
 		this._changesInElement.clear();
 		this._elementSnapshots.clear();
 		this._changedMarkers.clear();
+		this._cachedChanges = null;
 	}
 
 	/**
@@ -865,3 +926,11 @@ function _generateActionsFromChanges( oldChildrenLength, changes ) {
 
 	return actions;
 }
+
+// Filter callback for Array.filter that filters out change entries that are in graveyard.
+function _changesInGraveyardFilter( entry ) {
+	const posInGy = entry.position && entry.position.root.rootName == '$graveyard';
+	const rangeInGy = entry.range && entry.range.root.rootName == '$graveyard';
+
+	return !posInGy && !rangeInGy;
+}