Revert "New Widget.detach() is a user function called at detach time (#…

…2083)" (#2096) This reverts commit 5127662. This was added as an attempt to work around problems between Swift's reference counting and Kotlin's garbage collector. We've since found a simpler solution that doesn't require as much code.
cashapp · Jun 11, 2024 · 4a0845a · 4a0845a
1 parent 88dd8da
commit 4a0845a
Show file tree

Hide file tree

Showing 14 changed files with 41 additions and 182 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -6,7 +6,6 @@
 New:
 - Added a basic DOM-based LazyList implementation.
 -`TreehouseApp.close()` stops the app and prevents it from being started again later.
--`Widget.detach()` is called when a widget will receive no further updates from the presenter.
 - Added `UiConfiguration.layoutDirection` to support reading the host's layout direction.
 
 Changed:

diff --git a/...ylayout-uiview/src/commonMain/kotlin/app/cash/redwood/lazylayout/uiview/UIViewLazyList.kt b/...ylayout-uiview/src/commonMain/kotlin/app/cash/redwood/lazylayout/uiview/UIViewLazyList.kt
@@ -68,31 +68,16 @@ import platform.darwin.NSObject
 internal open class UIViewLazyList :
   LazyList<UIView>,
   ChangeListener {
-  private val updateProcessor = UIViewLazyListUpdateProcessor()
-
-  private val scrollProcessor = UIViewLazyListScrollProcessor()
-
-  private val dataSource = UIViewLazyListDataSource()
-
-  private val tableViewDelegate = UIViewLazyListTableViewDelegate()
-
-  internal val tableView = UIViewLazyListTableView()
-
-  internal class UIViewLazyListTableView :
-    UITableView(
-      CGRectZero.readValue(),
-      UITableViewStyle.UITableViewStylePlain,
-    ) {
-    var scrollProcessor: UIViewLazyListScrollProcessor? = null
-
+  internal val tableView: UITableView = object : UITableView(
+    CGRectZero.readValue(),
+    UITableViewStyle.UITableViewStylePlain,
+  ) {
     override fun setContentOffset(contentOffset: CValue<CGPoint>, animated: Boolean) {
-      val scrollProcessor = this.scrollProcessor ?: return // Detached.
-
       // If the caller is requesting a contentOffset with y == 0,
       // and the current contentOffset.y is not 0,
       // assume that it's a programmatic scroll-to-top call.
       if (contentOffset.useContents { y } == 0.0 && this.contentOffset.useContents { y } != 0.0) {
-        scrollProcessor.ignoreScrollUpdates = true
+        ignoreScrollUpdates = true
         scrollProcessor.onScrollToTop()
       }
       super.setContentOffset(contentOffset, animated)
@@ -104,16 +89,12 @@ internal open class UIViewLazyList :
   override val value: UIView
     get() = tableView
 
-  internal class UIViewLazyListUpdateProcessor : LazyListUpdateProcessor<LazyListContainerCell, UIView>() {
-    var tableView: UITableView? = null
-
-    override fun createPlaceholder(original: UIView): UIView = SizeOnlyPlaceholder(original)
+  private val updateProcessor = object : LazyListUpdateProcessor<LazyListContainerCell, UIView>() {
+    override fun createPlaceholder(original: UIView) = SizeOnlyPlaceholder(original)
 
     override fun isSizeOnlyPlaceholder(placeholder: UIView) = placeholder is SizeOnlyPlaceholder
 
     override fun insertRows(index: Int, count: Int) {
-      val tableView = this.tableView ?: return // Detached.
-
       // TODO(jwilson): pass a range somehow when 'count' is large?
       tableView.beginUpdates()
       UIView.performWithoutAnimation {
@@ -126,8 +107,6 @@ internal open class UIViewLazyList :
     }
 
     override fun deleteRows(index: Int, count: Int) {
-      val tableView = this.tableView ?: return // Detached.
-
       // TODO(jwilson): pass a range somehow when 'count' is large?
       tableView.beginUpdates()
       UIView.performWithoutAnimation {
@@ -144,19 +123,12 @@ internal open class UIViewLazyList :
     }
   }
 
-  internal class UIViewLazyListScrollProcessor : LazyListScrollProcessor() {
-    var tableView: UITableView? = null
-    var updateProcessor: UIViewLazyListUpdateProcessor? = null
-    var ignoreScrollUpdates = false
+  private var ignoreScrollUpdates = false
 
-    override fun contentSize(): Int {
-      val updateProcessor = this.updateProcessor ?: return 0 // Detached.
-      return updateProcessor.size
-    }
+  private val scrollProcessor = object : LazyListScrollProcessor() {
+    override fun contentSize() = updateProcessor.size
 
     override fun programmaticScroll(firstIndex: Int, animated: Boolean) {
-      val tableView = this.tableView ?: return // Detached.
-
       ignoreScrollUpdates = animated // Don't forward scroll updates to scrollProcessor.
       tableView.scrollToRowAtIndexPath(
         NSIndexPath.indexPathForItem(firstIndex.toLong(), 0),
@@ -170,27 +142,19 @@ internal open class UIViewLazyList :
 
   override val items: Widget.Children<UIView> = updateProcessor.items
 
-  private class UIViewLazyListDataSource :
-    NSObject(),
-    UITableViewDataSourceProtocol {
-    var updateProcessor: LazyListUpdateProcessor<LazyListContainerCell, UIView>? = null
-
+  private val dataSource = object : NSObject(), UITableViewDataSourceProtocol {
     override fun tableView(
       tableView: UITableView,
       numberOfRowsInSection: NSInteger,
     ): Long {
       require(numberOfRowsInSection == 0L)
-      val updateProcessor = this.updateProcessor ?: return 0L // Detached.
       return updateProcessor.size.toLong()
     }
 
     override fun tableView(
       tableView: UITableView,
       cellForRowAtIndexPath: NSIndexPath,
-    ): UITableViewCell {
-      val updateProcessor = this.updateProcessor
-        ?: return UITableViewCell(CGRectZero.readValue()) // Detached.
-
+    ): LazyListContainerCell {
       val index = cellForRowAtIndexPath.item.toInt()
       return updateProcessor.getOrCreateView(index) { binding ->
         createView(tableView, binding, index)
@@ -212,54 +176,36 @@ internal open class UIViewLazyList :
     }
   }
 
-  private class UIViewLazyListTableViewDelegate :
-    NSObject(),
-    UITableViewDelegateProtocol {
-    var tableView: UITableView? = null
-    var scrollProcessor: UIViewLazyListScrollProcessor? = null
-
-    override fun scrollViewDidScroll(scrollView: UIScrollView) {
-      val scrollProcessor = this.scrollProcessor ?: return // Detached.
-      val tableView = this.tableView ?: return // Detached.
+  private val tableViewDelegate: UITableViewDelegateProtocol =
+    object : NSObject(), UITableViewDelegateProtocol {
+      override fun scrollViewDidScroll(scrollView: UIScrollView) {
+        if (ignoreScrollUpdates) return // Only notify of user scrolls.
 
-      if (scrollProcessor.ignoreScrollUpdates) return // Only notify of user scrolls.
+        val visibleIndexPaths = tableView.indexPathsForVisibleRows ?: return
+        if (visibleIndexPaths.isEmpty()) return
 
-      val visibleIndexPaths = tableView.indexPathsForVisibleRows ?: return
-      if (visibleIndexPaths.isEmpty()) return
-
-      val firstIndex = visibleIndexPaths.minOf { (it as NSIndexPath).item.toInt() }
-      val lastIndex = visibleIndexPaths.maxOf { (it as NSIndexPath).item.toInt() }
-      scrollProcessor.onUserScroll(firstIndex, lastIndex)
-    }
+        val firstIndex = visibleIndexPaths.minOf { (it as NSIndexPath).item.toInt() }
+        val lastIndex = visibleIndexPaths.maxOf { (it as NSIndexPath).item.toInt() }
+        scrollProcessor.onUserScroll(firstIndex, lastIndex)
+      }
 
-    /**
-     * If the user begins a drag while we’re programmatically scrolling, well then we're not
-     * programmatically scrolling anymore.
-     */
-    override fun scrollViewWillBeginDragging(scrollView: UIScrollView) {
-      val scrollProcessor = this.scrollProcessor ?: return // Detached.
-      scrollProcessor.ignoreScrollUpdates = false
-    }
+      /**
+       * If the user begins a drag while we’re programmatically scrolling, well then we're not
+       * programmatically scrolling anymore.
+       */
+      override fun scrollViewWillBeginDragging(scrollView: UIScrollView) {
+        ignoreScrollUpdates = false
+      }
 
-    override fun scrollViewDidEndScrollingAnimation(scrollView: UIScrollView) {
-      val scrollProcessor = this.scrollProcessor ?: return // Detached.
-      scrollProcessor.ignoreScrollUpdates = false
+      override fun scrollViewDidEndScrollingAnimation(scrollView: UIScrollView) {
+        ignoreScrollUpdates = false
+      }
     }
-  }
 
   init {
-    // Connect each supporting helper interface to the others it depends on. This process is
-    // reversed in detach().
-    scrollProcessor.tableView = tableView
-    scrollProcessor.updateProcessor = updateProcessor
-    updateProcessor.tableView = tableView
-    dataSource.updateProcessor = updateProcessor
-    tableViewDelegate.tableView = tableView
-    tableViewDelegate.scrollProcessor = scrollProcessor
-    tableView.scrollProcessor = scrollProcessor
-    tableView.dataSource = dataSource
-    tableView.delegate = tableViewDelegate
     tableView.apply {
+      dataSource = this@UIViewLazyList.dataSource
+      delegate = tableViewDelegate
       rowHeight = UITableViewAutomaticDimension
       separatorStyle = UITableViewCellSeparatorStyleNone
       backgroundColor = UIColor.clearColor
@@ -314,20 +260,6 @@ internal open class UIViewLazyList :
     updateProcessor.onEndChanges()
     scrollProcessor.onEndChanges()
   }
-
-  override fun detach() {
-    // Reverse what we set up in init {}. This is strictly unnecessary but should maximize the
-    // number of Kotlin objects that are immediately eligible for garbage collection.
-    scrollProcessor.tableView = null
-    scrollProcessor.updateProcessor = null
-    updateProcessor.tableView = null
-    dataSource.updateProcessor = null
-    tableViewDelegate.tableView = null
-    tableViewDelegate.scrollProcessor = null
-    tableView.scrollProcessor = null
-    // Note that we don't clear tableView.dataSource. It's weakly held anyway.
-    tableView.delegate = null
-  }
 }
 
 private const val REUSE_IDENTIFIER = "LazyListContainerCell"
@@ -442,11 +374,6 @@ internal class UIViewRefreshableLazyList :
   override fun pullRefreshContentColor(pullRefreshContentColor: UInt) {
     refreshControl.tintColor = UIColor(pullRefreshContentColor)
   }
-
-  override fun detach() {
-    super<UIViewLazyList>.detach()
-    onRefresh = null
-  }
 }
 
 private fun UIColor(color: UInt): UIColor = UIColor(

diff --git a/redwood-testing/src/commonTest/kotlin/app/cash/redwood/testing/WidgetDetachingTest.kt b/redwood-testing/src/commonTest/kotlin/app/cash/redwood/testing/WidgetDetachingTest.kt
@@ -32,7 +32,6 @@ import assertk.assertions.isSameInstanceAs
 import com.example.redwood.testapp.compose.TestRow
 import com.example.redwood.testapp.compose.Text
 import com.example.redwood.testapp.compose.reuse
-import com.example.redwood.testapp.widget.Text
 import kotlin.test.Test
 import kotlin.test.assertFailsWith
 import kotlinx.coroutines.test.runTest
@@ -54,20 +53,14 @@ class WidgetDetachingTest {
 
       awaitSnapshot()
       val protocolText = widgetBridge.extractProtocolText()
-      val protocolTextWidget = protocolText.widget as Text<*>?
-      assertThat(protocolTextWidget).isNotNull()
+      assertThat(protocolText.widget).isNotNull()
 
       step++
       awaitSnapshot()
-      val thrownOnAccessWidget = assertFailsWith<IllegalStateException> {
+      val thrown = assertFailsWith<IllegalStateException> {
         protocolText.widget
       }
-      assertThat(thrownOnAccessWidget).hasMessage("detached")
-
-      val thrownOnMutateWidget = assertFailsWith<IllegalStateException> {
-        protocolTextWidget?.text("goodbye")
-      }
-      assertThat(thrownOnMutateWidget).hasMessage("detached")
+      assertThat(thrown).hasMessage("detached")
     }
   }
 
@@ -86,20 +79,14 @@ class WidgetDetachingTest {
 
       awaitSnapshot()
       val protocolText = widgetBridge.extractProtocolText()
-      val protocolTextWidget = protocolText.widget as Text<*>?
-      assertThat(protocolTextWidget).isNotNull()
+      assertThat(protocolText.widget).isNotNull()
 
       step++
       awaitSnapshot()
-      val thrownOnAccessWidget = assertFailsWith<IllegalStateException> {
+      val thrown = assertFailsWith<IllegalStateException> {
         protocolText.widget
       }
-      assertThat(thrownOnAccessWidget).hasMessage("detached")
-
-      val thrownOnMutateWidget = assertFailsWith<IllegalStateException> {
-        protocolTextWidget?.text("goodbye")
-      }
-      assertThat(thrownOnMutateWidget).hasMessage("detached")
+      assertThat(thrown).hasMessage("detached")
     }
   }
 

diff --git a/...ooling-codegen/src/main/kotlin/app/cash/redwood/tooling/codegen/protocolHostGeneration.kt b/...ooling-codegen/src/main/kotlin/app/cash/redwood/tooling/codegen/protocolHostGeneration.kt
@@ -506,7 +506,6 @@ internal fun generateProtocolNode(
                 }
               }
             }
-            .addStatement("_widget?.detach()")
             .addStatement("_widget = null")
             .build(),
         )

diff --git a/...ood-tooling-codegen/src/main/kotlin/app/cash/redwood/tooling/codegen/testingGeneration.kt b/...ood-tooling-codegen/src/main/kotlin/app/cash/redwood/tooling/codegen/testingGeneration.kt
@@ -163,8 +163,6 @@ internal fun generateMutableWidgetFactory(schema: Schema): FileSpec {
 
 /*
 internal class MutableButton : Button<WidgetValue> {
-  private var detached: Boolean = false
-
   public override val value: WidgetValue
     get() = ButtonValue(modifier, text, enabled!!, maxLength!!)
 
@@ -175,18 +173,12 @@ internal class MutableButton : Button<WidgetValue> {
   private var maxLength: Int? = null
 
   public override fun text(text: String?) {
-    check(!detached) { "detached" }
     this.text = text
   }
 
   public override fun enabled(enabled: Boolean) {
-    check(!detached) { "detached" }
     this.enabled = enabled
   }
-
-  public override fun detach() {
-    this.detached = true
-  }
 }
 */
 internal fun generateMutableWidget(schema: Schema, widget: Widget): FileSpec {
@@ -198,13 +190,6 @@ internal fun generateMutableWidget(schema: Schema, widget: Widget): FileSpec {
       TypeSpec.classBuilder(mutableWidgetType)
         .addModifiers(INTERNAL)
         .addSuperinterface(schema.widgetType(widget).parameterizedBy(RedwoodTesting.WidgetValue))
-        .addProperty(
-          PropertySpec.builder("detached", BOOLEAN)
-            .addModifiers(PRIVATE)
-            .mutable(true)
-            .initializer("false")
-            .build(),
-        )
         .addProperty(
           PropertySpec.builder("value", RedwoodTesting.WidgetValue)
             .addModifiers(OVERRIDE)
@@ -266,8 +251,7 @@ internal fun generateMutableWidget(schema: Schema, widget: Widget): FileSpec {
                   FunSpec.builder(trait.name)
                     .addModifiers(OVERRIDE)
                     .addParameter(trait.name, type)
-                    .addStatement("check(!detached) { %S }", "detached")
-                    .addStatement("this.%N = %N", trait.name, trait.name)
+                    .addCode("this.%N = %N", trait.name, trait.name)
                     .build(),
                 )
               }
@@ -287,12 +271,6 @@ internal fun generateMutableWidget(schema: Schema, widget: Widget): FileSpec {
             }
           }
         }
-        .addFunction(
-          FunSpec.builder("detach")
-            .addModifiers(OVERRIDE)
-            .addStatement("this.detached = true")
-            .build(),
-        )
         .build(),
     )
     .build()

diff --git a/redwood-widget/api/android/redwood-widget.api b/redwood-widget/api/android/redwood-widget.api
@@ -86,7 +86,6 @@ public final class app/cash/redwood/widget/ViewGroupChildren : app/cash/redwood/
 }
 
 public abstract interface class app/cash/redwood/widget/Widget {
-	public fun detach ()V
 	public abstract fun getModifier ()Lapp/cash/redwood/Modifier;
 	public abstract fun getValue ()Ljava/lang/Object;
 	public abstract fun setModifier (Lapp/cash/redwood/Modifier;)V

diff --git a/redwood-widget/api/jvm/redwood-widget.api b/redwood-widget/api/jvm/redwood-widget.api
@@ -64,7 +64,6 @@ public abstract interface class app/cash/redwood/widget/SavedStateRegistry {
 }
 
 public abstract interface class app/cash/redwood/widget/Widget {
-	public fun detach ()V
 	public abstract fun getModifier ()Lapp/cash/redwood/Modifier;
 	public abstract fun getValue ()Ljava/lang/Object;
 	public abstract fun setModifier (Lapp/cash/redwood/Modifier;)V

diff --git a/redwood-widget/api/redwood-widget.klib.api b/redwood-widget/api/redwood-widget.klib.api
@@ -33,7 +33,6 @@ abstract interface <#A: kotlin/Any> app.cash.redwood.widget/Widget { // app.cash
     abstract var modifier // app.cash.redwood.widget/Widget.modifier|{}modifier[0]
         abstract fun <get-modifier>(): app.cash.redwood/Modifier // app.cash.redwood.widget/Widget.modifier.<get-modifier>|<get-modifier>(){}[0]
         abstract fun <set-modifier>(app.cash.redwood/Modifier) // app.cash.redwood.widget/Widget.modifier.<set-modifier>|<set-modifier>(app.cash.redwood.Modifier){}[0]
-    open fun detach() // app.cash.redwood.widget/Widget.detach|detach(){}[0]
 }
 abstract interface <#A: kotlin/Any> app.cash.redwood.widget/WidgetFactoryOwner // app.cash.redwood.widget/WidgetFactoryOwner|null[0]
 abstract interface <#A: kotlin/Any> app.cash.redwood.widget/WidgetSystem { // app.cash.redwood.widget/WidgetSystem|null[0]
-Original file line number
+Diff line change
@@ Expand Up / @@ -506,7 +506,6 @@ internal fun generateProtocolNode( @@
                     }
                   }
                 }
-                .addStatement("_widget?.detach()")
                 .addStatement("_widget = null")
                 .build(),
             )
@@ Expand Down @@