Skip to content
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: main
Choose a base branch
from

Conversation

banjun
Copy link

@banjun 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'

…ns followed by insertions

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

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

@banjun
Copy link
Author

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
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
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants