Renew supplementary node on relayout #842
Conversation
|
🚫 CI failed with log |
Source/Private/ASMutableElementMap.m
Outdated
| } | ||
| for (NSIndexPath *indexPath in indexPaths) { | ||
| supplementariesForKind[indexPath] = nil; | ||
| } |
There was a problem hiding this comment.
Use [_supplementaryElements[kind] removeObjectsForKeys:indexPaths];
Source/Details/ASDataController.mm
Outdated
| ASCollectionLayoutContext *context = [self.layoutDelegate layoutContextWithElements:previousMap]; | ||
| Class<ASDataControllerLayoutDelegate> layoutDelegateClass = [self.layoutDelegate class]; | ||
| ASCollectionLayoutState *layoutState = [layoutDelegateClass calculateLayoutWithContext:context]; | ||
| UICollectionViewLayoutAttributes *attrs = [layoutState layoutAttributesForSupplementaryElementOfKind:kind atIndexPath:indexPath]; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
ok, I will just leave this block as todo.
There was a problem hiding this comment.
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 ✅.
Source/Details/ASDataController.mm
Outdated
| ASCollectionLayoutContext *context = [self.layoutDelegate layoutContextWithElements:previousMap]; | ||
| Class<ASDataControllerLayoutDelegate> layoutDelegateClass = [self.layoutDelegate class]; | ||
| ASCollectionLayoutState *layoutState = [layoutDelegateClass calculateLayoutWithContext:context]; | ||
| UICollectionViewLayoutAttributes *attrs = [layoutState layoutAttributesForSupplementaryElementOfKind:kind atIndexPath:indexPath]; |
There was a problem hiding this comment.
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?
Source/Private/ASMutableElementMap.m
Outdated
| } | ||
| for (NSIndexPath *indexPath in indexPaths) { | ||
| supplementariesForKind[indexPath] = nil; | ||
| } |
Source/Details/ASDataController.mm
Outdated
| UICollectionViewLayoutAttributes *attrs = [layoutState layoutAttributesForSupplementaryElementOfKind:kind atIndexPath:indexPath]; | ||
| newSizeRange = attrs ? ASSizeRangeMake(attrs.size, attrs.size) : ASSizeRangeMake(CGSizeZero, CGSizeZero); | ||
| } else { | ||
| newSizeRange = [self constrainedSizeForNodeOfKind:kind atIndexPath:indexPath]; |
There was a problem hiding this comment.
This will call out to
Texture/Source/ASCollectionView.mm
Line 2032 in 8a4bb6c
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.
…wsd0829/Texture into renew-supplementary-node-on-relayout
Source/Details/ASDataController.mm
Outdated
| // 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) { |
There was a problem hiding this comment.
do we need to set newSizeRange here? If so oZero? does not sound quite right though
There was a problem hiding this comment.
Yeah, doesn't sound good to me as well. I think we can just completely ignore this case and bail early.
There was a problem hiding this comment.
oh right. Done
|
Thanks for the diff Max! |
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