[ASCollectionView] Add delegate bridging and index space translation for missing UICollectionViewLayout properties. #440

appleguy · 2017-07-14T06:21:27Z

This is a first attempt at resolving #438.

I know there is already one similar method to do indexPath conversions, and that the #define for
accessing properties on the UICollectionViewFlowLayout should probably be moved somewhere else.

Looking for feedback on the general direction here. In particular, I'm a bit surprised that so much
has changed in how these calls occur, as I believe especially constrainedSizeForNodeAtIndexPath: is
now called with inconsistent index path spaces (but was not before this PR landed in March:
https://github.com/facebookarchive/AsyncDisplayKit/pull/3136/files)

Since the impact of mixing the spaces is fairly serious (can cause crashes), I'm also wondering if
I am misinterpreting some aspects of the code, or if maybe the crashing impact wasn't noticed yet.

…for missing UICollectionViewLayout properties This is a first attempt at resolving #438. I know there is already one similar method to do indexPath conversions, and that the #define for accessing properties on the UICollectionViewFlowLayout should probably be moved somewhere else. Looking for feedback on the general direction here. In particular, I'm a bit surprised that so much has changed in how these calls occur, as I believe especially constrainedSizeForNodeAtIndexPath: is now called with inconsistent index path spaces (but was not before this PR landed in March: https://github.com/facebookarchive/AsyncDisplayKit/pull/3136/files) Since the impact of mixing the spaces is fairly serious (can cause crashes), I'm also wondering if I am misinterpreting some aspects of the code, or if maybe the crashing impact wasn't noticed yet.

ghost · 2017-07-14T06:22:26Z

🚫 CI failed with log

nguyenhuy

I believe it is indeed a problem. It's never safe to ask the data controller using an index path in UIKit space. The change in this PR looks safe to me.

I think we need to have new GitHub issues to track and discuss each of the followings:

Have a mechanism in place that can detect and report illegal index-space crossings. I'm not sure, but it might be as simple as having an NSIndexPath wrapper that contains extra information regarding the index space.
Do an audit on the current code base and fix problems similar to this.
Discuss a new API for invalidating size ranges.

@Adlai-Holler Interested in your thoughts too.

ghost · 2017-07-14T13:22:36Z

🚫 CI failed with log

nguyenhuy · 2017-07-14T14:40:22Z

Source/ASCollectionView.mm

@@ -949,7 +955,8 @@ - (NSInteger)collectionView:(UICollectionView *)collectionView numberOfItemsInSe
  return [_dataController.visibleMap numberOfItemsInSection:section];
 }

- (CGSize)collectionView:(UICollectionView *)collectionView layout:(UICollectionViewLayout *)collectionViewLayout sizeForItemAtIndexPath:(NSIndexPath *)indexPath
+- (CGSize)collectionView:(UICollectionView *)collectionView
+                  layout:(UICollectionViewLayout *)layout sizeForItemAtIndexPath:(NSIndexPath *)indexPath


Since you changed the parameter name from collectionViewLayout to layout, you need to update this line.

nguyenhuy · 2017-07-14T15:14:26Z

Source/ASCollectionView.mm

-  if (supplementaryKind == nil) {
+  if (indexPath == nil) {
+    // In this case, the latest data no longer has the element, so we can't ask the delegate for its latest constrained size.
+    // The element is still visible, but will be deleted soon. We can reuse the last-known constrainedSize for the element.


The element is still visible

"visible" is quite misleading here, and so does the "vibisbleMap" name. We should change it to "current" or something similar that indicates that the element is still recognized by UIKit objects.

Sounds good. I agree that if we clarified the terminology here, and used it consistently everywhere (and we could even start putting it in method names, or comments at the start of methods), it would be easier to ensure correctness.

Ideas for "UIKit" space:

View

UIKit

Committed

Ideas for "Latest" space:

Source

App

DataSource

Latest

I feel like there must be a better, more parallel set of names. One trouble with "current" is that you could imagine that being the current UIKit state, or the current App state.

UIDataSourceTranslating uses "Presentation." I kind of like that name.

"DataSource" and "Presentation", or "Processed" and "Presenting"?

appleguy · 2017-07-14T17:48:42Z

@nguyenhuy I'm really glad you mentioned the NSIndexPath wrapper idea. This is exactly what I had in mind, too. We should make the data types incompatible to cross the index-space boundary!

@Adlai-Holler I'd like your feedback on this, but I think we should at least consider using a simple struct (exactly like ASIndexPath) for this purpose. Index path handling remains one of our biggest costs, including specifically main thread costs during large collection update application cycles that are the periods of time where we see frame drops.

There might be an easier way to distinguish between the index spaces using NSIndexPath (although I'm hesitant to use an associated value, even a non-object integer). That said, it would be interesting to consider what the "best" way is to represent these values if we weren't tied to NSIndexPath.

Adlai-Holler · 2017-07-14T17:52:47Z

@appleguy @nguyenhuy I'm opposed to migrating to a struct because on 64-bit, NSIndexPath is a tagged pointer, and it's absolutely ubiquitous when dealing with UIKit.

To prevent illegal index space crossings, I believe the best solution is for us to modify our APIs not to use index paths so often. For example, this delegate method should probably be changed to constrainedSizeForNode: and you can read the current data-source-indexpath, supplementary element kind, and visible-indexpath safely on your end if it's relevant.

UIKit is restricted always to using NSIndexPaths because their API doesn't reify elements. We do. We have ASCollectionElement and ASCellNode which both serve as better identifiers for content than index paths, and from which we can always derive the appropriate index path in a desired space.

appleguy · 2017-07-14T18:00:01Z

@Adlai-Holler - ok, good points. Keeping NSIndexPath where we need to use index-based references at all.

I think we are all supportive of using elements as widely as possible. However, we still need a way to keep the index spaces straight and physically prevent / assert against problems like the changing behavior of the constrainedSizeForNode: call (since this had been in the codebase for a while, it's an indication that some kind of assertion would've helped us catch it much sooner).

Another challenge with relying on elements is that some APIs just need to use indexPaths. This includes UICollectionView passthrough. Although we shouldn't let that influence the node-based API, the technical requirements of keeping the index spaces straight are quite limited because of how great the changeSet / elementMap infrastructure is, allowing translation between them. So just like the tweaks in this patch, making sure the delegate gets sane values is more a matter of verifying correctness than a tough implementation challenge.

Any feedback on this? Would an assertion to guard our assumptions be useful?

Adlai-Holler

This isn't beautiful, but it improves the reliability of using volatile data sources with flow layouts so I think we should roll with it. Thanks @appleguy for building this patch and following through on it.

Adlai-Holler · 2017-07-14T18:08:21Z

Source/ASCollectionView.mm

+  NSIndexPath *sectionIndexPath = [NSIndexPath indexPathForItem:0 inSection:section];
+  ASCollectionElement *element = [_dataController.visibleMap elementForItemAtIndexPath:sectionIndexPath];
+  NSIndexPath *pendingIndexPath = element ? [_dataController.pendingMap indexPathForElement:element] : nil;
+  return pendingIndexPath;


Should this be - (NSInteger)dataSourceSectionForSection:(NSInteger)section? I could see not-checking-NSNotFound on the caller side being a problem. Not checking nil is roughly as likely to occur but slightly less likely to cause issues immediately.

Currently ASSection objects are only created if the user implements the optional contextForSection: API. We should instead create ASSections unconditionally and use them as identifiers, similar to ASCollectionElement. That way looking up the indexes of sections between maps is easy!!

Adlai-Holler · 2017-07-14T18:09:31Z

Source/ASCollectionView.mm

-  if (supplementaryKind == nil) {
+  if (indexPath == nil) {
+    // In this case, the latest data no longer has the element, so we can't ask the delegate for its latest constrained size.
+    // The element is still visible, but will be deleted soon. We can reuse the last-known constrainedSize for the element.


UIDataSourceTranslating uses "Presentation." I kind of like that name.

nguyenhuy · 2017-07-14T19:14:57Z

Regarding preventing index space crossings, although I totally understand the reasons behind, I'm not so keen on changing the API. For the time being, adding assertions sounds fine, as long as those assertions always trigger, as opposed to trigger only after some race conditions (e.g different visible and pending maps).

appleguy · 2017-07-14T20:48:53Z

Model and Presentation could be good! It would mirror CALayer.

…

On Fri, Jul 14, 2017 at 12:14 PM Huy Nguyen ***@***.***> wrote: Regarding preventing index space crossings, although I totally understand the reasons behind, I'm not so keen on changing the API. For the time being, adding assertions sounds fine, as long as those assertions always trigger, without requiring race conditions (e.g different visible and pending maps) to occur beforehand. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#440 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAigA1j-PbU6JJHzFHvj9DpnL6e60Hp9ks5sN74zgaJpZM4OX35Q> .

appleguy · 2017-07-15T06:53:37Z

@nguyenhuy @Adlai-Holler I found another method that needs index-space conversion (layout:sizeForItem: itself). I have a fix for this locally, but haven't updated this patch yet.

Thanks for the feedback here. I need to focus on fixing locally to address a release in flight. Feel free to continue landing diffs (although I would appreciate if we can hold off for a few days on any non-functional changes such as naming updates) and I will check which changes are still needed once rebasing.

…xes.

appleguy · 2017-07-17T03:13:39Z

@nguyenhuy @Adlai-Holler I worked on this throughout the weekend, and it's pretty heavily tested now. If it passes your review, feel free to merge it. Thanks for your help!

nguyenhuy · 2017-07-17T11:22:48Z

Source/ASCollectionView.mm

  } else {
-    sizeRange = [self dataController:_dataController constrainedSizeForSupplementaryNodeOfKind:supplementaryKind atIndexPath:indexPath];
+    return [node layoutThatFits:element.constrainedSize].size;


Ok, so we no longer reach out to the delegate for a size range but rely on element.constrainedSize. This seems like a simpler approach 👍

It also means once a size range is returned by the delegate, it's no longer mutable by clients. They have to reload the element to change it, or request for a new API which simply exposes -[ASDataController relayoutNodes:nodesSizeChanged:]. Until then, let's get this rolling.

…for missing UICollectionViewLayout properties. (TextureGroup#440) * [ASCollectionView] Add delegate bridging and index space translation for missing UICollectionViewLayout properties This is a first attempt at resolving TextureGroup#438. I know there is already one similar method to do indexPath conversions, and that the #define for accessing properties on the UICollectionViewFlowLayout should probably be moved somewhere else. Looking for feedback on the general direction here. In particular, I'm a bit surprised that so much has changed in how these calls occur, as I believe especially constrainedSizeForNodeAtIndexPath: is now called with inconsistent index path spaces (but was not before this PR landed in March: https://github.com/facebookarchive/AsyncDisplayKit/pull/3136/files) Since the impact of mixing the spaces is fairly serious (can cause crashes), I'm also wondering if I am misinterpreting some aspects of the code, or if maybe the crashing impact wasn't noticed yet. * [ASCollectionView] Cleanup and final changes for UIKit passthrough fixes.

appleguy added the Collection Table Pager label Jul 14, 2017

appleguy self-assigned this Jul 14, 2017

appleguy requested review from nguyenhuy and Adlai-Holler July 14, 2017 06:21

nguyenhuy approved these changes Jul 14, 2017

View reviewed changes

nguyenhuy reviewed Jul 14, 2017

View reviewed changes

nguyenhuy mentioned this pull request Jul 14, 2017

[ASDataController] Avoid asking for size ranges of soon-to-be-delete elements during relayouts #442

Merged

Adlai-Holler approved these changes Jul 14, 2017

View reviewed changes

appleguy added 2 commits July 16, 2017 20:03

Merge branch 'master' into CollectionIndicies

5faa608

[ASCollectionView] Cleanup and final changes for UIKit passthrough fi…

eaf8ad1

…xes.

nguyenhuy approved these changes Jul 17, 2017

View reviewed changes

nguyenhuy merged commit 5115f66 into master Jul 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ASCollectionView] Add delegate bridging and index space translation for missing UICollectionViewLayout properties. #440

[ASCollectionView] Add delegate bridging and index space translation for missing UICollectionViewLayout properties. #440

appleguy commented Jul 14, 2017

ghost commented Jul 14, 2017

nguyenhuy left a comment •

edited

Loading

ghost commented Jul 14, 2017

nguyenhuy Jul 14, 2017

nguyenhuy Jul 14, 2017

appleguy Jul 14, 2017

Adlai-Holler Jul 14, 2017

nguyenhuy Jul 14, 2017

appleguy commented Jul 14, 2017

Adlai-Holler commented Jul 14, 2017

appleguy commented Jul 14, 2017

Adlai-Holler left a comment

Adlai-Holler Jul 14, 2017

Adlai-Holler Jul 14, 2017

nguyenhuy commented Jul 14, 2017 •

edited

Loading

appleguy commented Jul 14, 2017 via email

appleguy commented Jul 15, 2017

appleguy commented Jul 17, 2017

nguyenhuy Jul 17, 2017

[ASCollectionView] Add delegate bridging and index space translation for missing UICollectionViewLayout properties. #440

[ASCollectionView] Add delegate bridging and index space translation for missing UICollectionViewLayout properties. #440

Conversation

appleguy commented Jul 14, 2017

ghost commented Jul 14, 2017

nguyenhuy left a comment • edited Loading

Choose a reason for hiding this comment

ghost commented Jul 14, 2017

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

appleguy commented Jul 14, 2017

Adlai-Holler commented Jul 14, 2017

appleguy commented Jul 14, 2017

Adlai-Holler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nguyenhuy commented Jul 14, 2017 • edited Loading

appleguy commented Jul 14, 2017 via email

appleguy commented Jul 15, 2017

appleguy commented Jul 17, 2017

Choose a reason for hiding this comment

nguyenhuy left a comment •

edited

Loading

nguyenhuy commented Jul 14, 2017 •

edited

Loading