Skip to content

Rework command system to add context command and autocomplete support #368

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

Closed
11 of 14 tasks
Tais993 opened this issue Feb 6, 2022 · 12 comments · Fixed by #558
Closed
11 of 14 tasks

Rework command system to add context command and autocomplete support #368

Tais993 opened this issue Feb 6, 2022 · 12 comments · Fixed by #558
Assignees
Labels
enhancement New feature or request priority: major valid This issue/PR is validated and ready to be picked. This auto adds items to TJ project board.

Comments

@Tais993
Copy link
Member

Tais993 commented Feb 6, 2022

Is your feature request related to a problem? Please describe.
Discord has added a lot of things, and to use those, we need to update our command system to reflect these changes.

Describe the solution you'd like

Initial proposal

Context commands:

We add BotCommand which extends the UserInteractor added with b9b047a
This interface gets getType, getVisibility and getData.

After that we'll add a MessageContextInteraction and UserContextInteraction interface (for the context menus)
Those 2 and SlashCommand will all implement the BotCommand interface.

Now we will rename SlashCommandVisibility to CommandVisibility

Note, SlashCommand will return SlashCommandData instead of CommandData for the getData method

There will probably also come an abstract class like SlashCommand has for context-command's, but will see that when I go deeper into our system

Autocomplete:

JDA added OptionData#setAutoComplete(boolean). If autocomplete is set to true, Discord will send autocomplete interactions.
Thus, we add onCommandAutoCompleteInteractionEvent(CommandAutoCompleteInteractionEvent) to the SlashCommand
The command has to filter for which option the event is, our command-system will filter for which command.

To-do

  • Rename SlashCommandVisibility to CommandVisibility a0fb71e
  • Move getType, getVisibility and getData to BotCommand a0fb71e
  • Move component ID logic documentation into acceptComponentIdGenerator a0fb71e
  • Add BotCommandAdapter (and move some SlashCommandAdapter method to there) a0fb71e
  • Add ContextCommandAdapter (for the differentation between Message and User context commands)
  • Add MessageContextInteraction + UserContextInteraction a0fb71e
  • Update BotCore
  • Add onAutoComplete to SlashCommand and SlashCommandAdapter a0fb71e
  • Update all documentation
  • Add support for multiple commands of different types with the same name
  • Automatically update commands on start-up (and remove the reload command) e3aa80b
  • Update SlashCommandProvider to become a general CommandProvider e3aa80b

Future:

  • Update JDA
  • onModal method
@Tais993 Tais993 added enhancement New feature or request blocked This issue is currently blocked by another issue (see comments) priority: major labels Feb 6, 2022
@Tais993 Tais993 added this to the Improvement phase 1 milestone Feb 6, 2022
@Tais993
Copy link
Member Author

Tais993 commented Feb 6, 2022

If we agree, I'll work on this with love :p

@Tais993 Tais993 self-assigned this Feb 6, 2022
@marko-radosavljevic
Copy link
Contributor

Me likes autocomplete very much, so you have my unwavering support. 🥰

Now, don't know what 'context command' is, but I see interfaces, and interfaces are good, so I guess I like that too. I'm a bit suspicious that you bundled these two together, tho. Feels like a Trojan horse, you know, since everyone loves autocomplete... 🤔

@Tais993
Copy link
Member Author

Tais993 commented Feb 7, 2022

Context menu's are commands you receive when you right click an user / message.

@Budbomber
Copy link
Contributor

Auto complete could come in extremely handy in a lot of use cases, and I guess like Marko said interfaces makes it better

@Zabuzard
Copy link
Member

Zabuzard commented Feb 8, 2022

Im all in for the autocomplete and im also all in for context menus. But I dont quite get why context menus and slash commands are put together. Like, I am not sure why you proposed your design in the way you proposed it.

So from my side: green light on adding those features.
But orange light for the design.

That said, feel free to get started and once I am more familiar with whats going on, I might propose something different. Or you elaborate a bit on how things work and your reasoning and maybe I can give my 2cents then earlier already.

@Tais993
Copy link
Member Author

Tais993 commented Feb 8, 2022

But I dont quite get why context menus and slash commands are put together. Like, I am not sure why you proposed your design in the way you proposed it.

Within Discord's design context-commands and slash-commands are within the same hierarchy, thus I "copied" their design.
If you find it odd, I could change it without issues. I'd still keep the ComponentUser interface though, I'm fine with removing a general "command" interface, and just going for ContextCommand and SlashCommand separately
And for the fun, I've a quote from one of Discord's developers :p

our goal was to get something out quickly

@Zabuzard
Copy link
Member

Zabuzard commented Feb 8, 2022

Within Discord's design context-commands and slash-commands are within the same hierarchy, thus I "copied" their design.

I see.

If you find it odd, I could change it without issues.

Our SlashCommand design has been born without thinking a lot into the future regarding what else we might need (regular commands, context commands, ...), mostly also because it is just impossible to think that much ahead.

That said, if it makes sense to change existing design for new stuff, it is totally reasonable to do so. In general, it might maybe make sense to first create the new context commands by copy-pasta and then we see which parts are identical and which not and can unify them nicely.

We just have to be aware that in the future there might be another XY-Command thingy coming our way that we want to unify into the system... so as much as it makes sense to merge things together, we also have to be flexible enough to support extra logic at some point.

tldr: Your plan sounds reasonable, go ahead. But be prepared that we will probably change design a bit once we all have a clearer picture on the concrete code.

@Tais993
Copy link
Member Author

Tais993 commented Feb 8, 2022

Will make 2 separate designs, and take a look at mixing them together in the end then ^^

@Tais993
Copy link
Member Author

Tais993 commented Feb 12, 2022

When finished, I'll create a draft PR to the JDA5 branch.

This way the command-system can be viewed without having all changes from the JDA5 branch too.

Once JDA5 is merged, we'll change the target for sure, but this allows us to take a look at the system before hand

@github-actions
Copy link

This issue is stale because it has been open 30 days with no activity. Remove stale label, comment or add the valid label or this will be closed in 5 days.

@github-actions github-actions bot added the stale label Apr 17, 2022
@Tais993 Tais993 removed stale blocked This issue is currently blocked by another issue (see comments) labels Apr 17, 2022
@Tais993 Tais993 mentioned this issue May 31, 2022
8 tasks
@github-actions
Copy link

This issue is stale because it has been open 30 days with no activity. Remove stale label, comment or add the valid label or this will be closed in 5 days.

@github-actions github-actions bot added stale inactivity-closed Issues that have been closed due to inactivity, but are otherwise valid and might be reopened later labels Jun 17, 2022
@Tais993 Tais993 reopened this Jun 23, 2022
@Tais993 Tais993 removed the inactivity-closed Issues that have been closed due to inactivity, but are otherwise valid and might be reopened later label Jun 23, 2022
@marko-radosavljevic marko-radosavljevic added valid This issue/PR is validated and ready to be picked. This auto adds items to TJ project board. and removed stale labels Jun 27, 2022
@Tais993
Copy link
Member Author

Tais993 commented Sep 3, 2022

goat

@Taz03 Taz03 linked a pull request Sep 4, 2022 that will close this issue
Tais993 added a commit that referenced this issue Sep 30, 2022
This is a major rework of the command system

Support for both autocompletion events and context commands has been added in this rework.

The reload command has been removed, instead commands are automatically reloaded on server start

tldr:

Added classes:

CommandReloading
UserInteractorPrefix

BotCommand
BotCommandAdapter
MessageContextcommand
UserContextCommand

Renamed classes:

SlashCommandPriver to CommandProvider
SlashCommandVisibility to CommandVisibility

Removed classes:

ReloadCommand
ReloadCommandTest




Full change log

Application:
the Application now waits till JDA is ready before creating the BotCore, this means that commands can use the cached guilds and such.

CommandReloading:
added reloadCommands(JDA, CommandProvider), this method only automatically runs on start-up, but this can technically be used later on too.

BotCommand:
Added getName, getType, getVisibility and getData

BotCommandAdapter:
Overrides the above listed methods by requiring a CommandData and CommandVisibility in the constructor.
This also adds the generateComponenttId methods

MessageContextCommand:
Adds the onMessageContext method

SlashCommand:
Has been stripped down and many things have been moved to BotCommand
This method also adds onAutoComplete

SlashCommandAdapter:
Overrode getData to return SlashCommandData
Moved ID generation to BotCommandAdapter

UserContextCommand:
Adds the onUserContext method

UserInteractor:
acceptComponentIdGenerator has received documentation about how ID generation is done

UserInteractorPrefix:
Contains the prefixes for classes, in our system slashcommands get annotated with `s-` so other UserInteractor's can have the same name.
This is useful for a.e a report command, this way it can have a report slash command and message context to improve UX

Botcore:
Interactor names are validated here, this way a name cannot start with any of the assigned prefixes.
Also removed the code that registered the reload command, and removed the onReady method

CommandProvider:
This one contains many of the methods BotCore has to implement.
getInteractors and getCommands are part of this.

This also contains methods to get an interactor based of a Class<?> instance, or their prefixedName.

Closes #368
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request priority: major valid This issue/PR is validated and ready to be picked. This auto adds items to TJ project board.
Projects
None yet
4 participants