[critical] Fix Table View Updating Errors

[Adlai-Holler]

This resolves #869. @appleguy

See below. The diff is hard-to-read since I reordered lines, but all that is different is when we enumerate changes in reverse (for deletes) our "end index" is actually the first index in the set, not the last index.

[Adlai-Holler]

Actually, I've spotted another potential bug (not a new one) that I should fix as well.

[Adlai-Holler]

OK @appleguy now it's ready to rock. Go with split view for the diff IMO. We're trying to coalesce changes into contiguous blocks that all have the same animation option, so we can submit them in safe order up to the data controller.

The way the grouping works now, for each element in the sorted array, we do:

1. End current group if non-empty and animation option for this change doesn't match it.
2. Update current animation option if current group is empty.
3. Add element to current group.

And then after the enumeration, if the current group is non-empty, we end that group as well.

[Adlai-Holler]

OK I've added a fix for a separate issue. The issue was, if you inserted a section and inserted items into that section in the same transaction, we'd report those item inserts twice. Similar for deletions in deleted sections.

[Adlai-Holler]

@appleguy I know this is a tricky PR because it's touching such important code that currently has low test coverage. Let me know if I can do anything to ameliorate that other than add tests – which I can do later but don't have time/energy for right now.

[Adlai-Holler]

I've improved the item-change exclusion logic. For item deletes/reloads, we exclude item changes in sections that were deleted or reloaded. For item inserts, we exclude item changes in sections that were inserted or that correspond to reloaded sections.

[appleguy]

Thanks @Adlai-Holler - this makes my Thanksgiving! :-D @levi, @nguyenhuy, @eanagel if any one of you has a chance to review, please do. I may land this before anyone else can review (in part because I really trust the four of you, from past experience seeing the quality of your work and testing), but Because this code has had trouble since its inception, additional reviews of fixes are always good to help spot issues.

[appleguy]

@Adlai-Holler This looks good - definitely some stylistic cleanups / simplifications in here too, which is awesome. I think we could stand to add some additional comments in the code or give more descriptive names than just "current..." for some of the local variables to aid reading and debugging, but at the same time, I think we have reason to believe this code is very close to a stable implementation and it should not need significant changes in the future once it is dialed in.

[appleguy]

@Adlai-Holler regarding tests, that would definitely be an awesome next step to ensure these sensitive code paths never regress again - and thus enable us to refactor with greater ambition and confidence in the future.

We do have an Issue tracking test coverage in general, with ASTableView / ASCollectionView being of key value, so no worry / guilt if you don't have time to do this. You really have contributed a lot, so I think it is very reasonable to expect another engineer to beef up the tests. I will make sure we complete this in the next couple months, and specifically before any major refactoring in ASDataController or the base Table / Collection classes.

[Adlai-Holler]

Excellent!

Sent from my iPhone

On Nov 26, 2015, at 4:26 PM, appleguy notifications@github.com wrote:

@Adlai-Holler regarding tests, that would definitely be an awesome next step to ensure these sensitive code paths never regress again - and thus enable us to refactor with greater ambition and confidence in the future.

We do have an Issue tracking test coverage in general, with ASTableView / ASCollectionView being of key value, so no worry / guilt if you don't have time to do this. You really have contributed a lot, so I think it is very reasonable to expect another engineer to beef up the tests. I will make sure we complete this in the next couple months, and specifically before any major refactoring in ASDataController or the base Table / Collection classes.

—
Reply to this email directly or view it on GitHub.