zenangst / Blueprints

:cyclone: Blueprints - A framework that is meant to make your life easier when working with collection view flow layouts.
Other
990 stars 56 forks source link

Dynamic height for HorizontalBlueprintLayout #115

Open zishanj opened 5 years ago

zishanj commented 5 years ago

With UICollectionViewFlowLayout I can set the height dynamic which adjust itself according to the underlying content by setting height in estimatedItemSize to 1 and then overriding systemLayoutSizeFitting in cell and setting height again to 1. This makes the height auto-adjusted according to its content. In Blueprints, it seems to be overriding it and I am no longer able to make it dynamic. I am using single row layout with multiple items.

zenangst commented 5 years ago

@zishanj Blueprints does not currently support dynamic widths or heights. We would love to add it so if you are up for me, make a PR and we will gladly review it :) I don’t think it would be to hard, just make an exception for estimated heights and check the cell for the system layout size fitting method, I might be wrong but that is my initial gut feeling.

zenangst commented 5 years ago

Did a quick search and found this: https://link.medium.com/oj69yvGzoY Might be a good starting point for the implementation.

zishanj commented 5 years ago

I wonder why the usual code does not work any more with this lib like I mentioned in my main comment. It was simple with that by setting height to 1.

zenangst commented 5 years ago

I think it has to do with how we explicitly set the size of the items. We should revisit this and reimplement it in a way that works with dynamic size.

zishanj commented 5 years ago

Anxiously waiting for this fix.

zenangst commented 5 years ago

@zishanj I've started investigating how this could be implemented now, no ETA and no promises. Will post an updated when I have something more substantial to convey.

christoff-1992 commented 5 years ago

@zishanj I've started investigating how this could be implemented now, no ETA and no promises. Will post an updated when I have something more substantial to convey.

@zenangst - I think I still have the work for this in the work I was looking at for the invalidation context. I believe you already have access to the repo, if not send us a message on slack. I hadn't completed it and abandoned the update after the changes where made in this repo for the invalidation context. However the changes meant all layouts would support dynamic sizes.

https://github.com/christoff-1992/Blueprints/blob/Feature/Horizontal-layout/Sources/HorizontalBlueprintLayout.swift

zenangst commented 5 years ago

@zishanj I have a branch that you can try out here feature/support-for-dynamic-sizes. Take it for a spin and see how it works for you, I've done some testing using unit tests but I have yet to try it in a project (for the lack of time).

I hope this branch takes us closer to the goal.

zishanj commented 5 years ago

I have tried this branch today and the result was same. estimatedItemSize and systemLayoutSizeFitting does not makes it dynamic by setting height to 1.

zenangst commented 5 years ago

@zishanj Have you implemented preferredLayoutAttributesFitting on your cells?

zishanj commented 5 years ago

Previously with UICollectionView I have been using: layout.estimatedItemSize = CGSize(width: 180, height: 1) and in cell

override func systemLayoutSizeFitting(_ targetSize: CGSize) -> CGSize {
        return contentView.systemLayoutSizeFitting(CGSize(width: 180, height: 1))
}

This makes the height dynamic with fixed width. With blueprint I have just replaced layout.estimatedSize to be used in its init:

let layout = HorizontalBlueprintLayout(
            itemsPerColumn: 1,
            itemSize: CGSize(width: 180, height: 1),
            minimumInteritemSpacing: 10,
            minimumLineSpacing: 10
)

and using same systemLayoutSizeFitting in cell. But it didn't work!

zenangst commented 5 years ago

@zishanj mind setting up a demo project that we can have access to? That would definitely speed things up.

Did you try the preferred layout attributes approach? You can check the setup in one of the tests on the new branch.

zenangst commented 5 years ago

Also, itemSize is not the same as estimated item size, try to set that explicitly after you create the layout. (We can definitely improve the init methods here).

zishanj commented 5 years ago

I tried in this way now without any luck: layout.estimatedItemSize =UICollectionViewFlowLayout.automaticSize then in cell I have only explicitly mentioned the width expecting the height to get auto adjust:

override func preferredLayoutAttributesFitting(_ layoutAttributes: UICollectionViewLayoutAttributes) -> UICollectionViewLayoutAttributes {
        let attributes = super.preferredLayoutAttributesFitting(layoutAttributes)
        attributes.size.width = 180
        return attributes
    }
zishanj commented 5 years ago

I guess height in this init:

let layout = HorizontalBlueprintLayout(
            itemsPerColumn: 1,
            itemSize: CGSize(width: 180, height: 1),
            minimumInteritemSpacing: 10,
            minimumLineSpacing: 10
        )

is overriding the dynamic height in cell. When I debug the view hierarchy it still gives me height = 1.

zishanj commented 5 years ago

I tried to dig into your test and used this init from your Helper class:

let layout = HorizontalBlueprintLayout(
            minimumInteritemSpacing: 10,
            minimumLineSpacing: 10,
            sectionInset: EdgeInsets(top: 0, left: 0, bottom: 0, right: 0))

along with:

layout.estimatedItemSize = CGSize(width: 180, height: 1)

The result was very odd. It didn't even adjusted the width to 180 and it was very smaller than its layout. I then explicitly set the itemSize like in your test:

layout.itemSize = CGSize(width: 180, height: 1)

This then adjusted the width but the height again get to 1 like I mentioned in previous comment. Its overriding dynamic height calculation even I have tried it with preferredLayoutAttributesFitting and systemLayoutSizeFitting.

zenangst commented 5 years ago

@zishanj Can’t really help more than I have at the moment, on vacation with my family. Will have to revisit this at a later date.

One thing that struck me is that the current implementation is made to support dynamic widths, not height when doing horizontal layouts. I guess that was a faulty assumption on my part that only supporting dynamic width would be enough. This might be the reason for it not working at the moment.

You could check out the branch and make changes to it to see if you can’t get it to work like you want. Just check the latest commit, that should provide you with enough breadcrumb information to where you have to make your changes.

zenangst commented 5 years ago

@zishanj I pushed some new updates to the branch now that should support both dynamic width and height. What you return from preferredLayoutAttributesFitting on your cell will no take precedence over what is used on the layout. If you return a negative value on either width or height in preferredLayoutAttributesFitting, then the estimatedItemSize will be used.

zenangst commented 5 years ago

Did some testing on that branch, seems like it still needs some work to get it right.

zenangst commented 5 years ago

I've continued with the implementation, slowly making progress.

zenangst commented 5 years ago

@zishanj mind trying the branch again? I think we've made a lot of progress now.

zenangst commented 5 years ago

Minor update on the progress, the branch should be able to handle vertical layouts using dynamic sizes now. Looking into horizontal layouts now.

Johnathan-Aretos commented 5 years ago

Hi, Great developments by the way.

I spent the last few hours messing around with the dynamic height's and I can't seem to achieve the results. I had a look at the tests @zenangst wrote and they seem to be passing but I can't replicate the same.

My preferredLayoutAttributesFitting returns the correct value I desire in my layout but it doesn't update or render in the collectionView. Even after a invalidate() call.

What keeps happening is it displays at the itemSize height and won't change it at all.

Do you know any fixes?

zenangst commented 5 years ago

Hey @Johnathan-Aretos, the world has not allowed me to continue this adventure as of late. When it comes to preferred layout attributes, that invalidation happens after prepare has been invoked and the views start aggregating in the view hierarchy. The preferred attributes are then diffed against the layout attributes cache and if there is a diff, that specific layout attribute should be invalidate. Calling invalidate() would only restart the cycle and not render what you want.

I'm no expert on preferred layout attributes and it is a bit of a mystery to me how to tame it so take the above statement with a grain of salt as it is my current understanding on how it is suppose to work. When implementing this, I've see oddities like the preferred layout attributes not being rendered until the user scrolls the collection view or the layout attributes are about to be invalidated etc.

I hope I get some more time investigating this in the near future, I've basically spent all my open source time on optimizing Family because it did need some love when it came to rendering performance. I did spend some time yesterday optimizing the core Blueprints layout but all those changes were unrelated to preferred layout attributes.

Johnathan-Aretos commented 5 years ago

No worries! I'll play around with it and see if I can pick up where you left off, if I figure it out I'll make a PR.

Thanks for all the information as well I'll see what I can do

zenangst commented 5 years ago

@Johnathan-Aretos hey man, just wanted to do a quick follow-up on this; did you manage to get any traction when investigating this?

Johnathan-Aretos commented 5 years ago

Hey,

I spent about 3 days on it trying to figure it out but couldn't figure it out. I had to move onto something else because of work, just waiting for some free time to sit down and had another go at it. Sorry man.