Skip to content

Add support for tab completing and highlighting console input from the Brigadier command tree#5437

Merged
zachbr merged 1 commit into
PaperMC:masterfrom
jpenilla:console-moment
Apr 17, 2021
Merged

Add support for tab completing and highlighting console input from the Brigadier command tree#5437
zachbr merged 1 commit into
PaperMC:masterfrom
jpenilla:console-moment

Conversation

@jpenilla
Copy link
Copy Markdown
Member

@jpenilla jpenilla commented Mar 31, 2021

Adds support for tab completing and highlighting console input from the Brigadier command tree. This works with vanilla commands, regular bukkit commands, as well as bukkit commands that have registered with Brigadier using Commodore/CommandRegisteredEvent.

Screenshot:
image

@jpenilla jpenilla requested a review from a team as a code owner March 31, 2021 10:00
@TheFruxz
Copy link
Copy Markdown
Contributor

OMG, waited for this feature so long, I love it! Have to be inside of paper-api

@zml2008
Copy link
Copy Markdown
Member

zml2008 commented Mar 31, 2021

nice to see this make its way to paper :)

I think an event is unnecessary since plugin devs can already provide completions with the brig api and the event as it stands reeks of implementation detail

also, when I implemented this I ran I to some situations where it seemed like the ranges on brig nodes were out of bounds for the input string, not sure why. iirc I was able to get this with bossbar create at the argument that took a json Component

@wizjany
Copy link
Copy Markdown
Contributor

wizjany commented Mar 31, 2021

Honestly I'd prefer an event over forcing brig. Unfortunately brig doesn't allow for everything that plugins may want to use in commands and adapting to it causes loss of functionality (flags and mid-command optionals to name a few). That said I'm not sure if this needs to be console-limited. Would be cool to have component completions for in game commands too (again, short of relying on brig).

iirc when DemonWav played around with this for paperd he just hooked off the existing event (albeit didn't support components) and it worked pretty well.

@jpenilla
Copy link
Copy Markdown
Member Author

Honestly I'd prefer an event over forcing brig. Unfortunately brig doesn't allow for everything that plugins may want to use in commands and adapting to it causes loss of functionality (flags and mid-command optionals to name a few). That said I'm not sure if this needs to be console-limited. Would be cool to have component completions for in game commands too (again, short of relying on brig).

iirc when DemonWav played around with this for paperd he just hooked off the existing event (albeit didn't support components) and it worked pretty well.

It seems like the AsyncPlayerSendSuggestionsEvent should allow for the same type of functionality for the player. It exposes the Brigadier Suggestions object before sending the tab complete packet to the player. Needing to manually convert your Components to Messages for use as tooltips is a bit annoying and could probably be improved, though.

@jpenilla
Copy link
Copy Markdown
Member Author

jpenilla commented Mar 31, 2021

nice to see this make its way to paper :)

I think an event is unnecessary since plugin devs can already provide completions with the brig api and the event as it stands reeks of implementation detail

also, when I implemented this I ran I to some situations where it seemed like the ranges on brig nodes were out of bounds for the input string, not sure why. iirc I was able to get this with bossbar create at the argument that took a json Component

I think I am mostly in agreement about the event, it basically is an implementation detail.

This line prevents the out of bounds issue: https://github.com/PaperMC/Paper/pull/5437/files#diff-7f71fa74f053b28e5f753d6db8dff2530b32fa0369b2c0ed2e7bacbb28183247R231
image

@wizjany
Copy link
Copy Markdown
Contributor

wizjany commented Mar 31, 2021

Honestly I'd prefer an event over forcing brig. Unfortunately brig doesn't allow for everything that plugins may want to use in commands and adapting to it causes loss of functionality (flags and mid-command optionals to name a few). That said I'm not sure if this needs to be console-limited. Would be cool to have component completions for in game commands too (again, short of relying on brig).
iirc when DemonWav played around with this for paperd he just hooked off the existing event (albeit didn't support components) and it worked pretty well.

It seems like the AsyncPlayerSendSuggestionsEvent should allow for the same type of functionality for the player. It exposes the Brigadier Suggestions object before sending the tab complete packet to the player. Needing to manually convert your Components to Messages for use as tooltips is a bit annoying and could probably be improved, though.

Unfortunately the counterpart here is actually AsyncTabCompleteEvent. The SendSuggestions event happens at a different point in the command lifecycle and requires a complete second pass at processing essentially. I suppose what I'm saying is that I think components should be added to AsyncTabCompleteEvent instead of making a new event. Dunno how break-y that'd be though.

@zml2008
Copy link
Copy Markdown
Member

zml2008 commented Mar 31, 2021

That was something i asked jmp to look at doing as well -- but it sounded like there wasn't any nice way to shove tooltips into there.

@jpenilla
Copy link
Copy Markdown
Member Author

Something like that might be possible but I think it's out of scope for this PR.

@jpenilla
Copy link
Copy Markdown
Member Author

I've removed the event, as I agree with zml that it's too much of an implementation detail. This PR should be "ready for review" now.

@jpenilla jpenilla force-pushed the console-moment branch 3 times, most recently from 6b0d2e3 to f02203a Compare April 2, 2021 10:37
Copy link
Copy Markdown
Member

@JRoy JRoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested with latest EssentialsX which tampers with CommandMap quite a bit and this works!

@kennytv
Copy link
Copy Markdown
Member

kennytv commented Apr 14, 2021

Looks hot and works well from my testing 🐱‍🏍

@codebycam
Copy link
Copy Markdown

Looks hot and works well from my testing too 🐱‍🏍

@zachbr zachbr merged commit 06fb560 into PaperMC:master Apr 17, 2021
@jpenilla jpenilla deleted the console-moment branch May 9, 2021 02:52
ExcessiveAmountsOfZombies pushed a commit to ExcessiveAmountsOfZombies/Paper that referenced this pull request May 14, 2021
LeonTG pushed a commit to LeonTG/Paper that referenced this pull request May 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants