New event. CommandProcessEvent

[djxy]

I needed an event to handle the process of a command before and after that happen in the same tick. So I created this new event to handle that.

When some commands are fired by some players, I need to disable some functionalities. /help is one of them.

Currently there is only one event for when a command is send and it is SendCommandEvent. The commandResult isn't good.

https://github.com/SpongePowered/SpongeCommon/blob/bleeding/src/main/java/org/spongepowered/common/command/SpongeCommandManager.java#L258

SpongeCommon: SpongePowered/Sponge#888

[ryantheleach]

If these were not automatically generated by the tool, they probably should be to match the other events.

[djxy]

How can I generate them automatically?

[Deamon5550]

run ./gradlew genEventImpl licenseFormat in SpongeAPI

[djxy]

Removed SendCommandEvent and moved the setters of CommandProcessEvent to CommandProcessEvent.Pre

[ryantheleach]

The removal of setResult from the Pre event, means that plugins which provide command redirection / rewriting will be unable to return the result of the original processed command, when the event is cancelled.

Whether this is a good or bad thing however I can't decide.

You could argue that instead of cancelling, you should just rewrite the redirected command into the original and let it continue if you actually need the CommandResult.

But without setting a result, cancelling would have to make an assumption about the appropriate CommandResult to return, as

SpongeAPI/src/main/java/org/spongepowered/api/command/CommandManager.java

Line 155 in 11a6c07

CommandResult process(CommandSource source, String arguments);

does not return an Optional

[Meegooo]

What about CommandResult.empty()? Or maybe CommandResult.cancelled() (we'll have to create one)

[Meegooo]

So, posting here everything I have in mind

1. We already have SendCommandEvent. So we should just change that rather than creating a new event with almost the same functionality (apart from CommandProcessEvent.Post) (kinda solved already)

2. This is the question about when the event should be fired (I'm talking about CommandProcessEvent.Pre aka current SendCommandEvent)
   First possibility. Event is fired right after the user presses enter before any processing at all. At this point we only have a string that the server got from the client and the only thing we can do is edit that string. That is what currently is being done I believe, but it seems extremely low level. For example, we can't really tell what CommandExecutor the command will fire, who is the owner of the original command, what arguments are we supposed to have.

   Second possibility is firing the event right after Sponge parsed the command but before executor is fired. At this point in time we should have CommandMapping and CommandArgs available.
   But here we have a problem. If we change a command at this point, god knows what will happen. Unless Sponge implementation feels like parsing a command once again...
   Can be solved by introducing one more stage to the event (can't think of a name) and just not allow changing it there. We can still change the command with setCommand(CommandMapping)

   Also, while CommandArgs is a bit better than String, it's still essentially the same thing. CommandContext could be better, but we can't just get all arguments, we have to know the key. Also, we can't get to know what argument is supposed to represent (be it a player, a world or something else).
   Here I'm hoping that someone else who has better knowledge about implementation could correct me and suggest better solution. Right now my only suggestion is creating a new intermediate object (maybe mutable)

PS. What about renaming the event from CommandProcessEvent to ProcessCommandEvent (coincides with SendCommandEvent)?

(Github, will you stop formatting the comment for me 😦 )

[djxy]

You can't always get CommandArgs. Some plugin use CommandCallable and there is no CommandArgs created.

[Meegooo]

We can check if CommandCallable is instanceof CommandSpec and if it's not, do
new CommandArgs(args, Collections.singletonList(new SingleArg(args, 0,args.length()-1)));
And if it is, do
new CommandArgs(arguments, ((CommandSpec)spec).getInputTokenizer().tokenize(arguments, false));

But problem still remains - it's just a wrapper around String. And unmodifiable one too. So, do we really need it?

And after thinking some time, I can't think of any convenient way of getting what argument represents without rewriting a lot of code. Right now CommandElement doesn't store any info on what his parseValue method should return. And what if it doesn't return anything because it's just a container for other elements?

[Meegooo]

We can check if CommandCallable is instanceof CommandSpec and if it's not, do
new CommandArgs(args, Collections.singletonList(new SingleArg(args, 0,args.length()-1)));
And if it is, do
new CommandArgs(arguments, ((CommandSpec)spec).getInputTokenizer().tokenize(arguments, false));

And it just occurred to me that if we get CommandMapping from getCommand(), we could as easily parse the arguments ourselves.

So, after all that my only suggestions would be:

1. Replacing String with CommandMapping in getCommand() and setCommand()
2. Renaming the event from CommandProcessEvent to ProcessComandEvent
3. Maybe returning setResult(CommandResult) to Post

[Zidane]

@zml2008

Need a judgement call on this one.

[djxy]

I think this event could be nice to have. We will know when a command is sent by a player before and after its execution.

If you want to merge it, I will recreate a pull request because I deleted the repositories for SpongeAPI and SpongeCommon. I could do that tonight. It will contains the same code.

[zml2008]

Yeah, this functionality would be useful. Only issue I had was already mentioned by @ryantheleach. Once that's resolved this should be good to merge.

It should be relatively easy to recreate the changes -- GH keeps the commit even though its been deleted, so you can checkout the commit you already had from the SpongePowered/SpongeAPI repository.

[gabizou]

@djxy I can't clone your repository to look at merging this PR as it's marked as "private" or "unknown".

[djxy]

I deleted the repository when I cleaned my account. I can recreate it if you want.