Problem with ViewReuseId

[spekke]

Hi, As I understand it, you should be able to use the same viewReuseId for different layouts as long as they create the same UIView class and configure the same view properties. When I'm doing this and using my layouts with ReloadableViewLayoutAdapter on a UICollectionView, it's not cleaning up views correctly on a re-used UICollectionViewCell.

Here's a Playground example that reproduces the problem:

import Foundation
import UIKit
import LayoutKit
import PlaygroundSupport

struct FeedItem {
    let name: String
    let text: String
}

struct BannerItem {
    let headline: String
    let subtitle: String?
}

class MyViewController : UIViewController {

    let items: [Any] = [
        FeedItem(name: "User X", text: "Lorem ipsum dolor sit amet, consectetuer adipiscing elit. Aenean commodo"),
        BannerItem(headline: "Ad 1", subtitle: "Best market ever!"),
        FeedItem(name: "User Y", text: "Lorem ipsum dolor sit amet, consectetur adipiscing elit. An nisi populari fama? Scaevolam M. At enim sequor utilitatem. Duo Reges: constructio interrete. Bonum valitudo: miser morbus."),
        BannerItem(headline: "Ad 2", subtitle: nil),
        FeedItem(name: "User Z", text: "Rationis enim perfectio est virtus; Certe non potest. Ille incendat? Ut pulsi recurrant?"),
        BannerItem(headline: "Ad 3", subtitle: "Best lib ever!")
    ]

    let collectionViewLayout: UICollectionViewFlowLayout = {
        let layout = UICollectionViewFlowLayout()
        layout.sectionInset = UIEdgeInsets(top: 10, left: 0, bottom: 10, right: 0)
        return layout
    }()

    lazy var layoutAdapterCollectionView: LayoutAdapterCollectionView = {
        let collectionView = LayoutAdapterCollectionView(frame: .zero, collectionViewLayout: self.collectionViewLayout)
        collectionView.backgroundColor = .lightGray
        collectionView.alwaysBounceVertical = true
        return collectionView
    }()

    override func viewDidLayoutSubviews() {
        super.viewDidLayoutSubviews()
        self.layoutAdapterCollectionView.frame = self.view.bounds
    }

    override func viewDidLoad() {
        super.viewDidLoad()
        self.view.backgroundColor = .white
        self.view.addSubview(self.layoutAdapterCollectionView)
    }

    override func viewDidAppear(_ animated: Bool) {
        super.viewDidAppear(animated)

        let sections: [[Any]] = [
            self.items,
            self.items,
            self.items
        ]

        self.layoutAdapterCollectionView.layoutAdapter.reload(
            width: self.layoutAdapterCollectionView.bounds.width,
            synchronous: true,
            layoutProvider: { (Void) -> [Section<[Layout]>] in

                let sections: [Section<[Layout]>] = sections.enumerated().flatMap({ (index, section) in
                    let layouts: [Layout] = section.flatMap { item in
                        if let feedItem = item as? FeedItem {
                            return self.feedItemLayout(for: feedItem)
                        }
                        if let bannerItem = item as? BannerItem {
                            return self.bannerLayout(for: bannerItem)
                        }
                        return nil
                    }
                    let sectionLayout = self.sectionLayout(index: index)
                    return Section<[Layout]>(header: sectionLayout, items: layouts)
                })

                return sections
            },
            completion: nil
        )
    }

    private func sectionLayout(index: Int) -> Layout {
        let labelLayout = LabelLayout(
            text: "Section \(index)",
            font: UIFont.boldSystemFont(ofSize: 20),
            config: { label in
                label.textColor = .white
            }
        )
        return InsetLayout(
            insets: UIEdgeInsets(top: 20, left: 10, bottom: 0, right: 10),
            sublayout: labelLayout,
            config: { view in
                view.backgroundColor = .black
            }
        )
    }

    private func bannerLayout(for bannerItem: BannerItem) -> Layout {

        let headlineLayout = LabelLayout(
            text: bannerItem.headline,
            font: UIFont.systemFont(ofSize: 18),
            numberOfLines: 0,
            viewReuseId: "labelWithTextColorAndBackgroundColor",
            config: { label in
                label.textColor = .white
                label.backgroundColor = .black
            }
        )

        var subtitleLayout: Layout?
        if let subtitle = bannerItem.subtitle {
            subtitleLayout = LabelLayout(
                text: subtitle,
                font: UIFont.systemFont(ofSize: 18),
                numberOfLines: 0,
                viewReuseId: "labelWithTextColorAndBackgroundColor",
                config: { label in
                    label.textColor = .green
                    label.backgroundColor = .red
                }
            )
        }

        return StackLayout(
            axis: .vertical,
            viewReuseId: "viewWithBackgroundColor",
            sublayouts: [
                headlineLayout,
                subtitleLayout
            ].flatMap({ $0 }),
            config: { view in
                view.backgroundColor = .white
            }
        )
    }

    private func feedItemLayout(for feedItem: FeedItem) -> Layout {

        let imagePlaceholderLayout = SizeLayout(
            width: 80,
            height: 80,
            viewReuseId: "viewWithBackgroundColor",
            config: { view in
                view.backgroundColor = .gray
            }
        )

        let nameLayout = LabelLayout(
            text: feedItem.name,
            font: UIFont.systemFont(ofSize: 14),
            numberOfLines: 0,
            viewReuseId: "labelWithTextColorAndBackgroundColor",
            config: { label in
                label.textColor = .black
                label.backgroundColor = .clear
            }
        )

        let textLayout = LabelLayout(
            text: feedItem.text,
            font: UIFont.systemFont(ofSize: 12),
            numberOfLines: 0,
            viewReuseId: "labelWithTextColorAndBackgroundColor",
            config: { label in
                label.textColor = .black
                label.backgroundColor = UIColor.groupTableViewBackground
            }
        )

        let verticalStackLayout = StackLayout(
            axis: .vertical,
            sublayouts: [
                nameLayout,
                textLayout
            ]
        )

        return StackLayout(
            axis: .horizontal,
            viewReuseId: "viewWithBackgroundColor",
            sublayouts: [
                imagePlaceholderLayout,
                verticalStackLayout
            ],
            config: { view in
                view.backgroundColor = .yellow
            }
        )
    }
}

PlaygroundPage.current.liveView = MyViewController()
PlaygroundPage.current.needsIndefiniteExecution = true

It initially builds a list that looks like this:

But after scrolling down and then back up it looks like this:

Ad 2 and the item below now has a yellow view that is not removed after a re-use.

[spekke]

Ok, I looked into the ViewRecycler code and now it's clear that a viewReuseId needs to be unique within each cell view hierarchy.

In our scenario we have a rather complex list view where each row can differ quite a lot from the previous rows but they are all build using the same sub components, just in different content contexts. Most of the sub components have eg. UILabels where only font and textColor is different, container views with different background colors etc and each cell can have multiple instances of these.

For us it would be a big performance gain (number of views that are being re-used) if viewReuseId would be an identifier that describes views that are of the same UIView class and are configuring the same view properties instead of uniquely describing views in which context they are used.

Example of a UILabel Layout that shows number of likes with viewReuseId that describes the view instead of the view context:

let likesLayout = LabelLayout(
    text: "12 likes",
    font: UIFont.systemFont(ofSize: 12),
    numberOfLines: 0,
    viewReuseId: "labelTextColorBgColor",
    config: { label in
        label.textColor = .black
        label.backgroundColor = UIColor.groupTableViewBackground
    }
)

Instead of

let likesLayout = LabelLayout(
    text: "12 likes",
    font: UIFont.systemFont(ofSize: 12),
    numberOfLines: 0,
    viewReuseId: "numberOfLikesLabel",
    config: { label in
        label.textColor = .black
        label.backgroundColor = UIColor.groupTableViewBackground
    }
)

The downside of this is that it would affect the LayoutKit Animation behaviour since it uses the viewReuseId to identify which views to animate.

What's your thoughts on this?

[nicksnyder]

I looked into the ViewRecycler code and now it's clear that a viewReuseId needs to be unique within each cell view hierarchy.

Your observation is correct. viewReuseId is intended to identify exactly the same view, not a different but compatible view.

if viewReuseId would be an identifier that describes views that are of the same UIView class and are configuring the same view properties instead of uniquely describing views in which context they are used.

This is something that I experimented around with a while ago and didn't follow through with for reasons I forget. It should be possible to do what you want, but we would need two identifiers: one for uniquely identifying views for animations, and one for identifying a group of views that are interchangeable for view reuse purposes (i.e. viewReuseId and viewReuseGroup).

If this is a real problem for you and you are willing to make a contribution, I would be happy to review it.

[spekke]

This is something that I experimented around with a while ago and didn't follow through with for reasons I forget. It should be possible to do what you want, but we would need two identifiers: one for uniquely identifying views for animations, and one for identifying a group of views that are interchangeable for view reuse purposes (i.e. viewReuseId and viewReuseGroup).

If this is a real problem for you and you are willing to make a contribution, I would be happy to review it.

Cool, this is something we definitely would like to have. I would be happy to make a contribution. Just need to find time to work on it, so it would probably take week or so to have a PR ready.

Thanks!

[spekke]

Finally got time to look into this. I've created a pull request (https://github.com/linkedin/LayoutKit/pull/175) that adds support for viewReuseGroup. It would be awesome if you had a look at it.