FountainMC / FountainAPI

A 'simple but beautiful' Minecraft Server Plugin API
https://fountainmc.org/
MIT License
5 stars 5 forks source link

Command API #9

Closed PizzaCrust closed 8 years ago

phase commented 8 years ago

I'm a fan of annotations, so I was thinking of something like:

@Command(name = "test")
public void testCommad(String[] args, Sender sender) {
    sender.sendMessage("/test " + Arrays.toString(args));
}

And we could have some other options like

@Command(name = "othertest", sender = Player.class) {// Checks if sender is an instance of the class
}

To register the commands, we could add a method to Fountain or the Server.

@Plugin(id = "testCommands")
public class TestCommands {

    @EventHandler
    public void onEnable(ServerStartEvent event) {
        Fountain.getCommandManager().registerCommands(this);
    }

    @Command
    //...

}
PizzaCrust commented 8 years ago

Hmm, I'll create a pull request adding these API features.

PizzaCrust commented 8 years ago

Possibly, like this instead?

@Command(name = "test", allow = SenderType.PLAYER)
public void onCommandExecuted(CommandArguments arguments, Sender sender) {}

CommandArguments allows to merge and do additional parsing operations.

phase commented 8 years ago

CommandArguments could just hold the Sender.

PizzaCrust commented 8 years ago

@phase Good idea.

phase commented 8 years ago

I don't think that will work with the allow field. :confused:

PizzaCrust commented 8 years ago

Why?

phase commented 8 years ago

If Sender is a superinterface of Player and Console or whatever, then you'd have to cast it to the appropriate interface in your code. I think

@Command(allow = Player.class)
void command(String[] args, Player player){}

makes a lot more sense. You don't need to cast it to anything, nor we need an Enum. CommandArguments seems like a useless object.

PizzaCrust commented 8 years ago

Enums are much neater than using direct references to classes.

A player example.

@Command(name = "test", allow = SenderType.PLAYER)
void command(CommandInfo<Player> info) { Player player = info.getSender(); }

Console and player example.

@Command(name = "both")
void command(CommandInfo<Sender> info) { Sender sender = info.getSender(); }

CommandInfo provides parsing of arguments, and a direct instance to the sender using generics. I think CommandInfo should be added, so it's neater. And, no casting.

phase commented 8 years ago

I didn't think about generics :dizzy_face:

I don't see the point of a wrapper object for only an array and another object. It makes more sense to put them as the method arguments, and it's easier for plugins to directly access the arguments instead of creating local variables for the CommandInfo fields.

void command(String[] args, Player player) {
    player.sendMessage(args[0]);
}

// vs

void command(CommandInfo<Player> info) {
    info.getSender().sendMessage(info.getArguments()[0]);
}

// or

void command(CommandInfo<Player> info) {
    Player player = info.getSender();
    String[] args = info.getArguments();
    // lots of logic which access the player and arguments a lot
}
PizzaCrust commented 8 years ago

CommandInfo also provides parsing utilities, which saves time.

void onMessageCommand(CommandInfo<Sender> info) { 
     String message = info.mergeArguments(0, info.getArguments().length);
     System.out.println(message);
}

Otherwise, the developer must do it manually:

void onMessageCommand(CommandInfo<Sender> info) {
       int currentIndex = startingIndex;
        StringBuilder builder = new StringBuilder();
        while (currentIndex != endIndex) {
            builder.append(array[currentIndex] + " ");
            currentIndex++;
        }
        return builder.toString().trim();
}
phase commented 8 years ago

Guava has support for String joining, and so does Java 8.

PizzaCrust commented 8 years ago

I'll fix that in a pull request in a moment.

phase commented 8 years ago

Closed in https://github.com/FountainMC/FountainAPI/commit/04db67509dffd5c9b61ad74a4bfec80f837ef2ed

wgaylord commented 8 years ago

Correct this allows to If I dynamicly add commands? If not we should add that ability.

PizzaCrust commented 8 years ago

yes if you do some bytecode generation and register the generated class, lol

Techcable commented 8 years ago

We want to discourage bytecode generation.......

PizzaCrust commented 8 years ago

@Techcable Ok, then there should be a new proposed API dealing with this.

PizzaCrust commented 8 years ago

Probably like this:

Fountain.getServer().getCommandManager().register(ICommand command);
wgaylord commented 8 years ago

Yeah because this is the one thing bukkit lacks. (And everyone uses reflection to back at.)

PizzaCrust commented 8 years ago

BukkitCommand is like ICommand. (in bukkit)