Add collection view batch updates

[JosephDuffy]

The idea of this is that without breaking the API the collection view will batch updates.

Inserting a large number of sections is the main area of performance loss we are currently encountering, because the sections are inserted individually and not batched. This change alone has reduce the initial load time of one our screens (which has 100-150 sections added at once) from 30-45 seconds down to less than a second (at least it is not noticeable).

I had created composed-swift/Composed#17 to try and address this, which has the advantage that it would apply to other view types (e.g. UITableView), but I believe does not offer the same performance improvements and it is restricted to a single ComposedSectionProvider.

This is a draft to collect feedback; as you can see there are some TODOs but I think there's enough implemented to provide an overview of the changes that would be required to implement this.

This does not currently work; there are situations that cause the collection NSInternalInconsistencyException', reason: 'Invalid update error. I have some failing tests that demonstrate what the result should be.

[shaps80]

Ok so its obviously a lot more complex and likely this is necessary. I'm never a big fan of index tracking since its very easy to break.

Could we improve this with perhaps a type that maintains an index. Then include a few accessors like:

stack.push()
stack.pop()
stack.isEmpty

EDIT: I'm obviously referring to the += and -= balance calls in the mapping.

[shaps80]

👍

[shaps80]

Not sure what's going on here with the whitespace?

[JosephDuffy]

This is how Xcode indents when copy/pasting or using re-indent.

I've reverted it, although I think it's clearer when some extra line breaks are added and running re-indent:

guard
    let section = self.sectionProvider.sections[indexPath.section] as? CollectionUpdateHandler,
    !section.prefersReload(forElementAt: indexPath.item),
    let cell = self.collectionView.cellForItem(at: indexPath)
else {
    indexPathsToReload.append(indexPath)
    continue
}

[shaps80]

I think that's fine.

[JosephDuffy]

Ok so its obviously a lot more complex and likely this is necessary. I'm never a big fan of index tracking since its very easy to break.

I'm not enjoying it and I'm worried it's fragile, but the (pretty large and growing) test suite makes me feel a little better about it.

Could we improve this with perhaps a type that maintains an index. Then include a few accessors like:

stack.push()
stack.pop()
stack.isEmpty

EDIT: I'm obviously referring to the += and -= balance calls in the mapping.

I thought that tracking some sort of index (I was using the index provided by the mapping) would be useful/required, but the issue is that we don't know the order change could occur so I don't think there's a single index that can be tracked that helps.

At first I was going to create a Change type that contains the data of the change, but always with a var section: Int property to make some of the updates easier, e.g.:

allChanges.map { change in
	guard change.section > removedSection else { return change }

	var change = change
	change.section -= 1
	return change
}

But this isn't actually that useful and could cause more confusion because of differences with how changes are handled (see how sections removes modify other section removes for example)

I have been thinking about some sort of Changeset type that contains this logic though. It would allow for reuse with UITableView and would be a single unit that can be tested, rather than the current kind of hacky testing. The current tests actually apply the changes to a UICollectionView though so they are useful for catching mistakes in the expected results because the collection view will trigger an exception.

[shaps80]

Perhaps I'm misunderstanding but for the index stack I suggested, I meant for this value? Not sure how your response relates? Maybe you can elaborate here.

Also, yeah reuse across other coordinators makes total sense and esp from a testing POV.

[JosephDuffy]

Ah I thought you were referencing the index changes performed in each of the update delegate methods.

Moving batchedUpdatesCount to the Changeset type mentioned is part of the plan for that

[shaps80]

Ok cool.

[shaps80]

Only one question on the review. Otherwise, looks much better. Purposeful at least and definitely more testable and reusable.

EDIT: Approved by accident hahah!

[shaps80]

I assume you plan on adding more test cases later on?

[JosephDuffy]

This is only the start -- the CollectionCoordinator doesn't use it yet.

More tests will be added to this file before merging, but I think it's going to lean a little on the new CollectionCoordinator tests passing to validate correctness according to UICollectionView.

[JosephDuffy]

@shaps80 This test, along with testMoveElementAtSameIndexAsRemove below, are currently failing but I'm not 100% sure about the tests themselves.

Can you confirm that the test looks correct? e.g. this should produce removes and inserts, not a remove and moves?

[JosephDuffy]

Moving to main repo with merged libraries: composed-swift/Composed#28