-
Notifications
You must be signed in to change notification settings - Fork 138
feat: components v2 #1294
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
base: master
Are you sure you want to change the base?
feat: components v2 #1294
Conversation
…t` for now to avoid shadowing
…omponent` more specific
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the idea behind suffixing the types with Input
?
I think it's a bit of a misnomer given only buttons/selects can be used for actual "input"
I'd say MessageComponentInput
=> MessageComponent
sounds better, wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could definitely rename it, it was mostly a decision made on a whim. c:
The [...]Input
part of the type name was meant to signify that it's moreso a MessageComponent
-like thing, ie. a single component, a list of action rows, a list of buttons and selects, etc etc, instead of only a single component like MessageComponent
might suggest(?)
The library internally transforms these shortcuts into the "list of action rows" structure the API would expect (for components v1, at least). But yea, I'll gladly rename these type aliases if we can come up with a more fitting name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think just having it plural, like components: MessageComponents
would be good enough
Alternatively, components: abc.Sequence[MessageComponent(s)]
to better drive the fact it's always a sequence (unless we want to support singular components?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(unless we want to support singular components?)
For now, stuff like .send(components=Button())
is supposed to work, yea.
MessageComponents
(and the corresponding ModalComponents
) seems good though, I've renamed those in a8fb923.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bnuuy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think just having it plural, like components: MessageComponents
would be good enough
Alternatively, components: abc.Sequence[MessageComponent(s)]
to better drive the fact it's always a sequence (unless we want to support singular components?)
file: Any | ||
n/a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should work for existing Attachment
s, since really this only needs a url
|
||
__slots__: Tuple[str, ...] = ("accessory", "components") | ||
|
||
__repr_info__: ClassVar[Tuple[str, ...]] = __slots__ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, Component
has .__repr_info__
, but .ui.UIComponent
has .__repr_attributes__
. Is this desirable?
(I suggest we use the same class variable name everywhere)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, good point 🤔
I'd say __repr_attributes__
sounds more accurate? *_info
could mean anything, really. Both app commands and non-ui components use the latter, though we'd want to rename those in a separate PR to keep the scope somewhat concise in this PR
|
||
def _walk_internal(component: Component, seen: Set[Component]) -> Iterator[Component]: | ||
if component in seen: | ||
# prevent infinite recursion if anyone manages to nest a component in itself |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What it actually does is skipping a component that has already been referenced.
That's not exactly the same as recursion, as a component could be reused between different containers/rows
It is an error if the component has a custom_id/id set, but for non-interactive components with omitted id it's technically fine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch, thanks! This should work a little better: 7bee2ce
components = [
ui.Container(
ui.TextDisplay("this is text inside a container"),
ui.MediaGallery(
disnake.MediaGalleryItem(
{
"media": {"url": "https://placecats.com/500/600"},
"description": "a cool media item",
},
),
disnake.MediaGalleryItem(
{
"media": {"url": "https://placecats.com/800/600"},
"description": "more kitty",
"spoiler": True,
},
),
),
ui.Separator(spacing=disnake.SeparatorSpacingSize.large),
ui.ActionRow(ui.ChannelSelect(placeholder="Choose a Channel!")),
),
]
await inter.response.send_message(components=components, flags=disnake.MessageFlags(is_components_v2=True)) disnake\ui\item.py", line 48, in ensure_ui_component
raise TypeError(f"{name} should be a valid UI component, got {type(obj).__name__}.")
TypeError: components should be a valid UI component, got ActionRow.
The above exception was the direct cause of the following exception:
Traceback (most recent call last):
File "disnake\ext\commands\interaction_bot_base.py", line 1465, in process_application_commands
await app_command.invoke(interaction)
File "disnake\ext\commands\slash_core.py", line 763, in invoke
raise CommandInvokeError(exc) from exc
disnake.ext.commands.errors.CommandInvokeError: Command raised an exception: TypeError: components should be a valid UI component, got ActionRow. There is an issue where can't add ActionRow to ContainerComponent
|
…ge|Modal)Components`
Co-authored-by: Eneg <[email protected]>
Should be fixed as of 69591bf. I knew about this issue from the start (and had removed the check locally in my workspace while testing all this time), since it would be remedied by eventually making |
class ActionRow(TypedDict): | ||
|
||
class _BaseComponent(TypedDict): | ||
# type: ComponentType # FIXME: current version of pyright complains about overriding types, latest might be fine |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that's because all TypedDict entries are mutable by default, which makes them invariant
Try type: ReadOnly[ComponentType]
on the base
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few insights;
I've noticed none of ui.{...}
components have __slots__
, whereas components.{...}
have them. I think it makes sense to add them to ui.
as well
""" | ||
|
||
small = 1 | ||
"""TODO""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need to give a lot of context here?
"""TODO""" | |
"""Small spacing.""" |
small = 1 | ||
"""TODO""" | ||
large = 2 | ||
"""TODO""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"""TODO""" | |
"""Large spacing.""" |
@@ -206,6 +217,7 @@ def __init__( | |||
) -> None: | |||
super().__init__(data) | |||
self.custom_id: str = data["custom_id"] | |||
self.id: int = data.get("id") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is always present:
self.id: int = data.get("id") | |
self.id: int = data["id"] |
def __repr__(self) -> str: | ||
attrs = " ".join(f"{key}={getattr(self, key)!r}" for key in self.__repr_attributes__) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could change this to always strip _
from keys and then be able to just put private attributes in __repr_attributes__
, wdyt?
Wanted to use str.removeprefix
but that's 3.9+
def __repr__(self) -> str: | |
attrs = " ".join(f"{key}={getattr(self, key)!r}" for key in self.__repr_attributes__) | |
def __repr__(self) -> str: | |
attrs = " ".join( | |
f"{key.lstrip('_')}={getattr(self, key)!r}" for key in self.__repr_attributes__ | |
) |
Could fix:
disnake/disnake/ui/action_row.py
Lines 198 to 201 in 7bee2ce
def __repr__(self) -> str: | |
# implemented separately for now, due to SequenceProxy repr | |
return f"<ActionRow children={self._children!r}>" | |
disnake/disnake/ui/container.py
Lines 84 to 87 in 7bee2ce
def __repr__(self) -> str: | |
# implemented separately for now, due to SequenceProxy repr | |
return f"<Container components={self._components!r} accent_colour={self.accent_colour!r} spoiler={self.spoiler!r}>" | |
Lines 61 to 64 in 7bee2ce
def __repr__(self) -> str: | |
# implemented separately for now, due to SequenceProxy repr | |
return f"<Section components={self._components!r} accessory={self._accessory!r}>" | |
# n.b. this should be `*components: UIComponentT`, but pyright does not like it | ||
def __init__(self, *components: Union[MessageUIComponent, ModalUIComponent]) -> None: | ||
self._children: List[UIComponentT] = [] | ||
# n.b. this should be `*components: ActionRowChildT`, but pyright does not like it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. I'm guessing this is because self: ActionRow[ActionRowModalComponent]
is narrower than self: Self[ActionRowChildT]
(the overloads do not consider subclasses); it's probably as good as it gets without HKT or separate constructors
def __init__( | ||
self, | ||
*components: TextDisplay, | ||
accessory: Union[Thumbnail, Button], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
accessory: Union[Thumbnail, Button], | |
accessory: Union[Thumbnail, Button[Any]], |
(need to import Any
)
# TODO: consider moving runtime checks from constructor into property setters, also making these fields writable | ||
@property | ||
def components(self) -> Sequence[TextDisplay]: | ||
"""Sequence[:class:`~.ui.TextDisplay`]: A read-only copy of the text items in this section.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"""Sequence[:class:`~.ui.TextDisplay`]: A read-only copy of the text items in this section.""" | |
"""Sequence[:class:`~.ui.TextDisplay`]: A read-only proxy of the text items in this section.""" |
def __len__(self) -> int: | ||
return len(self._children) | ||
|
||
@property | ||
def children(self) -> Sequence[UIComponentT]: | ||
def children(self) -> Sequence[ActionRowChildT]: | ||
"""Sequence[:class:`WrappedComponent`]: | ||
A read-only copy of the UI components stored in this action row. To add/remove |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A read-only copy of the UI components stored in this action row. To add/remove | |
A read-only proxy of the UI components stored in this action row. To add/remove |
# TODO: consider moving runtime checks from constructor into property setters, also making these fields writable | ||
@property | ||
def components(self) -> Sequence[ContainerChildUIComponent]: | ||
"""Sequence[Union[:class:`~.ui.ActionRow`, :class:`~.ui.Section`, :class:`~.ui.TextDisplay`, :class:`~.ui.MediaGallery`, :class:`~.ui.File`, :class:`~.ui.Separator`]]: A read-only copy of the components in this container.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"""Sequence[Union[:class:`~.ui.ActionRow`, :class:`~.ui.Section`, :class:`~.ui.TextDisplay`, :class:`~.ui.MediaGallery`, :class:`~.ui.File`, :class:`~.ui.Separator`]]: A read-only copy of the components in this container.""" | |
"""Sequence[Union[:class:`~.ui.ActionRow`, :class:`~.ui.Section`, :class:`~.ui.TextDisplay`, :class:`~.ui.MediaGallery`, :class:`~.ui.File`, :class:`~.ui.Separator`]]: A read-only proxy of the components in this container.""" |
We love lengthy type classifiers!
|
||
.. | ||
TODO: add cv2 components to list | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.. | |
TODO: add cv2 components to list | |
- :class:`Section` | |
- :class:`TextDisplay` | |
- :class:`Thumbnail` | |
- :class:`MediaGallery` | |
- :class:`FileComponent` | |
- :class:`Separator` | |
- :class:`Container` |
Maybe also change "Currently, the only components [...]" to "Currently, the components [...]", the list has expanded quite a bit (or even just "The components [...]")
Summary
First off, this isn't quite finished, as evidenced by the fact that this is marked as a draft PR and the numerous
TODO
s scattered across the code, but still very much usable in its current state (once the experiment lock gets removed for everyone).Most of the remaining changes are quality-of-life/DX improvements. I consider the interface fairly stable at this point, don't expect many further changes there.
It would probably make sense to wait a little longer with any reviews though, unless it's some glaring issue.
Description
discord/discord-api-docs@af1843d
This implements the new v2 components, including:
Section
: displays a thumbnail or button next to some textTextDisplay
: plaintext, but as a componentMediaGallery
: gallery/mosaic for up to 10 imagesFile
: non-image attachmentsSeparator
: vertical spacer/separatorContainer
: can be considered similar to anEmbed
These new components are all non-interactive and intended for layout only, they are quite a bit more flexible than the previous
content
/embeds
/attachments
/components
combo, and allow for much nicer looking/better structured messages.Scope
I feel like it's worth clarifying that this is about as straightforward as the interface could be, only building on the existing implementation/functionality for components. There may very well be better ways (in terms of DX) to implement these components (namely, builder pattern, yippee), but I've considered this out of scope from the start.
I've intentionally not implemented support for the new components in
View
s, given the intended scope of this PR and the complexities that somehow integrating them would bring, especially since all of the components are non-interactive.The goal with this PR was to get just about the most basic implementation going, any further work building on top of it will be in separate future PRs.
Example
code
(consider
MediaGalleryItem
unfinished, it is largely a placeholder implementation for now)Checklist
pdm lint
pdm pyright