[ASCollectionView] Separate indexPath Spaces More Clearly #2348

Adlai-Holler · 2016-10-05T21:48:05Z

Users are getting confused when we tell them "the index paths are out-of-sync" or "don't line up."

To help tackle this major problem, this diff:

Add failing unit tests for the new behavior.
Makes nodeAtIndexPath and indexPathForNode work in the data source's index space, not in the UIKit index space. So immediately after submitting an update, if you call nodeAtIndexPath: you'll get the new node.
Deprecate calculatedSizeForNodeAtIndexPath: we should let them get the node and call calculatedSize instead – now that the index space distinction is more clear we should centralize all our logic on nodes instead.
Removes the indexPath argument from collectionView:willDisplayNode:forItemAtIndexPath: and friends.
- The indexPath was provided by the collection view and would not be correct for accessing their data source.
- They can always call indexPathForNode: to get the data-source index path for the node, if needed. If the item for the node has been deleted from the collection view i.e. no longer exists in the data source, they'll get nil back. It's a bummer, but c'est la vie. The delegate will probably still want to know that a given node is being displayed, even if it no longer corresponds to an item.
Add some assertions about what happens on main vs on editing queue. These are all pretty safe I believe, and are mostly there for documentation/regression-proofing.
Add supplementary nodes into our collection view tests.

NOTE: I have added a unit test for similar behavior to be applied to supplementary nodes. This diff is already big enough just handling it for rows/items, so the unit test is disabled and I will enable it + the feature in a follow-up diff.

Ready for review @nguyenhuy @appleguy @maicki @garrettmoon !

Adlai-Holler · 2016-10-05T21:53:08Z

Hmmm I need to adjust this to publicly support UIKit index paths for the sake of the UICollectionViewLayout. Kicking back to development in progress.

Adlai-Holler · 2016-10-05T22:09:28Z

@appleguy @maicki @garrettmoon Well, the plan to completely hide UIKit index paths is a no-go because UICollectionViewLayout subclasses will want to get nodes in their index space, which is the UIKit index space.

So if we have to expose them, we should come up with a public-facing name for the "completed" set or the "UIKit" set. The current implementation uses the word Completed which is pretty to look at but somewhat vague. We should obviously beef up the documentation about this, but simultaneously come up with good consistent words for these two spaces.

Ideas?

maicki

Not sure about completedIndexPathForNode: but overall PR looks good

maicki · 2016-10-06T20:25:41Z

AsyncDisplayKit/ASCollectionView.h

+ *
+ * @param indexPath The index path for the node of interest.
+ *
+ * @deprecated Call @c calculatedSize on the node of interest instead.


Can you add the version number we deprecated the method

Ooooh! Good call! We should start doing this.

garrettmoon · 2016-10-06T22:51:37Z

AsyncDisplayKit/ASCollectionView.h

@@ -298,11 +302,16 @@ NS_ASSUME_NONNULL_BEGIN
 /**
 * Similar to -indexPathForCell:.
 *
- * @param cellNode a cellNode part of the table view
+ * @param cellNode a cellNode part of the collection view


"a cellNode in the collection view"?

garrettmoon · 2016-10-06T22:56:57Z

AsyncDisplayKit/ASCollectionView.h

+ *   passed into this method may not correspond to the same item in your data source
+ *   if your data source has been updated since the last edit was processed.
+ */
+- (void)collectionView:(ASCollectionView *)collectionView willDisplayNode:(ASCellNode *)node forItemAtIndexPath:(NSIndexPath *)indexPath ASDISPLAYNODE_DEPRECATED;


Should we have information here like:

"@see collectionView:willDisplayNode: instead. If you need the index path, call indexPathForNode and pass in node"

garrettmoon · 2016-10-06T22:57:06Z

AsyncDisplayKit/ASCollectionView.h

+ * @param node The node which was removed from the view hierarchy.
+ * @param indexPath The index path at which the node was located before it was removed.
+ *
+ * @warning AsyncDisplayKit processes collection view edits asynchronously. The index path


Same question here.

garrettmoon · 2016-10-06T22:57:38Z

AsyncDisplayKit/ASCollectionView.mm

+    }
+    _asyncDelegateFlags.asyncDelegateCollectionViewDidEndDisplayingNode = [_asyncDelegate respondsToSelector:@selector(collectionView:didEndDisplayingNode:)];
+    if (_asyncDelegateFlags.asyncDelegateCollectionViewDidEndDisplayingNode == NO) {
+      _asyncDelegateFlags.asyncDelegateCollectionViewDidEndDisplayingNodeForItemAtIndexPath = [_asyncDelegate respondsToSelector:@selector(collectionView:didEndDisplayingNode:forItemAtIndexPath:)];


Dang these are getting crazy :( Wonder if there's some automatic thing we could do (future thought)

garrettmoon · 2016-10-06T23:00:29Z

AsyncDisplayKit/ASCollectionView.mm

@@ -1003,6 +1023,7 @@ - (ASCellNode *)dataController:(ASCollectionDataController *)dataController supp
  return node;
 }

+// TODO: Lock this


This looks like it should be to-done before we land.

Nah, this was always wrong but I noticed it while building this

garrettmoon · 2016-10-06T23:07:31Z

AsyncDisplayKit/ASTableView.h

 *
- * @return a node for display at this indexpath.
+ * @param indexPath The index path of the requested node.


Should this have an @return

So this is not in the datasource index space? Maybe that should be mentioned?

garrettmoon · 2016-10-06T23:10:53Z

AsyncDisplayKit/ASTableView.h

 */
- (ASCellNode *)nodeForRowAtIndexPath:(NSIndexPath *)indexPath;
+- (nullable ASCellNode *)nodeForRowAtCompletedIndexPath:(NSIndexPath *)indexPath;


nodeForRowAtAsynchronousIndexPath ? Not any clearer without documentation but…

Adlai-Holler · 2016-10-07T18:57:14Z

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 in the folder, but not in the project before. Strange.

Adlai-Holler · 2016-10-07T18:58:56Z

OK I've updated the implementation, going with a much cleaner API. The nodeAtCompletedIndexPath language is still used internally, in the data controller. The term completed is understood in that context so I'm just going to leave it for now.

appleguy · 2016-10-11T17:54:00Z

@Adlai-Holler here's the Jenkins failure:

10:38:16 ▸ Compiling ASCollectionView+Internal.m
10:38:16
10:38:16 ❌ /Users/engineering/jenkins_root/workspace/AsyncDisplayKit-PRs/AsyncDisplayKit/Private/ASCollectionView+Internal.m:9:9: 'ASCollectionView+Internal.h' file not found
10:38:16
10:38:16 #import "ASCollectionView+Internal.h"
10:38:16 ^
10:38:16
10:38:16
10:38:16 ** BUILD FAILED **
10:38:16
10:38:16
10:38:16 The following build commands failed:
10:38:16 CompileC /Users/engineering/Build/Intermediates/Pods.build/Debug-iphonesimulator/AsyncDisplayKit.build/Objects-normal/x86_64/ASCollectionView+Internal.o /Users/engineering/jenkins_root/workspace/AsyncDisplayKit-PRs/AsyncDisplayKit/Private/ASCollectionView+Internal.m normal x86_64 objective-c com.apple.compilers.llvm.clang.1_0.compiler
10:38:16 (1 failure)

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

Adlai-Holler · 2016-10-11T20:40:04Z

Thanks @appleguy! The scope of this has changed radically and it's under re-development. Closing and will be superseded.

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

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

Adlai-Holler added Development in Progress and removed Needs Review labels Oct 5, 2016

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

Adlai-Holler added Needs Review and removed Development in Progress labels Oct 5, 2016

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

maicki approved these changes Oct 6, 2016

View reviewed changes

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

garrettmoon reviewed Oct 6, 2016

View reviewed changes

Adlai-Holler force-pushed the AHNodeAtIndexPathAlwaysWorks branch from 08b03ab to 497bb73 Compare October 7, 2016 17:19

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

Adlai-Holler force-pushed the AHNodeAtIndexPathAlwaysWorks branch from bee866d to e983e3a Compare October 7, 2016 18:56

Adlai-Holler commented Oct 7, 2016

View reviewed changes

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

Adlai-Holler force-pushed the AHNodeAtIndexPathAlwaysWorks branch from f748c6b to da63b90 Compare October 10, 2016 18:04

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

Adlai-Holler force-pushed the AHNodeAtIndexPathAlwaysWorks branch from da63b90 to 69f809a Compare October 10, 2016 18:16

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

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

Adlai Holler added 3 commits October 11, 2016 13:39

Switch up the API

e284404

Move most ASCollectionView API to ASCollectionNode

2a58bb5

Adlai Holler added 6 commits October 11, 2016 13:39

Move most table logic over to ASTableNode

465029a

Do the things

d5f33fe

More conversion work

fa64ec8

Keep on keepin' on

ab9b7a1

Get table view delegate API done

dc6363c

More porting

3fe29e8

Adlai-Holler force-pushed the AHNodeAtIndexPathAlwaysWorks branch from 2191cbb to 3fe29e8 Compare October 11, 2016 20:39

Adlai-Holler closed this Oct 11, 2016

Adlai-Holler mentioned this pull request Oct 12, 2016

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ASCollectionView] Separate indexPath Spaces More Clearly #2348

[ASCollectionView] Separate indexPath Spaces More Clearly #2348

Adlai-Holler commented Oct 5, 2016 •

edited

Loading

Adlai-Holler commented Oct 5, 2016

Adlai-Holler commented Oct 5, 2016

maicki left a comment

maicki Oct 6, 2016

garrettmoon Oct 6, 2016

garrettmoon Oct 6, 2016

garrettmoon Oct 6, 2016

garrettmoon Oct 6, 2016

garrettmoon Oct 6, 2016

garrettmoon Oct 6, 2016

Adlai-Holler Oct 7, 2016

garrettmoon Oct 6, 2016

garrettmoon Oct 6, 2016

Adlai-Holler Oct 7, 2016

Adlai-Holler commented Oct 7, 2016

appleguy commented Oct 11, 2016

Adlai-Holler commented Oct 11, 2016

[ASCollectionView] Separate indexPath Spaces More Clearly #2348

[ASCollectionView] Separate indexPath Spaces More Clearly #2348

Conversation

Adlai-Holler commented Oct 5, 2016 • edited Loading

Adlai-Holler commented Oct 5, 2016

Adlai-Holler commented Oct 5, 2016

maicki left a 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

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

Choose a reason for hiding this comment

Adlai-Holler commented Oct 7, 2016

appleguy commented Oct 11, 2016

Adlai-Holler commented Oct 11, 2016

Adlai-Holler commented Oct 5, 2016 •

edited

Loading