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

Potential Memory Issue with New Cell Recycling Implementation #128

Open
kolinkrewinkel opened this issue Apr 4, 2012 · 8 comments · Fixed by #130
Open

Potential Memory Issue with New Cell Recycling Implementation #128

kolinkrewinkel opened this issue Apr 4, 2012 · 8 comments · Fixed by #130

Comments

@kolinkrewinkel
Copy link
Owner

If we're not removing them, and just hiding them, aren't we creating potential to just recreate new cells and create an infinite stack?

@jonsterling
Copy link
Collaborator

@zadr?

@zadr
Copy link
Contributor

zadr commented Apr 4, 2012

I haven't noticed any memory issues when using this code. The point of reuse is to keep cells around for the duration of the gridview, anyway. It's rare for a cell to go away before the gridview does.

If you like, I can add some logic on cell deletion (and low memory warnings, probably) to check to see if the total # of cells we have is less than the number of cells on screen, and remove unused cells as necessary. My thought for not doing this in the first place was that if we needed these cells in the first place, we will likely need them again.

@kolinkrewinkel
Copy link
Owner Author

I think before we move onto the removal/hidden thing, there's some sort of logic error with the cell recycling, as pointed out to me by several people (with cells lingering around after they were supposed to have been removed.)

@zadr
Copy link
Contributor

zadr commented Apr 4, 2012

Yeah, I made a comment on ticket #129, and saw Jon's comment on #127. Looking into it now. Wondering how I didn't notice this before, as well.

@kolinkrewinkel
Copy link
Owner Author

Yeah, it's visible with cells with variable content and translucent content. Not really sure why, as it was bulletproof from what I could tell, even through @Gi-lo's testing.

@zadr
Copy link
Contributor

zadr commented Apr 4, 2012

Could it have been a problem with my merge picking the wrong thing?

@kolinkrewinkel
Copy link
Owner Author

No, I got these reports before the merge.

@zadr
Copy link
Contributor

zadr commented Apr 4, 2012

This (memory) should be addressed by #130

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 a pull request may close this issue.

3 participants