Can someone shed some light on an IGAssert in IGListBindingSectionController?

[jeffbailey]

New issue checklist

• [X ] I have reviewed the README and documentation
• [X ] I have searched existing issues and this is not a duplicate

General information

• IGListKit version: 3.4.0
• iOS version(s): iOS 11.3

Can someone help me understand the purpose in the IGAssert in the IGListBindingSectionController::didUpdateToObject method?

- (void)didUpdateToObject:(id)object {
    id oldObject = self.object;
    self.object = object;

    if (oldObject == nil) {
        self.viewModels = [[self.dataSource sectionController:self viewModelsForObject:object] copy];
    } else {
        IGAssert([oldObject isEqualToDiffableObject:object],
                 @"Unequal objects %@ and %@ will cause IGListBindingSectionController to reload the entire section",
                 oldObject, object);
        [self updateAnimated:YES completion:nil];
    }
}

It is being triggered by a call to:
adapter.performUpdates(animated: true)
In order to show/hide some sections as well as update existing sections. I assume the assert is trying to point out a misuse of the API, but I must be missing something.

[rnystrom]

Sure thing! This assert is in place b/c if your models return NO for equality checks, then they will simply reload the entire section instead of giving you cell-level updates.

I could maybe see the need for this type of update in some cases. If that's the case, let us know. I'd be open to a PR converting this assert into a warning log.

[jeffbailey]

Thanks for the quick reply. I think our use case is a valid example of why it should not be an assert. We are showing/hiding questions in a survey. The header with the show/hide button is a section and each question is a section. Each section is using a IGListBindingSectionController. I believe the only way to animate the show/hide of questions is to call:
adapter.performUpdates(animated: true)
The assert is firing because the header view model is being updated with the state of the show/hide button.

Hope that makes sense. Let me know if you need more info or can think of another approach.

[jeffbailey]

@rnystrom, based on what you said earlier, are you recommending that we don't call
adapter.performUpdates(animated: true)
with IGListBindingSectionController?

If that's the case, is there a way to animate in cells for new sections to support show/hide functionality?

[rnystrom]

@jeffbailey no you should be able to still call updates. If the object is hidden, we’d recommend you just relive the item from the top-level array.

If you want to hide all the cells, but keep the section controller around, you can return 0 for numberOfItems.

Am I understanding your design correctly?

_{Sent with GitHawk}

[jeffbailey]

@rnystrom I think I understand what you're saying, but it does change our implementation a bit. Each question has its own section. When the survey first appears there's only one question shown (backed by 1 ListDiffable Question object and 1 section controller). When the "Show All" button is tapped, we call the adapter's performUpdates method to show all 5 questions. This results in all 5 ListDiffable objects being returned the other 4 section controllers getting created. I think you're suggesting we create all 5 section controllers up front, and have the other 4 return 0 for numberOfItems. When "Show All" is tapped, we then call the update method on each section controller.

On the surface, our current approach seems cleaner to me and I'd advocate the assert by changed to a warning (at most). Let me know if I understood your suggestion correctly and if you still think its the right approach given this additional background info about our current setup.

[rnystrom]

@jeffbailey hmm your approach sounds totally fine. I'm actually not sure why you're even hitting the assert in this case? Is the first question model changing after "show all" is tapped?

[jeffbailey]

@rnystrom Ah yes, I was oversimplifying. The first model is really for the state of the "Show All/Hide" button and its state changes when tapped to show/hide the questions. The diffIdentifier for the model remains unchanged, but the isEqual is different because of the change of show/hide state. So IGListKit uses the same section controller instance and calls didUpdateToObject to give it the new model. Sorry I left out that important detail! We have actually worked about the issue for now by using a UUID for the diffIdentifier which will trigger a new section controller being created.

[rnystrom]

@jeffbailey should we close this out?

_{Sent with GitHawk}

[SirensOfTitan]

@rnystrom : Would you support a PR making this assert a warning instead?

... We're using IGListKit alongside ReSwift (redux, I'm sure you're a bit familiar being at FB), and consider this example:

There's a contact section:
-- First Cell is the contact name/profile picture, both of which are editable
-- Second Cell is say, a list of friends
-- Third cell is a button

1. User edits info: this dispatches an update to the ContactsStore in ReSwift to update the contact's name
2. The ViewController that houses the IGListKit collection view gets an update from the ContactsStore (since it's subscribed), and pipes that into its ViewModels.
3. The ContactsSection gets the updated view model, where isEqual should be false since the first cell (the contact identity) is dirty.
4. The second and third cell, since their viewModels didn't change, should show isEqual is true. Only the first cell should reload.

The consequences of this assert is that any time any model that is bound to a section controller changes, we need to call performUpdates on that controller for it to reload. It makes a lot more sense for it to be automatic.

[jeffbailey]

I would also like to see the assert turned into a warning and would be happy to do a pull request if you supported that @rnystrom.

[SirensOfTitan]

Changing the assert to a warning is a definitely good first step here in my opinion, but I don't think its enough, as I don't think there's enough documentation on why this is.

Having ListBindingSectionController's isEqual methods always true is by far the most confusing part of this framework, and I think we need a bit of centralized discussion around why this is (as you'll inevitably run into this if you use it for anything more than an example).

This assert is in place b/c if your models return NO for equality checks, then they will simply reload the entire section instead of giving you cell-level updates.

So I get this now after poking in the source code: an update to a ListBindingSectionController's ViewModel will cause a reload (destructured to a delete and insert).

... but at least in my case, I don't want to reload a ListBindingSectionController if it changes, I want to rebind its viewmodel, then just recall bindViewModel on any of its cells that changed (i.e. isEqual is false for the cell View Models). I don't want to have to manually tell the section controller to update, as the data changes could come somewhere outside the section controller.

[rnystrom]

@SirensOfTitan @jeffbailey Ya I'm in support of changing this assert to simply a warning. I could see the case for wanting to totally reload the section.

[jeffbailey]

Great. I'd put a pull request together.

[jeffbailey]

Pull request has been submitted. To test the change, I updated IGTestDiffingObject::isEqualToDiffableObject to not always return YES. Now it does a simple compare for the number of objects associated with the key. That actually triggered the assert that was there previously when running IGListBindingSectionControllerTests::test_whenAdapterReloadsObjects_thatSectionUpdated. Now that I converted the assert to a warning, the test passes again.

[darsshanNair]

hey @rnystrom I am actually using IGListKit for one of my VCs and this assertion is still there. It is not being flagged as a warning. I just started using it a few weeks ago thus I am pretty sure I am using the latest. Has the PR not been merged yet?

[SirensOfTitan]

@darsshanNair: Use master, I don't think there's been a formal release in a while.