From 206ab27235c1e66658c7d4a1d847fb311f9a9788 Mon Sep 17 00:00:00 2001 From: CoderGamester Date: Wed, 20 Nov 2024 17:45:36 +0000 Subject: [PATCH 1/3] **Fix**: - Fixed the issues of *ObservableDictionary* when subscribing/unsubscribing to actions while removing/adding elements - Fixed the issues of *ObservableList* when subscribing/unsubscribing to actions while removing/adding elements --- CHANGELOG.md | 6 +++++ Runtime/ObservableDictionary.cs | 36 ++++++++++++++++++------------ Runtime/ObservableList.cs | 13 ++++++----- Tests/Editor/ObservableListTest.cs | 17 +++++++++++++- package.json | 2 +- 5 files changed, 52 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9a8ad5a..9c85b70 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,12 @@ All notable changes to this package will be documented in this file. The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/) and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html). +## [0.6.5] - 2024-11-20 + +**Fix**: +- Fixed the issues of *ObservableDictionary* when subscribing/unsubscribing to actions while removing/adding elements +- Fixed the issues of *ObservableList* when subscribing/unsubscribing to actions while removing/adding elements + ## [0.6.4] - 2024-11-13 **Fix**: diff --git a/Runtime/ObservableDictionary.cs b/Runtime/ObservableDictionary.cs index 5f44ce0..97a38f4 100644 --- a/Runtime/ObservableDictionary.cs +++ b/Runtime/ObservableDictionary.cs @@ -2,6 +2,7 @@ using System.Collections; using System.Collections.Generic; using System.Collections.ObjectModel; +using System.Linq; using UnityEngine.UIElements; // ReSharper disable once CheckNamespace @@ -275,16 +276,18 @@ public virtual bool Remove(TKey key) if (ObservableUpdateFlag != ObservableUpdateFlag.UpdateOnly && _keyUpdateActions.TryGetValue(key, out var actions)) { - for (var i = 0; i < actions.Count; i++) + var listCopy = actions.ToList(); + for (var i = 0; i < listCopy.Count; i++) { - actions[i](key, value, default, ObservableUpdateType.Removed); + listCopy[i](key, value, default, ObservableUpdateType.Removed); } } if (ObservableUpdateFlag != ObservableUpdateFlag.KeyUpdateOnly) { - for (var i = 0; i < _updateActions.Count; i++) + var listCopy = _updateActions.ToList(); + for (var i = 0; i < listCopy.Count; i++) { - _updateActions[i](key, value, default, ObservableUpdateType.Removed); + listCopy[i](key, value, default, ObservableUpdateType.Removed); } } @@ -294,31 +297,34 @@ public virtual bool Remove(TKey key) /// public virtual void Clear() { - var dictionary = new Dictionary(Dictionary); - - Dictionary.Clear(); - if (ObservableUpdateFlag != ObservableUpdateFlag.UpdateOnly) { - foreach (var data in _keyUpdateActions) + // Create a copy in case that one of the callbacks modifies the list (Ex: removing a subscriber) + var copy = new Dictionary>>(_keyUpdateActions); + + foreach (var data in copy) { - for (var i = 0; i < data.Value.Count; i++) + var listCopy = data.Value.ToList(); + for (var i = 0; i < listCopy.Count; i++) { - data.Value[i](data.Key, dictionary[data.Key], default, ObservableUpdateType.Removed); + listCopy[i](data.Key, Dictionary[data.Key], default, ObservableUpdateType.Removed); } } } if (ObservableUpdateFlag != ObservableUpdateFlag.KeyUpdateOnly) { - foreach (var data in dictionary) + foreach (var data in Dictionary) { - for (var i = 0; i < _updateActions.Count; i++) + var listCopy = _updateActions.ToList(); + for (var i = 0; i < listCopy.Count; i++) { - _updateActions[i](data.Key, data.Value, default, ObservableUpdateType.Removed); + listCopy[i](data.Key, data.Value, default, ObservableUpdateType.Removed); } } } + + Dictionary.Clear(); } /// @@ -371,6 +377,7 @@ public void StopObserving(Action onU if (actions.Value[i] == onUpdate) { actions.Value.RemoveAt(i); + break; } } } @@ -380,6 +387,7 @@ public void StopObserving(Action onU if (_updateActions[i] == onUpdate) { _updateActions.RemoveAt(i); + break; } } } diff --git a/Runtime/ObservableList.cs b/Runtime/ObservableList.cs index 354278a..1b1849c 100644 --- a/Runtime/ObservableList.cs +++ b/Runtime/ObservableList.cs @@ -241,17 +241,18 @@ public virtual void RemoveAt(int index) /// public virtual void Clear() { - var list = new List(List); + // Create a copy in case that one of the callbacks modifies the list (Ex: removing a subscriber) + var copy = _updateActions.ToList(); - List.Clear(); - - for (var i = 0; i < _updateActions.Count; i++) + for (var i = copy.Count - 1; i > -1; i--) { - for (var j = 0; j < list.Count; j++) + for (var j = 0; j < List.Count; j++) { - _updateActions[i](j, list[j], default, ObservableUpdateType.Removed); + copy[i](j, List[j], default, ObservableUpdateType.Removed); } } + + List.Clear(); } /// diff --git a/Tests/Editor/ObservableListTest.cs b/Tests/Editor/ObservableListTest.cs index be9b939..31848d5 100644 --- a/Tests/Editor/ObservableListTest.cs +++ b/Tests/Editor/ObservableListTest.cs @@ -125,6 +125,21 @@ public void StopObserveCheck() _caller.DidNotReceive().Call(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()); } + [Test] + public void StopObserve_MultipleCalls_StopsOnlyOne() + { + _list.Observe(_caller.Call); + _list.Observe(_caller.Call); + _list.StopObserving(_caller.Call); + _list.Add(_previousValue); + + _list[_index] = _previousValue; + + _list.RemoveAt(_index); + + _caller.Received().Call(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()); + } + [Test] public void StopObservingAllCheck() { @@ -137,7 +152,7 @@ public void StopObservingAllCheck() } [Test] - public void StopObservingAll_MultipleCalls_Check() + public void StopObservingAll_MultipleCalls_StopsAll() { _list.Observe(_caller.Call); _list.Observe(_caller.Call); diff --git a/package.json b/package.json index ba47160..0740b40 100644 --- a/package.json +++ b/package.json @@ -2,7 +2,7 @@ "name": "com.gamelovers.dataextensions", "displayName": "Unity Data Type Extensions", "author": "Miguel Tomas", - "version": "0.6.4", + "version": "0.6.5", "unity": "2022.3", "license": "MIT", "description": "This package extends various sets of data types to be used in any type of data containers or persistent serializable data", From c3e80bab4b0ad66f8c6c3f32070ebf801c6d3fc4 Mon Sep 17 00:00:00 2001 From: CoderGamester Date: Wed, 20 Nov 2024 23:11:08 +0000 Subject: [PATCH 2/3] PR fix for cases where unsubscriptions could happen in different places on the order list --- Runtime/ObservableDictionary.cs | 40 +++++++++++++++++++----- Runtime/ObservableList.cs | 27 ++++++++++++++-- Tests/Editor/ObservableDictionaryTest.cs | 1 - Tests/Editor/ObservableListTest.cs | 6 ++-- 4 files changed, 62 insertions(+), 12 deletions(-) diff --git a/Runtime/ObservableDictionary.cs b/Runtime/ObservableDictionary.cs index 97a38f4..a3792c9 100644 --- a/Runtime/ObservableDictionary.cs +++ b/Runtime/ObservableDictionary.cs @@ -3,7 +3,6 @@ using System.Collections.Generic; using System.Collections.ObjectModel; using System.Linq; -using UnityEngine.UIElements; // ReSharper disable once CheckNamespace @@ -276,18 +275,26 @@ public virtual bool Remove(TKey key) if (ObservableUpdateFlag != ObservableUpdateFlag.UpdateOnly && _keyUpdateActions.TryGetValue(key, out var actions)) { - var listCopy = actions.ToList(); - for (var i = 0; i < listCopy.Count; i++) + for (var i = actions.Count - 1; i > -1; i--) { - listCopy[i](key, value, default, ObservableUpdateType.Removed); + var action = actions[i]; + + action(key, value, default, ObservableUpdateType.Removed); + + // Shift the index if an action was unsubscribed + i = AdjustIndex(i, action, actions); } } if (ObservableUpdateFlag != ObservableUpdateFlag.KeyUpdateOnly) { - var listCopy = _updateActions.ToList(); - for (var i = 0; i < listCopy.Count; i++) + for (var i = _updateActions.Count - 1; i > -1; i--) { - listCopy[i](key, value, default, ObservableUpdateType.Removed); + var action = _updateActions[i]; + + action(key, value, default, ObservableUpdateType.Removed); + + // Shift the index if an action was unsubscribed + i = AdjustIndex(i, action, _updateActions); } } @@ -442,6 +449,25 @@ protected void InvokeUpdate(TKey key, TValue previousValue) } } } + + private int AdjustIndex(int index, Action action, + IList> list) + { + if (index < list.Count && list[index] == action) + { + return index; + } + + for (var i = index - 1; i > -1; i--) + { + if (list[index] == action) + { + return i; + } + } + + return index + 1; + } } /// diff --git a/Runtime/ObservableList.cs b/Runtime/ObservableList.cs index 1b1849c..303385b 100644 --- a/Runtime/ObservableList.cs +++ b/Runtime/ObservableList.cs @@ -232,9 +232,14 @@ public virtual void RemoveAt(int index) List.RemoveAt(index); - for (var i = 0; i < _updateActions.Count; i++) + for (var i = _updateActions.Count - 1; i > -1; i--) { - _updateActions[i](index, data, default, ObservableUpdateType.Removed); + var action = _updateActions[i]; + + action(index, data, default, ObservableUpdateType.Removed); + + // Shift the index if an action was unsubscribed + i = AdjustIndex(i, action); } } @@ -307,6 +312,24 @@ protected void InvokeUpdate(int index, T previousValue) _updateActions[i](index, previousValue, data, ObservableUpdateType.Updated); } } + + private int AdjustIndex(int index, Action action) + { + if (index < _updateActions.Count && _updateActions[index] == action) + { + return index; + } + + for (var i = index - 1; i > -1; i--) + { + if (_updateActions[index] == action) + { + return i; + } + } + + return index + 1; + } } /// diff --git a/Tests/Editor/ObservableDictionaryTest.cs b/Tests/Editor/ObservableDictionaryTest.cs index 02f51b1..b978a1a 100644 --- a/Tests/Editor/ObservableDictionaryTest.cs +++ b/Tests/Editor/ObservableDictionaryTest.cs @@ -3,7 +3,6 @@ using GameLovers; using NSubstitute; using NUnit.Framework; -using UnityEngine; // ReSharper disable once CheckNamespace diff --git a/Tests/Editor/ObservableListTest.cs b/Tests/Editor/ObservableListTest.cs index 31848d5..b5b9df7 100644 --- a/Tests/Editor/ObservableListTest.cs +++ b/Tests/Editor/ObservableListTest.cs @@ -126,7 +126,7 @@ public void StopObserveCheck() } [Test] - public void StopObserve_MultipleCalls_StopsOnlyOne() + public void StopObserve_WhenCalledOnce_RemovesOnlyOneObserverInstance() { _list.Observe(_caller.Call); _list.Observe(_caller.Call); @@ -137,7 +137,9 @@ public void StopObserve_MultipleCalls_StopsOnlyOne() _list.RemoveAt(_index); - _caller.Received().Call(Arg.Any(), Arg.Any(), Arg.Any(), Arg.Any()); + _caller.Received(1).Call(Arg.Any(), Arg.Is(0), Arg.Is(_previousValue), ObservableUpdateType.Added); + _caller.Received(1).Call(_index, _previousValue, _previousValue, ObservableUpdateType.Updated); + _caller.Received(1).Call(_index, _previousValue, 0, ObservableUpdateType.Removed); } [Test] From 3464a290b504e0e3a2886365e47934a248bb9c89 Mon Sep 17 00:00:00 2001 From: CoderGamester Date: Wed, 20 Nov 2024 23:24:12 +0000 Subject: [PATCH 3/3] PR fix issue in the indexer not being updated --- Runtime/ObservableDictionary.cs | 2 +- Runtime/ObservableList.cs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Runtime/ObservableDictionary.cs b/Runtime/ObservableDictionary.cs index a3792c9..e3ce5fb 100644 --- a/Runtime/ObservableDictionary.cs +++ b/Runtime/ObservableDictionary.cs @@ -460,7 +460,7 @@ private int AdjustIndex(int index, Action -1; i--) { - if (list[index] == action) + if (list[i] == action) { return i; } diff --git a/Runtime/ObservableList.cs b/Runtime/ObservableList.cs index 303385b..954fc7b 100644 --- a/Runtime/ObservableList.cs +++ b/Runtime/ObservableList.cs @@ -322,7 +322,7 @@ private int AdjustIndex(int index, Action actio for (var i = index - 1; i > -1; i--) { - if (_updateActions[index] == action) + if (_updateActions[i] == action) { return i; }