Skip to content

Release 0.6.5 #12

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Nov 20, 2024
Merged

Release 0.6.5 #12

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
206ab27
**Fix**:
CoderGamester Nov 20, 2024
c3e80ba
PR fix for cases where unsubscriptions could happen in different plac…
CoderGamester Nov 20, 2024
3464a29
PR fix issue in the indexer not being updated
CoderGamester Nov 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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**:
Comment on lines 4 to 15
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description CodeRabbit

Note: This review was outside of the patch, but no patch was found that overlapped with it. Original lines [22-22]

There's a syntax error in the add function. The return keyword is misspelled as retrn. It should be corrected as follows:

-    retrn z
+    return z

Expand Down
36 changes: 22 additions & 14 deletions Runtime/ObservableDictionary.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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);
}
}

Expand All @@ -294,31 +297,34 @@ public virtual bool Remove(TKey key)
/// <inheritdoc />
public virtual void Clear()
{
var dictionary = new Dictionary<TKey, TValue>(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<TKey, IList<Action<TKey, TValue, TValue, ObservableUpdateType>>>(_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();
Comment on lines 307 to +334
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description CodeRabbit

The Clear method has been refactored in the new code. Instead of directly iterating over the _keyUpdateActions dictionary and the Dictionary itself, copies of the lists are created before iterating over them. This is done to prevent issues when modifying the subscription lists during the iteration. The old code does not create copies and directly iterates over the original lists. The new implementation ensures that callbacks are properly handled even if a subscriber is removed during the iteration.

387:
In the new code, when removing an action from the _keyUpdateActions dictionary, there is a check to break out of the loop after removing the action. This optimization can improve performance by avoiding unnecessary iterations. However, this optimization is missing in the old code.

- 						actions.Value.RemoveAt(i);
+ 						actions.Value.RemoveAt(i);
+ 						break;

397:
Similar to the previous comment, in the new code, when removing an action from the _updateActions list, there is a check to break out of the loop after removing the action. This optimization is missing in the old code.

- 					_updateActions.RemoveAt(i);
+ 					_updateActions.RemoveAt(i);
+ 					break;

Comment on lines 307 to +334
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description CodeRabbit

The Clear method has been refactored to create copies of the lists before iterating over them. This is done to prevent issues when modifying the subscription lists during the iteration. The new implementation ensures that callbacks are properly handled even if a subscriber is removed during the iteration.

387:
When removing an action from the _keyUpdateActions dictionary, there is a check to break out of the loop after removing the action. This optimization can improve performance by avoiding unnecessary iterations. However, this optimization is missing in the old code.

397:
Similar to the previous comment, when removing an action from the _updateActions list, there is a check to break out of the loop after removing the action. This optimization is missing in the old code.

Comment on lines 307 to +334
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description CodeRabbit

The Clear method has been refactored to create copies of the lists before iterating over them. This prevents issues when modifying the subscription lists during the iteration.

387:
When removing an action from the _keyUpdateActions dictionary, add a check to break out of the loop after removing the action. This optimization avoids unnecessary iterations.

397:
When removing an action from the _updateActions list, add a check to break out of the loop after removing the action. This optimization avoids unnecessary iterations.

}

/// <inheritdoc />
Expand Down Expand Up @@ -371,6 +377,7 @@ public void StopObserving(Action<TKey, TValue, TValue, ObservableUpdateType> onU
if (actions.Value[i] == onUpdate)
{
actions.Value.RemoveAt(i);
break;
}
}
}
Comment on lines 384 to 390
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image description CodeRabbit

Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [387-397]

The code for removing an action from the _keyUpdateActions list and _updateActions list can be simplified. Instead of using a loop to find the index of the action and then remove it, you can use the Remove method directly. Here's the updated code:

private void RemoveActionFromList(Action<TKey, TValue, TValue, ObservableUpdateType> onUpdate,
    IDictionary<TKey, IList<Action<TKey, TValue, TValue, ObservableUpdateType>>> actionsDictionary)
{
    foreach (var actions in actionsDictionary.Values)
    {
-        for (var i = 0; i < actions.Count; i++)
+        actions.Remove(onUpdate);
+    }
+}

private void RemoveActionFromList(Action<TKey, TValue, TValue, ObservableUpdateType> onUpdate,
    IList<Action<TKey, TValue, TValue, ObservableUpdateType>> actionsList)
{
-        if (actions[i] == onUpdate)
-        {
-            actions.RemoveAt(i);
-        }
-    }
-}
-
-private int AdjustIndex(int index, Action<TKey, TValue, TValue, ObservableUpdateType> action,
-    IList<Action<TKey, TValue, TValue, ObservableUpdateType>> 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;
+    actionsList.Remove(onUpdate);
}

Expand All @@ -380,6 +387,7 @@ public void StopObserving(Action<TKey, TValue, TValue, ObservableUpdateType> onU
if (_updateActions[i] == onUpdate)
{
_updateActions.RemoveAt(i);
break;
}
}
}
Expand Down
13 changes: 7 additions & 6 deletions Runtime/ObservableList.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,17 +241,18 @@ public virtual void RemoveAt(int index)
/// <inheritdoc />
public virtual void Clear()
{
var list = new List<T>(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();
}

/// <inheritdoc />
Expand Down
17 changes: 16 additions & 1 deletion Tests/Editor/ObservableListTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,21 @@ public void StopObserveCheck()
_caller.DidNotReceive().Call(Arg.Any<int>(), Arg.Any<int>(), Arg.Any<int>(), Arg.Any<ObservableUpdateType>());
}

[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<int>(), Arg.Any<int>(), Arg.Any<int>(), Arg.Any<ObservableUpdateType>());
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Strengthen test assertions for better verification

While the test correctly verifies that observers remain active after one StopObserving call, the assertions could be more specific to ensure correct behavior.

Consider this improved implementation:

 [Test]
-public void StopObserve_MultipleCalls_StopsOnlyOne()
+public void StopObserve_WhenCalledOnce_RemovesOnlyOneObserverInstance()
 {
     _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<int>(), Arg.Any<int>(), Arg.Any<int>(), Arg.Any<ObservableUpdateType>());
+    // Verify each operation was observed exactly once (not twice)
+    _caller.Received(1).Call(Arg.Any<int>(), 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);
 }

This improves the test by:

  1. Making the test name more descriptive
  2. Verifying exact number of calls (should be 1, not 2)
  3. Checking specific parameters for each operation
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
[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<int>(), Arg.Any<int>(), Arg.Any<int>(), Arg.Any<ObservableUpdateType>());
}
[Test]
public void StopObserve_WhenCalledOnce_RemovesOnlyOneObserverInstance()
{
_list.Observe(_caller.Call);
_list.Observe(_caller.Call);
_list.StopObserving(_caller.Call);
_list.Add(_previousValue);
_list[_index] = _previousValue;
_list.RemoveAt(_index);
// Verify each operation was observed exactly once (not twice)
_caller.Received(1).Call(Arg.Any<int>(), 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]
public void StopObservingAllCheck()
{
Expand All @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
Loading