Change UIViewLazyList so cells can be removed and re-added without reuse #2242
Conversation
We observed in some test suites that table cells would observe a sequence of calls like this: LazyListContainerCell.createView() LazyListContainerCell.willMoveToSuperview(non-null) LazyListContainerCell.willMoveToSuperview(null) LazyListContainerCell.willMoveToSuperview(non-null) Note that we didn't receive a call to prepareForReuse() between the code being removed from its parent and added again. The code wasn't prepared for this sequence of calls.
| } | ||
|
|
||
| // Unbind the cell when its view is detached from the table. | ||
| if (superview != null && newSuperview == null) { | ||
| binding?.unbind() | ||
| binding = null |
There was a problem hiding this comment.
So we can bind again later
There was a problem hiding this comment.
This feels a little dangerous to me--what are the odds that this causes a memory management issue, because it's being retained?
There was a problem hiding this comment.
Good instinct. I double checked this and unbind() breaks the cycle.
But I don’t like that even with Redwood’s LeakTest, we don’t have dynamic coverage of leaks like this. I’ve opened #2245 to test it directly.
| return placeholder | ||
| require(binding.isPlaceholder) | ||
| if (binding.isBound) { | ||
| recyclePlaceholder(binding.content!!) |
There was a problem hiding this comment.
If a bound binding must have content, wdyt about restructuring the code to guarantee that requirement before this point? It looks like we could avoid some not-null assertions and have an access pattern here and below like...
when (val state = binding.state) {
is Bound -> { recyclePlaceholder(state.content) }
is Unbound -> {}
}
There was a problem hiding this comment.
I think this is a great idea. Let’s move this logic out of the code and into the type system! I’d like to do it in a follow-up though.
| if (isPlaceholder && this.content == null) { | ||
| this.content = processor.takePlaceholder() | ||
| } | ||
|
|
There was a problem hiding this comment.
Without the structural change to a binding's state, should we add require(this.content != null) for the non-placeholder case?
There was a problem hiding this comment.
Great idea. Lemme do that in the above-mentioned follow-up.
We observed in some test suites that table cells would observe a sequence of calls like this:
LazyListContainerCell.createView()
LazyListContainerCell.willMoveToSuperview(non-null) LazyListContainerCell.willMoveToSuperview(null)
LazyListContainerCell.willMoveToSuperview(non-null)
Note that we didn't receive a call to prepareForReuse() between the code being removed from its parent and added again. The code wasn't prepared for this sequence of calls.
CHANGELOG.md's "Unreleased" section has been updated, if applicable.