feat: delete-all button and language dropdown for multi-message code blocks#550
feat: delete-all button and language dropdown for multi-message code blocks#550Neil-Tomar wants to merge 5 commits into
Conversation
…ching Delete-all button removes every message of a block; ephemeral language dropdown re-colors every chunk. Only the bot's own messages are touched.
| * @return an action row containing the language dropdown | ||
| */ | ||
| public static ActionRow buildLanguageMenu(long requesterId, int total) { | ||
| return ActionRow.of(languageMenu(ComponentIdBuilder.build(COMPONENT_ID, requesterId, total))); |
There was a problem hiding this comment.
I noticed it's possible to do the following:
- User A runs the format code command
- User B runs the format code command
- User A deletes the code they formatted
- User A changes the language from their ephemeral message
In this case, the language of the code block of user B is changed. Could you prevent this in one of the following ways?
- In
LinkedMessages.resolveAfter, check that the last message contains the delete button with the delete button with the correct requestor (= current user) and that no other messages have a delete button - Change the ephemeral message to be sent after the formatted codeblocks, use
resolveBeforeand add an additional argument containing the message ID of the first code block message to the ID here. If this doesn't match, it is a failure.
I think the first approach is easier to do so I recommend that one.
| }, () -> Responses.error(event.getHook(), "Could not update the code block — it may have changed or no longer be yours.").queue()); | ||
| } | ||
|
|
||
| private static boolean ownsBlock(List<Message> messages, long userId) { |
There was a problem hiding this comment.
Could you please change this name to something more descriptive like isCodeBlockMessagesOwnedBy or isFormattedCodeMessagesOwnedBy?
| boolean lastIsUsers = buttonsOf(last).stream().anyMatch(button -> isDeleteButtonOf(button, userId)); | ||
| boolean noOtherHasButton = messages.size() <= 1 ; | ||
| for (Message message : messages.subList(0,messages.size()-1)){ | ||
| for(Button button: buttonsOf(message)){ |
There was a problem hiding this comment.
Formatted code block messages shouldn't have any componens except for the last message. I think the following would be easier:
for (int i = 1; i < messages.size(); i++) {
if (!message.getComponents().isEmpty()) {
return false;
}
}
return messages.size() >= 1 && hasDeleteButtonOf(messages.getFirst(), userId)| for (Message message : messages.subList(0,messages.size()-1)){ | ||
| for(Button button: buttonsOf(message)){ | ||
| if(isDeleteButton(button)){ | ||
| noOtherHasButton = true; |
There was a problem hiding this comment.
Here you are basically requiring that there is a delete button but you are excluding the chronologically first message from the list using subList which makes no sense.
| @@ -105,9 +110,46 @@ public void handleStringSelectMenu(@NonNull StringSelectInteractionEvent event, | |||
| event.deferEdit().queue(); | |||
|
|
|||
| LinkedMessages.resolveAfter(event.getMessage(), Integer.parseInt(id[2]), | |||
There was a problem hiding this comment.
Could you please document that resolveAfter and resolveBefore return the messages ordered from newest to oldest?
| return lastIsUsers && noOtherHasButton; | ||
| } | ||
|
|
||
| private static List<Button> buttonsOf(Message message) { |
There was a problem hiding this comment.
I don't think you'll need a buttonsOf method, you can just create a method like hasDeleteButtonOf that checks whether there is a delete button of a given user. Such a method could then just iterate over the components/buttons and checks for the delete button.
| }, () -> Responses.error(event.getHook(), "Could not update the code block — it may have changed or no longer be yours.").queue()); | ||
| } | ||
|
|
||
| private static boolean ownsBlock(List<Message> messages, long userId) { |
There was a problem hiding this comment.
If this method requires a certain order of the messages, please document this in a Javadoc.
| } | ||
|
|
||
| private static boolean isDeleteButtonOf(Button button, long userId) { | ||
| return isDeleteButton(button) |
There was a problem hiding this comment.
I think it would be easier to use ComponentIdBuilder.split and:
- verify that there are sufficiently many elements
- verify that the element at index
1equalsCOMPONENT_ID - verify that the element at index
1equalsString.valueOf(userId)
| messages.forEach(message -> | ||
| message.editMessage(withLanguage(message.getContentRaw(), language)).queue()); | ||
| } else { | ||
| Responses.error(event.getHook(),"Could not update the code block — it may have changed or no longer be yours.").queue(); |
There was a problem hiding this comment.
"no longer be yours" is a bit confusing in my opinion. What about "The code block could not be updated. The messages may have been deleted."
Also, I assume you are not able to delete it because the original message is ephemeral? Is this correct (this is perfectly fine)?
2 participants
What
Multi-message formatted code blocks now have two interactions, backed by a small message-linking helper:
How
Open questions