Kaktushose / jda-commands

A declarative, annotation driven command library for JDA
https://github.com/Kaktushose/jda-commands/wiki
Apache License 2.0
67 stars 11 forks source link

[QUESTION] How to handle expired buttons? #119

Closed Kaktushose closed 6 months ago

Kaktushose commented 1 year ago

response to expired buttons? delete message? auto remove buttons? send explaining message? Should buttons even expire?

Kaktushose commented 1 year ago

@stijnb1234 @aless2003 @lus (and all the other contributers) any thoughts on this one?

lus commented 1 year ago

I'd say specifying some kind of callback which gets executed on expiration should be possible to just handle this individually.

Kaktushose commented 1 year ago

The question is how we integrate this callback into the interaction class

Example Button Code

@Interaction
public class ButtonCommand {

    @SlashCommand
    public void onCommand(CommandEvent event) {
        event.withButtons("onButton").reply("Hello");
    }

    @Button("Click me!")
    public void onButton(CommandEvent event) {
        event.reply("You've clicked me");
    }
}

I think that a generic interface like ButtonExpirationHandler would be a bad solution since we want high friction between expiration handling and button defining.

Another question is if buttons always need to expire. But on this one I'm not sure what discords limitations are.

Also I'd like to avoid things like @Button(expiration = 10, unit = TimeUnit.MINUTES, strategy=ExpirationStrategy.DELETE)

aless2003 commented 1 year ago

I think the best option is to auto-remove the buttons after they expire, unless the Programmer specifies otherwise. It would keep the Programming experience quite easy. Additionally, it would be less confusing to a User as when there's no Button, you don't have to wonder as to why something doesn't work.

As for the part if it should expire, I would keep whatever the default behavior is for those in JDA and let the Programmer change it as needed. I could Imagine something like

@Interaction
public class MyCommand {
    //Commands...

    @Button("Click here")
    @Expiering(handler=MyHandler.class, time = 10, unit=TimeUnit.Seconds)
    public void onClickButton(CommandEvent event) {
        event.reply("Hey you clicked!");
    }
}

HyHandler.class

public class MyHandler implements ExpirationHandler {
    @Override
    public void handle(CommandEvent event) {
         event.reply("Your time has run out");
         event.getMessage().delete().queue();
    }
}

Not sure if CommandEvent is the best argument for the handle method though. We could also combine @Expiring with @Button, depending on whether other elements can expire. I haven’t looked too much into Interactions yet, so let me know what you think.

stijnb1234 commented 1 year ago

I agree with @aless2003, I think that's the best way to implement this.

lus commented 1 year ago

I see a problem in this proposed solution. We create one instance of the interaction class per invocation, making it very straightforward to store the state of this particular invocation as local class variables. If we create a new instance of an ExpirationHandler whenever the button/interaction has expired, we would lose this state in that class.

aless2003 commented 1 year ago

Couldn't we create just one instance of each ExpirationHandler needed in a particular class when checking the Annotations and just reuse this instance over and over again?

Kaktushose commented 1 year ago

Okay I've read some docs and from my understanding buttons cannot expire. Each button click creates a new interaction event, what expires is the ReplyHook after 15mins. As I am thinking about this right now, this would mean that we only have 15mins to remove a button? Because we can only edit the message (and its components) for 15mins. However we can always remove the button listener in which case #120 will handle the situation.

Still we should provide an option to remove buttons after a certain amount of time. I think @aless2003 proposal is a good one. However I'd like to modifiy it a little bit. Instead of creating a new class, we can just define a method inside the interaction class.

@Interaction
public class MyCommand {
    //Commands...

    @Button("Click here")
    @Expiration(handler = "onHandleExpiration", time = 10, unit=TimeUnit.SECONDS)
    public void onClickButton(CommandEvent event) {
        event.reply("Hey you clicked!");
    }

    @ExpirationHandler
    public void onHandleExpiration(ButtonExpirationEvent event) {
        event.removeButton();
    }
}

This, however adds overhead so maybe my idea of introducing different expiring strategies actually wasn't the worst. I could think about the following strategies:

DELETE: deletes the original message CLEAR: removes the component CLEAR_ALL: removes all components KEEP: keeps the component but unregisters its listener. This will also be the fallback strategy after 15 minutes have passed.

Kaktushose commented 1 year ago

turns out that this is a rather complex problem. I've moved the current state of implementation to another branch and will focus on different issues for now

Kaktushose commented 6 months ago

I've looked back at this issue and it's inherently difficult to find a good solution. Components are bound to the InteractionRuntime. If we close the runtime, no component can be executed anymore. That's where things get complicated if more than one component is bound to said runtime. I'll close this issue for now and might reopen it if anyone is explicitly requesting this feature