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

[ASCollectionNode / ASTableNode] Measure scrollable content size (main thread only due to UICV layout?) #108

Open
garrettmoon opened this issue May 1, 2017 · 10 comments

Comments

@garrettmoon
Copy link
Member

From @tettoffensive on November 4, 2015 1:48

I have an ASTableView which in each row has a custom ASCellNode that encapsulates an ASCollectionNode as well as an ASTextNode label.

If I have the data source cached so that is available on initialization it loads up and works fine. I see all the nodes in my ASCollectionView. However, if I turn the caching off so that I need to wait for the network request to come back and call reloadData on the ASCollectionView at this point. And then call reloadData on the ASTableView, then I do not see the nodes of the ASCollectionView but I do see the labels.

I'm wondering what the practice is to make this work. I've tried different orders of reloadData and also reloadDataImmediately

Maybe someone's built an example project for how to do this properly with a dynamic data source? I have anywhere between 0 and 4 collection views that may be unloaded/loaded depending if they have any nodes in them or not.

Copied from original issue: facebookarchive/AsyncDisplayKit#814

@garrettmoon
Copy link
Member Author

From @nguyenhuy on November 4, 2015 10:54

I'm gonna make a wild guess here, based on the fact that (unlike UICollectionView) ASCollectionView and its underlying ASDataController reload data by discarding all existing nodes and asking for new ones. It may be the case that you set your network response to your cell nodes, call reloadData on the table view and then the new cells don't have their data. If it is indeed the cause of your problem, you can fix it by not calling reloadData but update your cell nodes internally. By "internally", I mean after setting data to a node, you update its text and collection view (inserts?), then call -setNeedsLayout on the node itself. The method triggers a new layout pass on your node and (if you are using v1.9 or later) notifies the table view to update.

Please let me know if this helps, and if you have any feedbacks on your experience using ASDK.

@garrettmoon
Copy link
Member Author

From @tettoffensive on November 7, 2015 0:31

Thanks for your suggestion @nguyenhuy. I am going to have to spend some time building a test project or something to try this out. You may be on to something here, but I also think it may have something to do with the frame of the ASCollectionView. Putting it inside a ASCollectionNode to put it in a cell, I couldn't find a good way to lay it out properly via the calculateSizeThatFits and layout. This is kinda hacky, but I was storing the size of the collection view's frame on initialization since it crashes when you try to access the collectionNode's view in calculateSizeThatFits/layout.

#import "ASCollectionViewCellNode.h"

@interface ASCollectionViewCellNode ()
{
    CGSize _collectionViewSize;
}
@property (nonatomic, copy) ASDisplayNode *header;
@property (nonatomic, copy) ASCollectionNode *collectionNode;
@end

@implementation ASCollectionViewCellNode

- (instancetype)initWithHeader:(ASDisplayNode *)header CollectionView:(ASCollectionNode *)collectionNode
{
    if (!(self = [super init]))
        return nil;

    _header = header;
    _collectionNode = collectionNode;
    _collectionViewSize = collectionNode.view.frame.size;
    [self addSubnode:header];
    [self addSubnode:collectionNode];

    return self;
}

- (CGSize)calculateSizeThatFits:(CGSize)constrainedSize
{
    CGSize headerSize = [_header measure:constrainedSize];
    CGSize collectionSize = _collectionViewSize;
    return CGSizeMake(constrainedSize.width, headerSize.height+collectionSize.height);
}

- (void)layout
{
    [super layout];
    [_collectionNode setFrame:(CGRect){ 0., CGRectGetMaxY(_header.frame), _collectionNode.frame.size }];
}

@end

@garrettmoon
Copy link
Member Author

From @appleguy on November 8, 2015 4:43

@tettoffensive Yes, you're right, this is absolutely an issue. Today, collectionNodes typically need to be calculated without being able to consider their children because they depend on UIKit to drive the layout calls. There are definitely ways we could adjust the framework to fix this — in fact, ASFlowLayoutController already exists and would allow us to do exactly that for UICollectionViewFlowLayout nodes — but it can't work in the general case unless we were to stop supporting UICollectionViewLayout subclasses. There are so many advantages to either way (reusing them is great, but they bring several annoying limitations).

Pinterest will further optimize its grid layout in the future, most likely next spring, and I expect this will involve moving away from UICollectionViewLayout & UICV itself in favor of an ASLayoutController-like implementation (and a new custom class to pair with data & range controllers). That's all pretty far out, but there are much smaller changes that could allow you to call into the ASFlowLayoutController concurrently.

All this said, many of the use cases I expected for ASCollectionNode don't depend on the content size — for example, a typical horizontal scroller inside a vertical table / collection will simply be as wide as the screen and often all the items in it have to have the same height for the UI to look reasonable.

I would be very interested in learning more about your use case, so that I can have a visual mental model of the flexibilities that would have made this properly elegant for you. Bummed the framework let you down here, and definitely a high priority for me to finish polishing the rough edges of the rather new ASViewController and ASCollectionNode.

@garrettmoon
Copy link
Member Author

From @appleguy on November 8, 2015 5:1

This is somewhat related to the concept of sizing ASScrollNodes' content. We can implement a protocol for these to conform to that allows a special layout method to be called to drive the contentSize calculation / setting. It will be tricker for ASCollectionNode, though.

facebookarchive/AsyncDisplayKit#780

@garrettmoon
Copy link
Member Author

From @tettoffensive on November 9, 2015 19:29

@appleguy My use case is a vertical table view with each row being a horizontal collection of custom nodes which are basically images with captions on top of them. Each row also has a custom node which is a label for that collection. Right now, I have those as part of the cell in the table view, but I suppose each row could be a section as well. Right now the design is consistent with height. However, who knows what'll happen with the design. Things are always changing. At one point one of the bottom most of the collection views was 2 columns with vertical scroll.

But for simplicities sake right now I'd just like to get the simple case of horizontal collections inside vertical table view. What you're planning for next spring will be nice, but I'll need to find a good solution now in the meantime.

mockup

@garrettmoon
Copy link
Member Author

From @appleguy on April 4, 2016 0:35

@tettoffensive we have the same design in Pinterest and are able to implement it fairly easily. The solution we use is to request each cell node, call measureWithSizeRange: on them, and take the maximum returned.

If you retain a reference to the cell nodes and then return the same objects when the data sources queried, this can be quite efficient. Often the best time to perform this work is on a background network person thread as soon as the data comes back from the server, perform the layout to understand what the height of the element is and you also get the node creation accomplish in the background.

It does not look like you need the actual content size of the collection, which unfortunately depends on the collection layout and that is a main thread API as restricted by Apple. If you are more interested in the calculated sizes of all of the cell nodes, so that you can perform a maximum on them, this would be far easier to expose in the API.

@garrettmoon
Copy link
Member Author

@appleguy can this issue be closed?

@garrettmoon
Copy link
Member Author

From @appleguy on December 11, 2016 9:9

@garrettmoon No, unfortunately this is still an issue. In fact, I'd say it is easily within the top 3 most frequently requested features. We should open one of those feedback boards where we can have people rank them :).

The use case is as "simple" as having a horizontal collection that goes into a vertical scroll view, and wanting to know the height of the overall collection in consideration of its items. Another is when showing a modal sheet, if it is wrapping a small ASTableNode that may only have 3 or 4 items, calculating the necessary height of the modal can be tough.

@Adlai-Holler had looked into this a bit to see if we could at least provide a well-defined hook that, by its call time, contentSize is set. It seems like even this is difficult, even after controlling for the async measurement operations, but I'm not exactly sure why. I'd suggest we consider this for 2.1 as it is a capability that would come from the data controller & layout work that Adlai is hoping to take on.

@garrettmoon
Copy link
Member Author

I think this is a P2 but definitely should go into 2.1

@garrettmoon
Copy link
Member Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants