Skip to content

Trying to remove item from BindableListView causes ArgumentOutOfRangeException #11

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.

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

Closed
CunningFox146 opened this issue May 17, 2023 · 9 comments
Assignees
Labels
bug Something isn't working

Comments

@CunningFox146
Copy link
Contributor

CunningFox146 commented May 17, 2023

I've made a BindableListView that has some items that should get added or removed. When trying to remove item from list or calling Clrear on collection, I get ArgumentOutOfRangeException. My ViewModel class:

using System;
using System.Collections.ObjectModel;
using System.Collections.Specialized;
using ArPaint.Services.Draw;
using ArPaint.UI.Systems.Stack;
using ArPaint.UI.Views.DrawingInfo;
using ArPaint.Utils;
using UnityMvvmToolkit.Core;
using UnityMvvmToolkit.Core.Attributes;
using UnityMvvmToolkit.Core.Interfaces;

namespace ArPaint.UI.ViewModels.Home
{
    public class HomeViewModel : ViewModel
    {
        private readonly IDrawingsProvider _drawingsProvider;

        [Observable(nameof(Drawings))]
        private readonly IProperty<ObservableCollection<DrawingViewModel>> _drawings;

        public ObservableCollection<DrawingViewModel> Drawings => _drawings.Value;
        
        public ICommand CreateDrawingCommand { get; }

        public HomeViewModel(IDrawingsProvider drawingsProvider)
        {
            _drawingsProvider = drawingsProvider;

            CreateDrawingCommand = new Command(CreateDrawing);
            
            _drawings = new Property<ObservableCollection<DrawingViewModel>>(new ObservableCollection<DrawingViewModel>());
            _drawingsProvider.Drawings.CollectionChanged += OnDrawingsChanged;
            BuildDrawingsCollection();
        }

        private void OnDrawingsChanged(object sender, NotifyCollectionChangedEventArgs evt)
        {
            switch (evt.Action)
            {
                case NotifyCollectionChangedAction.Add:
                    foreach (var item in evt.NewItems)
                    {
                        if (item is DrawingData drawing)
                            Drawings.Add(new DrawingViewModel(drawing, SelectDrawing));
                    }
                    break;
                case NotifyCollectionChangedAction.Remove:
                    foreach (var item in evt.OldItems)
                    {
                        if (item is DrawingData drawing)
                            Drawings.RemoveAll(viewModel => viewModel.Drawing == drawing);
                    }
                    break;
                default:
                    break;
            }
        }

        private void BuildDrawingsCollection()
        {
            foreach (var drawing in _drawingsProvider.Drawings)
            {
                Drawings.Add(new DrawingViewModel(drawing, SelectDrawing));
            }
        }

        private void CreateDrawing()
        {
            _drawingsProvider.SelectDrawing(null);
            ViewStack.PushView<DrawingInfoView>();
        }

        private void SelectDrawing(DrawingData drawing)
        {
            _drawingsProvider.SelectDrawing(drawing);
            ViewStack.PushView<DrawingInfoView>();
        }
    }
}
@CunningFox146 CunningFox146 added the bug Something isn't working label May 17, 2023
@CunningFox146 CunningFox146 closed this as not planned Won't fix, can't repro, duplicate, stale May 17, 2023
@CunningFox146 CunningFox146 reopened this May 17, 2023
@CunningFox146
Copy link
Contributor Author

Full trace:

ArgumentOutOfRangeException: Index was out of range. Must be non-negative and less than the size of the collection.
Parameter name: index
System.Collections.Generic.List`1[T].get_Item (System.Int32 index) (at <27586baf39bf4babbfd8a2caabe8e228>:0)
System.Collections.ObjectModel.Collection`1[T].get_Item (System.Int32 index) (at <27586baf39bf4babbfd8a2caabe8e228>:0)
UnityMvvmToolkit.UITK.BindableUIElements.BindableListView`1[TItemBindingContext].UnbindItem (UnityEngine.UIElements.VisualElement item, System.Int32 index) (at ./Library/PackageCache/com.chebanovdd.unitymvvmtoolkit@1.0.1/Runtime/UITK/BindableUIElements/BindableListView.cs:93)
UnityEngine.UIElements.ListViewController.UnbindItem (UnityEngine.UIElements.VisualElement element, System.Int32 index) (at <8afd0ed9f16a4f65b41dafeb31525d3d>:0)
UnityEngine.UIElements.CollectionViewController.InvokeUnbindItem (UnityEngine.UIElements.ReusableCollectionItem reusableItem, System.Int32 index) (at <8afd0ed9f16a4f65b41dafeb31525d3d>:0)
UnityEngine.UIElements.VerticalVirtualizationController`1[T].ReleaseItem (System.Int32 activeItemsIndex) (at <8afd0ed9f16a4f65b41dafeb31525d3d>:0)
UnityEngine.UIElements.DynamicHeightVirtualizationController`1[T].ReleaseItem (System.Int32 activeItemsIndex) (at <8afd0ed9f16a4f65b41dafeb31525d3d>:0)
UnityEngine.UIElements.VerticalVirtualizationController`1[T].Refresh (System.Boolean rebuild) (at <8afd0ed9f16a4f65b41dafeb31525d3d>:0)
UnityEngine.UIElements.DynamicHeightVirtualizationController`1[T].Refresh (System.Boolean rebuild) (at <8afd0ed9f16a4f65b41dafeb31525d3d>:0)
UnityEngine.UIElements.BaseVerticalCollectionView.RefreshItems () (at <8afd0ed9f16a4f65b41dafeb31525d3d>:0)
UnityMvvmToolkit.UITK.BindableUIElements.BindableListView`1[TItemBindingContext].OnItemsCollectionChanged (System.Object sender, System.Collections.Specialized.NotifyCollectionChangedEventArgs e) (at ./Library/PackageCache/com.chebanovdd.unitymvvmtoolkit@1.0.1/Runtime/UITK/BindableUIElements/BindableListView.cs:98)
System.Collections.ObjectModel.ObservableCollection`1[T].OnCollectionChanged (System.Collections.Specialized.NotifyCollectionChangedEventArgs e) (at <ea6e75a573bd4813a6e91970c73a6145>:0)
System.Collections.ObjectModel.ObservableCollection`1[T].OnCollectionChanged (System.Collections.Specialized.NotifyCollectionChangedAction action, System.Object item, System.Int32 index) (at <ea6e75a573bd4813a6e91970c73a6145>:0)
System.Collections.ObjectModel.ObservableCollection`1[T].RemoveItem (System.Int32 index) (at <ea6e75a573bd4813a6e91970c73a6145>:0)
System.Collections.ObjectModel.Collection`1[T].RemoveAt (System.Int32 index) (at <27586baf39bf4babbfd8a2caabe8e228>:0)
ArPaint.Utils.CollectionExtensions.RemoveAll[T] (System.Collections.ObjectModel.ObservableCollection`1[T] coll, System.Func`2[T,TResult] condition) (at Assets/Scripts/Utils/CollectionExtensions.cs:25)
ArPaint.UI.ViewModels.Home.HomeViewModel.OnDrawingsChanged (System.Object sender, System.Collections.Specialized.NotifyCollectionChangedEventArgs evt) (at Assets/Scripts/UI/ViewModels/Home/HomeViewModel.cs:49)
System.Collections.ObjectModel.ObservableCollection`1[T].OnCollectionChanged (System.Collections.Specialized.NotifyCollectionChangedEventArgs e) (at <ea6e75a573bd4813a6e91970c73a6145>:0)
System.Collections.ObjectModel.ObservableCollection`1[T].OnCollectionChanged (System.Collections.Specialized.NotifyCollectionChangedAction action, System.Object item, System.Int32 index) (at <ea6e75a573bd4813a6e91970c73a6145>:0)
System.Collections.ObjectModel.ObservableCollection`1[T].RemoveItem (System.Int32 index) (at <ea6e75a573bd4813a6e91970c73a6145>:0)
System.Collections.ObjectModel.Collection`1[T].Remove (T item) (at <27586baf39bf4babbfd8a2caabe8e228>:0)
ArPaint.Services.Draw.DrawingsProvider.RemoveData (ArPaint.Services.Draw.DrawingData data) (at Assets/Scripts/Services/Draw/DrawingsProvider.cs:47)
ArPaint.UI.ViewModels.DrawingInfoViewModel.Delete () (at Assets/Scripts/UI/ViewModels/DrawingInfoViewModel.cs:62)
UnityMvvmToolkit.Core.Command.Execute () (at ./Library/PackageCache/com.chebanovdd.unitymvvmtoolkit@1.0.1/Runtime/Core/Command.cs:17)
UnityMvvmToolkit.Core.Interfaces.ICommand.UnityMvvmToolkit.Core.Interfaces.IBaseCommand.Execute (System.Int32 elementId) (at ./Library/PackageCache/com.chebanovdd.unitymvvmtoolkit@1.0.1/Runtime/Core/Interfaces/ICommand.cs:9)
UnityMvvmToolkit.UITK.BindableUIElements.BindableButton.OnButtonClicked () (at ./Library/PackageCache/com.chebanovdd.unitymvvmtoolkit@1.0.1/Runtime/UITK/BindableUIElements/BindableButton.cs:54)
UnityEngine.UIElements.Clickable.Invoke (UnityEngine.UIElements.EventBase evt) (at <8afd0ed9f16a4f65b41dafeb31525d3d>:0)
UnityEngine.UIElements.Clickable.ProcessUpEvent (UnityEngine.UIElements.EventBase evt, UnityEngine.Vector2 localPosition, System.Int32 pointerId) (at <8afd0ed9f16a4f65b41dafeb31525d3d>:0)
UnityEngine.UIElements.Clickable.OnMouseUp (UnityEngine.UIElements.MouseUpEvent evt) (at <8afd0ed9f16a4f65b41dafeb31525d3d>:0)
UnityEngine.UIElements.EventCallbackFunctor`1[TEventType].Invoke (UnityEngine.UIElements.EventBase evt, UnityEngine.UIElements.PropagationPhase propagationPhase) (at <8afd0ed9f16a4f65b41dafeb31525d3d>:0)
UnityEngine.UIElements.EventCallbackRegistry.InvokeCallbacks (UnityEngine.UIElements.EventBase evt, UnityEngine.UIElements.PropagationPhase propagationPhase) (at <8afd0ed9f16a4f65b41dafeb31525d3d>:0)
UnityEngine.UIElements.CallbackEventHandler.HandleEvent (UnityEngine.UIElements.EventBase evt) (at <8afd0ed9f16a4f65b41dafeb31525d3d>:0)
UnityEngine.UIElements.CallbackEventHandler.HandleEventAtCurrentTargetAndPhase (UnityEngine.UIElements.EventBase evt) (at <8afd0ed9f16a4f65b41dafeb31525d3d>:0)
UnityEngine.UIElements.CallbackEventHandler.UnityEngine.UIElements.IEventHandler.HandleEvent (UnityEngine.UIElements.EventBase evt) (at <8afd0ed9f16a4f65b41dafeb31525d3d>:0)
UnityEngine.UIElements.EventDispatchUtilities.HandleEventAcrossPropagationPath (UnityEngine.UIElements.EventBase evt) (at <8afd0ed9f16a4f65b41dafeb31525d3d>:0)
UnityEngine.UIElements.EventDispatchUtilities.PropagateEvent (UnityEngine.UIElements.EventBase evt) (at <8afd0ed9f16a4f65b41dafeb31525d3d>:0)
UnityEngine.UIElements.MouseEventDispatchingStrategy.SendEventToRegularTarget (UnityEngine.UIElements.EventBase evt, UnityEngine.UIElements.BaseVisualElementPanel panel) (at <8afd0ed9f16a4f65b41dafeb31525d3d>:0)
UnityEngine.UIElements.MouseEventDispatchingStrategy.SendEventToTarget (UnityEngine.UIElements.EventBase evt, UnityEngine.UIElements.BaseVisualElementPanel panel) (at <8afd0ed9f16a4f65b41dafeb31525d3d>:0)
UnityEngine.UIElements.MouseEventDispatchingStrategy.DispatchEvent (UnityEngine.UIElements.EventBase evt, UnityEngine.UIElements.IPanel iPanel) (at <8afd0ed9f16a4f65b41dafeb31525d3d>:0)
UnityEngine.UIElements.EventDispatcher.ApplyDispatchingStrategies (UnityEngine.UIElements.EventBase evt, UnityEngine.UIElements.IPanel panel, System.Boolean imguiEventIsInitiallyUsed) (at <8afd0ed9f16a4f65b41dafeb31525d3d>:0)
UnityEngine.UIElements.EventDispatcher.ProcessEvent (UnityEngine.UIElements.EventBase evt, UnityEngine.UIElements.IPanel panel) (at <8afd0ed9f16a4f65b41dafeb31525d3d>:0)
UnityEngine.UIElements.EventDispatcher.ProcessEventQueue () (at <8afd0ed9f16a4f65b41dafeb31525d3d>:0)
UnityEngine.UIElements.EventDispatcher.OpenGate () (at <8afd0ed9f16a4f65b41dafeb31525d3d>:0)
UnityEngine.UIElements.EventDispatcherGate.Dispose () (at <8afd0ed9f16a4f65b41dafeb31525d3d>:0)
UnityEngine.UIElements.EventDispatcher.ProcessEvent (UnityEngine.UIElements.EventBase evt, UnityEngine.UIElements.IPanel panel) (at <8afd0ed9f16a4f65b41dafeb31525d3d>:0)
UnityEngine.UIElements.EventDispatcher.Dispatch (UnityEngine.UIElements.EventBase evt, UnityEngine.UIElements.IPanel panel, UnityEngine.UIElements.DispatchMode dispatchMode) (at <8afd0ed9f16a4f65b41dafeb31525d3d>:0)
UnityEngine.UIElements.BaseVisualElementPanel.SendEvent (UnityEngine.UIElements.EventBase e, UnityEngine.UIElements.DispatchMode dispatchMode) (at <8afd0ed9f16a4f65b41dafeb31525d3d>:0)
UnityEngine.UIElements.VisualElement.SendEvent (UnityEngine.UIElements.EventBase e) (at <8afd0ed9f16a4f65b41dafeb31525d3d>:0)
UnityEngine.UIElements.DefaultEventSystem.SendPositionBasedEvent[TArg] (UnityEngine.Vector3 mousePosition, UnityEngine.Vector3 delta, System.Int32 pointerId, System.Nullable`1[T] targetDisplay, System.Func`4[T1,T2,T3,TResult] evtFactory, TArg arg, System.Boolean deselectIfNoTarget) (at <8afd0ed9f16a4f65b41dafeb31525d3d>:0)
UnityEngine.UIElements.DefaultEventSystem.ProcessMouseEvents () (at <8afd0ed9f16a4f65b41dafeb31525d3d>:0)
UnityEngine.UIElements.DefaultEventSystem.Update (UnityEngine.UIElements.DefaultEventSystem+UpdateMode updateMode) (at <8afd0ed9f16a4f65b41dafeb31525d3d>:0)
UnityEngine.UIElements.UIElementsRuntimeUtility.UpdateRuntimePanels () (at <8afd0ed9f16a4f65b41dafeb31525d3d>:0)
UnityEngine.UIElements.UIElementsRuntimeUtilityNative.UpdateRuntimePanels () (at <8afd0ed9f16a4f65b41dafeb31525d3d>:0)

@ChebanovDD
Copy link
Collaborator

ChebanovDD commented May 18, 2023

Hi. Could you share your BindableListView and Drawings.RemoveAll implementations?

@ChebanovDD
Copy link
Collaborator

ChebanovDD commented May 18, 2023

And why don't you just directly use the _drawingsProvider.Drawings collection?

public class HomeViewModel : ViewModel
{
    [Observable(nameof(Drawings))]
    private readonly IReadOnlyProperty<ObservableCollection<DrawingViewModel>> _drawings;

    public ObservableCollection<DrawingViewModel> Drawings => _drawings.Value;

    public HomeViewModel(IDrawingsProvider drawingsProvider)
    {
        _drawingsProvider = drawingsProvider;
        _drawings = new ReadOnlyProperty<ObservableCollection<DrawingViewModel>>(_drawingsProvider.Drawings);
    }

    ....
}

If you don't use the Drawings property in your code, you can remove it.

public class HomeViewModel : ViewModel
{
    // The field name will be used if you don't provide a property name.
    // Names '_drawings' and 'm_drawings' will be auto-converted to 'Drawings'.
    [Observable]
    private readonly IReadOnlyProperty<ObservableCollection<DrawingViewModel>> _drawings;

    public HomeViewModel(IDrawingsProvider drawingsProvider)
    {
        _drawingsProvider = drawingsProvider;
        _drawings = new ReadOnlyProperty<ObservableCollection<DrawingViewModel>>(_drawingsProvider.Drawings);
    }

    ....
}

@CunningFox146
Copy link
Contributor Author

CunningFox146 commented May 18, 2023

Thanks for the fast reply! I'll think of a way of refactoring my code to use ViewModels collection from the start then. My RemoveAll implementation:

public static int RemoveAll<T>(
            this ObservableCollection<T> coll, Func<T, bool> condition)
        {
            var itemsToRemove = coll.Where(condition).ToList();

            foreach (var itemToRemove in itemsToRemove) coll.Remove(itemToRemove);

            return itemsToRemove.Count;
        }

My DrawvingViewModel:

using System;
using ArPaint.Services.Draw;
using UnityMvvmToolkit.Common.Interfaces;
using UnityMvvmToolkit.Core;
using UnityMvvmToolkit.Core.Attributes;
using UnityMvvmToolkit.Core.Interfaces;

namespace ArPaint.UI.ViewModels.Home
{
    public class DrawingViewModel : ICollectionItem
    {

        [Observable(nameof(DrawingName))]
        private readonly IProperty<string> _drawingName;

        [Observable(nameof(DrawingDescription))]
        private readonly IProperty<string> _drawingDescription;

        private readonly Action<DrawingData> _selectDrawing;

        public int Id { get; }
        public DrawingData Drawing { get; }
        public ICommand SelectDrawingCommand { get; }
        
        public string DrawingName
        {
            get => _drawingName.Value;
            set => _drawingName.Value = value;
        }
        
        public string DrawingDescription
        {
            get => _drawingDescription.Value;
            set => _drawingDescription.Value = value;
        }
        
        public DrawingViewModel(DrawingData drawing, Action<DrawingData> selectDrawing)
        {
            Drawing = drawing;
            _selectDrawing = selectDrawing;

            _drawingName = new Property<string>(Drawing.Name);
            _drawingDescription = new Property<string>(Drawing.Description);
            
            Id = new Guid().GetHashCode();
            SelectDrawingCommand = new Command(SelectDrawing);

            Drawing.ItemUpdate += Update;
        }

        private void Update()
        {
            DrawingName = Drawing.Name;
            DrawingDescription = Drawing.Description;
        }

        private void SelectDrawing()
        {
            _selectDrawing?.Invoke(Drawing);
        }
    }
}

And my DrawingListView:

using ArPaint.UI.ViewModels.Draw;
using ArPaint.UI.ViewModels.Home;
using UnityEngine.UIElements;
using UnityMvvmToolkit.UITK.BindableUIElements;

namespace ArPaint.UI.Views.Home
{
    public class DrawingsListView : BindableListView<DrawingViewModel>
    {
        public new class UxmlFactory : UxmlFactory<DrawingsListView, UxmlTraits>
        {
            public override string uxmlName => nameof(DrawingsListView);
        }
    }
}

@CunningFox146
Copy link
Contributor Author

CunningFox146 commented May 18, 2023

Small update: made a test script that uses ViewModels list from the start, I still get the same exception

public HomeViewModel(IDrawingsProvider drawingsProvider)
        {
            _drawingsProvider = drawingsProvider;
            _drawings = new ReadOnlyProperty<ObservableCollection<DrawingViewModel>>(BuildDrawingsCollection());
            Test();
        }

        private async void Test()
        {
            for (var i = 0; i < 5; i++)
            {
                await Task.Delay(1000);
                _drawings.Value.Remove(_drawings.Value.FirstOrDefault());
            }
        }

        private ObservableCollection<DrawingViewModel> BuildDrawingsCollection()
        {
            var items = new ObservableCollection<DrawingViewModel>();
            foreach (var drawing in _drawingsProvider.Drawings)
            {
                items.Add(new DrawingViewModel(drawing, SelectDrawing));
            }

            return items;
        }

P.S I've started getting the same exception after adding new items. (First time adding item to list works fine, second one causes exception)

@CunningFox146
Copy link
Contributor Author

Made empty project and made test scripts that add and remove data. On remove throws exception too. Unity package:
mvvm-test.zip

@ChebanovDD
Copy link
Collaborator

Yes, I found the problem. Unity 2022 calls the bind and unbind methods on removed elements for some reason. I will fix this later today.

Thank you.

@CunningFox146
Copy link
Contributor Author

Update: made a fix:
#13

@CunningFox146
Copy link
Contributor Author

CunningFox146 commented May 18, 2023

Yes, I found the problem. Unity 2022 calls the bind and unbind methods on removed elements for some reason. I will fix this later today.

Thank you.

Oh, haven't seen your reply 😄
Thanks!

ChebanovDD added a commit that referenced this issue May 18, 2023
Fix #11. ArgumentOutOfRange when deleting item from BindableListView in Unity 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants