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

[ASWrapperCellNode] Introduce a new class allowing more control of UIKit passthrough cells. #797

Merged
merged 13 commits into from
Mar 13, 2018

Conversation

appleguy
Copy link
Member

A few minor fixes to Collections behavior as well, including a new isSynchronized
API. The difference from processingUpdates is that after Synchronized, all animations
have also completed (or runloop turn if animations disabled, so .collectionViewLayout
can be relied on being fully in sync).

More upstreaming to come after this can land...

…UIKit passthrough cells.

A few minor fixes to Collections behavior as well, including a new isSynchronized
API. The difference from processingUpdates is that after Synchronized, all animations
have also completed (or runloop turn if animations disabled, so .collectionViewLayout
can be relied on being fully in sync).

More upstreaming to come after this can land...
@appleguy
Copy link
Member Author

BTW, I'm thinking it would be optimal to iterate on some things like this while they are in +Private or +Beta headers. Let me know what you all would suggest.

There's a backlog of APIs like this which are pretty well tested, but not necessarily flawless for broad public consumption, so it might be easiest to get them in (if they seem generally useful) and evolve towards public.

@appleguy appleguy changed the title - [ASWrapperCellNode] Introduce a new class allowing more control of UIKit passthrough cells. [ASWrapperCellNode] Introduce a new class allowing more control of UIKit passthrough cells. Feb 11, 2018
Copy link
Member

@Adlai-Holler Adlai-Holler left a comment

Choose a reason for hiding this comment

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

This is a pretty clever idea – a reasonably straightforward way to pipe data through Texture while using passthrough. 👍

*
* When isSynchronized == YES, the block is run block immediately (before the method returns).
*/
- (void)onDidFinishSynchronizing:(nullable void (^)(void))didFinishSynchronizing;
Copy link
Member

Choose a reason for hiding this comment

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

Nit: This decl says nullable but the impl will crash with a nil block.

Probably best to let this argument default to nonnull since I can't think of a use case for knowingly passing nil in here and also add a nil check in the implementation.

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 cool, fixed all definitions (and also made the change for the existing onDidFinishProcessingUpdates:).

I moved Synchronizing to +Beta for now. A better name might be onDidFinishSynchronizingLayout, or ...LayoutSynchronization. Or even merging with onDidFinishProcessingUpdates with the option to specify a type of "finished", with synchronized including the end of animation. It's all a bit subtle and confusing, but having this call point available has been very useful even as the name remains a bit unclear.

/**
* Returns YES if the ASCollectionNode contents are completely synchronized with the underlying collection-view layout.
*/
@property (nonatomic, readonly) BOOL isSynchronized;
Copy link
Member

Choose a reason for hiding this comment

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

synchronized with an isSynchronized getter?

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 This is an interesting case because it's a read-only property, so only the getter applies. I'm happy to change it in the name of convention, though it felt a bit unusual to write!

@appleguy
Copy link
Member Author

@Adlai-Holler thanks! In practice I've found it's useful to be able to hook into ASInterfaceState calls, even for cells that rely on a more traditional architecture. It's possible to define your own delegate on a subclass of ASWrapperCellNode, and pair it with a specific hand-authored NSObject "cell controller" or other pattern that a standard UIKit app might use, and integrate more elegantly with true ASCellNodes.

Agree with your feedback, will update.

@TextureGroup TextureGroup deleted a comment Mar 11, 2018
@TextureGroup TextureGroup deleted a comment Mar 11, 2018
Copy link
Member

@Adlai-Holler Adlai-Holler left a comment

Choose a reason for hiding this comment

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

Approved again, great diff and I regret letting this linger here for so long. ✅

@Adlai-Holler
Copy link
Member

Looks like there was a subtle behavior change on the processing flag:

ASCollectionViewTests
  testInitialRangeBounds, ((!cn.isProcessingUpdates) is true) failed - ASCollectionNode should no longer be processing updates inside -onDidFinishProcessingUpdates: block
  [cn onDidFinishProcessingUpdates:^{
    XCTAssertTrue(!cn.isProcessingUpdates, @"ASCollectionNode should no longer be processing updates inside -onDidFinishProcessingUpdates: block");
  }];

@ghost
Copy link

ghost commented Mar 13, 2018

1 Warning
⚠️ This is a big PR, please consider splitting it up to ease code review.

Generated by 🚫 Danger

@TextureGroup TextureGroup deleted a comment Mar 13, 2018
@TextureGroup TextureGroup deleted a comment Mar 13, 2018
@TextureGroup TextureGroup deleted a comment Mar 13, 2018
@TextureGroup TextureGroup deleted a comment Mar 13, 2018
@appleguy
Copy link
Member Author

appleguy commented Mar 13, 2018

Maybe useful for debugging CI - even after fixing the test failure, I got the spurious failure below. Fingers crossed that the re-run is successful. These logs seem to suggest a simulator / local environment error.

@Adlai-Holler, @garrettmoon — is it possible that updating our default Xcode and/or iOS version for tests could improve this? iOS 10.2 is pretty old now; if 11.2 has any screenshot diff failures, can we pass with 10.3 and see if it improves?

2018-03-12 23:08:42.596 xcodebuild[23026:7708106]  IDETestOperationsObserverDebug: Writing diagnostic log for test session to:
/var/folders/6h/ql1brcjx3936vr5vwgblbnc80000rm/T/com.apple.dt.XCTest/IDETestRunSession-6AA79403-76C1-4E99-A2C3-476FD9606EB0/AsyncDisplayKitTests-14725EE5-09C7-4221-B224-DF25EFF83DC7/Session-AsyncDisplayKitTests-2018-03-12_230842-DP6U86.log
2018-03-12 23:08:42.596 xcodebuild[23026:7706335] [MT] IDETestOperationsObserverDebug: (A5539DBA-C7B3-4DE5-9BA9-4BD1262E4DD5) Beginning test session AsyncDisplayKitTests-A5539DBA-C7B3-4DE5-9BA9-4BD1262E4DD5 at 2018-03-12 23:08:42.596 with Xcode 9A235 on target <DVTiPhoneSimulator: 0x7f81eaf133f0> {
		SimDevice: iPhone 7 (B2A2393E-7201-4E96-9C7B-2CB9B96BB40D, iOS 10.2, Booted)
} (10.2 (14C89))
2018-03-12 23:08:42.650 xcodebuild[23026:7707207] Connection peer refused channel request for "dtxproxy:XCTestManager_IDEInterface:XCTestManager_DaemonConnectionInterface"; channel canceled <DTXChannel: 0x7f81ec6375b0>
2018-03-12 23:08:42.651 xcodebuild[23026:7706335] [MT] IDETestOperationsObserverDebug: (A5539DBA-C7B3-4DE5-9BA9-4BD1262E4DD5) Failed to make test runner session:
Error Domain=DTXProxyChannel Code=1 "(null)" UserInfo={DTXProxyChannelErrorMessage=Channel disconnected}
2018-03-12 23:08:42.929 xcodebuild[23026:7706335] Error Domain=IDETestOperationsObserverErrorDomain Code=3 "The operation couldn’t be completed. (DTXProxyChannel error 1.) If you believe this error represents a bug, please attach the log file at /var/folders/6h/ql1brcjx3936vr5vwgblbnc80000rm/T/com.apple.dt.XCTest/IDETestRunSession-6AA79403-76C1-4E99-A2C3-476FD9606EB0/AsyncDisplayKitTests-14725EE5-09C7-4221-B224-DF25EFF83DC7/Session-AsyncDisplayKitTests-2018-03-12_230842-DP6U86.log" UserInfo={NSLocalizedDescription=The operation couldn’t be completed. (DTXProxyChannel error 1.) If you believe this error represents a bug, please attach the log file at /var/folders/6h/ql1brcjx3936vr5vwgblbnc80000rm/T/com.apple.dt.XCTest/IDETestRunSession-6AA79403-76C1-4E99-A2C3-476FD9606EB0/AsyncDisplayKitTests-14725EE5-09C7-4221-B224-DF25EFF83DC7/Session-AsyncDisplayKitTests-2018-03-12_230842-DP6U86.log}

Testing failed:
	The operation couldn’t be completed. (DTXProxyChannel error 1.) If you believe this error represents a bug, please attach the log file at /var/folders/6h/ql1brcjx3936vr5vwgblbnc80000rm/T/com.apple.dt.XCTest/IDETestRunSession-6AA79403-76C1-4E99-A2C3-476FD9606EB0/AsyncDisplayKitTests-14725EE5-09C7-4221-B224-DF25EFF83DC7/Session-AsyncDisplayKitTests-2018-03-12_230842-DP6U86.log
** TEST FAILED **

@Adlai-Holler
Copy link
Member

=/ Sucky to see that. Since it's an Xcode failure, I guess that changing iOS versions wouldn't do much but changing Xcode versions might. Fingers crossed here too, that the rebuild works.

@appleguy
Copy link
Member Author

Yeah, you're probably right. Xcode version would be the best bet. Is it viable to update more on our current build service?

That said, as an "omg it almost never works, let's try anything!" approach — the SDK version does actually affect all of the Simulator-related frameworks, e.g. including the OS X hooks / interfaces to Xcode. So, there's at least a chance it would help.

@appleguy appleguy merged commit a41cbb4 into master Mar 13, 2018
@appleguy
Copy link
Member Author

Thanks for all your help Adlai!

@appleguy appleguy deleted the ASWrapperCellNode branch March 13, 2018 08:03
bernieperez pushed a commit to AtomTickets/Texture that referenced this pull request Apr 25, 2018
…Kit passthrough cells. (TextureGroup#797)

* - [ASWrapperCellNode] Introduce a new class allowing more control of UIKit passthrough cells.

A few minor fixes to Collections behavior as well, including a new isSynchronized
API. The difference from processingUpdates is that after Synchronized, all animations
have also completed (or runloop turn if animations disabled, so .collectionViewLayout
can be relied on being fully in sync).

More upstreaming to come after this can land...

* Fix -[ASDataController clearData] to take no action before initial data loading.

* Empty commit to kick CI

* Spacing change to kick CI (since an empty commit doesn't work...)

* Tweak ASDataController changes to handle an edge case in _editingTransactionQueueCount management.

* Avoid excess cyclic calls to onDidFinishProcessingUpdates: by avoiding ASMainSerialQueue.

* Reverting my initial change as it wasn't the right approach, following the real fix before this.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants