Auto diffing section controller #494

rnystrom · 2017-02-15T16:03:14Z

Started work on the plane to get this moving since #418 is up and ready to land. We'll likely need to spend some time fleshing out the API of this, and I think I'll split it up into a couple different PRs once ready for review. Putting this up now to get early feedback.

What is this?

This adds an auto-diffing section controller as outlined in #38. There are several key parts:

Subclass a new section controller IGListAutoSectionController (naming wip)
Connect a data source
Implement the data source methods that do 3 things:
- Given a top-level object, transform it into an array of diffable view models
- Given a view model, return a cell
- Given a view model, return a size for a cell
A new protocol for the cell IGListBindable so that we can control when the cell is updated w/ the view model.
- The most important part of this is that it unlocks moving and reloading a cell, which you can't do w/ UICollectionView

TODO

IGListAdapter

I had to make some changes to IGListAdapterUpdater in order for this to work. The biggest challenge is when the section controller receives a new object, it has to split it up into view models and then apply changes.

However, didUpdateToObject: will be called inside performBatchUpdates:, so we need to diff and fire off the changes on the collection view immediately. A few issues:

You couldn't use -[IGListCollectionContext performBatchAnimated:updates:completion:] inside didUpdateToObject: and have it update immediately (async updates)
There are other ways that didUpdateToObject: gets called, so we can't just insert/delete/move
We're planning to remove non-batched updates entirely in RFC: Remove item-level updates from IGListCollectionContext, require them done in batch updates #392

Diffing top-level models

One super important piece to getting this right is having top-level (the ones given to IGListAdapter and handed to the SC in didUpdateToObject: always equal itself, even if its values change. I solved this in the unit tests w/ an object that implements diffing like:

@interface IGTestObject: NSObject <IGListDiffable>
@property NSString *key;
@end

@implementation IGTestObject
- (id)diffIdentifier {
  return self.key;
}

- (BOOL)isEqualToDiffableObject:(id)object {
  return [self.key isEqual:[object key]];
}
@end

If you use a traditional isEqualToDiffableObject: (where you check all props/values) you'll end up reloading the section, avoiding all the nice cell animations.

I started making a stock object that does this, something like IGListAutoObject, but its so simple that it's kind of annoying. It's essentially a model box that will probably end up more confusing than having to learn how to diff w/ the auto SC.

Would love thoughts here.

Selection

didSelectItemAtIndex: is totally broken. Options are:

Add a new method on the data source protocol
- Gross b/c that's another empty method for a lot of folks
Add another delegate protocol you can optionally wire up

We've discussed splitting off didSelectItemAtIndex: into a selectionDelegate (like the display/scroll/etc delegate) in #184 and elsewhere. That might help, though the view models array is private, so we'd have to expose that or create another delegate API.

How can I help?

Since this is super early, please hold on nit picks, formatting, etc. I'll work on that as we go along. But there are some big things I'd love your thoughts on:

The name IGListAutoSectionController. Anything better out there? I thought IGListDiffableSectionController but idk
I'm using a data source to split the object into view models, return cells, and handle sizes. I started off passing the SC blocks on init that did the transform (sort of like IGListSingleSectionController). I thought the API was kind of gross though, and you have to subclass the SC anyways.
IGListBindable is great, but you can't just use the view model to config the cell. For instance, how do you make the SC the cell delegate? That's why I'm leaving the method to still return a cell, but we will call bind before returning it. Is this ok?
Sample project now or later?
Let me know if you see some glaring Swift interop issues. I haven't tried it yet.
Throw all the crazy test-case scenarios you got at me!

facebook-github-bot · 2017-03-01T14:48:41Z

@rnystrom updated the pull request - view changes

heumn · 2017-03-01T17:14:27Z

The name IGListAutoSectionController. Anything better out there? I thought IGListDiffableSectionController but idk

I think it is a good name. It describes what it does pretty well without being too verbose 👌

To get a better feeling for how it would be to use this, it would be awesome with a description on how to test it (in an example project) 💯

jessesquires · 2017-03-01T17:47:14Z

How about IGListAutoDiffSectionController ?

heumn · 2017-03-01T17:51:39Z

Yeah, IGListAutoDiffSectionController is actually a better name

rnystrom · 2017-03-01T18:57:53Z

@jessesquires @heumn we had some convo in #449 with other ideas:

IGListAutoSectionController
IGListReactSectionController
IGListDiffableSectionController @zhubofei
IGListDiffingSectionController @amonshiz
IGList(Cell)BindingSectionController
IGListViewModelSectionController

I like IGListAutoDiffSectionController too!

rnystrom · 2017-03-04T22:25:56Z

Will rebase off of #525 to lighten the load here.

rnystrom · 2017-03-04T23:09:54Z

Anyone got any ideas on what we could demo as an example? Something that isn't crazy complicated, but would show off cells updating automatically.

heumn · 2017-03-05T12:04:58Z

Maybe something like you do in the nested controller example, but use colours and circles to easier track the diffing inside the section?

Can not wait to play with this :)

Summary: Batch updates are complicated b/c its unknown when the update block will actually execute. When executing the block, we want to collect inserts/deletes/reloads/moves (item and section). Allow mutations to happen synchronously outside of the update block. Tracking state will also help with the auto-diff where we need to allow re-entrant updates. Peeling off a chunk from #494 Issue fixed: #288 - [x] All tests pass. Demo project builds and runs. - [x] I added tests, an experiment, or detailed why my change isn't tested. - [x] I added an entry to the `CHANGELOG.md` for any breaking changes, enhancements, or bug fixes. Closes #525 Reviewed By: jessesquires Differential Revision: D4656463 Pulled By: rnystrom fbshipit-source-id: 8f4d3dc21b03d595e02ee9ee9664277e8ead0531

jessesquires · 2017-03-07T19:28:21Z

@rnystrom - can we defer this to 3.1.0 or does this contain breaking changes?

rnystrom · 2017-03-11T20:10:06Z

@jessesquires no breaking changes, we could kick this from 3.0 but I think I'll have this working + example this weekend.

facebook-github-bot · 2017-03-11T20:32:43Z

@rnystrom updated the pull request - view changes

facebook-github-bot · 2017-03-11T22:32:04Z

@rnystrom updated the pull request - view changes

rnystrom · 2017-03-11T22:33:05Z

Ok, I'm getting super excited about this. Check out the demo (gif below).

I'm about ready to move this from WIP to review. Should be ready tomorrow.

cc @jessesquires @zhubofei @heumn @Sherlouk @PhilCai1993 for feedback

heumn · 2017-03-12T11:20:43Z

Initial reaction to this is that you've just saved me hundreds of lines of code and a hack 😂 Looks great

I also postponed "Call mom" until the 25th, and rather "Started cycling" the 20th. No flashing reloading of the sections and things updated just as I wanted 👌 💯

Maybe it could be an idea to add something like this to extend on how powerful this is?

I also keep thinking how you could make it extra clear that the section models has to follow this principle? No ideas apart from the obvious documentation that is needed :)

If not later today then tomorrow I will have more time to start refactoring on my current project and get more hands on experience with this

One super important piece to getting this right is having top-level (the ones given to IGListAdapter and handed to the SC in didUpdateToObject: always equal itself, even if its values change.

heumn · 2017-03-12T18:49:46Z

Btw, I forgot to that big f***ing thank you for the awesome work you are putting in @rnystrom 🙌

rnystrom · 2017-03-12T19:50:52Z

@heumn great idea! I'm going to add an assert inside -[IGListDiffingSectionController didUpdateToObject:] that checks [object isEqual:self.object] (e.g. two objects should always be equal otherwise a section-reload happens). Also going to load the headers w/ docs and examples.

And nice extension! Maybe we can add that as a follow up?

heumn · 2017-03-12T19:53:08Z

@heumn great idea! I'm going to add an assert inside -[IGListDiffingSectionController didUpdateToObject:] that checks [object isEqual:self.object] (e.g. two objects should always be equal otherwise a section-reload happens). Also going to load the headers w/ docs and examples.

Perfect 🎉

And nice extension! Maybe we can add that as a follow up?

👌

…cOS target

rnystrom · 2017-03-13T02:05:25Z

Importing for internal review!

facebook-github-bot · 2017-03-13T02:05:59Z

@rnystrom has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

jessesquires · 2017-03-13T02:51:37Z

that calendar view though 😚 👌

amonshiz · 2017-03-13T10:30:04Z

Early morning review before a run is probably the best kind of review right?

rnystrom · 2017-03-13T14:24:15Z

@amonshiz forget to hit "Submit Review"? 🤔

amonshiz · 2017-03-13T10:28:50Z

Source/IGListDiffingSectionController.m

+
+#pragma mark - Public API
+
+- (void)updateAnimated:(BOOL)animated completion:(void (^)())completion {


I don't think this completion block is getting used. And I may have missed it but I don't think it's use is being tested either.

Good eye! Means I need to add a test for this too.

amonshiz · 2017-03-13T14:56:31Z

@rnystrom yup! reviewing and trying to comment on github at 6:15am right after waking up is clearly not what i'm best at.

iglistkit-bot · 2017-03-14T02:11:32Z

	1 Warning
⚠️	Big PR

Generated by 🚫 Danger

rnystrom · 2017-03-14T02:58:23Z

Waddup @iglistkit-bot catching my own mistakes. I'm in love w/ this tool.

rnystrom · 2017-03-14T19:55:01Z

I'm still going back and forth on naming. Now I'm starting to lean towards IGListBindingSectionController b/c:

The section controller does the "binding"
"Diffing" is an implementation detail

The emphasis of this API should be on the model -> view model -> binding parts, not that it diffs.

What do you all think? @jessesquires @heumn @PhilCai1993 @amonshiz

amonshiz · 2017-03-14T21:22:33Z

sure? i'm bad at naming.

jessesquires · 2017-03-15T01:05:49Z

i'll defer to @amonshiz for naming strategies.

facebook-github-bot · 2017-03-15T01:57:40Z

@rnystrom has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

heumn · 2017-03-15T07:57:31Z

I'm still going back and forth on naming. Now I'm starting to lean towards IGListBindingSectionController b/c:

The section controller does the "binding"
"Diffing" is an implementation detail
The emphasis of this API should be on the model -> view model -> binding parts, not that it diffs.

What do you all think? @jessesquires @heumn @PhilCai1993 @amonshiz

Good reasoning 👌 👍

PhilCai1993 · 2017-03-17T02:36:03Z

It's excited! I wish I could download this commits but how? 😅

heumn · 2017-03-17T05:55:22Z

It was merged into master, so you can just point you podfile to the master branch :)

jessesquires · 2017-03-17T16:39:23Z

@PhilCai1993 check out installation guide

https://instagram.github.io/IGListKit/installation.html

PhilCai1993 · 2017-04-06T03:24:48Z

May I have a question here? If in a collectionview with various "Month", since isEqualToDiffableObject: simply returns true, how to reload the entire collectionview?

rnystrom · 2017-04-16T15:34:27Z

@PhilCai1993 covering that in #621 I think

rnystrom added this to the 3.0.0 milestone Feb 15, 2017

rnystrom mentioned this pull request Feb 15, 2017

WIP: Auto diffing section controller #449

Closed

8 tasks

facebook-github-bot added the CLA Signed label Feb 15, 2017

heumn mentioned this pull request Feb 22, 2017

Out of bounds collectionview update (crash) #503

Closed

2 tasks

rnystrom mentioned this pull request Feb 22, 2017

Design choice and a heavy load advice #357

Closed

2 tasks

rnystrom force-pushed the auto-diff1 branch from 0ae5a5c to 7c97f72 Compare March 1, 2017 14:47

rnystrom mentioned this pull request Mar 4, 2017

Track batch updates in object and explicit state #525

Closed

3 tasks

rnystrom force-pushed the auto-diff1 branch from 7c97f72 to c3d19d5 Compare March 11, 2017 20:32

Ryan Nystrom and others added 4 commits March 12, 2017 15:26

wip

72c4175

rebase from master

ba23fb3

everything working

fe52f77

add example

e31ee19

rnystrom force-pushed the auto-diff1 branch from d6b8453 to 44cec64 Compare March 12, 2017 20:47

rnystrom changed the title ~~WIP: Auto diffing section controller~~ Auto diffing section controller Mar 12, 2017

Ryan Nystrom added 3 commits March 12, 2017 17:05

diffing selection delegate public

7109171

fix unit tests

1293bb8

test moves and reloads, fix macOS tests, remove diff SC stuff from ma…

8655760

…cOS target

amonshiz reviewed Mar 13, 2017

View reviewed changes

Ryan Nystrom added 2 commits March 13, 2017 11:54

test completion block, execute block, pass animated param

e57e9bd

fix test shadow var

2ebb5ef

add changelog entry

cf6d7f2

rename to binding

674a4f7

facebook-github-bot closed this in 7934816 Mar 15, 2017

jessesquires deleted the auto-diff1 branch March 15, 2017 15:32


		#pragma mark - Public API

		- (void)updateAnimated:(BOOL)animated completion:(void (^)())completion {

Auto diffing section controller #494

Auto diffing section controller #494

Conversation

rnystrom commented Feb 15, 2017 • edited Loading

What is this?

TODO

IGListAdapter

Diffing top-level models

Selection

How can I help?

facebook-github-bot commented Mar 1, 2017

heumn commented Mar 1, 2017 • edited Loading

jessesquires commented Mar 1, 2017

heumn commented Mar 1, 2017

rnystrom commented Mar 1, 2017

rnystrom commented Mar 4, 2017

rnystrom commented Mar 4, 2017

heumn commented Mar 5, 2017

jessesquires commented Mar 7, 2017

rnystrom commented Mar 11, 2017

facebook-github-bot commented Mar 11, 2017

facebook-github-bot commented Mar 11, 2017

rnystrom commented Mar 11, 2017 • edited Loading

heumn commented Mar 12, 2017 • edited Loading

heumn commented Mar 12, 2017 • edited Loading

rnystrom commented Mar 12, 2017

heumn commented Mar 12, 2017

rnystrom commented Mar 13, 2017

facebook-github-bot commented Mar 13, 2017

jessesquires commented Mar 13, 2017

amonshiz commented Mar 13, 2017

rnystrom commented Mar 13, 2017

amonshiz Mar 13, 2017

Choose a reason for hiding this comment

rnystrom Mar 13, 2017

Choose a reason for hiding this comment

amonshiz commented Mar 13, 2017

iglistkit-bot commented Mar 14, 2017 • edited Loading

rnystrom commented Mar 14, 2017

rnystrom commented Mar 14, 2017

amonshiz commented Mar 14, 2017

jessesquires commented Mar 15, 2017

facebook-github-bot commented Mar 15, 2017

heumn commented Mar 15, 2017

PhilCai1993 commented Mar 17, 2017

heumn commented Mar 17, 2017

jessesquires commented Mar 17, 2017

PhilCai1993 commented Apr 6, 2017

rnystrom commented Apr 16, 2017

rnystrom commented Feb 15, 2017 •

edited

Loading

heumn commented Mar 1, 2017 •

edited

Loading

rnystrom commented Mar 11, 2017 •

edited

Loading

heumn commented Mar 12, 2017 •

edited

Loading

heumn commented Mar 12, 2017 •

edited

Loading

iglistkit-bot commented Mar 14, 2017 •

edited

Loading