[ASRangeController] properly update _rangeTypeIndexPaths when adding/removing index paths and sections

[knopp]

This fixes #1028

[appleguy]

@knopp thanks a lot, I can't tell you how awesome it is to have your help here - especially in the production implementation, as I'm hoping to get the Beta working well enough in the next several weeks, but still it will be a riskier direction for any production apps than fixing any holes in the Stable version.

[appleguy]

This issue is avoided by the recompute-everything architecture of the Beta controller, right?

We are starting work soon on asynchronously applying the Display and FetchData ranges in the Beta version, on top of really substantial performance improvements I landed recently (I'd made a couple pretty stupid mistakes that just hadn't been tested through yet). You might try out Beta in your code and let me know if there are any immediate problems you detect.

[knopp]

@appleguy, in a way this is also an issue with ASRangeControllerBeta, because you store _allPreviousIndexPaths between _updateVisibleNodeIndexPaths invocations. When adding / removing nodes, the index paths in _allPreviousIndexPaths become stale and may point to items no longer existing data controller.

[appleguy]

@knopp aha, ok. Yes I actually ran into that but fixed it by handling nil nodes and skipping over them in the Beta controller, so I'm pretty sure that is a stable / correct behavior for the newer one. Most important thing now is getting a new release out to fix this.

[knopp]

@appleguy, which commit are you referring to? Also, are you sure that merely handling nil nodes is correct? When adding / removing nodes, the index paths in _allPreviousIndexPaths may simply point to wrong nodes, but still be non null. So even if it doesn't crash, it won't behave correctly.

I think there are two possible solutions - either do what I did in the branch - update _allPreviousIndexPaths every time the data controller adds/removes nodes/sections, or simply discard _allPreviousIndexPaths when adding/removing and next time _updateVisibleNodeIndexPaths is called, iterate over all nodes from data source.

[appleguy]

@knopp are you sure it won't behave correctly in the *Beta version? Very very interested in your thoughts there. For each index path, we independently check if it currently lies within each range. I think that should be sufficient, but I have not yet deeply considered this _editing vs _completed nodes issue. _editingExternalNodes was added after my original refactor of ASDataController and I'm actually pretty concerned about it, and will evaluate if it is possible to re-simplify some of that stuff.

For this diff, do you have any test apps / test cases that you used? I want to get a fix for this issue out as quickly as possible, and I would believe that this approach addresses conditions that #1060 does not, but I am worried about pushing this out with minimal testing. Is there a scenario you can describe that does in fact break #1060 (or more importantly, *Beta, because it will become Stable within a couple weeks), but is addressed by this change?

[knopp]

@appleguy, here is project that is broken with current ASRangeController but works with this pull request: https://github.com/tomizimobile/ASDKSearchIssue

Regarding ASRangeControllerBeta: Each index path (allIndexPaths) that you check belongs to union of fetch, display, visible and _allPreviousIndexPaths. Now consider following scenario:

We have node[0,0] that is visible, and appropriate indexPath [0,0] is in _allPreviousIndexPaths; Let's call it original node;

Now this happens between invocations of _updateVisibleNodeIndexPaths

1. New node is added to [0,0]; This causes original node index path [0,0] to become [0,1].(_allPreviousIndexPaths doesn't change in any way though and still contains [0,0])
2. Collection view is instantly scrolled in a way where section 0 is no longer in fetch, display or visible;

The original node is still marked as visible, even though it no longer is. That is because the union of fetch, display, visible and _allPreviousIndexPaths contains [0,0]. But original node is no longer [0,0], it is now [0,1] (because another node was added to [0,0]. So the original node is missed while iterating allIndexPaths and not updated.

[knopp]

It's probably not worth committing this. ASRangeControllerStable has other issues as well, so let's focus on ASRangeControllerBeta instead.

[appleguy]

@knopp ok, I'll take your advice. I tend to agree insofar as there is risk in changing code that is this fragile.