Skip to content

Commit

Permalink
Fix crash in uniform grid when fed an empty list (#529)
Browse files Browse the repository at this point in the history
  • Loading branch information
@ranjeshj @jevansaks
ranjeshj authored and jevansaks committed Apr 5, 2019
1 parent d5abae6 commit 1199f38
Show file tree
Hide file tree
Showing 3 changed files with 99 additions and 70 deletions.
114 changes: 70 additions & 44 deletions dev/Repeater/APITests/FlowLayoutTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ namespace Windows.UI.Xaml.Tests.MUXControls.ApiTests.RepeaterTests
{
[TestClass]
public class FlowLayoutTests : TestsBase
{
{
[TestMethod]
public void ValidateStackLayout()
{
Expand All @@ -68,17 +68,17 @@ public void ValidateStackLayout()
Layout = new StackLayout() { Orientation = scrollOrientation.ToLayoutOrientation() }
};

SetPanelMinorSize(panel, om, panelMinorSize);
for (int i = 0; i < numItems; i++)
{
panel.Children.Add(
new Button()
{
Content = i,
Width = om.IsVerical ? itemMinorSize : itemMajorSize,
Height = om.IsVerical ? itemMajorSize : itemMinorSize
});
}
SetPanelMinorSize(panel, om, panelMinorSize);
for (int i = 0; i < numItems; i++)
{
panel.Children.Add(
new Button()
{
Content = i,
Width = om.IsVerical ? itemMinorSize : itemMajorSize,
Height = om.IsVerical ? itemMajorSize : itemMinorSize
});
}

Content = panel;
Content.UpdateLayout();
Expand Down Expand Up @@ -150,7 +150,7 @@ public void ValidateGridLayout()
}
});
}

[TestMethod]
public void ValidateResizingFirstItemResizesOtherItemsInGridLayout()
{
Expand Down Expand Up @@ -480,8 +480,8 @@ public void ValidateFlowLayout()
{
Log.Comment(string.Format("ScrollOrientation: {0}", scrollOrientation));
var om = new OrientationBasedMeasures(scrollOrientation);
int [] panelMinorSizeChoices = new int[] { 10, 50 };
foreach(int panelMinorSize in panelMinorSizeChoices)
int[] panelMinorSizeChoices = new int[] { 10, 50 };
foreach (int panelMinorSize in panelMinorSizeChoices)
{
const int numItems = 10;
LayoutPanel panel = new LayoutPanel()
Expand All @@ -493,11 +493,11 @@ public void ValidateFlowLayout()
}
};

SetPanelMinorSize(panel, om, panelMinorSize);
for (int i = 0; i < numItems; i++)
{
panel.Children.Add(new Button() { Content = i });
}
SetPanelMinorSize(panel, om, panelMinorSize);
for (int i = 0; i < numItems; i++)
{
panel.Children.Add(new Button() { Content = i });
}

Content = panel;

Expand All @@ -513,7 +513,7 @@ public void ValidateFlowLayout()
Content.UpdateLayout();
ValidateFlowLayoutChildrenLayoutBounds(om, (i) => panel.Children[i], minItemSpacing, lineSpacing, panel.Children.Count, panel.DesiredSize);
}
}
}
});
}

Expand All @@ -527,8 +527,10 @@ public void ValidateFlowLayoutVaryingHeights()
Log.Comment(string.Format("ScrollOrientation: {0}", scrollOrientation));
var om = new OrientationBasedMeasures(scrollOrientation);
const int numItems = 10;
LayoutPanel panel = new LayoutPanel() {
Layout = new FlowLayout() {
LayoutPanel panel = new LayoutPanel()
{
Layout = new FlowLayout()
{
Orientation = scrollOrientation.ToOrthogonalLayoutOrientation(),
LineAlignment = FlowLayoutLineAlignment.Start
}
Expand Down Expand Up @@ -982,15 +984,15 @@ public void ValidateLayoutWithInfiniteMeasureSizeInNonVirtualizingDirection()
Verify.AreEqual(expected.Height, scrollOrientation == ScrollOrientation.Vertical ? actual.Height : actual.Width);
});

foreach(var layout in layouts)
foreach (var layout in layouts)
{
Log.Comment("Layout is " + layout.GetType().Name);
repeater.Layout = layout;
Content.UpdateLayout();
if (layout is StackLayout) verifyDesiredSize(new Size(160, 430), repeater.DesiredSize);
if (layout is UniformGridLayout) verifyDesiredSize(new Size(190, 40), repeater.DesiredSize);
if (layout is FlowLayout) verifyDesiredSize(new Size(430, 160), repeater.DesiredSize);

if (layout is StackLayout) verifyDesiredSize(new Size(160, 430), repeater.DesiredSize);
if (layout is UniformGridLayout) verifyDesiredSize(new Size(190, 40), repeater.DesiredSize);
if (layout is FlowLayout) verifyDesiredSize(new Size(430, 160), repeater.DesiredSize);
}
}
});
Expand Down Expand Up @@ -1086,14 +1088,35 @@ public void ValidateIntersectionWithRealizationWindow()
}
}

[TestMethod]
public void ValidateUniformGridWithNoItems()
{
RunOnUIThread.Execute(() =>
{
var repeater = new ItemsRepeater()
{
ItemsSource = new List<string>(), // no items
Layout = new UniformGridLayout() { Orientation = Orientation.Vertical },
};

Content = repeater;
Content.UpdateLayout();

// Ensure we do not crash and get zero size.
Verify.AreEqual(0, repeater.DesiredSize.Width);
Verify.AreEqual(0, repeater.DesiredSize.Height);
});
}

[TestMethod]
public void ValidateFlowLayoutWithOneItemHasNonZeroExtent()
{
RunOnUIThread.Execute(() =>
{
foreach (ScrollOrientation scrollOrientation in Enum.GetValues(typeof(ScrollOrientation)))
{
var repeater = new ItemsRepeater() {
var repeater = new ItemsRepeater()
{
ItemsSource = new List<string>() { "single item" },
Layout = new FlowLayout() { Orientation = scrollOrientation.ToLayoutOrientation() },
};
Expand All @@ -1119,8 +1142,9 @@ public void ValidateGridLayoutWithSameOrientationAsScrolling()
var firstRealizedIndex = int.MaxValue;
RunOnUIThread.Execute(() =>
{

var repeater = new ItemsRepeater() {

var repeater = new ItemsRepeater()
{
ItemsSource = Enumerable.Range(1, 1000),
ItemTemplate = GetDataTemplate(@"<Button Content='{Binding}' Width='100' Height='100'/>"),
Layout = new UniformGridLayout() { Orientation = Orientation.Vertical },
Expand All @@ -1134,19 +1158,21 @@ public void ValidateGridLayoutWithSameOrientationAsScrolling()
lastRealizedIndex = Math.Max(lastRealizedIndex, args.Index);
};

scrollViewer = new ScrollViewer() {
scrollViewer = new ScrollViewer()
{
Content = repeater,
Height = 200,
};

var anchorProvier = new ItemsRepeaterScrollHost() {
var anchorProvier = new ItemsRepeaterScrollHost()
{
ScrollViewer = scrollViewer
};

Content = anchorProvier;
Content.UpdateLayout();

Verify.IsLessThan(lastRealizedIndex - firstRealizedIndex, 10);
Verify.IsLessThan(lastRealizedIndex - firstRealizedIndex, 10);

scrollViewer.ViewChanged += (sender, args) =>
{
Expand All @@ -1162,16 +1188,16 @@ public void ValidateGridLayoutWithSameOrientationAsScrolling()
{
lastRealizedIndex = int.MinValue;
firstRealizedIndex = int.MaxValue;
scrollViewer.ChangeView(horizontalOffset: 0, verticalOffset: (i+1)*80, zoomFactor: null, disableAnimation: true);
scrollViewer.ChangeView(horizontalOffset: 0, verticalOffset: (i + 1) * 80, zoomFactor: null, disableAnimation: true);
});

IdleSynchronizer.Wait();
Verify.IsTrue(viewChanged.WaitOne(DefaultWaitTime));

RunOnUIThread.Execute(() =>
{
// we used to crash due to a layout cycle before we get here.
Verify.IsLessThan(lastRealizedIndex - firstRealizedIndex, 10);
// we used to crash due to a layout cycle before we get here.
Verify.IsLessThan(lastRealizedIndex - firstRealizedIndex, 10);
});
}
}
Expand Down Expand Up @@ -1251,7 +1277,7 @@ private void ValidateFlowLayoutChildrenLayoutBounds(
double minItemSpacing,
double lineSpacing,
int childCount,
Size desiredSize,
Size desiredSize,
float? expectedItemWidth = null,
float? expectedItemHeight = null)
{
Expand All @@ -1265,8 +1291,8 @@ private void ValidateFlowLayoutChildrenLayoutBounds(

var layoutBounds = LayoutInformation.GetLayoutSlot(child);

expectedRect.Width = expectedItemWidth.HasValue ? expectedItemWidth.Value: child.DesiredSize.Width;
expectedRect.Height = expectedItemHeight.HasValue? expectedItemHeight.Value: child.DesiredSize.Height;
expectedRect.Width = expectedItemWidth.HasValue ? expectedItemWidth.Value : child.DesiredSize.Width;
expectedRect.Height = expectedItemHeight.HasValue ? expectedItemHeight.Value : child.DesiredSize.Height;
lineSize = Math.Max(lineSize, om.Major(child.DesiredSize));

Log.Comment(string.Format(@"Index:{0}, Expected:{1} Actual:{2}", i, expectedRect, layoutBounds));
Expand Down Expand Up @@ -1707,9 +1733,9 @@ private static void VerifyRealizedRange(int expectedStartindex, int expectedCoun
Verify.IsTrue(actualIndices.Contains(i));
}
}


private void SetPanelMinorSize(LayoutPanel panel , OrientationBasedMeasures om, double value)

private void SetPanelMinorSize(LayoutPanel panel, OrientationBasedMeasures om, double value)
{
if (om.IsVerical)
{
Expand All @@ -1734,7 +1760,7 @@ private static void ValidateElementsSizeInGridLayout(LayoutPanel panel, double i
{
for (int i = 0; i < panel.Children.Count; i++)
{
var child = (FrameworkElement) panel.Children.ElementAt(i);
var child = (FrameworkElement)panel.Children.ElementAt(i);
var layoutBounds = LayoutInformation.GetLayoutSlot(child);

Verify.AreEqual(itemWidth, layoutBounds.Width);
Expand All @@ -1744,7 +1770,7 @@ private static void ValidateElementsSizeInGridLayout(LayoutPanel panel, double i

private static void ModifyElementSize(LayoutPanel panel, int index, double itemWidth, double itemHeight)
{
var item = (FrameworkElement) panel.Children.ElementAt(index);
var item = (FrameworkElement)panel.Children.ElementAt(index);

if (!Double.IsNaN(itemWidth))
{
Expand Down
14 changes: 7 additions & 7 deletions dev/Repeater/UniformGridLayout.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -199,14 +199,14 @@ winrt::Rect UniformGridLayout::Algorithm_GetExtent(
static_cast<int>(availableSizeMinor / GetMinorSizeWithSpacing(context)) : itemsCount);
const float lineSize = GetMajorSizeWithSpacing(context);

extent.*MinorSize() =
std::isfinite(availableSizeMinor) ?
availableSizeMinor :
std::max(0.0f, itemsCount * GetMinorSizeWithSpacing(context) - static_cast<float>(MinItemSpacing()));
extent.*MajorSize() = std::max(0.0f, (itemsCount / itemsPerLine) * lineSize - static_cast<float>(LineSpacing()));

if (itemsCount > 0)
{
{
extent.*MinorSize() =
std::isfinite(availableSizeMinor) ?
availableSizeMinor :
std::max(0.0f, itemsCount * GetMinorSizeWithSpacing(context) - static_cast<float>(MinItemSpacing()));
extent.*MajorSize() = std::max(0.0f, (itemsCount / itemsPerLine) * lineSize - static_cast<float>(LineSpacing()));

if (firstRealized)
{
MUX_ASSERT(lastRealized);
Expand Down
41 changes: 22 additions & 19 deletions dev/Repeater/UniformGridLayoutState.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,29 +38,32 @@ void UniformGridLayoutState::EnsureElementSize(
double minRowSpacing,
double minColumnSpacing)
{
// If the first element is realized we don't need to cache it or to get it from the context
if (auto realizedElement = m_flowAlgorithm.GetElementIfRealized(0))
if (context.ItemCount() > 0)
{
realizedElement.Measure(availableSize);
SetSize(realizedElement, layoutItemWidth, LayoutItemHeight, availableSize, stretch, orientation, minRowSpacing, minColumnSpacing);
m_cachedFirstElement = nullptr;
}
else
{
if (!m_cachedFirstElement)
// If the first element is realized we don't need to cache it or to get it from the context
if (auto realizedElement = m_flowAlgorithm.GetElementIfRealized(0))
{
// we only cache if we aren't realizing it
m_cachedFirstElement = context.GetOrCreateElementAt(0, winrt::ElementRealizationOptions::ForceCreate | winrt::ElementRealizationOptions::SuppressAutoRecycle); // expensive
realizedElement.Measure(availableSize);
SetSize(realizedElement, layoutItemWidth, LayoutItemHeight, availableSize, stretch, orientation, minRowSpacing, minColumnSpacing);
m_cachedFirstElement = nullptr;
}

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

// See if we can move ownership to the flow algorithm. If we can, we do not need a local cache.
bool added = m_flowAlgorithm.TryAddElement0(m_cachedFirstElement);
if (added)
else
{
m_cachedFirstElement = nullptr;
if (!m_cachedFirstElement)
{
// we only cache if we aren't realizing it
m_cachedFirstElement = context.GetOrCreateElementAt(0, winrt::ElementRealizationOptions::ForceCreate | winrt::ElementRealizationOptions::SuppressAutoRecycle); // expensive
}

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

// See if we can move ownership to the flow algorithm. If we can, we do not need a local cache.
bool added = m_flowAlgorithm.TryAddElement0(m_cachedFirstElement);
if (added)
{
m_cachedFirstElement = nullptr;
}
}
}
}
Expand Down

0 comments on commit 1199f38

Please sign in to comment.