Implement copy-on-write collection for LogicalChildren/VisualChildren #20028
Conversation
Changed Visual/Logical Children properties to use CopyOnWrite
MrJul
added
enhancement
backport-candidate-11.3.x
|
|
||
| for (var i = 0; i < visualChildrenCount; i++) | ||
|
|
||
| foreach (var child in VisualChildren) |
There was a problem hiding this comment.
You haven't replaced VisualChildren with the new list type, is that an oversight?
| { | ||
| internal class SafeEnumerableList<T> : IAvaloniaList<T>, INotifyCollectionChanged | ||
| { | ||
| private AvaloniaList<T> _list = new(); |
There was a problem hiding this comment.
I'm a bit worried about the allocations here. We have this inner list, plus the two event handlers in the constructor, or 3 extra allocations. I wouldn't worry about such a number in most other classes, but this is used in every visual, twice. That's 6 long-lived objects per visual.
|
|
||
| public void AddRange(IEnumerable<T> items) | ||
| { | ||
| ((IAvaloniaList<T>)GetList()).AddRange(items); |
There was a problem hiding this comment.
Here and below: the cast seems unnecessary.
| return GetEnumerator(); | ||
| } | ||
|
|
||
| private AvaloniaList<T> GetList() |
There was a problem hiding this comment.
Nit: rename to GetWriteableList (or something along those lines) to make the difference clearer.
| } | ||
| }; | ||
| }); | ||
| /*IDataTemplate windowTemplate = new FuncDataTemplate<object?>((i, ns) => |
There was a problem hiding this comment.
Remove all the commented code in this file.
|
|
||
| namespace Avalonia.Base.UnitTests.Collections | ||
| { | ||
| public class SafeEnumerateCollectionTests |
There was a problem hiding this comment.
While it's neat to have a test corresponding to real issue, a handful of tests for the SafeEnumerableList itself would be nice, i.e. checking that adding or removing an item doesn't affect enumerations that are already in progress.
| { | ||
| private readonly SafeEnumerableList<T> _owner; | ||
| private readonly int _generation; | ||
| private readonly IEnumerator<T> _enumerator; |
There was a problem hiding this comment.
This should be typed as AvaloniaList<T>.Enumerator to avoid boxing. (Make it non-readonly in that case.)
|
|
||
| object IEnumerator.Current => _enumerator.Current!; | ||
|
|
||
| public void Reset() => throw new InvalidOperationException(); |
There was a problem hiding this comment.
It's unlikely to be used, but why is Reset not implemented? The underlying list cannot change so it should be safe to call _enumerator.Reset().
MrJul
changed the title
2 participants
What does the pull request do?
Fixes #14437 & #13497
What is the updated/expected behavior with this PR?
ControlCatalog doesn't fail anymore using scenario from #13497
How was the solution implemented (if it's not obvious)?
implemented new SafeEnumerableList class using "Copy-on-change" behavior and LogicalChildren/VisualChildren collections are changed to be instanced using class
Checklist
Fixed issues
Fixes #14437
Fixes #13497