-
Notifications
You must be signed in to change notification settings - Fork 28
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 supplementary reuse bug by ensuring we track the right view instance in our displayed views cache #321
Conversation
…nce in our displayed views cache.
@@ -64,6 +64,8 @@ final class SupplementaryContainerView : UICollectionReusableView | |||
) as! SupplementaryContainerView | |||
|
|||
view.reuseCache = reuseCache | |||
|
|||
// TODO: We need to also set this during on-screen updates; I don't think it's updated correctly now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be fixed after; but found this when debugging. It's theoretical; but I think it can happen.
@@ -163,7 +163,7 @@ extension ListView | |||
|
|||
headerFooter.willDisplay(view: container) | |||
|
|||
self.displayedSupplementaryItems[ObjectIdentifier(view)] = headerFooter | |||
self.displayedSupplementaryItems[ObjectIdentifier(container)] = headerFooter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the fix! Grr.
@@ -89,7 +89,9 @@ class SupplementaryContainerViewTests: XCTestCase | |||
|
|||
// Add a header | |||
|
|||
view.headerFooter = self.newHeaderFooter() | |||
let header = self.newHeaderFooter() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is now weak
; need a strong ref to the header.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - will let you decide if the non-necessary changes should go in this PR
This PR fixes a bug introduced in #288 (well sort of, 288 uncovered the actual bug):
A long time ago, a method argument was renamed from
view
toanyView
, which meant we were no longer shadowing theview
variable on theDelegate
:So the fix here was to properly pass the
ObjectIdentifier
of thecontainer
, notview
(self.view
is actually theListView
).Without this change,
didEndDisplay()
was never invoked on the header/footer (becausedisplayedSupplementaryItems
is used to track what is being used by the collection view), meaning we never cleared out values during view reuse, which broke the next time a view was reused if the view type was different.