Sort edit commands during batch updating of table and collection views

[nguyenhuy]

#684, #693.

@levi, @Adlai-Holler: Please also let me know your thoughts on this PR. Thanks.

• Implement ASChangetSetDataController
• Failing ASTableViewTests (Periodic test failure: check for number of relayouts is sometimes 0 instead of 1. #758)
• Write documentation for ASChangetSetDataController
• Remove duplicate steps while coalescing changes in _ASHierarchyChangeSet
• Write tests for ASChangetSetDataController

[levi]

Gonna check this out tonight, @nguyenhuy.

[levi]

asdk_inverseCompare, or switch asdk_isFlowLayout to match.

[levi]

Clean addition on top of data controller. Would love it if the reasoning behind the design was well documented at the ASChangeSetDataController level (as described in a massive conversation in #684) and @Adlai-Holler's design of the change set objects at the internal level. Thanks for doing this, @nguyenhuy , I can tell you there will be a few happy people on my team once this lands. 👍

[nguyenhuy]

@levi Thanks for taking a look. I agree with you that ASChangeSetDataController deserves a better documentation. I'm just not so sure whether people would agree with me about the class existence. Its reasonings are separation of concerns, no UITableView/UICollectionView specific behaviours embedded in ASDataController and therefore reducing its complexity. Another minor benefit is that developers can switch back to ASDataController if ASChangeSetDataController causes any problems. I will add more documentation once others approve this approach.

That's said, it's really helpful if you could point out what are the things you would love to see in the documentation, from a new user (of that class) point of view.

[appleguy]

@nguyenhuy can you look at the test failures with the build? I did try re-running it, in case it was a flaky test, but it seems like it probably isn't.

[appleguy]

BTW - this is an extremely awesome patch and another huge contribution to the community. I can't believe how fast you did this following @levi's diff landing, lol!! Thank you!

[nguyenhuy]

Sure, I will look at the failed test. Looks like its the same one reported in #758.

Although I believe this diff should work nicely with @levi's supplementary-view feature, I admit that I haven't looked at the underlying diff closely and there might be some integration problems that I overlooked. I will recheck everything.

[appleguy]

Awesome! @nguyenhuy once the test is fixed, do you consider this diff ready to land, or believe that any iteration is needed?

[nguyenhuy]

Probably one iteration needed. I will add short documentation and some tests, as well as fixing small implementation details in _ASHierarchyChangeSet.

[levi]

@nguyenhuy just a quick glance at the conversations in the other issues, I'd love to see the documentation talk about the native ordering in UITableView/UICollectionView and that the ASChangeSetDataController is designed to simulate those characteristics. It's not entirely apparent to most people that table and collection views have a specific ordering of modification events.

[nguyenhuy]

@levi: yes, I will write that down. Thanks!

[Adlai-Holler]

Awesome PR! Pursuant to @levi's point, maybe include this link: https://developer.apple.com/library/ios/documentation/UserExperience/Conceptual/TableView_iPhone/ManageInsertDeleteRow/ManageInsertDeleteRow.html#//apple_ref/doc/uid/TP40007451-CH10-SW17

The xcdoc:// variant would be better but I can't get it to jump down to that specific section near the bottom of the document

[nguyenhuy]

@levi I removed duplicate steps in _ASHierarchyChangeSet in 7e3df3b 2b0829c. Please help me to review it.

@levi, @Adlai-Holler: I also added documentation to ASChangeSetDataController. Let me know what you think. Thanks.

@appleguy This PR is more or less ready to land. I will write some tests for these new classes in the next few days (and push to a new PR if this one landed by then).

[appleguy]

@nguyenhuy Thanks! Do you think this is safe to include the 1.9 release if I were to push it today? I would like to do the release today, and having this improvement would definitely be a milestone.

[nguyenhuy]

Yeah it would be nice to have this diff tested by other users. I will act quickly on any problems reported.

[appleguy]

@nguyenhuy could you take a quick look at the test failures now occurring on master? I'm downloading Xcode 6.4 so I can try to reproduce locally, but it's definitely blocked my work for the weekend (wanted to get a lot of PRs merged today, but lots else to do, like writing release notes / preparing release / working on documentation / ASPagerNode etc)

[nguyenhuy]

Btw, the failed test looks familiar and should be resolved by #770.

[nguyenhuy]

Yeah, looking at the failures on master.
On Oct 25, 2015 10:13 PM, "appleguy" notifications@github.com wrote:

@nguyenhuy https://github.com/nguyenhuy could you take a quick look at
the test failures now occurring on master? I'm downloading Xcode 6.4 so I
can try to reproduce locally, but it's definitely blocked my work for the
weekend (wanted to get a lot of PRs merged today, but lots else to do, like
writing release notes / preparing release / working on documentation /
ASPagerNode etc)

—
Reply to this email directly or view it on GitHub
#753 (comment)
.

[appleguy]

@nguyenhuy it looks like master is green, but even re-running the tests here is failing. It's possible the test is still flaky but I think this diff may have actually regressed something.

Check out the build...

Failures:
0) -ASTableViewTests testRelayoutAllRowsWithZeroSizeInitially
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
/Users/travis/build/facebook/AsyncDisplayKit/AsyncDisplayKitTests/ASTableViewTests.m:426: ((node.numberOfLayoutsOnMainThread) equal to (1)) failed: ("0") is not equal to ("1"):
423 for (int row = 0; row < NumberOfRowsPerSection; row++) {
424 NSIndexPath indexPath = [NSIndexPath indexPathForRow:row inSection:section];
425 ASTestTextCellNode *node = (ASTestTextCellNode *)[tableView nodeForRowAtIndexPath:indexPath];
426 XCTAssertEqual(node.numberOfLayoutsOnMainThread, 1);
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
427 XCTAssertEqual(node.constrainedSizeForCalculatedLayout.max.width, newSize.width);
428 }
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
* TEST FAILED: 213 passed, 1 failed, 0 errored, 214 total ** (49140 ms)

[appleguy]

@nguyenhuy "fortunately", it looks like the test failures are real. I re-confirmed master is passing tests on Xcode 6.4 locally, and checked out this PR to run them again and got the same failure as the builedbot (2 total tests in my case though, both testRelayoutAllRowsWithZeroSizeInitially and testRelayoutAllRowsWithNonZeroSizeInitially).

[nguyenhuy]

I think it's because Travis run the tests without virtually/temporary merging this branch into master, i.e the new fix on master isn't included. Let me rebase this branch.

[appleguy]

Oh, weird that it didn't rebase when re-running the build. I'm pretty sure it normally does that, but maybe some other automatic action usually triggers the rebase builds. Thanks, I'm excited to have almost all the open PRs merged.

Now we need master to be hit with a day of testing to pave out the recent table changes. I'd like to reset the 1.9 release tag point to include the recent improvements, if at all possible.

Could folks bump the ASDK version you're testing against and let us know if you hit anything? @rcancro, @eanagel, @levi, @Adlai-Holler

[appleguy]

@nguyenhuy hmm, still failed in the number of layouts on main thing...

https://travis-ci.org/facebook/AsyncDisplayKit/jobs/87395262

/Users/travis/build/facebook/AsyncDisplayKit/AsyncDisplayKitTests/ASTableViewTests.m:426: ((node.numberOfLayoutsOnMainThread) equal to (1)) failed: ("0") is not equal to ("1"):
423 for (int row = 0; row < NumberOfRowsPerSection; row++) {
424 NSIndexPath *indexPath = [NSIndexPath indexPathForRow:row inSection:section];
425 ASTestTextCellNode *node = (ASTestTextCellNode *)[tableView nodeForRowAtIndexPath:indexPath];
426 XCTAssertEqual(node.numberOfLayoutsOnMainThread, 1);
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
427 XCTAssertEqual(node.constrainedSizeForCalculatedLayout.max.width, newSize.width);
428 }

[nguyenhuy]

Ok, I will recheck everything here. Thanks for the heads up :)

[appleguy]

If it’s late wherever you are, don’t rush! I need to write the release notes anyway, which I won’t have done for approximately 24hrs anyway.

    — Scott

On Oct 25, 2015, at 9:35 PM, Huy Nguyen notifications@github.com wrote:

Ok, I will recheck everything here. Thanks for the heads up :)

—
Reply to this email directly or view it on GitHub #753 (comment).

[nguyenhuy]

It's early morning over here. But yeah, shouldn't rush this thing. Thanks.

[nguyenhuy]

@appleguy You are right, Travis does merge the branch with master and run the tests on top of the merge commit. I just had a hard time believing the tests were still failing and desperately looked for someone to blame. How silly I am! But I think I found out the root cause and fixed it. All checks have passed 👏

[appleguy]

This may be one of the most satisfying-to-click Merge buttons I've done for ASDK :). ASDataController has really gone from the Achilles heel of the framework to one of its most capable and unique components.