Merge pull request #8 from opennetltd/fix-reload-after-item-deletes

Perform item reloads in a separate batch of updates

Sources/ComposedUI/CollectionView/CollectionCoordinator.swift

@@ -365,6 +365,12 @@ extension CollectionCoordinator: SectionProviderMappingDelegate {
         debugLog("Layout out collection view, if needed")
         collectionView.layoutIfNeeded()
         debugLog("Collection view has been laid out")
+
+        /// The index paths of the items that need to be updated. Due to a bug in UICollectionView
+        /// these updates are performed in a second performBatchUpdates immediately after the first
+        /// batch updates.
+        var elementsUpdated: Set<IndexPath>?
+
         collectionView.performBatchUpdates({
             debugLog("Starting batch updates")
             changesReducer.beginUpdating()
@@ -384,14 +390,10 @@ extension CollectionCoordinator: SectionProviderMappingDelegate {
             debugLog("Deleting items \(changeset.elementsRemoved.sorted(by: >))")
             collectionView.deleteItems(at: Array(changeset.elementsRemoved))
 
-            debugLog("Reloaded sections \(changeset.groupsUpdated.sorted(by: >))")
-            collectionView.reloadSections(IndexSet(changeset.groupsUpdated))
-
             debugLog("Inserting items \(changeset.elementsInserted.sorted(by: <))")
             collectionView.insertItems(at: Array(changeset.elementsInserted))
 
-            debugLog("Reloading items \(changeset.elementsUpdated.sorted(by: <))")
-            collectionView.reloadItems(at: Array(changeset.elementsUpdated))
+            elementsUpdated = changeset.elementsUpdated
 
             changeset.elementsMoved.forEach { move in
                 debugLog("Moving \(move.from) to \(move.to)")
@@ -405,6 +407,18 @@ extension CollectionCoordinator: SectionProviderMappingDelegate {
         }, completion: { [weak self] isFinished in
             self?.debugLog("Batch updates completed. isFinished: \(isFinished)")
         })
+
+        if let elementsUpdated, !elementsUpdated.isEmpty {
+            debugLog("Need to perform a second `performBatchUpdates` to apply reloads")
+            collectionView.performBatchUpdates({
+                debugLog("Reloading items \(elementsUpdated.sorted(by: <))")
+                collectionView.reloadItems(at: Array(elementsUpdated))
+
+                debugLog("Item reload updates have been applied")
+            }, completion: { [weak self] isFinished in
+                self?.debugLog("Item reload batch updates completed. isFinished: \(isFinished)")
+            })
+        }
         isPerformingUpdates = false
         debugLog("`performBatchUpdates` call has completed")
     }

Sources/ComposedUI/CollectionView/Debugging.md

@@ -80,7 +80,6 @@ func testInsertThenRemoveGroups() {
             XCTAssertTrue(changeset.elementsInserted.isEmpty)
             XCTAssertTrue(changeset.elementsUpdated.isEmpty)
             XCTAssertTrue(changeset.groupsRemoved.isEmpty)
-            XCTAssertTrue(changeset.groupsUpdated.isEmpty)
             XCTAssertEqual(
                 changeset.groupsInserted,
                 [
@@ -106,7 +105,6 @@ func testInsertThenRemoveGroups() {
             XCTAssertTrue(changeset.elementsUpdated.isEmpty)
             XCTAssertTrue(changeset.groupsInserted.isEmpty)
             XCTAssertTrue(changeset.groupsRemoved.isEmpty)
-            XCTAssertTrue(changeset.groupsUpdated.isEmpty)
         })
 }
 ```

Sources/ComposedUI/Common/ChangesReducer.swift

@@ -70,26 +70,14 @@ internal struct ChangesReducer: CustomReflectable {
             return nil
         }
 
-        var changeset = self.changeset
-        let updatedGroups = changeset.groupsRemoved.intersection(changeset.groupsInserted)
-        updatedGroups.forEach { updatedGroup in
-            changeset.groupsRemoved.remove(updatedGroup)
-            changeset.groupsInserted.remove(updatedGroup)
-            changeset.groupsUpdated.insert(updatedGroup)
-        }
-        let updatedElements = changeset.elementsRemoved.intersection(changeset.elementsInserted)
-        updatedElements.forEach { updatedElement in
-            changeset.elementsRemoved.remove(updatedElement)
-            changeset.elementsInserted.remove(updatedElement)
-            changeset.elementsUpdated.insert(updatedElement)
-        }
+        let changeset = self.changeset
         self.changeset = Changeset()
         return changeset
     }
 
     internal mutating func insertGroups(_ groups: IndexSet) {
         groups.forEach { insertedGroup in
-            let insertedGroup = insertedGroup + changeset.groupsUpdated.filter { $0 >= insertedGroup }.count
+            let insertedGroup = insertedGroup
 
             changeset.groupsInserted = Set(changeset.groupsInserted.map { existingInsertedGroup in
                 if existingInsertedGroup >= insertedGroup {
@@ -99,11 +87,7 @@ internal struct ChangesReducer: CustomReflectable {
                 return existingInsertedGroup
             })
 
-            if changeset.groupsRemoved.remove(insertedGroup) != nil {
-                changeset.groupsUpdated.insert(insertedGroup)
-            } else {
-                changeset.groupsInserted.insert(insertedGroup)
-            }
+            changeset.groupsInserted.insert(insertedGroup)
 
             changeset.elementsInserted = Set(changeset.elementsInserted.map { insertedIndexPath in
                 var insertedIndexPath = insertedIndexPath
@@ -115,6 +99,16 @@ internal struct ChangesReducer: CustomReflectable {
                 return insertedIndexPath
             })
 
+            changeset.elementsUpdated = Set(changeset.elementsUpdated.map { updatedIndexPath in
+                var updatedIndexPath = updatedIndexPath
+
+                if updatedIndexPath.section >= insertedGroup {
+                    updatedIndexPath.section += 1
+                }
+
+                return updatedIndexPath
+            })
+
             changeset.elementsMoved = Set(changeset.elementsMoved.map { move in
                 var move = move
 
@@ -137,20 +131,29 @@ internal struct ChangesReducer: CustomReflectable {
 
     internal mutating func removeGroups(_ groups: IndexSet) {
         groups.sorted(by: >).forEach { removedGroup in
-            var removedGroup = removedGroup
-            let groupsInsertedBefore = changeset.groupsInserted.filter { $0 < removedGroup }.count
+            changeset.elementsUpdated = Set(changeset.elementsUpdated.compactMap { updatedIndexPath in
+                guard updatedIndexPath.section != removedGroup else { return nil }
+
+                var updatedIndexPath = updatedIndexPath
 
-            if changeset.groupsInserted.remove(removedGroup) != nil || changeset.groupsUpdated.remove(removedGroup) != nil {
+                if updatedIndexPath.section > removedGroup {
+                    updatedIndexPath.section -= 1
+                }
+
+                return updatedIndexPath
+            })
+
+            if changeset.groupsInserted.remove(removedGroup) != nil {
                 changeset.groupsInserted = Set(changeset.groupsInserted.map { insertedGroup in
                     if insertedGroup > removedGroup {
                         return insertedGroup - 1
                     }
 
                     return insertedGroup
                 })
-            } else if changeset.groupsInserted.remove(removedGroup - groupsInsertedBefore) != nil {
-                changeset.groupsUpdated.insert(removedGroup - groupsInsertedBefore)
             } else {
+                let transformedRemovedGroup = transformSection(removedGroup)
+
                 changeset.groupsInserted = Set(changeset.groupsInserted.map { insertedGroup in
                     if insertedGroup > removedGroup {
                         return insertedGroup - 1
@@ -159,21 +162,11 @@ internal struct ChangesReducer: CustomReflectable {
                     return insertedGroup
                 })
 
-                let availableSpaces = (0..<Int.max)
-                    .lazy
-                    .filter { [groupsRemoved = changeset.groupsRemoved] in
-                        return !groupsRemoved.contains($0)
-                    }
-                let availableSpaceIndex = availableSpaces.index(availableSpaces.startIndex, offsetBy: removedGroup)
-                removedGroup = availableSpaces[availableSpaceIndex]
-
-                changeset.groupsRemoved.insert(removedGroup)
+                changeset.groupsRemoved.insert(transformedRemovedGroup)
             }
 
             changeset.elementsRemoved = Set(changeset.elementsRemoved.filter { $0.section != removedGroup })
 
-            changeset.elementsUpdated = Set(changeset.elementsUpdated.filter { $0.section != removedGroup })
-
             changeset.elementsInserted = Set(changeset.elementsInserted.compactMap { insertedIndexPath in
                 guard insertedIndexPath.section != removedGroup else { return nil }
 
@@ -206,7 +199,6 @@ internal struct ChangesReducer: CustomReflectable {
 
     internal mutating func insertElements(at indexPaths: [IndexPath]) {
         indexPaths.forEach { insertedIndexPath in
-            guard !changeset.groupsUpdated.contains(insertedIndexPath.section) else { return }
             guard !changeset.groupsInserted.contains(insertedIndexPath.section) else { return }
 
             changeset.elementsInserted = Set(changeset.elementsInserted.map { existingInsertedIndexPath in
@@ -224,6 +216,21 @@ internal struct ChangesReducer: CustomReflectable {
                 return existingInsertedIndexPath
             })
 
+            changeset.elementsUpdated = Set(changeset.elementsUpdated.map { existingUpdatedIndexPath in
+                guard existingUpdatedIndexPath.section == insertedIndexPath.section else {
+                    // Different section; don't modify
+                    return existingUpdatedIndexPath
+                }
+
+                var existingUpdatedIndexPath = existingUpdatedIndexPath
+
+                if existingUpdatedIndexPath.item >= insertedIndexPath.item {
+                    existingUpdatedIndexPath.item += 1
+                }
+
+                return existingUpdatedIndexPath
+            })
+
             changeset.elementsInserted.insert(insertedIndexPath)
         }
     }
@@ -236,54 +243,56 @@ internal struct ChangesReducer: CustomReflectable {
             let originalRemovedIndexPath = removedIndexPath
             let removedIndexPath = transformIndexPath(removedIndexPath)
 
-            guard !changeset.groupsInserted.contains(removedIndexPath.section), !changeset.groupsUpdated.contains(removedIndexPath.section) else { return }
+            guard !changeset.groupsInserted.contains(removedIndexPath.section) else { return }
 
-            let isInInserted = changeset.elementsInserted.contains(removedIndexPath)
             let originalWasInInserted = changeset.elementsInserted.contains(originalRemovedIndexPath)
 
-            defer {
-                if !isInInserted {
-                    changeset.elementsRemoved.insert(removedIndexPath)
-                } else if removedIndexPath.item != originalRemovedIndexPath.item, !originalWasInInserted {
-                    changeset.elementsUpdated.insert(removedIndexPath)
-                }
+            if !originalWasInInserted {
+                changeset.elementsRemoved.insert(removedIndexPath)
             }
 
-            changeset.elementsUpdated = Set(changeset.elementsUpdated.filter { existingUpdatedIndexPath in
-                return existingUpdatedIndexPath != originalRemovedIndexPath
-            })
-
             changeset.elementsInserted = Set(changeset.elementsInserted.compactMap { existingInsertedIndexPath in
-                guard existingInsertedIndexPath.section == removedIndexPath.section else {
+                guard existingInsertedIndexPath.section == originalRemovedIndexPath.section else {
                     // Different section; don't modify
                     return existingInsertedIndexPath
                 }
 
                 var existingInsertedIndexPath = existingInsertedIndexPath
 
-                if existingInsertedIndexPath.item > removedIndexPath.item/*, !changeset.elementsRemoved.contains(existingInsertedIndexPath)*/
-                {
+                if existingInsertedIndexPath.item > originalRemovedIndexPath.item {
                     existingInsertedIndexPath.item -= 1
-                } else if existingInsertedIndexPath.item == removedIndexPath.item {
+                } else if existingInsertedIndexPath.item == originalRemovedIndexPath.item {
                     return nil
                 }
 
                 return existingInsertedIndexPath
             })
+
+            changeset.elementsUpdated = Set(changeset.elementsUpdated.compactMap { existingUpdatedIndexPath in
+                guard existingUpdatedIndexPath.section == originalRemovedIndexPath.section else {
+                    // Different section; don't modify
+                    return existingUpdatedIndexPath
+                }
+
+                var existingUpdatedIndexPath = existingUpdatedIndexPath
+
+                if existingUpdatedIndexPath.item > originalRemovedIndexPath.item {
+                    existingUpdatedIndexPath.item -= 1
+                } else if existingUpdatedIndexPath.item == originalRemovedIndexPath.item {
+                    return nil
+                }
+
+                return existingUpdatedIndexPath
+            })
         }
     }
 
     internal mutating func updateElements(at indexPaths: [IndexPath]) {
         indexPaths.sorted(by: { $0.item > $1.item }).forEach { updatedElement in
             guard !changeset.elementsInserted.contains(updatedElement) else { return }
+            guard !changeset.groupsInserted.contains(updatedElement.section) else { return }
 
-            let updatedElement = transformIndexPath(updatedElement)
-
-            if !changeset.groupsInserted.contains(updatedElement.section),
-               !changeset.groupsUpdated.contains(updatedElement.section)
-            {
-                changeset.elementsUpdated.insert(updatedElement)
-            }
+            changeset.elementsUpdated.insert(updatedElement)
         }
     }
 
@@ -314,7 +323,8 @@ internal struct ChangesReducer: CustomReflectable {
         let groupsInserted = changeset.groupsInserted
         let availableSpaces = (0..<Int.max)
             .lazy
-            .filter { !groupsRemoved.contains($0) || groupsInserted.contains($0) }
+            .filter { !groupsRemoved.contains($0) }
+        let section = section - groupsInserted.filter { $0 < section }.count
         let availableSpaceIndex = availableSpaces.index(availableSpaces.startIndex, offsetBy: section)
 
         return availableSpaces[availableSpaceIndex]
@@ -327,15 +337,8 @@ internal struct ChangesReducer: CustomReflectable {
     /// - Parameter section: The section index to the item belongs to.
     /// - Returns: The transformed item index.
     private func transformItem(_ item: Int, inSection section: Int) -> Int {
-        /// This is a collection of all the items in the current section that
-        /// will be coalesced in to a reload, but are not yet in the `elementsReloaded`.
-        let itemsReloaded = changeset.elementsRemoved
-            .intersection(changeset.elementsInserted)
-            .filter({ $0.section == section })
-            .map(\.item)
-
         func isIncluded(indexPath: IndexPath) -> Bool {
-            indexPath.section == section && !itemsReloaded.contains(indexPath.item)
+            indexPath.section == section
         }
 
         let itemsRemoved = changeset.elementsRemoved.filter(isIncluded(indexPath:)).map(\.item)

Sources/ComposedUI/Common/Changeset.swift

@@ -16,9 +16,14 @@ public struct Changeset {
 
     public var groupsInserted: Set<Int> = []
     public var groupsRemoved: Set<Int> = []
-    public var groupsUpdated: Set<Int> = []
     public var elementsRemoved: Set<IndexPath> = []
     public var elementsInserted: Set<IndexPath> = []
     public var elementsMoved: Set<Move> = []
+
+    /// The elements that have been updated. When applied in a batch of updates to a collection view
+    /// these would use the index paths from _before_ the updates are applied, however these will
+    /// use the index paths _after_ the updates are applied; UICollectionView has a bug that can
+    /// cause updates to not be applied when applied in the same batch as deletions and Composed
+    /// works around this by applying updates in a second batch.
     public var elementsUpdated: Set<IndexPath> = []
 }

Sources/ComposedUI/TableView/TableCoordinator.swift

@@ -198,8 +198,6 @@ extension TableCoordinator: SectionProviderMappingDelegate {
 
             tableView.deleteRows(at: Array(changeset.elementsRemoved), with: .automatic)
 
-            tableView.reloadSections(IndexSet(changeset.groupsUpdated), with: .automatic)
-
             tableView.insertRows(at: Array(changeset.elementsInserted), with: .automatic)
 
             tableView.reloadRows(at: Array(changeset.elementsUpdated), with: .automatic)