-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Improve console completion with brig suggestions #9251
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve console completion with brig suggestions #9251
Conversation
7df7cfe
to
edbc97c
Compare
edbc97c
to
39dd11a
Compare
I just noticed this PR. It reminded me of SPIGOT-7366, which I reported about the same time as this actually. In my own testing, I did notice the two consoles acted a bit differently, but it looks like they have the same root issue where There hasn't been any activity on that issue, but I thought I'd link it here for context. It seems likely Paper will fix this before Spigot. However, if that issue gets resolved first, it would probably affect the solution here, so it's worth keeping an eye on. |
My understanding is that that is the very issue that we ended up actually resolving here |
e87a688
to
924a5a7
Compare
Rebased, and generalized the fix to now just emulate replacing the full last word of the command with the suggestion even if there is no matching characters between the current word and the suggestion. |
Yeah, what I meant by this is that when I noticed the bug on Spigot, I tested it on Paper and got a different (but still wrong result). On Spigot, after typing I would expect the issue on Spigot needs to be resolved by modifying That's not to say this PR won't work for Paper. It sounds like you all have tested it and it does work. However, if SPIGOT-7366 is resolved by Spigot, that might affect the results here. So, it's just worth keeping an eye on that issue in case something changes. Also, my first encounter with this issue wasn't with the vanilla tp command. I discovered the bug myself when designing a custom Brigadier suggestion, and tp was the first example I thought Spigot would accept as a "real bug". So, it's possible this bug could also happen with arguments other than the entity selector, and just with brigadier-provided suggestions in general. The general solution of "replacing the full last word of the command" probably solves these solutions, though it might be worth checking if you haven't already. (Personally, I will test this PR with my custom suggestions to see if it solves my original issue) Also, another thing I just thought of, #8235 might also solve this issue. If that PR moves all command logic to a Brigadier CommandDipatcher, I'd guess the bugged logic in |
TLDR; I tested my custom Brigadier suggestions using this patch. Unfortunately, it didn't quite work, and gave me this error:
For context, I'll explain what I'm doing and how it works in other places. I found this bug while working on a plugin called CommandAPI. I was working on some changes to their Here's the test plugin I was using when I got that error: That can be reproduced independently by using CommandAPI version 9.0.3 and this command: new CommandAPICommand("test")
.withArguments(
new MapArgumentBuilder<String, String>("map", '=', ", ")
.withKeyMapper(s -> s)
.withValueMapper(s -> s)
.withKeyList(List.of("a", "b", "c"))
.withValueList(List.of("1", "2", "3"))
.build()
)
.executes((sender, args) -> {
sender.sendMessage("Command run");
sender.sendMessage(args.get(0).toString());
})
.register(); You can verify the command and suggestions generation are correct by using the player. Here's the 4 situations I tested (all Minecraft 1.20.1): Latest Spigot
When typing Latest Paper
Try to type
Note that no suggestions appear until inserting a space to start the next argument. Eg, This PR ("Console patch")
This PR is promising at first. Typing However, there seems to be a little inconsistency with the player's behavior still. Pressing tab after Additionally, if you press tab with These two problems, the #8235 ("Command recode thing")
I also tried testing this with Owen's command recode, but the CommandAPI currently seems to be incompatible and throws an error when I try to register a command. So, I can't test my command with that PR. However, typing That might mean once these patches and Owen's patches are accepted, it would be possible to trigger the exception I got with the custom Brigadier suggestions, but only using the "correct" pathway given by Owen's API. |
924a5a7
to
d1b3f18
Compare
@willkroboth Ok, I think I fixed your issue. It came from your suggestions including a trailing space. The console completion library tries to "escape" any space character inserted in a suggestion as spaces are the delimiter between 2 arguments, not part of a suggestion for an argument. |
Cool, thank you! It does seem to work better, but unfortunately I still found some issues. The change does fix the usual suggestions. Before, typing One problem is that I can still trigger the exception from my previous comment. If I type [org.jline] Error while finding completion candidates
java.lang.StringIndexOutOfBoundsException: begin 0, end -4, length 0
at java.lang.String.checkBoundsBeginEnd(String.java:4604) ~[?:?]
at java.lang.String.substring(String.java:2707) ~[?:?]
at io.papermc.paper.console.BrigadierCommandCompleter.toCandidate(BrigadierCommandCompleter.java:77) ~[paper-1.20.1.jar:git-Paper-"6471d4f"]
at io.papermc.paper.console.BrigadierCommandCompleter.lambda$addCandidates$0(BrigadierCommandCompleter.java:60) ~[paper-1.20.1.jar:git-Paper-"6471d4f"]
at java.util.ArrayList.forEach(ArrayList.java:1511) ~[?:?]
at io.papermc.paper.console.BrigadierCommandCompleter.addCandidates(BrigadierCommandCompleter.java:58) ~[paper-1.20.1.jar:git-Paper-"6471d4f"]
at io.papermc.paper.console.BrigadierCommandCompleter.complete(BrigadierCommandCompleter.java:44) ~[paper-1.20.1.jar:git-Paper-"6471d4f"]
at org.bukkit.craftbukkit.v1_20_R1.command.ConsoleCommandCompleter.addCompletions(ConsoleCommandCompleter.java:150) ~[paper-1.20.1.jar:git-Paper-"6471d4f"]
at org.bukkit.craftbukkit.v1_20_R1.command.ConsoleCommandCompleter.complete(ConsoleCommandCompleter.java:119) ~[paper-1.20.1.jar:git-Paper-"6471d4f"]
at org.jline.reader.impl.LineReaderImpl.doComplete(LineReaderImpl.java:4381) ~[jline-reader-3.20.0.jar:?]
at org.jline.reader.impl.LineReaderImpl.doComplete(LineReaderImpl.java:4347) ~[jline-reader-3.20.0.jar:?]
at org.jline.reader.impl.LineReaderImpl.expandOrComplete(LineReaderImpl.java:4286) ~[jline-reader-3.20.0.jar:?]
at org.jline.reader.impl.LineReaderImpl$1.apply(LineReaderImpl.java:3778) ~[jline-reader-3.20.0.jar:?]
at org.jline.reader.impl.LineReaderImpl.readLine(LineReaderImpl.java:679) ~[jline-reader-3.20.0.jar:?]
at org.jline.reader.impl.LineReaderImpl.readLine(LineReaderImpl.java:468) ~[jline-reader-3.20.0.jar:?]
at net.minecrell.terminalconsole.SimpleTerminalConsole.readCommands(SimpleTerminalConsole.java:158) ~[terminalconsoleappender-1.3.0.jar:?]
at net.minecrell.terminalconsole.SimpleTerminalConsole.start(SimpleTerminalConsole.java:141) ~[terminalconsoleappender-1.3.0.jar:?]
at net.minecraft.server.dedicated.DedicatedServer$1.run(DedicatedServer.java:102) ~[paper-1.20.1.jar:git-Paper-"6471d4f"] While the console no longer suggests that malformed input, someone could accidently type it in. This might be something weird with my special case, but since it works for the player I think there might be a bug hiding somewhere for the console specifically. The second problem is new. When you mentioned spaces inside an argument, I immediately thought of location arguments. Those have 3 numbers for the 3 dimensions separated by spaces, but they are handled and suggested as a single argument. I tried this on the main branch and with this PR, and unfortunately both give the same broken result. In both cases, the console suggestions incorrectly escape the space characters (typed Note that Spigot also gets this wrong but in its own way. In the image below, I typed For Paper, this seems to be a separate issue from SPIGOT-7366, so maybe not the original purpose of this PR. I don't think escaping spaces in the suggestions is ever correct behavior? Even then, I imagine the fix here would end up in the same patch file. Actually, there is a difference here between main paper and this branch: you can trigger the [org.jline] Error while finding completion candidates
java.lang.StringIndexOutOfBoundsException: begin 0, end -2, length 0
at java.lang.String.checkBoundsBeginEnd(String.java:4604) ~[?:?]
at java.lang.String.substring(String.java:2707) ~[?:?]
at io.papermc.paper.console.BrigadierCommandCompleter.toCandidate(BrigadierCommandCompleter.java:77) ~[paper-1.20.1.jar:git-Paper-"6471d4f"]
at io.papermc.paper.console.BrigadierCommandCompleter.lambda$addCandidates$0(BrigadierCommandCompleter.java:60) ~[paper-1.20.1.jar:git-Paper-"6471d4f"]
at java.util.ArrayList.forEach(ArrayList.java:1511) ~[?:?]
at io.papermc.paper.console.BrigadierCommandCompleter.addCandidates(BrigadierCommandCompleter.java:58) ~[paper-1.20.1.jar:git-Paper-"6471d4f"]
at io.papermc.paper.console.BrigadierCommandCompleter.complete(BrigadierCommandCompleter.java:44) ~[paper-1.20.1.jar:git-Paper-"6471d4f"]
at org.bukkit.craftbukkit.v1_20_R1.command.ConsoleCommandCompleter.addCompletions(ConsoleCommandCompleter.java:150) ~[paper-1.20.1.jar:git-Paper-"6471d4f"]
at org.bukkit.craftbukkit.v1_20_R1.command.ConsoleCommandCompleter.complete(ConsoleCommandCompleter.java:119) ~[paper-1.20.1.jar:git-Paper-"6471d4f"]
at org.jline.reader.impl.LineReaderImpl.doComplete(LineReaderImpl.java:4381) ~[jline-reader-3.20.0.jar:?]
at org.jline.reader.impl.LineReaderImpl.doComplete(LineReaderImpl.java:4347) ~[jline-reader-3.20.0.jar:?]
at org.jline.reader.impl.LineReaderImpl.expandOrComplete(LineReaderImpl.java:4286) ~[jline-reader-3.20.0.jar:?]
at org.jline.reader.impl.LineReaderImpl$1.apply(LineReaderImpl.java:3778) ~[jline-reader-3.20.0.jar:?]
at org.jline.reader.impl.LineReaderImpl.readLine(LineReaderImpl.java:679) ~[jline-reader-3.20.0.jar:?]
at org.jline.reader.impl.LineReaderImpl.readLine(LineReaderImpl.java:468) ~[jline-reader-3.20.0.jar:?]
at net.minecrell.terminalconsole.SimpleTerminalConsole.readCommands(SimpleTerminalConsole.java:158) ~[terminalconsoleappender-1.3.0.jar:?]
at net.minecrell.terminalconsole.SimpleTerminalConsole.start(SimpleTerminalConsole.java:141) ~[terminalconsoleappender-1.3.0.jar:?]
at net.minecraft.server.dedicated.DedicatedServer$1.run(DedicatedServer.java:102) ~[paper-1.20.1.jar:git-Paper-"6471d4f"] So, I guess that's not just a problem with my special case. That error is reproducible without plugins. Here's the full server log for that. |
d1b3f18
to
ae74f52
Compare
Ok, I think I handled suggestions with spaces correctly now. I had to build a custom parser to make sure words were created correctly and not separated purely on spaces. There are a couple caveats that can probably be addressed. Querying completion while not 8000 at the end of the command probably doesn't work 100%, but I'm less concerned about that. |
71f4158
to
58623df
Compare
I tested it myself, and it does all seem to work as I expect.
I'm not sure if this is exactly what you're talking about, but I managed to get this exception to show up: [Server console handler/INFO]: [org.jline] Error while parsing line
java.lang.IndexOutOfBoundsException: Index -1 out of bounds for length 2
at jdk.internal.util.Preconditions.outOfBounds(Preconditions.java:64) ~[?:?]
at jdk.internal.util.Preconditions.outOfBoundsCheckIndex(Preconditions.java:70) ~[?:?]
at jdk.internal.util.Preconditions.checkIndex(Preconditions.java:266) ~[?:?]
at java.util.Objects.checkIndex(Objects.java:359) ~[?:?]
at java.util.ArrayList.get(ArrayList.java:427) ~[?:?]
at io.papermc.paper.console.BrigadierConsoleParser.parse(BrigadierConsoleParser.java:68) ~[paper-1.20.4.jar:git-Paper-"58648ae"]
at org.jline.reader.impl.LineReaderImpl.doComplete(LineReaderImpl.java:4371) ~[jline-reader-3.20.0.jar:?]
at org.jline.reader.impl.LineReaderImpl.doComplete(LineReaderImpl.java:4347) ~[jline-reader-3.20.0.jar:?]
at org.jline.reader.impl.LineReaderImpl.expandOrComplete(LineReaderImpl.java:4286) ~[jline-reader-3.20.0.jar:?]
at org.jline.reader.impl.LineReaderImpl$1.apply(LineReaderImpl.java:3778) ~[jline-reader-3.20.0.jar:?]
at org.jline.reader.impl.LineReaderImpl.readLine(LineReaderImpl.java:679) ~[jline-reader-3.20.0.jar:?]
at org.jline.reader.impl.LineReaderImpl.readLine(LineReaderImpl.java:468) ~[jline-reader-3.20.0.jar:?]
at net.minecrell.terminalconsole.SimpleTerminalConsole.readCommands(SimpleTerminalConsole.java:158) ~[terminalconsoleappender-1.3.0.jar:?]
at net.minecrell.terminalconsole.SimpleTerminalConsole.start(SimpleTerminalConsole.java:141) ~[terminalconsoleappender-1.3.0.jar:?]
at net.minecraft.server.dedicated.DedicatedServer$1.run(DedicatedServer.java:102) ~[paper-1.20.4.jar:git-Paper-"58648ae"] This happened when I typed For reference, this is what the client does in the same situation: Here's the full server log for this test. |
Brig still does do the parsing, but jline was doing its own parsing as well. And its parsing treated all spaces (not in quotes) as word separators and that messed up how it tried to replace the last word with the suggestion text. If the suggestion was I can probably fix the exceptions from completing from not-the-end, but idk if I wanna dive into trying to make it work with them. |
Ah, I'm not familiar with jline, but that makes sense. It's too bad you can't just plonk the suggestion down where Brigadier's |
Did some testing, and encountered this error when tab-completing with an empty console:
EDIT: I've found a similar error, this time tab-completing with input
|
58623df
to
7a83872
Compare
Ok, I think I fixed those specific issues. |
It seems to me that all issues brought up in previous comments are now resolved 👍. I did notice this warning in the console when starting the server: I didn't stumble upon any issues though when experimenting with suggesting arguments containing quotes, so I don't know if it's an important warning. |
Yeah, I don't think escaped or quoted words apply to this, so it should be fine. Maybe there's a way to hide that warning so people don't get the wrong idea. |
7a83872
to
e8400c6
Compare
c5c564c
to
9877fc3
Compare
So I noticed that tab completions for entity selectors were not working in the console. Basically after typing in
tp @e
and hitting tab, nothing would happen when in-game, a suggestion of[
would show up. Turns out this is due to an incompatibility between jline's tab completion which expects tab completion candidates to include the full "word" instead of just any next characters. So I fixed this by combining the last word in the command with the suggestions, merging them if there is some overlap.Now it works as expected, with a minor quirk.
If you type in
tp @e[type=z
and hit tab, it insertsminecraft:z
after your current position. This is because vanilla matches keys against the namespace and path of ResourceLocation separately. So you end up withtp @e[type=zminecraft:z
which is wrong. I'm not sure of the best approach to fix this? Perhaps we can somehow disable this double matching, and only match against the full namespace:path from the beginning just for consoles?@jpenilla this might be of some interest to you with your better fabric console mod as I know this shares similar logic to that.
Download the paperclip jar for this pull request: paper-9251.zip