Skip to content
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

Merged
merged 6 commits into from
Aug 23, 2017
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
## master

* Add your own contributions to the next release on the line below this with your name.
- [ASCollectionNode] Add -isProcessingUpdates and -onDidFinishProcessingUpdates: APIs. [#522](https://github.com/TextureGroup/Texture/pull/522) [Scott Goodson](https://github.com/appleguy)
- [Accessibility] Add .isAccessibilityContainer property, allowing automatic aggregation of children's a11y labels. [#468][Scott Goodson](https://github.com/appleguy)
- [ASImageNode] Enabled .clipsToBounds by default, fixing the use of .cornerRadius and clipping of GIFs. [Scott Goodson](https://github.com/appleguy) [#466](https://github.com/TextureGroup/Texture/pull/466)
- Fix an issue in layout transition that causes it to unexpectedly use the old layout [Huy Nguyen](https://github.com/nguyenhuy) [#464](https://github.com/TextureGroup/Texture/pull/464)
Expand Down
39 changes: 36 additions & 3 deletions Source/ASCollectionNode.h
Original file line number Diff line number Diff line change
Expand Up @@ -252,10 +252,39 @@ NS_ASSUME_NONNULL_BEGIN
*/
- (void)performBatchUpdates:(nullable AS_NOESCAPE void (^)())updates completion:(nullable void (^)(BOOL finished))completion;

/**
* Returns YES if the ASCollectionNode is still processing changes from performBatchUpdates:.
* This is typically the concurrent allocation (calling nodeBlocks) and layout of newly inserted
* ASCellNodes. If YES is returned, then calling -waitUntilAllUpdatesAreProcessed may take tens of
* milliseconds to return as it blocks on these concurrent operations.
*
* Returns NO if ASCollectionNode is fully synchronized with the underlying UICollectionView. This
* means that until the next performBatchUpdates: is called, it is safe to compare UIKit values
* (such as from UICollectionViewLayout) with your app's data source.
*
* This method will always return NO if called immediately after -waitUntilAllUpdatesAreProcessed.
*/
@property (nonatomic, readonly) BOOL isProcessingUpdates;

/**
* Schedules a block to be performed (on the main thread) after processing of performBatchUpdates:
* is finished (completely synchronized to UIKit). The blocks will be run at the moment that
* -isProcessingUpdates changes from YES to NO;
*
* When isProcessingUpdates == NO, the block is run block immediately (before the method returns).
*
* Blocks scheduled by this mechanism are NOT guaranteed to run in the order they are scheduled.
* They may also be delayed if performBatchUpdates continues to be called; the blocks will wait until
* all running updates are finished.
*
* Calling -waitUntilAllUpdatesAreProcessed is one way to flush any pending update completion blocks.
*/
- (void)onDidFinishProcessingUpdates:(nullable void (^)())didFinishProcessingUpdates;

/**
* Blocks execution of the main thread until all section and item updates are committed to the view. This method must be called from the main thread.
*/
- (void)waitUntilAllUpdatesAreCommitted;
- (void)waitUntilAllUpdatesAreProcessed;

/**
* Inserts one or more sections.
Expand Down Expand Up @@ -501,9 +530,13 @@ NS_ASSUME_NONNULL_BEGIN
* @warning This method is substantially more expensive than UICollectionView's version.
*
* @deprecated This method is deprecated in 2.0. Use @c reloadDataWithCompletion: and
* then @c waitUntilAllUpdatesAreCommitted instead.
* then @c waitUntilAllUpdatesAreProcessed instead.
*/
- (void)reloadDataImmediately ASDISPLAYNODE_DEPRECATED_MSG("Use -reloadData / -reloadDataWithCompletion: followed by -waitUntilAllUpdatesAreCommitted instead.");
- (void)reloadDataImmediately ASDISPLAYNODE_DEPRECATED_MSG("Use -reloadData / -reloadDataWithCompletion: followed by -waitUntilAllUpdatesAreProcessed instead.");

// TODO: Rename framework uses of this method and add deprecation message.
// ASDISPLAYNODE_DEPRECATED_MSG("This method has been renamed to -waitUntilAllUpdatesAreProcessed.");
Copy link
Member

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.

- (void)waitUntilAllUpdatesAreCommitted;

@end

Expand Down
19 changes: 19 additions & 0 deletions Source/ASCollectionNode.mm
Original file line number Diff line number Diff line change
Expand Up @@ -713,6 +713,25 @@ - (void)waitUntilAllUpdatesAreCommitted
}
}

- (void)waitUntilAllUpdatesAreProcessed
{
[self waitUntilAllUpdatesAreCommitted];
}

- (BOOL)isProcessingUpdates
{
return (self.nodeLoaded ? [self.view isProcessingUpdates] : NO);
}

- (void)onDidFinishProcessingUpdates:(nullable void (^)())completion
{
if (!self.nodeLoaded) {
completion();
} else {
[self.view onDidFinishProcessingUpdates:completion];
}
}

- (void)reloadDataWithCompletion:(void (^)())completion
{
ASDisplayNodeAssertMainThread();
Expand Down
4 changes: 3 additions & 1 deletion Source/ASCollectionView.h
Original file line number Diff line number Diff line change
Expand Up @@ -296,8 +296,10 @@ NS_ASSUME_NONNULL_BEGIN
- (void)relayoutItems ASDISPLAYNODE_DEPRECATED_MSG("Use ASCollectionNode method instead.");

/**
* Blocks execution of the main thread until all section and row updates are committed. This method must be called from the main thread.
* See ASCollectionNode.h for full documentation of these methods.
*/
@property (nonatomic, readonly) BOOL isProcessingUpdates;
- (void)onDidFinishProcessingUpdates:(nullable void (^)())completion;
- (void)waitUntilAllUpdatesAreCommitted ASDISPLAYNODE_DEPRECATED_MSG("Use ASCollectionNode method instead.");

/**
Expand Down
10 changes: 10 additions & 0 deletions Source/ASCollectionView.mm
Original file line number Diff line number Diff line change
Expand Up @@ -371,6 +371,16 @@ - (void)waitUntilAllUpdatesAreCommitted
[_dataController waitUntilAllUpdatesAreCommitted];
}

- (BOOL)isProcessingUpdates
{
return [_dataController isProcessingUpdates];
}

- (void)onDidFinishProcessingUpdates:(nullable void (^)())completion
{
[_dataController onDidFinishProcessingUpdates:completion];
}

- (void)setDataSource:(id<UICollectionViewDataSource>)dataSource
{
// UIKit can internally generate a call to this method upon changing the asyncDataSource; only assert for non-nil. We also allow this when we're doing interop.
Expand Down
5 changes: 5 additions & 0 deletions Source/Details/ASDataController.h
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,11 @@ extern NSString * const ASCollectionInvalidUpdateException;
*/
- (void)relayoutNodes:(id<NSFastEnumeration>)nodes nodesSizeChanged:(NSMutableArray * _Nonnull)nodesSizesChanged;

/**
* See ASCollectionNode.h for full documentation of these methods.
*/
@property (nonatomic, readonly) BOOL isProcessingUpdates;
- (void)onDidFinishProcessingUpdates:(nullable void (^)())completion;
- (void)waitUntilAllUpdatesAreCommitted;

/**
Expand Down
29 changes: 29 additions & 0 deletions Source/Details/ASDataController.mm
Original file line number Diff line number Diff line change
Expand Up @@ -437,6 +437,35 @@ - (void)waitUntilAllUpdatesAreCommitted
[self _scheduleBlockOnMainSerialQueue:^{ }];
}

- (BOOL)isProcessingUpdates
{
ASDisplayNodeAssertMainThread();
if (_mainSerialQueue.numberOfScheduledBlocks > 0) {
return YES;
Copy link
Member

Choose a reason for hiding this comment

The 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];
}];
});
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple low-impact notes on this method:

  • If we're asserting main, is it worth removing ASPerformBlockOnMainThread? Presumably an error of this kind can be reliably caught during development. If the notion here is "better safe than sorry," I'm down with that as well.
  • Should we schedule the retry on the main serial queue, so that if they wait on updates to complete we can be sure this block is run before returning from the wait? I suppose it's not a big deal – there's no guarantee this block will be run as soon as we finish processing updates. But at the same time, this is a break from the usual pattern of editingQueue -> main serial queue for main-thread-injection into data controller's async work.

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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();
Expand Down
1 change: 1 addition & 0 deletions Source/Details/ASMainSerialQueue.h
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
AS_SUBCLASSING_RESTRICTED
@interface ASMainSerialQueue : NSObject

@property (nonatomic, readonly) NSUInteger numberOfScheduledBlocks;
- (void)performBlockOnMainThread:(dispatch_block_t)block;

@end
5 changes: 5 additions & 0 deletions Source/Details/ASMainSerialQueue.mm
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,11 @@ - (instancetype)init
return self;
}

- (NSUInteger)numberOfScheduledBlocks
{
return _blocks.count;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to grab _serialQueueLock lock here.

}

- (void)performBlockOnMainThread:(dispatch_block_t)block
{
ASDN::MutexLocker l(_serialQueueLock);
Expand Down
9 changes: 9 additions & 0 deletions Tests/ASCollectionViewTests.mm
Original file line number Diff line number Diff line change
Expand Up @@ -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");

Copy link
Member

Choose a reason for hiding this comment

The 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];

Expand Down