-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Add allowsBackgroundReloading
Flag to ListAdapterUpdater to Give User Control of Behavior
#375
Conversation
skipsDiffingWhenOffscreen
Flag to ListAdapterUpdater to Give User Control of Behavior
Thanks @Adlai-Holler ! 💯 Not 100% sure about this change. I'll defer to @rnystrom. One thing to note though: we'll need the default value to be |
@jessesquires The current behavior is to default to EDIT: Thanks for the review btw! Love your work! |
@Adlai-Holler - ahh ok 👍 |
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.
Really curious about the ASDK integration stuff! Sounds cool.
Just a few nits then we can go ahead and land this. We had batch updates in the background for a while and removed it simply to save main queue resources for things that might be updating off-screen. Should be a harmless option.
@note This will result in better performance, but will not generate the same delegate | ||
callbacks and your layout will not receive `prepareForCollectionViewUpdates:`. | ||
|
||
@warning On iOS < 8.3, this behavior is unsupported and will always be treated as `NO`. |
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.
❤️
@@ -155,7 +153,8 @@ - (void)performBatchUpdatesWithCollectionView:(UICollectionView *)collectionView | |||
|
|||
// if the collection view isn't in a visible window, skip diffing and batch updating. execute all transition blocks, | |||
// reload data, execute completion blocks, and get outta here | |||
if (_canBackgroundReload && collectionView.window == nil) { | |||
BOOL iOS83OrLater = (NSFoundationVersionNumber >= NSFoundationVersionNumber_iOS_8_3); |
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.
nit: Can keep this in here for now, but IMO better to keep PRs changing one thing at a time. Makes crawling history and blames a lot easier.
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.
let's make this const BOOL
too 😊
@@ -155,7 +153,8 @@ - (void)performBatchUpdatesWithCollectionView:(UICollectionView *)collectionView | |||
|
|||
// if the collection view isn't in a visible window, skip diffing and batch updating. execute all transition blocks, | |||
// reload data, execute completion blocks, and get outta here | |||
if (_canBackgroundReload && collectionView.window == nil) { | |||
BOOL iOS83OrLater = (NSFoundationVersionNumber >= NSFoundationVersionNumber_iOS_8_3); | |||
if (iOS83OrLater && _skipsDiffingWhenOffscreen && collectionView.window == nil) { |
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.
Let's use the getter self.skipsDiffingWhenOffscreen
since we're using more than an ivar
@@ -459,4 +459,52 @@ - (void)test_whenCallingReloadData_withUICollectionViewFlowLayout_withEstimatedS | |||
XCTAssertEqual([collectionView numberOfItemsInSection:1], 4); | |||
} | |||
|
|||
- (void)test_whenCollectionViewNotInWindow_andDiffSkippingFlagSetNO_diffHappens | |||
{ |
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.
nit: sameline the {
for all tests
@Adlai-Holler updated the pull request - view changes |
@rnystrom Thanks for the review! |
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.
Actually, can we simplify this?
How about naming: allowsBackgroundReloading
?
Cocoa usually follows an "affirmative" naming style, i.e. prefersStatusBarHidden
, hidesBottomBarWhenPushed
, etc.
We'll need to invert the logic too.
Marked this for |
skipsDiffingWhenOffscreen
Flag to ListAdapterUpdater to Give User Control of BehaviorallowsBackgroundReloading
Flag to ListAdapterUpdater to Give User Control of Behavior
@Adlai-Holler updated the pull request - view changes |
@Adlai-Holler updated the pull request - view changes |
Thanks @Adlai-Holler ! Can you check the box on the PR to "allow owners to edit" ? (something like that) |
@jessesquires Done and done. |
@Adlai-Holler updated the pull request - view changes |
@Adlai-Holler updated the pull request - view changes |
@Adlai-Holler updated the pull request - view changes |
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.
Looks awesome! 🌮 🎉
@jessesquires has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
Such a powerful framework y'all have made! Game status: changed.
Changes in this pull request
ListAdapterUpdater
to require it to perform diffing, even when collection view is not in a window.prepareForCollectionViewUpdates:
.Pull request checklist
CHANGELOG.md
for any breaking changes, enhancements, or bug fixes.