Fix issue where supplementary elements don't track section changes #404
Conversation
| [supps removeObjectForKey:oldIndexPath]; | ||
| } else { | ||
| // Section index changed, move it. | ||
| NSIndexPath *newIndexPath = [NSIndexPath indexPathForItem:oldIndexPath.item inSection:newSection]; |
There was a problem hiding this comment.
I think in this case we may also need to do the removeObjectForKey:oldIndexPath ?
|
@Adlai-Holler I've verified that a5a9627 fixes #385. Thanks a lot! |
|
In turn, thank you for filing the issue *and* verifying the fix so quickly!
That really helps increase our confidence to land and cut a release
rapidly, and enables us to move on to more great things :-)
…On Fri, Jun 30, 2017 at 10:57 PM mrvincenzo ***@***.***> wrote:
@Adlai-Holler <https://github.com/adlai-holler> I've verified that a5a9627
<a5a9627>
fixes #385 <#385>. Thanks a
lot!
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#404 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAigAxLKi1it05pRUD0S05WE_Eqws27jks5sJd-vgaJpZM4OLKBc>
.
|
|
Note: Unfortunately there's still a subtle bug here! Imagine you have sections 0 1, you delete section 0, and during the dictionary enumeration you visit in the order 1 0. Visit 1: I'll update the diff to be a little less eager about performance and simply generate a new dictionary every time without erasing elements. |
Adlai-Holler
force-pushed
the
AHFixCollectionCrash
branch
from
db8aebe
to
6799975
Compare
|
@appleguy and the rest of the Texture gang, you guys are the best, thanks for fixing this so fast. |
| // For each index path of that kind, move entries into a new dictionary. | ||
| // Note: it's tempting to update the dictionary in-place but because of the likely collision between old and new index paths, | ||
| // subtle bugs are possible. Note that this process is rare (only on section-level updates), | ||
| // that this work is done off-main, and that the typical supplementary element use case is just 1-per-section (header). |
There was a problem hiding this comment.
@Adlai-Holler this is a very worthwhile comment, thanks!
…extureGroup#404) * Fix collection view supplementary issue * Add a fast-path * Remove the old index path entry unconditionally * Handle overwriting bug
Unfortunately, since supplementary elements are keyed by section index, we have to migrate supplementaries when sections are inserted/deleted. So if you have section 0 with a header, and you insert a section at index 0, that previous header should now be at index path e.g.
{1, 0}.So we migrate all supplementary elements when there are section-level changes. This is meant to be a minimal diff to fix the crash, and it was reduced down from the larger change #405 which unifies our index change tracking.
Resolves #397 and (I believe) #385 @Pacek @mrvincenzo