diff --git a/dev/Repeater/APITests/FlowLayoutCollectionChangeTests.cs b/dev/Repeater/APITests/FlowLayoutCollectionChangeTests.cs index 50fd9e68bd..61aaade4e2 100644 --- a/dev/Repeater/APITests/FlowLayoutCollectionChangeTests.cs +++ b/dev/Repeater/APITests/FlowLayoutCollectionChangeTests.cs @@ -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 @@ -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(() => { @@ -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); }); @@ -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); @@ -296,6 +308,88 @@ public void CanReplaceSingleItem() }); } + [TestMethod] + public void VerifyElement0OwnershipInUniformGridLayout() + { + CustomItemsSource dataSource = null; + RunOnUIThread.Execute(() => dataSource = new CustomItemsSource(new List())); + 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() { @@ -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; } diff --git a/dev/Repeater/ElementManager.cpp b/dev/Repeater/ElementManager.cpp index f6b3f6a2f2..36a8826124 100644 --- a/dev/Repeater/ElementManager.cpp +++ b/dev/Repeater/ElementManager.cpp @@ -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. @@ -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; @@ -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{ m_owner, nullptr }; + } + } + } + else + { + OnItemsRemoved(oldStartIndex, oldSize); + OnItemsAdded(newStartIndex, newSize); + } } break; diff --git a/dev/Repeater/UniformGridLayout.cpp b/dev/Repeater/UniformGridLayout.cpp index a7d1fb2801..f7210bad1f 100644 --- a/dev/Repeater/UniformGridLayout.cpp +++ b/dev/Repeater/UniformGridLayout.cpp @@ -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 }; } diff --git a/dev/Repeater/UniformGridLayoutState.cpp b/dev/Repeater/UniformGridLayoutState.cpp index b75cb60edb..ad17a72db8 100644 --- a/dev/Repeater/UniformGridLayoutState.cpp +++ b/dev/Repeater/UniformGridLayoutState.cpp @@ -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; } } diff --git a/dev/Repeater/UniformGridLayoutState.h b/dev/Repeater/UniformGridLayoutState.h index a7350d759a..5c7b5e03ab 100644 --- a/dev/Repeater/UniformGridLayoutState.h +++ b/dev/Repeater/UniformGridLayoutState.h @@ -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,