-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[ASCollectionNode] Add -isProcessingUpdates and -onDidFinishProcessingUpdates: APIs. #522
Changes from 5 commits
6d687a8
de635f2
b3b7384
b66415b
f8b1fab
b54d4d1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -437,6 +437,35 @@ - (void)waitUntilAllUpdatesAreCommitted | |
[self _scheduleBlockOnMainSerialQueue:^{ }]; | ||
} | ||
|
||
- (BOOL)isProcessingUpdates | ||
{ | ||
ASDisplayNodeAssertMainThread(); | ||
if (_mainSerialQueue.numberOfScheduledBlocks > 0) { | ||
return YES; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
} else if (dispatch_group_wait(_editingTransactionGroup, DISPATCH_TIME_NOW) != 0) { | ||
// After waiting for zero duration, a nonzero value is returned if blocks are still running. | ||
return YES; | ||
} | ||
// Both the _mainSerialQueue and _editingTransactionQueue are drained; we are fully quiesced. | ||
return NO; | ||
} | ||
|
||
- (void)onDidFinishProcessingUpdates:(nullable void (^)())completion | ||
{ | ||
ASDisplayNodeAssertMainThread(); | ||
if ([self isProcessingUpdates] == NO) { | ||
ASPerformBlockOnMainThread(completion); | ||
} else { | ||
dispatch_async(_editingTransactionQueue, ^{ | ||
// Retry the block. If we're done processing updates, it'll run immediately, otherwise | ||
// wait again for updates to quiesce completely. | ||
[_mainSerialQueue performBlockOnMainThread:^{ | ||
[self onDidFinishProcessingUpdates:completion]; | ||
}]; | ||
}); | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A couple low-impact notes on this method:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, the idea is some extra safety in the case the assertion fails in production. It is not likely to be necessary, but the semantics of PerformOnMain are clear enough that it seems OK. Interested in your input on the other point. I had originally implemented it as waiting on the mainSerialQueue instead of a dispatch. I was thinking (and now believe this may be incorrect) that doing it this way would allow us to call out to the blocks in an "intermediate quiesced" state to perform some operations before potentially more edits start getting processed. However with the changes to isProcessingUpdates, I think this is actually invalid and we should schedule on the serial queue. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Adlai-Holler is it correct that we do NOT perform using the _editingTransactionGroup? I don't want multiple scheduled blocks to get stuck waiting for each other (as they do schedule in the main serial queue, and so I think this is correct, but I'm not completely confident. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's right! The transaction group is only (and always) applied to work blocks on the editing transaction queue. |
||
|
||
- (void)updateWithChangeSet:(_ASHierarchyChangeSet *)changeSet | ||
{ | ||
ASDisplayNodeAssertMainThread(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -40,6 +40,12 @@ - (instancetype)init | |
return self; | ||
} | ||
|
||
- (NSUInteger)numberOfScheduledBlocks | ||
{ | ||
ASDN::MutexLocker l(_serialQueueLock); | ||
return _blocks.count; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we need to grab |
||
} | ||
|
||
- (void)performBlockOnMainThread:(dispatch_block_t)block | ||
{ | ||
ASDN::MutexLocker l(_serialQueueLock); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1050,8 +1050,17 @@ - (void)testInitialRangeBounds | |
// Trigger the initial reload to start | ||
[window layoutIfNeeded]; | ||
|
||
// Test the APIs that monitor ASCollectionNode update handling | ||
XCTAssertTrue(cn.isProcessingUpdates, @"ASCollectionNode should still be processing updates after initial layoutIfNeeded call (reloadData)"); | ||
[cn onDidFinishProcessingUpdates:^{ | ||
XCTAssertTrue(!cn.isProcessingUpdates, @"ASCollectionNode should no longer be processing updates inside -onDidFinishProcessingUpdates: block"); | ||
}]; | ||
|
||
// Wait for ASDK reload to finish | ||
[cn waitUntilAllUpdatesAreCommitted]; | ||
|
||
XCTAssertTrue(!cn.isProcessingUpdates, @"ASCollectionNode should no longer be processing updates after -wait call"); | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💕 |
||
// Force UIKit to read updated data & range controller to update and account for it | ||
[cn.view layoutIfNeeded]; | ||
|
||
|
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.
If you're not going to do this in this PR, make sure to create an issue to track it.