Fix ForEach Collection change rendered View correctness #245
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Author
|
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.