Skip to content
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

feat(model, validate, cache)!: Add new select components #2051

Closed
wants to merge 5 commits into from
Closed

feat(model, validate, cache)!: Add new select components #2051

wants to merge 5 commits into from

Conversation

ImDarkDiamond
Copy link

Adds support for new select components in twilight-model and the twilight-validation crates. See Discord changelog.

I believe that I have included all the necessary changes relating to them but it is possible I missed something.

Not sure if this is how you guys want them implemented. Just let me know or someone else can take it over if you'd like 😄

Breaking changes

  • Renames SelectMenu to StringSelectMenu (Struct & component enum variant)
  • Renames SelectMenuOption to StringSelectMenuOption
  • Renames CommandInteractionDataResolved to InteractionDataResolved

We don't need to rename these and I can revert them back but I do think it's a good change to make since it clarifies exactly what can be done with them. Discord documentation also refers to them as such.

Additions

I added a TypeSelectMenu since 3 (user, mentionable, & role) of the 4 new selects use the same data structure. Not sure if this should be named something else.

Channel selects got their own type, ChannelSelectMenu, which lets you filter which channels are shown by setting channel_types.

Tests

Added and updated a few tests but likely need to add some more. Just not sure what you want specifically tested. I did manually test with one of my bots & they do work.

@github-actions github-actions bot added c-cache Affects the cache crate c-model Affects the model crate c-standby Affects the standby crate c-validate Affects the validate crate m-breaking change Breaks the public API. t-feature Addition of a new feature labels Jan 9, 2023
@vilgotf vilgotf linked an issue Jan 13, 2023 that may be closed by this pull request
Copy link
Member

@zeylahellyer zeylahellyer left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Overall this looks quite good. I have some comments on this on a high-level design.

Presently, we have only SelectMenu "type", and that is what now maps to the StringSelectMenu type. More or less the options in this type functions as a simple K-V choice. With this PR, we will now have three types: TypeSelectMenu, which is in essense the "component base" with no additional fields; ChannelSelectMenu, which is the base component but with a channel_types field; and StringSelectMenu, which is the base component with the options field (and presently the only type with options).

TypeSelectMenu covers the "mentionables", "roles", and "users" component types, which have no fields on top of the base component definition. If we think about how this might be extended in the future, I think it might not be unreasonable to conceptualize having some sort of user_types field (with variants of users and bots), or some other sort of filter. We can also apply this to the roles selection, as "types" or "classes" of roles may be conceptualized (premium role filters? role tags? I don't know).

Were Rust an inheritance class-based language, we could simply call TypeSelectMenu a SelectMenu and have our ChannelSelectMenu and StringSelectMenu types superclass it. Alas, Rust is not, and extending types isn't so simple. Were it just the options field that had a variable type we could use generics, but it is not. Select menus will probably continue to drift from how minimally different these three types are today.

This presents us with a question: do we create a SelectMenu* type for every type of select menu and encapsulate them in a SelectMenu enum? The benefit here could be that, in today's world, a UserSelectMenu wouldn't share the fields that are unique to ChannelSelectMenu and StringSelectMenu. Typically, this feels better and is clearer for users, because it prevents one from even attempting to access the options of a RoleSelectMenu, which conceptually doesn't apply to role select menus.

There is an alternative here: flatten it all into one type. This has been done before, and we could look to inspiration from those types on whether we should do the same here, Our most prominent example of this in history is the Channel type: it was "flattened" from many types into one because there was a lot of overlap between them all, and because it closely resembles how Discord itself structures channels. There is no "audio channel" or "video channel" as such at an API level, there are only channels with those fields identifiable by the type field. Channels with the "guild voice" type, once not able to "be a text channel", is now nearly equivalent to a "guild text" channel. Stage channels, once not able to be used with "video features", was showcased to soon support video during the December Discord Developers event.

Looking to Interaction, it was flattened for a different reason: it was an enum with a unique type for each variant, such as an [ApplicationCommand] type for the ApplicationCommand variant, Ping for the Ping variant, ModalSubmitInteraction for the ModalSubmit variant, and so on. These shared the same base details about what an interaction is, and only contained some additional fields on top of it. Thus, we flattened it into one type, documented what applies when, and trusted users to know/check when something is present.

The case of SelectMenu is perhaps not so different: the base fields of a select menu are largely shared, and there is only one field on top of them. channel_types is somewhat obviously for specifying the types of channels and thus is known on a type name level to not be for, say, roles. options is the only field remaining. If we leave options unchanged then if any other select menu type supports options, the type of option (string select menu options) won't fit. Therefore, we should be prudent here and expect this possibility. There are a few ways we could go about this, such as making options be an array of an enum with a variant being for string options or applying the same logic and making a base SelectMenuOption struct type with value being an enum. Or what if we made options' type be an enum like this?

struct SelectMenuOption<T> {
    ...,
    value: T,
}

enum SelectMenuOptions {
    String(Vec<SelectMenuOption<String>>),
}

It may need touching up, but in my mind this last option blends the benefits of the other two. A select menu can only have one type of option, so its variant is asserted upfront rather than matched upon in the option itself (causing us to not have an enum of different option value types in the option). It also doesn't duplicate types, leaving the type of value in the signature itself.

This is my first impression on where we should go with this, and what routes we've gone in similar situations. Let me know if you have any questions or thoughts, since I'm definitely open to debate on this.

Debug,
Deserialize,
Eq,
// Hash,
Copy link
Member

Choose a reason for hiding this comment

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

just leaving a pr comment on this (todo?) for visibility

);
assert_impl_all!(
MessageComponentInteractionData: Clone,
Debug,
Deserialize<'static>,
Eq,
Hash,
// Hash,
Copy link
Member

Choose a reason for hiding this comment

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

just leaving a pr comment on this (todo?) for visibility

@cptpiepmatz
Copy link
Contributor

Would it be reasonable to use generics and add markers for the select menu, something like the ids do?

@ImDarkDiamond ImDarkDiamond closed this by deleting the head repository Apr 24, 2023
@laralove143
Copy link
Member

Would it be reasonable to use generics and add markers for the select menu, something like the ids do?

Do you mean a method for extracting the target Ids from the components?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c-cache Affects the cache crate c-model Affects the model crate c-standby Affects the standby crate c-validate Affects the validate crate m-breaking change Breaks the public API. t-feature Addition of a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants