The great flattening. [[Model]] -> [Model]

[danielrhammond]

Overview

Took a crack at #theflattening on my 🛫 home. This is an initial cut of transitioning to where the most common implementations of ModelCollection don't have to operate on sectioned data but can instead store and operate on a simple array ([Model])

This is a first draft, wanted to push up a bit early to get some feedback on direction. Some known remaining TODOs before merge:

• Get alignment on approach vs alternatives
• Documentation / code comment update pass
• Dry run of migration on our apps to see if there's any unforeseen challenges
• Audit tests for any redundant (or missing) cases

Approach to transition

Decided to keep the diff engine sectioned, this meant ModelCollection (or the datasource?) needed to still provide some concept of sections so the previous property on ModelCollection sections: [[Model]] remains where the default implementation is just a wrapper on the new models: [Model] property.

Alternatives Considered

Option A: Protocol + Default Conformance

Add a new protocol SectionedDataSource which could look something like:

public protocol SectionedModelCollection {
    var state: [ModelCollectionState] { get }
}

and then default conformance for a ModelCollection could be as simple as:

extension ModelCollection: SectionedModelCollection {
    public var state: [ModelCollectionState] { return [state] }
}

This would be ideal since you could have the common single-sectioned model collections be completely obvlivious to the nature of sections while still overriding that in order to support them in cases where its valuable (ex: a MultiplexedModelCollection). Unfortunately swift doesn't allow adding conformance to a protocol via an extension. Which removes a lot of the awesomeness of this solution.

Option B: Sectioned as root protocol/interface

Make ModelCollection inherit directly from SectionedModelCollection:

public protocol ModelCollection: SectionedModelCollection {

    // MARK: Observable

    func addObserver(_ observer: @escaping  CollectionEventObserver) -> CollectionEventObserverToken
    func removeObserver(with token: CollectionEventObserverToken)

    var collectionId: ModelCollectionId { get }

    // MARK: State

    /// Current state of the model collection. Typically used by reference-type implementations, as value-type
    /// implementations stay `.Loaded`.
    var state: ModelCollectionState { get }
}

public protocol SectionedModelCollection: class {
    func addSectionedObserver(_ observer: @escaping  ???) -> Token
    func removeSectionedObserver(with token: Token)
    var sectionedState: [ModelCollectionState] { get }
}

public extension ModelCollection {
    func addObserver(_ observer: @escaping  ???) -> Token { ... }
    func removeObserver(with token: Token) { ... }
    public var sectionedState: [ModelCollectionState] { return [state] }
}

This has some nice properties, in that it allows all existing model collections to remain mostly unchanged, while reaping the benefits of being single-sectioned for the common code paths. However it has a few downsides:

1. Requires to have two observation types/paths. Due to model collection state changing to support the single section world, a model collection would need to have a way to observe it for updates while also providing a way to observe the sectioned data for updates. While it is possible to provide a default implementation for this, it still will be pretty messy/noisy on the client side of this API.
2. Requires more changes to CollectionViewModelDataSource/ViewController
3. There would be some additional friction where a concrete implementation of SectionedModelCollection couldn't be used/passed everywhere a ModelCollection would.

Option C: Remove concept of sections from Pilot

If we remove sections from the diff engine, it becomes pretty easy to flatten all references to [[Model]] in the app and delete a bunch of code. However, this comes at a pretty significant tradeoff for future use-cases where using sections makes a lot of sense, and didn't seem worth biting off at this moment.

[wkiefer]

Awesome work - thanks for digging into this!

I'm definitely onboard with Option A. Something like what you mentioned:

public protocol SectionedModelCollection {
    var sectionState: [ModelCollectionState] { get }
}

To get the "free" section support on ModelCollections, we could flip conformance like this:

extension SectionedModelCollection where Self : ModelCollection {
    public var sectionState: [ModelCollectionState] {
        return [state.models]
    }
}

which would require classes opting in to section support (which is probably fine). And then something like this could make code that requires sections (e.g. diff engine) easier, or "free":

extension ModelCollection {
    public func withSections() -> SectionedModelCollection {
        if let sectionedModel = self as? SectionedModelCollection {
            return sectionedModel
        }
        return StaticSectionedModelCollection(self)
    }
}

Long way of saying, this is great. Let's run with it!

[wkiefer]

We could remove this in the name of a smaller API surface (and it's not hard for apps to write). Also, we don't use it anymore.

[wkiefer]

Same as above. Could remove modelIdToIndexPathMap

[wkiefer]

I'm a fan of removing this now, while we're making breaking changes.

[wkiefer]

we also have ModelCollectionState.isEmpty.. wonder if that's too redundant.

[wkiefer]

What do you think about
a) moving these index path related methods to SectionedModelCollection ?
b) moving the three indexpath methods to a protocol and sectioned model collection & model collection can have their own implementations?

[wkiefer]

until swift 4 reduce improvements land, should this be a for substate in substates?

[wkiefer]

👍

[wkiefer]

this comment should still refer to sections

[wkiefer]

same, probably still sections instead of models since the diff engine still works on sections

[wkiefer]

same

[wkiefer]

Two more thoughts:

• ModelPath concept is a little odd now, but probably ok for the collection/table bindings.
• Do we want to land this on a branch to continue work, or as a single PR?