Iglistkit: RFC: Add section property to IGListSectionController

[amonshiz]

IGListKit should remove the -[IGListCollectionContext sectionForSectionController:] method from IGListCollectionContext and replace it with a section property on IGListSectionController.

Reasons

• sectionForSectionController: has nothing to do with the collection displaying the section/list. This method alone requires that the IGListAdapter be the collectionContext object.
• A section controller's actual section index cannot change without the IGListAdapter knowing, and at that point it would be a new section controller anyhow.
• The property allows the collectionContext object to really be anything and not something that owns a mapping from section controller to an index.

The reasons that this method was originally added are all solved by adding a section property and as mentioned, that property is known at creation time.

Options

• Add this as a property to the init method of IGListSectionController
• Add this as @property (nonatomic, readwrite, assign) NSInteger index;

The init option is great because it means the property can be readonly, but it does mean that all subclasses have to be careful to include the property also.

The readwrite property is nice because then the IGListKit infra can own setting it. The downside is everyone knows that property can be written to. Maybe this is declared in IGListSectionController.h as readonly and in IGListSectionControllerInternal.h it is redeclared as readwrite?

Context

Speaking with @rnystrom in person about ways to break up implicit dependencies on UICollectionView and this method stood out as one that could be culled and would easily break up a hidden dependency on IGListAdapter.

[jessesquires]

Makes sense to me.

Maybe this is declared in IGListSectionController.h as readonly and in IGListSectionControllerInternal.h it is redeclared as readwrite?

Yep. This is what we already do for .isFirstSection and .isLastSection

I would say we definitely don't want this in init since clients could then override the section, or simply make mistakes calling super

[jessesquires]

Let's do this in 3.0

[rnystrom]

On board w/ this, ++ 3.0

@amonshiz you wanna take this on?

[amonshiz]

Will do. Finally back from a pretty crazy week. Gonna start this week and should have PR pretty quickly.

[Adlai-Holler]

How would this handle updating the index of all section controllers, if I inserted a section at index 0?

If it's a matter of convenience, then we should make the property readonly and implement it under-the-hood by calling [self.collectionContext sectionForSectionController:]. That way we never get stale data.

[jessesquires]

^^ I assumed that's always what this was about, just a convenience wrapper

[amonshiz]

The overall goal was to remove sectionForSectionController: from IGListCollectionContext in order to clean up the protocol to be only methods that go through to the backing list view. With that goal, the sectionForSectionController: method doesn't fit because it was going through to the sectionMap property of IGListAdapter which is not necessarily something that would be present on all IGListCollectionContext conforming objects that handle the presentation of a list.

[Adlai-Holler]

How would this handle updating the index of all section controllers, if I inserted a section at index 0?

[amonshiz]

How would this handle updating the index of all section controllers, if I inserted a section at index 0?

In a similar fashion to isFirstSection and isLastSection , but instead use the map to determine the section index right before calling didUpdateToItem:.

[Adlai-Holler]

But if the object wasn't updated, then we wouldn't visit the section controller in that for loop.

[Adlai-Holler]

You could set the section index right up there where you set isFirstSection by adding a counting variable for the for-loop.

[amonshiz]

just to follow back up with this, i kind of melded both suggestions: move setting the isFirstSection and isLastSection into the IGListSectionMap object's update method since it already has all the context, and then within the iteration during the update set the sectionIndex. this effectively unifies the location as @Adlai-Holler suggested and removes the necessity for an extra indexing variable which could theoretically get out of sync with the canonical mapping.