Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ConfirmView utility #6176

Merged
merged 18 commits into from Jun 21, 2023
Merged

Conversation

TrustyJAID
Copy link
Member

closes #6174

Description of the changes

Have the changes in this PR been tested?

Yes

@github-actions github-actions bot added the Category: Core - API - Utils Package This is related to stuff in `redbot.core.utils` label May 25, 2023
Copy link
Member

@flaree flaree left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be worth adding an on_timeout that sets the buttons to disabled?

@TrustyJAID
Copy link
Member Author

I am open to customizing the timeout but I will not be adding support for disabling the buttons automatically. That would require sending the message through the view which I feel takes away from the simplicity of this. The buttons when interacted with will remove the buttons from the message but timeout will do nothing and it should be up to the user to handle that I think.

@flaree
Copy link
Member

flaree commented May 25, 2023

Huh, I have a confirm that doesn't send through a view and works fine.

async def on_timeout(self):
        for item in self.children:
            item.disabled = True

@TrustyJAID
Copy link
Member Author

That would disable the button and prevent any response but would still require editing the original message which this does not have any reference to.

…w was interacted with or not allowing for easier cleanup.

Add examples of cleanup including how to disable the buttons if they're not pressed.
@TrustyJAID
Copy link
Member Author

I have reworked this slightly so we can know if the buttons were not pressed so users can clean them up however they wish, whether that be disabling the buttons or removing them after the timeout.

@Jackenmen
Copy link
Member

It shouldn't be required (i.e. we should only try doing it if not None) but I think that it would be simple enough to allow setting view.message attribute after the message is sent and remove the view from the message on_timeout in such a case. I would also suggest updating the relevant example to use that, i.e.

view = ConfirmView(ctx.author)
view.message = await ctx.send("Are you sure you about that?", view=view)
await view.wait()
if view.result:
    await ctx.send("Okay I will do that.")
else:
    await ctx.send("I will not be doing that then.")

@Jackenmen Jackenmen added the Type: Feature New feature or request. label Jun 20, 2023
@Jackenmen Jackenmen added this to the 3.5.4 milestone Jun 20, 2023
…ead of removing them

Move all logic for removal/disabling into `on_timeout` and update examples about setting a `view.message` attribute when sending the view as per Jack's suggestion.
redbot/core/utils/views.py Outdated Show resolved Hide resolved
redbot/core/utils/views.py Outdated Show resolved Hide resolved
redbot/core/utils/views.py Outdated Show resolved Hide resolved
redbot/core/utils/views.py Outdated Show resolved Hide resolved
redbot/core/utils/views.py Outdated Show resolved Hide resolved
redbot/core/utils/views.py Outdated Show resolved Hide resolved
redbot/core/utils/views.py Outdated Show resolved Hide resolved
redbot/core/utils/views.py Outdated Show resolved Hide resolved
redbot/core/utils/views.py Outdated Show resolved Hide resolved
@Jackenmen Jackenmen self-assigned this Jun 20, 2023
remove disable() method
Actually respond to the interaction and call the `on_timeout` method if a button is pressed and we have stopped the view
@Jackenmen Jackenmen modified the milestones: 3.5.4, 3.5.3 Jun 20, 2023
Rename the button methods to more accurately describe their intended use `confirm_button` and `dismiss_button`.
Add docstrings explaining how one might customize the style of the `confirm/dismiss` buttons
Copy link
Member

@Jackenmen Jackenmen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's mostly a bunch of docstring fixes left but I also commented with one implementation question.

redbot/core/utils/views.py Show resolved Hide resolved
redbot/core/utils/views.py Outdated Show resolved Hide resolved
redbot/core/utils/views.py Outdated Show resolved Hide resolved
redbot/core/utils/views.py Outdated Show resolved Hide resolved
redbot/core/utils/views.py Outdated Show resolved Hide resolved
redbot/core/utils/views.py Outdated Show resolved Hide resolved
redbot/core/utils/views.py Outdated Show resolved Hide resolved
redbot/core/utils/views.py Outdated Show resolved Hide resolved
redbot/core/utils/views.py Outdated Show resolved Hide resolved
Copy link
Member

@Jackenmen Jackenmen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This new utility works as expected so from my side, this is good to go.

I would, however, like you to look at the rendered documentation and assess whether the current version of it is acceptable:
https://red-discordbot--6176.org.readthedocs.build/en/6176/framework_utils.html#redbot.core.utils.views.ConfirmView

Basically, I looked at the rendered documentation and thought that showing the documentation from the base class on our overridden version of the methods was confusing and have instead opted to document it with the actual behavior of them. Additionally, I documented confirm/dismiss_button as a regular attribute of the Button type since that's what it actually ends up being on the instance of this view.

If everything seems fine to you, feel free to just merge this.

@TrustyJAID TrustyJAID merged commit 7dff136 into Cog-Creators:V3/develop Jun 21, 2023
17 checks passed
@red-githubbot red-githubbot bot added the Changelog Entry: Pending Changelog entry for this PR hasn't been added by repo maintainers yet. label Jun 21, 2023
@Jackenmen Jackenmen added Changelog Entry: Added Changelog entry for this PR has already been added to changelog PR. and removed Changelog Entry: Pending Changelog entry for this PR hasn't been added by repo maintainers yet. labels Jul 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Core - API - Utils Package This is related to stuff in `redbot.core.utils` Changelog Entry: Added Changelog entry for this PR has already been added to changelog PR. Type: Feature New feature or request.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

A ConfirmView similar to SimpleView
3 participants