From df66dd4b1db44852a146c38a616158ab777816ce Mon Sep 17 00:00:00 2001 From: Roland Pheasant Date: Tue, 5 Dec 2023 05:44:56 -0500 Subject: [PATCH] Remove some index -1 exceptions (#781) * Include index =0 for inital change set on source list. Fixes #723 * Add clone of AvaloniaDictionary to the test project for testing * Add tests for Avalonia + throw the dreaded -1 exception. Fixes #710. Fixes #683 -> hopefully ! * Dear Editor.Config, you've got to be kidding me, Telling me I should mark something with [Serializable] when that was a bad idea in 2004 and is totally evil in 2023. Binary formatting support required?? Please grow up and remove this as a suggestion to spare people the problem of having to turn it off. * Add comment regarding avalonia dictionarytest * Use UnspecifiedIndexException --- .editorconfig | 2 + .../Binding/AvaloniaDictionaryFixture.cs | 311 ++++++++++++++++++ .../List/SourceListFixture.cs | 24 ++ .../Binding/ObservableCollectionEx.cs | 43 ++- src/DynamicData/List/SourceList.cs | 2 +- 5 files changed, 376 insertions(+), 6 deletions(-) create mode 100644 src/DynamicData.Tests/Binding/AvaloniaDictionaryFixture.cs create mode 100644 src/DynamicData.Tests/List/SourceListFixture.cs diff --git a/.editorconfig b/.editorconfig index 0454e2555..9881231de 100644 --- a/.editorconfig +++ b/.editorconfig @@ -29,6 +29,7 @@ dotnet_style_prefer_compound_assignment = true:suggestion dotnet_style_prefer_simplified_interpolation = true:suggestion dotnet_style_namespace_match_folder = true:suggestion dotnet_style_prefer_collection_expression = true:suggestion +dotnet_diagnostic.CA2237.severity = none [project.json] indent_size = 2 @@ -699,6 +700,7 @@ csharp_style_namespace_declarations = block_scoped:silent csharp_style_prefer_method_group_conversion = true:silent csharp_style_prefer_top_level_statements = true:silent csharp_style_prefer_primary_constructors = true:suggestion +dotnet_diagnostic.RCS1194.severity = none # C++ Files [*.{cpp,h,in}] diff --git a/src/DynamicData.Tests/Binding/AvaloniaDictionaryFixture.cs b/src/DynamicData.Tests/Binding/AvaloniaDictionaryFixture.cs new file mode 100644 index 000000000..f069592f4 --- /dev/null +++ b/src/DynamicData.Tests/Binding/AvaloniaDictionaryFixture.cs @@ -0,0 +1,311 @@ +using System; +using System.Collections; +using System.Collections.Generic; +using System.Collections.Specialized; +using System.ComponentModel; +using System.Diagnostics.CodeAnalysis; +using System.Linq; + +using DynamicData.Binding; +using DynamicData.Tests.Domain; +using FluentAssertions; +using Xunit; + +namespace DynamicData.Tests.Binding; + +public class AvaloniaDictionaryFixture +{ + private readonly AvaloniaDictionary _collection; + private readonly ChangeSetAggregator _results; + + public AvaloniaDictionaryFixture() + { + _collection = new AvaloniaDictionary(); + _results = _collection.ToObservableChangeSet, KeyValuePair>() + .Transform(x=>x.Value) + .AsAggregator(); + } + + [Fact] + public void Add() + { + var person = new Person("Someone",10, "M"); + + _collection.Add("Someone", person); + + _results.Messages.Count.Should().Be(1); + _results.Data.Count.Should().Be(1); + _results.Data.Items.First().Should().Be(person); + } + + [Fact] + public void Replace() + { + var person1 = new Person("Someone", 10, "M"); + var person2 = new Person("Someone", 11, "M"); + + _collection.Add("Someone", person1); + _collection["Someone"] = person2; + + _results.Data.Count.Should().Be(1); + _results.Data.Items.First().Should().Be(person2); + } + + + [Fact] + public void Remove() + { + var person = new Person("Someone", 10, "M"); + + _collection.Add("Someone", person); + _collection.Remove(person.Key); + + _results.Data.Count.Should().Be(0); + } +} + + +public interface IAvaloniaDictionary + : IDictionary, + IAvaloniaReadOnlyDictionary, + IDictionary + where TKey : notnull +{ +} + + +public interface IAvaloniaReadOnlyDictionary + : IReadOnlyDictionary, + INotifyCollectionChanged, + INotifyPropertyChanged + where TKey : notnull +{ +} + + +/* + Copied from Avalionia because an issue was raised due to compatibility issues with ToObservableChangeSet(). + +There's not other way of testing it. + +See https://github.com/AvaloniaUI/Avalonia/blob/d7c82a1a6f7eb95b2214f20a281fa0581fb7b792/src/Avalonia.Base/Collections/AvaloniaDictionary.cs#L17 + */ +public class AvaloniaDictionary : IAvaloniaDictionary where TKey : notnull +{ + private Dictionary _inner; + + /// + /// Initializes a new instance of the class. + /// + public AvaloniaDictionary() + { + _inner = new Dictionary(); + } + + /// + /// Initializes a new instance of the class. + /// + public AvaloniaDictionary(int capacity) + { + _inner = new Dictionary(capacity); + } + + /// + /// Occurs when the collection changes. + /// + public event NotifyCollectionChangedEventHandler? CollectionChanged; + + /// + /// Raised when a property on the collection changes. + /// + public event PropertyChangedEventHandler? PropertyChanged; + + /// + public int Count => _inner.Count; + + /// + public bool IsReadOnly => false; + + /// + public ICollection Keys => _inner.Keys; + + /// + public ICollection Values => _inner.Values; + + bool IDictionary.IsFixedSize => ((IDictionary)_inner).IsFixedSize; + + ICollection IDictionary.Keys => ((IDictionary)_inner).Keys; + + ICollection IDictionary.Values => ((IDictionary)_inner).Values; + + bool ICollection.IsSynchronized => ((IDictionary)_inner).IsSynchronized; + + object ICollection.SyncRoot => ((IDictionary)_inner).SyncRoot; + + IEnumerable IReadOnlyDictionary.Keys => _inner.Keys; + + IEnumerable IReadOnlyDictionary.Values => _inner.Values; + + /// + /// Gets or sets the named resource. + /// + /// The resource key. + /// The resource, or null if not found. + public TValue this[TKey key] + { + get + { + return _inner[key]; + } + + set + { + bool replace = _inner.TryGetValue(key, out var old); + _inner[key] = value; + + if (replace) + { + PropertyChanged?.Invoke(this, new PropertyChangedEventArgs($"{CommonPropertyNames.IndexerName}[{key}]")); + + if (CollectionChanged != null) + { + var e = new NotifyCollectionChangedEventArgs( + NotifyCollectionChangedAction.Replace, + new KeyValuePair(key, value), + new KeyValuePair(key, old!)); + CollectionChanged(this, e); + } + } + else + { + NotifyAdd(key, value); + } + } + } + + object? IDictionary.this[object key] { get => ((IDictionary)_inner)[key]; set => ((IDictionary)_inner)[key] = value; } + + /// + public void Add(TKey key, TValue value) + { + _inner.Add(key, value); + NotifyAdd(key, value); + } + + /// + public void Clear() + { + var old = _inner; + + _inner = new Dictionary(); + + PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(Count))); + PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(CommonPropertyNames.IndexerName)); + + + if (CollectionChanged != null) + { + var e = new NotifyCollectionChangedEventArgs( + NotifyCollectionChangedAction.Remove, + old.ToArray(), + -1); + CollectionChanged(this, e); + } + } + + /// + public bool ContainsKey(TKey key) => _inner.ContainsKey(key); + + /// + public void CopyTo(KeyValuePair[] array, int arrayIndex) + { + ((IDictionary)_inner).CopyTo(array, arrayIndex); + } + + /// + public IEnumerator> GetEnumerator() => _inner.GetEnumerator(); + + /// + public bool Remove(TKey key) + { + if (_inner.TryGetValue(key, out var value)) + { + _inner.Remove(key); + PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(Count))); + PropertyChanged?.Invoke(this, new PropertyChangedEventArgs($"{CommonPropertyNames.IndexerName}[{key}]")); + + if (CollectionChanged != null) + { + var e = new NotifyCollectionChangedEventArgs( + NotifyCollectionChangedAction.Remove, + new[] { new KeyValuePair(key, value) }, + -1); + CollectionChanged(this, e); + } + + return true; + } + else + { + return false; + } + } + + /// + public bool TryGetValue(TKey key, [MaybeNullWhen(false)] out TValue value) => _inner.TryGetValue(key, out value); + /// + IEnumerator IEnumerable.GetEnumerator() => _inner.GetEnumerator(); + + /// + void ICollection.CopyTo(Array array, int index) => ((ICollection)_inner).CopyTo(array, index); + + /// + void ICollection>.Add(KeyValuePair item) + { + Add(item.Key, item.Value); + } + + /// + bool ICollection>.Contains(KeyValuePair item) + { + return _inner.Contains(item); + } + + /// + bool ICollection>.Remove(KeyValuePair item) + { + return Remove(item.Key); + } + + /// + void IDictionary.Add(object key, object? value) => Add((TKey)key, (TValue)value!); + + /// + bool IDictionary.Contains(object key) => ((IDictionary)_inner).Contains(key); + + /// + IDictionaryEnumerator IDictionary.GetEnumerator() => ((IDictionary)_inner).GetEnumerator(); + + /// + void IDictionary.Remove(object key) => Remove((TKey)key); + + private void NotifyAdd(TKey key, TValue value) + { + PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(nameof(Count))); + PropertyChanged?.Invoke(this, new PropertyChangedEventArgs($"{CommonPropertyNames.IndexerName}[{key}]")); + + + if (CollectionChanged != null) + { + var e = new NotifyCollectionChangedEventArgs( + NotifyCollectionChangedAction.Add, + new[] { new KeyValuePair(key, value) }, + -1); + CollectionChanged(this, e); + } + } +} +internal static class CommonPropertyNames +{ + public const string IndexerName = "Item"; +} diff --git a/src/DynamicData.Tests/List/SourceListFixture.cs b/src/DynamicData.Tests/List/SourceListFixture.cs new file mode 100644 index 000000000..dc37bdaed --- /dev/null +++ b/src/DynamicData.Tests/List/SourceListFixture.cs @@ -0,0 +1,24 @@ +using System; +using System.Collections.Generic; +using System.Linq; +using FluentAssertions; +using Xunit; + +namespace DynamicData.Tests.List; + +public class SourceListFixture +{ + [Fact] + public void InitialChangeIsRange() + { + var source = new SourceList(); + source.Add("A"); + var changeSets = new List>(); + + source.Connect().Subscribe(changeSets.Add).Dispose(); + + + changeSets[0].First().Type.Should().Be(ChangeType.Range); + changeSets[0].First().Range.Index.Should().Be(0); + } +} diff --git a/src/DynamicData/Binding/ObservableCollectionEx.cs b/src/DynamicData/Binding/ObservableCollectionEx.cs index ccb137368..938847ec9 100644 --- a/src/DynamicData/Binding/ObservableCollectionEx.cs +++ b/src/DynamicData/Binding/ObservableCollectionEx.cs @@ -6,6 +6,7 @@ using System.Collections.Specialized; using System.Reactive; using System.Reactive.Linq; +using DynamicData.Kernel; namespace DynamicData.Binding; @@ -156,13 +157,17 @@ public static IObservable> ToObservableChangeSet(t { case NotifyCollectionChangedAction.Add when changes.NewItems is not null: { + var newIndex = changes.NewStartingIndex == -1 + ? list.Count + : changes.NewStartingIndex; + if (changes.NewItems.Count == 1 && changes.NewItems[0] is T item) { - list.Insert(changes.NewStartingIndex, item); + list.Insert(newIndex, item); } else { - list.InsertRange(changes.NewItems.Cast(), changes.NewStartingIndex); + list.InsertRange(changes.NewItems.Cast(), newIndex); } break; @@ -170,7 +175,14 @@ public static IObservable> ToObservableChangeSet(t case NotifyCollectionChangedAction.Remove when changes.OldItems is not null: { - if (changes.OldItems.Count == 1) + if (changes.OldStartingIndex == -1) + { + foreach (var item in changes.OldItems.Cast()) + { + list.Remove(item); + } + } + else if (changes.OldItems.Count == 1) { list.RemoveAt(changes.OldStartingIndex); } @@ -182,14 +194,35 @@ public static IObservable> ToObservableChangeSet(t break; } - case NotifyCollectionChangedAction.Replace when changes.NewItems is not null && changes.NewItems[0] is T replacedItem: - list[changes.NewStartingIndex] = replacedItem; + case NotifyCollectionChangedAction.Replace when changes.NewItems is not null && + changes.NewItems[0] is T replacedItem: + { + if (changes.NewStartingIndex == -1) + { + var original = changes.OldItems!.Cast().SingleOrDefault(); + var oldIndex = list.IndexOf(original!); + + list[oldIndex] = replacedItem; + } + else + { + list[changes.NewStartingIndex] = replacedItem; + } + } + break; case NotifyCollectionChangedAction.Reset: list.Clear(); list.AddRange(source); break; case NotifyCollectionChangedAction.Move: + + if (changes.OldStartingIndex == -1) + throw new UnspecifiedIndexException("Move -> OldStartingIndex"); + + if (changes.NewStartingIndex == -1) + throw new UnspecifiedIndexException("Move -> NewStartingIndex"); + list.Move(changes.OldStartingIndex, changes.NewStartingIndex); break; } diff --git a/src/DynamicData/List/SourceList.cs b/src/DynamicData/List/SourceList.cs index 8be553eb7..7494601f6 100644 --- a/src/DynamicData/List/SourceList.cs +++ b/src/DynamicData/List/SourceList.cs @@ -85,7 +85,7 @@ public IObservable> Connect(Func? predicate = null) observer.OnNext( new ChangeSet { - new(ListChangeReason.AddRange, _readerWriter.Items) + new(ListChangeReason.AddRange, _readerWriter.Items, 0) }); }