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 all commits
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
64 changes: 49 additions & 15 deletions Runtime/ObservableDictionary.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
using System.Collections;
using System.Collections.Generic;
using System.Collections.ObjectModel;
using UnityEngine.UIElements;
using System.Linq;

// ReSharper disable once CheckNamespace

Expand Down Expand Up @@ -275,16 +275,26 @@ public virtual bool Remove(TKey key)

if (ObservableUpdateFlag != ObservableUpdateFlag.UpdateOnly && _keyUpdateActions.TryGetValue(key, out var actions))
{
for (var i = 0; i < actions.Count; i++)
for (var i = actions.Count - 1; i > -1; i--)
{
actions[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)
{
for (var i = 0; i < _updateActions.Count; i++)
for (var i = _updateActions.Count - 1; i > -1; i--)
{
_updateActions[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);
Comment on lines +278 to +297
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

In the new code, the loop for unsubscribing actions in the ObservableDictionary has been changed to iterate in reverse order (for (var i = actions.Count - 1; i > -1; i--)). This change ensures that the correct actions are unsubscribed even if an action is removed during the iteration. The old code iterates from index 0 to the end of the list (for (var i = 0; i < actions.Count; i++)). The new implementation is more robust and avoids potential issues when modifying the subscription list.

Comment on lines +278 to +297
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 loop for unsubscribing actions in the ObservableDictionary has been changed to iterate in reverse order (for (var i = actions.Count - 1; i > -1; i--)). This change ensures that the correct actions are unsubscribed even if an action is removed during the iteration. The new implementation is more robust and avoids potential issues when modifying the subscription list.

Comment on lines +278 to +297
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 loop for unsubscribing actions in the ObservableDictionary has been improved to iterate in reverse order, which ensures correct behavior even if actions are removed during the iteration.

Comment on lines +278 to +297
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Simplify the index adjustment logic for better reliability

The current implementation of handling unsubscriptions during iteration is overly complex and potentially buggy. A simpler and more reliable approach would be to use a direct check.

Apply this diff to simplify the logic:

 if (ObservableUpdateFlag != ObservableUpdateFlag.UpdateOnly && _keyUpdateActions.TryGetValue(key, out var actions))
 {
     for (var i = actions.Count - 1; i > -1; i--)
     {
         var action = actions[i];
         action(key, value, default, ObservableUpdateType.Removed);
-        // Shift the index if an action was unsubscribed
-        i = AdjustIndex(i, action, actions);
+        // Adjust index if the action was unsubscribed
+        i = actions.Contains(action) ? i : i - 1;
     }
 }
 if (ObservableUpdateFlag != ObservableUpdateFlag.KeyUpdateOnly)
 {
     for (var i = _updateActions.Count - 1; i > -1; i--)
     {
         var action = _updateActions[i];
         action(key, value, default, ObservableUpdateType.Removed);
-        // Shift the index if an action was unsubscribed
-        i = AdjustIndex(i, action, _updateActions);
+        // Adjust index if the action was unsubscribed
+        i = _updateActions.Contains(action) ? i : i - 1;
     }
 }
📝 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
for (var i = actions.Count - 1; i > -1; i--)
{
actions[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)
{
for (var i = 0; i < _updateActions.Count; i++)
for (var i = _updateActions.Count - 1; i > -1; i--)
{
_updateActions[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);
for (var i = actions.Count - 1; i > -1; i--)
{
var action = actions[i];
action(key, value, default, ObservableUpdateType.Removed);
// Adjust index if the action was unsubscribed
i = actions.Contains(action) ? i : i - 1;
}
}
if (ObservableUpdateFlag != ObservableUpdateFlag.KeyUpdateOnly)
{
for (var i = _updateActions.Count - 1; i > -1; i--)
{
var action = _updateActions[i];
action(key, value, default, ObservableUpdateType.Removed);
// Adjust index if the action was unsubscribed
i = _updateActions.Contains(action) ? i : i - 1;

}
Comment on lines +278 to 298
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Approve the safer iteration approach with a minor fix needed

The changes improve safety by using backward iteration and action caching to handle unsubscriptions during iteration. However, there's a bug in the index adjustment logic.

In the AdjustIndex method call, you're passing the action that was just executed, but you should be checking if that action still exists in the list at the current index. Apply this fix:

-i = AdjustIndex(i, action, actions);
+i = actions.Contains(action) ? i : i - 1;

The same fix should be applied to the _updateActions loop.

Committable suggestion skipped: line range outside the PR's diff.

}

Expand All @@ -294,31 +304,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 +384,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 +394,7 @@ public void StopObserving(Action<TKey, TValue, TValue, ObservableUpdateType> onU
if (_updateActions[i] == onUpdate)
{
_updateActions.RemoveAt(i);
break;
}
}
}
Expand Down Expand Up @@ -434,6 +449,25 @@ protected void InvokeUpdate(TKey key, TValue previousValue)
}
}
}

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[i] == action)
{
return i;
}
}

return index + 1;
}
Comment on lines +452 to +470
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 AdjustIndex method has been added in the new code. This method is used to adjust the index when an action is unsubscribed during the iteration. It ensures that the correct index is returned even if the action is removed from the list. This method improves the reliability of the iteration process. The old code does not have this method.

Overall Comments

The changes made to the ObservableDictionary.cs file address issues related to subscribing and unsubscribing actions while adding or removing elements. The new implementation improves the reliability and robustness of the subscription mechanism. The changes also include optimizations to avoid unnecessary iterations and handle modifications to the subscription lists during iteration.

The code changes appear to be well-implemented and address the identified issues effectively. No further improvements or concerns are identified.

Suggestions

No suggestions for improvement. The code changes are satisfactory.

Review comments

5:
The System.Linq namespace is not being used in the new code. It can be removed.

Comment on lines +452 to +470
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 AdjustIndex method has been added to adjust the index when an action is unsubscribed during the iteration. It ensures that the correct index is returned even if the action is removed from the list. This method improves the reliability of the iteration process. The old code does not have this method.

Review comments

5:
Remove the unused System.Linq namespace import.

Comment on lines +453 to +470
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Remove AdjustIndex in favor of simpler Contains check

The AdjustIndex method is complex and potentially error-prone. As suggested earlier, replace its usage with a simple Contains check.

Remove this method and use the simpler approach shown in the earlier review comment for the Remove method.

}

/// <inheritdoc />
Expand Down
40 changes: 32 additions & 8 deletions Runtime/ObservableList.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,26 +232,32 @@ 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);
}
Comment on lines +235 to 243
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 code block is responsible for removing an element from the list and notifying the observers. However, there are a few issues with the implementation:

  1. Line 237: The loop iterates over _updateActions in reverse order, but it doesn't handle the case where an action is unsubscribed during the iteration. This can lead to incorrect indices being used for subsequent actions. It's recommended to use a for loop instead of a foreach loop to allow modifying the collection safely.

  2. Line 242: The line i = AdjustIndex(i, action); adjusts the index if an action was unsubscribed. However, the AdjustIndex method is missing from the code snippet, so it's unclear how it handles the adjustment. Please provide the implementation of the AdjustIndex method for further review.

}

/// <inheritdoc />
public virtual void Clear()
{
var list = new List<T>(List);

List.Clear();
// Create a copy in case that one of the callbacks modifies the list (Ex: removing a subscriber)
var copy = _updateActions.ToList();

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();
Comment on lines 246 to +260
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 code block is responsible for clearing the list and notifying the observers. There are a couple of issues with the implementation:

  1. Line 250: Creating a copy of _updateActions using ToList() is unnecessary since the loop only reads from _updateActions and doesn't modify it. Removing the copy operation will improve performance.

  2. Lines 254-257: The nested loop iterates over List and invokes each action for every element in the list. This can result in redundant notifications if there are multiple actions subscribed. It's more efficient to notify all actions once for each element before clearing the list.

}

/// <inheritdoc />
Comment on lines 232 to 263
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]

The loop in the RemoveAt method can be simplified and optimized by iterating from the first index to the last index using a for loop instead of a foreach loop.

Comment on lines 232 to 263
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 [24-25]

The loop in the Clear method can be simplified by directly iterating over _updateActions instead of creating a copy. Additionally, the nested loop that iterates over List can be simplified by using a foreach loop instead of a for loop.

Expand Down Expand Up @@ -306,6 +312,24 @@ protected void InvokeUpdate(int index, T previousValue)
_updateActions[i](index, previousValue, data, ObservableUpdateType.Updated);
}
}

private int AdjustIndex(int index, Action<int, T, T, ObservableUpdateType> action)
{
if (index < _updateActions.Count && _updateActions[index] == action)
{
return index;
}

for (var i = index - 1; i > -1; i--)
{
if (_updateActions[i] == action)
{
return i;
}
}

return index + 1;
}
Comment on lines +316 to +332
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

In the AdjustIndex method, there is a linear search through _updateActions to find the index of an action. This can be optimized by using a dictionary or a lookup table to map actions to their indices, allowing for constant-time lookup.

Error handling mechanisms, such as try-catch blocks, should be added to handle any potential exceptions that may occur during element removal or clearing.

Consider using a more descriptive variable name instead of copy in the Clear method to indicate its purpose.


Note: The suggestions provided are based on the code snippets and the context provided. It is possible that additional changes or considerations may be necessary depending on the surrounding code and requirements.


<!-- This is an auto-generated comment by OSS CodeRabbit -->

Comment on lines +316 to +332
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Logical errors and risk of exceptions in AdjustIndex method

The AdjustIndex method may cause IndexOutOfRangeException if _updateActions is modified and the index becomes invalid. Accessing _updateActions[index] without verifying the validity of index can result in exceptions. Additionally, returning index + 1 when the action is not found may not properly adjust the index, potentially skipping subsequent actions.

Consider revising the method to safely handle index adjustments:

-        if (index < _updateActions.Count && _updateActions[index] == action)
+        if (index >= 0 && index < _updateActions.Count && _updateActions[index] == action)

Also, ensure that the logic correctly adjusts the index when actions are removed to prevent skipping notifications or causing exceptions.

Committable suggestion skipped: line range outside the PR's diff.

}

/// <inheritdoc />
Expand Down
1 change: 0 additions & 1 deletion Tests/Editor/ObservableDictionaryTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
using GameLovers;
using NSubstitute;
using NUnit.Framework;
using UnityEngine;

// ReSharper disable once CheckNamespace

Expand Down
19 changes: 18 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,23 @@ public void StopObserveCheck()
_caller.DidNotReceive().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);

_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 +154,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