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

feat(View): add prevent_update param #886

Merged
merged 4 commits into from Jun 17, 2023
Merged

Conversation

TrapDrap
Copy link
Contributor

@TrapDrap TrapDrap commented Nov 5, 2022

Summary

Adding prevent_update to View. Option preventing storing permanent views for separate messages by default.
(since there's no limit to how many different buttons get stored, and if not stopped properly can cause issues over time when running a bot for long periods without reboot, and users are not notified of these being stored)

Checklist

  • If code changes were made then they have been tested.
    • I have updated the documentation to reflect the changes.
    • I have run task pyright and fixed the relevant issues.
  • 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, ...)

@EmreTech EmreTech changed the title Adding auto_stop to View. Option to prevent storing permanent views for separate messages. feat(View): add auto_stop param Nov 5, 2022
@EmreTech
Copy link
Collaborator

EmreTech commented Nov 5, 2022

If the auto_stop parameter limits storing persistent views for specific messages, force_global or just global would be a better name.

@EmreTech
Copy link
Collaborator

EmreTech commented Nov 5, 2022

Also, to avoid breaking changes, the default for auto_stop or whatever the parameter will end up being named should be False.

@TrapDrap
Copy link
Contributor Author

TrapDrap commented Nov 5, 2022

If the auto_stop parameter limits storing persistent views for specific messages, force_global or just global would be a better name.

I'm thinking something like use_persistent or persistent could also work.

@EmreTech
Copy link
Collaborator

EmreTech commented Nov 5, 2022

If the auto_stop parameter limits storing persistent views for specific messages, force_global or just global would be a better name.

I'm thinking something like use_persistent or persistent could also work.

That doesn't make sense imo. The whole point of this PR is to limit storing persistent views with a specific message, right?

@TrapDrap
Copy link
Contributor Author

TrapDrap commented Nov 5, 2022

Also, to avoid breaking changes, the default for auto_stop or whatever the parameter will end up being named should be False.

Well, the main part of this is to fix the issue of views being stored without the user's knowledge, since most people probably call their views without knowing they need to properly stop it, it being set to True makes sense. Will wait on more input tho.

@EmreTech EmreTech added t: enhancement Type: enhancement - new feature or request p: low Priority: low - not important to be worked on s: awaiting review Status: the issue or PR is awaiting reviews labels Nov 5, 2022
@EmreTech
Copy link
Collaborator

EmreTech commented Nov 5, 2022

Well, the main part of this is to fix the issue of views being stored without the user's knowledge

Can you elaborate how this happens? Also, wouldn't more precise docs be better instead of this breaking change?

@TrapDrap
Copy link
Contributor Author

TrapDrap commented Nov 5, 2022

Well, the main part of this is to fix the issue of views being stored without the user's knowledge

Can you elaborate how this happens? Also, wouldn't more precise docs be better instead of this breaking change?

On every message sent with a view, all components are added to a list specific to that message. Over time, these are not deleted, even when the messages are uncached, accumulating in the background. It's a very rare scenario you would need to use message-specific data if you're using a persistent view, usually, you get all the message data from interaction.message, so it wouldn't be a noticeable breaking change to the large majority of View users.
Message-specific view also causes updated views not to reflect on older messages, this fixes that.

@EmreTech
Copy link
Collaborator

EmreTech commented Nov 5, 2022

Well, the main part of this is to fix the issue of views being stored without the user's knowledge

Can you elaborate how this happens? Also, wouldn't more precise docs be better instead of this breaking change?

On every message sent with a view, all components are added to a list specific to that message. Over time, these are not deleted, even when the messages are uncached, accumulating in the background. It's a very rare scenario you would need to use message-specific data if you're using a persistent view, usually, you get all the message data from interaction.message, so it wouldn't be a noticeable breaking change to the large majority of View users.

Ah I understand now.

@TrapDrap TrapDrap changed the title feat(View): add auto_stop param feat(View): add global param Nov 5, 2022
@TrapDrap
Copy link
Contributor Author

TrapDrap commented Nov 5, 2022

that's really weird, changing the var and it's suddenly failing

@TrapDrap
Copy link
Contributor Author

TrapDrap commented Nov 5, 2022

nevermind it's just a the python global, guess cant use "global" as name

@EmreTech

This comment was marked as outdated.

@EmreTech
Copy link
Collaborator

EmreTech commented Nov 5, 2022

Ignore my last comment, I confused the global keyword with globals.

@TrapDrap
Copy link
Contributor Author

TrapDrap commented Nov 5, 2022

hold on i used the wrong version 💀

@TrapDrap TrapDrap changed the title feat(View): add global param feat(View): add prevent_update param Nov 5, 2022
@TrapDrap
Copy link
Contributor Author

TrapDrap commented Nov 5, 2022

I changed the name to prevent_update to match with the already existing ...prevent_update... function when setting the message-specific view

nextcord/ui/view.py Outdated Show resolved Hide resolved
nextcord/ui/view.py Show resolved Hide resolved
nextcord/ui/view.py Show resolved Hide resolved
nextcord/ui/view.py Show resolved Hide resolved
@spifory
Copy link
Collaborator

spifory commented Mar 12, 2023

Also correct me if I'm wrong, but I don't think this exactly fixes an issue; And also this is not a breaking change since you're just adding a parameter.

image

@TrapDrap
Copy link
Contributor Author

Also correct me if I'm wrong, but I don't think this exactly fixes an issue; And also this is not a breaking change since you're just adding a parameter.

image

Ah yes, some of those checks are outdated. It use to be a breaking change & fixes an issue but now is an optional param.

@TrapDrap
Copy link
Contributor Author

I have no idea why there's a conflict.

@TrapDrap TrapDrap closed this Jun 10, 2023
@TrapDrap TrapDrap force-pushed the master branch 2 times, most recently from 9d18267 to 65ee8af Compare June 10, 2023 16:43
* Readded updated to abc.py
* Readded updated to view.py
@TrapDrap
Copy link
Contributor Author

Reopen, still adding back changes

@TrapDrap TrapDrap reopened this Jun 10, 2023
@EmreTech EmreTech merged commit e639344 into nextcord:master Jun 17, 2023
10 checks passed
@EmreTech EmreTech removed the s: awaiting review Status: the issue or PR is awaiting reviews label Jun 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p: low Priority: low - not important to be worked on t: enhancement Type: enhancement - new feature or request
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants