Merge pull request #114 from ckeditor/t/110

Fix: Moved EditingController, DataController and EditingKeystrokeHandler from StandardEditor to Editor class. Closes #110.

Author: Piotr Jasiun

src/editor/editor.js

@@ -8,17 +8,21 @@
  */
 
 import Config from '@ckeditor/ckeditor5-utils/src/config';
+import EditingController from '@ckeditor/ckeditor5-engine/src/controller/editingcontroller';
 import PluginCollection from '../plugincollection';
 import CommandCollection from '../commandcollection';
 import Locale from '@ckeditor/ckeditor5-utils/src/locale';
 import DataController from '@ckeditor/ckeditor5-engine/src/controller/datacontroller';
 import Model from '@ckeditor/ckeditor5-engine/src/model/model';
+import EditingKeystrokeHandler from '../editingkeystrokehandler';
 
 import ObservableMixin from '@ckeditor/ckeditor5-utils/src/observablemixin';
 import mix from '@ckeditor/ckeditor5-utils/src/mix';
 
 /**
- * Class representing a basic editor. It contains a base architecture, without much additional logic.
+ * Class representing the base of the editor. It is the API all plugins can expect to get when using editor property.
+ * Editors implementation (like Classic Editor or Inline Editor) should extend this class. They can add their own
+ * methods and properties.
  *
  * See also {@link module:core/editor/standardeditor~StandardEditor}.
  *
@@ -73,53 +77,57 @@ export default class Editor {
 		 */
 		this.t = this.locale.t;
 
+		/**
+		 * Defines whether this editor is in read-only mode.
+		 *
+		 * In read-only mode the editor {@link #commands commands} are disabled so it is not possible
+		 * to modify document using them.
+		 *
+		 * @observable
+		 * @member {Boolean} #isReadOnly
+		 */
+		this.set( 'isReadOnly', false );
+
 		/**
 		 * The editor's model.
 		 *
 		 * The center of the editor's abstract data model.
 		 *
-		 * Besides the model, the editor usually contains two controllers –
-		 * {@link #data data controller} and {@link #editing editing controller}.
-		 * The former is used e.g. when setting or retrieving editor data and contains a useful
-		 * set of methods for operating on the content. The latter controls user input and rendering
-		 * the content for editing.
-		 *
 		 * @readonly
 		 * @member {module:engine/model/model~Model}
 		 */
 		this.model = new Model();
 
+		// Creates main root.
+		this.model.document.createRoot();
+
 		/**
 		 * The {@link module:engine/controller/datacontroller~DataController data controller}.
+		 * Used e.g. for setting or retrieving editor data.
 		 *
 		 * @readonly
 		 * @member {module:engine/controller/datacontroller~DataController}
 		 */
 		this.data = new DataController( this.model );
 
 		/**
-		 * Defines whether this editor is in read-only mode.
-		 *
-		 * In read-only mode the editor {@link #commands commands} are disabled so it is not possible
-		 * to modify document using them.
+		 * The {@link module:engine/controller/editingcontroller~EditingController editing controller}.
+		 * Controls user input and rendering the content for editing.
 		 *
-		 * @observable
-		 * @member {Boolean} #isReadOnly
+		 * @readonly
+		 * @member {module:engine/controller/editingcontroller~EditingController}
 		 */
-		this.set( 'isReadOnly', false );
+		this.editing = new EditingController( this.model );
+		this.editing.view.bind( 'isReadOnly' ).to( this );
 
 		/**
-		 * The {@link module:engine/controller/editingcontroller~EditingController editing controller}.
-		 *
-		 * This property is set by more specialized editor classes (such as {@link module:core/editor/standardeditor~StandardEditor}),
-		 * however, it's required for features to work as their engine-related parts will try to connect converters.
-		 *
-		 * When defining a virtual editor class, like one working in Node.js, it's possible to plug a virtual
-		 * editing controller which only instantiates necessary properties, but without any observers and listeners.
+		 * Instance of the {@link module:core/editingkeystrokehandler~EditingKeystrokeHandler}.
 		 *
 		 * @readonly
-		 * @member {module:engine/controller/editingcontroller~EditingController} #editing
+		 * @member {module:core/editingkeystrokehandler~EditingKeystrokeHandler}
 		 */
+		this.keystrokes = new EditingKeystrokeHandler( this );
+		this.keystrokes.listenTo( this.editing.view );
 	}
 
 	/**
@@ -173,6 +181,8 @@ export default class Editor {
 			.then( () => {
 				this.model.destroy();
 				this.data.destroy();
+				this.editing.destroy();
+				this.keystrokes.destroy();
 			} );
 	}
 

src/editor/standardeditor.js

@@ -8,16 +8,16 @@
  */
 
 import Editor from './editor';
-import EditingKeystrokeHandler from '../editingkeystrokehandler';
-import EditingController from '@ckeditor/ckeditor5-engine/src/controller/editingcontroller';
 import isFunction from '@ckeditor/ckeditor5-utils/src/lib/lodash/isFunction';
 
 import getDataFromElement from '@ckeditor/ckeditor5-utils/src/dom/getdatafromelement';
 import setDataInElement from '@ckeditor/ckeditor5-utils/src/dom/setdatainelement';
 
 /**
- * Class representing a typical browser-based editor. It handles a single source element and
- * uses {@link module:engine/controller/editingcontroller~EditingController}.
+ * The standard editor class, extending base editor by adding methods needed in the typical
+ * editor implementations: editor with a single editable area created based on the DOM element.
+ * It provides base API to manage data and to integrate the editor with the form when it is
+ * initialized on the textarea element.
  *
  * @extends module:core/editor/editor~Editor
  */
@@ -40,18 +40,6 @@ export default class StandardEditor extends Editor {
 		 */
 		this.element = element;
 
-		// Documented in Editor.
-		this.editing = new EditingController( this.model );
-		this.editing.view.bind( 'isReadOnly' ).to( this );
-
-		/**
-		 * Instance of the {@link module:core/editingkeystrokehandler~EditingKeystrokeHandler}.
-		 *
-		 * @readonly
-		 * @member {module:core/editingkeystrokehandler~EditingKeystrokeHandler}
-		 */
-		this.keystrokes = new EditingKeystrokeHandler( this );
-
 		/**
 		 * Editor UI instance.
 		 *
@@ -63,21 +51,9 @@ export default class StandardEditor extends Editor {
 		 * @member {module:core/editor/editorui~EditorUI} #ui
 		 */
 
-		this.keystrokes.listenTo( this.editing.view );
-
 		this._attachToForm();
 	}
 
-	/**
-	 * @inheritDoc
-	 */
-	destroy() {
-		return Promise.resolve()
-			.then( () => this.keystrokes.destroy() )
-			.then( () => this.editing.destroy() )
-			.then( super.destroy() );
-	}
-
 	/**
 	 * Sets the data in the editor's main root.
 	 *

tests/_utils-tests/classictesteditor.js

@@ -37,13 +37,6 @@ describe( 'ClassicTestEditor', () => {
 			expect( editor.element ).to.equal( editorElement );
 			expect( editor.ui ).to.be.instanceOf( ClassicTestEditorUI );
 			expect( editor.ui.view ).to.be.instanceOf( BoxedEditorUIView );
-		} );
-
-		it( 'creates model and view roots', () => {
-			const editor = new ClassicTestEditor( editorElement );
-
-			expect( editor.model.document.getRoot() ).to.have.property( 'name', '$root' );
-			expect( editor.editing.view.getRoot() ).to.have.property( 'name', 'div' );
 			expect( editor.data.processor ).to.be.instanceof( HtmlDataProcessor );
 		} );
 

tests/_utils-tests/modeltesteditor.js

@@ -21,15 +21,22 @@ describe( 'ModelTestEditor', () => {
 			const editor = new ModelTestEditor( { foo: 1 } );
 
 			expect( editor ).to.be.instanceof( Editor );
-
 			expect( editor.config.get( 'foo' ) ).to.equal( 1 );
+			expect( editor.data.processor ).to.be.instanceof( HtmlDataProcessor );
 		} );
 
-		it( 'creates model and view roots', () => {
+		it( 'creates model root', () => {
 			const editor = new ModelTestEditor( { foo: 1 } );
 
 			expect( editor.model.document.getRoot() ).to.have.property( 'name', '$root' );
-			expect( editor.data.processor ).to.be.instanceof( HtmlDataProcessor );
+		} );
+
+		it( 'should disable editing pipeline and clear main root', () => {
+			const editor = new ModelTestEditor( { foo: 1 } );
+
+			editor.model.document.createRoot( 'second', 'second' );
+
+			expect( Array.from( editor.editing.view.roots ) ).to.length( 0 );
 		} );
 	} );
 

tests/_utils-tests/virtualtesteditor.js

@@ -19,16 +19,8 @@ describe( 'VirtualTestEditor', () => {
 			const editor = new VirtualTestEditor( { foo: 1 } );
 
 			expect( editor ).to.be.instanceof( StandardEditor );
-
-			expect( editor.config.get( 'foo' ) ).to.equal( 1 );
-		} );
-
-		it( 'creates model and view roots', () => {
-			const editor = new VirtualTestEditor( { foo: 1 } );
-
-			expect( editor.model.document.getRoot() ).to.have.property( 'name', '$root' );
-			expect( editor.editing.view.getRoot() ).to.have.property( 'name', 'div' );
 			expect( editor.data.processor ).to.be.instanceof( HtmlDataProcessor );
+			expect( editor.config.get( 'foo' ) ).to.equal( 1 );
 		} );
 	} );
 

tests/_utils/classictesteditor.js

@@ -23,8 +23,6 @@ export default class ClassicTestEditor extends StandardEditor {
 	constructor( element, config ) {
 		super( element, config );
 
-		this.model.document.createRoot();
-		this.editing.createRoot( 'div' );
 		this.data.processor = new HtmlDataProcessor();
 
 		this.ui = new ClassicTestEditorUI( this, new BoxedEditorUIView( this.locale ) );

tests/_utils/modeltesteditor.js

@@ -18,9 +18,11 @@ export default class ModelTestEditor extends Editor {
 	constructor( config ) {
 		super( config );
 
-		this.model.document.createRoot();
-
 		this.data.processor = new HtmlDataProcessor();
+
+		// Disable editing pipeline for model editor.
+		this.editing.destroy();
+		this.editing.view.roots.clear();
 	}
 
 	/**

tests/_utils/virtualtesteditor.js

@@ -18,10 +18,6 @@ export default class VirtualTestEditor extends StandardEditor {
 	constructor( config ) {
 		super( null, config );
 
-		this.model.document.createRoot();
-
-		this.editing.createRoot( 'div' );
-
 		this.data.processor = new HtmlDataProcessor();
 	}
 

tests/editor/editor.js

@@ -8,11 +8,13 @@
 import Editor from '../../src/editor/editor';
 import Plugin from '../../src/plugin';
 import Config from '@ckeditor/ckeditor5-utils/src/config';
+import EditingController from '@ckeditor/ckeditor5-engine/src/controller/editingcontroller';
 import PluginCollection from '../../src/plugincollection';
 import CommandCollection from '../../src/commandcollection';
 import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror';
 import Locale from '@ckeditor/ckeditor5-utils/src/locale';
 import Command from '../../src/command';
+import EditingKeystrokeHandler from '../../src/editingkeystrokehandler';
 
 class PluginA extends Plugin {
 	constructor( editor ) {
@@ -103,6 +105,8 @@ describe( 'Editor', () => {
 
 			expect( editor.config ).to.be.an.instanceof( Config );
 			expect( editor.commands ).to.be.an.instanceof( CommandCollection );
+			expect( editor.editing ).to.be.instanceof( EditingController );
+			expect( editor.keystrokes ).to.be.instanceof( EditingKeystrokeHandler );
 
 			expect( editor.plugins ).to.be.an.instanceof( PluginCollection );
 			expect( getPlugins( editor ) ).to.be.empty;
@@ -133,6 +137,31 @@ describe( 'Editor', () => {
 
 			expect( editor.config.get( 'bar' ) ).to.equal( 'foo' );
 		} );
+
+		it( 'should bind editing.view#isReadOnly to the editor', () => {
+			const editor = new Editor();
+
+			editor.isReadOnly = false;
+
+			expect( editor.editing.view.isReadOnly ).to.false;
+
+			editor.isReadOnly = true;
+
+			expect( editor.editing.view.isReadOnly ).to.true;
+		} );
+
+		it( 'should create main root element', () => {
+			const editor = new Editor();
+
+			expect( editor.model.document.getRoot( 'main' ) ).to.ok;
+		} );
+
+		it( 'should activate #keystrokes', () => {
+			const spy = sinon.spy( EditingKeystrokeHandler.prototype, 'listenTo' );
+			const editor = new Editor();
+
+			sinon.assert.calledWith( spy, editor.editing.view );
+		} );
 	} );
 
 	describe( 'plugins', () => {
@@ -192,15 +221,19 @@ describe( 'Editor', () => {
 		it( 'should destroy all components it initialized', () => {
 			const editor = new Editor();
 
-			const spy1 = sinon.spy( editor.data, 'destroy' );
-			const spy2 = sinon.spy( editor.model, 'destroy' );
-			const spy3 = sinon.spy( editor.plugins, 'destroy' );
+			const dataDestroySpy = sinon.spy( editor.data, 'destroy' );
+			const modelDestroySpy = sinon.spy( editor.model, 'destroy' );
+			const editingDestroySpy = sinon.spy( editor.editing, 'destroy' );
+			const pluginsDestroySpy = sinon.spy( editor.plugins, 'destroy' );
+			const keystrokesDestroySpy = sinon.spy( editor.keystrokes, 'destroy' );
 
 			return editor.destroy()
 				.then( () => {
-					expect( spy1.calledOnce ).to.be.true;
-					expect( spy2.calledOnce ).to.be.true;
-					expect( spy3.calledOnce ).to.be.true;
+					sinon.assert.calledOnce( dataDestroySpy );
+					sinon.assert.calledOnce( modelDestroySpy );
+					sinon.assert.calledOnce( editingDestroySpy );
+					sinon.assert.calledOnce( pluginsDestroySpy );
+					sinon.assert.calledOnce( keystrokesDestroySpy );
 				} );
 		} );
 	} );