Collection/Table view changes should be sorted

[Adlai-Holler]

UITableView and UICollectionView allow you to make changes in any order during batch updates. When endUpdates is called, the changes are sorted and applied in a safe order. The ASDK versions don't, so if your changes (say, from an NSFetchedResultsController) aren't submitted in a specific order, you'll get an index-out-of-bounds crash sooner or later.

Here's the order changes must be applied in, so we should update ASDataController to accommodate this:

1. Updated/reloaded items
2. Deleted items, descending order
3. Deleted sections, descending order
4. Inserted sections, ascending order
5. Inserted items, ascending order

EDIT: Oh and moves are tricky. Best bet is to break them up into delete/insert, because the fromIndexPath is pre-update while the toIndexPath is post-update. So tell the UITableView about the move whenever you want, but update the array by breaking the move into two operations that'll get sorted separately.

[smyrgl]

This has been a seriously problem for my app and I'm not sure your ordering changes are entirely correct (I suspect it is the moves that are causing the problem as I tend to have a lot of these) but I'm trying to make your workaround function and I'll update if I can point to specific steps that are failing.

[smyrgl]

Wait one thing about your solution is confusing to me.

So tell the UITableView about the move whenever you want, but update the array by breaking the move into two operations that'll get sorted separately.

So in UITableView you update your backing datastore (I assume that's what you mean by array) before you start telling the table view about the batch updates. Are you suggesting that you sort the changes and then update a backing datastore incrementally as the batch updates are occurring (using the lock/unlock calls on the ASTableViewDatasource to ensure consistency)? I could try that but it would require adding a storage object which mirrors what the table view state is at any given time.

[Adlai-Holler]

Thanks for engaging on this @smyrgl! As a user of ASDK, there's currently no safe way to do moves intermingled with other changes in a batch update. This is because ASTableView immediately applies both the delete and insert for the move, which is unsafe. This is a pretty major problem.

For example, say you have 1 section with 7 items ABCDEFG. You want to do a batch update with (move 5 to 1), (insert X at 0), so you expect the end result to be XFABCDEG.
If you tell ASTableView about the changes in that order you'll end up with XAFBCDEG which is wrong. If you tell ASTableView about the changes in reverse order you'll end up with XEABCDFG

I may not understand your question about updating your backing store… it's fine to change it all at once just as you always have. If your backing data store is a 2D array, then you need to apply these changes in the correct order just like ASTableView/ASCollectionView to avoid errors. If that's not really what you're getting at, let me know!

EDIT: Here's an operation I'm using in a production app to apply updates from an NSFetchedResultsController to an ASTableView/ASCollectionView. It's a little hairy since it's from production but hopefully it'll help https://gist.github.com/Adlai-Holler/a67f7a96779952ed5062

[nguyenhuy]

Hi guys, thanks for your useful reports and explanations. I've been thinking about ASDataController lately and realized that there are 2 separate problems:

1. Operations within a transaction are applied on-the-fly to UITableView/UICollectionView: This causes inconsistency between ASDataController's _completedNodes and UITableView/UICollectionView's state after each operation. This is reported in ASCollection/TableView nodeForRowAtIndexPath not safe when using begin/endUpdates #619 (if I understand the issue correctly). I have some ideas on how to fix this and experimenting them.
2. Operations are not sorted the way UITableView and UICollectionView do: On the way to fix this issue, I imagine the major obstacle is that ASDataController uses a block-based queue, so we can't really sort them without some kind of metadata for each block. My current idea is to catch and normalize all operations between beginUpdates and endUpdates calls into easy-to-query data structures, sort those structs and finally fire editing method calls to ASDataController (in the correct order). To avoid breaking current clients, this strategy may be implemented as an extension on top of ASDataController for now (although I really think ASDataController should be refactored to use an id/metadata based queue. But without a good test coverage, I don't have the courage to do it :( ).

Anyway, I will try to propose a fix for the first problem in 1 or 2 days. The fix for the second problem will come after that, if it's still unfixed by that time.

P/S: I briefly read the code to see how ComponentKit does edits. This would be the entry point to explore. While I don't think we are gonna add the change set pattern (at least as a public API), we can probably study the sorting algorithm.

[smyrgl]

@Adlai-Holler Thanks for the gist...I'll try applying it. What I mean by the backing store is whatever the Table/Collection view is using as its datasource. The reason I mentioned changing the store during the batch operation is that right now I noticed that ASTableView is that if you (for example) tell ASTableView to do an insert operation after calling beginUpdates() it actually does the insert operation and rebuilds the cell node immediately and calls lock/unlock to the datasource to ensure consistency. What I gather from this is that the intent of theASTableView datasource protocol is that it wants the data updated as you go through the batch updates--it is not like the UITableViewDataSource protocol that expects all of the data to be updated ahead of time while the batch updates are the formula to rationalize between the old and new state of the table view. Because the datasource is being queried DURING the batch process I'm not sure how this could work unless the ASTableViewDataSource is also being updated incrementally. Please feel free to correct me if you think I'm misunderstanding any of this.

@nguyenhuy I think you are right about the issues, the danger here is that because ASTableView mimics the UITableView API as closely as possible it can downright confuse people what's going on. If you don't think you guys are going to deal with changes the way UITableView does I can understand that but it probably would be a good idea to add some documentation around exactly how the ASTableViewDataSource needs to behave and what the right way of handling these change operations are. I assume (please correct me if I'm wrong) that the way you expect users to use this API is something like this:

1. Receive change list.
2. Call beginUpdates()
3. Enumerate through the change list, for each item dispatch a block operation to a serial operation queue which will make the change to whatever is acting as the ASTableViewDataSource and to tell the table view to insert/delete/move/update the row section associated with that block. The calls to lock/unlock dataSource on the ASTableViewDataSource will suspend this queue.
4. Once all of the changes have been processed called endUpdates().

Do I have that right?

[Adlai-Holler]

@smyrgl I think you're wrong about the difference in data source behavior i.e. the data source is not queried during the batch process. ASDataController enqueues those update blocks and drains the queue on endUpdates, so it's fine to update your data source all at once (I like to do this just after beginUpdates and before I submit any update operations, but I believe it's fine to do it any time before endUpdates)

[smyrgl]

@Adlai-Holler Checking my debug logging it looks like you are right--it threw me off how the dataSource lock/unlock is being called before/after each batch event like so:

�[;�[fg0,255,0;2015-09-26 17:44:47:771 Disqus[3340:6215626] Locking data source
�[;�[fg0,255,0;2015-09-26 17:44:47:771 Disqus[3340:6215626] Rebuilding node at indexPath <NSIndexPath: 0xc000000000c00016> {length = 2, path = 0 - 6}
�[;�[fg0,255,0;2015-09-26 17:44:47:771 Disqus[3340:6215626] ActivityID: 26dab260-64a2-11e5-9ff2-00004d6a4600 Title: Optional("Miniature Art Show ;)") Time: 2015-09-26 22:58:50 +0000, String: 2h
�[;�[fg0,255,0;2015-09-26 17:44:47:773 Disqus[3340:6215626] Unlocking data source

But there are no calls to numberOfSectionsInTableView: or tableView:numberOfRowsInSection: being made. That makes this a significantly easier problem to work around. Thanks for the clarification!

[Adlai-Holler]

You got it man, happy to help!

[Adlai-Holler]

@nguyenhuy Glad to have you in here, thanks for the input and analysis! I'll take care of the second one in the next couple days. I want to understand the risk to current clients you mentioned – could you elaborate on that?

[smyrgl]

@Adlai-Holler Not to pollute this thread too much but I applied a fix based on your suggestion and it seems to resolve the issues even for move operations. Thanks a bunch for the help with that.

[appleguy]

Thanks everyone, @smyrgl, @Adlai-Holler, @nguyenhuy ! I apologize for the implementation failure in this respect. It's squarely my error, and due to the fact that I didn't recognize there were a set of unique considerations for applying batch changes. This is largely due to the fact that my work in building tests for this area was cut short after I created the thrash test that did exercise basics of batch requests, but was clearly not sophisticated enough (I didn't feel it was "done" at the time, but thought it was unlikely there were further issues).

Taking the time to report and assist in debugging issues like this is a great volunteering of value to the ASDK community, and I really appreciate it. It looks like we are upon a final solution here. @nguyenhuy has a patch up that would be great for you to try and comment on.

My next few diffs coming in relate to more advanced control of display, including the option to prevent placeholders from displaying when scrolling (e.g. if the working range fails to finish drawing something, block main for a brief time while the final subnodes finish drawing). This will fold into extensions to ASViewController, including the ability to contain ASTableViews and coordinate the "ensure displayed" functionality (e.g. if you were to push with a navigation controller, or switch tabs, to an async table and wanted to be sure the initial content is displayed). I'll continue the push with improvements to the thread affinity system, and ASCollectionNode. Let me know if you encounter any further issues with Table / Collection so that I can prioritize coordinating a solution!

[nguyenhuy]

@smyrgl I think we want to mimic UITableView and UICollectionView behaviours, if not directly in ASDataController then in a wrapper/subclass of it that can be used by ASTableView and ASCollectionView. It will be easier for new developers to switch to ASDK.

@Adlai-Holler Thanks for taking the second issue. Let me elaborate my words:

• The current behaviour is somewhat broken, but people have came up with many creative workarounds. I'm just wondering if we apply the fix directly to ASDataController, those workarounds may no longer work and it would be quite painful if they happen to be in production code. Also, IIRC, @appleguy planned to use ASDataController for other classes as well. So changing that class to work with UITableView/UICollectionView may not be ideal.
• What I'm thinking is that we can do the sorting as an extension on top of ASDataController, like a subclass or a wrapper. Such extension will only be used by UITableView/UICollectionView. It actually sits between UITableView/UICollectionView and ASDataController.
• After that, we can allow a way for developers to opt-in for that extension and test it. For example, we can add a new flag to ASTableView/ASCollectionView initializers. Such flag can be used by a new table/collection view to determine whether it should create an ASDataController or the extension. ASDataController will be used by default, so there will be no breaking API changes and existing workarounds will still work.
• Once we are confident that the extension works correctly, we can use it as the default option.

[smyrgl]

Thanks @appleguy and @nguyenhuy and I'll be glad to test your patch out this evening (I've made the update calls modular so I can easily switch over to the "standard" batch updating code to see what happens).

My app is actually a really good case for testing this because I use table view sections to represent root nodes in a flattened infinite depth tree data structure that is stored in Core Data and listened to by an NSFetchedResultsController (which as noted above does not produce ordered changes). I have live updates going on (via web socket listeners which fire Core Data change events), batch fetching (for infinite scroll), manual collapse/expand of nodes (which takes the form of delete/inserts) and manual sort control (which generates a metric ton of section/row move operations). This is actually important because most people assume that an NSFetchedResultsController won't return section moves because the Apple sample code doesn't even bother to implement that conditional but it does.