sbaeumlisberger / VirtualizingWrapPanel

Implementation of a comprehensive VirtualisingWrapPanel for WPF
MIT License
256 stars 35 forks source link

Collecition with same item references breaks the VirtualizingWrapPanel and throws ThrowKeyNotFoundException #66

Closed ScarletKuro closed 1 month ago

ScarletKuro commented 1 month ago

Describe your issue Hello.

I have a reproduction (it's part of production app, I tried to remove as much code as possible):

VirtualizationPanelRepro.zip

If you look at MainWindow.xaml.cs, line 245:

groupReceipt.Coupons = groupReceipt.Coupons.SelectMany(item => Enumerable.Repeat(item, 10)).ToArray();

This line breaks virtualization:

EmptyItems

The items are not displayed, and when you try to scroll using the left and right buttons, some items appear. However, when you reach the end, you encounter an exception:

 at System.ThrowHelper.ThrowKeyNotFoundException[T](T key)
 at System.Collections.Generic.Dictionary`2.get_Item(TKey key)
 at WpfToolkit.Controls.VirtualizingWrapPanel.ArrangeOverride(Size finalSize)
 at System.Windows.FrameworkElement.ArrangeCore(Rect finalRect)
 at System.Windows.UIElement.Arrange(Rect finalRect)
 at System.Windows.ContextLayoutManager.UpdateLayout()
 at System.Windows.ContextLayoutManager.UpdateLayoutCallback(Object arg)
 at System.Windows.Media.MediaContext.FireInvokeOnRenderCallbacks()
 at System.Windows.Media.MediaContext.RenderMessageHandlerCore(Object resizedCompositionTarget)
 at System.Windows.Media.MediaContext.RenderMessageHandler(Object resizedCompositionTarget)
 at System.Windows.Threading.ExceptionWrapper.InternalRealCall(Delegate callback, Object args, Int32 numArgs)
 at System.Windows.Threading.ExceptionWrapper.TryCatchWhen(Object source, Delegate callback, Object args, Int32 numArgs, Delegate catchHandler)

This issue is resolved if you replace VirtualizingWrapPanel with WrapPanel in App.xaml, line 462. The items and side scrolling will then work correctly.

NB! Honestly, the Enumerable.Repeat was for testing purpose as I wanted to see how it works with many items and in real code I don't have it, but I thought you might be interested to fix it as I'm sure someone can be affected by it.

Version Info Package Version [e.g. 2.0.11] .NET Version: [e.g. .NET 6] OS Version: [e.g. Windows 10 Build 19045.4894]

sbaeumlisberger commented 1 month ago

For virtualization to work efficiently, the items need to be distinguishable in some way. Therefore, it will not work if your items are not unique. I have added a check so that the user will get a helpful error message.

ScarletKuro commented 1 month ago

For virtualization to work efficiently, the items need to be distinguishable in some way. Therefore, it will not work if your items are not unique. I have added a check so that the user will get a helpful error message.

Thanks for the explanation and for the better error message. I wonder if this would be more efficient on paper.

private void VerifyItemsDistinct()
{
    var seenItems = new HashSet<object>();
    foreach (var item in Items)
    {
        if (!seenItems.Add(item))
        {
            throw new InvalidOperationException("Items must be distinct.");
        }
    }
}

Since HashSet has O(1) in average for both insertions and lookups, while the Items.Distinct().Count() != Items.Count should be O(n). But HashSet would take more memory.

sbaeumlisberger commented 1 month ago

Since Distinct also uses a set internally, as in your suggestion, it should be similarly efficient. There may be a very tiny overhead, but I think it is negligible. Only in the rare error case that the items are not unique would your suggestion really be faster, since it does not have to iterate over all items.