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

Change UIViewLazyList so cells can be removed and re-added without reuse #2242

Merged
merged 2 commits into from
Aug 12, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ New:

Changed:
- `ProtocolFactory` interface is now sealed as arbitrary subtypes were never supported. Only schema-generated subtypes should be used.
- `UIViewLazyList` doesn't crash with a `NullPointerException` if cells are added, removed, and re-added without being reused.

Fixed:
- Nothing yet!
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -301,14 +301,13 @@ internal class LazyListContainerCell(
}

// Confirm the cell is bound when it's about to be displayed.
if (superview == null && newSuperview != null) {
require(binding!!.isBound) { "about to display a cell that isn't bound!" }
if (superview == null && newSuperview != null && !binding!!.isBound) {
binding!!.bind(this)
}

// Unbind the cell when its view is detached from the table.
if (superview != null && newSuperview == null) {
binding?.unbind()
binding = null
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we can bind again later

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels a little dangerous to me--what are the odds that this causes a memory management issue, because it's being retained?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -314,22 +314,24 @@ public abstract class LazyListUpdateProcessor<V : Any, W : Any> {
}

private fun placeholderToLoaded(
placeholder: Binding<V, W>?,
binding: Binding<V, W>?,
loadedContent: Widget<W>,
): Binding<V, W> {
// No binding for this index. Create one.
if (placeholder == null) {
if (binding == null) {
return Binding(this).apply {
setContentAndModifier(loadedContent)
}
}

// We have a binding. Give it loaded content.
require(placeholder.isPlaceholder)
recyclePlaceholder(placeholder.content)
placeholder.isPlaceholder = false
placeholder.setContentAndModifier(loadedContent)
return placeholder
require(binding.isPlaceholder)
if (binding.isBound) {
recyclePlaceholder(binding.content!!)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 -> {}
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
binding.isPlaceholder = false
binding.setContentAndModifier(loadedContent)
return binding
}

private fun loadedToPlaceholder(loaded: Binding<V, W>): Binding<V, W>? {
Expand Down Expand Up @@ -438,8 +440,13 @@ public abstract class LazyListUpdateProcessor<V : Any, W : Any> {
public var view: V? = null
private set

/** The content of this binding; either a loaded widget or a placeholder. */
internal lateinit var content: W
/**
* The content of this binding; either a loaded widget, a placeholder, or null.
*
* This may be null if its content is a placeholder that is not bound to a view. That way we
* don't take placeholders from the pool until we need them.
*/
internal var content: W? = null
private set
private var modifier: Modifier = Modifier

Expand All @@ -458,9 +465,13 @@ public abstract class LazyListUpdateProcessor<V : Any, W : Any> {
public val isBound: Boolean
get() = view != null

internal fun bind(view: V) {
public fun bind(view: V) {
require(this.view == null) { "already bound" }

if (isPlaceholder && this.content == null) {
this.content = processor.takePlaceholder()
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without the structural change to a binding's state, should we add require(this.content != null) for the non-placeholder case?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great idea. Lemme do that in the above-mentioned follow-up.

this.view = view
processor.setContent(view, content, modifier)
}
Expand All @@ -481,7 +492,8 @@ public abstract class LazyListUpdateProcessor<V : Any, W : Any> {
if (itemsAfterIndex != -1) processor.itemsAfter.set(itemsAfterIndex, null)

// When a placeholder is reused, recycle its widget.
processor.recyclePlaceholder(content)
processor.recyclePlaceholder(content!!)
this.content = null
}
}
}
Expand Down
Loading