Instagram / IGListKit

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

Cells not showing up after reloadData #305

Closed stevejcox closed 7 years ago

stevejcox commented 7 years ago

New issue checklist

General information

Hey guys,

Firstly, I adore this library. Amazing job! So I am probably doing something wrong, but here goes.

I have a toggle button that filters objects based on a Bool. Within the DataSource I have:

public func objects(for listAdapter: IGListAdapter) -> [IGListDiffable] {
        guard let delegate = delegate else { return [] }

        var objects = !isFeatured ? delegate.model.posts as [IGListDiffable] : delegate.model.featuredPosts as [IGListDiffable]

model.featuredPosts is a simple filter:

var featuredPosts: [Post] {
        get {
            return posts.filter { $0.isFeatured }
        }
}

Within my delegate I am reloading that data with:

adapter.reloadData { (completed) in
    print(completed)
}

When I toggle to only show isFeatured posts, the collection view updates as expected and I see 3 items. When I toggle it again to show all posts (8 in total), I can see it requesting the sections and cells, however the collection view isn't updated.

When I update the completion block as:

adapter.reloadData(completion: { (completed) in
            self.adapter.performUpdates(animated: false, completion: nil)
            print(completed)
            print(self.adapter.collectionView?.numberOfSections)
 })

I can see the correct number of sections being printed (8), but not displayed.

Any suggestions on what I may be doing wrong?

For what it's worth the collection view frame is being set:

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

Thanks in advance, Steve

rnystrom commented 7 years ago

@stevejcox Hmm interesting, what do your models look like when conforming to IGListDiffable?

Also, what does your listAdapter(_ listAdapter:, sectionControllerFor object:) look like?

If you find a way to reproduce this behavior I'd love to take a look at a sample project!

stevejcox commented 7 years ago

@rnystrom The Post object is pretty basic, has the below IGListDIffable extension:

extension Post: IGListDiffable {
    public func diffIdentifier() -> NSObjectProtocol {
        return key as! NSObjectProtocol
    }

    public func isEqual(toDiffableObject object: IGListDiffable?) -> Bool {
        if object === self {
            return true
        }
        if let object = object as? Post {
            return key == object.key
        }
        return false
    }
}

I have 3 different Section types. The filter I mentioned with isFeatured is only applied to the PostSectionController items.

public func listAdapter(_ listAdapter: IGListAdapter, sectionControllerFor object: Any) -> IGListSectionController {
        if let obj = object as? String, obj == spinToken {
            return SpinnerSectionController()
        }

        if let obj = object as? String, obj == recentToken {
            let sectionController = RecentFeatureTabSectionController()
            sectionController.delegate = self
            return sectionController
        }

        if let obj = object as? Post, obj == heroPost {
            let sectionController = HeroPostSectionController()
            sectionController.delegate = self
            sectionController.displayDelegate = self
            sectionController.post = obj
            return sectionController
        }

        let sectionController = PostSectionController()
        if let obj = object as? Post {
            sectionController.post = obj
            sectionController.delegate = self
            sectionController.displayDelegate = self
        }
        return sectionController
    }

The filter is only being applied to the objects that populate the PostSectionConttoller.

stevejcox commented 7 years ago

Weirdly, if I call reload twice, it displays the cells.

adapter.reloadData(completion: { (completed) in
      self.adapter.reloadData(completion: nil)
})
rnystrom commented 7 years ago

Hmph, this is really hard to debug w/out a project to investigate. Think you could take one of our examples and modify it to mimic your models and setup to get it into the same state? Doesn't seem like a complicated setup, and its something we do in the examples as well as in Instagram. 🤔

stevejcox commented 7 years ago

Yeah, I created the same system in the Examples and it worked fine! Going to keep trying to figure out what's different between the app and examples. Totally understand it's near impossible for you without an example!

rnystrom commented 7 years ago

Some tips with debugging the collection view:

(lldb) p (int)[collectionView numberOfSections]
(lldb) p (int)[collectionView numberOfItemsInSection:0] // do for each section
(lldb) po [collectionView recursiveDescription] // see if the cells are in the hierarchy

Also to get info on the backing store of the IGListAdapter:

(lldb) [adapter objects]
(lldb) [[adapter sectionMap] sectionControllerToObjectMap]
(lldb) [[adapter sectionMap] sectionControllerToSectionMap]

Makes me want to work on some debugging tools for IGListKit....

jeffbailey commented 7 years ago

I run into a similar issue when I first started using IGListKit. I'm filtering a list based on text typed into a search bar. Sometimes the number of items shown in the collection view would not be correct when the item count increased based on the new filter text. Reloading twice fixed the issue as @stevejcox said. Also rotating the device caused the correct number of items to display. However, I realized I really wanted to be calling performUpdates instead of reloadData to get a nice animation:

adapter.performUpdates(animated: true, completion: nil)

I never did get to the bottom of the reloadData issue.

stevejcox commented 7 years ago

Thanks for the debugging tips @rnystrom and for your comment @jeffbailey !

I've switched to performUpdates and this does indeed work as intended. I'll look into reloadObjects when I get a chance.

stevejcox commented 7 years ago

Hmm, unfortunately now adapter.performUpdates sometimes causes a crash:

Terminating app due to uncaught exception 'NSRangeException', reason: '*** -[__NSArrayM objectAtIndex:]: index 3 beyond bounds [0 .. 2]

@rnystrom - I'm going to add you as a collaborator to a stripped down version of what I am working on so you can see the issue. The issue is within the UserFeed, performUpdate being called in UserFeedViewModel.

Take a look at the curate-client project within the iOS folder.

You can reproduce by clicking one of the home tiles to take you to the 'UserFeed'. then toggle the RECENT / FEATURED options. Sometimes it works, sometimes it crashes with the above error.

jeffbailey commented 7 years ago

Here's a workaround to try for until you can resolve the crashes: First call adapter.performUpdates with an empty data list. Then in the completion block you can call adapter.performUpdates with the updated object list. I bet that will get rid of your index out of bounds exceptions. Not an ideal solution, but maybe it will be a decent workaround for now.

stevejcox commented 7 years ago

Thanks @jeffbailey - worked a treat.

rnystrom commented 7 years ago

Thanks for sharing the sample @stevejcox! I was able to track this down. Looks like we found a good UICollectionView + UICollectionViewFlowLayout bug! Might be worth opening a radar.

When you use -[IGListAdapter reloadDataWithCompletion:], the infra eventually does:

[collectionView reloadData];
[collectionView layoutIfNeeded];

Problem is that during reloadData UICollectionViewFlowLayout will query and cache all of the layout attributes (sizes). Its only when layoutIfNeeded is called does numberOfSections/etc get called, after the layout attributes have been created.

However since the data source is updated, there are no section controllers, thus nil and CGSizeZero are cached for the layout attributes. That explains the empty space even though the counts are all right.

I'll toss up a PR and unit tests for this soon! I think I'll package this and another internal bug fix in for 2.0.1.

Again, great stuff!

rnystrom commented 7 years ago

Also FWIW, if you're just going for an unanimated effect, I recommend using performUpdatesAnimated:NO instead of reloadData because reloading will rebuild all of your section controllers vs performing updates figuring out what needs to be updated/destroyed/inserted.

stevejcox commented 7 years ago

Thanks @rnystrom - glad to be able to help. Awesome news that it'll be fixed for 2.0.1, for now I will use @jeffbailey 's workaround.

rnystrom commented 7 years ago

@stevejcox another update! I figured out what the "bug" is:

Your project is using estimatedSize on UICollectionViewFlowLayout. This makes flow layout query/store layout attributes (sizes) during reloadData, but then doesn't query them again until the layout is invalidated.

Absolutely no clue why it works this way.

It doesn't look like your cells use AL to size themselves, so if you remove that line things work just fine calling reloadData when the toggle changes (tried this myself).

Working on a repro unit test b/c fixing this is still ideal, even if we're just patching another bug in UICollectionView 😂

stevejcox commented 7 years ago

Thanks @rnystrom - unfortunately there are a few cells that do use AL (PostCaptionCell) for one, so unfortunately removing it causes some layout issues. That being said Jeff's work-around is working just great for me, so there's no rush on the bug fix on my account.

Thanks again for your response and looking into this, IGListKit is just awesome.