Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid reset of containers when anchor element is replaced in ItemsRepeater #926

Merged
merged 2 commits into from
Jun 25, 2019
Merged

Avoid reset of containers when anchor element is replaced in ItemsRepeater #926

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
41beac8
avoid reset of containers when anchor element is replaced
ranjeshj Jun 22, 2019
0be6d12
fix element 0 ownership issue
ranjeshj Jun 24, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 48 additions & 0 deletions dev/Repeater/APITests/FlowLayoutCollectionChangeTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,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 +264,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 +287,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 +306,44 @@ public void CanReplaceSingleItem()
});
}

[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
36 changes: 32 additions & 4 deletions dev/Repeater/ElementManager.cpp
View file
Open in desktop
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) &&
ranjeshj marked this conversation as resolved.
Show resolved Hide resolved
IsDataIndexRealized(oldStartIndex + oldSize))
{
// 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);
ranjeshj marked this conversation as resolved.
Show resolved Hide resolved
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(args.OldStartingIndex(), oldSize);
OnItemsAdded(args.NewStartingIndex(), newSize);
}
}
break;

Expand Down