Introduce ASCellLayoutMode #1273
Conversation
Source/ASCollectionViewProtocols.h
Outdated
There was a problem hiding this comment.
Document the default behavior.
There was a problem hiding this comment.
Tried to clarify the default values for ASCellLayoutMode flags a bit better.
Source/ASCollectionView.mm
Outdated
There was a problem hiding this comment.
Why have this fork in two places i.e. here and also below at 1750 and 2214?
There was a problem hiding this comment.
In the two places we have a different logic when to perform a batch update or reload data. In rangeController:updateWithChangeSet:updates: besides checking for the ASCellLayoutModeAlwaysReloadData flag we also checking some more conditions to decide if we should reload data or batch update.
That said I removed the "automatic" behavior within _superPerformBatchUpdates:completion: and we are now explicitly calling if else in places where we would like to differentiate between the calls. It was not really obvious from a caller perspective that this automatic behavior exists in the first place.
maicki
force-pushed
the
MSCellLayoutModes
branch
from
7ca9f6b to
ff65dcb
Compare
|
@Adlai-Holler Can you please give it another look. Thanks! |
Source/ASCollectionView.mm
Outdated
There was a problem hiding this comment.
Seems like the updates block should be non-nil. At the very least, we want to swap the maps. If it's non-nil, we can mark it that way and skip the nil check? The main point is I don't want people to think that it can ever be nil.
There was a problem hiding this comment.
It can be nil, if you just would like to call [super reloadData] with a completion block. I actually replaced all places where we call [super reloadData] with the call to _superReloadData so we have one central point to call in.
There was a problem hiding this comment.
Oh yeah, I overlooked those calls. 👍
Tests/ASCollectionViewTests.mm
Outdated
There was a problem hiding this comment.
Shouldn't we test the default behavior? Maybe add another test case if need to for this new mode.
There was a problem hiding this comment.
Good call! Added a test.
Source/ASCollectionView.mm
Outdated
There was a problem hiding this comment.
Oh yeah, I overlooked those calls. 👍
maicki
force-pushed
the
MSCellLayoutModes
branch
from
604776f to
3502636
Compare
appleguy
left a comment
appleguy
left a comment
There was a problem hiding this comment.
More than a year in the making! Thanks a lot for cleaning this up before upstreaming...there was a lot of complexity you were able to drop, @maicki. Hopefully we'll thin this down even more in the coming months, but for now it is a great feeling to be fully (or very nearly) in sync with upstream.
* Introduce ASCellLayoutMode * Some smaller improvements * Improve logic around _superPerformBatchUpdates:completion: * Add comment about default values for ASCellLayoutModeNone * Always call _superReloadData:completion: within UICollectionView * Add initial range test for ASCellLayoutModeNone
* Introduce ASCellLayoutMode * Some smaller improvements * Improve logic around _superPerformBatchUpdates:completion: * Add comment about default values for ASCellLayoutModeNone * Always call _superReloadData:completion: within UICollectionView * Add initial range test for ASCellLayoutModeNone
ASCellLayoutModes are flexible settings for applications to improve the flexibility of integrating Texture within an app.Providing this mode allows for developers to debug potential issues or work towards resolving technical debt in their collection view data source, while ramping up asynchronous collection layout.