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 a native timeout for modal.py #1434

Merged
merged 4 commits into from Jul 5, 2022
Merged

Add a native timeout for modal.py #1434

merged 4 commits into from Jul 5, 2022

Conversation

nexy7574
Copy link
Contributor

@nexy7574 nexy7574 commented Jun 21, 2022

Summary

Modals do not currently have a native timeout (like Views do), meaning they never leave the modal store unless the form is submitted (resulting in them being in there forever when dismissed). This PR copies and slightly modifies the same behaviour and functions from discord.ui.View, and reflects it into discord.ui.Modal.

Checklist

  • If code changes were made then they have been tested.
    • I have updated the documentation to reflect the changes.
  • If type: ignore comments were used, a comment is also left explaining why
  • This PR fixes an issue.
  • This PR adds something new (e.g. new method or parameters).
  • This PR is a breaking change (e.g. methods or parameters removed/renamed)
  • This PR is not a code change (e.g. documentation, README, typehinting, examples, ...)

@nexy7574
Copy link
Contributor Author

I seem to be getting a RuntimeWarning when the timeout task exists:

c:\users\nexus\pycharmprojects\pycord\discord\ui\modal.py:103: RuntimeWarning: coroutine 'TestModal.on_timeout' was never awaited
  self.loop.create_task(self.on_timeout(), name=f"discord-ui-view-timeout-{self.id}")
RuntimeWarning: Enable tracemalloc to get the object allocation traceback

I don't get this with the Views, which is odd, considering I've basically copy-pasted the code. I'm gonna look into this, fix it, then undraft the PR.

@Mihitoko
Copy link
Contributor

Mihitoko commented Jun 21, 2022

I cant fork your fork since i already have a fork of pycord. I cant write to your fork unfortunaly.

change line 49 to

self.timeout: float = timeout

and line 103 to:

self.loop.create_task(self.on_timeout(), name=f"discord-ui-view-timeout-{self._title}")

This should do the trick

@Mihitoko
Copy link
Contributor

We should also make sure to call Modal.stop() on remove

@nexy7574
Copy link
Contributor Author

I’ll make the changes when I get home tomorrow 👍

@nexy7574
Copy link
Contributor Author

Okay so with ecd8d60, the errors are no-longer present.
However, for some reason, as soon as I init my test model, the timeout immediately starts. Looking at interaction.response.send_modal, it looks like it should only be added to the modal store when the modal is sent, however it seems to be starting off the timeout as soon as init() is called. I'm gonna dig through the rest of the code to see what's going on, but I must admit, the timeout system is rather funky to look at lol

@nexy7574
Copy link
Contributor Author

I retract my previous comment, I think I'm just being a little bit dense, I'm gonna try with some different timeouts. If I am just being dense then this PR is ready to go.

@nexy7574
Copy link
Contributor Author

Can confirm that this now works and has been tested. No more errors in console, and debugging seems to match up with expected results. I'm gonna open this PR for review now :)

@nexy7574 nexy7574 marked this pull request as ready for review June 24, 2022 12:42
@Mihitoko
Copy link
Contributor

👍

@EmreTech
Copy link
Contributor

EmreTech commented Jul 4, 2022

I cant fork your fork since i already have a fork of pycord. I cant write to your fork unfortunaly.

change line 49 to

self.timeout: float = timeout

and line 103 to:

self.loop.create_task(self.on_timeout(), name=f"discord-ui-view-timeout-{self._title}")

This should do the trick

FYI, you could've just reviewed this PR and included suggestions.

Copy link
Contributor

@EmreTech EmreTech left a comment

Choose a reason for hiding this comment

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

Haven't tested, but the code portions look good

@Dorukyum Dorukyum merged commit ea230a1 into Pycord-Development:master Jul 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants