[ASWrapperCellNode] Introduce a new class allowing more control of UIKit passthrough cells. by appleguy · Pull Request #797 · TextureGroup/Texture

appleguy · 2018-02-10T08:08:23Z

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 · 2018-02-10T08:12:43Z

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.

Adlai-Holler

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

Adlai-Holler · 2018-02-22T18:09:01Z

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


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.

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

Adlai-Holler · 2018-02-22T18:10:12Z

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


synchronized with an isSynchronized getter?

@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 · 2018-02-26T04:34:09Z

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

…exture into ASWrapperCellNode

…ta loading.

Adlai-Holler

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

Adlai-Holler · 2018-03-11T23:32:36Z

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");
  }];

…sactionQueueCount management.

…g ASMainSerialQueue.

…g the real fix before this.

ghost · 2018-03-13T06:14:39Z

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

Generated by 🚫 Danger

appleguy · 2018-03-13T06:22:15Z

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 · 2018-03-13T06:27:25Z

=/ 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 · 2018-03-13T06:40:01Z

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 · 2018-03-13T08:03:28Z

Thanks for all your help Adlai!

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

appleguy self-assigned this Feb 10, 2018

appleguy requested review from Adlai-Holler, maicki and nguyenhuy February 10, 2018 08:08

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

Merge branch 'master' into ASWrapperCellNode

ad7a69d

Adlai-Holler reviewed Feb 22, 2018

View reviewed changes

appleguy added 6 commits February 26, 2018 00:58

Merge branch 'master' into ASWrapperCellNode

f5754be

Merge branch 'master' into ASWrapperCellNode

78d8a27

Merge branch 'ASWrapperCellNode' of https://github.com/TextureGroup/T…

2791710

…exture into ASWrapperCellNode

Fix -[ASDataController clearData] to take no action before initial da…

bdc3b3c

…ta loading.

Empty commit to kick CI

35efb21

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

c2b3f0b

TextureGroup deleted a comment Mar 11, 2018

Merge branch 'master' into ASWrapperCellNode

962609f

Adlai-Holler approved these changes Mar 11, 2018

View reviewed changes

appleguy added 4 commits March 12, 2018 21:51

Merge branch 'master' into ASWrapperCellNode

24a18c9

Tweak ASDataController changes to handle an edge case in _editingTran…

1fb1802

…sactionQueueCount management.

Avoid excess cyclic calls to onDidFinishProcessingUpdates: by avoidin…

de0f437

…g ASMainSerialQueue.

Reverting my initial change as it wasn't the right approach, followin…

1195699

…g the real fix before this.

TextureGroup deleted a comment Mar 13, 2018

appleguy merged commit a41cbb4 into master Mar 13, 2018

appleguy deleted the ASWrapperCellNode branch March 13, 2018 08:03

flovouin mentioned this pull request Apr 10, 2018

[ASDataController] Crash due to new assert in #797 #878

Closed

flovouin mentioned this pull request May 23, 2018

[ASCollectionNode] More info on changes in #797 #936

Open

Adlai-Holler mentioned this pull request Jun 19, 2018

Create an experiment to remove extra collection teardown step #975

Merged

Uh oh!

Conversation

appleguy commented Feb 10, 2018

Uh oh!

appleguy commented Feb 10, 2018

Uh oh!

Adlai-Holler left a comment

Choose a reason for hiding this comment

Uh oh!

Adlai-Holler Feb 22, 2018

Choose a reason for hiding this comment

Uh oh!

appleguy Feb 28, 2018

Choose a reason for hiding this comment

Uh oh!

Adlai-Holler Feb 22, 2018

Choose a reason for hiding this comment

Uh oh!

appleguy Feb 28, 2018

Choose a reason for hiding this comment

Uh oh!

appleguy commented Feb 26, 2018

Uh oh!

Adlai-Holler left a comment

Choose a reason for hiding this comment

Uh oh!

Adlai-Holler commented Mar 11, 2018

Uh oh!

ghost commented Mar 13, 2018

Uh oh!

appleguy commented Mar 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Adlai-Holler commented Mar 13, 2018

Uh oh!

appleguy commented Mar 13, 2018

Uh oh!

appleguy commented Mar 13, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

appleguy commented Mar 13, 2018 •

edited

Loading