[ASCollectionView][Architecture] Introduce ASSectionContext#2178
[ASCollectionView][Architecture] Introduce ASSectionContext#2178Adlai-Holler merged 1 commit intofacebookarchive:masterfrom
Conversation
ghost
added
the
nguyenhuy
changed the title
nguyenhuy
changed the title
nguyenhuy
changed the title
nguyenhuy
force-pushed
the
ASSectionContext
branch
from
d122578 to
306a56f
Compare
AsyncDisplayKit/ASCollectionView.h
Outdated
| */ | ||
| - (void)collectionViewUnlockDataSource:(ASCollectionView *)collectionView ASDISPLAYNODE_DEPRECATED; | ||
|
|
||
| - (_Nonnull id<ASSectionContext>)collectionView:(ASCollectionView *)collectionView contextForSectionAtIndex:(NSInteger)sectionIndex; |
There was a problem hiding this comment.
Shouldn't this be nullable?
I see – if you implement this we assume you always want to provide one. OK!
There was a problem hiding this comment.
Exactly. This method is optional and if users implement it, they need to provide a context.
That's said, in some cases, they may want to provide contexts for just a subset of sections. @Adlai-Holler Should we support it?
There was a problem hiding this comment.
Actually if we create an object for each section that holds a nullable context, we can relax on the nullability of this method. Will work on it.
|
@nguyenhuy This is looking perfect! This will be a huge boon for Pinterest, where synchronizing section info has been a major pain point. Looking toward the future, I think creating an object for each section even if they don't provide us with a context is valuable. I have some pretty ambitious plans for collection view debugging tools, and reifying the section abstraction (with an auto-incrementing ID) will help move us toward that. These section objects do not need to hold any nodes or anything – I agree with you that those other two approaches got too messy too fast. Just an ID and optionally a context. Thoughts? |
|
@nguyenhuy How's this going? Let me know if there's something that seems wonky or not quite right – once landed this will resolve an entire class of possible inconsistency issues in one shot, so I'm really excited to get it dialed in. |
nguyenhuy
force-pushed
the
ASSectionContext
branch
from
3e442b4 to
3c1fc55
Compare
nguyenhuy
force-pushed
the
ASSectionContext
branch
from
3c1fc55 to
96425b9
Compare
|
@Adlai-Holler: Sorry this took so long. I've added ASSection and addressed your comments. Please give this PR another review. Thank you! |
Adlai-Holler
left a comment
Adlai-Holler
left a comment
There was a problem hiding this comment.
@nguyenhuy This is absolutely excellent! I'm champing at the bit to deploy this and see how it works!
I left a couple notes, and I think in terms of unit tests some very basic cases will be sufficient to get the ball rolling:
- Test that after the initial layout, the collection view returns the correct context objects.
- Test that after section move, the collection view returns the correct context objects.
- Test that after reload data, the collection view returns the correct context objects.
- Test that after reloading a section, the collection view returns the correct context objects.
I can't wait to have this up and running, so don't hesitate to reach out if anything could be clearer or seems wonky.
AsyncDisplayKit/ASCollectionView.mm
Outdated
| - (id<ASSectionContext>)dataController:(ASDataController *)dataController contextForSection:(NSInteger)section | ||
| { | ||
| ASDisplayNodeAssertMainThread(); | ||
| id<ASSectionContext> context; |
There was a problem hiding this comment.
Initialize this to nil
AsyncDisplayKit/ASCollectionView.h
Outdated
| */ | ||
| - (void)collectionViewUnlockDataSource:(ASCollectionView *)collectionView ASDISPLAYNODE_DEPRECATED; | ||
|
|
||
| - (id<ASSectionContext> _Nullable)collectionView:(ASCollectionView *)collectionView contextForSection:(NSInteger)section; |
There was a problem hiding this comment.
Use the prettier - (nullable id<ASSectionContext>)collectionView: style throughout.
|
@Adlai-Holler — is it OK if we merge this before @nguyenhuy 's final changes? He's getting ready for the big move and so any polishing touches here would be better to do after integrating it, but only if they don't have important functional impacts. |
nguyenhuy
force-pushed
the
ASSectionContext
branch
from
3bc2966 to
58d3266
Compare
|
@Adlai-Holler @appleguy: Addressed comments and added test cases. |
nguyenhuy
force-pushed
the
ASSectionContext
branch
from
58d3266 to
227f097
Compare
|
@nguyenhuy Wow excellent! I really appreciate the quality of your work. If you'll rebase or merge latest master, the CI should be fixed and this should be ready for primetime. I'm confident this will work well, but for the sake of due diligence we'll test this against Pinterest via a cherry-pick and then land it immediately after. |
nguyenhuy
force-pushed
the
ASSectionContext
branch
from
227f097 to
b4675a2
Compare
nguyenhuy
force-pushed
the
ASSectionContext
branch
from
b4675a2 to
e5d4285
Compare
|
@Adlai-Holler: Squashed all commits into one and rebased it on top of latest master. Waiting for Travis to finish which seems to take forever :( Appreciate your test against Pinterest. Let me know what you find! Edit: Apparently Travis failed because of a misconfiguration in xcodeproj (i.e ASSectionContext.h should be a public header). Fixed it and we're good to go :) |
nguyenhuy
force-pushed
the
ASSectionContext
branch
from
e5d4285 to
2d85b16
Compare
- Objects conform to ASSectionContext protocol can be provided via ASCollectionDataSource and later retrieved from the collection view. They are guaranteed to be in sync with sections of the collection view. They can be used to store additional data associated with each section, to be used in collection view layout and the like. - ASSection is an internal object that is the foundation for coming debugging tools. - Unit tests included.
nguyenhuy
force-pushed
the
ASSectionContext
branch
from
2d85b16 to
df1fbd8
Compare
|
Hannah has tested it for a while and I did the same. This is running excellently! Thanks so much Huy – I'm going to start building on this system immediately. Let's do this again sometime! |
|
@Adlai-Holler: I'm really glad it works. More than happy to do this again, anytime! |
- Objects conform to ASSectionContext protocol can be provided via ASCollectionDataSource and later retrieved from the collection view. They are guaranteed to be in sync with sections of the collection view. They can be used to store additional data associated with each section, to be used in collection view layout and the like. - ASSection is an internal object that is the foundation for coming debugging tools. - Unit tests included.
4 participants
It is a custom object that can be optionally provided by ASCollectionDataSource and later retrieved from the collection view to be used in collection view layout. The idea was first proposed by @Adlai-Holler in #2067.
I've been playing around with different ways to implement this feature, including having an ASSection that is a proxy of cell nodes array (here), or an ASSection that is a subclass of NSMutableArray<ASCellNode *> (here). I think the implementation in this PR is simpler and thus, more elegant.
Please let me know what you think, @Adlai-Holler @maicki @appleguy @garrettmoon @levi. Thanks!
TODO: