Skip to content

Commit

Permalink
Fix crash with UniformGridLayout trying to scroll to first item (micr…
Browse files Browse the repository at this point in the history 
…osoft/microsoft-ui-xaml#2969)
  • Loading branch information
@Kinnara
Kinnara committed Aug 9, 2020
1 parent 7cd2362 commit f35d17d
Show file tree
Hide file tree
Showing 3 changed files with 59 additions and 90 deletions.
7 changes: 0 additions & 7 deletions ModernWpf.Controls/Repeater/Layouts/UniformGridLayout/UniformGridLayout.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -179,10 +179,6 @@ protected override Size MeasureOverride(
false /* disableVirtualization */,
LayoutId);

// If after Measure the first item is in the realization rect, then we revoke grid state's ownership,
// and only use the layout when to clear it when it's done.
gridState.EnsureFirstElementOwnership(context);

return desiredSize;
}

Expand All @@ -207,9 +203,6 @@ protected override void OnItemsChangedCore(
GetFlowAlgorithm(context).OnItemsSourceChanged(source, args, context);
// Always invalidate layout to keep the view accurate.
InvalidateLayout();

var gridState = GetAsGridState(context.LayoutState);
gridState.ClearElementOnDataSourceChange(context, args);
}

#region IFlowLayoutAlgorithmDelegates
Expand Down
93 changes: 10 additions & 83 deletions ModernWpf.Controls/Repeater/Layouts/UniformGridLayout/UniformGridLayoutState.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
// Licensed under the MIT License. See LICENSE in the project root for license information.

using System;
using System.Collections.Specialized;
using System.Windows;
using System.Windows.Controls;

Expand All @@ -21,11 +20,6 @@ internal void InitializeForContext(
internal void UninitializeForContext(VirtualizingLayoutContext context)
{
FlowAlgorithm.UninitializeForContext(context);

if (m_cachedFirstElement != null)
{
context.RecycleElement(m_cachedFirstElement);
}
}

internal FlowLayoutAlgorithm FlowAlgorithm { get; } = new FlowLayoutAlgorithm();
Expand All @@ -50,36 +44,27 @@ internal void EnsureElementSize(

if (context.ItemCount > 0)
{
// If the first element is realized we don't need to cache it or to get it from the context
// If the first element is realized we don't need to get it from the context
var realizedElement = FlowAlgorithm.GetElementIfRealized(0);
if (realizedElement != null)
{
realizedElement.Measure(availableSize);
SetSize(realizedElement, layoutItemWidth, LayoutItemHeight, availableSize, stretch, orientation, minRowSpacing, minColumnSpacing, maxItemsPerLine);
m_cachedFirstElement = null;
SetSize(realizedElement.DesiredSize, layoutItemWidth, LayoutItemHeight, availableSize, stretch, orientation, minRowSpacing, minColumnSpacing, maxItemsPerLine);
}
else
{
if (m_cachedFirstElement == null)
{
// we only cache if we aren't realizing it
m_cachedFirstElement = context.GetOrCreateElementAt(0, ElementRealizationOptions.ForceCreate | ElementRealizationOptions.SuppressAutoRecycle); // expensive
}

m_cachedFirstElement.Measure(availableSize);
SetSize(m_cachedFirstElement, layoutItemWidth, LayoutItemHeight, availableSize, stretch, orientation, minRowSpacing, minColumnSpacing, maxItemsPerLine);

// See if we can move ownership to the flow algorithm. If we can, we do not need a local cache.
bool added = FlowAlgorithm.TryAddElement0(m_cachedFirstElement);
if (added)
// Not realized by flowlayout, so do this now!
if (context.GetOrCreateElementAt(0, ElementRealizationOptions.ForceCreate) is { } firstElement)
{
m_cachedFirstElement = null;
firstElement.Measure(availableSize);
SetSize(firstElement.DesiredSize, layoutItemWidth, LayoutItemHeight, availableSize, stretch, orientation, minRowSpacing, minColumnSpacing, maxItemsPerLine);
context.RecycleElement(firstElement);
}
}
}
}

private void SetSize(UIElement UIElement,
private void SetSize(Size desiredItemSize,
double layoutItemWidth,
double LayoutItemHeight,
Size availableSize,
Expand All @@ -94,8 +79,8 @@ private void SetSize(UIElement UIElement,
maxItemsPerLine = 1;
}

EffectiveItemWidth = double.IsNaN(layoutItemWidth) ? UIElement.DesiredSize.Width : layoutItemWidth;
EffectiveItemHeight = double.IsNaN(LayoutItemHeight) ? UIElement.DesiredSize.Height : LayoutItemHeight;
EffectiveItemWidth = double.IsNaN(layoutItemWidth) ? desiredItemSize.Width : layoutItemWidth;
EffectiveItemHeight = double.IsNaN(LayoutItemHeight) ? desiredItemSize.Height : LayoutItemHeight;

var availableSizeMinor = orientation == Orientation.Horizontal ? availableSize.Width : availableSize.Height;
var minorItemSpacing = orientation == Orientation.Vertical ? minRowSpacing : minColumnSpacing;
Expand Down Expand Up @@ -146,63 +131,5 @@ private void SetSize(UIElement UIElement,
}
}
}

// If it's realized then we shouldn't be caching it
internal void EnsureFirstElementOwnership(VirtualizingLayoutContext context)
{
if (m_cachedFirstElement != null && FlowAlgorithm.GetElementIfRealized(0) != null)
{
// We created the element, but then flowlayout algorithm took ownership, so we can clear it and
// let flowlayout algorithm do its thing.
context.RecycleElement(m_cachedFirstElement);
m_cachedFirstElement = null;
}
}

internal void ClearElementOnDataSourceChange(VirtualizingLayoutContext context, NotifyCollectionChangedEventArgs args)
{
if (m_cachedFirstElement != null)
{
// The first element of UniformGridLayout is special since we use its size to
// determine the size of all the other elements. So if the first item has changed
// we will need to clear it and re-evalauate all the items with the new item size.
bool shouldClear = false;
switch (args.Action)
{
case NotifyCollectionChangedAction.Add:
shouldClear = args.NewStartingIndex == 0;
break;

case NotifyCollectionChangedAction.Replace:
shouldClear = args.NewStartingIndex == 0 || args.OldStartingIndex == 0;
break;

case NotifyCollectionChangedAction.Remove:
shouldClear = args.OldStartingIndex == 0;
break;

case NotifyCollectionChangedAction.Reset:
shouldClear = true;
break;

case NotifyCollectionChangedAction.Move:
shouldClear = args.NewStartingIndex == 0 || args.OldStartingIndex == 0;
break;
}

if (shouldClear)
{
context.RecycleElement(m_cachedFirstElement);
m_cachedFirstElement = null;
}
}
}

// We need to measure the element at index 0 to know what size to measure all other items.
// If FlowlayoutAlgorithm has already realized element 0 then we can use that.
// If it does not, then we need to do context.GetElement(0) at which point we have requested an element and are on point to clear it.
// If we are responsible for clearing element 0 we keep m_cachedFirstElement valid.
// If we are not (because FlowLayoutAlgorithm is holding it for us) then we just null out this field and use the one from FlowLayoutAlgorithm.
private UIElement m_cachedFirstElement = null;
}
}
49 changes: 49 additions & 0 deletions test/ModernWpfTestApp/ApiTests/RepeaterTests/LayoutTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@
using ElementRealizationOptions = ModernWpf.Controls.ElementRealizationOptions;
using LayoutContext = ModernWpf.Controls.LayoutContext;
using LayoutPanel = ModernWpf.Controls.LayoutPanel;
using System.Windows.Media;
using ModernWpf.Controls;

namespace ModernWpf.Tests.MUXControls.ApiTests.RepeaterTests
{
Expand Down Expand Up @@ -254,6 +256,53 @@ public void ValidateStackLayoutDisabledVirtualizationWithItemsRepeater()
});
}

[TestMethod]
public void VerifyUniformGridLayoutDoesntCrashWhenTryingToScrollToEnd()
{

ItemsRepeater repeater = null;
ScrollViewer scrollViewer = null;
RunOnUIThread.Execute(() =>
{
repeater = new ItemsRepeater
{
ItemsSource = Enumerable.Range(0, 1000).Select(i => new Border
{
Background = new SolidColorBrush(Colors.Blue),
Child = new TextBlock { Text = "#" + i }
}).ToArray(),
Layout = new UniformGridLayout
{
MinItemWidth = 100,
MinItemHeight = 40,
MinRowSpacing = 10,
MinColumnSpacing = 10
}
};
scrollViewer = new ScrollViewer { Content = repeater };
Content = scrollViewer;
});

IdleSynchronizer.Wait();

RunOnUIThread.Execute(() =>
{
scrollViewer.ChangeView(0, repeater.ActualHeight, null);
});

IdleSynchronizer.Wait();

RunOnUIThread.Execute(() =>
{
scrollViewer.ChangeView(0, 0, null);
});

IdleSynchronizer.Wait();

// The test guards against an app crash, so this is enough to verify
Verify.IsTrue(true);
}

private ItemsRepeaterScrollHost CreateAndInitializeRepeater(
object itemsSource,
VirtualizingLayout layout,
Expand Down

0 comments on commit f35d17d

Please sign in to comment.