Skip to content

Commit e671374

Browse files
grokysmaxkatz6
authored andcommitted
Fix Some Virtualizing List Update Bugs (#13795)
* Added failing unit tests for #11666. * Handle items replaced with its own logic. Don't rely on a remove/insert - it creates more work than needed. * Fix some off-by-N errors in RealizedStackElements. Fixes #11666
1 parent be75c80 commit e671374

File tree

3 files changed

+157
-5
lines changed

3 files changed

+157
-5
lines changed

src/Avalonia.Controls/Utils/RealizedStackElements.cs

Lines changed: 36 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -281,13 +281,13 @@ public void ItemsInserted(int index, int count, Action<Control, int, int> update
281281
// elements after the insertion point.
282282
var elementCount = _elements.Count;
283283
var start = Math.Max(realizedIndex, 0);
284-
var newIndex = realizedIndex + count;
285284

286285
for (var i = start; i < elementCount; ++i)
287286
{
288-
if (_elements[i] is Control element)
289-
updateElementIndex(element, newIndex - count, newIndex);
290-
++newIndex;
287+
if (_elements[i] is not Control element)
288+
continue;
289+
var oldIndex = i + first;
290+
updateElementIndex(element, oldIndex, oldIndex + count);
291291
}
292292

293293
if (realizedIndex < 0)
@@ -341,7 +341,7 @@ public void ItemsRemoved(
341341
for (var i = 0; i < _elements.Count; ++i)
342342
{
343343
if (_elements[i] is Control element)
344-
updateElementIndex(element, newIndex - count, newIndex);
344+
updateElementIndex(element, newIndex + count, newIndex);
345345
++newIndex;
346346
}
347347
}
@@ -384,6 +384,37 @@ public void ItemsRemoved(
384384
}
385385
}
386386

387+
/// <summary>
388+
/// Updates the elements in response to items being replaced in the source collection.
389+
/// </summary>
390+
/// <param name="index">The index in the source collection of the remove.</param>
391+
/// <param name="count">The number of items removed.</param>
392+
/// <param name="recycleElement">A method used to recycle elements.</param>
393+
public void ItemsReplaced(int index, int count, Action<Control> recycleElement)
394+
{
395+
if (index < 0)
396+
throw new ArgumentOutOfRangeException(nameof(index));
397+
if (_elements is null || _elements.Count == 0)
398+
return;
399+
400+
// Get the index within the realized _elements collection.
401+
var startIndex = index - FirstIndex;
402+
var endIndex = Math.Min(startIndex + count, Count);
403+
404+
if (startIndex >= 0 && endIndex > startIndex)
405+
{
406+
for (var i = startIndex; i < endIndex; ++i)
407+
{
408+
if (_elements[i] is { } element)
409+
{
410+
recycleElement(element);
411+
_elements[i] = null;
412+
_sizes![i] = double.NaN;
413+
}
414+
}
415+
}
416+
}
417+
387418
/// <summary>
388419
/// Recycles all elements in response to the source collection being reset.
389420
/// </summary>

src/Avalonia.Controls/VirtualizingStackPanel.cs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -254,6 +254,8 @@ protected override void OnItemsChanged(IReadOnlyList<object?> items, NotifyColle
254254
_realizedElements.ItemsRemoved(e.OldStartingIndex, e.OldItems!.Count, _updateElementIndex, _recycleElementOnItemRemoved);
255255
break;
256256
case NotifyCollectionChangedAction.Replace:
257+
_realizedElements.ItemsReplaced(e.OldStartingIndex, e.OldItems!.Count, _recycleElementOnItemRemoved);
258+
break;
257259
case NotifyCollectionChangedAction.Move:
258260
_realizedElements.ItemsRemoved(e.OldStartingIndex, e.OldItems!.Count, _updateElementIndex, _recycleElementOnItemRemoved);
259261
_realizedElements.ItemsInserted(e.NewStartingIndex, e.NewItems!.Count, _updateElementIndex);

tests/Avalonia.Controls.UnitTests/VirtualizingStackPanelTests.cs

Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -611,6 +611,125 @@ public void ContainerClearing_Is_Raised_When_Scrolling()
611611
Assert.Equal(10, raised);
612612
}
613613

614+
[Fact]
615+
public void ContainerIndexChanged_Is_Raised_On_Insert()
616+
{
617+
using var app = App();
618+
var (target, scroll, itemsControl) = CreateTarget();
619+
var items = (IList)itemsControl.ItemsSource!;
620+
var raised = 0;
621+
var index = 1;
622+
623+
itemsControl.ContainerIndexChanged += (s, e) =>
624+
{
625+
++raised;
626+
Assert.Equal(index, e.OldIndex);
627+
Assert.Equal(++index, e.NewIndex);
628+
};
629+
630+
items.Insert(index, "new");
631+
632+
Assert.Equal(9, raised);
633+
}
634+
635+
[Fact]
636+
public void ContainerIndexChanged_Is_Raised_When_Item_Inserted_Before_Realized_Elements()
637+
{
638+
using var app = App();
639+
var (target, scroll, itemsControl) = CreateTarget();
640+
var items = (IList)itemsControl.ItemsSource!;
641+
var raised = 0;
642+
var index = 20;
643+
644+
itemsControl.ContainerIndexChanged += (s, e) =>
645+
{
646+
++raised;
647+
Assert.Equal(index, e.OldIndex);
648+
Assert.Equal(++index, e.NewIndex);
649+
};
650+
651+
scroll.Offset = new Vector(0, 200);
652+
Layout(target);
653+
654+
items.Insert(10, "new");
655+
656+
Assert.Equal(10, raised);
657+
}
658+
659+
[Fact]
660+
public void ContainerIndexChanged_Is_Raised_On_Remove()
661+
{
662+
using var app = App();
663+
var (target, scroll, itemsControl) = CreateTarget();
664+
var items = (IList)itemsControl.ItemsSource!;
665+
var raised = 0;
666+
var index = 1;
667+
668+
itemsControl.ContainerIndexChanged += (s, e) =>
669+
{
670+
++raised;
671+
Assert.Equal(index + 1, e.OldIndex);
672+
Assert.Equal(index++, e.NewIndex);
673+
};
674+
675+
items.RemoveAt(index);
676+
677+
Assert.Equal(8, raised);
678+
}
679+
680+
[Fact]
681+
public void ContainerIndexChanged_Is_Raised_When_Item_Removed_Before_Realized_Elements()
682+
{
683+
using var app = App();
684+
var (target, scroll, itemsControl) = CreateTarget();
685+
var items = (IList)itemsControl.ItemsSource!;
686+
var raised = 0;
687+
var index = 20;
688+
689+
itemsControl.ContainerIndexChanged += (s, e) =>
690+
{
691+
Assert.Equal(index, e.OldIndex);
692+
Assert.Equal(index - 1, e.NewIndex);
693+
++index;
694+
++raised;
695+
};
696+
697+
scroll.Offset = new Vector(0, 200);
698+
Layout(target);
699+
700+
items.RemoveAt(10);
701+
702+
Assert.Equal(10, raised);
703+
}
704+
705+
[Fact]
706+
public void Fires_Correct_Container_Lifecycle_Events_On_Replace()
707+
{
708+
using var app = App();
709+
var (target, scroll, itemsControl) = CreateTarget();
710+
var items = (IList)itemsControl.ItemsSource!;
711+
var events = new List<string>();
712+
713+
itemsControl.ContainerPrepared += (s, e) => events.Add($"Prepared #{e.Container.GetHashCode()} = {e.Index}");
714+
itemsControl.ContainerClearing += (s, e) => events.Add($"Clearing #{e.Container.GetHashCode()}");
715+
itemsControl.ContainerIndexChanged += (s, e) => events.Add($"IndexChanged #{e.Container.GetHashCode()} {e.OldIndex} -> {e.NewIndex}");
716+
717+
var toReplace = target.GetRealizedElements().ElementAt(2)!;
718+
items[2] = "New Item";
719+
720+
Assert.Equal(
721+
new[] { $"Clearing #{toReplace.GetHashCode()}" },
722+
events);
723+
events.Clear();
724+
725+
itemsControl.UpdateLayout();
726+
727+
Assert.Equal(
728+
new[] { $"Prepared #{toReplace.GetHashCode()} = 2" },
729+
events);
730+
events.Clear();
731+
}
732+
614733
[Fact]
615734
public void Scrolling_Down_With_Larger_Element_Does_Not_Cause_Jump_And_Arrives_At_End()
616735
{

0 commit comments

Comments
 (0)