Skip to content

Commit

Permalink
Avoid reset of containers when anchor element is replaced in ItemsRep…
Browse files Browse the repository at this point in the history 
…eater (#926)

* avoid reset of containers when anchor element is replaced

* fix element 0 ownership issue
  • Loading branch information
@ranjeshj
ranjeshj authored Jun 25, 2019
1 parent b86cd53 commit e45e83d
Show file tree
Hide file tree
Showing 5 changed files with 138 additions and 10 deletions.
101 changes: 99 additions & 2 deletions dev/Repeater/APITests/FlowLayoutCollectionChangeTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,10 @@
using RecyclePool = Microsoft.UI.Xaml.Controls.RecyclePool;
using StackLayout = Microsoft.UI.Xaml.Controls.StackLayout;
using FlowLayout = Microsoft.UI.Xaml.Controls.FlowLayout;
using UniformGridLayout = Microsoft.UI.Xaml.Controls.UniformGridLayout;
using ItemsRepeaterScrollHost = Microsoft.UI.Xaml.Controls.ItemsRepeaterScrollHost;
using AnimationContext = Microsoft.UI.Xaml.Controls.AnimationContext;
using System.Collections.Generic;
#endif

namespace Windows.UI.Xaml.Tests.MUXControls.ApiTests.RepeaterTests
Expand Down Expand Up @@ -251,6 +253,8 @@ public void CanReplaceSingleItem()
ScrollViewer scrollViewer = null;
ItemsRepeater repeater = null;
var viewChangedEvent = new ManualResetEvent(false);
int elementsCleared = 0;
int elementsPrepared = 0;

RunOnUIThread.Execute(() =>
{
Expand All @@ -262,6 +266,10 @@ public void CanReplaceSingleItem()
viewChangedEvent.Set();
}
};

repeater.ElementPrepared += (sender, args) => { elementsPrepared++; };
repeater.ElementClearing += (sender, args) => { elementsCleared++; };

scrollViewer.ChangeView(null, 200, null, true);
});

Expand All @@ -281,8 +289,12 @@ public void CanReplaceSingleItem()
Verify.AreEqual(4, realized);

Log.Comment("Replace in realized range.");
elementsPrepared = 0;
elementsCleared = 0;
dataSource.Replace(index: 2, oldCount: 1, newCount: 1, reset: false);
repeater.UpdateLayout();
Verify.AreEqual(1, elementsPrepared);
Verify.AreEqual(1, elementsCleared);

realized = VerifyRealizedRange(repeater, dataSource);
Verify.AreEqual(4, realized);
Expand All @@ -296,6 +308,88 @@ public void CanReplaceSingleItem()
});
}

[TestMethod]
public void VerifyElement0OwnershipInUniformGridLayout()
{
CustomItemsSource dataSource = null;
RunOnUIThread.Execute(() => dataSource = new CustomItemsSource(new List<int>()));
ItemsRepeater repeater = null;
int elementsCleared = 0;
int elementsPrepared = 0;

RunOnUIThread.Execute(() =>
{
repeater = SetupRepeater(dataSource, new UniformGridLayout());
repeater.ElementPrepared += (sender, args) => { elementsPrepared++; };
repeater.ElementClearing += (sender, args) => { elementsCleared++; };
});

IdleSynchronizer.Wait();

RunOnUIThread.Execute(() =>
{
Log.Comment("Add two item");
dataSource.Insert(index: 0, count: 1, reset: false);
dataSource.Insert(index: 1, count: 1, reset: false);
repeater.UpdateLayout();
var realized = VerifyRealizedRange(repeater, dataSource);
Verify.AreEqual(2, realized);

Log.Comment("replace the first item");
dataSource.Replace(index: 0, oldCount: 1, newCount: 1, reset: false);
repeater.UpdateLayout();
realized = VerifyRealizedRange(repeater, dataSource);
Verify.AreEqual(2, realized);

Log.Comment("Remove two items");
dataSource.Remove(index: 1, count: 1, reset: false);
dataSource.Remove(index: 0, count: 1, reset:false);
repeater.UpdateLayout();
realized = VerifyRealizedRange(repeater, dataSource);
Verify.AreEqual(0, realized);

Verify.AreEqual(elementsPrepared, elementsCleared);
});
}

[TestMethod]
public void EnsureReplaceOfAnchorDoesNotResetAllContainers()
{
CustomItemsSource dataSource = null;
RunOnUIThread.Execute(() => dataSource = new CustomItemsSource(Enumerable.Range(0, 10).ToList()));
ScrollViewer scrollViewer = null;
ItemsRepeater repeater = null;
var viewChangedEvent = new ManualResetEvent(false);
int elementsCleared = 0;
int elementsPrepared = 0;

RunOnUIThread.Execute(() =>
{
repeater = SetupRepeater(dataSource, ref scrollViewer);
repeater.ElementPrepared += (sender, args) => { elementsPrepared++; };
repeater.ElementClearing += (sender, args) => { elementsCleared++; };
});

IdleSynchronizer.Wait();

RunOnUIThread.Execute(() =>
{
var realized = VerifyRealizedRange(repeater, dataSource);
Verify.AreEqual(3, realized);

Log.Comment("Replace anchor element 0");
elementsPrepared = 0;
elementsCleared = 0;
dataSource.Replace(index: 0, oldCount: 1, newCount: 1, reset: false);
repeater.UpdateLayout();
Verify.AreEqual(1, elementsPrepared);
Verify.AreEqual(1, elementsCleared);

realized = VerifyRealizedRange(repeater, dataSource);
Verify.AreEqual(3, realized);
});
}

//[TestMethod]
public void ReplaceMultipleItems()
{
Expand Down Expand Up @@ -421,8 +515,11 @@ private ItemsRepeater SetupRepeater(CustomItemsSource dataSource, ElementFactory
};

Content.UpdateLayout();
int realized = VerifyRealizedRange(repeater, dataSource);
Verify.IsGreaterThan(realized, 0);
if (dataSource.Count > 0)
{
int realized = VerifyRealizedRange(repeater, dataSource);
Verify.IsGreaterThan(realized, 0);
}

return repeater;
}
Expand Down
36 changes: 32 additions & 4 deletions dev/Repeater/ElementManager.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ void ElementManager::Insert(int realizedIndex, int dataIndex, const winrt::UIEle
void ElementManager::ClearRealizedRange(int realizedIndex, int count)
{
MUX_ASSERT(IsVirtualizingContext());
for (int i = 0 ; i < count; i++)
for (int i = 0; i < count; i++)
{
// Clear from the edges so that ItemsRepeater can optimize on maintaining
// realized indices without walking through all the children every time.
Expand Down Expand Up @@ -240,7 +240,7 @@ bool ElementManager::IsWindowConnected(const winrt::Rect& window, const ScrollOr
auto effectiveOrientation = scrollOrientationSameAsFlow ?
(orientation == ScrollOrientation::Vertical ? ScrollOrientation::Horizontal : ScrollOrientation::Vertical) :
orientation;


auto windowStart = effectiveOrientation == ScrollOrientation::Vertical ? window.Y : window.X;
auto windowEnd = effectiveOrientation == ScrollOrientation::Vertical ? window.Y + window.Height : window.X + window.Width;
Expand Down Expand Up @@ -270,8 +270,36 @@ void ElementManager::DataSourceChanged(const winrt::IInspectable& /*source*/, wi

case winrt::NotifyCollectionChangedAction::Replace:
{
OnItemsRemoved(args.OldStartingIndex(), args.OldItems().Size());
OnItemsAdded(args.NewStartingIndex(), args.NewItems().Size());
int oldSize = args.OldItems().Size();
int newSize = args.NewItems().Size();
int oldStartIndex = args.OldStartingIndex();
int newStartIndex = args.NewStartingIndex();

if (oldSize == newSize &&
oldStartIndex == newStartIndex &&
IsDataIndexRealized(oldStartIndex) &&
IsDataIndexRealized(oldStartIndex + oldSize -1))
{
// Straight up replace of n items within the realization window.
// Removing and adding might causes us to lose the anchor causing us
// to throw away all containers and start from scratch.
// Instead, we can just clear those items and set the element to
// null (sentinel) and let the next measure get new containers for them.
auto startRealizedIndex = GetRealizedRangeIndexFromDataIndex(oldStartIndex);
for (int realizedIndex = startRealizedIndex; realizedIndex < startRealizedIndex + oldSize; realizedIndex++)
{
if (auto elementRef = m_realizedElements[realizedIndex])
{
m_context.RecycleElement(elementRef.get());
m_realizedElements[realizedIndex] = tracker_ref<winrt::UIElement>{ m_owner, nullptr };
}
}
}
else
{
OnItemsRemoved(oldStartIndex, oldSize);
OnItemsAdded(newStartIndex, newSize);
}
}
break;

Expand Down
2 changes: 1 addition & 1 deletion dev/Repeater/UniformGridLayout.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ winrt::Size UniformGridLayout::MeasureOverride(

// 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();
gridState->EnsureFirstElementOwnership(context);

return { desiredSize.Width, desiredSize.Height };
}
Expand Down
7 changes: 5 additions & 2 deletions dev/Repeater/UniformGridLayoutState.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -119,10 +119,13 @@ void UniformGridLayoutState::SetSize(
}
}

void UniformGridLayoutState::EnsureFirstElementOwnership()
void UniformGridLayoutState::EnsureFirstElementOwnership(winrt::VirtualizingLayoutContext const& context)
{
if (m_flowAlgorithm.GetElementIfRealized(0))
if (m_cachedFirstElement != nullptr && m_flowAlgorithm.GetElementIfRealized(0))
{
// 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 = nullptr;
}
}
Expand Down
2 changes: 1 addition & 1 deletion dev/Repeater/UniformGridLayoutState.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ class UniformGridLayoutState :
double EffectiveItemHeight() { return m_effectiveItemHeight; }

// If it's realized then we shouldn't be caching it
void EnsureFirstElementOwnership();
void EnsureFirstElementOwnership(winrt::VirtualizingLayoutContext const& context);

void EnsureElementSize(
const winrt::Size availableSize,
Expand Down

0 comments on commit e45e83d

Please sign in to comment.