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

Rejigger Cell Visibility Tracking #317

Merged
merged 6 commits into from
May 30, 2017
Merged

Rejigger Cell Visibility Tracking #317

merged 6 commits into from
May 30, 2017

Conversation

Adlai-Holler
Copy link
Member

cc #145 . I never had a reliable repro for it, so I'm counting on the community to test this patch.

willDisplayCell and didEndDisplayingCell are the authoritative sources for what elements are visible. Elements may be visible in multiple cells e.g. during animations, so we use a counted set.

  • Replace (weak)_ASCollectionViewCell.node with (strong)_ASCollectionViewCell.element.
  • Add an NSCountedSet<ASCollectionElement *> *_visibleElements to collection view.
  • In willDisplayCell: and didEndDisplayingCell:, add/remove the cell's element from the visible set.
  • Modify visibleElementsForRangeController: to just return a copy of this set.
  • Put back the dealloc-while-visible assertion.

@Adlai-Holler
Copy link
Member Author

@rurza @temkich @appleguy Could y'all test this patch and see if it resolves the dealloc-while-visible cases you were seeing? It should be solid.

@ghost
Copy link

ghost commented May 29, 2017

🚫 CI failed with log

Copy link
Member

@appleguy appleguy left a comment

Choose a reason for hiding this comment

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

@Adlai-Holler this is a nontrivial patch! Thanks for creating it on a holiday!

Everything here looks good to me. I think we should land this and push out a release as soon as practical, although we might want to get additional reviews (e.g. from @nguyenhuy) or at least get a certain pure-Texture app running against it to look for any subtle issues :).

[_rangeController setNeedsUpdate];
return;
}
auto cell = (_ASCollectionViewCell *)rawCell;
Copy link
Member

Choose a reason for hiding this comment

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

y hallo there, auto

@ghost
Copy link

ghost commented May 30, 2017

1 Warning
⚠️ Any source code changes should have an entry in CHANGELOG.md or have #trivial in their title.

Generated by 🚫 Danger

@rurza
Copy link

rurza commented May 30, 2017 via email

@Adlai-Holler
Copy link
Member Author

Let's land this and get it tested. I'll keep an eye on its behavior and fix any regressions.

@Adlai-Holler Adlai-Holler merged commit 4d0eeb6 into master May 30, 2017
@Adlai-Holler Adlai-Holler deleted the AHCellVisibility branch May 30, 2017 17:25
bernieperez pushed a commit to AtomTickets/Texture that referenced this pull request Apr 25, 2018
* Rejigger visible elements tracking

* Put the assertion back

* Remove unused stuff

* Make it stronk
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants