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

Renew supplementary node on relayout #842

Conversation

wsdwsd0829
Copy link
Contributor

@wsdwsd0829 wsdwsd0829 commented Mar 18, 2018

When a supplementary view experiences layout change from zero to non-zero, the map in dataController need to be updated.

Thanks @nguyenhuy to provide the hints here:)
nguyenhuy@478d68e

@ghost
Copy link

ghost commented Mar 27, 2018

🚫 CI failed with log

@TextureGroup TextureGroup deleted a comment May 26, 2018
}
for (NSIndexPath *indexPath in indexPaths) {
supplementariesForKind[indexPath] = nil;
}
Copy link
Member

Choose a reason for hiding this comment

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

Use [_supplementaryElements[kind] removeObjectsForKeys:indexPaths];

Copy link
Member

Choose a reason for hiding this comment

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

👍

ASCollectionLayoutContext *context = [self.layoutDelegate layoutContextWithElements:previousMap];
Class<ASDataControllerLayoutDelegate> layoutDelegateClass = [self.layoutDelegate class];
ASCollectionLayoutState *layoutState = [layoutDelegateClass calculateLayoutWithContext:context];
UICollectionViewLayoutAttributes *attrs = [layoutState layoutAttributesForSupplementaryElementOfKind:kind atIndexPath:indexPath];
Copy link
Member

Choose a reason for hiding this comment

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

Let's not call calculateLayoutWithContext: in a loop on main like this.

These methods should only gather data and not perform expensive operations like layout. The problem I guess is that the ASCollectionLayout API is intentionally opaque about what size ranges it uses to measure its nodes.

@nguyenhuy Maybe we need new API to let ASCollectionLayoutDelegates specify what supplementary items exist during this phase, so that we can put them in the map.

Copy link
Member

Choose a reason for hiding this comment

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

I agree that we shouldn't perform a layout here just to figure out the size range. This is a limitation of ASCollectionLayout API.

Given the limited usage of ASCollectionLayout at the moment, and the possibility that it would take some time to come up with a sensible extension for its API, I'm wondering if we should aim to merge this PR without focusing much on ASCollectionLayout support, and then create a new ticket to follow up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, I will just leave this block as todo.

Copy link
Member

@nguyenhuy nguyenhuy left a comment

Choose a reason for hiding this comment

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

Thank you for putting this up, @wsdwsd0829. As stated inline, we need a better fix for ASCollectionLayout. However, I don't think it's a blocker for this PR so ✅.

ASCollectionLayoutContext *context = [self.layoutDelegate layoutContextWithElements:previousMap];
Class<ASDataControllerLayoutDelegate> layoutDelegateClass = [self.layoutDelegate class];
ASCollectionLayoutState *layoutState = [layoutDelegateClass calculateLayoutWithContext:context];
UICollectionViewLayoutAttributes *attrs = [layoutState layoutAttributesForSupplementaryElementOfKind:kind atIndexPath:indexPath];
Copy link
Member

Choose a reason for hiding this comment

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

I agree that we shouldn't perform a layout here just to figure out the size range. This is a limitation of ASCollectionLayout API.

Given the limited usage of ASCollectionLayout at the moment, and the possibility that it would take some time to come up with a sensible extension for its API, I'm wondering if we should aim to merge this PR without focusing much on ASCollectionLayout support, and then create a new ticket to follow up?

}
for (NSIndexPath *indexPath in indexPaths) {
supplementariesForKind[indexPath] = nil;
}
Copy link
Member

Choose a reason for hiding this comment

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

👍

UICollectionViewLayoutAttributes *attrs = [layoutState layoutAttributesForSupplementaryElementOfKind:kind atIndexPath:indexPath];
newSizeRange = attrs ? ASSizeRangeMake(attrs.size, attrs.size) : ASSizeRangeMake(CGSizeZero, CGSizeZero);
} else {
newSizeRange = [self constrainedSizeForNodeOfKind:kind atIndexPath:indexPath];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will call out to

ASDisplayNodeAssert(NO, @"To support supplementary nodes in ASCollectionView, it must have a layoutInspector for layout inspection. (See ASCollectionViewFlowLayoutInspector for an example.)");

Is it true that the assert will never trigged if called from here.
I remember the only case to accidentally trigger it might be calling it if canDelegate == true.

// If supplementary node doesn't exist and size is now non-zero, insert one.
for (NSIndexPath *indexPath in indexPaths) {
ASCollectionElement *previousElement = [previousMap supplementaryElementOfKind:kind atIndexPath:indexPath];
if (canDelegate) {
Copy link
Contributor Author

@wsdwsd0829 wsdwsd0829 May 30, 2018

Choose a reason for hiding this comment

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

do we need to set newSizeRange here? If so oZero? does not sound quite right though

Copy link
Member

@nguyenhuy nguyenhuy May 30, 2018

Choose a reason for hiding this comment

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

Yeah, doesn't sound good to me as well. I think we can just completely ignore this case and bail early.

Copy link
Contributor Author

@wsdwsd0829 wsdwsd0829 May 30, 2018

Choose a reason for hiding this comment

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

oh right. Done

@Adlai-Holler Adlai-Holler merged commit 9214e3c into TextureGroup:master Jun 4, 2018
@Adlai-Holler
Copy link
Member

Thanks for the diff Max!

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 this pull request may close these issues.

3 participants