search

Rapptz / discord-ext-menus

MIT License
234 stars 87 forks source link

Add auto_remove_reactions option to Menu #36

Closed CircuitSacul closed 3 years ago

CircuitSacul commented 3 years ago

Essentially, this allows the Menu to function the way 99% of other paginators work, where the user reaction to one of the buttons, and the bot automatically removes it, instead of listening for both the add and remove events.

It's simply another option, that when enabled, means the bot will only listen to the add event and will try to remove the reaction. I intentionally passed the discord.Forbidden error and didn't add to the menu permission checks, because if the bot doesn't have permission to remove reactions I want it to keep working and not demand those perms.

Also, thanks so much for making this -- it makes working with reaction menus way more than a "little" easier :)

Gobot1234 commented 3 years ago

Similar pull requests to this have been denied see https://github.com/Rapptz/discord-ext-menus/pull/19#issuecomment-698574075 and https://github.com/Rapptz/discord-ext-menus/pull/35#issuecomment-806237803 for the rationale behind not supporting it

CircuitSacul commented 3 years ago

If I'm understanding this correct, the concern is that when the bot removes the reaction the raw_reaction_remove is called and causes the paginator to skip. With the way I implemented this, the bot only listens to the raw_reaction_add event if this setting is enabled, therefore preventing any risk.

Right now, the only way to do this would be to completely overwrite the _internal_loop function in my menu, which is a rather long function and something I would prefer not to do.

CircuitSacul commented 3 years ago

Also on the second comment, I see why it might be considered bad design, but I want to have this option. It defaults to False anyways.

SebbyLaw commented 3 years ago

Since you didn't bother to read the comment at all I've copied it here for you

  1. It doesn't work in DMs, since you can only remove your own reaction and not the other person. So they have to click twice anyway in that case.
  2. It doesn't work if you don't have permissions. In this case you have to click twice again.
  3. If you remove it then it causes an extra HTTP request for the removal if it even works. This route is heavily rate limited so having extra requests for this is undesirable.

Ultimately the current code offers a consistent UX -- 1 click or tap and the button works. With the PR then this stuff will always take two clicks (bad UX) and if there's auto removal in the user code somewhere then there's going to be an extra HTTP request that might work or not leading it to require 2 clicks for the user anyway.

I'm just not a fan and would rather not offer library users a footgun like this.

CircuitSacul commented 3 years ago

Since you didn't bother to read the comment at all I've copied it here for you

  1. It doesn't work in DMs, since you can only remove your own reaction and not the other person. So they have to click twice anyway in that case.
  2. It doesn't work if you don't have permissions. In this case you have to click twice again.
  3. If you remove it then it causes an extra HTTP request for the removal if it even works. This route is heavily rate limited so having extra requests for this is undesirable.

Ultimately the current code offers a consistent UX -- 1 click or tap and the button works. With the PR then this stuff will always take two clicks (bad UX) and if there's auto removal in the user code somewhere then there's going to be an extra HTTP request that might work or not leading it to require 2 clicks for the user anyway.

I'm just not a fan and would rather not offer library users a footgun like this.

Actually I did read it, and I responded