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: context menu for !raw #2773

Closed

Conversation

VirdanTheBurden
Copy link
Member

Closes Issue #2760.

The !raw command has been restricted to only prefix command usage since inception, and has not been touched since. With the introduction of new interfaces for executing commands (i.e. application context menus) and the inconvenience of copying message links to check their raw content, this pull request hopes to simplify the process by adding a context menu button for users to click.

@VirdanTheBurden VirdanTheBurden added a: utility Related to utility commands: (bot, eval, extensions, jams, reminders, snekbox, utils) t: feature New feature or request s: WIP Work In Progress p: 2 - normal Normal Priority labels Oct 1, 2023
@@ -480,19 +485,29 @@ def format_fields(self, mapping: Mapping[str, Any], field_width: int | None = No
# remove trailing whitespace
return out.rstrip()

async def send_raw_content(self, ctx: Context, message: Message, json: bool = False) -> None:
async def send_raw_content(self, ctx: Context | Interaction, message: Message, json: bool = False) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this function should be simplified into just a function that accepts a message, and returns a str or list[str] (to accommodate for pagination). Accepting both a Context and Interaction feels a bit janky in my opinion.

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't feel quite the same way at first, but due to other reasons, this route of implementation is frankly garbage. I'll be moving the message sending responsibilities to the... overlying(?) methods.

Copy link
Contributor

@Robin5605 Robin5605 Oct 1, 2023

Choose a reason for hiding this comment

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

I'll be moving the message sending responsibilities to the... overlying(?) methods.

That sounds like a good idea, now that we have two different methods that send the message in two different ways (normal !raw command and context-menu command. Former needs to use ctx.send while the latter needs to use interaction.response.send_message). It would make sense to practice principle of least responsibility here and have one function that's just responsible for fetching the raw content.

Copy link
Member Author

@VirdanTheBurden VirdanTheBurden Oct 2, 2023

Choose a reason for hiding this comment

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

I've pushed some new changes that made send_raw_content only fetch and return a Paginator object instead. Feel free to take a look at the new commits while I go fix the tests. I think they're borked with the ContextMenu being there in the __init__.

Copy link
Member Author

Choose a reason for hiding this comment

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

Er, take that back. Tests passed with flying colors, haha!

bot/exts/info/information.py Outdated Show resolved Hide resolved
After Robin's suggestion, I've realized making
send_raw_content manage building the paginator
and message sending responsibilties is silly.

Thus, send_raw_content is now only responsible
for constructing the response message. To reflect
this change, the signature for send_raw_content
is now get_raw_content, and no longer requires
a discord.ext.commands.Context object to work.

The only other change needed was moving a
permission check to the !raw command coroutine.
It cannot exist inside get_raw_content without
the context of the author who sent the !raw
command.
Added the context menu object to the command tree
and finished the callback. The only thing iffy
about it is this: due to the nature of
interactions, only one message can be sent as a
response to an interaction, with followup
webhooks needed for further response.
`paginator.pages` is at LEAST 2 elements long,
meaning I can only send either element 0
(the "header") or element 1, which is the ACTUAL
raw content. I opted to only send element 1.
I did try followups in order to send both, but I
felt the result was janky, especially since I
implemented the response to be ephemeral.

Also imported Interaction object in the `from
discord import ...` for easier typehinting.
@VirdanTheBurden VirdanTheBurden added s: needs review Author is waiting for someone to review and approve and removed s: WIP Work In Progress labels Oct 2, 2023
async def _raw_context_menu_callback(self, interaction: Interaction, message: Message) -> None:
"""The callback to be ran when the Raw Text context menu button is used."""
paginator = await self.get_raw_content(message)
await interaction.response.send_message(paginator.pages[1], ephemeral=True)
Copy link
Contributor

@Robin5605 Robin5605 Oct 3, 2023

Choose a reason for hiding this comment

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

Perhaps we could trial out some button-based pagination here? It's a shame that users can only see one page with the context menu version. I think one of the largest (pun intended) issues people had with buttons for pagination is their large vertical size, though it may be less of an issue in ephemeral messages.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, okay. I'll try adding a view with some buttons to see if we can scroll through.

Copy link
Contributor

Choose a reason for hiding this comment

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

An alternative would be to do what was suggested in #1037 and just send a pastebin link if the message is too long, maybe in a similar way to the eval command where it displays as much as it can first.

@wookie184 wookie184 added s: waiting for author Waiting for author to address a review or respond to a comment and removed s: needs review Author is waiting for someone to review and approve labels Jan 1, 2024
@VirdanTheBurden VirdanTheBurden deleted the context-menu branch February 13, 2024 00:06
@hedyhli hedyhli linked an issue Mar 16, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: utility Related to utility commands: (bot, eval, extensions, jams, reminders, snekbox, utils) p: 2 - normal Normal Priority s: waiting for author Waiting for author to address a review or respond to a comment t: feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Turn !raw into a context menu command
3 participants