diff --git a/cell/src/commonMain/kotlin/world/phantasmal/cell/ChangeObserver.kt b/cell/src/commonMain/kotlin/world/phantasmal/cell/ChangeObserver.kt index 853206f4..6fd266f3 100644 --- a/cell/src/commonMain/kotlin/world/phantasmal/cell/ChangeObserver.kt +++ b/cell/src/commonMain/kotlin/world/phantasmal/cell/ChangeObserver.kt @@ -3,7 +3,10 @@ package world.phantasmal.cell typealias ChangeObserver = (ChangeEvent) -> Unit open class ChangeEvent( - /** 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 diff --git a/cell/src/commonMain/kotlin/world/phantasmal/cell/MutationManager.kt b/cell/src/commonMain/kotlin/world/phantasmal/cell/MutationManager.kt index e9f2211c..f9e9b5ae 100644 --- a/cell/src/commonMain/kotlin/world/phantasmal/cell/MutationManager.kt +++ b/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() /** 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 (mutationNestingLevel == 0) { - try { - for (dependent in invalidatedLeaves) { - dependent.pull() - } - } finally { - dependencyChanging = false - invalidatedLeaves.clear() - applyDeferredMutations() - } + if (artificialMutation) { + artificialMutation = false + mutationEnd() } else { dependencyChanging = false } @@ -114,9 +135,7 @@ object MutationManager { var idx = 0 while (idx < deferredMutations.size) { - mutate { - deferredMutations[idx]() - } + mutate(deferredMutations[idx]) idx++ } diff --git a/cell/src/commonMain/kotlin/world/phantasmal/cell/list/AbstractFilteredListCell.kt b/cell/src/commonMain/kotlin/world/phantasmal/cell/list/AbstractFilteredListCell.kt index 6e7544f5..3ca4a872 100644 --- a/cell/src/commonMain/kotlin/world/phantasmal/cell/list/AbstractFilteredListCell.kt +++ b/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( protected val list: ListCell, @@ -10,6 +12,10 @@ abstract class AbstractFilteredListCell( /** 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( return elementsWrapper } - final override var changeEvent: ListChangeEvent? = null + private var _changeEvent: ListChangeEvent? = null + final override val changeEvent: ListChangeEvent? get() { computeValueAndEvent() - return field + return _changeEvent } - private set private fun computeValueAndEvent() { if (!valid) { @@ -43,93 +49,106 @@ abstract class AbstractFilteredListCell( ignoreOtherChanges() recompute() - changeEvent = ListChangeEvent( + _changeEvent = ListChangeEvent( elementsWrapper, listOf(ListChange(0, removed.size, removed, elementsWrapper)), ) } else { // TODO: Conditionally copyAndResetWrapper? copyAndResetWrapper() - val filteredChanges = mutableListOf>() - if (listInvalidated) { - list.changeEvent?.let { listChangeEvent -> - for (change in listChangeEvent.changes) { - 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. - var eventIndex = prevSize + // Reuse the same list of changes during a mutation. + val event = _changeEvent + val filteredChanges: MutableList> = + 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) + } - for (index in change.index..maxDepIndex()) { - val i = mapIndex(index) + val listChangeEvent = list.changeEvent - if (i != -1) { - eventIndex = i - break - } - } + 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? + var eventIndex = prevSize - // Process removals. - val removed = mutableListOf() + for (index in change.index..maxDepIndex()) { + val i = mapIndex(index) - for (element in change.removed) { - val index = removeIndexMapping(change.index) - - if (index != -1) { - elements.removeAt(eventIndex) - removed.add(element) - } - } - - // Process insertions. - val inserted = mutableListOf() - var insertionIndex = eventIndex - - for ((i, element) in change.inserted.withIndex()) { - if (applyPredicate(element)) { - insertIndexMapping(change.index + i, insertionIndex, element) - elements.add(insertionIndex, element) - inserted.add(element) - insertionIndex++ - } else { - insertIndexMapping(change.index + i, -1, element) - } - } - - // Shift mapped indices by a certain amount. This amount can be - // positive, negative or zero. - val diff = inserted.size - removed.size - - if (diff != 0) { - // Indices before the change index stay the same. Newly inserted - // indices are already correct. So we only need to shift everything - // after the new indices. - val startIndex = change.index + change.inserted.size - - for (index in startIndex..maxDepIndex()) { - shiftIndexMapping(index, diff) - } - } - - // Add a list change if something actually changed. - if (removed.isNotEmpty() || inserted.isNotEmpty()) { - filteredChanges.add( - ListChange( - eventIndex, - prevSize, - removed, - inserted, - ) - ) + if (i != -1) { + eventIndex = i + break } } + + // Process removals. + val removed = mutableListOf() + + for (element in change.removed) { + val index = removeIndexMapping(change.index) + + if (index != -1) { + elements.removeAt(eventIndex) + removed.add(element) + } + } + + // Process insertions. + val inserted = mutableListOf() + var insertionIndex = eventIndex + + for ((i, element) in change.inserted.withIndex()) { + if (applyPredicate(element)) { + insertIndexMapping(change.index + i, insertionIndex, element) + elements.add(insertionIndex, element) + inserted.add(element) + insertionIndex++ + } else { + insertIndexMapping(change.index + i, -1, element) + } + } + + // Shift mapped indices by a certain amount. This amount can be + // positive, negative or zero. + val diff = inserted.size - removed.size + + if (diff != 0) { + // Indices before the change index stay the same. Newly inserted + // indices are already correct. So we only need to shift everything + // after the new indices. + val startIndex = change.index + change.inserted.size + + for (index in startIndex..maxDepIndex()) { + shiftIndexMapping(index, diff) + } + } + + // Add a list change if something actually changed. + if (removed.isNotEmpty() || inserted.isNotEmpty()) { + filteredChanges.add( + ListChange( + eventIndex, + prevSize, + removed, + inserted, + ) + ) + } } + + listChangeIndex = listChangeEvent.changes.size } processOtherChanges(filteredChanges) - changeEvent = + _changeEvent = if (filteredChanges.isEmpty()) { null } else { diff --git a/cell/src/commonMain/kotlin/world/phantasmal/cell/list/ListElementsDependentCell.kt b/cell/src/commonMain/kotlin/world/phantasmal/cell/list/ListElementsDependentCell.kt index 2e4e328f..6be66cc1 100644 --- a/cell/src/commonMain/kotlin/world/phantasmal/cell/list/ListElementsDependentCell.kt +++ b/cell/src/commonMain/kotlin/world/phantasmal/cell/list/ListElementsDependentCell.kt @@ -36,8 +36,8 @@ class ListElementsDependentCell( private fun updateElementDependenciesAndEvent() { if (!valid) { if (listInvalidated) { - // At this point we can remove this dependent from the removed elements' dependencies - // and add it to the newly inserted elements' dependencies. + // At this point we can remove this dependent from the removed elements' + // 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)) { diff --git a/cell/src/commonMain/kotlin/world/phantasmal/cell/list/SimpleListCell.kt b/cell/src/commonMain/kotlin/world/phantasmal/cell/list/SimpleListCell.kt index 8646e835..839832a1 100644 --- a/cell/src/commonMain/kotlin/world/phantasmal/cell/list/SimpleListCell.kt +++ b/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( override val elements: MutableList, ) : AbstractElementsWrappingListCell(), MutableListCell { @@ -19,6 +20,9 @@ class SimpleListCell( override var changeEvent: ListChangeEvent? = 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( } override fun clear() { + if (elements.isEmpty()) { + return + } + applyChange { val prevSize = elements.size val removed = elementsWrapper @@ -185,9 +193,19 @@ class SimpleListCell( removed: List, inserted: List, ) { - changeEvent = ListChangeEvent( - elementsWrapper, - listOf(ListChange(index, prevSize, removed, inserted)), - ) + val event = changeEvent + + // Reuse the same list of changes during a mutation. + val changes: MutableList> = + 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) } } diff --git a/cell/src/commonTest/kotlin/world/phantasmal/cell/CellTests.kt b/cell/src/commonTest/kotlin/world/phantasmal/cell/CellTests.kt index 1089d5e8..e5d44e20 100644 --- a/cell/src/commonTest/kotlin/world/phantasmal/cell/CellTests.kt +++ b/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,64 +243,76 @@ interface CellTests : CellTestSuite { } ) - mutate { - repeat(5) { - p.emit() + // Repeat 3 times to check if temporary state is always reset. + repeat(3) { + changes = 0 - // Change should be deferred until this lambda returns. - assertEquals(0, changes) + mutate { + repeat(5) { + p.emit() + + // Change should be deferred until this mutation finishes. + assertEquals(0, changes) + } } - } - // All changes to the same cell should be collapsed to a single change. - assertEquals(1, changes) + // 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() } ) - val v1 = p.cell.value.snapshot() - var v3: Snapshot? = null + // Repeat 3 times to check that temporary state is always reset. + repeat(3) { + observedValue = null - mutate { - val v2 = p.cell.value.snapshot() + val v1 = p.cell.value.snapshot() + var v3: Snapshot? = null - assertEquals(v1, v2) + mutate { + val v2 = p.cell.value.snapshot() - p.emit() - v3 = p.cell.value.snapshot() + assertEquals(v1, v2) - assertNotEquals(v2, v3) + p.emit() + v3 = p.cell.value.snapshot() - p.emit() + assertNotEquals(v2, v3) + + p.emit() + + assertNull(observedValue) + } + + val v4 = p.cell.value.snapshot() + + assertNotNull(v3) + assertNotEquals(v3, v4) + assertEquals(v4, observedValue) } - - val v4 = p.cell.value.snapshot() - - assertNotNull(v3) - 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() diff --git a/cell/src/commonTest/kotlin/world/phantasmal/cell/list/SuperFilteredListCellTests.kt b/cell/src/commonTest/kotlin/world/phantasmal/cell/list/AbstractFilteredListCellTests.kt similarity index 94% rename from cell/src/commonTest/kotlin/world/phantasmal/cell/list/SuperFilteredListCellTests.kt rename to cell/src/commonTest/kotlin/world/phantasmal/cell/list/AbstractFilteredListCellTests.kt index 32ef189a..83de2c4e 100644 --- a/cell/src/commonTest/kotlin/world/phantasmal/cell/list/SuperFilteredListCellTests.kt +++ b/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 createFilteredListCell(list: ListCell, predicate: Cell<(E) -> Boolean>): ListCell @Test @@ -174,11 +178,11 @@ interface SuperFilteredListCellTests : CellTestSuite { for (newElement in newElements) { changes.add( ListChange( - index = elements.size, - prevSize = elements.size, - removed = emptyList(), - inserted = listOf(newElement), - ) + index = elements.size, + prevSize = elements.size, + removed = emptyList(), + inserted = listOf(newElement), + ) ) elements.add(newElement) } diff --git a/cell/src/commonTest/kotlin/world/phantasmal/cell/list/FilteredListCellTests.kt b/cell/src/commonTest/kotlin/world/phantasmal/cell/list/FilteredListCellTests.kt index 0c2b7f84..e7e97b49 100644 --- a/cell/src/commonTest/kotlin/world/phantasmal/cell/list/FilteredListCellTests.kt +++ b/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 createFilteredListCell(list: ListCell, predicate: Cell<(E) -> Boolean>) = FilteredListCell(list, predicate.map { p -> { cell(p(it)) } }) } diff --git a/cell/src/commonTest/kotlin/world/phantasmal/cell/list/ListCellTests.kt b/cell/src/commonTest/kotlin/world/phantasmal/cell/list/ListCellTests.kt index 1697e2d2..4e1968bf 100644 --- a/cell/src/commonTest/kotlin/world/phantasmal/cell/list/ListCellTests.kt +++ b/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? = 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? = 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 diff --git a/cell/src/commonTest/kotlin/world/phantasmal/cell/list/SimpleFilteredListCellTests.kt b/cell/src/commonTest/kotlin/world/phantasmal/cell/list/SimpleFilteredListCellTests.kt index d34a968d..043d5e5c 100644 --- a/cell/src/commonTest/kotlin/world/phantasmal/cell/list/SimpleFilteredListCellTests.kt +++ b/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 createFilteredListCell(list: ListCell, predicate: Cell<(E) -> Boolean>) = SimpleFilteredListCell(list, predicate) } diff --git a/cell/src/commonTest/kotlin/world/phantasmal/cell/test/Utils.kt b/cell/src/commonTest/kotlin/world/phantasmal/cell/test/Utils.kt index ea40c2d3..6a2f6092 100644 --- a/cell/src/commonTest/kotlin/world/phantasmal/cell/test/Utils.kt +++ b/cell/src/commonTest/kotlin/world/phantasmal/cell/test/Utils.kt @@ -7,16 +7,3 @@ fun assertListCellEquals(expected: List, actual: ListCell) { 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()