10000 Improve console completion with brig suggestions by Machine-Maker · Pull Request #9251 · PaperMC/Paper · GitHub
[go: up one dir, main page]

Skip to content

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

Merged

Conversation

Machine-Maker
Copy link
Member
@Machine-Maker Machine-Maker commented Jun 3, 2023

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 inserts minecraft:z after your current position. This is because vanilla matches keys against the namespace and path of ResourceLocation separately. So you end up with tp @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

@Machine-Maker Machine-Maker requested a review from a team as a code owner June 3, 2023 17:17
@Machine-Maker Machine-Maker changed the title improve brig console completion improve brig console completion for selectors Jun 3, 2023
@Machine-Maker Machine-Maker force-pushed the fix/brig-selector-completion branch from 7df7cfe to edbc97c Compare June 3, 2023 17:17
@electronicboy
Copy link
Member

The Suggestion object supplies a range in which the completion is supposed to start/end for, screwing around, I've had some variations of luck trying to coax it to do something

image

just, what I have right now fails as soon as you try:
image

    private static @NonNull String possiblyPrefixBrigSuggestion(final @NonNull Suggestion suggestion, final @NonNull String line) {
        String suggestionLine = suggestion.getText();
        if (true)
        return line.substring(0, suggestion.getRange().getStart()).substring(line.lastIndexOf(' ') + 1) + suggestion.getText();

I've got a headache coming on, it's probably something stupid though

@Machine-Maker Machine-Maker force-pushed the fix/brig-selector-completion branch from edbc97c to 39dd11a Compare June 5, 2023 03:45
@willkroboth
Copy link
Contributor

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 VanillaCommandWrapper doesn't handle the range from the Suggestions correctly.

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.

@electronicboy
Copy link
Member

My understanding is that that is the very issue that we ended up actually resolving here

@Machine-Maker Machine-Maker force-pushed the fix/brig-selector-completion branch 2 times, most recently from e87a688 to 924a5a7 Compare July 26, 2023 02:36
@Machine-Maker
Copy link
Member Author

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.

@willkroboth
Copy link
Contributor

In my own testing, I did notice the two consoles acted a bit differently

My understanding is that that is the very issue that we ended up actually resolving here

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 tp @e in the console, pressing tab would immediately replace the console text with tp [. On Paper (as the original comment here describes), pressing tab wouldn't do anything.

I would expect the issue on Spigot needs to be resolved by modifying VanillaCommandWrapper#tabComplete. I don't think the patch in this PR wouldn't work directly for Spigot by itself because it looks like it changes Paper-specific classes added by previous patches, and not anything in VanillaCommandWrapper.

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 VanillaCommandWrapper wouldn't interfere with the console suggestions anymore. This is probably still a good patch until that gets worked out though.

@willkroboth
Copy link
Contributor

TLDR; I tested my custom Brigadier suggestions using this patch. Unfortunately, it didn't quite work, and gave me this error:

[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-"924a5a7"]
        at io.papermc.paper.console.BrigadierCommandCompleter.lambda$addCandidates$0(BrigadierCommandCompleter.java:60) ~[paper-1.20.1.jar:git-Paper-"924a5a7"]
        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-"924a5a7"]
        at io.papermc.paper.console.BrigadierCommandCompleter.complete(BrigadierCommandCompleter.java:44) ~[paper-1.20.1.jar:git-Paper-"924a5a7"]
        at org.bukkit.craftbukkit.v1_20_R1.command.ConsoleCommandCompleter.addCompletions(ConsoleCommandCompleter.java:150) ~[paper-1.20.1.jar:git-Paper-"924a5a7"]
        at org.bukkit.craftbukkit.v1_20_R1.command.ConsoleCommandCompleter.complete(ConsoleCommandCompleter.java:119) ~[paper-1.20.1.jar:git-Paper-"924a5a7"]
        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-"924a5a7"]

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 MapArgument. The MapArgument uses custom Brigadier suggestions and a parser to turn a greedy string into a Java Map.

Here's the test plugin I was using when I got that error:
CommandAPITest-1.0-SNAPSHOT.zip

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

[Server thread/INFO]: This server is running CraftBukkit version 3838-Spigot-3374045-a2fafdd (MC: 1.20.1) (Implementing API version 1.20.1-R0.1-SNAPSHOT)
[Server thread/INFO]: Checking version, please wait...
[Thread-20/INFO]: You are running the latest version

When typing test a=, it should suggest 1, 2, and 3 to make test a=1, test a=2, and test a=3. On Spigot, it does suggest 1, 2, and 3, but it cuts off the old input to make test 1, test 2, and test 3. Hence, SPIGOT-7366: Console tab complete cuts off old input when Vanilla commands use offset Brigadier suggestions.

Latest Paper

Checking version, please wait...
This server is running Paper version git-Paper-97 (MC: 1.20.1) (Implementing API version 1.20.1-R0.1-SNAPSHOT) (Git: 1837f6c)
You are running the latest version

Try to type test a=, the same as before. Pressing tab makes no suggestions, just like the original comment here says.

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

Note that no suggestions appear until inserting a space to start the next argument. Eg, test a=1, suggests nothing (should suggest , to make test a=1, ), but test a=1, does correctly suggest the next keys, b and c.

This PR ("Console patch")

Checking version, please wait...
This server is running Paper version git-Paper-"924a5a7" (MC: 1.20.1) (Implementing API version 1.20.1-R0.1-SNAPSHOT) (Git: 924a5a7 on fix/brig-selector-completion)
Error obtaining version information

This PR is promising at first. Typing test a= and pressing tab suggests a=1, a=2, and a=3, to make test a=1, test a=2, and test a=3, as it should. So, this PR fixes the original problem I had.

However, there seems to be a little inconsistency with the player's behavior still. Pressing tab after test a=1 should suggest , which becomes test a=1, . However, if I try this in the console, it gives test a=1,\ . I'm not sure why the \ character was added in, but that shouldn't happen.

Additionally, if you press tab with /test a=1,\ as the player, the suggestions get corrected to /test a=1, . In the console, pressing tab with test a=1,\ gives the exception posted at the top of this comment.

These two problems, the \ character being added in when completing test a=1 and the exception that happens when completing test a=1,\ , both only happen when using this PR and using the console. The exception looks to be coming from the classes targeted by this PR, so I suspect there's a logic bug in the new code being triggered by these situations.

#8235 ("Command recode thing")

Checking version, please wait...
This server is running Paper version git-Paper-"fa50e35" (MC: 1.20.1) (Implementing API version 1.20.1-R0.1-SNAPSHOT) (Git: fa50e35 on command-recode-thing)
Error obtaining version information

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 tp @e and pressing tab does not give any suggestions, just like latest Paper. So, it seems the Paper version of the issue persists over there, and my test command situation would probably not be fixed.

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.

@Machine-Maker Machine-Maker force-pushed the fix/brig-selector-completion branch from 924a5a7 to d1b3f18 Compare August 9, 2023 00:08
@Machine-Maker
Copy link
Member Author

@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.

@Machine-Maker Machine-Maker added the publish-pr Enables a workflow to build Paperclip jars on the pull request. label Aug 9, 2023
@willkroboth
Copy link
Contributor

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 test a=1 would incorrectly suggest ,\ to make test a=1,\ , escaping the space character as you said. Now, test a=1 correctly suggests , to make test a=1, , which is the same behavior as the player.


One problem is that I can still trigger the exception from my previous comment. If I type test a=1,\ as the player, the suggestions are supposed to correct my mistake and suggest , to make test a=1, . However, if I type test a=1,\ and press tab as the console, this message shows up:

[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.

Main branch:
Setblock base

This PR:
Setblock branch

In both cases, the console suggestions incorrectly escape the space characters (typed setblock and pressed tab 4 times: expected: ~ ~ ~, actual: ~\ ~\ ~).

Note that Spigot also gets this wrong but in its own way. In the image below, I typed setblock and pressed tab 3 times. Spigot somehow decided that I wanted to type ~ ~ ~ ~ (I think this is caused by the same Spigot issue).

Setblock spigot

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 exception with setblock. If you type setblock ~ on main paper, it continues giving wrong suggestions. However, using this branch, typing setblock ~ and pressing tab gives this exception:

[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.

@Machine-Maker Machine-Maker force-pushed the fix/brig-selector-completion branch from d1b3f18 to ae74f52 Compare December 19, 2023 00:12
@Machine-Maker Machine-Maker changed the title improve brig console completion for selectors Improve console completion with brig suggestions Dec 19, 2023
@Machine-Maker
Copy link
Member Author

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.

@Machine-Maker Machine-Maker force-pushed the fix/brig-selector-completion branch 2 times, most recently from 71f4158 to 58623df Compare December 19, 2023 02:35
@willkroboth
Copy link
Contributor

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.

I tested it myself, and it does all seem to work as I expect. setblock's location argument, tp's entity selector argument, and the CommandAPI's MapArgument all look like they match up with the client's suggestions. I wouldn't say I understand the need for a custom parser since I thought Brigadier could handle all the parsing, but if it works, it works.

There are a couple caveats that can probably be addressed. Querying completion while not at the end of the command probably doesn't work 100%, but I'm less concerned about that.

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 setblock |~ ~ ~ (| indicates where my cursor was placed) and pressed tab.

For reference, this is what the client does in the same situation:

Client setblock

Here's the full server log for this test.

@Machine-Maker
Copy link
Member Author

since I though 8000 t Brigadier could handle all the parsing, but if it works, it works.

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 ~ ~ ~ after typing in ~ ~, it would replace the last word with that suggestion. Which only replaces the single ~ instead of both ~ ~ since jline just split on spaces. So the custom parser is for jline to read words the same way brig does so some words can have spaces.

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.

@willkroboth
Copy link
Contributor

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 Suggestion object says it should go.

@TonytheMacaroni
Copy link
Contributor
TonytheMacaroni commented Jun 10, 2024

Did some testing, and encountered this error when tab-completing with an empty console:

[org.jline] Error while parsing line
java.lang.IndexOutOfBoundsException: Index -1 out of bounds for length 1
        at java.base/jdk.internal.util.Preconditions.outOfBounds(Preconditions.java:100) ~[?:?]
        at java.base/jdk.internal.util.Preconditions.outOfBoundsCheckIndex(Preconditions.java:106) ~[?:?]
        at java.base/jdk.internal.util.Preconditions.checkIndex(Preconditions.java:302) ~[?:?]
        at java.base/java.util.Objects.checkIndex(Objects.java:385) ~[?:?]
        at java.base/java.util.ArrayList.get(ArrayList.java:427) ~[?:?]
        at io.papermc.paper.console.BrigadierConsoleParser.parse(BrigadierConsoleParser.java:68) ~[paper-1.20.6.jar:1.20.6-DEV-472d555]
        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:113) ~[paper-1.20.6.jar:1.20.6-DEV-472d555]

EDIT: I've found a similar error, this time tab-completing with input execute as <username> :

[org.jline] Error while parsing line
java.lang.IndexOutOfBoundsException: Index -1 out of bounds for length 3
        at java.base/jdk.internal.util.Preconditions.outOfBounds(Preconditions.java:100) ~[?:?]
        at java.base/jdk.internal.util.Preconditions.outOfBoundsCheckIndex(Preconditions.java:106) ~[?:?]
        at java.base/jdk.internal.util.Preconditions.checkIndex(Preconditions.java:302) ~[?:?]
        at java.base/java.util.Objects.checkIndex(Objects.java:385) ~[?:?]
        at java.base/java.util.ArrayList.get(ArrayList.java:427) ~[?:?]
        at io.papermc.paper.console.BrigadierConsoleParser.parse(BrigadierConsoleParser.java:68) ~[paper-1.20.6.jar:1.20.6-DEV-472d555]
        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:113) ~[paper-1.20.6.jar:1.20.6-DEV-472d555]

@Machine-Maker Machine-Maker force-pushed the fix/brig-selector-completion branch from 58623df to 7a83872 Compare August 27, 2024 20:10
@Machine-Maker
Copy link
Member Author

Ok, I think I fixed those specific issues.

@willkroboth
Copy link
Contributor

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:

Screenshot from 2024-08-28 15-37-50

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.

@Machine-Maker
Copy link
Member Author

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.

@Machine-Maker Machine-Maker force-pushed the fix/brig-selector-completion branch from 7a83872 to e8400c6 Compare September 6, 2024 19:59
@Machine-Maker Machine-Maker force-pushed the fix/brig-selector-completion branch from c5c564c to 9877fc3 Compare September 6, 2024 20:32
@Machine-Maker Machine-Maker merged commit 805a974 into PaperMC:master Sep 6, 2024
2 checks passed
@Machine-Maker Machine-Maker deleted the fix/brig-selector-completion branch September 6, 2024 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
publish-pr Enables a workflow to build Paperclip jars on the pull request.
Projects
Status: Merged
Development

Successfully merging this pull request may close these issues.

5 participants
0