Make bookmark a context menu command #1176

shtlrs · 2022-12-23T16:30:10Z

NOTE
This needs to wait for this PR before it can be merged, since it will be the one syncing the command tree.
Once that has been completed & merged, bot-core version needs to be bumped and then merged into main, then this branch will be updated

Closes #1089

This adds the support of bookmarking a message from a context menu, but preserves the old behavior as well.

To not lose the functionality of choosing a custom title, a modal has been added where a user is prompted for a title, which they can freely skip.

Workflow

As usual, a user needs to identify a message to bookmark
A user hovers on the message, then clicks on the ellipsis (three dots) -> Apps -> Bookmark
A user will be asked to choose a custom title for their bookmark, but they can freely ignore that step by clicking on Submit and the default title Bookmark will be chosen
Once submitted, the bookmark is sent as usual as a DM to the user, the only difference is that the bookmark embed in the invoked channel is now ephemeral

bot/exts/utilities/bookmark.py

Robin5605

Hello, thanks for the PR!
Just a few issues 🙂

bot/exts/utilities/bookmark.py

Robin5605 · 2023-01-13T22:25:05Z

bot/exts/utilities/bookmark.py

+        """Sends the bookmark embed to the user with the newly chosen title."""
+        title = self.bookmark_title.value or self.bookmark_title.default
+        await self.action_bookmark(interaction.channel, interaction.user, self.message, title)
+        view = SendBookmark(self.action_bookmark, interaction.user, interaction.channel, self.message, title)


Since this message is ephemeral, I don't think there's a need for the button that says "Click this to get your very own bookmark" or whatever it was. The whole point of the button was that multiple people wouldn't be running the .bookmark command in succession to the same message and flooding the channel - but with ephemeral messages, that's not an issue. So I think this button would just lead to more confusion and is unnecessary.

This one has also been resolved in cf49035

I just happened to make a new static method that builds an embed that's only used when the context menu command is invoked

bot/exts/utilities/bookmark.py

shtlrs

Alright, comments have been addressed and it's ready for review again

bot/exts/utilities/bookmark.py

shtlrs · 2023-01-14T09:38:48Z

bot/exts/utilities/bookmark.py

+        """Sends the bookmark embed to the user with the newly chosen title."""
+        title = self.bookmark_title.value or self.bookmark_title.default
+        await self.action_bookmark(interaction.channel, interaction.user, self.message, title)
+        view = SendBookmark(self.action_bookmark, interaction.user, interaction.channel, self.message, title)


This one has also been resolved in cf49035

I just happened to make a new static method that builds an embed that's only used when the context menu command is invoked

wookie184

Played around with the command and noticed a few things:

The command didn't seem to sync automatically, I had to run .int eval `await bot.tree.sync()` for it to appear.
The first time I use the command, this seems to stay there forever:

For bookmarks run after that it disappears normally, although after refreshing the client the same thing happens again.
Running the command in DMs with the bot gives a message saying you don't have permission to view that channel

It's pretty minor, and there's no point in allowing using the command in DMs, but the error message could be better.

Not sure if any of these are discord bugs rather than issues with the code, although worth looking into.

bot/exts/utilities/bookmark.py

shtlrs · 2023-01-15T11:28:05Z

The command didn't seem to sync automatically, I had to run .int eval `await bot.tree.sync()` for it to appear.

I think you might have missed the description, this relies on the newest release of pydis_core, where the tree is synced upon loading extensions. So that might be the reason.

The first time I use the command, this seems to stay there forever:

For bookmarks run after that it disappears normally, although after refreshing the client the same thing happens again.

This has also happened to me, but since it threw no errors, i thought it could be a discord bug or something.
I couldn't seem to figure out why since, like you said, it only happens the first time.

Running the command in DMs with the bot gives a message saying you don't have permission to view that channel

It's pretty minor, and there's no point in allowing using the command in DMs, but the error message could be better.

Well, we could check if the interaction channel is part of a guild or not, if not then it's a DM --> we change the message
But the thing is, we are completely overlooking the dm_permissions method here, which does include read_permissions.

So I think it's just better to agree on not allowing the usage of such command in DMs, and if that's the case, we send a message saying so.

wookie184 · 2023-01-15T11:45:13Z

I think you might have missed the description, this relies on the newest release of pydis_core, where the tree is synced upon loading extensions. So that might be the reason.

I noticed that but since the PR was merged I assumed poetry install would do fine. Is it just that the version used by this project needs to be bumped now?

Indeed, I'll send a PR to bump it.

wookie184 · 2023-01-17T19:57:16Z

Looks like there are some commits from other PRs that shouldn't be here

shtlrs · 2023-01-17T20:22:44Z

Looks like there are some commits from other PRs that shouldn't be here

I think those are the results of the rebase ?

wookie184

Also seems like the hanging command problem isn't there for me either any more👍

This removes the need of the `user_is_permitted_to_bookmark` func, and keeps the code cleaner & more coherent It always relis on using interaction.response to reply instead of context, which might result in a failed interaction

This bus was introduced upon refactoring the perms check, and it calls channel from within a channel.

This bus was introduced upon refactoring the perms check. It tries to access an author property from within a user, which doesn't exist.

This prompts the user for a custom title for their input. This has been added to not lose the functionality of adding a custom title for a bookmark for those who need it.

This will be sent along the ephemeral message

onerandomusername · 2023-02-19T19:02:28Z

bot/exts/utilities/bookmark.py

+    """
+
+    bookmark_title = discord.ui.TextInput(
+        label="Choose a title for you bookmark (optional)",


Suggested change

label="Choose a title for you bookmark (optional)",

label="Choose a title for your bookmark (optional)",

Fixed in 5d0ac27 (#1211)

shtlrs requested review from ChrisLovering and Robin5605 December 23, 2022 16:32

shtlrs commented Dec 23, 2022

View reviewed changes

bot/exts/utilities/bookmark.py Outdated Show resolved Hide resolved

Robin5605 requested changes Dec 24, 2022

View reviewed changes

bot/exts/utilities/bookmark.py Outdated Show resolved Hide resolved

bot/exts/utilities/bookmark.py Outdated Show resolved Hide resolved

bot/exts/utilities/bookmark.py Show resolved Hide resolved

shtlrs requested a review from Robin5605 December 27, 2022 10:27

shtlrs added status: needs review Author is waiting for someone to review and approve type: enhancement Changes or improvements to existing features labels Dec 27, 2022

Robin5605 requested changes Dec 28, 2022

View reviewed changes

bot/exts/utilities/bookmark.py Outdated Show resolved Hide resolved

bot/exts/utilities/bookmark.py Outdated Show resolved Hide resolved

shtlrs requested a review from Robin5605 December 28, 2022 08:11

shtlrs added the review: do not merge label Dec 28, 2022

shtlrs removed the review: do not merge label Jan 13, 2023

Robin5605 requested changes Jan 13, 2023

View reviewed changes

shtlrs commented Jan 14, 2023

View reviewed changes

shtlrs requested a review from Robin5605 January 14, 2023 09:42

wookie184 requested changes Jan 15, 2023

View reviewed changes

bot/exts/utilities/bookmark.py Outdated Show resolved Hide resolved

shtlrs changed the base branch from main to command-match-errors January 22, 2023 20:05

shtlrs requested review from Den4200, SebastiaanZ and ks129 as code owners January 22, 2023 20:05

shtlrs changed the base branch from command-match-errors to main January 22, 2023 20:05

wookie184 approved these changes Feb 5, 2023

View reviewed changes

Amrou Bellalouna added 5 commits February 19, 2023 14:42

sync commands upon cog load

a33806f

move running permission checks in a static method

0c10ceb

delegate building bm embed

7c711ec

add bm context menu

ff9085f

appease linter

701018a

Amrou Bellalouna added 14 commits February 19, 2023 14:42

rename run_permission_check to user_is_permitted_to_bookmark

409209b

make bookmark_context_menu_callback protected

cab71fb

keep style consistency when forming embeds

89973f8

fix AttributeError on interaction.channel

74e2ba8

This bus was introduced upon refactoring the perms check, and it calls channel from within a channel.

fix AttributeError on interaction.user

2794ed8

This bus was introduced upon refactoring the perms check. It tries to access an author property from within a user, which doesn't exist.

add a bookmark selection form

accd319

This prompts the user for a custom title for their input. This has been added to not lose the functionality of adding a custom title for a bookmark for those who need it.

return embed directly from build_bookmark_embed

cdb3ed5

move class var before ctor

09476fd

mark label as optional

a9bc849

build a new bookmark embed for the context menu command

d6183d7

This will be sent along the ephemeral message

add formatting to the ephemeral embed

05c1d9d

remove the call to action to check DMS for the bm

329514b

disallow usage of the context menu command in DMs

0dae917

shtlrs force-pushed the 1089-make-bookmark-a-context-menu-command branch from f262d72 to 0dae917 Compare February 19, 2023 13:44

ChrisLovering approved these changes Feb 19, 2023

View reviewed changes

ChrisLovering merged commit 310d199 into python-discord:main Feb 19, 2023

onerandomusername reviewed Feb 19, 2023

View reviewed changes

onerandomusername mentioned this pull request Feb 19, 2023

fix typo in user-facing bookmark message #1212

Merged

4 tasks

Xithrius removed the status: needs review Author is waiting for someone to review and approve label Mar 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make bookmark a context menu command #1176

Make bookmark a context menu command #1176

shtlrs commented Dec 23, 2022 •

edited

Loading

Robin5605 left a comment

Robin5605 Jan 13, 2023

shtlrs Jan 14, 2023

shtlrs left a comment

shtlrs Jan 14, 2023

wookie184 left a comment

shtlrs commented Jan 15, 2023 •

edited

Loading

wookie184 commented Jan 15, 2023 •

edited by shtlrs

Loading

wookie184 commented Jan 17, 2023

shtlrs commented Jan 17, 2023

wookie184 left a comment

onerandomusername Feb 19, 2023

ChrisLovering Feb 19, 2023

	label="Choose a title for you bookmark (optional)",
	label="Choose a title for your bookmark (optional)",

Make bookmark a context menu command #1176

Make bookmark a context menu command #1176

Conversation

shtlrs commented Dec 23, 2022 • edited Loading

Workflow

Robin5605 left a comment

Choose a reason for hiding this comment

Robin5605 Jan 13, 2023

Choose a reason for hiding this comment

shtlrs Jan 14, 2023

Choose a reason for hiding this comment

shtlrs left a comment

Choose a reason for hiding this comment

shtlrs Jan 14, 2023

Choose a reason for hiding this comment

wookie184 left a comment

Choose a reason for hiding this comment

shtlrs commented Jan 15, 2023 • edited Loading

wookie184 commented Jan 15, 2023 • edited by shtlrs Loading

wookie184 commented Jan 17, 2023

shtlrs commented Jan 17, 2023

wookie184 left a comment

Choose a reason for hiding this comment

onerandomusername Feb 19, 2023

Choose a reason for hiding this comment

ChrisLovering Feb 19, 2023

Choose a reason for hiding this comment

shtlrs commented Dec 23, 2022 •

edited

Loading

shtlrs commented Jan 15, 2023 •

edited

Loading

wookie184 commented Jan 15, 2023 •

edited by shtlrs

Loading