Merge pull request #308 from kyleve/kve/fix-checkout-reorder-crashes

Fix Reordering Crash

Demo/Sources/Demos/Demo Screens/CollectionViewAppearance.swift

@@ -188,15 +188,15 @@ struct Toggle : Element {
         {
             super.init(frame: frame)
 
-            self.addTarget(self, action: #selector(toggled), for: .valueChanged)
+            self.addTarget(self, action: #selector(didToggleValue), for: .valueChanged)
         }
 
         @available(*, unavailable)
         required init?(coder: NSCoder) {
             fatalError()
         }
 
-        @objc func toggled()
+        @objc func didToggleValue()
         {
             self.onToggle(self.isOn)
         }

Demo/Sources/Demos/Demo Screens/PaymentTypesViewController.swift

@@ -16,6 +16,7 @@ final class PaymentTypesViewController : ListViewController {
 
         list.layout = .table { table in
             table.layout.interSectionSpacingWithNoFooter = 10.0
+            table.layout.padding.bottom = 100
         }
 
         let types = self.types
@@ -48,9 +49,15 @@ final class PaymentTypesViewController : ListViewController {
 
             section.header = HeaderFooter(PaymentTypeHeader(title: SectionID.disabled.title))
 
+            section.reordering.minItemCount = 0
+
             section += types.filter { $0.isEnabled == false }
             .sorted { $0.sortOrder < $1.sortOrder }
             .map(makeItem(with:))
+
+            if section.items.isEmpty {
+                section += EmptyRow()
+            }
         }
     }
 
@@ -131,6 +138,25 @@ fileprivate struct PaymentTypeHeader : BlueprintHeaderFooterContent, Equatable {
         }
         .inset(uniform: 15.0)
     }
+
+    var background: Element? {
+        Box(backgroundColor: .white)
+    }
+}
+
+fileprivate struct EmptyRow : BlueprintItemContent, Equatable {
+
+    var identifierValue: String {
+        ""
+    }
+
+    func element(with info: ApplyItemContentInfo) -> Element {
+        Label(text: "No Contents") {
+            $0.font = .systemFont(ofSize: 16.0, weight: .semibold)
+            $0.color = .lightGray
+        }
+        .inset(uniform: 15.0)
+    }
 }
 
 fileprivate struct PaymentTypeRow : BlueprintItemContent {

ListableUI/Sources/Layout/CollectionViewLayout.swift

@@ -161,12 +161,8 @@ final class CollectionViewLayout : UICollectionViewLayout
         let view = self.collectionView!
         let context = context as! InvalidationContext
 
-        super.invalidateLayout(with: context)
-
         // Handle Moved Items
-
-        self.isReordering = context.interactiveMoveAction != nil
-
+
         if let action = context.interactiveMoveAction {
 
             switch action {
@@ -176,13 +172,16 @@ final class CollectionViewLayout : UICollectionViewLayout
                 }
 
             case .complete(_):
-                break
+                self.sendEndQueuingEditsAfterDelay()
 
             case .cancelled(let info):
                 self.layout.content.move(from: info.from, to: info.to)
+                self.sendEndQueuingEditsAfterDelay()
             }
         }
 
+        super.invalidateLayout(with: context)
+
         // Handle View Width Changing
 
         context.viewPropertiesChanged = self.viewProperties != CollectionViewLayoutProperties(collectionView: view)
@@ -192,13 +191,44 @@ final class CollectionViewLayout : UICollectionViewLayout
         self.neededLayoutType.merge(with: context)
     }
 
+    private func sendEndQueuingEditsAfterDelay() {
+
+        ///
+        /// Hello! Welcome to the source code. You're probably wondering why this perform after runloop hack is here.
+        ///
+        /// Well, it is because `UICollectionView` does not play well with removals that occur synchronously
+        /// as a result of a reorder being messaged.
+        ///
+        /// Please, consider the following:
+        ///
+        /// 1) A user begins dragging an item.
+        /// 2) They drop the item at the last point in the list; (2,1). The collection view records this index path (2,1).
+        /// 3) Via `collectionView(_:moveItemAt:to:)`, we notify the observer(s) of the change.
+        /// 4) Synchronously via that notification, they remove the item at (2,0), moving the item now at (2,1) to (2,0).
+        ///
+        /// Unfortunately, this causes `super.invalidateLayout(with: context)` to then fail with an invalid
+        /// index path; because it seems to take one runloop to let the reorder "settle" through the collection view –
+        /// most notably, the `context.targetIndexPathsForInteractivelyMovingItems` contains an
+        /// invalid index path – the item which was previously at (2,1) is still there, when it should now be at (2,0).
+        ///
+        /// So thus, we queue updates a runloop to let the collection view figure its internal state out before we begin
+        /// processing any further updates 🥴.
+        /// 
+
+        OperationQueue.main.addOperation {
+            self.delegate.listViewShouldEndQueueingEditsForReorder()
+        }
+    }
+
     override func invalidationContext(
         forInteractivelyMovingItems targetIndexPaths: [IndexPath],
         withTargetPosition targetPosition: CGPoint,
         previousIndexPaths: [IndexPath],
         previousPosition: CGPoint
     ) -> UICollectionViewLayoutInvalidationContext
     {
+        self.isReordering = true
+
         let context = super.invalidationContext(
             forInteractivelyMovingItems: targetIndexPaths,
             withTargetPosition: targetPosition,
@@ -224,6 +254,8 @@ final class CollectionViewLayout : UICollectionViewLayout
         movementCancelled: Bool
     ) -> UICollectionViewLayoutInvalidationContext
     {
+        self.isReordering = false
+
         let context = super.invalidationContextForEndingInteractiveMovementOfItems(
             toFinalIndexPaths: indexPaths,
             previousIndexPaths: previousIndexPaths,
@@ -361,9 +393,12 @@ final class CollectionViewLayout : UICollectionViewLayout
             let shouldLayout = size.isEmpty == false
 
             switch self.neededLayoutType {
-            case .none: return true
-            case .relayout: self.performLayout()
-            case .rebuild: self.performRebuild(andLayout: shouldLayout)
+            case .none:
+                return true
+            case .relayout:
+                self.performLayout()
+            case .rebuild:
+                self.performRebuild(andLayout: shouldLayout)
             }
 
             return true
@@ -641,6 +676,8 @@ public protocol CollectionViewLayoutDelegate : AnyObject
     ) -> ListLayoutContent
 
     func listViewLayoutDidLayoutContents()
+
+    func listViewShouldEndQueueingEditsForReorder()
 }
 
 

ListableUI/Sources/ListView/ListChangesQueue.swift

@@ -0,0 +1,99 @@
+//
+//  ListChangesQueue.swift
+//  ListableUI
+//
+//  Created by Kyle Van Essen on 7/19/21.
+//
+
+import Foundation
+
+
+/// Used to queue updates into a list view.
+/// Note: This type is only safe to use from the main thread.
+final class ListChangesQueue {
+
+    /// Adds a synchronous block to the queue, marked as done once the block exits.
+    func add(_ block : @escaping () -> ()) {
+        self.waiting.append(.init(block))
+        self.runIfNeeded()
+    }
+
+    /// Set by consumers to enable and disable queueing during a reorder event.
+    var isQueuingForReorderEvent : Bool = false {
+        didSet {
+            self.runIfNeeded()
+        }
+    }
+
+    /// Prevents processing other events in the queue.
+    ///
+    /// Note: Right now this just checks `isQueuingForReorderEvent`, but may check more props in the future.
+    var isPaused : Bool {
+        self.isQueuingForReorderEvent
+    }
+
+    /// Operations waiting to execute.
+    private(set) var waiting : [Operation] = []
+
+    /// Invoked to continue processing queue events.
+    private func runIfNeeded() {
+        precondition(Thread.isMainThread)
+
+        /// Nothing to do if we're currently paused!
+        guard self.isPaused == false else {
+            return
+        }
+
+        while let next = self.waiting.popFirst() {
+            autoreleasepool {
+                guard case .new(let new) = next.state else {
+                    fatalError("State of enqueued operation was wrong")
+                }
+
+                /// Ok, we have a runnable operation; let's run it.
+
+                next.state = .done
+
+                new.body()
+            }
+        }
+    }
+}
+
+
+extension ListChangesQueue {
+
+    final class Operation {
+
+        fileprivate(set) var state : State
+
+        init(_ body : @escaping () -> ()) {
+            self.state = .new(.init(body: body))
+        }
+
+        enum State {
+            case new(New)
+            case done
+
+            struct New {
+                let body : () -> ()
+            }
+        }
+    }
+}
+
+
+fileprivate extension Array {
+
+    mutating func popFirst() -> Element? {
+        guard self.isEmpty == false else {
+            return nil
+        }
+
+        let first = self[0]
+
+        self.remove(at: 0)
+
+        return first
+    }
+}

ListableUI/Sources/ListView/ListView.DataSource.swift

@@ -97,6 +97,16 @@ internal extension ListView
                 return
             }
 
+            ///
+            /// Mark us as queuing for re-orders, to prevent destructive edits which could break the collection
+            /// view's layout while the re-order event settles.
+            ///
+            /// Later on, the call to `listViewShouldEndQueueingEditsForReorder` will set this value to false.
+            ///
+            /// See `sendEndQueuingEditsAfterDelay` for a more in-depth explanation.
+            ///
+            self.view.updateQueue.isQueuingForReorderEvent = true
+
             /// Perform the change in our data source.
 
             self.storage.moveItem(from: from, to: to)

ListableUI/Sources/ListView/ListView.Delegate.swift

@@ -261,10 +261,13 @@ extension ListView
             )
         }
 
-        func listViewLayoutDidLayoutContents()
-        {
+        func listViewLayoutDidLayoutContents() {
             self.view.visibleContent.update(with: self.view)
         }
+
+        func listViewShouldEndQueueingEditsForReorder() {
+            self.view.updateQueue.isQueuingForReorderEvent = false
+        }
 
         // MARK: UIScrollViewDelegate
 

ListableUI/Sources/ListView/ListView.swift

@@ -60,9 +60,9 @@ public final class ListView : UIView, KeyboardObserverDelegate
         self.stateObserver = ListStateObserver()
 
         self.collectionView.isPrefetchingEnabled = false
-
-        self.collectionView.dataSource = self.dataSource
+
         self.collectionView.delegate = self.delegate
+        self.collectionView.dataSource = self.dataSource
 
         // Super init.
 
@@ -684,25 +684,36 @@ public final class ListView : UIView, KeyboardObserverDelegate
         self.configure(with: description)
     }
 
+    let updateQueue = ListChangesQueue()
+
     public func configure(with properties : ListProperties)
     {
-        let animated = properties.animatesChanges
-
-        self.appearance = properties.appearance
-        self.behavior = properties.behavior
-        self.autoScrollAction = properties.autoScrollAction
-        self.scrollIndicatorInsets = properties.scrollIndicatorInsets
-        self.collectionView.accessibilityIdentifier = properties.accessibilityIdentifier
-        self.debuggingIdentifier = properties.debuggingIdentifier
-        self.actions = properties.actions
-
-        self.stateObserver = properties.stateObserver
-
-        self.environment = properties.environment
-
-        self.set(layout: properties.layout, animated: animated)
+        /// We enqueue these changes into the update queue to ensure they are not applied
+        /// before it is safe to do so. Currently, "safe" means "during the application of a reorder".
+        ///
+        /// See `CollectionViewLayout.sendEndQueuingEditsAfterDelay()` for more.
 
-        self.setContent(animated: animated, properties.content)
+        self.updateQueue.add { [weak self] in
+            guard let self = self else { return }
+
+            let animated = properties.animatesChanges
+
+            self.appearance = properties.appearance
+            self.behavior = properties.behavior
+            self.autoScrollAction = properties.autoScrollAction
+            self.scrollIndicatorInsets = properties.scrollIndicatorInsets
+            self.collectionView.accessibilityIdentifier = properties.accessibilityIdentifier
+            self.debuggingIdentifier = properties.debuggingIdentifier
+            self.actions = properties.actions
+
+            self.stateObserver = properties.stateObserver
+
+            self.environment = properties.environment
+
+            self.set(layout: properties.layout, animated: animated)
+
+            self.setContent(animated: animated, properties.content)
+        }
     }
 
     private func setContentFromSource(animated : Bool = false)