search

facebookarchive / AsyncDisplayKit

Smooth asynchronous user interfaces for iOS apps.
http://asyncdisplaykit.org
Other
13.4k stars 2.2k forks source link

Issues with deleting sections/rows in ASTableView #869

Closed tomizimobile closed 8 years ago

tomizimobile commented 8 years ago

I've created a simple test project that shows the issue: https://github.com/tomizimobile/ASDKSearchIssue

The project is pretty basic - just an ASTableView that shows some randomly generated names, and a search bar to filter the results. Using UIKit, everything works as expected, but using AsyncDisplayKit, a crash occurs when trying to delete sections/rows (using tableView.beginUpdates() + update calls) or reload the data (using tableView.reloadData()) after the data source has been filtered. It appears to be calling - tableView:titleForHeaderInSection: with a section that is out of range for the filtered data.

tettoffensive commented 8 years ago

I am also having trouble with something similar. - (ASCellNode *)tableView:(ASTableView *)tableView nodeForRowAtIndexPath:(NSIndexPath *)indexPath is being called at some point when the data source has changed and indexPath.row is out of index

ldb) bt
* thread #1: tid = 0x15baab, 0x0000000199e5bf48 libobjc.A.dylib`objc_exception_throw, queue = 'com.apple.main-thread', stop reason = breakpoint 1.1
    frame #0: 0x0000000199e5bf48 libobjc.A.dylib`objc_exception_throw
    frame #1: 0x000000018527f0e4 CoreFoundation`-[__NSArrayI objectAtIndex:] + 196
  * frame #2: 0x00000001000dbef8 Channels`-[ListChannelsViewController tableView:nodeForRowAtIndexPath:](self=0x0000000156f4d460, _cmd="tableView:nodeForRowAtIndexPath:", tableView=0x0000000157811e00, indexPath=0xc000000000800016) + 212 at ListChannelsViewController.m:295
    frame #3: 0x00000001013c7f4c AsyncDisplayKit`-[ASTableView dataController:nodeAtIndexPath:](self=0x0000000157811e00, _cmd="dataController:nodeAtIndexPath:", dataController=0x0000000156f1cf50, indexPath=0xc000000000800016) + 124 at ASTableView.mm:821
    frame #4: 0x00000001013464a4 AsyncDisplayKit`__64-[ASDataController reloadRowsAtIndexPaths:withAnimationOptions:]_block_invoke_2(.block_descriptor=<unavailable>) + 480 at ASDataController.mm:796
    frame #5: 0x0000000101340ebc AsyncDisplayKit`-[ASDataController accessDataSourceSynchronously:withBlock:](self=0x0000000156f1cf50, _cmd="accessDataSourceSynchronously:withBlock:", synchronously=NO, block=(AsyncDisplayKit`__64-[ASDataController reloadRowsAtIndexPaths:withAnimationOptions:]_block_invoke_2 at ASDataController.mm:788)) + 436 at ASDataController.mm:447
    frame #6: 0x0000000101340cbc AsyncDisplayKit`-[ASDataController accessDataSourceWithBlock:](self=0x0000000156f1cf50, _cmd="accessDataSourceWithBlock:", block=(AsyncDisplayKit`__64-[ASDataController reloadRowsAtIndexPaths:withAnimationOptions:]_block_invoke_2 at ASDataController.mm:788)) + 68 at ASDataController.mm:434
    frame #7: 0x0000000101346260 AsyncDisplayKit`__64-[ASDataController reloadRowsAtIndexPaths:withAnimationOptions:]_block_invoke(.block_descriptor=<unavailable>) + 584 at ASDataController.mm:788
    frame #8: 0x0000000101341ff4 AsyncDisplayKit`__50-[ASDataController endUpdatesAnimated:completion:]_block_invoke224(.block_descriptor=<unavailable>, block=(AsyncDisplayKit`__64-[ASDataController reloadRowsAtIndexPaths:withAnimationOptions:]_block_invoke at ASDataController.mm:781), idx=0, stop=NO) + 88 at ASDataController.mm:530
    frame #9: 0x000000018528f990 CoreFoundation`__53-[__NSArrayM enumerateObjectsWithOptions:usingBlock:]_block_invoke + 132
    frame #10: 0x000000018528f800 CoreFoundation`-[__NSArrayM enumerateObjectsWithOptions:usingBlock:] + 172
    frame #11: 0x0000000101341b64 AsyncDisplayKit`-[ASDataController endUpdatesAnimated:completion:](self=0x0000000156f1cf50, _cmd="endUpdatesAnimated:completion:", animated=YES, completion=<parent is NULL>) + 264 at ASDataController.mm:528
    frame #12: 0x00000001013243a8 AsyncDisplayKit`-[ASChangeSetDataController endUpdatesAnimated:completion:](self=0x0000000156f1cf50, _cmd="endUpdatesAnimated:completion:", animated=YES, completion=<parent is NULL>) + 3572 at ASChangeSetDataController.m:79
    frame #13: 0x00000001013c20dc AsyncDisplayKit`-[ASTableView endUpdatesAnimated:completion:](self=0x0000000157811e00, _cmd="endUpdatesAnimated:completion:", animated=YES, completion=<parent is NULL>) + 480 at ASTableView.mm:391
    frame #14: 0x00000001013c1ef0 AsyncDisplayKit`-[ASTableView endUpdates](self=0x0000000157811e00, _cmd="endUpdates") + 56 at ASTableView.mm:385
Adlai-Holler commented 8 years ago

@tomizimobile Thanks for posting this issue, and sweet test project!

I'm investigating it currently. If I type "T", the app requests to delete sections {0, 5, 11} but we end up only deleting section 11 and thus requesting the title for now-gone section 10. The other two indices are dropped somewhere in +[_ASHierarchyChangeSet sortAndCoalesceChanges:]. I'll post back here as soon as I discover a fix for this critical issue!

Adlai-Holler commented 8 years ago

@tomizimobile @tettoffensive As the author of the broken code, I'm really sorry about this issue. I believe #884 will resolve it – it fixed the sample project on my machine. If you have time, could you try replacing the line in your Podfile with

pod 'AsyncDisplayKit', :git => 'https://github.com/Adlai-Holler/AsyncDisplayKit', :branch => 'FixTableViewUpdatingIssue'

and running pod update AsyncDisplayKit to see if the proposed PR fixes the issue?

Adlai-Holler commented 8 years ago

Sorry, put a quick hold on that. There's another potential issue that I'd like to fix at the same time. Shouldn't be more than an hour. Done.

tomizimobile commented 8 years ago

@Adlai-Holler That fix seems to be handling deletions pretty fine, but I noticed when putting it into my test project, it has weird behavior when inserting sections. I searched for "T" to filter the results, which filters data, and removes sections "A", "J", and "Y". Now, when deleting the search text, all the data is back but the rows in sections "A", "J", and "Y" are doubled-up.

Adlai-Holler commented 8 years ago

OK thanks for saying so. It looks like a separate issue but I'm attempting to fix it right now. I'll post again soon with an update.

Adlai-Holler commented 8 years ago

NOTE: The issue causing the doubling appears to be this:

If you insert a section during a batch update and also insert rows into that section, we will insert those rows twice. It's an open question which animation options UIKit uses in that case – the options given for the section insert or for the row insert – I'll assume it uses the options for the section insert.

Adlai-Holler commented 8 years ago

@tomizimobile OK could you give the latest on that branch a try? I've fixed the duplication issue as well.

tomizimobile commented 8 years ago

Looks great! I only inserted the rows along with the section because I seem to remember needing to do that in an older version of ASDK... it might be unnecessary but your branch handles it just fine. Thanks for taking a look at this.

appleguy commented 8 years ago

@tomizimobile thanks so much for not only providing the project, but helping with followup testing of the patch - it really helps us move quickly and more confidently. Bummed to hear you needed a weird workaround for a prior version, but sadly I totally understand as a filtering typeahead is a sure way to bust the shady implementation of this code from 1.2.x and even some of the more recent versions :-P.

Of course, it turns out that workaround allowed us to make this code a lot better — I am very glad to hear that we caught the issue of editing a section that hasn't yet been added, since that should absolutely work reliably even if it's possible to avoid in your app.

Thanks as well @tettoffensive for adding more details!

Wondering, have you encountered any other problems with the framework or have feature requests?

tomizimobile commented 8 years ago

@appleguy Happy to provide the sample project and follow up with testing - I know how much easier it makes debugging.

I've only ran across a couple previously reported issues (I've reported https://github.com/facebook/AsyncDisplayKit/issues/860, and https://github.com/facebook/AsyncDisplayKit/issues/261 is a longstanding issue). I might have another issue with an occasional deadlock on collection view updates, but I haven't reliably tracked it down, and I'm not sure if it's an issue with my own code.

Really loving the recent layout spec additions. I don't have any feature requests at the moment, but I'll post them here if I do in the future.

tettoffensive commented 8 years ago

Thanks for the fix!

FYI,

When I updated to the latest master to get this fix my table view started highlighting as I selected rows. This was not happening before. So I just added tableView.allowsSelection = NO;