Instagram / IGListKit

A data-driven UICollectionView framework for building fast and flexible lists.
https://instagram.github.io/IGListKit/
MIT License
12.87k stars 1.54k forks source link

Reusing IGListSectionControllers issues [question] #422

Closed ryancstack closed 7 years ago

ryancstack commented 7 years ago

New issue checklist

General information

I'm trying to implement an embedded collection view in a section controller similar to the nested data example in the demo project. I'm noticing two issues so far in regards to reusing IGListSectionControllers. When you scroll horizontally through one of them and scroll down, the next section that reuses the embedded section continues to scroll as well. Secondly, if I set different heights while using the same IGListSectionController,

case .video:
    return EmbeddedAdapterSectionController(height: 180, dataSource: VideoListDataSource())
case .user:
    return EmbeddedAdapterSectionController(height: 130, dataSource: UserListDataSource())

it will sometimes use the wrong section controller as it tries to reuse them. Is there anyway to ensure that a section uses the correct IGListSectionController instance or best practices in using embedded collection views and adapters?

Are there any best practices for storing contentOffset for the embedded collection views?

Edit: I assume this has to do with IGListSectionMap's implementation

rnystrom commented 7 years ago

@ryancstack section controllers actually shouldn't be reused between different objects. Can you post either an example app or some code that shows your model setup?

One key thing is to make sure that your diffIdentifiers are unique between objects given to the list adapter. If they do share identifiers, you should hit asserts when debugging. It sounds like that might be the case w/ the vide/user height issue too.

Are there any best practices for storing contentOffset for the embedded collection views?

This is something I hope to tackle w/ #31. But for now, you could try using the IGListDisplayDelegate functions and when your cell ends display, get the content offset and save it in a local variable.

Then in will-display, you can set the content offset. Something like:

var contentOffset = CGPoint.zero

// abbreviated
func willDisplayCell(cell: UICollectionViewCell) {
  guard let cell = cell as? EmbeddedCell else { return }
  cell.collectionView.contentOffset = contentOffset
}

func didEndDisplayCell(cell: UICollectionViewCell) {
  guard let cell = cell as? EmbeddedCell else { return }
  contentOffset = cell.collectionView.contentOffset
}
ryancstack commented 7 years ago

thanks for the response @rnystrom. I've been using your models from SimpleWeather as starters for the embedded section controllers:

class EmbeddedSection {
    let identifier: NSObjectProtocol
    let items: [IGListDiffable]

    init(identifier: NSObjectProtocol, items: [IGListDiffable]) {
        self.identifier = identifier
        self.items = items
    }
}

extension EmbeddedSection: IGListDiffable {
    func diffIdentifier() -> NSObjectProtocol {
        return identifier
    }

    func isEqual(toDiffableObject object: IGListDiffable?) -> Bool {
        if self === object { return true }
        guard let object = object as? EmbeddedSection else { return false }
        return identifier.isEqual(object.identifier)
    }
}

I supply the EmbeddedSection with an array of view models like so:

class UserViewModel {

    let id: String
    let username: String
    let verified: Bool

    init(user: User) {
        id = user.id
        username = user.username
        verified = user.vip
    }
}

extension UserViewModel: IGListDiffable {
    func diffIdentifier() -> NSObjectProtocol {
        return id as NSString
    }

    func isEqual(toDiffableObject object: IGListDiffable?) -> Bool {
        if self === object { return true }
        guard let object = object as? VidmeUserViewModel else { return false }
        return id == object.id &&
            username == object.username &&
            verified == object.verified
    }
}

I debugged as well and all EmbeddedSections have unique diffIdentifiers

A little additional information:

While debugging, I found that the IGListAdapterDelegate method

public func listAdapter(_ listAdapter: IGListAdapter!, willDisplay object: Any!, at index: Int)

will return the wrong object and index. For example, if the controller is about to display the 6th item, it would return the item at index 12 or 4 instead.

I'll get a project hosted soon if you need more information.

rnystrom commented 7 years ago

Still on my radar, just need to set aside a chunk of time to investigate here. An example reproducing this is probably critical in determining if its a bug in IGListKit or just a setup issue.

A couple things to note from the weather example:

ryancstack commented 7 years ago

ReusingSectionControllers.zip

I couldn't replicate the resizing issue with this (probably something to do with my earlier implementation) but the scrolling bugs are apparent in this example.

rnystrom commented 7 years ago

@ryancstack really appreciate this, will spin through it tomorrow!

rnystrom commented 7 years ago

@ryancstack I took a spin through your example and am a little stumped about what is wrong exactly.

If the "scrolling bugs" are that the content offset is wrong when scrolling down, that's b/c the cells are being reused and the scroll view content offset needs to be reset. An easy way to do that is to set collectionView.contentOffset = .zero in prepareForReuse() in the cells. That doesn't save the state when you scroll back, though, so you can use the IGListDisplayDelegate methods on the section controller to store and restore the offset as well.

I don't see any issue with IGListAdapterDelegate parameters either. Can you help me see what the issue is?

ryancstack commented 7 years ago

The offset bug is in regards to when you scroll a horizontal section, scroll down as it's still horizontally scrolling, and see that another section is continuing to scroll as well.

I set the contentOffset = .zero in the EmbeddedCollectionViewCell's prepareForReuse() and it had no effect.

(I think the delegate bug was a reading error by me :( it was ending display on some and I mistook that for willDisplay)

Thanks for all your help so far!

rnystrom commented 7 years ago

@ryancstack try

collectionView.setContentOffset(.zero, animated: false)
ryancstack commented 7 years ago

That'll do it! Thanks for the help @rnystrom