Filtered list cells now support changing multiple times in a single mutation.

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

@@ -3,7 +3,10 @@ package world.phantasmal.cell
 typealias ChangeObserver<T> = (ChangeEvent<T>) -> Unit
 
 open class ChangeEvent<out T>(
-    /** The cell's new value. */
+    /**
+     * The cell's new value. Don't keep long-lived references to this object, it may change after
+     * change observers have been called.
+     */
     val value: T,
 ) {
     operator fun component1() = value

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

@@ -4,18 +4,27 @@ import world.phantasmal.core.assert
 import kotlin.contracts.InvocationKind.EXACTLY_ONCE
 import kotlin.contracts.contract
 
-// TODO: Throw exception by default when triggering early recomputation during change set. Allow to
-//       to turn this check off, because partial, early recomputation might be useful in rare cases.
-//       Dependents will need to partially apply ListChangeEvents etc. and remember which part of
-//       the event they've already applied (i.e. an index into the changes list).
 object MutationManager {
     private val invalidatedLeaves = HashSet<LeafDependent>()
 
     /** Non-zero when a mutation is active. */
-    private var mutationNestingLevel = 0
+    private var mutationNestingLevel: Int = 0
+
+    /**
+     * ID of the current outermost mutation. Meaningless when not in a mutation. Nested mutations
+     * don't have IDs at the moment.
+     */
+    var currentMutationId: Long = -1
+        private set
+
+    /**
+     * Set to true when a mutation was automatically started by changing a cell without first
+     * manually starting a mutation.
+     */
+    private var artificialMutation = false
 
-    /** Whether a dependency's value is changing at the moment. */
     private var dependencyChanging = false
+    private var pulling = false
 
     private val deferredMutations: MutableList<() -> Unit> = mutableListOf()
     private var applyingDeferredMutations = false
@@ -35,7 +44,7 @@ object MutationManager {
     }
 
     fun mutateDeferred(block: () -> Unit) {
-        if (dependencyChanging || mutationNestingLevel > 0) {
+        if (mutationNestingLevel > 0) {
             deferredMutations.add(block)
         } else {
             block()
@@ -43,23 +52,38 @@ object MutationManager {
     }
 
     fun mutationStart() {
+        assert(!dependencyChanging) { "Can't start a mutation while a dependency is changing." }
+        assert(!pulling) { "Can't start a mutation while pulling." }
+
+        if (mutationNestingLevel == 0) {
+            currentMutationId++
+        }
+
         mutationNestingLevel++
     }
 
     fun mutationEnd() {
         assert(mutationNestingLevel > 0) { "No mutation was started." }
 
-        mutationNestingLevel--
+        if (mutationNestingLevel == 1) {
+            assert(!pulling) { "Already pulling." }
 
-        if (mutationNestingLevel == 0) {
             try {
+                pulling = true
+
                 for (dependent in invalidatedLeaves) {
                     dependent.pull()
                 }
             } finally {
+                dependencyChanging = false
+                pulling = false
+                mutationNestingLevel--
                 invalidatedLeaves.clear()
                 applyDeferredMutations()
             }
+        } else {
+            dependencyChanging = false
+            mutationNestingLevel--
         }
     }
 
@@ -79,23 +103,20 @@ object MutationManager {
 
     fun dependencyChangeStart() {
         check(!dependencyChanging) { "A cell is already changing." }
+        assert(!pulling) { "Can't change a cell while pulling." }
+
+        if (mutationNestingLevel == 0) {
+            mutationStart()
+            artificialMutation = true
+        }
 
         dependencyChanging = true
     }
 
     fun dependencyChangeEnd() {
-        assert(dependencyChanging) { "No cell was changing." }
+        if (artificialMutation) {
-
+            artificialMutation = false
-        if (mutationNestingLevel == 0) {
+            mutationEnd()
-            try {
-                for (dependent in invalidatedLeaves) {
-                    dependent.pull()
-                }
-            } finally {
-                dependencyChanging = false
-                invalidatedLeaves.clear()
-                applyDeferredMutations()
-            }
         } else {
             dependencyChanging = false
         }
@@ -114,9 +135,7 @@ object MutationManager {
                 var idx = 0
 
                 while (idx < deferredMutations.size) {
-                    mutate {
+                    mutate(deferredMutations[idx])
-                        deferredMutations[idx]()
-                    }
 
                     idx++
                 }

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

@@ -2,6 +2,8 @@ package world.phantasmal.cell.list
 
 import world.phantasmal.cell.Dependency
 import world.phantasmal.cell.Dependent
+import world.phantasmal.cell.MutationManager
+import world.phantasmal.core.unsafe.unsafeCast
 
 abstract class AbstractFilteredListCell<E>(
     protected val list: ListCell<E>,
@@ -10,6 +12,10 @@ abstract class AbstractFilteredListCell<E>(
     /** Set during a change wave when [list] changes. */
     private var listInvalidated = false
 
+    /** Mutation ID during which the current list of changes was created. */
+    private var changesMutationId: Long = -1
+    private var listChangeIndex = 0
+
     /** Set during a change wave when [predicateDependency] changes. */
     private var predicateInvalidated = false
 
@@ -25,12 +31,12 @@ abstract class AbstractFilteredListCell<E>(
             return elementsWrapper
         }
 
-    final override var changeEvent: ListChangeEvent<E>? = null
+    private var _changeEvent: ListChangeEvent<E>? = null
+    final override val changeEvent: ListChangeEvent<E>?
         get() {
             computeValueAndEvent()
-            return field
+            return _changeEvent
         }
-        private set
 
     private fun computeValueAndEvent() {
         if (!valid) {
@@ -43,22 +49,34 @@ abstract class AbstractFilteredListCell<E>(
                 ignoreOtherChanges()
                 recompute()
 
-                changeEvent = ListChangeEvent(
+                _changeEvent = ListChangeEvent(
                     elementsWrapper,
                     listOf(ListChange(0, removed.size, removed, elementsWrapper)),
                 )
             } else {
                 // TODO: Conditionally copyAndResetWrapper?
                 copyAndResetWrapper()
-                val filteredChanges = mutableListOf<ListChange<E>>()
 
-                if (listInvalidated) {
+                // Reuse the same list of changes during a mutation.
-                    list.changeEvent?.let { listChangeEvent ->
+                val event = _changeEvent
-                        for (change in listChangeEvent.changes) {
+                val filteredChanges: MutableList<ListChange<E>> =
+                    if (event == null || changesMutationId != MutationManager.currentMutationId) {
+                        changesMutationId = MutationManager.currentMutationId
+                        listChangeIndex = 0
+                        mutableListOf()
+                    } else {
+                        // This cast is safe because we know we always instantiate our change event with a mutable list.
+                        unsafeCast(event.changes)
+                    }
+
+                val listChangeEvent = list.changeEvent
+
+                if (listInvalidated && listChangeEvent != null) {
+                    for (change in listChangeEvent.changes.listIterator(listChangeIndex)) {
                         val prevSize = elements.size
                         // Map the incoming change index to an index into our own elements list.
                         // TODO: Avoid this loop by storing the index where an element "would"
-                            //       be if it passed the predicate.
+                        //       be if it passed the predicate?
                         var eventIndex = prevSize
 
                         for (index in change.index..maxDepIndex()) {
@@ -124,12 +142,13 @@ abstract class AbstractFilteredListCell<E>(
                             )
                         }
                     }
-                    }
+
+                    listChangeIndex = listChangeEvent.changes.size
                 }
 
                 processOtherChanges(filteredChanges)
 
-                changeEvent =
+                _changeEvent =
                     if (filteredChanges.isEmpty()) {
                         null
                     } else {

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

@@ -36,8 +36,8 @@ class ListElementsDependentCell<E>(
     private fun updateElementDependenciesAndEvent() {
         if (!valid) {
             if (listInvalidated) {
-                // At this point we can remove this dependent from the removed elements' dependencies
+                // At this point we can remove this dependent from the removed elements'
-                // and add it to the newly inserted elements' dependencies.
+                // dependencies and add it to the newly inserted elements' dependencies.
                 list.changeEvent?.let { listChangeEvent ->
                     for (change in listChangeEvent.changes) {
                         for (i in change.index until (change.index + change.removed.size)) {

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

@@ -1,11 +1,12 @@
 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].
  */
-// TODO: Support change sets by sometimes appending to changeEvent instead of always overwriting it.
 class SimpleListCell<E>(
     override val elements: MutableList<E>,
 ) : AbstractElementsWrappingListCell<E>(), MutableListCell<E> {
@@ -19,6 +20,9 @@ class SimpleListCell<E>(
     override var changeEvent: ListChangeEvent<E>? = null
         private set
 
+    /** Mutation ID during which the current list of changes was created. */
+    private var changesMutationId: Long = -1
+
     override operator fun get(index: Int): E =
         elements[index]
 
@@ -135,6 +139,10 @@ class SimpleListCell<E>(
     }
 
     override fun clear() {
+        if (elements.isEmpty()) {
+            return
+        }
+
         applyChange {
             val prevSize = elements.size
             val removed = elementsWrapper
@@ -185,9 +193,19 @@ class SimpleListCell<E>(
         removed: List<E>,
         inserted: List<E>,
     ) {
-        changeEvent = ListChangeEvent(
+        val event = changeEvent
-            elementsWrapper,
+
-            listOf(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()
+            } else {
+                // This cast is safe because we know we always instantiate our change event with a mutable list.
+                unsafeCast(event.changes)
+            }
+
+        changes.add(ListChange(index, prevSize, removed, inserted))
+        changeEvent = ListChangeEvent(elementsWrapper, changes)
     }
 }

cell/src/commonTest/kotlin/world/phantasmal/cell/CellTests.kt

@@ -1,8 +1,6 @@
 package world.phantasmal.cell
 
 import world.phantasmal.cell.test.CellTestSuite
-import world.phantasmal.cell.test.Snapshot
-import world.phantasmal.cell.test.snapshot
 import world.phantasmal.core.disposable.use
 import kotlin.test.Test
 import kotlin.test.assertEquals
@@ -245,11 +243,15 @@ interface CellTests : CellTestSuite {
             }
         )
 
+        // Repeat 3 times to check if temporary state is always reset.
+        repeat(3) {
+            changes = 0
+
             mutate {
                 repeat(5) {
                     p.emit()
 
-                // Change should be deferred until this lambda returns.
+                    // Change should be deferred until this mutation finishes.
                     assertEquals(0, changes)
                 }
             }
@@ -257,21 +259,26 @@ interface CellTests : CellTestSuite {
             // All changes to the same cell should be collapsed to a single change.
             assertEquals(1, changes)
         }
+    }
 
     @Test
     fun value_can_be_accessed_during_a_mutation() = test {
         val p = createProvider()
 
-        // Change will be observed exactly once.
         var observedValue: Snapshot? = null
 
         disposer.add(
             p.cell.observeChange {
+                // Change will be observed exactly once every loop iteration.
                 assertNull(observedValue)
                 observedValue = it.value.snapshot()
             }
         )
 
+        // Repeat 3 times to check that temporary state is always reset.
+        repeat(3) {
+            observedValue = null
+
             val v1 = p.cell.value.snapshot()
             var v3: Snapshot? = null
 
@@ -286,6 +293,8 @@ interface CellTests : CellTestSuite {
                 assertNotEquals(v2, v3)
 
                 p.emit()
+
+                assertNull(observedValue)
             }
 
             val v4 = p.cell.value.snapshot()
@@ -294,15 +303,16 @@ interface CellTests : CellTestSuite {
             assertNotEquals(v3, v4)
             assertEquals(v4, observedValue)
         }
+    }
 
     @Test
     fun mutations_can_be_nested() = test {
         // 3 Cells.
         val ps = Array(3) { createProvider() }
-        val observedChanges = IntArray(3)
+        val observedChanges = IntArray(ps.size)
 
         // Observe each cell.
-        repeat(3) { idx ->
+        for (idx in ps.indices) {
             disposer.add(
                 ps[idx].cell.observeChange {
                     assertEquals(0, observedChanges[idx])
@@ -342,3 +352,16 @@ interface CellTests : CellTestSuite {
         fun emit()
     }
 }
+
+/** See [snapshot]. */
+typealias Snapshot = String
+
+/**
+ * We use toString to create "snapshots" of values throughout the tests. Most of the time cells will
+ * actually have a new value after emitting a change event, but this is not always the case with
+ * more complex cells or cells that point to complex values. So instead of keeping references to
+ * values and comparing them with == (or using e.g. assertEquals), we compare snapshots.
+ *
+ * This of course assumes that all values have sensible toString implementations.
+ */
+fun Any?.snapshot(): Snapshot = toString()

cell/src/commonTest/kotlin/world/phantasmal/cell/list/AbstractFilteredListCellTests.kt

@@ -4,12 +4,16 @@ import world.phantasmal.cell.Cell
 import world.phantasmal.cell.ImmutableCell
 import world.phantasmal.cell.SimpleCell
 import world.phantasmal.cell.test.CellTestSuite
-import kotlin.test.*
+import kotlin.test.Test
+import kotlin.test.assertEquals
+import kotlin.test.assertNotNull
+import kotlin.test.assertNull
+import kotlin.test.assertTrue
 
 /**
  * Tests that apply to all filtered list implementations.
  */
-interface SuperFilteredListCellTests : CellTestSuite {
+interface AbstractFilteredListCellTests : CellTestSuite {
     fun <E> createFilteredListCell(list: ListCell<E>, predicate: Cell<(E) -> Boolean>): ListCell<E>
 
     @Test

cell/src/commonTest/kotlin/world/phantasmal/cell/list/FilteredListCellTests.kt

@@ -8,7 +8,7 @@ import world.phantasmal.cell.map
 // TODO: A test suite that tests FilteredListCell while the predicate results are changing.
 // TODO: A test suite that tests FilteredListCell while all 3 types of dependencies are changing.
 @Suppress("unused")
-class FilteredListCellTests : SuperFilteredListCellTests {
+class FilteredListCellTests : AbstractFilteredListCellTests {
     override fun <E> createFilteredListCell(list: ListCell<E>, predicate: Cell<(E) -> Boolean>) =
         FilteredListCell(list, predicate.map { p -> { cell(p(it)) } })
 }

cell/src/commonTest/kotlin/world/phantasmal/cell/list/ListCellTests.kt

@@ -1,7 +1,14 @@
 package world.phantasmal.cell.list
 
 import world.phantasmal.cell.CellTests
-import kotlin.test.*
+import world.phantasmal.cell.mutate
+import kotlin.test.Test
+import kotlin.test.assertEquals
+import kotlin.test.assertFailsWith
+import kotlin.test.assertNotEquals
+import kotlin.test.assertNotNull
+import kotlin.test.assertNull
+import kotlin.test.assertTrue
 
 /**
  * Test suite for all [ListCell] implementations. There is a subclass of this suite for every
@@ -216,6 +223,56 @@ interface ListCellTests : CellTests {
         }
     }
 
+    /**
+     * The same as [value_can_be_accessed_during_a_mutation], except that it also checks expected
+     * list sizes.
+     */
+    @Test
+    fun list_cell_value_can_be_accessed_during_a_mutation() = test {
+        val p = createProvider()
+
+        var observedValue: List<Any>? = null
+
+        disposer.add(
+            p.cell.observeChange {
+                // Change will be observed exactly once every loop iteration.
+                assertNull(observedValue)
+                observedValue = it.value.toList()
+            }
+        )
+
+        // Repeat 3 times to check that temporary state is always reset.
+        repeat(3) {
+            observedValue = null
+
+            val v1 = p.cell.value.toList()
+            var v3: List<Any>? = null
+
+            mutate {
+                val v2 = p.cell.value.toList()
+
+                assertEquals(v1, v2)
+
+                p.addElement()
+                v3 = p.cell.value.toList()
+
+                assertNotEquals(v2, v3)
+                assertEquals(v2.size + 1, v3!!.size)
+
+                p.addElement()
+
+                assertNull(observedValue)
+            }
+
+            val v4 = p.cell.value.toList()
+
+            assertNotNull(v3)
+            assertNotEquals(v3, v4)
+            assertEquals(v3!!.size + 1, v4.size)
+            assertEquals(v4, observedValue)
+        }
+    }
+
     interface Provider : CellTests.Provider {
         override val cell: ListCell<Any>
 

cell/src/commonTest/kotlin/world/phantasmal/cell/list/SimpleFilteredListCellTests.kt

@@ -9,7 +9,7 @@ import world.phantasmal.cell.Cell
  * [SimpleFilteredListCellPredicateDependencyEmitsTests].
  */
 @Suppress("unused")
-class SimpleFilteredListCellTests : SuperFilteredListCellTests {
+class SimpleFilteredListCellTests : AbstractFilteredListCellTests {
     override fun <E> createFilteredListCell(list: ListCell<E>, predicate: Cell<(E) -> Boolean>) =
         SimpleFilteredListCell(list, predicate)
 }

cell/src/commonTest/kotlin/world/phantasmal/cell/test/Utils.kt

@@ -7,16 +7,3 @@ fun <E> assertListCellEquals(expected: List<E>, actual: ListCell<E>) {
     assertEquals(expected.size, actual.size.value)
     assertEquals(expected, actual.value)
 }
-
-/** See [snapshot]. */
-typealias Snapshot = String
-
-/**
- * We use toString to create "snapshots" of values throughout the tests. Most of the time cells will
- * actually have a new value after emitting a change event, but this is not always the case with
- * more complex cells or cells that point to complex values. So instead of keeping references to
- * values and comparing them with == (or using e.g. assertEquals), we compare snapshots.
- *
- * This of course assumes that all values have sensible toString implementations.
- */
-fun Any?.snapshot(): Snapshot = toString()