Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Command: afterExecute event (and possibly beforeExecute) #2857

Closed
scofalik opened this issue Sep 19, 2016 · 4 comments
Closed

Command: afterExecute event (and possibly beforeExecute) #2857

scofalik opened this issue Sep 19, 2016 · 4 comments
Labels
package:core type:improvement This issue reports a possible enhancement of an existing feature.

Comments

@scofalik
Copy link
Contributor

This issue is a followup to the discussion we recently had concerning how Commands could work and how can we extend them.

We came to the conculsion, that it would be useful, if command fired afterExecute event which could be listened to by other features that might want to modify results of command execution. This behavior already got implemented in EnterCommand in https://github.com/ckeditor/ckeditor5-enter/issues/27. However, we would like to see this automated.

Requirements:

  1. afterExecute should be fired with data object. Each command can define it's own contents of data object, but every command that modifies model and uses a Batch instance, should add it in data.batch. This is needed to add deltas to the same batch so changes done by command and done by custom callback are undone together.
  2. afterExecute should be fired in enqueueChanges block (if command uses it). This is also to ensure that changes done by command and by custom callback are indistinguishable from user point of view.
  3. This functionallity needs to be automated and guarantee 1. and 2. The proposal is to make a flag member in core.Command. If the flag is true, _doExecute is wrapped in enqueueChanges call (this is for 2.). Returned value of _doExecute is passed to afterExecute callback (this is for 1.).

Other things to keep in mind:

  1. We might want to introduce beforeExecute callback too. Right now there is no use case for such callback but we might want to prepare for it. The problem is that right now the Batch used by command is instantiated in _doExecute, so it would be unavailable for beforeExecute. We might use flag described in point 3. above to create a batch for command's _doExecute but pass it to beforeExecute callback before _doExecute is called.
  2. If beforeExecute is introduced, we may want to have a way to stop default command behavior. This could be done either by passing data object to beforeExecute and checking value of, let's say, data.preventDefault or by adding default command behavior as beforeExecute callback with lowest priority.
@Reinmar
Copy link
Member

Reinmar commented Sep 28, 2016

We'll need both events, and since we decided to make commands the central point of extension (see #340), those events are pretty important.

@Reinmar
Copy link
Member

Reinmar commented Sep 29, 2016

While discussing #340 I realised a bug in the current implementation. If the enter feature is loaded after the list feature (and this is possible, because list feature doesn't require the enter feature), then the enter command won't exist during list initialisation, so the specific behaviour won't be injected.

This means that we need to fire beforeExecute and afterExecuter on the command collection (which doesn't exist yet, so it must be implemented). This, most likely can be done like this (pseudocode):

class CommandCollection extends Collection {
    constructor() {
        this.on( 'add', ( commandName, item ) => {
            item
                .delegate( 'beforeExecute', 'afterExecute' )
                .to( this, ( evtName ) => `${ commandName }:${ evtName }` );
        } );
    }

Something like this should work, but a feature will need to be added to delegate() so it is possible to prefix the events (the callback is not supported yet).

@Reinmar
Copy link
Member

Reinmar commented Sep 29, 2016

I added this to iteration 4, cause this is kind of a bug.

@Reinmar
Copy link
Member

Reinmar commented Jun 13, 2017

In ckeditor/ckeditor5-core#88 we introduced the execute event which covers a big part of making command execution extensible.

However, it doesn't solve the EnterCommand case (https://github.com/ckeditor/ckeditor5-enter/issues/41). That's because the requirements which @scofalik described in his first post would require some really special features. I don't think that the command API should have such features because they would make it awfully complicated.

Therefore, I consider this issue partially solved and partially invalid. We still need to figure out what to do with the EnterCommand though.

@Reinmar Reinmar closed this as completed Jun 13, 2017
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-core Oct 9, 2019
@mlewand mlewand added resolution:solved type:improvement This issue reports a possible enhancement of an existing feature. package:core labels Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:core type:improvement This issue reports a possible enhancement of an existing feature.
Projects
None yet
Development

No branches or pull requests

3 participants