Update node IDs when recycling widgets (#2258)

* Update node IDs when recycling widgets

* apiDump

CHANGELOG.md

@@ -14,6 +14,7 @@ Changed:
 
 Fixed:
 - Breaking `content: UIView` retain cycle in `UIViewLazyList`'s `LazyListContainerCell`.
+- Update `ProtocolNode` widget IDs when recycling widgets. This was causing pooled nodes to be leaked.
 
 
 ## [0.13.0] - 2024-07-25

redwood-protocol-host/api/redwood-protocol-host.api

@@ -41,6 +41,7 @@ public abstract class app/cash/redwood/protocol/host/ProtocolNode {
 	public final fun getId-0HhLjSo ()I
 	public abstract fun getWidget ()Lapp/cash/redwood/widget/Widget;
 	public final fun getWidgetTag-BlhN7y0 ()I
+	public final fun setId-ou3jOuA (I)V
 	public final fun updateModifier (Lapp/cash/redwood/Modifier;)V
 	public abstract fun visitIds (Lkotlin/jvm/functions/Function1;)V
 }

redwood-protocol-host/api/redwood-protocol-host.klib.api

@@ -34,11 +34,13 @@ abstract class <#A: kotlin/Any> app.cash.redwood.protocol.host/ProtocolNode { //
 
     abstract val widget // app.cash.redwood.protocol.host/ProtocolNode.widget|{}widget[0]
         abstract fun <get-widget>(): app.cash.redwood.widget/Widget<#A> // app.cash.redwood.protocol.host/ProtocolNode.widget.<get-widget>|<get-widget>(){}[0]
-    final val id // app.cash.redwood.protocol.host/ProtocolNode.id|{}id[0]
-        final fun <get-id>(): app.cash.redwood.protocol/Id // app.cash.redwood.protocol.host/ProtocolNode.id.<get-id>|<get-id>(){}[0]
     final val widgetTag // app.cash.redwood.protocol.host/ProtocolNode.widgetTag|{}widgetTag[0]
         final fun <get-widgetTag>(): app.cash.redwood.protocol/WidgetTag // app.cash.redwood.protocol.host/ProtocolNode.widgetTag.<get-widgetTag>|<get-widgetTag>(){}[0]
 
+    final var id // app.cash.redwood.protocol.host/ProtocolNode.id|{}id[0]
+        final fun <get-id>(): app.cash.redwood.protocol/Id // app.cash.redwood.protocol.host/ProtocolNode.id.<get-id>|<get-id>(){}[0]
+        final fun <set-id>(app.cash.redwood.protocol/Id) // app.cash.redwood.protocol.host/ProtocolNode.id.<set-id>|<set-id>(app.cash.redwood.protocol.Id){}[0]
+
     abstract fun apply(app.cash.redwood.protocol/PropertyChange, app.cash.redwood.protocol/EventSink) // app.cash.redwood.protocol.host/ProtocolNode.apply|apply(app.cash.redwood.protocol.PropertyChange;app.cash.redwood.protocol.EventSink){}[0]
     abstract fun children(app.cash.redwood.protocol/ChildrenTag): app.cash.redwood.protocol.host/ProtocolChildren<#A>? // app.cash.redwood.protocol.host/ProtocolNode.children|children(app.cash.redwood.protocol.ChildrenTag){}[0]
     abstract fun detach() // app.cash.redwood.protocol.host/ProtocolNode.detach|detach(){}[0]

redwood-protocol-host/src/commonMain/kotlin/app/cash/redwood/protocol/host/HostProtocolAdapter.kt

@@ -353,6 +353,9 @@ public class HostProtocolAdapter<W : Any>(
         "Insert attempted to replace existing widget with ID $widgetId"
       }
 
+      val skippedCreate = changesAndNulls[changeIndexForCreate] as Create
+      pooled.id = skippedCreate.id
+
       // Remove the corresponding changes that we avoided by node reuse. We don't clear the 'Add'
       // that adds the node to its new parent.
       changesAndNulls[changeIndexForCreate] = null

redwood-protocol-host/src/commonMain/kotlin/app/cash/redwood/protocol/host/ProtocolNode.kt

@@ -33,7 +33,8 @@ import kotlin.math.min
  */
 @RedwoodCodegenApi
 public abstract class ProtocolNode<W : Any>(
-  public val id: Id,
+  /** Updated in place when a node is reused due to pooling. */
+  public var id: Id,
   public val widgetTag: WidgetTag,
 ) {
   public abstract val widget: Widget<W>

redwood-testing/src/commonTest/kotlin/app/cash/redwood/testing/ViewRecyclingTest.kt

@@ -16,14 +16,19 @@
 package app.cash.redwood.testing
 
 import app.cash.redwood.Modifier
+import app.cash.redwood.RedwoodCodegenApi
 import app.cash.redwood.layout.compose.Box
 import app.cash.redwood.layout.compose.Column
 import app.cash.redwood.layout.testing.BoxValue
 import app.cash.redwood.layout.testing.ColumnValue
 import app.cash.redwood.layout.widget.Box
+import app.cash.redwood.protocol.ChildrenTag
+import app.cash.redwood.protocol.Id
+import app.cash.redwood.protocol.host.HostProtocolAdapter
 import assertk.assertThat
 import assertk.assertions.containsExactly
 import assertk.assertions.hasSize
+import assertk.assertions.isEmpty
 import assertk.assertions.isNotSameInstanceAs
 import assertk.assertions.isSameInstanceAs
 import com.example.redwood.testapp.compose.Button
@@ -56,6 +61,7 @@ class ViewRecyclingTest {
       )
       val snapshot1Box = widgets.single() as Box<WidgetValue>
       val snapshot1BoxText = snapshot1Box.children.widgets.single()
+      val snapshot1ProtocolNodeIds = hostAdapter.allProtocolNodeIds()
 
       // Update the content. The old widgets are pooled and new widgets are created.
       setContent {
@@ -71,6 +77,7 @@ class ViewRecyclingTest {
       )
       val snapshot2Box = widgets.single() as Box<WidgetValue>
       val snapshot2BoxText = snapshot2Box.children.widgets.single()
+      val snapshot2ProtocolNodeIds = hostAdapter.allProtocolNodeIds()
       assertThat(snapshot2Box).isNotSameInstanceAs(snapshot1Box)
       assertThat(snapshot2BoxText).isNotSameInstanceAs(snapshot1Box)
 
@@ -88,8 +95,13 @@ class ViewRecyclingTest {
       )
       val snapshot3Box = widgets.single() as Box<WidgetValue>
       val snapshot3BoxText = snapshot3Box.children.widgets.single()
+      val snapshot3ProtocolNodeIds = hostAdapter.allProtocolNodeIds()
       assertThat(snapshot3Box).isSameInstanceAs(snapshot1Box)
       assertThat(snapshot3BoxText).isSameInstanceAs(snapshot1BoxText)
+
+      // Confirm that the protocol IDs are updated in place.
+      assertThat(snapshot1ProtocolNodeIds.intersect(snapshot2ProtocolNodeIds)).isEmpty()
+      assertThat(snapshot2ProtocolNodeIds.intersect(snapshot3ProtocolNodeIds)).isEmpty()
     }
   }
 
@@ -698,4 +710,14 @@ class ViewRecyclingTest {
     // We should hit for every element in the pool (and 1 miss).
     assertThat(step1Texts.intersect(step3Texts.toSet())).hasSize(poolSize)
   }
+
+  @OptIn(RedwoodCodegenApi::class)
+  @Suppress("INVISIBLE_MEMBER", "INVISIBLE_REFERENCE") // To test implementation details!
+  private fun HostProtocolAdapter<WidgetValue>.allProtocolNodeIds(): Set<Id> {
+    val result = mutableSetOf<Id>()
+    node(Id.Root).children(ChildrenTag.Root)!!.visitIds {
+      result += it
+    }
+    return result
+  }
 }