Fix ForEach Collection change rendered View correctness #245
Conversation
|
Edit: Fixed oh, apparently I mixed some of my branches… there are changes that not belong to this pr… I’m going to try and remove them... |
…ntifiable No non-identifiable support in this commit
StressTestExample is broken
…leanup sadly an argument name seems to be required on menuitem forEach initializer, the compiler is apparently unable to infer the right child from the context.
…ntifiable No non-identifiable support in this commit # Conflicts: # Examples/Bundler.toml # Examples/Package.swift # Examples/Sources/ForEachExample/ForEachApp.swift # Sources/SwiftCrossUI/Views/ForEach.swift
StressTestExample is broken
MiaKoring
force-pushed
the
fix/foreach-removals
branch
from
996b537 to
743a488
Compare
stackotter
left a comment
stackotter
left a comment
There was a problem hiding this comment.
I'll review ForEach once I get the time. For now I've just reviewed all of the other files changed by the PR. I unfortunately ran out of time
| @State var items = { | ||
| var items = [Item]() | ||
| for i in 0..<20 { | ||
| items.append(.init("\(i)")) | ||
| } | ||
| return items | ||
| }() |
There was a problem hiding this comment.
Replace with
@State var items = (0..<20).map { Item("\($0)") }| guard insertionPosition > items.count - 1 else { | ||
| return | ||
| } | ||
| insertionPosition = max(items.count - 1, 0) |
There was a problem hiding this comment.
It feels odd having a guard to protect the 'bad' path. I think it should be
let upperLimit = max(items.count - 1, 0)
insertionPosition = min(insertionPosition, upperLimit)If you want you can add an if statement to only set the insertionPosition if it has changed, but I don't believe that that will be necessary for performance at all once we do dependency analysis stuff.
|
|
||
| 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: { |
There was a problem hiding this comment.
I think it'd be best to have the ForEach iterate over items.enumerated so that each ItemRow can know its own index and avoid this linear operation in what could be a constant-time moveUp implementation (same for moveDown). I'm pointing this out cause I can see ForEachExample being used to stress test SwiftCrossUI's performance, in which case these move functions should be implemented with best practice.
| item: item, isFirst: Optional(item.id) == items.first?.id, | ||
| isLast: Optional(item.id) == items.last?.id | ||
| ) { | ||
| items.removeAll(where: { $0.id == item.id }) |
There was a problem hiding this comment.
The comment I left on moveUp applies here too. We can make this a little more efficient by directly removing at an index (although the gain won't be as big as for moveUp and moveDown.
| } | ||
|
|
||
| struct ItemRow: View { | ||
| @State var item: Item |
There was a problem hiding this comment.
Item shouldn't be State here cause its true source of truth is from the external array. Can we make the Item class into a struct instead of an observable object?
|
|
||
| ScrollView { | ||
| ForEach(greetings.reversed()[1...]) { greeting in | ||
| ForEach(items: greetings.reversed()[1...]) { greeting in |
There was a problem hiding this comment.
Fix this up to use the new non-deprecated syntax.
| urlInput = url.absoluteString | ||
| } | ||
| #if !os(tvOS) | ||
| WebView($url) |
There was a problem hiding this comment.
Add an #else with Text("WebView isn't supported on tvOS").
| ), | ||
| .package( | ||
| url: "https://github.com/apple/swift-collections.git", | ||
| exact: "1.2.1" |
There was a problem hiding this comment.
Is there a reason that this has to be exact and not from? We should avoid exact dependencies because then it's harder to e.g. use swift-collections and swift-cross-ui in the same project.
2 participants
This pr fixes the issue of only the last items getting removed from the rendered content, even if they werent the ones changing and item insertion/reordering correctness like mentioned in #243
I added apple/swift-collections as a dependency, I’m using OrderedSet in the changes.
If a duplicate identifier is detected every node gets replaced with a new one, diffing is not possible.
For unique identifiers:
If it was included in the previous update, the node gets reused.
It checks if there is an identifier it doesn’t know from the previous update anywhere before it, if there is, ongoing every existing node gets removed from the view graph and reinserted at its new place.
if it wasn’t included, a new node gets created and ongoing every existing node gets readded.
if its the first update every node gets added to the view graph
if there was no duplicate the nodes included in the previous update not included in the current one get removed from the view graph
if there were duplicate(s) every node gets replaced
Also I added some new initializers:
for Identifiable, setting the identifier key path to .id if not specified otherwise
for existing identifier a new one allowing customization of the id KeyPath
Sadly this PR needs to be a breaking change. I was forced to add labels to the elements property on some existing initializers. Otherwise the Swift Compiler wouldn’t select the right initializer (or even select one). disfavouredOverload didn’t help. While updating you can decide between adding the label (if needed) or a keyPath. Choosing the label option uses the same update method as before. With adding a keyPath for the Identifier you choose the new, improved update method. ForEach with Range or [Identifiable] automatically recieve the new method.
I added a new Example App, ForEachExample, showcasing deletion, insertion, appending and reordering.
I tested iOS, macOS, macCatalyst, GtkBackend on Linux and WinUIBackend on Windows successfully.
While it works with non-unique Identifiers I strongly recommend using unique and constant Identifiers. It should be considerably more performant due to it making as few as possible operations on the view graph.
In my tests it still was consistently about 11% faster than the previous implementation.
As soon as the reason for a view update is available ForEach’s update method should be optimized to do only whats necessary. For Example the whole diffing, reordering,… is obsolete when only the size changed. For now at least the correctness got fixed and performance at least slightly improved.