[Umbrella] ASCollectionView -> ASCollectionNode Migration, Separate Index Spaces #2372

Adlai-Holler · 2016-10-12T19:51:17Z

I've tested this thoroughly with Pinterest using the deprecated API.

Supersedes #2348 .

Methods involving the ASCollectionNode always use data source index space. Methods on the view always use UIKit index space. I may remove nodeForRowAtIndexPath and indexPathForNode from ASTableView to avoid confusion but I'm still thinking about possibly legitimate use cases for them such as interactive reordering and scrollToRowAtIndexPath: or after they call indexPathForCell: for some reason, which they shouldn't do but we can't stop them.

Adlai-Holler · 2016-10-12T22:58:49Z

AsyncDisplayKit.xcodeproj/project.pbxproj

@@ -440,6 +440,7 @@
 		CC7FD9DF1BB5E962005CCB2B /* ASPhotosFrameworkImageRequest.m in Sources */ = {isa = PBXBuildFile; fileRef = CC7FD9DD1BB5E962005CCB2B /* ASPhotosFrameworkImageRequest.m */; };
 		CC7FD9E11BB5F750005CCB2B /* ASPhotosFrameworkImageRequestTests.m in Sources */ = {isa = PBXBuildFile; fileRef = CC7FD9E01BB5F750005CCB2B /* ASPhotosFrameworkImageRequestTests.m */; };
 		CC7FD9E21BB603FF005CCB2B /* ASPhotosFrameworkImageRequest.h in Headers */ = {isa = PBXBuildFile; fileRef = CC7FD9DC1BB5E962005CCB2B /* ASPhotosFrameworkImageRequest.h */; settings = {ATTRIBUTES = (Public, ); }; };
+		CC87BB951DA8193C0090E380 /* ASCellNode+Internal.h in Headers */ = {isa = PBXBuildFile; fileRef = CC87BB941DA8193C0090E380 /* ASCellNode+Internal.h */; };


This file existed before, but wasn't part of the project for some reason.

Adlai-Holler · 2016-10-12T22:59:27Z

AsyncDisplayKit/ASCellNode.mm

+    NSIndexPath *ip = [(ASCollectionNode *)owningNode indexPathForNode:self];
+    if (ip != nil) {
+      [result addObject:@{ @"indexPath" : ip }];
+    }


This is sort of unrelated and should have been here before. Really helps logging.

Adlai-Holler · 2016-10-12T23:20:10Z

AsyncDisplayKit/ASCollectionNode.h

+/**
+ * This is a node-based UICollectionViewDataSource.
+ */
+@protocol ASCollectionDataSource <ASCommonCollectionDataSource>


This (and ASCollectionDelegate ASTableDataSource and ASTableDelegate) was kept as-is, but moved from the View.h file to the Node.h file.

Adlai-Holler · 2016-10-12T23:24:58Z

AsyncDisplayKit/ASCollectionView.mm

+  return [_dataController nodeAtCompletedIndexPath:indexPath];
+}
+
+- (NSIndexPath *)convertIndexPathFromCollectionNode:(NSIndexPath *)indexPath waitingIfNeeded:(BOOL)wait


This is private, and only here for backwards-compatibility reasons. Currently the scrollToItemAtIndexPath methods on ASTableView and ASCollectionView take index paths in the data source space, and so for the time being we use this as a shim to maintain that behavior. In the future, after some testing, we'll remove our override on that method, and remove this with it.

Adlai-Holler · 2016-10-13T00:19:46Z

AsyncDisplayKit/ASCollectionNode.h

+- (void)collectionNode:(ASCollectionNode *)collectionNode didEndDisplayingItemWithNode:(ASCellNode *)node;
+
+- (BOOL)collectionNode:(ASCollectionNode *)collectionNode shouldHighlightItemWithNode:(ASCellNode *)node;
+- (void)collectionNode:(ASCollectionNode *)collectionNode didHighlightItemWithNode:(ASCellNode *)node;


This was a tough question. I could have gone with didSelectItemAtIndexPath etc but since the item/section may have been deleted or reloaded, the argument would have to be nullable which would be really wacky, giving users no information at all. I could filter out cases like that, but there could be legitimate arguments for behaviors like "whenever a header node of X class is showing, the background color of my view should be Y" so filtering out these callbacks would break that. Or if the user happened to select an item just before a section is reloaded, the select would get ignored.

So the user can either 1) build their architecture around nodes directly, and check the node class or 2) call [collectionNode indexPathForNode:node]. The only trouble here is, if they don't nil-check the index path, they'll get {0,0} for item and section, and they may do something even worse! Tough calls… @appleguy @maicki

Well, I've reversed stance on this. In order to keep things simple, all delegate callbacks except willDisplayRow and didEndDisplayingRow will pass a nonnull NSIndexPath *. In the extremely unlikely case that the user selected an item that is pending deletion/reload, the event won't get sent.

garrettmoon · 2016-10-13T20:15:13Z

AsyncDisplayKit/ASCollectionNode.h

+ *                    Boolean parameter that contains the value YES if all of the related animations completed successfully or
+ *                    NO if they were interrupted. This parameter may be nil. If supplied, the block is run on the main thread.
+ */
+- (void)performBatchAnimated:(BOOL)animated updates:(nullable __attribute((noescape)) void (^)())updates completion:(nullable void (^)(BOOL finished))completion;


Can we add back in performBatchUpdates without the animated flag?

garrettmoon · 2016-10-13T20:18:21Z

AsyncDisplayKit/ASCollectionNode.h

+/**
+ * Asks the data source for the number of items in the given section of the collection node.
+ *
+ * @see @c numberOfSectionsInCollectionView:


garrettmoon · 2016-10-13T20:24:27Z

AsyncDisplayKit/ASCollectionView.mm

@@ -209,33 +238,19 @@ + (Class)layerClass

 - (instancetype)initWithCollectionViewLayout:(UICollectionViewLayout *)layout
 {
-  return [self _initWithFrame:CGRectZero collectionViewLayout:layout ownedByNode:NO];
+  return [self _initWithFrame:CGRectZero collectionViewLayout:layout layoutFacilitator:nil];


Super nit: can this call through into initWithFrame:

hannahmbanana · 2016-10-13T21:44:54Z

@Adlai-Holler: Want me to push these example project updates?

ASCollectionView
ASDKgram
ASDKTube
CatDealsCollectionView
CustomCollectionView
HorizontalWithinVerticalScrolling

Adlai-Holler · 2016-10-13T22:07:27Z

@hannahmbanana Sure – feel free to push them onto this branch.

These examples were updated to use ASCollectionNode instead of ASCollectionView as part of PR #2372

hannahmbanana · 2016-10-14T04:03:08Z

@Adlai-Holler: could you review the example file updates?

Beef up our supplementary node support Make the API way better Go nuts Add a unit test for UICollectionView's handling of reloadData inside batch updates Wrap indexPathForNode: in a cache Convert index paths in delegate methods Go back on table view Put collection view back Switch up the API Move most ASCollectionView API to ASCollectionNode Move most table logic over to ASTableNode Do the things More conversion work Keep on keepin' on Get table view delegate API done More porting Simplify Clear the delegate More cleanup Move more stuff around Remove pointless file Re-add some API Put back more API Use the right flag

…expected)

Adlai-Holler · 2016-10-14T17:16:34Z

examples/ASDKgram/Sample/PhotoFeedNodeController.m

@@ -192,7 +192,7 @@ - (void)tableView:(ASTableView *)tableView willBeginBatchFetchWithContext:(ASBat
 - (void)resetAllData
 {
  [_photoFeed clearFeed];


@hannahmbanana Change tableView:willBeginBatchFetchWithContext: above to tableNode:willBeginBatchFetchWithContext:. Is that not showing a deprecation warning? Bummer.

Also change the other ASTableDelegate methods to tableNode:numberOfRowsInSection: etc.

Adlai-Holler · 2016-10-14T17:25:18Z

examples/ASCollectionView/Sample/ViewController.m

@@ -92,6 +96,8 @@ - (void)viewDidLoad

 - (void)reloadTapped
 {
+  // This method is deprecated because we reccommend using ASCollectionNode instead of ASCollectionView.
+  // This functionality & example project remains for users who insist on using ASCollectionView.


@hannahmbanana Since ASCollectionView outside of the node isn't going to be supported for long, this sample should either be changed majorly or discarded entirely.

hannahmbanana · 2016-10-14T23:49:43Z

@Adlai-Holler: There's a bug with ASTableNode / nodeForRowAtIndexPath: not getting called.

ASTableView.mm (line 1389) should be this (the if and elseif: were checking the same flag)
} else if (_asyncDataSourceFlags.tableNodeNodeForRow) {

I'll push the fix when I push the example updates.

Adlai-Holler · 2016-10-15T00:21:13Z

Here we go!

Adlai-Holler added Needs Review ASCollectionView ASTableView API Change labels Oct 12, 2016

Adlai-Holler added this to the 2.0 milestone Oct 12, 2016

facebook-github-bot added the CLA Signed label Oct 12, 2016

Adlai-Holler force-pushed the AHDeprecateTableCollectionView branch from 4b8a691 to 57de591 Compare October 12, 2016 22:58

Adlai-Holler commented Oct 12, 2016

View reviewed changes

facebook-github-bot added the CLA Signed label Oct 12, 2016

Adlai-Holler force-pushed the AHDeprecateTableCollectionView branch from 57de591 to dc0c956 Compare October 12, 2016 23:17

Adlai-Holler commented Oct 12, 2016

View reviewed changes

facebook-github-bot added the CLA Signed label Oct 12, 2016

Adlai-Holler commented Oct 12, 2016

View reviewed changes

Adlai-Holler commented Oct 13, 2016

View reviewed changes

facebook-github-bot added the CLA Signed label Oct 13, 2016

Adlai-Holler force-pushed the AHDeprecateTableCollectionView branch from 59ba5ab to 55d787f Compare October 13, 2016 16:54

facebook-github-bot added the CLA Signed label Oct 13, 2016

garrettmoon reviewed Oct 13, 2016

View reviewed changes

facebook-github-bot added the CLA Signed label Oct 13, 2016

hannahmbanana added a commit that referenced this pull request Oct 14, 2016

[Examples] rename CatDealsCollectionView & CustomCollectionView

e7453b1

These examples were updated to use ASCollectionNode instead of ASCollectionView as part of PR #2372

facebook-github-bot added the CLA Signed label Oct 14, 2016

hannahmbanana mentioned this pull request Oct 14, 2016

[Examples] Layout 2.0 updates: Switch YES to 1.0 for flexGrow and flexShrink #2382

Merged

facebook-github-bot added the CLA Signed label Oct 14, 2016

Adlai Holler and others added 2 commits October 14, 2016 10:01

Some cleanup

130b830

Adlai-Holler and others added 6 commits October 14, 2016 10:01

Remove incorrect comment

ff3eeb9

Tweak the API

70cd643

Put back a couple methods

7313ca8

update example projects (note: ASCollectionView deprecation warnings …

84ad640

…expected)

change reloadDataWithCompletion:nil --> reloadData

a86ab5f

Clean up rebase

372f2ca

Adlai-Holler force-pushed the AHDeprecateTableCollectionView branch from f867ccc to 372f2ca Compare October 14, 2016 17:02

facebook-github-bot added the CLA Signed label Oct 14, 2016

Adlai-Holler commented Oct 14, 2016

View reviewed changes

facebook-github-bot added the CLA Signed label Oct 14, 2016

Adlai-Holler commented Oct 14, 2016

View reviewed changes

facebook-github-bot added the CLA Signed label Oct 14, 2016

Make deprecated numberOfItemsInSection methods optional

2444505

hannahmbanana force-pushed the AHDeprecateTableCollectionView branch from 40c49d8 to 2444505 Compare October 14, 2016 22:21

facebook-github-bot added the CLA Signed label Oct 14, 2016

facebook-github-bot added the CLA Signed label Oct 15, 2016

Adlai-Holler and others added 3 commits October 14, 2016 17:10

Use the right flag

5e8d871

Address nits

8c82f35

update ASDKTube, ASDKgram & ASViewController examples

7c3c565

Adlai-Holler merged commit c2ea7cf into master Oct 15, 2016

Adlai-Holler deleted the AHDeprecateTableCollectionView branch October 15, 2016 00:21

hannahmbanana mentioned this pull request Oct 15, 2016

[Examples] 2.0 Collection/Table API updates #2390

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Umbrella] ASCollectionView -> ASCollectionNode Migration, Separate Index Spaces #2372

[Umbrella] ASCollectionView -> ASCollectionNode Migration, Separate Index Spaces #2372

Adlai-Holler commented Oct 12, 2016 •

edited

Loading

Adlai-Holler Oct 12, 2016

Adlai-Holler Oct 12, 2016

Adlai-Holler Oct 12, 2016

Adlai-Holler Oct 12, 2016

Adlai-Holler Oct 13, 2016 •

edited

Loading

Adlai-Holler Oct 13, 2016

garrettmoon Oct 13, 2016

Adlai-Holler Oct 14, 2016

garrettmoon Oct 13, 2016

garrettmoon Oct 13, 2016

hannahmbanana commented Oct 13, 2016 •

edited

Loading

Adlai-Holler commented Oct 13, 2016

hannahmbanana commented Oct 14, 2016

Adlai-Holler Oct 14, 2016

Adlai-Holler Oct 14, 2016

Adlai-Holler Oct 14, 2016

hannahmbanana commented Oct 14, 2016 •

edited

Loading

Adlai-Holler commented Oct 15, 2016

[Umbrella] ASCollectionView -> ASCollectionNode Migration, Separate Index Spaces #2372

[Umbrella] ASCollectionView -> ASCollectionNode Migration, Separate Index Spaces #2372

Conversation

Adlai-Holler commented Oct 12, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Adlai-Holler Oct 13, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hannahmbanana commented Oct 13, 2016 • edited Loading

Adlai-Holler commented Oct 13, 2016

hannahmbanana commented Oct 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hannahmbanana commented Oct 14, 2016 • edited Loading

Adlai-Holler commented Oct 15, 2016

Adlai-Holler commented Oct 12, 2016 •

edited

Loading

Adlai-Holler Oct 13, 2016 •

edited

Loading

hannahmbanana commented Oct 13, 2016 •

edited

Loading

hannahmbanana commented Oct 14, 2016 •

edited

Loading