浏览代码

Merge pull request #89 from ckeditor/t/88

Other: The command API has been redesigned. The `Command` methods are now public and consistent. Commands can be used in a standalone mode (without the editor). The `CommandCollection` was introduced and replaced the `Map` of commands used in `editor.commands`. Closes #88.

  Besides changes mentioned in this point and in the "Breaking changes" section, other minor changes were done:

  * `Editor#execute()` now accepts multiple command arguments.
  * `Command#value` property was standardized.

BREAKING CHANGES: The `Command`'s protected `_doExecute()` and `_checkEnabled()` methods have been replaced by public `execute()` and `refresh()` methods.

BREAKING CHANGES: The `Command`'s `refreshState` event was removed and one should use `change:isEnabled` in order to control and override its state.

BREAKING CHANGES: The `core/command/command` module has been moved to the root directory (so the `Command` class is `core/command~Command` now).

BREAKING CHANGES: The `Command#refresh()` method is now automatically called on `editor.document#changesDone`.

BREAKING CHANGES: The `editor.commands` map was replaced by a `CommandCollection` instance so `editor.commands.set()` calls need to be replaced with `editor.commands.add()`.
Szymon Kupś 8 年之前
父节点
当前提交
661f894253

+ 122 - 0
packages/ckeditor5-core/src/command.js

@@ -0,0 +1,122 @@
+/**
+ * @license Copyright (c) 2003-2017, CKSource - Frederico Knabben. All rights reserved.
+ * For licensing, see LICENSE.md.
+ */
+
+/**
+ * @module core/command
+ */
+
+import ObservableMixin from '@ckeditor/ckeditor5-utils/src/observablemixin';
+import mix from '@ckeditor/ckeditor5-utils/src/mix';
+
+/**
+ * The base class for CKEditor commands.
+ *
+ * Commands are the main way to manipulate editor contents and state. They are mostly used by UI elements (or by other
+ * commands) to make changes in the model. Commands are available in every part of code that has access to
+ * the {@link module:core/editor/editor~Editor editor} instance.
+ *
+ * Instances of registered commands can be retrieved from {@link module:core/editor/editor~Editor#commands}.
+ * The easiest way to execute a command is through {@link module:core/editor/editor~Editor#execute}.
+ *
+ * @mixes module:utils/observablemixin~ObservableMixin
+ */
+export default class Command {
+	/**
+	 * Creates a new `Command` instance.
+	 *
+	 * @param {module:core/editor/editor~Editor} editor Editor on which this command will be used.
+	 */
+	constructor( editor ) {
+		/**
+		 * The editor on which this command will be used.
+		 *
+		 * @readonly
+		 * @member {module:core/editor/editor~Editor}
+		 */
+		this.editor = editor;
+
+		/**
+		 * The value of a command. Concrete command class should define what it represents.
+		 *
+		 * For example, the `bold` command's value is whether the selection starts in a bolded text.
+		 * And the value of the `link` command may be an object with links details.
+		 *
+		 * It's possible for a command to have no value (e.g. for stateless actions such as `uploadImage`).
+		 *
+		 * @observable
+		 * @readonly
+		 * @member #value
+		 */
+		this.set( 'value', undefined );
+
+		/**
+		 * Flag indicating whether a command is enabled or disabled.
+		 * A disabled command should do nothing when executed.
+		 *
+		 * @observable
+		 * @readonly
+		 * @member {Boolean} #isEnabled
+		 */
+		this.set( 'isEnabled', false );
+
+		this.decorate( 'execute' );
+
+		// By default every command is refreshed when changes are applied to the model.
+		this.listenTo( this.editor.document, 'changesDone', () => {
+			this.refresh();
+		} );
+
+		this.on( 'execute', evt => {
+			if ( !this.isEnabled ) {
+				evt.stop();
+			}
+		}, { priority: 'high' } );
+	}
+
+	/**
+	 * Refreshes the command. The command should update its {@link #isEnabled} and {@link #value} property
+	 * in this method.
+	 *
+	 * This method is automatically called when
+	 * {@link module:engine/model/document~Document#event:changesDone any changes are applied to the model}.
+	 */
+	refresh() {
+		this.isEnabled = true;
+	}
+
+	/**
+	 * Executes the command.
+	 *
+	 * A command may accept parameters. They will be passed from {@link module:core/editor/editor~Editor#execute}
+	 * to the command.
+	 *
+	 * The `execute()` method will automatically abort when the command is disabled ({@link #isEnabled} is `false`).
+	 * This behavior is implemented by a high priority listener to the {@link #event:execute} event.
+	 *
+	 * @fires execute
+	 */
+	execute() {}
+
+	/**
+	 * Destroys the command.
+	 */
+	destroy() {
+		this.stopListening();
+	}
+
+	/**
+	 * Event fired by the {@link #execute} method. The command action is a listener to this event so it's
+	 * possible to change/cancel the behavior of the command by listening to this event.
+	 *
+	 * See {@link module:utils/observablemixin~ObservableMixin.decorate} for more information and samples.
+	 *
+	 * **Note:** This event is fired even if command is disabled. However, it is automatically blocked
+	 * by a high priority listener in order to prevent command execution.
+	 *
+	 * @event execute
+	 */
+}
+
+mix( Command, ObservableMixin );

+ 0 - 146
packages/ckeditor5-core/src/command/command.js

@@ -1,146 +0,0 @@
-/**
- * @license Copyright (c) 2003-2017, CKSource - Frederico Knabben. All rights reserved.
- * For licensing, see LICENSE.md.
- */
-
-/**
- * @module core/command/command
- */
-
-import ObservableMixin from '@ckeditor/ckeditor5-utils/src/observablemixin';
-import mix from '@ckeditor/ckeditor5-utils/src/mix';
-
-/**
- * The base class for CKEditor commands.
- *
- * Commands are main way to manipulate editor contents and state. They are mostly used by UI elements (or by other
- * commands) to make changes in Tree Model. Commands are available in every part of code that has access to
- * {@link module:core/editor/editor~Editor} instance, since they are registered in it and executed through
- * {@link module:core/editor/editor~Editor#execute}.
- * Commands instances are available through {@link module:core/editor/editor~Editor#commands}.
- *
- * This is an abstract base class for all commands.
- *
- * @mixes module:utils/observablemixin~ObservaleMixin
- */
-export default class Command {
-	/**
-	 * Creates a new Command instance.
-	 *
-	 * @param {module:core/editor/editor~Editor} editor Editor on which this command will be used.
-	 */
-	constructor( editor ) {
-		/**
-		 * Editor on which this command will be used.
-		 *
-		 * @readonly
-		 * @member {module:core/editor/editor~Editor} #editor
-		 */
-		this.editor = editor;
-
-		/**
-		 * Flag indicating whether a command is enabled or disabled.
-		 * A disabled command should do nothing upon it's execution.
-		 *
-		 * @observable
-		 * @member {Boolean} #isEnabled
-		 */
-		this.set( 'isEnabled', true );
-
-		// If schema checking function is specified, add it to the `refreshState` listeners.
-		// Feature will be disabled if it does not apply to schema requirements.
-		if ( this._checkEnabled ) {
-			this.on( 'refreshState', ( evt, data ) => {
-				data.isEnabled = this._checkEnabled();
-			} );
-		}
-	}
-
-	destroy() {
-		this.stopListening();
-	}
-
-	/**
-	 * Fires `refreshState` event and checks it's resolve value to decide whether command should be enabled or not.
-	 * Other parts of code might listen to `refreshState` event on this command and add their callbacks. This
-	 * way the responsibility of deciding whether a command should be enabled is shared.
-	 *
-	 * @fires refreshState
-	 */
-	refreshState() {
-		const data = { isEnabled: true };
-		this.fire( 'refreshState', data );
-
-		this.isEnabled = data.isEnabled;
-	}
-
-	/**
-	 * Executes the command if it is enabled.
-	 *
-	 * @protected
-	 * @param {*} param Parameter passed to {@link module:core/editor/editor~Editor#execute execute} method of this command.
-	 */
-	_execute( param ) {
-		if ( this.isEnabled ) {
-			this._doExecute( param );
-		}
-	}
-
-	/**
-	 * Disables the command. This should be used only by the command itself. Other parts of code should add
-	 * listeners to `refreshState` event.
-	 *
-	 * @protected
-	 */
-	_disable() {
-		this.on( 'refreshState', disableCallback );
-		this.refreshState();
-	}
-
-	/**
-	 * Enables the command (internally). This should be used only by the command itself. Command will be enabled if
-	 * other listeners does not return false on `refreshState` event callbacks. Firing {@link #_enable}
-	 * does not guarantee that {@link #isEnabled} will be set to true, as it depends on other listeners.
-	 *
-	 * @protected
-	 */
-	_enable() {
-		this.off( 'refreshState', disableCallback );
-		this.refreshState();
-	}
-
-	/**
-	 * Executes command.
-	 * This is an abstract method that should be overwritten in child classes.
-	 *
-	 * @protected
-	 */
-	_doExecute() {}
-
-	/**
-	 * Checks if a command should be enabled according to its own rules. Mostly it will check schema to see if the command
-	 * is allowed to be executed in given position. This method can be defined in child class (but is not obligatory).
-	 * If it is defined, it will be added as a callback to `refreshState` event.
-	 *
-	 * @protected
-	 * @method #_checkEnabled
-	 * @returns {Boolean} `true` if command should be enabled according to
-	 * {@link module:engine/model/document~Document#schema}. `false` otherwise.
-	 */
-}
-
-function disableCallback( evt, data ) {
-	data.isEnabled = false;
-}
-
-mix( Command, ObservableMixin );
-
-/**
- * Fired whenever command has to have its {@link #isEnabled} property refreshed. Every feature,
- * command or other class which needs to disable command (set `isEnabled` to `false`) should listen to this
- * event.
- *
- * @event refreshState
- * @param {Object} data
- * @param {Boolean} [data.isEnabled=true]
- */

+ 19 - 32
packages/ckeditor5-core/src/command/toggleattributecommand.js

@@ -7,20 +7,21 @@
  * @module core/command/toggleattributecommand
  */
 
-import Command from './command';
+import Command from '../command';
 import getSchemaValidRanges from './helpers/getschemavalidranges';
 import isAttributeAllowedInSelection from './helpers/isattributeallowedinselection';
 
 /**
- * An extension of the base {@link module:core/command/command~Command} class, which provides utilities for a command which toggles a single
+ * An extension of the base {@link module:core/command~Command} class, which provides utilities for a command which toggles a single
  * attribute on a text or an element. `ToggleAttributeCommand` uses {@link module:engine/model/document~Document#selection}
  * to decide which nodes (if any) should be changed, and applies or removes attributes from them.
  *
  * The command checks {@link module:engine/model/document~Document#schema} to decide if it should be enabled.
+ *
+ * @extends module:core/command~Command
  */
 export default class ToggleAttributeCommand extends Command {
 	/**
-	 * @see module:core/command/command~Command
 	 * @param {module:core/editor/editor~Editor} editor
 	 * @param {String} attributeKey Attribute that will be set by the command.
 	 */
@@ -30,43 +31,29 @@ export default class ToggleAttributeCommand extends Command {
 		/**
 		 * Attribute that will be set by the command.
 		 *
+		 * @readonly
 		 * @member {String}
 		 */
 		this.attributeKey = attributeKey;
 
 		/**
-		 * Flag indicating whether command is active. For collapsed selection it means that typed characters will have
+		 * Flag indicating whether the command is active. For collapsed selection it means that typed characters will have
 		 * the command's attribute set. For range selection it means that all nodes inside have the attribute applied.
 		 *
 		 * @observable
+		 * @readonly
 		 * @member {Boolean} #value
 		 */
-		this.set( 'value', false );
-
-		this.listenTo( editor.document, 'changesDone', () => {
-			this.refreshValue();
-			this.refreshState();
-		} );
-	}
-
-	/**
-	 * Updates command's {@link #value value} based on the current selection.
-	 */
-	refreshValue() {
-		this.value = this.editor.document.selection.hasAttribute( this.attributeKey );
 	}
 
 	/**
-	 * Checks if {@link module:engine/model/document~Document#schema} allows to create attribute in
-	 * {@link module:engine/model/document~Document#selection}.
-	 *
-	 * @private
-	 * @returns {Boolean}
+	 * Updates command's {@link #value} based on the current selection.
 	 */
-	_checkEnabled() {
-		const document = this.editor.document;
+	refresh() {
+		const doc = this.editor.document;
 
-		return isAttributeAllowedInSelection( this.attributeKey, document.selection, document.schema );
+		this.value = doc.selection.hasAttribute( this.attributeKey );
+		this.isEnabled = isAttributeAllowedInSelection( this.attributeKey, doc.selection, doc.schema );
 	}
 
 	/**
@@ -84,19 +71,19 @@ export default class ToggleAttributeCommand extends Command {
 	 *
 	 * If the command is disabled (`isEnabled == false`) when it is executed, nothing will happen.
 	 *
-	 * @private
+	 * @fires execute
 	 * @param {Object} [options] Options of command.
 	 * @param {Boolean} [options.forceValue] If set it will force command behavior. If `true`, command will apply attribute,
 	 * otherwise command will remove attribute. If not set, command will look for it's current value to decide what it should do.
 	 * @param {module:engine/model/batch~Batch} [options.batch] Batch to group undo steps.
 	 */
-	_doExecute( options = {} ) {
-		const document = this.editor.document;
-		const selection = document.selection;
+	execute( options = {} ) {
+		const doc = this.editor.document;
+		const selection = doc.selection;
 		const value = ( options.forceValue === undefined ) ? !this.value : options.forceValue;
 
 		// If selection has non-collapsed ranges, we change attribute on nodes inside those ranges.
-		document.enqueueChanges( () => {
+		doc.enqueueChanges( () => {
 			if ( selection.isCollapsed ) {
 				if ( value ) {
 					selection.setAttribute( this.attributeKey, true );
@@ -104,10 +91,10 @@ export default class ToggleAttributeCommand extends Command {
 					selection.removeAttribute( this.attributeKey );
 				}
 			} else {
-				const ranges = getSchemaValidRanges( this.attributeKey, selection.getRanges(), document.schema );
+				const ranges = getSchemaValidRanges( this.attributeKey, selection.getRanges(), doc.schema );
 
 				// Keep it as one undo step.
-				const batch = options.batch || document.batch();
+				const batch = options.batch || doc.batch();
 
 				for ( const range of ranges ) {
 					if ( value ) {

+ 103 - 0
packages/ckeditor5-core/src/commandcollection.js

@@ -0,0 +1,103 @@
+/**
+ * @license Copyright (c) 2003-2017, CKSource - Frederico Knabben. All rights reserved.
+ * For licensing, see LICENSE.md.
+ */
+
+/**
+ * @module core/commandcollection
+ */
+
+import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror';
+
+/**
+ * Collection of commands. Its instance is available in {@module:core/editor/editor~Editor#commands}.
+ */
+export default class CommandCollection {
+	/**
+	 * Creates collection instance.
+	 */
+	constructor() {
+		/**
+		 * Command map.
+		 *
+		 * @private
+		 * @member {Map}
+		 */
+		this._commands = new Map();
+	}
+
+	/**
+	 * Registers a new command.
+	 *
+	 * @param {String} commandName The name of the command.
+	 * @param {module:core/command~Command} command
+	 */
+	add( commandName, command ) {
+		this._commands.set( commandName, command );
+	}
+
+	/**
+	 * Retrieves a command from the collection.
+	 *
+	 * @param {String} commandName The name of the command.
+	 * @returns {module:core/command~Command}
+	 */
+	get( commandName ) {
+		return this._commands.get( commandName );
+	}
+
+	/**
+	 * Executes a command.
+	 *
+	 * @param {String} commandName The name of the command.
+	 */
+	execute( commandName, ...args ) {
+		const command = this.get( commandName );
+
+		if ( !command ) {
+			/**
+			 * Command does not exist.
+			 *
+			 * @error commandcollection-command-not-found
+			 * @param {String} commandName Name of the command.
+			 */
+			throw new CKEditorError( 'commandcollection-command-not-found: Command does not exist.', { commandName } );
+		}
+
+		command.execute( ...args );
+	}
+
+	/**
+	 * Returns iterator of command names.
+	 *
+	 * @returns {Iterator.<String>}
+	 */
+	* names() {
+		yield* this._commands.keys();
+	}
+
+	/**
+	 * Returns iterator of command instances.
+	 *
+	 * @returns {Iterator.<module:core/command~Command>}
+	 */
+	* commands() {
+		yield* this._commands.values();
+	}
+
+	/**
+	 * Collection iterator.
+	 */
+	[ Symbol.iterator ]() {
+		return this._commands[ Symbol.iterator ]();
+	}
+
+	/**
+	 * Destroys all collection commands.
+	 */
+	destroy() {
+		for ( const command of this.commands() ) {
+			command.destroy();
+		}
+	}
+}

+ 16 - 20
packages/ckeditor5-core/src/editor/editor.js

@@ -7,14 +7,14 @@
  * @module core/editor/editor
  */
 
-import EmitterMixin from '@ckeditor/ckeditor5-utils/src/emittermixin';
 import Config from '@ckeditor/ckeditor5-utils/src/config';
 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 Document from '@ckeditor/ckeditor5-engine/src/model/document';
 
-import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror';
+import EmitterMixin from '@ckeditor/ckeditor5-utils/src/emittermixin';
 import mix from '@ckeditor/ckeditor5-utils/src/mix';
 
 /**
@@ -55,9 +55,9 @@ export default class Editor {
 		 * Commands registered to the editor.
 		 *
 		 * @readonly
-		 * @member {Map.<module:core/command/command~Command>}
+		 * @member {module:core/command/commandcollection~CommandCollection}
 		 */
-		this.commands = new Map();
+		this.commands = new CommandCollection();
 
 		/**
 		 * @readonly
@@ -140,13 +140,16 @@ export default class Editor {
 	/**
 	 * Destroys the editor instance, releasing all resources used by it.
 	 *
-	 * @fires module:core/editor/editor~Editor#destroy
+	 * @fires destroy
 	 * @returns {Promise} A promise that resolves once the editor instance is fully destroyed.
 	 */
 	destroy() {
 		this.fire( 'destroy' );
+
 		this.stopListening();
 
+		this.commands.destroy();
+
 		return Promise.resolve()
 			.then( () => {
 				this.document.destroy();
@@ -155,24 +158,17 @@ export default class Editor {
 	}
 
 	/**
-	 * Executes specified command with given parameter.
+	 * Executes specified command with given parameters.
+	 *
+	 * Shorthand for:
+	 *
+	 *		editor.commands.get( commandName ).execute( ... );
 	 *
 	 * @param {String} commandName Name of command to execute.
-	 * @param {*} [commandParam] If set, command will be executed with this parameter.
+	 * @param {*} [...commandParams] Command parameters.
 	 */
-	execute( commandName, commandParam ) {
-		const command = this.commands.get( commandName );
-
-		if ( !command ) {
-			/**
-			 * Specified command has not been added to the editor.
-			 *
-			 * @error editor-command-not-found
-			 */
-			throw new CKEditorError( 'editor-command-not-found: Specified command has not been added to the editor.' );
-		}
-
-		command._execute( commandParam );
+	execute( ...args ) {
+		this.commands.execute( ...args );
 	}
 
 	/**

+ 137 - 0
packages/ckeditor5-core/tests/command.js

@@ -0,0 +1,137 @@
+/**
+ * @license Copyright (c) 2003-2017, CKSource - Frederico Knabben. All rights reserved.
+ * For licensing, see LICENSE.md.
+ */
+
+import Command from '../src/command';
+import ModelTestEditor from './_utils/modeltesteditor';
+
+describe( 'Command', () => {
+	let editor, command;
+
+	beforeEach( () => {
+		return ModelTestEditor
+			.create()
+			.then( newEditor => {
+				editor = newEditor;
+				command = new Command( editor );
+			} );
+	} );
+
+	afterEach( () => {
+		command.destroy();
+
+		return editor.destroy();
+	} );
+
+	describe( 'constructor()', () => {
+		it( 'sets the editor property', () => {
+			expect( command.editor ).to.equal( editor );
+		} );
+
+		it( 'sets the state properties', () => {
+			expect( command.value ).to.be.undefined;
+			expect( command.isEnabled ).to.be.false;
+		} );
+
+		it( 'adds a listener which refreshed the command on editor.document#changesDone', () => {
+			sinon.spy( command, 'refresh' );
+
+			editor.document.fire( 'changesDone' );
+
+			expect( command.refresh.calledOnce ).to.be.true;
+		} );
+	} );
+
+	describe( 'value', () => {
+		it( 'fires change event', () => {
+			const spy = sinon.spy();
+
+			command.on( 'change:value', spy );
+
+			command.value = 1;
+
+			expect( spy.calledOnce ).to.be.true;
+		} );
+	} );
+
+	describe( 'isEnabled', () => {
+		it( 'fires change event', () => {
+			const spy = sinon.spy();
+
+			command.on( 'change:isEnabled', spy );
+
+			command.isEnabled = true;
+
+			expect( spy.calledOnce ).to.be.true;
+		} );
+	} );
+
+	describe( 'execute()', () => {
+		it( 'is decorated', () => {
+			const spy = sinon.spy();
+
+			command.on( 'execute', spy );
+
+			command.isEnabled = true;
+
+			command.execute( 1, 2 );
+
+			expect( spy.calledOnce ).to.be.true;
+			expect( spy.args[ 0 ][ 1 ] ).to.deep.equal( [ 1, 2 ] );
+		} );
+
+		it( 'is automatically blocked (with low priority listener) if command is disabled', () => {
+			const spyExecute = sinon.spy();
+			const spyHighest = sinon.spy();
+			const spyHigh = sinon.spy();
+
+			class SpyCommand extends Command {
+				execute() {
+					spyExecute();
+				}
+			}
+
+			const command = new SpyCommand( editor );
+
+			command.on( 'execute', spyHighest, { priority: 'highest' } );
+			command.on( 'execute', spyHigh, { priority: 'high' } );
+
+			command.execute();
+
+			expect( spyExecute.called ).to.be.false;
+			expect( spyHighest.calledOnce ).to.be.true;
+			expect( spyHigh.called ).to.be.false;
+		} );
+	} );
+
+	describe( 'refresh()', () => {
+		it( 'sets isEnabled to true', () => {
+			command.refresh();
+
+			expect( command.isEnabled ).to.be.true;
+		} );
+
+		// This is an acceptance test for the ability to override a command's state from outside
+		// in a way that at any moment the action can be reverted by just offing the listener and
+		// refreshing the command once again.
+		it( 'is safely overridable using change:isEnabled', () => {
+			command.on( 'change:isEnabled', callback, { priority: 'high' } );
+			command.isEnabled = false;
+			command.refresh();
+
+			expect( command.isEnabled ).to.be.false;
+
+			command.off( 'change:isEnabled', callback );
+			command.refresh();
+
+			expect( command.isEnabled ).to.be.true;
+
+			function callback( evt ) {
+				command.isEnabled = false;
+
+				evt.stop();
+			}
+		} );
+	} );
+} );

+ 0 - 162
packages/ckeditor5-core/tests/command/command.js

@@ -1,162 +0,0 @@
-/**
- * @license Copyright (c) 2003-2017, CKSource - Frederico Knabben. All rights reserved.
- * For licensing, see LICENSE.md.
- */
-
-import Editor from '../../src/editor/editor';
-import Command from '../../src/command/command';
-
-describe( 'Command', () => {
-	let editor, command;
-
-	class CommandWithSchema extends Command {
-		constructor( editor, schemaValid ) {
-			super( editor );
-
-			this.schemaValid = schemaValid;
-		}
-
-		_checkEnabled() {
-			return this.schemaValid;
-		}
-	}
-
-	beforeEach( () => {
-		editor = new Editor();
-		command = new Command( editor );
-	} );
-
-	afterEach( () => {
-		// Might be redundant if editor destroys the commands.
-		command.destroy();
-		editor.destroy();
-	} );
-
-	describe( 'constructor()', () => {
-		it( 'should create a new command instance, that is enabled and bound to given editor', () => {
-			expect( command ).to.have.property( 'editor' ).equal( editor );
-			expect( command.isEnabled ).to.be.true;
-		} );
-
-		it( 'Command should have _doExecute method', () => {
-			expect( () => {
-				command._doExecute();
-			} ).not.to.throw;
-		} );
-
-		it( 'should add listener to its refreshState event if checkSchema method is present', () => {
-			expect( command._checkEnabled ).to.be.undefined;
-
-			command._checkEnabled = sinon.spy();
-			command.refreshState();
-
-			expect( command._checkEnabled.called ).to.be.false;
-
-			const newCommand = new CommandWithSchema( editor, true );
-			sinon.spy( newCommand, '_checkEnabled' );
-
-			newCommand.refreshState();
-
-			expect( newCommand._checkEnabled.calledOnce ).to.be.true;
-		} );
-	} );
-
-	describe( 'destroy', () => {
-		it( 'should stop listening', () => {
-			sinon.spy( command, 'stopListening' );
-
-			command.destroy();
-
-			expect( command.stopListening.calledOnce ).to.be.true;
-		} );
-	} );
-
-	describe( 'refreshState', () => {
-		it( 'should fire refreshState event', () => {
-			const spy = sinon.spy();
-
-			command.on( 'refreshState', spy );
-			command.refreshState();
-
-			expect( spy.called ).to.be.true;
-		} );
-
-		it( 'should set isEnabled property to the value passed by object-reference', () => {
-			command.on( 'refreshState', ( evt, data ) => {
-				data.isEnabled = true;
-			} );
-
-			expect( command.isEnabled ).to.be.true;
-		} );
-
-		it( 'should set isEnabled to false if _checkEnabled returns false', () => {
-			const disabledCommand = new CommandWithSchema( editor, false );
-
-			disabledCommand.refreshState();
-
-			expect( disabledCommand.isEnabled ).to.be.false;
-		} );
-	} );
-
-	describe( 'disable', () => {
-		it( 'should make command disabled', () => {
-			command._disable();
-
-			expect( command.isEnabled ).to.be.false;
-		} );
-
-		it( 'should not make command disabled if there is a high-priority listener forcing command to be enabled', () => {
-			command.on( 'refreshState', evt => {
-				evt.stop();
-
-				return true;
-			}, command, 1 );
-
-			command._disable();
-
-			expect( command.isEnabled ).to.be.true;
-		} );
-	} );
-
-	describe( 'enable', () => {
-		it( 'should make command enabled if it was previously disabled by disable()', () => {
-			command._disable();
-			command._enable();
-
-			expect( command.isEnabled ).to.be.true;
-		} );
-
-		it( 'should not make command enabled if there are other listeners disabling it', () => {
-			command._disable();
-
-			command.on( 'refreshState', ( evt, data ) => {
-				data.isEnabled = false;
-			} );
-
-			command.refreshState();
-			command._enable();
-
-			expect( command.isEnabled ).to.be.false;
-		} );
-	} );
-
-	describe( '_execute', () => {
-		it( 'should not execute command if it is disabled', () => {
-			command._disable();
-
-			sinon.spy( command, '_doExecute' );
-
-			command._execute();
-
-			expect( command._doExecute.called ).to.be.false;
-		} );
-
-		it( 'should execute command if it is enabled', () => {
-			sinon.spy( command, '_doExecute' );
-
-			command._execute();
-
-			expect( command._doExecute.called ).to.be.true;
-		} );
-	} );
-} );

+ 58 - 66
packages/ckeditor5-core/tests/command/toggleattributecommand.js

@@ -44,21 +44,15 @@ describe( 'ToggleAttributeCommand', () => {
 	} );
 
 	describe( 'value', () => {
-		// https://github.com/ckeditor/ckeditor5-core/issues/50
-		it( 'should be updated on document#changesDone', () => {
-			const spy = sinon.spy( command, 'refreshValue' );
-
-			modelDoc.fire( 'changesDone' );
-			sinon.assert.calledOnce( spy );
-		} );
-
-		it( 'should be set to true or false basing on selection attribute', () => {
+		it( 'is true when selection has the attribute', () => {
 			modelDoc.enqueueChanges( () => {
 				modelDoc.selection.setAttribute( attrKey, true );
 			} );
 
 			expect( command.value ).to.be.true;
+		} );
 
+		it( 'is false when selection does not have the attribute', () => {
 			modelDoc.enqueueChanges( () => {
 				modelDoc.selection.removeAttribute( attrKey );
 			} );
@@ -67,23 +61,57 @@ describe( 'ToggleAttributeCommand', () => {
 		} );
 	} );
 
-	describe( 'state', () => {
-		// https://github.com/ckeditor/ckeditor5-core/issues/50
-		it( 'should be updated on document#changesDone', () => {
-			const spy = sinon.spy( command, 'refreshState' );
+	describe( 'isEnabled', () => {
+		// This test doesn't tests every possible case.
+		// Method `refresh()` uses `isAttributeAllowedInSelection` helper which is fully tested in his own test.
+
+		beforeEach( () => {
+			modelDoc.schema.registerItem( 'x', '$block' );
+			modelDoc.schema.disallow( { name: '$text', inside: 'x', attributes: 'link' } );
+		} );
+
+		describe( 'when selection is collapsed', () => {
+			it( 'should return true if characters with the attribute can be placed at caret position', () => {
+				setData( modelDoc, '<p>f[]oo</p>' );
+				expect( command.isEnabled ).to.be.true;
+			} );
+
+			it( 'should return false if characters with the attribute cannot be placed at caret position', () => {
+				setData( modelDoc, '<x>fo[]o</x>' );
+				expect( command.isEnabled ).to.be.false;
+			} );
+		} );
+
+		describe( 'when selection is not collapsed', () => {
+			it( 'should return true if there is at least one node in selection that can have the attribute', () => {
+				setData( modelDoc, '<p>[foo]</p>' );
+				expect( command.isEnabled ).to.be.true;
+			} );
 
-			modelDoc.fire( 'changesDone' );
-			sinon.assert.calledOnce( spy );
+			it( 'should return false if there are no nodes in selection that can have the attribute', () => {
+				setData( modelDoc, '<x>[foo]</x>' );
+				expect( command.isEnabled ).to.be.false;
+			} );
 		} );
 	} );
 
-	describe( '_doExecute', () => {
+	describe( 'execute()', () => {
+		it( 'should do nothing if the command is disabled', () => {
+			setData( modelDoc, '<p>fo[ob]ar</p>' );
+
+			command.isEnabled = false;
+
+			command.execute();
+
+			expect( getData( modelDoc ) ).to.equal( '<p>fo[ob]ar</p>' );
+		} );
+
 		it( 'should add attribute on selected nodes if the command value was false', () => {
 			setData( modelDoc, '<p>a[bc<$text bold="true">fo]obar</$text>xyz</p>' );
 
 			expect( command.value ).to.be.false;
 
-			command._doExecute();
+			command.execute();
 
 			expect( command.value ).to.be.true;
 			expect( getData( modelDoc ) ).to.equal( '<p>a[<$text bold="true">bcfo]obar</$text>xyz</p>' );
@@ -94,7 +122,7 @@ describe( 'ToggleAttributeCommand', () => {
 
 			expect( command.value ).to.be.true;
 
-			command._doExecute();
+			command.execute();
 
 			expect( getData( modelDoc ) ).to.equal( '<p>abc[foo]<$text bold="true">bar</$text>xyz</p>' );
 			expect( command.value ).to.be.false;
@@ -105,7 +133,7 @@ describe( 'ToggleAttributeCommand', () => {
 
 			expect( command.value ).to.be.true;
 
-			command._doExecute( { forceValue: true } );
+			command.execute( { forceValue: true } );
 
 			expect( command.value ).to.be.true;
 			expect( getData( modelDoc ) ).to.equal( '<p>abc<$text bold="true">foob[arx</$text>]yz</p>' );
@@ -114,7 +142,7 @@ describe( 'ToggleAttributeCommand', () => {
 		it( 'should remove attribute on selected nodes if execute parameter was set to false', () => {
 			setData( modelDoc, '<p>a[bc<$text bold="true">fo]obar</$text>xyz</p>' );
 
-			command._doExecute( { forceValue: false } );
+			command.execute( { forceValue: false } );
 
 			expect( command.value ).to.be.false;
 			expect( getData( modelDoc ) ).to.equal( '<p>a[bcfo]<$text bold="true">obar</$text>xyz</p>' );
@@ -125,12 +153,12 @@ describe( 'ToggleAttributeCommand', () => {
 
 			expect( command.value ).to.be.false;
 
-			command._doExecute();
+			command.execute();
 
 			expect( command.value ).to.be.true;
 			expect( modelDoc.selection.hasAttribute( 'bold' ) ).to.be.true;
 
-			command._doExecute();
+			command.execute();
 
 			expect( command.value ).to.be.false;
 			expect( modelDoc.selection.hasAttribute( 'bold' ) ).to.be.false;
@@ -139,7 +167,7 @@ describe( 'ToggleAttributeCommand', () => {
 		it( 'should not store attribute change on selection if selection is collapsed in non-empty parent', () => {
 			setData( modelDoc, '<p>a[]bc<$text bold="true">foobar</$text>xyz</p>' );
 
-			command._doExecute();
+			command.execute();
 
 			// It should not save that bold was executed at position ( root, [ 0, 1 ] ).
 
@@ -159,7 +187,7 @@ describe( 'ToggleAttributeCommand', () => {
 
 			expect( command.value ).to.be.false;
 
-			command._doExecute();
+			command.execute();
 
 			expect( command.value ).to.be.true;
 			expect( modelDoc.selection.hasAttribute( 'bold' ) ).to.be.true;
@@ -180,7 +208,7 @@ describe( 'ToggleAttributeCommand', () => {
 			// Attribute should be restored.
 			expect( command.value ).to.be.true;
 
-			command._doExecute();
+			command.execute();
 
 			expect( command.value ).to.be.false;
 			expect( modelDoc.selection.hasAttribute( 'bold' ) ).to.be.false;
@@ -192,7 +220,7 @@ describe( 'ToggleAttributeCommand', () => {
 
 			expect( command.isEnabled ).to.be.true;
 
-			command._doExecute();
+			command.execute();
 
 			expect( getData( modelDoc ) )
 				.to.equal( '<p>ab[<$text bold="true">c</$text><img></img><$text bold="true">foobarxy</$text><img></img>]z</p>' );
@@ -204,7 +232,7 @@ describe( 'ToggleAttributeCommand', () => {
 
 			expect( batch.deltas.length ).to.equal( 0 );
 
-			command._doExecute( { batch } );
+			command.execute( { batch } );
 
 			expect( batch.deltas.length ).to.equal( 1 );
 			expect( getData( modelDoc ) ).to.equal( '<p>a[<$text bold="true">bcfo]obar</$text>xyz</p>' );
@@ -222,7 +250,7 @@ describe( 'ToggleAttributeCommand', () => {
 
 				modelDoc.on( 'changesDone', spy );
 
-				command._doExecute();
+				command.execute();
 
 				expect( spy.calledOnce ).to.be.true;
 			} );
@@ -232,7 +260,7 @@ describe( 'ToggleAttributeCommand', () => {
 
 				modelDoc.on( 'changesDone', spy );
 
-				command._doExecute();
+				command.execute();
 
 				expect( spy.calledOnce ).to.be.true;
 			} );
@@ -242,46 +270,10 @@ describe( 'ToggleAttributeCommand', () => {
 
 				modelDoc.on( 'changesDone', spy );
 
-				command._doExecute();
+				command.execute();
 
 				expect( spy.calledOnce ).to.be.true;
 			} );
 		} );
 	} );
-
-	describe( '_checkEnabled', () => {
-		describe( '_checkEnabled', () => {
-			// This test doesn't tests every possible case.
-			// Method `_checkEnabled` uses `isAttributeAllowedInSelection` helper which is fully tested in his own test.
-
-			beforeEach( () => {
-				modelDoc.schema.registerItem( 'x', '$block' );
-				modelDoc.schema.disallow( { name: '$text', inside: 'x', attributes: 'link' } );
-			} );
-
-			describe( 'when selection is collapsed', () => {
-				it( 'should return true if characters with the attribute can be placed at caret position', () => {
-					setData( modelDoc, '<p>f[]oo</p>' );
-					expect( command._checkEnabled() ).to.be.true;
-				} );
-
-				it( 'should return false if characters with the attribute cannot be placed at caret position', () => {
-					setData( modelDoc, '<x>fo[]o</x>' );
-					expect( command._checkEnabled() ).to.be.false;
-				} );
-			} );
-
-			describe( 'when selection is not collapsed', () => {
-				it( 'should return true if there is at least one node in selection that can have the attribute', () => {
-					setData( modelDoc, '<p>[foo]</p>' );
-					expect( command._checkEnabled() ).to.be.true;
-				} );
-
-				it( 'should return false if there are no nodes in selection that can have the attribute', () => {
-					setData( modelDoc, '<x>[foo]</x>' );
-					expect( command._checkEnabled() ).to.be.false;
-				} );
-			} );
-		} );
-	} );
 } );

+ 138 - 0
packages/ckeditor5-core/tests/commandcollection.js

@@ -0,0 +1,138 @@
+/**
+ * @license Copyright (c) 2003-2017, CKSource - Frederico Knabben. All rights reserved.
+ * For licensing, see LICENSE.md.
+ */
+
+import CommandCollection from '../src/commandcollection';
+import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror';
+import Command from '../src/command';
+import ModelTestEditor from './_utils/modeltesteditor';
+
+class SomeCommand extends Command {
+	execute() {}
+}
+
+describe( 'CommandCollection', () => {
+	let collection, editor;
+
+	beforeEach( () => {
+		collection = new CommandCollection();
+
+		return ModelTestEditor
+			.create()
+			.then( newEditor => {
+				editor = newEditor;
+			} );
+	} );
+
+	afterEach( () => {
+		collection.destroy();
+
+		return editor.destroy();
+	} );
+
+	describe( 'add() and get()', () => {
+		it( 'adds and retrieves a command', () => {
+			const command = new SomeCommand( editor );
+
+			collection.add( 'foo', command );
+
+			expect( collection.get( 'foo' ) ).to.equal( command );
+		} );
+	} );
+
+	describe( 'execute()', () => {
+		it( 'executes given method with given attributes', () => {
+			const command = new SomeCommand( editor );
+
+			sinon.spy( command, 'execute' );
+
+			collection.add( 'foo', command );
+
+			collection.execute( 'foo', 1, 2 );
+
+			expect( command.execute.calledOnce ).to.be.true;
+			expect( command.execute.args[ 0 ] ).to.deep.equal( [ 1, 2 ] );
+		} );
+
+		it( 'throws an error if command does not exist', () => {
+			expect( () => {
+				collection.execute( 'foo' );
+			} ).to.throw( CKEditorError, /^commandcollection-command-not-found:/ );
+		} );
+	} );
+
+	describe( 'names()', () => {
+		it( 'returns iterator', () => {
+			const names = collection.names();
+
+			expect( names.next ).to.be.a.function;
+		} );
+
+		it( 'returns iterator of command names', () => {
+			collection.add( 'foo', new SomeCommand( editor ) );
+			collection.add( 'bar', new SomeCommand( editor ) );
+
+			expect( Array.from( collection.names() ) ).to.have.members( [ 'foo', 'bar' ] );
+		} );
+	} );
+
+	describe( 'commands()', () => {
+		it( 'returns iterator', () => {
+			const commands = collection.commands();
+
+			expect( commands.next ).to.be.a.function;
+		} );
+
+		it( 'returns iterator of commands', () => {
+			const c1 = new SomeCommand( editor );
+			const c2 = new SomeCommand( editor );
+
+			collection.add( 'foo', c1 );
+			collection.add( 'bar', c2 );
+
+			const commandArray = Array.from( collection.commands() );
+
+			expect( commandArray ).to.have.length( 2 );
+			expect( commandArray ).to.have.members( [ c1, c2 ] );
+		} );
+	} );
+
+	describe( 'iterator', () => {
+		it( 'exists', () => {
+			expect( collection ).to.have.property( Symbol.iterator );
+		} );
+
+		it( 'returns iterator of [ name, command ]', () => {
+			const c1 = new SomeCommand( editor );
+			const c2 = new SomeCommand( editor );
+
+			collection.add( 'foo', c1 );
+			collection.add( 'bar', c2 );
+
+			const collectionArray = Array.from( collection );
+
+			expect( collectionArray ).to.have.length( 2 );
+			expect( collectionArray.map( pair => pair[ 0 ] ) ).to.have.members( [ 'foo', 'bar' ] );
+			expect( collectionArray.map( pair => pair[ 1 ] ) ).to.have.members( [ c1, c2 ] );
+		} );
+	} );
+
+	describe( 'commands()', () => {
+		it( 'returns iterator of commands', () => {
+			const c1 = new SomeCommand( editor );
+			const c2 = new SomeCommand( editor );
+
+			sinon.spy( c1, 'destroy' );
+			sinon.spy( c2, 'destroy' );
+
+			collection.add( 'foo', c1 );
+			collection.add( 'bar', c2 );
+
+			collection.destroy();
+
+			expect( c1.destroy.calledOnce ).to.be.true;
+			expect( c2.destroy.calledOnce ).to.be.true;
+		} );
+	} );
+} );

+ 0 - 74
packages/ckeditor5-core/tests/editor/editor-base.js

@@ -1,74 +0,0 @@
-/**
- * @license Copyright (c) 2003-2017, CKSource - Frederico Knabben. All rights reserved.
- * For licensing, see LICENSE.md.
- */
-
-import Editor from '../../src/editor/editor';
-import Command from '../../src/command/command';
-import Locale from '@ckeditor/ckeditor5-utils/src/locale';
-import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror';
-
-describe( 'Editor', () => {
-	describe( 'locale', () => {
-		it( 'is instantiated and t() is exposed', () => {
-			const editor = new Editor();
-
-			expect( editor.locale ).to.be.instanceof( Locale );
-			expect( editor.t ).to.equal( editor.locale.t );
-		} );
-
-		it( 'is configured with the config.lang', () => {
-			const editor = new Editor( { lang: 'pl' } );
-
-			expect( editor.locale.lang ).to.equal( 'pl' );
-		} );
-	} );
-
-	describe( 'destroy', () => {
-		it( 'should fire "destroy"', () => {
-			const editor = new Editor();
-			const spy = sinon.spy();
-
-			editor.on( 'destroy', spy );
-
-			return editor.destroy().then( () => {
-				expect( spy.calledOnce ).to.be.true;
-			} );
-		} );
-
-		it( 'should destroy all components it initialized', () => {
-			const editor = new Editor();
-
-			const spy1 = sinon.spy( editor.data, 'destroy' );
-			const spy2 = sinon.spy( editor.document, 'destroy' );
-
-			return editor.destroy()
-				.then( () => {
-					expect( spy1.calledOnce ).to.be.true;
-					expect( spy2.calledOnce ).to.be.true;
-				} );
-		} );
-	} );
-
-	describe( 'execute', () => {
-		it( 'should execute specified command', () => {
-			const editor = new Editor();
-
-			const command = new Command( editor );
-			sinon.spy( command, '_execute' );
-
-			editor.commands.set( 'commandName', command );
-			editor.execute( 'commandName' );
-
-			expect( command._execute.calledOnce ).to.be.true;
-		} );
-
-		it( 'should throw an error if specified command has not been added', () => {
-			const editor = new Editor();
-
-			expect( () => {
-				editor.execute( 'command' );
-			} ).to.throw( CKEditorError, /^editor-command-not-found:/ );
-		} );
-	} );
-} );

+ 74 - 3
packages/ckeditor5-core/tests/editor/editor.js

@@ -9,6 +9,10 @@ import Editor from '../../src/editor/editor';
 import Plugin from '../../src/plugin';
 import Config from '@ckeditor/ckeditor5-utils/src/config';
 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';
 
 class PluginA extends Plugin {
 	constructor( editor ) {
@@ -98,7 +102,7 @@ describe( 'Editor', () => {
 			const editor = new Editor();
 
 			expect( editor.config ).to.be.an.instanceof( Config );
-			expect( editor.commands ).to.be.an.instanceof( Map );
+			expect( editor.commands ).to.be.an.instanceof( CommandCollection );
 
 			expect( editor.plugins ).to.be.an.instanceof( PluginCollection );
 			expect( getPlugins( editor ) ).to.be.empty;
@@ -139,7 +143,74 @@ describe( 'Editor', () => {
 		} );
 	} );
 
-	describe( 'create', () => {
+	describe( 'locale', () => {
+		it( 'is instantiated and t() is exposed', () => {
+			const editor = new Editor();
+
+			expect( editor.locale ).to.be.instanceof( Locale );
+			expect( editor.t ).to.equal( editor.locale.t );
+		} );
+
+		it( 'is configured with the config.lang', () => {
+			const editor = new Editor( { lang: 'pl' } );
+
+			expect( editor.locale.lang ).to.equal( 'pl' );
+		} );
+	} );
+
+	describe( 'destroy()', () => {
+		it( 'should fire "destroy"', () => {
+			const editor = new Editor();
+			const spy = sinon.spy();
+
+			editor.on( 'destroy', spy );
+
+			return editor.destroy().then( () => {
+				expect( spy.calledOnce ).to.be.true;
+			} );
+		} );
+
+		it( 'should destroy all components it initialized', () => {
+			const editor = new Editor();
+
+			const spy1 = sinon.spy( editor.data, 'destroy' );
+			const spy2 = sinon.spy( editor.document, 'destroy' );
+
+			return editor.destroy()
+				.then( () => {
+					expect( spy1.calledOnce ).to.be.true;
+					expect( spy2.calledOnce ).to.be.true;
+				} );
+		} );
+	} );
+
+	describe( 'execute()', () => {
+		it( 'should execute specified command', () => {
+			class SomeCommand extends Command {
+				execute() {}
+			}
+
+			const editor = new Editor();
+
+			const command = new SomeCommand( editor );
+			sinon.spy( command, 'execute' );
+
+			editor.commands.add( 'someCommand', command );
+			editor.execute( 'someCommand' );
+
+			expect( command.execute.calledOnce ).to.be.true;
+		} );
+
+		it( 'should throw an error if specified command has not been added', () => {
+			const editor = new Editor();
+
+			expect( () => {
+				editor.execute( 'command' );
+			} ).to.throw( CKEditorError, /^commandcollection-command-not-found:/ );
+		} );
+	} );
+
+	describe( 'create()', () => {
 		it( 'should return a promise that resolves properly', () => {
 			const promise = Editor.create();
 
@@ -179,7 +250,7 @@ describe( 'Editor', () => {
 		} );
 	} );
 
-	describe( 'initPlugins', () => {
+	describe( 'initPlugins()', () => {
 		it( 'should load plugins', () => {
 			const editor = new Editor( {
 				plugins: [ PluginA, PluginB ]