Experimental - Cancel-able Menus #69
Conversation
kantenkugel
added
Enhancement
|
Should probably also wait for #58 to be merged so i can apply the changes from this PR to that code as well. Adding it to the TODO |
kantenkugel
changed the title
There was a problem hiding this comment.
Here are some initial things I have questions about that we can start with.
I am rather hesitant about this proposal, I will need to see how it evolves as we progress through the todo list.
My main concerns as of right now are compatibility, this PR introduces some pretty big breaking changes as it is. I am not opposed to breaking things, but we should try to avoid any if possible.
| private Future<Void> cancelFuture; | ||
| private Message attachedMessage; | ||
|
|
||
| protected Menu(EventWaiter waiter, Set<User> users, Set<Role> roles, long timeout, TimeUnit unit, Consumer<Message> finalAction) |
There was a problem hiding this comment.
Unfortunately, this breaks compatibility with previous versions for people who implemented their own menus.
There was a problem hiding this comment.
Will add an overloaded Constructor without finalAction that just calls this one with null finalAction (for old menus, not using the new methods)
| * @param cancelFuture | ||
| * The cancel Future returned from {@code EventWaiter#waitForEvent} | ||
| */ | ||
| protected final void setCancelFuture(Future<Void> cancelFuture) |
There was a problem hiding this comment.
I am concerned about the implied functionality through these two methods.
What if my menu doesn't need cancellation functionality? Should I still use them? Is it safe for my menu to not have cancel functionality?
This needs to be documented, and if it is required I'd appreciate it mentioned in the class level documentation for Menu.
There was a problem hiding this comment.
I added a big block in the class-level docs of Menu describing the whole system and its use
| public void cancel() | ||
| { | ||
| if(cancelFuture == null) | ||
| throw new IllegalStateException("Menu was not yet displayed or is already cancelled"); |
There was a problem hiding this comment.
So if I don't set the cancelFuture, then the menu wasn't displayed or was already cancelled...?
I think more appropriately this should be "Menu didn't have cancelFuture set to cancel" or something to that effect.
There was a problem hiding this comment.
Probably better to change the message to something like "Menu can not be cancelled (not supported, not yet displayed or already ended)"
Pull Request
Pull Request Checklist
Please follow the following steps before opening this PR.
PRs that do not complete the checklist will be subject to denial for
missing information.
or merged features/bug fixes.
Pull Request Information
Check and fill in the blanks for all that apply:
menuandcommonsmodules of the JDA-Utilities library.______.Description
This module adds the possibility to cancel
EventWaitertasks via the new return value ofEventWaiter#waitForEvent(Future<Void>)This new feature is used to add
Menu#cancel()to cancel menus that are already running.Todo:
cancelFuture.cancel(true)vsfalsedisplay()if already being displayed somewhere, as this would currently prevent proper functionality ofMenu#cancel()