aikar / commands

Java Command Dispatch Framework - (Bukkit, Spigot, Paper, Sponge, Bungee, JDA, Velocity supported, generically usable anywhere)
https://acfspigot.emc.gs
MIT License
562 stars 144 forks source link

Parameter Switches #150

Open aikar opened 6 years ago

aikar commented 6 years ago

Implement @Switch("res") to be used such as:

@Subcommand("set")
public void onResInfo(Player player, @Switch("r,res") Residence res, Permission perm, PermissionState value) {
  res.getPermissions().setState(perm, value);
}

With this, the following commands should work:

/res set 1111 build true
/res set build -res 1111 true
/res set -r 1111 build true
/res set build true -r 1111

When a parameter has a switch, it can be "Supplied" at any location of parameters (but not before the subcommands)

Switches should process command replacements

We can achieve this by first pre-parsing all switches during context resolution, marking what parameters have been satisfied, and then iterating the remainder of unsatisfied to complete them in order.

No new registration of switches needs to occur.

Switches should use - prefix by default, but the manager should allow customizing switch prefixes.

ZENEGIX commented 6 years ago

It's a good idea.

Is it possible to do (if not already possible) the following? /res set build true - if the player is a Residence, then the changes in the current one in it /res set build true -res 1111 - changes will take place in Residence '1111', but only if the player has the permission to use this switch

aikar commented 6 years ago

I do that already with my res context resolver, its issuer aware.

switch's wont need to care about that, it either has the input or it doesn't, and the resolver handles the case.

right now I'm hacking it with /res set !1111 build true where the ! is 'special', and I want to switch to -r to make it more clear

aikar commented 6 years ago

Need to consider tab completion ideas.

177 suggests putting the names with the handlers, but I think keeping them in parameter order is the most logical.

So if your tab completing a parameter at index 7, even if your filling it at index 3, look up index 7's completion still as we mapped the name to the parameter and we know the parameters real index.

That way, completions still work even when switches are not specified.

bundabrg commented 6 years ago

Some issues.

  1. If you want to have completion working then you'll need to process it like completion does it. Unfortunately completion doesn't have the luxury of the whole input so cannot do a pre-process. Thus the following rules would need to be in place:
    1. A Switch'ed parameter can be referenced at any location BEFORE its parameterized location or can optionally be referenced without the switch at its parameter position, but not after (Its impossible for the tab completion to know if a switch has been seen yet or not otherwise in progress).
    2. An Optional Switch (IE: @Optional @Switch("param") String param) can ONLY be referenced by reference to its name and not position. (Imagine 2 optional switches at positions 5 and 6. It would be impossible to know which one is being completed if using its position only)

This invalidates the original requirement as the required parameter 'res' is referenced after its location in the 2nd and 4th example. This would be impossible if you were doing it whilst inputting the the data for tab completion. A close fix would be to move res to after the value parameter but then it could only be referenced by position after value.

  1. As resolveContexts() is parameter based rather than argument based (A parameter can consume zero or more arguments, rather than an argument consuming parameters) I can't actually think of a way to allow the completion based purely upon parameter location.

For example one may have a parameter that consumes 3 arguments (lets say an X Y and Z space separated as a contrived example) that a single MyLocation parameter could consume. An example could be:

    @CommandCompletion("@players @range:50 @range:50 @range:50")
    public void setLocation(CommandSender sender, OnlinePlayer player, MyLocation location)

MyLocation context would consume the 3 numbered arguments.

Now if MyLocation has a Switch(), then we don't know to move all 3 range parameters when referencing it before the player parameter.

A fix could require the parameter declaring the amount of items it consumes but I think it only does this when resolving and not during completion (not sure if this is possible to do during completion but if so would resolve this issue).

My proposal in the closed Issue was to namespace the CommandCompletions. This means:

@CommandCompletition("@players location:@range:50 location:@range:50 location:@range:50")
public void setLocation(CommandSender sender, OnlinePlayer player, @Switch("location|loc") MyLocation location)

Note I don't really like the namespaced completion either as it looks a little messy so my recommendation is if the parameter can declare up front how many items it consumes. Probably not possible since a casual look shows that even the World.class context can consume either 0 or 1 arguments (based upon if the argument actually resolves to a world or not) which totally messes up even normal completion.

Alternatively, if the parameter itself provides completion. This is the method I'm looking at. For example:

@CommandCompletition("@players")
public void setLocation(CommandSender sender, OnlinePlayer player, @CommandCompletion("@range:50 @range:50 @range:50") @Switch("location|loc") MyLocation location)
aikar commented 6 years ago

Consuming multiple inputs is not something well supported in ACF yet to begin with.

A Switch'ed parameter can be referenced at any location BEFORE its parameterized location or can optionally be referenced without the switch at its parameter position, but not after (Its impossible for the tab completion to know if a switch has been seen yet or not otherwise in progress).

No it's not. We will parse switched parameters and parse them out of the buffer and fill in the parameters before we process unswitched params.

So we can very easily find params done after the parameter index still.

Same can be done for completions too. If you tpe /res foo bar baz blah -res <tab> for (sender, res, foo, bar, baz, blah)

we parse out -res, fill in index 1

then map the rest to where they go

command completion would look like @Completion("res foo bar baz blah")

How to handle multiple inputs will have to be considered for another day, as will need some form of parser system that allows it to pre calculate parsed parameters before it goes into completion and execution stages.

This PR can be completed with the current primary use of single word parameters.

aikar commented 6 years ago

Though I don't mind the idea of moving CommandCompletions to support being placed on the parameter too, but that gets harder to read

I'm not opposed to coming up with some form of naming to facilitate skipping params, but on the fence about encouraging that.

bundabrg commented 6 years ago

Unless I'm missing something (which could be very likely) the commandcompletion and the command execution operate 2 different ways. Execution uses parameters that consume arguments, and completion uses arguments that consume the Completion options.

This means if you have the following code:

@Subcommand("set")
@Completion("@res @permission @permissionstate")
public void onResInfo(Player player, @Switch("r,res") Residence res, Permission perm, PermissionState value) {
  res.getPermissions().setState(perm, value);
}

(assuming those completions are defined) then try the following:

/cmd set build true -res 1111

It would work fine for execution. You can parse out the '-res' and sort it all out.

But whilst typing it, when you reach the 'build' parameter, the tab completer will not know that its a permission and will try match residence instead. So you'll get all the hints for residence. Even once past that point whilst typing 'true' it will be popping up completions for permissions. There is not enough context to provide the tab completion knowledge that '-res' is yet to be typed and wasn't just provided as a positional parameter.

bundabrg commented 6 years ago

I have the following in my code that is working okish. Still ironing out bugs. Handles multiple consumed arguments (not that I use more than 1 anyway)

    @Subcommand("open|o")
    @Description("Open Menu")
    @CommandCompletion("@players")
    public void onOpen(CommandSender sender,
                       @CommandCompletion("@menu.config") String path,
                       @Switch("player,p") OnlinePlayer player,
                       @CommandCompletion("test1|test2|test3") @Switch("test,t") String test) {

Works with:

/cmd open menu1 PlayerName test3
/cmd open -player PlayerName menu1 test3
/cmd open -test test2 menu1 PlayerName
/cmd open -player PlayerName -test test1 menu1

with full code completion: (shows -player, -test if first character is - for an arg, removing those already seen either positionally or by name, and will show the proper completion for the argument)

(Note in this example I use both the Method and Parameter version of CommandCompletion. It merges it in properly based upon if the parameter has one or not).

Note also that I can't have:

/cmd open menu1 test1 -player PlayerName

As the tab completion will think test1 is a positional Player variable, though if I ignore its hints the executor will get it right with the extra context.

aikar commented 6 years ago

Well you do bring up a good point. For Completions that use context (ie, only show permissions that are allowed/set on the specified res), there is an order issue here.

but nothing solves that outside of the user simply not putting it after. naming it like you suggested doesn't solve that either.

My goal for this is that it would be parsed in 2 stages, parse out switches, build a new buffer without the switched data, reparse for the rest.

We just need to handle the case where completion handler does .getContext(switchedParameter) and that parameter isn't supplied yet, on how to handle that.

Either error, fall back to sender aware, etc.

bundabrg commented 6 years ago

I don't think it needs to be named as I originally thought (that was more to deal with edge cases when a parameter consumes something other than 1 argument), and can easily be worked around with a parameter CommandCompletion (I actually got that horrid 3 argument MyLocation example to work as a proof of concept)

But I still think you may be missing my point (or we may just be agreeing with each, I'm a little tired :). I totally agree that during execution you can do the 2 stage parse. That's exactly what I do. I go through, pull out all the name parameters first and then go through the positional ones. That works fine.

But during tab completion you can't do this as you can't see the future. It means you either have to put up with incorrect tab completions which get worse the more parameters you have (as the executor will get it right no matter what) or ensure you properly position the arguments to take care of when its used positionally or include the @Optional flag as well to force it to only be recognized by name and never positionally which helps a lot as well. It means that whilst all the examples in the original post will work during execution, they will show incorrectly during tab completion (at least example 2 and example 4).

heroslender commented 3 years ago

Is this feature still being worked on? Saw the PR but there's no activity there for over 2 years :confused:

Also, would it be possible to use boolean @Switch without the the need to specify the value? For example: /foo -s would result in the Switch s being true, and /foo would result in s being false, wich could be obtained with the use of @Default("false").

The problem would really be the 1st one, maybe make it so that boolean switches don't have a input value.

bundabrg commented 3 years ago

I ended up implementing it in my own command parser (Docs) mainly for fun.