PaperMC / Velocity

The modern, next-generation Minecraft server proxy.
https://papermc.io/software/velocity
GNU General Public License v3.0
1.68k stars 583 forks source link

Tab completion for first argument does not work correctly using SimpleCommand #1319

Open EvilJavaSkill opened 1 month ago

EvilJavaSkill commented 1 month ago

Hello Velocity-Team,

I just migrated from waterfall to Velocity 3.3.0-SNAPSHOT-b389 and found that the first argument of an SimpleCommand isn't tab-completed correctly after I rewrote my plugins commands from waterfall to velocity.

In my command I used the following code to check what's the cause.

@Override
public CompletableFuture<List<String>> suggestAsync(Invocation invocation)
{
    CommandSource sender = invocation.source();
    String label = invocation.alias();
    String[] args = invocation.arguments();
    System.out.println("COMMAND | " + sender + " | " + label + " | " + String.join(", ", args) + " | " + args.length);
    if(args.length == 1)
        // tab completion here
}

I got these outputs:

Input: /mycommand (note the space at the end) Output: COMMAND | [connected player] EvilJavaSkill (/ip) | mycommand | | 0

Input: /mycommand E Output: COMMAND | [connected player] EvilJavaSkill (/ip) | mycommand | E | 1

Input: /mycommand (note the 2 spaces at the end) Output: COMMAND | [connected player] EvilJavaSkill (/ip) | mycommand | , | 2

Inputting two spaces sets the args.length to 2 (output nr. 3) but inputting only one does set it to 0 (output nr. 1). The player has to enter a character first before the args length says that the player is trying to tab-complete the first argument.

In my code I check for an args.length equals 1 to make sure the player is tab-completing the first argument. But somehow when the player hasn't entered any letter no tab-completion will be done due to invocation.arguments(); returning an empty array. I would say that invocation.arguments(); should return an array with an empty string in it.

I consider this is a bug if there is no special reason why velocity does not provide an array length of 1.

If there are any questions, let me know and I will help as much as I can.

Kind regards

EvilJavaSkill

0PandaDEV commented 1 month ago

I have the same issue, argument 1 does not exist in my case. I did some debugging and got this in the console. image

0PandaDEV commented 1 month ago

I found a workaround, you can somehow trigger the first argument when it has a value.

if (args.length == 0 || (args.length == 1 && args[0].isEmpty())) {
    return TABCOMPLETEARRAYS;
} else if (args.length == 1) {
    return TABCOMPLETEARRAYS;
}
electronicboy commented 1 month ago

https://github.com/PaperMC/Velocity/blob/afd8b55fb52daa36e59f10b3ceb1fa953c29c485/proxy/src/main/java/com/velocitypowered/proxy/command/brigadier/StringArrayArgumentType.java#L49

EvilJavaSkill commented 1 month ago

https://github.com/PaperMC/Velocity/blob/afd8b55fb52daa36e59f10b3ceb1fa953c29c485/proxy/src/main/java/com/velocitypowered/proxy/command/brigadier/StringArrayArgumentType.java#L49

Yeah, that looks like it's the issue. In my opnion it has to return new String[] { "" } or a constant containing a single empty string and not EMPTY, so there is an element in the array which makes it has a length of 1.

An alternativ workaround is to check if(args.length == 0 || args.length == 1), but why should it be an empty array? I don't really like the idea of having two states which indicate that the player is trying to tab-complete the first argument.