AdvancedCollectionView ArgumentOutOfRangeException on Filter when last item from the list is filtered.

[blakepell]

I'm submitting a...

Bug report (I searched for similar issues and did not find one)

Current behavior

When I apply a filter to an AdvancedCollectionView that's bound to a DataGrid I am met with an ArgumentOutOfRangeException if the last item was filtered off the list.

The application of the filter eventually causes "public object this[int index]" to try to access a position that is 1 more than the upper bound of the list which throws the ArgumentOutOfRangeException. This occurs when the last item in the list is filtered off. If the last item is not filtered off, no exception occurs.

// GetVariable returns either the requested variable or a blank string, never null.
string username = Utilities.GetVariable("Character");
TriggerList.Filter = x => (((Trigger)x).Character == "" || ((Trigger)x).Character == username);

When an item is filtered out in the last position, it is removed and then "VectorChanged?.Invoke" is called but it's passing the index for the position that no longer exists that is no one above the upper bound. As a test, I modified the "public object this[int index]" to check the upper bound of the "_view" first and not access it if the index was too high (I returned a null if that was the case). This fixed the behavior and allowed the filter to work as far as I can see in my limited testing for my limited use cases.

Expected behavior

You should be able to set a filter with valid syntax / data and not have it throw an exception.

Minimal reproduction of the problem with instructions

1.) Create an AdvancedCollectionView and populate it with test data.
2.) Apply a filter that will remove the last item in the list.

Environment

Nuget Package(s): 

Package Version(s): 

Windows 10 Build Number:
- [ ] Fall Creators Update (16299)
- [ ] April 2018 Update (17134)
- [x] October 2018 Update (17763)
- [ ] Insider Build (build number: )

App min and target version:
- [ ] Fall Creators Update (16299)
- [ ] April 2018 Update (17134)
- [x] October 2018 Update (17763)
- [ ] Insider Build (xxxxx)

Device form factor:
- [x] Desktop
- [ ] Mobile
- [ ] Xbox
- [ ] Surface Hub
- [ ] IoT

Visual Studio 
- [x] 2019 (version: 16.0.0)

[blakepell]

I have another potentially related bug to this behavior with this but I'll need to do more research later.

Subsequently, if I delete the last item out of the bound list the grid appears to error out also. I have a button bound on each row to delete it, but if you use it on the final element in the collection you get an index out of bounds error (I'm not personally deleting by the index, I'm removing the item from the collection by the object). I'll have to bring the source code down so I can step through it, I suspect it's the same issue.

I've patched the code previously to work but I'm a little nervous about submitting it without knowledge of the full breadth of use cases (e.g. I fixed it and it worked for my cases but I wasn't confident about breaking other people stuff because, mainly because this is my first usage of the grid and I'm by in large not aware of how others use it and the implications of what my fix might do to someone else's code or whether my fix was the ideal way to fix it).

[blakepell]

Another fix I tried that worked was altering the "void RemoveFromView" in AdvancedCollectionView.cs and moving "_view.RemoteAt(itemIndex) to be the last line in that function (again, not sure of the full implications or if this is where the fix should occur). The item was both deleted from the collection and it reflecting it on the grid without the ArgumentOutOfRangeException.

        private void RemoveFromView(int itemIndex, object item)
        {
            // The remove was here
            // _view.RemoveAt(itemIndex);

            if (itemIndex <= CurrentPosition)
            {
                CurrentPosition--;
            }

            var e = new VectorChangedEventArgs(CollectionChange.ItemRemoved, itemIndex, item);
            OnVectorChanged(e);

            // Moving it to here fixes the behavior of an exception being thrown when the last item is
            // either filtered off or removed.  I'm unsure if this will cause issues in OnVectorChanged in
            // other use cases.
            _view.RemoveAt(itemIndex);
        }

[harinikmsft]

@michael-hawker this does not seem to be a DataGrid issue but likely an issue with AdvancedCollectionView. Can you take a look?

[michael-hawker]

@blakepell the contract for the VectorChangedEventArgs should be firing after the element has already been removed. So the AdvancedCollectionView code should be correct here.

@HerrickSpencer is setting up a repro without the ACV and just a straight IObservableVector implementation. ObservableCollection appears to work fine as an ItemSource as it doesn't have a VectorChanged event.

[HerrickSpencer]

Created a branch for this Issue. seems to be in DataGrid events when the notify data source event fires on the vector changed event. The code requests the deleted item after it has been removed. OutOfRange when this is the last item, but always wrong otherwise.

Attempted to use ObservableVector instead of ACV to prove it is not the ACV, but using OV will also cause an exception in setting the itemsource of the datagrid (I will add a bug for this too)

[Sasino97]

I'm getting the following issue when deleting item 0 from an ACV bound to a (community toolkit) DataGrid:

System.ArgumentOutOfRangeException: Index was out of range. Must be non-negative and less than the size of the collection.
Parameter name: index
   at Microsoft.Toolkit.Uwp.UI.AdvancedCollectionView.get_Item(Int32 index)
   at Microsoft.Toolkit.Uwp.UI.Controls.DataGridInternals.DataGridDataConnection.NotifyingDataSource_VectorChanged(IObservableVector`1 sender, IVectorChangedEventArgs e)
   at Microsoft.Toolkit.Uwp.UI.AdvancedCollectionView.OnVectorChanged(IVectorChangedEventArgs e)
   at Microsoft.Toolkit.Uwp.UI.AdvancedCollectionView.SourceNcc_CollectionChanged(Object sender, NotifyCollectionChangedEventArgs e)
   at System.Collections.ObjectModel.ObservableCollection`1.OnCollectionChanged(NotifyCollectionChangedEventArgs e)

[ghost]

Thanks blakepell for patiently waiting while the team is still investigating the issue.

@anawishnoff @RBrid can we please have an update on this issue? Thanks

[ghost]

Thanks blakepell for patiently waiting while the team is still investigating the issue.

@anawishnoff @RBrid can we please have an update on this issue? Thanks

[ghost]

Thanks blakepell for patiently waiting while the team is still investigating the issue.

@anawishnoff @RBrid can we please have an update on this issue? Thanks

[ghost]

Thanks blakepell for patiently waiting while the team is still investigating the issue.

@anawishnoff @RBrid can we please have an update on this issue? Thanks

[djonasdev]

I can reproduce this issue.. are there specific reasons why the PRs are not merged?

[blakepell]

I'm not sure there is a pull request (not from me anyway). I did have something working based off of the snippet above, but I ended up porting my app to WPF when .NET Core started supporting it. The fix I used worked flawlessly (for me) but without a broad perspective of how this class is used elsewhere I didn't have the confidence say it couldn't wreak havoc for someone else. As I recall, I ended up pulling the code and having basically a shadow copy of it in my project I used. Obviously, that's not ideal but it definitely worked.

[sungaila]

I ran into the same issue. CommunityToolkit.WinUI.UI.Controls.DataGrid and CommunityToolkit.WinUI.UI.AdvancedCollectionView cannot be used together when items are removed or filtered.

I am using @blakepell's suggested hack (#2913 (comment)) as a workaround right now. However, this hack only works for DataGrid and will break normal controls like ListView.

Now I have the original and a code duplicate of AdvancedCollectionView in my app because the same collection must be bound by DataGrid and ListView.

[michael-hawker]

Not sure if this similar scenario was fixed by this PR? CommunityToolkit/Windows#309

We released an 8.1-rc to NuGet the other week, if someone wants to check this issue, we can move and close it if it is resolved now. Thanks.

[sungaila]

@michael-hawker I think this PR is unrelated. Not AdvancedCollectionView is broken, DataGrid is (as correctly predicted by @HerrickSpencer).

By the way: is DataGrid abandonend or just not moved to the new repo yet? The WinUI 3 gallery links to the CommunityToolkit gallery but it doesn't mention DataGrid at all.