List cells now internally change their backing list, this means that list cell values and events are no longer immutable. This does simplify the code and probably improves performance.

cell/src/commonMain/kotlin/world/phantasmal/cell/AbstractFlatteningDependentCell.kt

@@ -38,7 +38,7 @@ abstract class AbstractFlatteningDependentCell<T, ComputedCell : Cell<T>, Event
                 computedCell = unsafeAssertNotNull(this.computedCell)
             }
 
-            val newValue = computedCell.value
+            val newValue = transformNewValue(computedCell.value)
             _value = newValue
             changeEvent = createEvent(oldValue, newValue)
             // We stay invalid if we have no dependents to ensure our value is always recomputed.
@@ -46,6 +46,8 @@ abstract class AbstractFlatteningDependentCell<T, ComputedCell : Cell<T>, Event
         }
     }
 
+    protected abstract fun transformNewValue(value: T): T
+
     protected abstract fun createEvent(oldValue: T?, newValue: T): Event
 
     override fun addDependent(dependent: Dependent) {

cell/src/commonMain/kotlin/world/phantasmal/cell/ChangeObserver.kt

@@ -2,6 +2,10 @@ package world.phantasmal.cell
 
 typealias ChangeObserver<T> = (ChangeEvent<T>) -> Unit
 
+/**
+ * Don't keep long-lived references to change events, they may change internally after change
+ * observers have been called.
+ */
 open class ChangeEvent<out T>(
     /**
      * The cell's new value. Don't keep long-lived references to this object, it may change after

cell/src/commonMain/kotlin/world/phantasmal/cell/FlatteningDependentCell.kt

@@ -7,6 +7,7 @@ class FlatteningDependentCell<T>(
     vararg dependencies: Cell<*>,
     compute: () -> Cell<T>,
 ) : AbstractFlatteningDependentCell<T, Cell<T>, ChangeEvent<T>>(dependencies, compute) {
+    override fun transformNewValue(value: T): T = value
 
     override fun createEvent(oldValue: T?, newValue: T): ChangeEvent<T> =
         ChangeEvent(newValue)

cell/src/commonMain/kotlin/world/phantasmal/cell/list/AbstractElementsWrappingListCell.kt

@@ -1,34 +0,0 @@
-package world.phantasmal.cell.list
-
-import world.phantasmal.core.unsafe.unsafeAssertNotNull
-
-abstract class AbstractElementsWrappingListCell<E> : AbstractListCell<E>() {
-    /**
-     * When [value] is accessed and this property is null, a new wrapper is created that points to
-     * [elements]. Before changes to [elements] are made, if there's a wrapper, the current
-     * wrapper's backing list is set to a copy of [elements] and this property is set to null. This
-     * way, accessing [value] acts like accessing a snapshot without making an actual copy
-     * everytime. This is necessary because the contract is that a cell's new value is always != to
-     * its old value whenever a change event was emitted (TODO: is this still the contract?).
-     */
-    // TODO: Optimize this by using a weak reference to avoid copying when nothing references the
-    //       wrapper.
-    // TODO: Just remove this because it's a huge headache? Does it matter that events are
-    //       immutable?
-    private var _elementsWrapper: DelegatingList<E>? = null
-    protected val elementsWrapper: DelegatingList<E>
-        get() {
-            if (_elementsWrapper == null) {
-                _elementsWrapper = DelegatingList(elements)
-            }
-
-            return unsafeAssertNotNull(_elementsWrapper)
-        }
-
-    protected abstract val elements: List<E>
-
-    protected fun copyAndResetWrapper() {
-        _elementsWrapper?.backingList = elements.toList()
-        _elementsWrapper = null
-    }
-}

cell/src/commonMain/kotlin/world/phantasmal/cell/list/AbstractFilteredListCell.kt

@@ -7,7 +7,7 @@ import world.phantasmal.core.unsafe.unsafeCast
 
 abstract class AbstractFilteredListCell<E>(
     protected val list: ListCell<E>,
-) : AbstractElementsWrappingListCell<E>(), Dependent {
+) : AbstractListCell<E>(), Dependent {
 
     /** Set during a change wave when [list] changes. */
     private var listInvalidated = false
@@ -21,14 +21,14 @@ abstract class AbstractFilteredListCell<E>(
 
     private var valid = false
 
-    final override val elements = mutableListOf<E>()
+    protected val elements = mutableListOf<E>()
 
     protected abstract val predicateDependency: Dependency<*>
 
     final override val value: List<E>
         get() {
             computeValueAndEvent()
-            return elementsWrapper
+            return elements
         }
 
     private var _changeEvent: ListChangeEvent<E>? = null
@@ -44,29 +44,29 @@ abstract class AbstractFilteredListCell<E>(
 
             if (predicateInvalidated || !hasDependents) {
                 // Simply assume the entire list changes and recompute.
-                val removed = elementsWrapper
+                val removed = elements.toList()
 
                 ignoreOtherChanges()
                 recompute()
 
                 _changeEvent = ListChangeEvent(
-                    elementsWrapper,
+                    elements,
-                    listOf(ListChange(0, removed.size, removed, elementsWrapper)),
+                    listOf(ListChange(index = 0, prevSize = removed.size, removed, elements)),
                 )
             } else {
-                // TODO: Conditionally copyAndResetWrapper?
-                copyAndResetWrapper()
-
                 // Reuse the same list of changes during a mutation.
                 val event = _changeEvent
-                val filteredChanges: MutableList<ListChange<E>> =
+                val filteredChanges: MutableList<ListChange<E>>
+
                 if (event == null || changesMutationId != MutationManager.currentMutationId) {
                     changesMutationId = MutationManager.currentMutationId
                     listChangeIndex = 0
-                        mutableListOf()
+                    filteredChanges = mutableListOf()
+                    _changeEvent = ListChangeEvent(elements, filteredChanges)
                 } else {
-                        // This cast is safe because we know we always instantiate our change event with a mutable list.
+                    // This cast is safe because we know we always instantiate our change event
-                        unsafeCast(event.changes)
+                    // with a mutable list.
+                    filteredChanges = unsafeCast(event.changes)
                 }
 
                 val listChangeEvent = list.changeEvent
@@ -148,11 +148,10 @@ abstract class AbstractFilteredListCell<E>(
 
                 processOtherChanges(filteredChanges)
 
-                _changeEvent =
                 if (filteredChanges.isEmpty()) {
-                        null
+                    _changeEvent = null
                 } else {
-                        ListChangeEvent(elementsWrapper, filteredChanges)
+                    // Keep the previous change event, it has been changed internally.
                 }
             }
 

cell/src/commonMain/kotlin/world/phantasmal/cell/list/DelegatingList.kt

@@ -1,35 +0,0 @@
-package world.phantasmal.cell.list
-
-/**
- * Simply delegates all methods to [backingList], even [equals], [hashCode] and [toString].
- */
-class DelegatingList<E>(var backingList: List<E>) : List<E> {
-    override val size: Int = backingList.size
-
-    override fun contains(element: E): Boolean = backingList.contains(element)
-
-    override fun containsAll(elements: Collection<E>): Boolean = backingList.containsAll(elements)
-
-    override fun get(index: Int): E = backingList[index]
-
-    override fun indexOf(element: E): Int = backingList.indexOf(element)
-
-    override fun isEmpty(): Boolean = backingList.isEmpty()
-
-    override fun iterator(): Iterator<E> = backingList.iterator()
-
-    override fun lastIndexOf(element: E): Int = backingList.lastIndexOf(element)
-
-    override fun listIterator(): ListIterator<E> = backingList.listIterator()
-
-    override fun listIterator(index: Int): ListIterator<E> = backingList.listIterator(index)
-
-    override fun subList(fromIndex: Int, toIndex: Int): List<E> =
-        backingList.subList(fromIndex, toIndex)
-
-    override fun equals(other: Any?): Boolean = other == backingList
-
-    override fun hashCode(): Int = backingList.hashCode()
-
-    override fun toString(): String = backingList.toString()
-}

cell/src/commonMain/kotlin/world/phantasmal/cell/list/FilteredListCell.kt

@@ -148,7 +148,6 @@ class FilteredListCell<E>(
     }
 
     override fun recompute() {
-        copyAndResetWrapper()
         elements.clear()
 
         for (mapping in indexMap) {

cell/src/commonMain/kotlin/world/phantasmal/cell/list/FlatteningDependentListCell.kt

@@ -1,16 +1,14 @@
 package world.phantasmal.cell.list
 
+import world.phantasmal.cell.*
 import world.phantasmal.core.disposable.Disposable
 import world.phantasmal.core.unsafe.unsafeAssertNotNull
-import world.phantasmal.cell.CallbackChangeObserver
-import world.phantasmal.cell.ChangeObserver
-import world.phantasmal.cell.AbstractFlatteningDependentCell
-import world.phantasmal.cell.Cell
-import world.phantasmal.cell.DependentCell
 
 /**
  * Similar to [DependentListCell], except that this cell's computeElements returns a [ListCell].
  */
+// TODO: Improve performance when transitive cell changes. At the moment a change event is generated
+//       that just pretends the whole list has changed.
 class FlatteningDependentListCell<E>(
     vararg dependencies: Cell<*>,
     computeElements: () -> ListCell<E>,
@@ -59,6 +57,10 @@ class FlatteningDependentListCell<E>(
 
     override fun toString(): String = listCellToString(this)
 
+    override fun transformNewValue(value: List<E>): List<E> =
+        // Make a copy because this value is later used as the "removed" field of a list change.
+        value.toList()
+
     override fun createEvent(oldValue: List<E>?, newValue: List<E>): ListChangeEvent<E> {
         val old = oldValue ?: emptyList()
         return ListChangeEvent(

cell/src/commonMain/kotlin/world/phantasmal/cell/list/ListCellUtils.kt

@@ -86,11 +86,12 @@ fun <T1, T2, R> flatMapToList(
 ): ListCell<R> =
     FlatteningDependentListCell(c1, c2) { transform(c1.value, c2.value) }
 
-fun listCellToString(cell: ListCell<*>): String = buildString {
+fun listCellToString(cell: ListCell<*>): String =
-    append(this::class.simpleName)
+    buildString {
+        append(cell::class.simpleName)
         append('[')
         cell.value.joinTo(this, limit = 20) {
-        if (it === this) "(this cell)" else it.toString()
+            if (it === cell) "(this cell)" else it.toString()
         }
         append(']')
-}
+    }

cell/src/commonMain/kotlin/world/phantasmal/cell/list/SimpleFilteredListCell.kt

@@ -58,7 +58,6 @@ class SimpleFilteredListCell<E>(
     }
 
     override fun recompute() {
-        copyAndResetWrapper()
         elements.clear()
         indexMap.clear()
 

cell/src/commonMain/kotlin/world/phantasmal/cell/list/SimpleListCell.kt

@@ -1,18 +1,17 @@
 package world.phantasmal.cell.list
 
 import world.phantasmal.cell.MutationManager
-import world.phantasmal.core.replaceAll
 import world.phantasmal.core.unsafe.unsafeCast
 
 /**
  * @param elements The backing list for this [ListCell].
  */
 class SimpleListCell<E>(
-    override val elements: MutableList<E>,
+    private var elements: MutableList<E>,
-) : AbstractElementsWrappingListCell<E>(), MutableListCell<E> {
+) : AbstractListCell<E>(), MutableListCell<E> {
 
     override var value: List<E>
-        get() = elementsWrapper
+        get() = elements
         set(value) {
             replaceAll(value)
         }
@@ -30,7 +29,6 @@ class SimpleListCell<E>(
         checkIndex(index, elements.lastIndex)
 
         applyChange {
-            copyAndResetWrapper()
             val removed = elements.set(index, element)
 
             finalizeChange(
@@ -47,7 +45,6 @@ class SimpleListCell<E>(
     override fun add(element: E) {
         applyChange {
             val index = elements.size
-            copyAndResetWrapper()
             elements.add(element)
 
             finalizeChange(
@@ -64,7 +61,6 @@ class SimpleListCell<E>(
         checkIndex(index, prevSize)
 
         applyChange {
-            copyAndResetWrapper()
             elements.add(index, element)
 
             finalizeChange(index, prevSize, removed = emptyList(), inserted = listOf(element))
@@ -87,8 +83,6 @@ class SimpleListCell<E>(
 
         applyChange {
             val prevSize = elements.size
-
-            copyAndResetWrapper()
             val removed = elements.removeAt(index)
 
             finalizeChange(index, prevSize, removed = listOf(removed), inserted = emptyList())
@@ -98,25 +92,21 @@ class SimpleListCell<E>(
 
     override fun replaceAll(elements: Iterable<E>) {
         applyChange {
-            val prevSize = this.elements.size
+            val removed = this.elements
-            val removed = elementsWrapper
 
-            copyAndResetWrapper()
+            this.elements = elements.toMutableList()
-            this.elements.replaceAll(elements)
 
-            finalizeChange(index = 0, prevSize, removed, inserted = elementsWrapper)
+            finalizeChange(index = 0, prevSize = removed.size, removed, inserted = this.elements)
         }
     }
 
     override fun replaceAll(elements: Sequence<E>) {
         applyChange {
-            val prevSize = this.elements.size
+            val removed = this.elements
-            val removed = elementsWrapper
 
-            copyAndResetWrapper()
+            this.elements = elements.toMutableList()
-            this.elements.replaceAll(elements)
 
-            finalizeChange(index = 0, prevSize, removed, inserted = elementsWrapper)
+            finalizeChange(index = 0, prevSize = removed.size, removed, inserted = this.elements)
         }
     }
 
@@ -130,7 +120,6 @@ class SimpleListCell<E>(
         }
 
         applyChange {
-            copyAndResetWrapper()
             repeat(removeCount) { elements.removeAt(fromIndex) }
             elements.add(fromIndex, newElement)
 
@@ -144,20 +133,17 @@ class SimpleListCell<E>(
         }
 
         applyChange {
-            val prevSize = elements.size
+            val removed = elements
-            val removed = elementsWrapper
 
-            copyAndResetWrapper()
+            elements = mutableListOf()
-            elements.clear()
 
-            finalizeChange(index = 0, prevSize, removed, inserted = emptyList())
+            finalizeChange(index = 0, prevSize = removed.size, removed, inserted = emptyList())
         }
     }
 
     override fun sortWith(comparator: Comparator<E>) {
         applyChange {
-            val removed = elementsWrapper
+            val removed = elements.toList()
-            copyAndResetWrapper()
             var throwable: Throwable? = null
 
             try {
@@ -170,7 +156,7 @@ class SimpleListCell<E>(
                 index = 0,
                 prevSize = elements.size,
                 removed,
-                inserted = elementsWrapper,
+                inserted = elements,
             )
 
             if (throwable != null) {
@@ -194,18 +180,16 @@ class SimpleListCell<E>(
         inserted: List<E>,
     ) {
         val event = changeEvent
+        val listChange = ListChange(index, prevSize, removed, inserted)
 
-        // Reuse the same list of changes during a mutation.
-        val changes: MutableList<ListChange<E>> =
         if (event == null || changesMutationId != MutationManager.currentMutationId) {
             changesMutationId = MutationManager.currentMutationId
-                mutableListOf()
+            changeEvent = ListChangeEvent(elements, mutableListOf(listChange))
         } else {
-                // This cast is safe because we know we always instantiate our change event with a mutable list.
+            // Reuse the same list of changes during a mutation.
-                unsafeCast(event.changes)
+            // This cast is safe because we know we always instantiate our change event with a
+            // mutable list.
+            unsafeCast<MutableList<ListChange<E>>>(event.changes).add(listChange)
         }
-
-        changes.add(ListChange(index, prevSize, removed, inserted))
-        changeEvent = ListChangeEvent(elementsWrapper, changes)
     }
 }