IGListKit icon indicating copy to clipboard operation
IGListKit copied to clipboard

[Automatic cell sizing] Master task for auto-sizing cells / estimated cell sizing and auto-sizing supplementary views

Open jessesquires opened this issue 8 years ago • 50 comments

There are known issues with auto-sizing / auto-layout / estimated size cells in IGListKit.

A number of issues have been opened: #487, #508, #266, #463, #452, #588, #739, #864, #906

We'll track and discuss all auto-sizing bugs/problems in this task to consolidate the discussion.


If any new issues are opened regarding this, please close and redirect here. 😄

jessesquires avatar Feb 27 '17 18:02 jessesquires

This bug hit me hard today.

Commit on Example project to repro bug: https://github.com/dhavalcue/IGListKit/commit/c4f191f0fe7bf0ca66a536f415dbbbb93f47fac2 Video of bug: Video

My repro is only for self-sizing NIB cells.

Two issues:

  1. Cells are centered on screen. Workaround is to setup a constraint to set the width to size of screen.

  2. While performing updates, the cells immediately size themselves to the size as returned by the section controller and then animate to expected size. See Video I don't know of any reasonable workarounds for this.

@jessesquires: Is this something you guys expect to look into or is it not a priority for the team right now?

Thanks team, ❤️️ IGListKit!

ghost avatar Mar 03 '17 02:03 ghost

I tried the same without IGListKit, standard UICollectionView and UICollectionViewFlowLayout. The bug still exists there. This one is not on IGListKit.

https://github.com/dhavalcue/UICollectionViewSelfSizingCellsBug

ghost avatar Mar 03 '17 17:03 ghost

Is it possible to have a setup which combines self-sizing and fixed size cells within the same collection view/layout? Can only get either to work properly, not both together.

EDIT: err, nevermind. Fixed with some AutoLayout magic.

HarshilShah avatar Mar 03 '17 18:03 HarshilShah

Is this something you guys expect to look into or is it not a priority for the team right now?

This isn't a huge priority for us, as we don't use this in IG. But, we would like to support it.

jessesquires avatar Mar 04 '17 19:03 jessesquires

This bug is a problem, our entire project uses variable height cells! We will take a look later. Any idea?

Ricardo1980 avatar Mar 27 '17 16:03 Ricardo1980

Guys, I'm struggling for few days with self sizing items in collectionview. I used :

  • The technique of estimatedSize + preferredMaxWidth label + contentView width constraint + contentView translatesMaskIntoConstraint false, but it crashed on iPhone Plus models iOS 10

  • The technique of estimatedSize + preferredLayoutAttributesFitting + systemLayoutSizeFitting compression

  • The technique of estimatedSize + systemLayoutSizeFitting:withHorizontalFittingPriority:verticalFittingPriority it works on iPhone Plus but it doesn't work when change iPhone model to iPhone 5 and 6 doesn't have the good width.

I read a ton of articles and nothing seem to be working perfectly. I even tried the fake sizing cell, but it's really low performance, you can see it at scroll even on an iPhone 7 Plus.

I was considering switching to IGListKit but I see that this edge case is not handled neither. For me I have two options : Either I go back to TableView :'( Either like in JSQMessagesViewController or IGListKit raywenderlich tutorial where he computes the height of the cell in helper in the cell.

Guys, you who have experience with dynamic cell height, what would you recommend ?

LivioGama avatar Mar 29 '17 13:03 LivioGama

Similar problems here. At the moment I am waiting.

Ricardo1980 avatar Mar 29 '17 14:03 Ricardo1980

I couldn't get dynamic heights to work on my end. My workaround is to use systemLayoutSizeFitting and cache the size. For this to work I require all self-sizing cells to implement the SelfSizingCell protocol, which takes in an item which implements the IGListDiffable protocol.

The whole thing to make this work is here: https://gist.github.com/dhavalcue/4b081e65ded5c261d301beddbd2ddfcb

ghost avatar Mar 29 '17 17:03 ghost

@LivioGama Instagram is almost entirely dynamic cell sizes, its just that we don't use any of the UIKit estimated cell sizing b/c of how buggy they can be (evidenced by this issue). Definitely want to find a good recommended way to do self-sizing cells, but it's really low priority for us since we don't use it and honestly don't really recommend it (perf, bugs, etc).

Also as @dhavalcue pointed out, this bug happens outside of IGListKit, maybe our best approach would be to file and dupe some radars?

rnystrom avatar Mar 30 '17 01:03 rnystrom

This is kinda a hacky approach, but my “solution” to this is to just handle supplementary views as regular cells instead.

My method:

  • Tweak the sectionController/add a new one to handle headers/footers/etc.
  • Set an estimatedItemSize for the collectionView’s layout
  • Return a size with the required cell width and some random height value in sizeForItem
  • Use the default preferredLayoutAttributesFitting implementation as shown in the examples

This seems to work well for me. Getting the contentView to return the correct size in preferredLayoutAttributesFitting gets a bit tricky sometimes, but here’s the golden rule I follow: Never touch it other than to add subviews or constraint subviews to it. Also, if you see some overlapping in cells, set the bogus height in sizeForItem to some massive value. Dunno why, but it seems to work.

HarshilShah avatar Mar 30 '17 02:03 HarshilShah

@rnystrom Hello thanks for your reply. I'm really happy about your recommendation, because I'm not stubborn to use absolutely estimatedCellSize. I just want something that work 100%. So in Instagram you do static function in each cells that calculate the height according to the text ? Something like this https://gist.github.com/gnou/52bd2f31e99c9311ffa0eb0183ac7cfd

LivioGama avatar Mar 30 '17 08:03 LivioGama

@LivioGama almost exactly that 😄

rnystrom avatar Mar 30 '17 21:03 rnystrom

@rnystrom Is there any sample code where I could see this library and calculating the height manually?

@LivioGama Do you have a sample project with your code? Thanks a lot.

BTW, I tried to open the sample code, but it does not compile. I opened a bug here: https://github.com/Instagram/IGListKit/issues/635

Ricardo1980 avatar Apr 10 '17 22:04 Ricardo1980

@Ricardo1980: Did you see my comment here: https://github.com/Instagram/IGListKit/issues/516#issuecomment-290156370

ghost avatar Apr 10 '17 22:04 ghost

@dhavalcue I tried that (without the caching). I mean, I tried to get the cell inside sizeForItem calling cellForItem and them apply something like:

    let cell = FeedCommentCell(frame: .zero)
    cell.fillWithData()
    cell.contentView.setNeedsLayout()
    cell.contentView.layoutIfNeeded()
    let maxSize = CGSize(width: context.containerSize.width, height: 0)
    let properSize = cell.contentView.systemLayoutSizeFitting(maxSize, withHorizontalFittingPriority: UILayoutPriorityRequired, verticalFittingPriority: UILayoutPriorityFittingSizeLevel)
    print(properSize.height)
    return properSize

in order to return that size for that cell. Is that the main idea? It seems it is working but I see a lot of autolayout conflicts in the console. I have setup the cell with a mutiple line UILabel pin to the super view (contentView), top, left, right, bottom. Is that right? I guess that should be required for systemLayoutSizeFitting.

Any clue about this? Any recommendation is welcome. How do you do that part? Thanks a lot.

I forgot to say my app should work under iOS 9, maybe that is important.

BTW, when UILabel is involved, do I have to use preferredMaxLayoutWidth always? I don't see the reason if constraints are used properly.

Ricardo1980 avatar Apr 11 '17 09:04 Ricardo1980

@Ricardo1980 i do same like yours, but my first item always got wrong height. Did you find any solution already ?

👆👆👆👆

yulichandra avatar May 22 '17 11:05 yulichandra

It's working fine for me. I instantiate another cell, I apply autolayout using the context width, I take the height and I return that number in the size method. Not very nice but it is working perfectly for me in all cases.

Ricardo1980 avatar May 22 '17 18:05 Ricardo1980

@Ricardo1980 , i was not using auto layout, but solved it already. Thanks btw.

yulichandra avatar May 23 '17 07:05 yulichandra

Does IG recommend using autolayout to configure the things around the comments that don't "really" need to be the dynamic height or is just using CG rect frames the way to go about this?

jboo1212 avatar May 30 '17 04:05 jboo1212

@jboo1212 we definitely don't for complicated layouts that need to be performant. We don't actually have a single line of Auto Layout code in IG, but that's mostly for consistency. AL is a great framework when used in moderation, and away from perf-tight situations.

For top-notch scroll perf, nothing beats plain CGRect math. But using something like Yoga to compute layouts on a background queue and then mount them is also possible, but can get pretty complicated.

rnystrom avatar Jun 05 '17 01:06 rnystrom

Anyone else here having issues with using multiple section controlle and for one use a supplementary view header? For some reason when I use prefferedLayoutAttributesFitting to return the real cell size the header is staying at the old position before the cells got resize to fit its content. anyone seen this before?

weyert avatar Aug 20 '17 01:08 weyert

Hey guys, I guess, I've found a way how to make self-sizing work:

  1. In viewDidLoad():
       if let collectionViewFlowLayout = collectionView.collectionViewLayout as? UICollectionViewFlowLayout {
            collectionViewFlowLayout.estimatedItemSize = UICollectionViewFlowLayoutAutomaticSize 
        }
  1. In your cell class:
  override func preferredLayoutAttributesFitting(_ layoutAttributes: UICollectionViewLayoutAttributes) -> UICollectionViewLayoutAttributes {
        setNeedsLayout()
        layoutIfNeeded()

        let size = contentView.systemLayoutSizeFitting(CGSize(width:  layoutAttributes.frame.width,
                                                              height: CGFloat.greatestFiniteMagnitude),
                                                       withHorizontalFittingPriority: UILayoutPriorityRequired, verticalFittingPriority: UILayoutPriorityFittingSizeLevel)
        layoutAttributes.frame.size = size
        return layoutAttributes
    }

artemkalinovsky avatar Aug 22 '17 15:08 artemkalinovsky

Thank you @artemkalinovsky! I will try this out. What's the difference between the systemLayoutSizeFitting with the extra parameters and the plain contentView.systemLayoutSizeFitting? It's unclear for me when reading the docs

weyert avatar Aug 22 '17 20:08 weyert

@weyert func systemLayoutSizeFitting(_ targetSize: CGSize) -> CGSize give me wrong size (width and height), so I've used method, where I can set correct width by myself:

func systemLayoutSizeFitting(_ targetSize: CGSize, 
withHorizontalFittingPriority horizontalFittingPriority: UILayoutPriority, 
     verticalFittingPriority: UILayoutPriority) -> CGSize

where I've said that I want cell with fixed width and flexible height value.

artemkalinovsky avatar Aug 22 '17 20:08 artemkalinovsky

@artemkalinovsky Thanks, it work for me. But there are two problems.

  1. UICollectionViewFlowLayoutAutomaticSize NS_AVAILABLE_IOS(10_0)
  2. I still get warnings about two extra NSAutoresizingMaskLayoutConstraint - width and height set in section controller

Any thoughts?

alex-sarkisov avatar Aug 23 '17 05:08 alex-sarkisov

So, how I solved this problem in Objective-C:

  1. Remove "preferredLayoutAttributesFitting" method.
  2. Add any non CGSizeZero "estimatedItemSize", for example:
UICollectionViewFlowLayout *layout = [[UICollectionViewFlowLayout alloc] init];
layout.estimatedItemSize = CGSizeMake(44.0f, 44.0f);
  1. Set content view "translatesAutoresizingMaskIntoConstraints" to false.
- (instancetype)initWithFrame:(CGRect)frame {
    if (self = [super initWithFrame:frame]) {
        self.contentView.translatesAutoresizingMaskIntoConstraints = NO;
    }
    return self;
}
  1. Set constraints for your custom view, label, etc. in cell.
  2. Return size in ListSectionController, for example:
- (CGSize)sizeForItemAtIndex:(NSInteger)index {
    return CGSizeMake(self.collectionContext.containerSize.width, 44.0f);
}

alex-sarkisov avatar Aug 23 '17 07:08 alex-sarkisov

@agsarkisov Our app is targeting iOS10 and above, so we don't care about UICollectionViewFlowLayoutAutomaticSize NS_AVAILABLE_IOS(10_0)

¯_(ツ)_/¯

artemkalinovsky avatar Aug 23 '17 07:08 artemkalinovsky

@rnystrom What are you guys using for this below image at instagram? Tableview? I need dynamic height for cells with textView embedded, preferably in collectionviewcell.

joseph-francis avatar Mar 10 '18 20:03 joseph-francis

My bet would be UITableViewController to not have to handle the scroll and keyboard :D

LivioGama avatar Mar 10 '18 20:03 LivioGama

Then, what is the best way to do this?

joseph-francis avatar Mar 10 '18 20:03 joseph-francis