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.

Sign up for GitHub

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

Add ability to view previous topics with .topic command #1148

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Add ability to view previous topics with .topic command #1148

Changes from 3 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
ecc33e0
Add ability to view previous topics with .topic command
Anonymous4045 Nov 15, 2022
7c1f0f8
Fix rerolled message in channels with specific topics
Anonymous4045 Nov 16, 2022
6241f1e
Merge branch 'main' into previous_topics
Nov 25, 2022
823e8e7
Reformat conversationstarters.py
Dec 22, 2022
9ec251a
Optimize nested code in conversationstarters.py
Dec 22, 2022
fdc1894
Remove reroll reaction at max embed length
Dec 23, 2022
53ff19f
Remove unneeded comment
Feb 27, 2023
aee35d7
Merge branch 'main' into previous_topics
Feb 27, 2023
8bcd48b
Merge branch 'python-discord:main' into previous_topics
Anonymous4045 May 11, 2024
a149c6f
Change message format
Anonymous4045 May 12, 2024
5ff99a3
Handle forced topic reroll
Anonymous4045 May 12, 2024
d763ea9
Move form link to description
Anonymous4045 May 12, 2024
9c518be
Fix forced reroll
Anonymous4045 May 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 43 additions & 17 deletions bot/exts/utilities/conversationstarters.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ def __init__(self, bot: Bot):
self.bot = bot

@staticmethod
def _build_topic_embed(channel_id: int) -> discord.Embed:
def _build_topic_embed(channel_id: int, previous_topic: None | str) -> discord.Embed:
"""
Build an embed containing a conversation topic.

Expand All @@ -56,21 +56,45 @@ def _build_topic_embed(channel_id: int) -> discord.Embed:
color=discord.Colour.og_blurple()
)

try:
channel_topics = TOPICS[channel_id]
except KeyError:
# Channel doesn't have any topics.
embed.title = f"**{next(TOPICS['default'])}**"
if previous_topic is None:
# Message first sent
try:
channel_topics = TOPICS[channel_id]
except KeyError:
# Channel doesn't have any topics.
embed.title = f"**{next(TOPICS['default'])}**"
else:
embed.title = f"**{next(channel_topics)}**"
else:
embed.title = f"**{next(channel_topics)}**"
# Message is being edited
try:
channel_topics = TOPICS[channel_id]
except KeyError:
# Channel doesn't have any topics.
new_topic = f"**{next(TOPICS['default'])}**"
else:
new_topic = f"**{next(channel_topics)}**"
Anonymous4045 marked this conversation as resolved.
Show resolved Hide resolved

total_topics = previous_topic.count("\n") + 1

# Add 1 before first topic
if total_topics == 1:
previous_topic = f"1. {previous_topic}"

embed.title = previous_topic + f"\n{total_topics + 1}. {new_topic}"

# When the embed will be larger than the limit, use the previous embed instead
if len(embed.title) > 256:
embed.title = previous_topic
Copy link
Member

Choose a reason for hiding this comment

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

This logic means that clicking on the topic refresh button doesn't actually output a new topic, it just uses the previous input, if the title is beyond the limit.

I think a better way to deal with this would be when a user clicks on the emoji the new topic becomes number 1 and the rest get pushed down a number.
Then, when adding the topics if the last topic would make the title go beyond 256 characters, don't add it.

This would mean the newest topic is always the number 1 topic, and then the older ones are there a reference, until the button is pressed too many times.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Anonymous4045 Also resolve this, by implementing it.

Copy link
Author

Choose a reason for hiding this comment

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

@Ibrahim2750mi I thought we were going with preserving the order that the topics appeared in, so people could reference them by number and not have that number change

Copy link
Contributor

Choose a reason for hiding this comment

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

But that will not allow any new topics to be shown in the embed. Therefore as Chris said the new topics should be added on the number 1 position and oldest topic should be at number 5.

Copy link
Author

Choose a reason for hiding this comment

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

@Ibrahim2750mi I'm not sure I get what you mean. New topics are currently added to the end of the embed's title in a way in which their number doesn't change, so if it says something like "2. How did you start Python?" the number 2 won't change after a third topic is added. If that's not what you meant, could you explain a bit more?

Copy link
Contributor

Choose a reason for hiding this comment

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

If the embed's title exceed the character limit you directly use the previous_topic which means that it would not append any new topics, so it will stay stuck at those already present topics no matter how many times it is called, because your function just returns the previous_topic when it has reached the character limit. Eg: assume this has exceeded the character limit

1. How to do python
2. How to not do python
3. beep boop

Now even if your function is called it would just return the same embed that is shown in the codeblock, a better approach would be to keep the newest topic at top and the oldest at bottom like this

1. beep boop
2. How to not do python
3. How to do python

And now if your function is called it should omit How to do python so that a new topic can be added to it. Keep in mind you can omit multiple topics if the new topic still exceeds the character limit.

1. New cool topic
2. beep boop
3. How to not do python

Copy link
Author

Choose a reason for hiding this comment

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

That's certainly an option, but that would change the order of each topic, which could cause some confusion for people unaware of how it works or with responding to a particular prompt. At the moment, the reaction disappears once the limit is reached to dissuade people from trying to get too many prompts. The reason being that someone in the chat should have something to say about one of the four or so prompts.

I think we're also considering moving the topics from the embed title into the main body/description to not have so much bolded text and to extend the character limit. That way, we wouldn't really need to worry about the limit and can instead just stop at something like 5 prompts.

I don't want to make any changes until we decide which route to go.


return embed

@staticmethod
def _predicate(
command_invoker: Union[discord.User, discord.Member],
message: discord.Message,
reaction: discord.Reaction,
user: discord.User
command_invoker: Union[discord.User, discord.Member],
message: discord.Message,
reaction: discord.Reaction,
user: discord.User
Anonymous4045 marked this conversation as resolved.
Show resolved Hide resolved
) -> bool:
user_is_moderator = any(role.id in MODERATION_ROLES for role in getattr(user, "roles", []))
user_is_invoker = user.id == command_invoker.id
Expand All @@ -83,9 +107,9 @@ def _predicate(
return is_right_reaction

async def _listen_for_refresh(
self,
command_invoker: Union[discord.User, discord.Member],
message: discord.Message
self,
command_invoker: Union[discord.User, discord.Member],
message: discord.Message
Anonymous4045 marked this conversation as resolved.
Show resolved Hide resolved
) -> None:
await message.add_reaction("🔄")
while True:
Expand All @@ -101,23 +125,25 @@ async def _listen_for_refresh(
break

try:
await message.edit(embed=self._build_topic_embed(message.channel.id))
# The returned discord.Message object from discord.Message.edit is different from the current
# discord.Message object, so it must be reassigned to update properly
message = await message.edit(embed=self._build_topic_embed(message.channel.id, message.embeds[0].title))
except discord.NotFound:
break

with suppress(discord.NotFound):
await message.remove_reaction(reaction, user)

@commands.command()
@commands.cooldown(1, 60*2, commands.BucketType.channel)
@commands.cooldown(1, 60 * 2, commands.BucketType.channel)
Anonymous4045 marked this conversation as resolved.
Show resolved Hide resolved
@whitelist_override(channels=ALL_ALLOWED_CHANNELS)
async def topic(self, ctx: commands.Context) -> None:
"""
Responds with a random topic to start a conversation.

Allows the refresh of a topic by pressing an emoji.
"""
message = await ctx.send(embed=self._build_topic_embed(ctx.channel.id))
message = await ctx.send(embed=self._build_topic_embed(ctx.channel.id, None))
self.bot.loop.create_task(self._listen_for_refresh(ctx.author, message))


Expand Down