Add ability to view previous topics with .topic command #1148
Conversation
After a user invokes the ".topic" command and reacts with the reroll emoji, the embed will now show the previous few messages. It cannot display more than 256 characters, which is usually around 5 prompts. Implements: python-discord#1145
|
I'll keep this open, but be sure next time to get your issue approved by a core dev before continuing with a PR. |
Xithrius
added
area: backend
|
Could you share a screenshot of what this looks like after being used a few times? I think we will also need to add a limit to the number of previous topics that get shown, if there isn't already present. Users could easily spam the button and cause a very large embed from the bot otherwise. |
|
Discord sets the limit on the number of characters in the title of an embed to be 256. From my testing, that seems to mean there's around 4 or 5 topics that can be on display at once. Personally I think 4 or 5 is enough for people to have something to talk about, so it seems good to me. Here's some screenshots of my bot running the code
|
| # When the embed will be larger than the limit, use the previous embed instead | ||
| if len(embed.title) > 256: | ||
| embed.title = previous_topic |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@Anonymous4045 Also resolve this, by implementing it.
There was a problem hiding this comment.
@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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
Revert the extra whitespaces added by my formatter to meet the style guide.
I considered this,but this would change the ordering of the older messages. One point of adding this feature is so that people wanting to respond to a specific topic could include the number of that topic in with their reply, e.g. someone responding to the second conversation starter could say something like "For topic 2, ...". Changing the numbers could be confusing or change the sentence someone was referring to. |
This commit will remove some duplicate and unnecessary code, as suggested by @ChrisLovering.
If we're going to go with that behaviour, we should remove the reaction button when it will no longer send a new topic. |
Once the number of characters in the embed's title exceeds the limit of 256, the bot will remove the reroll emoji. Suggested (here)[python-discord#1148 (comment)]
|
This seems good to me, the only thing I would suggest is limiting the number of prompts to 3, as 5 seems like quite a lot (i'd still leave the detection for when it goes over the title limit, just in case there are 3 really long topics). Let's see what others think first though, as I don't mind too much and some people may prefer more. |
|
Could you move the previous topics as part of the description instead? I feel like it might be too distracting to be in all bold and big like that |
Once the number of characters in the embed's title exceeds the limit of 256, the bot will remove the reroll emoji. Suggested (here)[python-discord#1148 (comment)]
We could do that, but there's a few things to work out first. What would the title in bold be? And what would we set the limit for the topics to show as, since we wouldn't be bound by the title character limit? |
|
Hey @Anonymous4045, are you still planning to work on this? I agree with wookie that allowing five topics simultaneously is a little too much, perhaps 3 could do. If a topic is approved in the repo, they should already be considered plausible topics. Keep in mind that people also like to reference the topic embed when replying to the topic. Having too many topics to choose from in that embed can lead to fragmented conversations, which one might say defeats part of the purpose of this Which also means, regarding Chris' suggestion to pop the first topic off and append new ones to the bottom: This addresses both Chris' concern (with reaction not actually getting a new topic) and yours (people responding to Topic N).
Since titles are not mandatory, moving the topic from the title to the description could look better once the reroll emoji is reacted IMO. So all that this PR needs now as far as I can tell is to 1) improve the UI (bold text) and 2) implement a reasonable behaviour to limit the number of topic rerolls allowed. What do you think? Footnotes
|
|
Sounds good to me. I can start working on this and see how it feels. |
|
@Anonymous4045 Hello! Any updates on the progress of this PR? Thanks! |
This commit changes the message to have a set title, then up to 3 topics, followed by a footer (currently link is broken).
This commit handles when a user presses the reroll emoji after it's already removed by the bot. It just returns what the embed was before and re-clears the reactions.
|
@Xithrius thanks for the reminder! I've implemented most of what you said. I also tried switching the suggestion form to be a footer, but this seems to break the hyperlink. Do you know of a way to have links in the footer? We could also just make it in the description, but I think it looks better as a footer for formatting purposes. |
|
Thank you for the update! Footers in embeds do not support hyperlinks, unfortunately. I believe previously the link was just a part of the description. |
Because embed footers can't have markup, the topic suggestion hyperlink can't be in it. This commit moves it from the footer to the description of the embed.
|
@Xithrius bummer. How's this instead?
|
|
Looks good to me! I'll test out this PR on my local machine in the coming days. |
Relevant Issues
Closes #1145
Description
After a user invokes the ".topic" command and reacts with the reroll emoji, the embed will now show the previous few conversation prompts. It cannot display more than 256 characters because of the limit on embed title character length, which is usually around 5 prompts.
Did you: