Fixed reordering, insertion and removal in ForEach where Element: Identifiable

MiaKoring · MiaKoring · commit e9b94a860bfa · 2025-11-06T18:49:18.000+01:00
No non-identifiable support in this commit
diff --git a/Examples/Bundler.toml b/Examples/Bundler.toml
@@ -64,3 +64,8 @@ version = '0.1.0'
 identifier = 'dev.swiftcrossui.HoverExample'
 product = 'HoverExample'
 version = '0.1.0'
+
+[apps.ForEachExample]
+identifier = 'dev.swiftcrossui.ForEachExample'
+product = 'ForEachExample'
+version = '0.1.0'
diff --git a/Examples/Package.resolved b/Examples/Package.resolved
diff --git a/Examples/Package.swift b/Examples/Package.swift
@@ -75,6 +75,10 @@ let package = Package(
         ),
         .executableTarget(
             name: "HoverExample",
+            dependencies: exampleDependencies
+		),
+		.executableTarget(
+            name: "ForEachExample",
             dependencies: exampleDependencies
         )
     ]
diff --git a/Examples/Sources/ForEachExample/ForEachApp.swift b/Examples/Sources/ForEachExample/ForEachApp.swift
@@ -0,0 +1,102 @@
+import DefaultBackend
+import Foundation
+import SwiftCrossUI
+
+#if canImport(SwiftBundlerRuntime)
+    import SwiftBundlerRuntime
+#endif
+
+@main
+@HotReloadable
+struct ForEachApp: App {
+    @State var items = {
+        var items = [Item]()
+        for i in 0..<20 {
+            items.append(.init("\(i)"))
+        }
+        return items
+    }()
+    @State var biggestValue = 19
+    @State var insertionPosition = 10
+
+    var body: some Scene {
+        WindowGroup("ForEach") {
+            #hotReloadable {
+                ScrollView {
+                    VStack {
+                        Text("Items")
+
+                        Button("Append") {
+                            biggestValue += 1
+                            items.append(.init("\(biggestValue)"))
+                        }
+
+                        Button("Insert in front of current item at position \(insertionPosition)") {
+                            biggestValue += 1
+                            items.insert(.init("\(biggestValue)"), at: insertionPosition)
+                        }
+                        Slider($insertionPosition, minimum: 0, maximum: items.count - 1)
+                            .onChange(of: items.count) {
+                                guard insertionPosition > items.count - 1 else {
+                                    return
+                                }
+                                insertionPosition = max(items.count - 1, 0)
+                            }
+
+                        ForEach(items) { item in
+                            ItemRow(
+                                item: item, isFirst: Optional(item.id) == items.first?.id,
+                                isLast: Optional(item.id) == items.last?.id
+                            ) {
+                                items.removeAll(where: { $0.id == item.id })
+                            } moveUp: {
+                                guard
+                                    let ownIndex = items.firstIndex(where: { $0.id == item.id }),
+                                    ownIndex != items.startIndex
+                                else { return }
+                                items.swapAt(ownIndex, ownIndex - 1)
+                            } moveDown: {
+                                guard
+                                    let ownIndex = items.firstIndex(where: { $0.id == item.id }),
+                                    ownIndex != items.endIndex
+                                else { return }
+                                items.swapAt(ownIndex, ownIndex + 1)
+                            }
+                        }
+                    }
+                    .padding(10)
+                }
+            }
+        }
+        .defaultSize(width: 400, height: 800)
+    }
+}
+
+struct ItemRow: View {
+    @State var item: Item
+    let isFirst: Bool
+    let isLast: Bool
+    var remove: () -> Void
+    var moveUp: () -> Void
+    var moveDown: () -> Void
+
+    var body: some View {
+        HStack {
+            Text(item.value)
+            Button("Delete") { remove() }
+            Button("⌃") { moveUp() }
+                .disabled(isFirst)
+            Button("⌄") { moveDown() }
+                .disabled(isLast)
+        }
+    }
+}
+
+class Item: Identifiable, SwiftCrossUI.ObservableObject {
+    let id = UUID()
+    @SwiftCrossUI.Published var value: String
+
+    init(_ value: String) {
+        self.value = value
+    }
+}
diff --git a/Package.resolved b/Package.resolved
diff --git a/Package.swift b/Package.swift
@@ -110,6 +110,10 @@ let package = Package(
             url: "https://github.com/stackotter/swift-winui",
             revision: "1695ee3ea2b7a249f6504c7f1759e7ec7a38eb86"
         ),
+        .package(
+            url: "https://github.com/apple/swift-collections.git",
+            .upToNextMajor(from: "1.3.0")
+        ),
         // .package(
         //     url: "https://github.com/stackotter/TermKit",
         //     revision: "163afa64f1257a0c026cc83ed8bc47a5f8fc9704"
@@ -129,6 +133,7 @@ let package = Package(
             dependencies: [
                 "HotReloadingMacrosPlugin",
                 .product(name: "ImageFormats", package: "swift-image-formats"),
+                .product(name: "OrderedCollections", package: "swift-collections")
             ],
             exclude: [
                 "Builders/ViewBuilder.swift.gyb",
diff --git a/Sources/SwiftCrossUI/Views/ForEach.swift b/Sources/SwiftCrossUI/Views/ForEach.swift
@@ -1,3 +1,5 @@
+import OrderedCollections
+
 /// A view that displays a variable amount of children.
 public struct ForEach<Items: Collection, Child> where Items.Index == Int {
     /// A variable-length collection of elements to display.
@@ -40,7 +42,7 @@ extension ForEach where Items == [Int] {
     }
 }
 
-extension ForEach: TypeSafeView, View where Child: View {
+extension ForEach: TypeSafeView, View where Child: View, Items.Element: Identifiable {
     typealias Children = ForEachViewChildren<Items, Child>
 
     public var body: EmptyView {
@@ -112,20 +114,69 @@ extension ForEach: TypeSafeView, View where Child: View {
             children.queuedChanges = []
         }
 
-        // TODO: The way we're reusing nodes for technically different elements means that if
-        //   Child has state of its own then it could get pretty confused thinking that its state
-        //   changed whereas it was actually just moved to a new slot in the array. Probably not
-        //   a huge issue, but definitely something to keep an eye on.
         var layoutableChildren: [LayoutSystem.LayoutableChild] = []
-        for (i, node) in children.nodes.enumerated() {
-            guard i < elements.count else {
-                break
+
+        let oldNodes = children.nodes
+        let oldMap = children.nodeIdentifierMap
+        let oldIdentifiers = children.identifiers
+        let identifiersStart = oldIdentifiers.startIndex
+
+        children.nodes = []
+        children.nodeIdentifierMap = [:]
+        children.identifiers = []
+
+        // Once this is true, every node that existed in the previous update and
+        // still exists in the new one is reinserted to ensure that items are
+        // rendered in the correct order.
+        var requiresOngoingReinsertion = false
+
+        for (i, element) in elements.enumerated() {
+            let childContent = child(element)
+            let node: AnyViewGraphNode<Child>
+
+            if let oldNode = oldMap[element.id] {
+                node = oldNode
+
+                // Checks if there is a preceding item that was not preceding in
+                // the previous update. If such an item exists, it means that
+                // the order of the collection has changed or that an item was
+                // inserted somewhere in the middle, rather than simply appended.
+                requiresOngoingReinsertion =
+                    requiresOngoingReinsertion
+                    || {
+                        guard
+                            let ownOldIndex = oldIdentifiers.firstIndex(of: element.id)
+                        else { return false }
+
+                        let subset = oldIdentifiers[identifiersStart..<ownOldIndex]
+                        return !children.identifiers.subtracting(subset).isEmpty
+                    }()
+
+                if requiresOngoingReinsertion {
+                    removeChild(oldNode.widget.into())
+                    addChild(oldNode.widget.into())
+                }
+            } else {
+                // New Items need ongoing reinsertion to get
+                // displayed at the correct location.
+                requiresOngoingReinsertion = true
+                node = AnyViewGraphNode(
+                    for: childContent,
+                    backend: backend,
+                    environment: environment
+                )
+                addChild(node.widget.into())
             }
-            let index = elements.startIndex.advanced(by: i)
-            let childContent = child(elements[index])
+
+            children.nodeIdentifierMap[element.id] = node
+            children.identifiers.append(element.id)
+
+            children.nodes.append(node)
+
             if children.isFirstUpdate {
                 addChild(node.widget.into())
             }
+
             layoutableChildren.append(
                 LayoutSystem.LayoutableChild(
                     update: { proposedSize, environment, dryRun in
@@ -139,40 +190,13 @@ extension ForEach: TypeSafeView, View where Child: View {
                 )
             )
         }
+
         children.isFirstUpdate = false
 
-        let nodeCount = children.nodes.count
-        let remainingElementCount = elements.count - nodeCount
-        if remainingElementCount > 0 {
-            let startIndex = elements.startIndex.advanced(by: nodeCount)
-            for i in 0..<remainingElementCount {
-                let childContent = child(elements[startIndex.advanced(by: i)])
-                let node = AnyViewGraphNode(
-                    for: childContent,
-                    backend: backend,
-                    environment: environment
-                )
-                children.nodes.append(node)
-                addChild(node.widget.into())
-                layoutableChildren.append(
-                    LayoutSystem.LayoutableChild(
-                        update: { proposedSize, environment, dryRun in
-                            node.update(
-                                with: childContent,
-                                proposedSize: proposedSize,
-                                environment: environment,
-                                dryRun: dryRun
-                            )
-                        }
-                    )
-                )
-            }
-        } else if remainingElementCount < 0 {
-            let unused = -remainingElementCount
-            for i in (nodeCount - unused)..<nodeCount {
-                removeChild(children.nodes[i].widget.into())
-            }
-            children.nodes.removeLast(unused)
+        for removed in oldMap.filter({
+            !children.identifiers.contains($0.key)
+        }).values {
+            removeChild(removed.widget.into())
         }
 
         return LayoutSystem.updateStackLayout(
@@ -199,9 +223,17 @@ extension ForEach: TypeSafeView, View where Child: View {
 class ForEachViewChildren<
     Items: Collection,
     Child: View
->: ViewGraphNodeChildren where Items.Index == Int {
+>: ViewGraphNodeChildren where Items.Index == Int, Items.Element: Identifiable {
     /// The nodes for all current children of the ``ForEach`` view.
     var nodes: [AnyViewGraphNode<Child>] = []
+
+    /// The nodes for all current children of the ``ForEach`` view, queriable by their identifier.
+    var nodeIdentifierMap: [Items.Element.ID: AnyViewGraphNode<Child>]
+
+    /// The identifiers of all current children ``ForEach`` view in the order they are displayed.
+    /// Can be used for checking if an element was moved or an element was inserted in front of it.
+    var identifiers: OrderedSet<Items.Element.ID>
+
     /// Changes queued during `dryRun` updates.
     var queuedChanges: [Change] = []
 
@@ -227,18 +259,30 @@ class ForEachViewChildren<
         snapshots: [ViewGraphSnapshotter.NodeSnapshot]?,
         environment: EnvironmentValues
     ) {
-        nodes = view.elements
-            .map(view.child)
-            .enumerated()
-            .map { (index, child) in
+        var nodeIdentifierMap = [Items.Element.ID: AnyViewGraphNode<Child>]()
+        var identifiers = OrderedSet<Items.Element.ID>()
+        var viewNodes = [AnyViewGraphNode<Child>]()
+
+        for (index, element) in view.elements.enumerated() {
+            let child = view.child(element)
+            let viewGraphNode = {
                 let snapshot = index < snapshots?.count ?? 0 ? snapshots?[index] : nil
                 return ViewGraphNode(
                     for: child,
                     backend: backend,
                     snapshot: snapshot,
                     environment: environment
                 )
-            }
-            .map(AnyViewGraphNode.init(_:))
+            }()
+
+            let anyViewGraphNode = AnyViewGraphNode(viewGraphNode)
+            viewNodes.append(anyViewGraphNode)
+
+            identifiers.append(element.id)
+            nodeIdentifierMap[element.id] = anyViewGraphNode
+        }
+        nodes = viewNodes
+        self.identifiers = identifiers
+        self.nodeIdentifierMap = nodeIdentifierMap
     }
 }

Original file line number	Diff line number	Diff line change
`@@ -75,6 +75,10 @@ let package = Package(`
`75`	`75`	`),`
`76`	`76`	`.executableTarget(`
`77`	`77`	`name: "HoverExample",`
	`78`	`+ dependencies: exampleDependencies`
	`79`	`+ ),`
	`80`	`+ .executableTarget(`
	`81`	`+ name: "ForEachExample",`
`78`	`82`	`dependencies: exampleDependencies`
`79`	`83`	`)`
`80`	`84`	`]`