diff --git a/cell/src/commonMain/kotlin/world/phantasmal/cell/AbstractFlatteningDependentCell.kt b/cell/src/commonMain/kotlin/world/phantasmal/cell/AbstractFlatteningDependentCell.kt index 521ce691..50a7eca8 100644 --- a/cell/src/commonMain/kotlin/world/phantasmal/cell/AbstractFlatteningDependentCell.kt +++ b/cell/src/commonMain/kotlin/world/phantasmal/cell/AbstractFlatteningDependentCell.kt @@ -38,7 +38,7 @@ abstract class AbstractFlatteningDependentCell, 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, Event } } + protected abstract fun transformNewValue(value: T): T + protected abstract fun createEvent(oldValue: T?, newValue: T): Event override fun addDependent(dependent: Dependent) { diff --git a/cell/src/commonMain/kotlin/world/phantasmal/cell/ChangeObserver.kt b/cell/src/commonMain/kotlin/world/phantasmal/cell/ChangeObserver.kt index 6fd266f3..b1154192 100644 --- a/cell/src/commonMain/kotlin/world/phantasmal/cell/ChangeObserver.kt +++ b/cell/src/commonMain/kotlin/world/phantasmal/cell/ChangeObserver.kt @@ -2,6 +2,10 @@ package world.phantasmal.cell typealias ChangeObserver = (ChangeEvent) -> Unit +/** + * Don't keep long-lived references to change events, they may change internally after change + * observers have been called. + */ open class ChangeEvent( /** * The cell's new value. Don't keep long-lived references to this object, it may change after diff --git a/cell/src/commonMain/kotlin/world/phantasmal/cell/FlatteningDependentCell.kt b/cell/src/commonMain/kotlin/world/phantasmal/cell/FlatteningDependentCell.kt index e57bd635..6c9effb1 100644 --- a/cell/src/commonMain/kotlin/world/phantasmal/cell/FlatteningDependentCell.kt +++ b/cell/src/commonMain/kotlin/world/phantasmal/cell/FlatteningDependentCell.kt @@ -7,6 +7,7 @@ class FlatteningDependentCell( vararg dependencies: Cell<*>, compute: () -> Cell, ) : AbstractFlatteningDependentCell, ChangeEvent>(dependencies, compute) { + override fun transformNewValue(value: T): T = value override fun createEvent(oldValue: T?, newValue: T): ChangeEvent = ChangeEvent(newValue) diff --git a/cell/src/commonMain/kotlin/world/phantasmal/cell/list/AbstractElementsWrappingListCell.kt b/cell/src/commonMain/kotlin/world/phantasmal/cell/list/AbstractElementsWrappingListCell.kt deleted file mode 100644 index 6dc49b18..00000000 --- a/cell/src/commonMain/kotlin/world/phantasmal/cell/list/AbstractElementsWrappingListCell.kt +++ /dev/null @@ -1,34 +0,0 @@ -package world.phantasmal.cell.list - -import world.phantasmal.core.unsafe.unsafeAssertNotNull - -abstract class AbstractElementsWrappingListCell : AbstractListCell() { - /** - * 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? = null - protected val elementsWrapper: DelegatingList - get() { - if (_elementsWrapper == null) { - _elementsWrapper = DelegatingList(elements) - } - - return unsafeAssertNotNull(_elementsWrapper) - } - - protected abstract val elements: List - - protected fun copyAndResetWrapper() { - _elementsWrapper?.backingList = elements.toList() - _elementsWrapper = null - } -} 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 3ca4a872..4ef19d67 100644 --- a/cell/src/commonMain/kotlin/world/phantasmal/cell/list/AbstractFilteredListCell.kt +++ b/cell/src/commonMain/kotlin/world/phantasmal/cell/list/AbstractFilteredListCell.kt @@ -7,7 +7,7 @@ import world.phantasmal.core.unsafe.unsafeCast abstract class AbstractFilteredListCell( protected val list: ListCell, -) : AbstractElementsWrappingListCell(), Dependent { +) : AbstractListCell(), Dependent { /** Set during a change wave when [list] changes. */ private var listInvalidated = false @@ -21,14 +21,14 @@ abstract class AbstractFilteredListCell( private var valid = false - final override val elements = mutableListOf() + protected val elements = mutableListOf() protected abstract val predicateDependency: Dependency<*> final override val value: List get() { computeValueAndEvent() - return elementsWrapper + return elements } private var _changeEvent: ListChangeEvent? = null @@ -44,30 +44,30 @@ abstract class AbstractFilteredListCell( if (predicateInvalidated || !hasDependents) { // Simply assume the entire list changes and recompute. - val removed = elementsWrapper + val removed = elements.toList() ignoreOtherChanges() recompute() _changeEvent = ListChangeEvent( - elementsWrapper, - listOf(ListChange(0, removed.size, removed, elementsWrapper)), + elements, + 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> = - 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 filteredChanges: MutableList> + + if (event == null || changesMutationId != MutationManager.currentMutationId) { + changesMutationId = MutationManager.currentMutationId + listChangeIndex = 0 + filteredChanges = mutableListOf() + _changeEvent = ListChangeEvent(elements, filteredChanges) + } else { + // This cast is safe because we know we always instantiate our change event + // with a mutable list. + filteredChanges = unsafeCast(event.changes) + } val listChangeEvent = list.changeEvent @@ -148,12 +148,11 @@ abstract class AbstractFilteredListCell( processOtherChanges(filteredChanges) - _changeEvent = - if (filteredChanges.isEmpty()) { - null - } else { - ListChangeEvent(elementsWrapper, filteredChanges) - } + if (filteredChanges.isEmpty()) { + _changeEvent = null + } else { + // Keep the previous change event, it has been changed internally. + } } // Reset for next change wave. diff --git a/cell/src/commonMain/kotlin/world/phantasmal/cell/list/DelegatingList.kt b/cell/src/commonMain/kotlin/world/phantasmal/cell/list/DelegatingList.kt deleted file mode 100644 index 3545b740..00000000 --- a/cell/src/commonMain/kotlin/world/phantasmal/cell/list/DelegatingList.kt +++ /dev/null @@ -1,35 +0,0 @@ -package world.phantasmal.cell.list - -/** - * Simply delegates all methods to [backingList], even [equals], [hashCode] and [toString]. - */ -class DelegatingList(var backingList: List) : List { - override val size: Int = backingList.size - - override fun contains(element: E): Boolean = backingList.contains(element) - - override fun containsAll(elements: Collection): 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 = backingList.iterator() - - override fun lastIndexOf(element: E): Int = backingList.lastIndexOf(element) - - override fun listIterator(): ListIterator = backingList.listIterator() - - override fun listIterator(index: Int): ListIterator = backingList.listIterator(index) - - override fun subList(fromIndex: Int, toIndex: Int): List = - backingList.subList(fromIndex, toIndex) - - override fun equals(other: Any?): Boolean = other == backingList - - override fun hashCode(): Int = backingList.hashCode() - - override fun toString(): String = backingList.toString() -} diff --git a/cell/src/commonMain/kotlin/world/phantasmal/cell/list/FilteredListCell.kt b/cell/src/commonMain/kotlin/world/phantasmal/cell/list/FilteredListCell.kt index 9df03a6d..5412c62c 100644 --- a/cell/src/commonMain/kotlin/world/phantasmal/cell/list/FilteredListCell.kt +++ b/cell/src/commonMain/kotlin/world/phantasmal/cell/list/FilteredListCell.kt @@ -148,7 +148,6 @@ class FilteredListCell( } override fun recompute() { - copyAndResetWrapper() elements.clear() for (mapping in indexMap) { diff --git a/cell/src/commonMain/kotlin/world/phantasmal/cell/list/FlatteningDependentListCell.kt b/cell/src/commonMain/kotlin/world/phantasmal/cell/list/FlatteningDependentListCell.kt index edc24e6c..29748f1a 100644 --- a/cell/src/commonMain/kotlin/world/phantasmal/cell/list/FlatteningDependentListCell.kt +++ b/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( vararg dependencies: Cell<*>, computeElements: () -> ListCell, @@ -59,6 +57,10 @@ class FlatteningDependentListCell( override fun toString(): String = listCellToString(this) + override fun transformNewValue(value: List): List = + // Make a copy because this value is later used as the "removed" field of a list change. + value.toList() + override fun createEvent(oldValue: List?, newValue: List): ListChangeEvent { val old = oldValue ?: emptyList() return ListChangeEvent( diff --git a/cell/src/commonMain/kotlin/world/phantasmal/cell/list/ListCellUtils.kt b/cell/src/commonMain/kotlin/world/phantasmal/cell/list/ListCellUtils.kt index 6cc912b4..667dd0f9 100644 --- a/cell/src/commonMain/kotlin/world/phantasmal/cell/list/ListCellUtils.kt +++ b/cell/src/commonMain/kotlin/world/phantasmal/cell/list/ListCellUtils.kt @@ -86,11 +86,12 @@ fun flatMapToList( ): ListCell = FlatteningDependentListCell(c1, c2) { transform(c1.value, c2.value) } -fun listCellToString(cell: ListCell<*>): String = buildString { - append(this::class.simpleName) - append('[') - cell.value.joinTo(this, limit = 20) { - if (it === this) "(this cell)" else it.toString() +fun listCellToString(cell: ListCell<*>): String = + buildString { + append(cell::class.simpleName) + append('[') + cell.value.joinTo(this, limit = 20) { + if (it === cell) "(this cell)" else it.toString() + } + append(']') } - append(']') -} diff --git a/cell/src/commonMain/kotlin/world/phantasmal/cell/list/SimpleFilteredListCell.kt b/cell/src/commonMain/kotlin/world/phantasmal/cell/list/SimpleFilteredListCell.kt index 7e6f4005..7db2eff6 100644 --- a/cell/src/commonMain/kotlin/world/phantasmal/cell/list/SimpleFilteredListCell.kt +++ b/cell/src/commonMain/kotlin/world/phantasmal/cell/list/SimpleFilteredListCell.kt @@ -58,7 +58,6 @@ class SimpleFilteredListCell( } override fun recompute() { - copyAndResetWrapper() elements.clear() indexMap.clear() 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 839832a1..d3abb449 100644 --- a/cell/src/commonMain/kotlin/world/phantasmal/cell/list/SimpleListCell.kt +++ b/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( - override val elements: MutableList, -) : AbstractElementsWrappingListCell(), MutableListCell { + private var elements: MutableList, +) : AbstractListCell(), MutableListCell { override var value: List - get() = elementsWrapper + get() = elements set(value) { replaceAll(value) } @@ -30,7 +29,6 @@ class SimpleListCell( checkIndex(index, elements.lastIndex) applyChange { - copyAndResetWrapper() val removed = elements.set(index, element) finalizeChange( @@ -47,7 +45,6 @@ class SimpleListCell( override fun add(element: E) { applyChange { val index = elements.size - copyAndResetWrapper() elements.add(element) finalizeChange( @@ -64,7 +61,6 @@ class SimpleListCell( checkIndex(index, prevSize) applyChange { - copyAndResetWrapper() elements.add(index, element) finalizeChange(index, prevSize, removed = emptyList(), inserted = listOf(element)) @@ -87,8 +83,6 @@ class SimpleListCell( 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( override fun replaceAll(elements: Iterable) { applyChange { - val prevSize = this.elements.size - val removed = elementsWrapper + val removed = this.elements - copyAndResetWrapper() - this.elements.replaceAll(elements) + this.elements = elements.toMutableList() - finalizeChange(index = 0, prevSize, removed, inserted = elementsWrapper) + finalizeChange(index = 0, prevSize = removed.size, removed, inserted = this.elements) } } override fun replaceAll(elements: Sequence) { applyChange { - val prevSize = this.elements.size - val removed = elementsWrapper + val removed = this.elements - copyAndResetWrapper() - this.elements.replaceAll(elements) + this.elements = elements.toMutableList() - finalizeChange(index = 0, prevSize, removed, inserted = elementsWrapper) + finalizeChange(index = 0, prevSize = removed.size, removed, inserted = this.elements) } } @@ -130,7 +120,6 @@ class SimpleListCell( } applyChange { - copyAndResetWrapper() repeat(removeCount) { elements.removeAt(fromIndex) } elements.add(fromIndex, newElement) @@ -144,20 +133,17 @@ class SimpleListCell( } applyChange { - val prevSize = elements.size - val removed = elementsWrapper + val removed = elements - copyAndResetWrapper() - elements.clear() + elements = mutableListOf() - finalizeChange(index = 0, prevSize, removed, inserted = emptyList()) + finalizeChange(index = 0, prevSize = removed.size, removed, inserted = emptyList()) } } override fun sortWith(comparator: Comparator) { applyChange { - val removed = elementsWrapper - copyAndResetWrapper() + val removed = elements.toList() var throwable: Throwable? = null try { @@ -170,7 +156,7 @@ class SimpleListCell( index = 0, prevSize = elements.size, removed, - inserted = elementsWrapper, + inserted = elements, ) if (throwable != null) { @@ -194,18 +180,16 @@ class SimpleListCell( inserted: List, ) { val event = changeEvent + val listChange = ListChange(index, prevSize, removed, inserted) - // 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) + if (event == null || changesMutationId != MutationManager.currentMutationId) { + changesMutationId = MutationManager.currentMutationId + changeEvent = ListChangeEvent(elements, mutableListOf(listChange)) + } else { + // Reuse the same list of changes during a mutation. + // This cast is safe because we know we always instantiate our change event with a + // mutable list. + unsafeCast>>(event.changes).add(listChange) + } } }