Improve a typical command class

[Reinmar]

This is a message quote command implementation:

import Command from '@ckeditor/ckeditor5-core/src/command/command';
import Position from '@ckeditor/ckeditor5-engine/src/model/position';

/**
 * The message quote command.
 *
 * @extends module:core/command/command~Command
 */
export default class MessageQuoteCommand extends Command {
	/**
	 * @inheritDoc
	 */
	constructor( editor ) {
		super( editor );

		/**
		 * Flag indicating whether command is active.
		 *
		 * @readonly
		 * @observable
		 * @member {Boolean} #value
		 */
		this.set( 'value', false );

		// Update current value each time changes are done on document.
		this.listenTo( editor.document, 'changesDone', () => {
			this.refreshValue();
			this.refreshState();
		} );
	}

	/**
	 * Updates command's {@link #value} based on the current selection.
	 */
	refreshValue() {
		const firstBlock = this.editor.document.selection.getSelectedBlocks().next().value;

		// In the current implementation, the message quote must be an immediate parent of a block element.
		this.value = firstBlock && ( firstBlock.parent.name == 'messageQuote' );
	}

	_doExecute( options = {} ) {

	}

	_checkEnabled() {
		if ( this.value ) {
			return true;
		}

		const selection = this.editor.document.selection;
		const schema = this.editor.document.schema;

		const firstBlock = selection.getSelectedBlocks().next().value;

		if ( !firstBlock ) {
			return false;
		}

		return schema.check( {
			name: 'messageQuote',
			inside: Position.createBefore( firstBlock )
		} );
	}
}

It's very similar to the list command and heading command could be aligned to it as well (now it differs a bit, which is an issue itself).

What I don't like here is:

• Needing to manually add a listener to changesDone. See also https://github.com/ckeditor/ckeditor5-engine/issues/698. I think that we could define something like Command#refreshOnChange() method which would register such a listener itself.
• The fact that the state check is in a private method, while the value check is in a public one. Thanks to that we have two public methods refreshState() and refreshValue(), but a class definition is ugly. I think that we should hoist the "value" concept to the base command and create Command#refreshValue() there. Then, the actual logic would be enclosed in _checkValue() and _checkEnabled(). Or, we can go a bit further and somehow join these two.
• The logic in _checkEnabled() will be really similar in all 3 commands so perhaps it can be moved to some util.

[scofalik]

I think it was made like this because not every command was supposed to have value property. If we want to have value in base class, we can just have _checkValue and _checkEnabled (or rename it to _checkState) methods that will take care of setting value and isEnabled properties.

We could even go a step further and set observable value property only if _checkValue is defined:

class Command {
  constructor() {
    // ...
    
    if ( this._checkValue ) {
      // ...
    }
  }
  
  // ...
}

class MyCommand extends Command {
  _checkValue() {
    // ...
  }
}

We have something like this with _checkEnabled method at the moment. We do some magic and set the listeners basing on the method existence.

I am not sure about joining those two, though. As you know, CKE4 has tri-state property for this and AFAIR it caused problems. We separated those two on purpose.

[Reinmar]

I am not sure about joining those two, though. As you know, CKE4 has tri-state property for this and AFAIR it caused problems. We separated those two on purpose.

I didn't mean merging them into one property of course. Just to merge the two methods for refreshing the command. But I'm not sure about this anyway.

[scofalik]

Well, I understand that value and state are set under similar circumstances (I mean after changes are made on document). But I wouldn't join them together at the moment. Having them separated makes some things easier:

• you don't have value property if it is not needed,
• _checkValue or _checkEnabled may just return value instead of setting something on command,
• implementation of both is simpler,
• there are less things to miss (for example forgetting to set .value or .isEnabled in common method).

[Reinmar]

Fixed by ckeditor/ckeditor5-core#88.