New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix crash moving rows & sections #14

Open
wants to merge 2 commits into
base: master
from

Conversation

3 participants
@banjun
Copy link

banjun commented Jan 23, 2018

I noticed that sometimes UITableView asserts failure when applying diff with moveRows & moveSections.
It's reproducible in the TableViewExample project with some modification to the example data.

So far separating moves into deletions & insertions can resolve this issue, though I have not inspected the diff logic.
This PR includes a patch for UITableView.

2018-01-23 12:35:51.464473+0900 TableViewExample[16749:1512966] *** Assertion failure in -[_UITableViewUpdateSupport _setupAnimationsForNewlyInsertedCells], /BuildRoot/Library/Caches/com.apple.xbs/Sources/UIKit/UIKit-3698.33.7/UITableViewSupport.m:1295
2018-01-23 12:35:51.465157+0900 TableViewExample[16749:1512966] *** Terminating app due to uncaught exception 'NSInternalInconsistencyException', reason: 'Attempt to create two animations for cell'

banjun added some commits Jan 18, 2018

avoid using moveRows and moveSections by separating them into deletio…
…ns followed by insertions

*** Terminating app due to uncaught exception 'NSInternalInconsistencyException', reason: 'Attempt to create two animations for cell
@tonyarnold

This comment has been minimized.

Copy link
Owner

tonyarnold commented Mar 7, 2018

Doesn't this cause a different animation to occur? Is this required for all UITableViews?

@banjun

This comment has been minimized.

Copy link

banjun commented Mar 8, 2018

The animation changes. I think there's a more clean way but I've not yet found it and wonder why the example crashes. For crash safety, I use this patch as a remedy for now.

@rayfix

This comment has been minimized.

Copy link

rayfix commented Aug 17, 2018

Hi @banjun ! I was surprised to see your PR. You are everywhere. 🤩

I was wondering if you are convinced that the actual steps in the patch correct? Or is the patch wrong and it is crashing the animation. 🤔

There appear to be some cases where the wrong patch is being generated. See #32 I still need to read the paper to figure out how the algorithm actually works. It would be cool if we could make a UT that repo'ed the problem.

[PS: Coming to Tokyo next week for iOSDC. Maybe we can get together and hack out a good solution sometime then. 😀]

@banjun

This comment has been minimized.

Copy link

banjun commented Aug 22, 2018

@rayfix you, too. 😉

In my case I think the patch is logically correct, but some patch that have moving rows can cause crash in UITableView. (By the way, checking #32, wrong patches may be generated in some case...? 🤔 )

The fact that a logically correct patch can cause crash is noted on other differential change libs, such as RxDataSources & DifferenceKit. They use multi-stage datasource commit to avoid problematic patches.

https://github.com/RxSwiftCommunity/RxDataSources/blob/99329c31119abe3a5e8d17319a144d1238f32d3a/Sources/Differentiator/Diff.swift#L232

https://github.com/ra1028/DifferenceKit/blob/master/Sources/StagedChangeset.swift#L5

@tonyarnold tonyarnold force-pushed the tonyarnold:master branch from 27a96fb to 6dbd920 Sep 5, 2018

@tonyarnold tonyarnold force-pushed the tonyarnold:master branch from 37d3f01 to 2547774 Sep 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment