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

fix: prevent resource leak in animateToItem #76

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Zekfad
Copy link

@Zekfad Zekfad commented Jan 6, 2025

resolves #65

@knopp
Copy link
Collaborator

knopp commented Jan 6, 2025

I think I'd prefer disposing the animation from ListController.dispose.

@Zekfad
Copy link
Author

Zekfad commented Jan 6, 2025

It will cause issues with leaked ticker. Because list can be deleted (and associated ticker provider), but controller will remain with playing animations and will cause problem in mentioned issue.

@Zekfad
Copy link
Author

Zekfad commented Jan 22, 2025

Is there any ETA for a new release?
I'm currently using pubspec override to my git repo with this fix, which is inconvenient for receiving further upstream updates.

Also, talking about this PR, why do you even extract this functionality to a class which is essentially a single method that is immediately called. I don't see any reason for this class to be public also, as it's used only internally.

Maybe it's a good reason to refactor this to a private method?

@gbtb16
Copy link

gbtb16 commented Jan 22, 2025

@Zekfad, I agree with what you said!

@CillianMyles, @renancaraujo, @knopp Maybe one of you can take a look at it and fix it, please!

@knopp
Copy link
Collaborator

knopp commented Jan 22, 2025

Please don't tag random people! I plan to look at this, but I am currently busy with other work. Also I'd like to see a test in the PR.

@Zekfad
Copy link
Author

Zekfad commented Jan 23, 2025

@knopp I've added test for this specific case. You can comment out these lines and test will fail:

https://github.com/superlistapp/super_sliver_list/pull/76/files#diff-5988a0720eec97069caff7143b58f8844be4ab9c7533e495062953b8e5e48666R283-R288

@gbtb16
Copy link

gbtb16 commented Jan 23, 2025

@knopp

I can even understand the frustration of getting an unexpected mention. But you are an official maintainer of the repository. Whatever problems there are, you and your colleagues should be responsible for fixing them.

If you're not interested in having that kind of responsibility, consider passing it on to someone else.

We all have jobs to do. And I include myself in that group by having a large repository that needs attention too, even if I have other things to do!

Cheers.

lib/src/extent_list.dart Outdated Show resolved Hide resolved
pubspec.yaml Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants