Parameter injection for commands

[sk89q]

This adds support for parameter injection with commands.

I don't actually want to merge right away because I want to test a bit more, but I'd like some input (and any objections).

Old vs. new

@Command(
    aliases = { "/set" },
    usage = "<block>",
    desc = "Set all the blocks inside the selection to a block",
    min = 1,
    max = 1
)
@CommandPermissions("worldedit.region.set")
@Logging(REGION)
public void set(CommandContext args, LocalSession session, LocalPlayer player,
        EditSession editSession) throws WorldEditException {

    Pattern pattern = we.getBlockPattern(player, args.getString(0));

    int affected;

    if (pattern instanceof SingleBlockPattern) {
        affected = editSession.setBlocks(session.getSelection(player.getWorld()),
                ((SingleBlockPattern) pattern).getBlock());
    } else {
        affected = editSession.setBlocks(session.getSelection(player.getWorld()), pattern);
    }

    player.print(affected + " block(s) have been changed.");
}

becomes a more manageable

@Command(aliases = "/set",
         desc = "Set all the blocks inside the selection to a block")
@CommandPermissions("worldedit.region.set")
@Logging(REGION)
public void setBlocks(LocalPlayer player, EditSession editSession,
        @Selection Region region, Pattern replaceWith) throws MaxChangedBlocksException {
    int affected = editSession.setBlocks(region, replaceWith);
    player.print(affected + " block(s) have been changed.");
}

Complex constructs

You can also use more complex constructs.

public void smoothen(LocalPlayer player, EditSession editSession,
        @Selection Region region,
        @Optional("1") @Range(min = 1) int iterations,
        @Switch('n') boolean onlyNaturalBlocks)
    // ...
}

which results in //smooth [-n] [<iterations=1>]

Backwards compatibility

Old-style commands are not known to the new code, but they work just fine. They are processed using the same framework as with the "newer" approach.

void biomeList(CommandContext args, LocalSession session,
               LocalPlayer player, EditSession editSession)

However, usage = (and other relevant properties) must be defined on @Command otherwise usage information would be unavailable.

Bindings

Adding support for a new type of parameter is easy.

@BindingMatch(type = Pattern.class,
              behavior = BindingBehavior.CONSUMES,
              consumedCount = 1)
public Pattern getPattern(ArgumentStack context) 
        throws ParameterException, WorldEditException {
    return worldEdit.getBlockPattern(getLocalPlayer(context), context.next());
}

Exception handling

Previously, non-command related exceptions were handled in an ugly big try-catch that couldn't be 'reused' anywhere else. Now it's stored off in a reusuable class.

@ExceptionMatch
public void convert(IncompleteRegionException e) throws CommandException {
    throw new CommandException("Make a region selection first.");
}

Invocation listeners

WorldEdit has an annotation-based logging mechanism, and this was handled by overriding the method performing the invoke() call. Now it's much cleaner, and you can mix and match modules.

builder.attach(new CommandPermissionsHandler());
builder.attach(new WorldEditExceptionConverter(config));
builder.attach(new LegacyCommandsHandler());
builder.attach(commandLogger = new CommandLoggingHandler(this, config));

Better responses

Before, we had to define usage = "var1 var2 [var3]" by hand, but that was annoying and it could become out of date if the command was changed but the annotation wasn't. Now the new command builder generates these usage strings automatically and uses the names of the parameters (in the source code) to fill in parameter names.

It also provides generally better responses if you type too many arguments or too few. Each argument parser can also throw its own exceptions and present detailed error messages.

Auto-complete

There's also support for generating suggestions dynamically (for when you press TAB to auto-complete) but I'm currently not using it and it needs some minor work first. For example, if you pressed tab on n for a direction, it could complete it to north. No work is needed on part of the command.

[tonybruess]

😮 😮 😮

Very nice. Good utilization of the Github PR as well, very nice indeed.

One thing you may want to consider, is factoring all this code out of WorldEdit. Over at Overcast Network, we've factored out your framework into several packages. Originally, a few months back, we just used what's in WorldEdit with a few minor changes. Recently, we factored it out to add support for both bukkit and bungee.

If you want to take a look at it, the repo is here: https://github.com/OvercastNetwork/sk89q-command-framework

Now, with all the included refactoring you're doing, might be a great opportunity to factor your command framework out of WorldEdit and into a separate repository for other plugin developers to use. We've enjoyed using our command framework in bukkit, and now bungee, and I'm sure other plugin developers would as well.

[sk89q]

I've considered it and even started a repo or two before, but I've been partly on the fence, and also partly busy and not worrying too much about it. I'll think about it again.

Note: I also added some more sections to my initial PR post.

[tonybruess]

We, and other plugin developers would really appreciate it. Also, if you could take our bukkit/bungee/core module system into consideration, we would be even happier.

[sk89q]

I write most of my libraries, when reasonable, to be API-agnostic, so this could be used in anything, even a standalone program. I know some of the libraries in WE are Bukkit-specific, but I can't vouch for what other people do with my libraries after I have written them.

[TomyLobo]

could you allow this to use primitives where you're now only allowing boxed classes? (i.e. "int iterations" instead of "Integer iterations")

[sk89q]

Oh, you can use both primitives and their autoboxed equivalents. The Integer in the example is unnecessary.

[sk89q]

Any objections to merging this?

The commands were tested, and frankly, I don't want to look at this anymore at the moment -- just rather use it :P

I might put it into a separate library, but later because I want to work on some other things first.

[ewized]

Nope go ahead

[tonybruess]

I would prefer if you factored it out into a new library now as opposed to later. Just my opinion

[DarkArc]

A second lib for this new system -- even if temporarily in the WE repo -- would probably be easier to do now rather than later... But no, no objections.

Edit: Never mind, already packaged outside of com.sk89q.worldedit

[sk89q]

Factoring out is just copy code and add as a dependency (+shade).

I don't want to yet because I don't want to do a half ass dump into a repo, and then have someone do something like bundle it into their plugin and give us dependency hell. If I'm going to do it, I'm going to go the entire way.

[sk89q]

Well, I actually could maybe do that towards the end of the week, but y'know, it'd be more of a minor organizational move because WorldEdit has to shade all of these libraries. I can't distribute WE and require dependencies.

I'd likely put it into https://github.com/sk89q/rebar if I do.

[tonybruess]

What if developers want to use your framework, but not rebar?

[wizjany]

The point is that there's many different frameworks/APIs/whatever in worldedit and in the end we either have one dependency or worldedit ends up shading 100 dependencies.

[sk89q]

I don't see what the particular problem with using a library that does more than one thing is. Having several projects means that I have to maintain and release them all individually, which is a lot of work, work that could be spent on writing code instead.

And to be fair, Rebar (or the project behind that) is the reason why Bukkit has @EventHandler to begin with (it was solely my idea), rather than that old god-awful event registration system, plus I was the one that designed the node.permissions system to begin with. Surely if you like using @EventHandler over event registration methods, the permissions system in Minecraft, this command library, and the services framework in Bukkit, there's potentially more API of mine that you could find useful, at least if I get around to adding more of my stuff to Rebar.

[DarkArc]

Some people may have concerns about the size of their files/security reasons, but in terms of a MineCraft plugin library there probably isn't any reason to have these concerns.

[ewized]

There's one thing you must do, trust what sk89q does he know what is good.

[TomyLobo]

yeah, security is kind of a bs concern when it comes to unused stuff

[sk89q]

1.6's coming out soon so I'll probably be starting on another branch off this and adding to it. Primarily breaking up EditSession.

[TomyLobo]

oh, good that you mention it. I have some uncommitted cleanup changes there. I'll drop them then.
Maybe you can add the parts that still apply to your branch.

diff --git src/main/java/com/sk89q/worldedit/EditSession.java src/main/java/com/sk89q/worldedit/EditSession.java
index 59b26ce..35627e4 100644
--- src/main/java/com/sk89q/worldedit/EditSession.java
+++ src/main/java/com/sk89q/worldedit/EditSession.java
@@ -19,6 +19,7 @@
 package com.sk89q.worldedit;

 import java.util.ArrayList;
+import java.util.Collection;
 import java.util.Collections;
 import java.util.Deque;
 import java.util.HashMap;
@@ -533,6 +534,18 @@ public int countBlock(Region region, Set<Integer> searchIDs) {
         return countBlocks(region, passOn);
     }

+    private static boolean containsFuzzy(Collection<BaseBlock> collection, Object o) {
+        // allow -1 data in the searchBlocks to match any type
+        for (BaseBlock b : collection) {
+            if (o instanceof BaseBlock) {
+                if (b.equalsFuzzy((BaseBlock) o)) {
+                    return true;
+                }
+            }
+        }
+        return false;
+    }
+
     /**
      * Count the number of blocks of a list of types in a region.
      *
@@ -543,22 +556,6 @@ public int countBlock(Region region, Set<Integer> searchIDs) {
     public int countBlocks(Region region, Set<BaseBlock> searchBlocks) {
         int count = 0;

-        // allow -1 data in the searchBlocks to match any type
-        Set<BaseBlock> newSet = new HashSet<BaseBlock>() {
-            @Override
-            public boolean contains(Object o) {
-                for (BaseBlock b : this.toArray(new BaseBlock[this.size()])) {
-                    if (o instanceof BaseBlock) {
-                        if (b.equalsFuzzy((BaseBlock) o)) {
-                            return true;
-                        }
-                    }
-                }
-                return false;
-            }
-        };
-        newSet.addAll(searchBlocks);
-
         if (region instanceof CuboidRegion) {
             // Doing this for speed
             Vector min = region.getMinimumPoint();
@@ -577,7 +574,7 @@ public boolean contains(Object o) {
                         Vector pt = new Vector(x, y, z);

                         BaseBlock compare = new BaseBlock(getBlockType(pt), getBlockData(pt));
-                        if (newSet.contains(compare)) {
+                        if (containsFuzzy(searchBlocks, compare)) {
                             ++count;
                         }
                     }
@@ -586,7 +583,7 @@ public boolean contains(Object o) {
         } else {
             for (Vector pt : region) {
                 BaseBlock compare = new BaseBlock(getBlockType(pt), getBlockData(pt));
-                if (newSet.contains(compare)) {
+                if (containsFuzzy(searchBlocks, compare)) {
                     ++count;
                 }
             }

[sk89q]

Well, I'm probably not going to touch EditSession yet. See #250

[DarkArc]

Is this system dead for the foreseeable future, or just this PR?

Edit: Never mind, "Switch to a declarative command registration framework (i.e. the new-commands branch or something similar)."