Release 0.6.5 #12
Release 0.6.5 #12
Conversation
|
Warning CodeRabbit GitHub Action detectedThe repository is using both CodeRabbit Pro and CodeRabbit Open Source (via GitHub Actions), which is not recommended as it may lead to duplicate comments and extra noise. Please remove the CodeRabbit GitHub Action. 📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request include updates to the Changes
Sequence Diagram(s)sequenceDiagram
participant Observer
participant ObservableList
participant ObservableDictionary
Observer->>ObservableList: Subscribe
ObservableList->>ObservableList: Add Item
ObservableList->>Observer: Notify Item Added
Observer->>ObservableList: Stop Observing
ObservableList->>ObservableList: Clear List
ObservableList->>Observer: Notify Item Removed
ObservableDictionary->>ObservableDictionary: Synchronize Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
WalkthroughThe changes include improvements to the Changes
Uplevel your code reviews with CodeRabbit ProCodeRabbit ProIf you like this project, please support us by purchasing the Pro version. The Pro version has advanced context, superior noise reduction and several proprietary improvements compared to the open source version. Moreover, CodeRabbit Pro is free for open source projects. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Files selected (4)
- CHANGELOG.md (1)
- Runtime/ObservableDictionary.cs (5)
- Runtime/ObservableList.cs (1)
- Tests/Editor/ObservableListTest.cs (2)
Files ignored due to filter (1)
- package.json
Review comments generated (3)
- Review: 1
- LGTM: 2
Tips
Chat with 
CodeRabbit Bot (@coderabbitai)
- Reply on review comments left by this bot to ask follow-up questions. A review comment is a comment on a diff or a file.
- Invite the bot into a review comment chain by tagging
@coderabbitaiin a reply.
Code suggestions
- The bot may make code suggestions, but please review them carefully before committing since the line number ranges may be misaligned.
- You can edit the comment made by the bot and manually tweak the suggestion if it is slightly off.
Pausing incremental reviews
- Add
@coderabbitai: ignoreanywhere in the PR description to pause further reviews from the bot.
| 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**: |
There was a problem hiding this comment.
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![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
Tests/Editor/ObservableListTest.cs (1)
128-141: Consider adding tests for concurrent subscription changesThe PR objectives mention fixing issues with subscribing/unsubscribing while elements are being added or removed. Consider adding tests that specifically verify this behavior.
Example test scenarios to consider:
- Subscribe to the list within an existing observer's callback
- Unsubscribe from the list within an existing observer's callback
- Verify that observers added during an operation receive subsequent notifications
- Verify that observers removed during an operation don't receive subsequent notifications
Would you like me to provide example implementations for these test cases?
Runtime/ObservableList.cs (2)
244-245: LGTM! Creating a copy prevents concurrent modification issues.Creating a copy of
_updateActionsis a good practice to prevent issues when observers unsubscribe during notification. However, consider caching the copy if Clear is called frequently to optimize memory allocation.public virtual void Clear() { - // Create a copy in case that one of the callbacks modifies the list (Ex: removing a subscriber) - var copy = _updateActions.ToList(); + // Cache the copy to prevent frequent allocations + var copy = _cachedUpdateActions ?? (_cachedUpdateActions = new List<Action<int, T, T, ObservableUpdateType>>()); + copy.Clear(); + copy.AddRange(_updateActions);
244-255: Add null checks for robustness.The method assumes
Listand_updateActionsare never null. Consider adding null checks for better robustness.public virtual void Clear() { + if (List == null || _updateActions == null || List.Count == 0) + { + return; + } + // Create a copy in case that one of the callbacks modifies the list (Ex: removing a subscriber) var copy = _updateActions.ToList(); for (var i = copy.Count - 1; i > -1; i--) { for (var j = 0; j < List.Count; j++) { copy[i](j, List[j], default, ObservableUpdateType.Removed); } } List.Clear(); }Runtime/ObservableDictionary.cs (3)
279-290: LGTM! Consider memory pooling for list copies.The defensive copying using
ToList()correctly prevents collection modification issues during observer notifications. However, in high-frequency scenarios with many observers, this could lead to memory churn.Consider using an object pool for the temporary lists to reduce garbage collection pressure:
- var listCopy = actions.ToList(); + var listCopy = ListPool<Action<TKey, TValue, TValue, ObservableUpdateType>>.Get(actions); for (var i = 0; i < listCopy.Count; i++) { listCopy[i](key, value, default, ObservableUpdateType.Removed); } + ListPool<Action<TKey, TValue, TValue, ObservableUpdateType>>.Release(listCopy);
302-327: LGTM! Consider optimizing dictionary copy and notification order.The defensive copying strategy correctly prevents collection modification issues. A few suggestions for improvement:
- Consider using a more memory-efficient approach for the dictionary copy:
- var copy = new Dictionary<TKey, IList<Action<TKey, TValue, TValue, ObservableUpdateType>>>(_keyUpdateActions); + var copy = _keyUpdateActions.ToList(); // List<KeyValuePair> is more lightweight than Dictionary
- Consider moving the Dictionary.Clear() before notifications to prevent observers from accessing invalid state:
+ Dictionary.Clear(); if (ObservableUpdateFlag != ObservableUpdateFlag.UpdateOnly) { // ... notifications ... } if (ObservableUpdateFlag != ObservableUpdateFlag.KeyUpdateOnly) { // ... notifications ... } - Dictionary.Clear();
380-380: Document single registration behavior.The break statements after observer removal indicate that each observer can only be registered once. This important behavior should be documented in the interface.
Add documentation to the Observe methods:
/// <inheritdoc /> + /// <remarks> + /// Each observer can only be registered once. Subsequent registrations of the same observer will be ignored. + /// </remarks> public void Observe(Action<TKey, TValue, TValue, ObservableUpdateType> onUpdate)Also applies to: 390-390
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
CHANGELOG.md(1 hunks)Runtime/ObservableDictionary.cs(5 hunks)Runtime/ObservableList.cs(1 hunks)Tests/Editor/ObservableListTest.cs(2 hunks)package.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- package.json
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.md
[style] ~10-~10: Consider using a different verb for a more formal wording.
Context: ... ## [0.6.5] - 2024-11-20
Fix:
- Fixed the issues of ObservableDictionary wh...
(FIX_RESOLVE)
[style] ~11-~11: Consider using a different verb for a more formal wording.
Context: ...tions while removing/adding elements
- Fixed the issues of ObservableList when sub...
(FIX_RESOLVE)
🔇 Additional comments (2)
Tests/Editor/ObservableListTest.cs (1)
155-155: LGTM! Clear and descriptive test name
The rename from StopObservingAll_MultipleCalls_Check to StopObservingAll_MultipleCalls_StopsAll better describes the test's purpose and expected behavior.
Runtime/ObservableList.cs (1)
244-255: Verify the observer notification behavior.
While the changes address the subscription/unsubscription issues during Clear operations, let's verify the behavior with a test that subscribes/unsubscribes observers during notification.
Tests/Editor/ObservableListTest.cs
Outdated
| [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>()); | ||
| } |
There was a problem hiding this comment.
🛠️ 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:
- Making the test name more descriptive
- Verifying exact number of calls (should be 1, not 2)
- 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.
| [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); | |
| } |
…es on the order list
There was a problem hiding this comment.
Files selected (4)
- Runtime/ObservableDictionary.cs (6)
- Runtime/ObservableList.cs (2)
- Tests/Editor/ObservableDictionaryTest.cs (1)
- Tests/Editor/ObservableListTest.cs (2)
Files skipped from review due to trivial changes (1)
- Tests/Editor/ObservableDictionaryTest.cs
Review comments generated (8)
- Review: 7
- LGTM: 1
Tips
Chat with CodeRabbit Bot (@coderabbitai)
- Reply on review comments left by this bot to ask follow-up questions. A review comment is a comment on a diff or a file.
- Invite the bot into a review comment chain by tagging
@coderabbitaiin a reply.
Code suggestions
- The bot may make code suggestions, but please review them carefully before committing since the line number ranges may be misaligned.
- You can edit the comment made by the bot and manually tweak the suggestion if it is slightly off.
Pausing incremental reviews
- Add
@coderabbitai: ignoreanywhere in the PR description to pause further reviews from the bot.
| 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); | ||
| } |
There was a problem hiding this comment.
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:
-
Line 237: The loop iterates over
_updateActionsin 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 aforloop instead of aforeachloop to allow modifying the collection safely. -
Line 242: The line
i = AdjustIndex(i, action);adjusts the index if an action was unsubscribed. However, theAdjustIndexmethod is missing from the code snippet, so it's unclear how it handles the adjustment. Please provide the implementation of theAdjustIndexmethod 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(); |
There was a problem hiding this comment.
The code block is responsible for clearing the list and notifying the observers. There are a couple of issues with the implementation:
-
Line 250: Creating a copy of
_updateActionsusingToList()is unnecessary since the loop only reads from_updateActionsand doesn't modify it. Removing the copy operation will improve performance. -
Lines 254-257: The nested loop iterates over
Listand 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.
Runtime/ObservableList.cs
Outdated
|
|
||
| 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[index] == action) | ||
| { | ||
| return i; | ||
| } | ||
| } | ||
|
|
||
| return index + 1; | ||
| } |
There was a problem hiding this comment.
The code block contains the AdjustIndex method, which is referenced in the previous code block. However, the logic of the method is incomplete in the provided code snippet. Please provide the complete implementation of the AdjustIndex method for further review.
Considering the issues mentioned above, here are the suggested changes:
public virtual void RemoveAt(int index)
{
- for (var i = _updateActions.Count - 1; i > -1; i--)
+ for (var i = _updateActions.Count - 1; i >= 0; i--)
{
var action = _updateActions[i];
action(index, data, default, ObservableUpdateType.Removed);
- i = AdjustIndex(i, action);
}
}
public virtual void Clear()
{
- var copy = _updateActions.ToList();
-
- for (var i = copy.Count - 1; i > -1; i--)
+ for (var i = _updateActions.Count - 1; i >= 0; i--)
{
- for (var j = 0; j < List.Count; j++)
- {
- copy[i](j, List[j], default, ObservableUpdateType.Removed);
- }
+ for (var j = List.Count - 1; j >= 0; j--)
+ _updateActions[i](j, List[j], default, ObservableUpdateType.Removed);
}
List.Clear();
}
+ private int AdjustIndex(int index, Action<int, T, T, ObservableUpdateType> action)
+ {
+ // TODO: Implement the logic for adjusting the index
+ return index;
+ }Please provide the missing implementation of the AdjustIndex method so that it can be reviewed further.
Note: The suggested changes assume that the missing parts of the code are implemented correctly. If there are any additional issues or concerns, please provide more context or the complete code for a thorough review.
| if (actions.Value[i] == onUpdate) | ||
| { | ||
| actions.Value.RemoveAt(i); | ||
| break; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
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);
}
Runtime/ObservableDictionary.cs
Outdated
|
|
||
| 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; | ||
| } |
There was a problem hiding this comment.
The AdjustIndex method is no longer used and can be removed.
Overall, the changes in ObservableDictionary.cs address issues related to subscribing/unsubscribing actions and improve the handling of callbacks during element addition/removal. The suggested optimizations simplify the code and enhance performance.
Please note that the review comments are based on the provided code snippets and may not cover all aspects of the implementation. It's recommended to thoroughly test the modified code and ensure it behaves as expected.
Let me know if you have any questions or need further assistance!
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
Runtime/ObservableDictionary.cs (1)
309-334: Approve the safer Clear implementation with optimization suggestionsThe changes correctly prevent concurrent modification issues by creating copies before iteration. However, there's room for optimization.
Consider these optimizations:
- Reuse the dictionary copy:
// Create a copy in case that one of the callbacks modifies the list -var copy = new Dictionary<TKey, IList<Action<TKey, TValue, TValue, ObservableUpdateType>>>(_keyUpdateActions); +var dictionaryCopy = new Dictionary<TKey, TValue>(Dictionary); -foreach (var data in copy) +foreach (var data in dictionaryCopy) { var listCopy = data.Value.ToList(); for (var i = 0; i < listCopy.Count; i++) { - listCopy[i](data.Key, Dictionary[data.Key], default, ObservableUpdateType.Removed); + listCopy[i](data.Key, data.Value, default, ObservableUpdateType.Removed); } }
- Avoid creating multiple list copies in the second loop:
+var updateActionsCopy = _updateActions.ToList(); foreach (var data in Dictionary) { - var listCopy = _updateActions.ToList(); for (var i = 0; i < listCopy.Count; i++) { - listCopy[i](data.Key, data.Value, default, ObservableUpdateType.Removed); + updateActionsCopy[i](data.Key, data.Value, default, ObservableUpdateType.Removed); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
Runtime/ObservableDictionary.cs(6 hunks)Runtime/ObservableList.cs(2 hunks)Tests/Editor/ObservableDictionaryTest.cs(0 hunks)Tests/Editor/ObservableListTest.cs(2 hunks)
💤 Files with no reviewable changes (1)
- Tests/Editor/ObservableDictionaryTest.cs
🔇 Additional comments (5)
Tests/Editor/ObservableListTest.cs (2)
128-143: LGTM! Well-structured test with specific assertions.
The test effectively verifies that calling StopObserving once only removes one observer instance, with clear and specific assertions for each operation.
157-157: LGTM! Clear and descriptive test name.
The renamed test method better describes its purpose by explicitly stating that it verifies all observers are stopped.
Runtime/ObservableDictionary.cs (1)
387-387: LGTM! Optimization for action removal
The addition of break statements after removing actions is a good optimization since an action can only be registered once.
Also applies to: 397-397
Runtime/ObservableList.cs (2)
235-242: Inconsistent iteration order for observer notifications.
The iteration over _updateActions in the RemoveAt method is in reverse order, while in the Add method, it is in forward order. This creates inconsistent notification sequences for observers between different operations. Consider standardizing the iteration order across methods for consistency.
249-260: Inconsistent iteration order in Clear method.
Similar to RemoveAt, the Clear method iterates over the observers in reverse order, whereas other methods like Add iterate in forward order. This inconsistency might lead to unpredictable notification sequences for observers.
| 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); | ||
| } |
There was a problem hiding this comment.
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.
Runtime/ObservableDictionary.cs
Outdated
| 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; | ||
| } |
There was a problem hiding this comment.
Fix critical bugs in AdjustIndex method
The method contains several bugs that could lead to skipped notifications or index out of range exceptions.
Apply these fixes:
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;
+ // If the action was removed, adjust the index
+ return list.Contains(action) ? index : index - 1;
}Alternatively, as suggested earlier, consider removing this method entirely and replacing its usage with a simpler inline check:
-i = AdjustIndex(i, action, actions);
+i = actions.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.
| 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; | |
| } | |
| private int AdjustIndex(int index, Action<TKey, TValue, TValue, ObservableUpdateType> action, | |
| IList<Action<TKey, TValue, TValue, ObservableUpdateType>> list) | |
| { | |
| // If the action was removed, adjust the index | |
| return list.Contains(action) ? index : index - 1; | |
| } |
Runtime/ObservableList.cs
Outdated
| for (var i = index - 1; i > -1; i--) | ||
| { | ||
| if (_updateActions[index] == action) |
There was a problem hiding this comment.
Potential logic error in AdjustIndex method.
In the AdjustIndex method, the condition inside the loop incorrectly references _updateActions[index] instead of _updateActions[i]. This means the same action is compared repeatedly, potentially leading to incorrect index adjustments when observers unsubscribe during iteration.
Apply this diff to correct the condition:
- if (_updateActions[index] == action)
+ if (_updateActions[i] == action)📝 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.
| for (var i = index - 1; i > -1; i--) | |
| { | |
| if (_updateActions[index] == action) | |
| for (var i = index - 1; i > -1; i--) | |
| { | |
| if (_updateActions[i] == action) |
There was a problem hiding this comment.
Files selected (2)
- Runtime/ObservableDictionary.cs (6)
- Runtime/ObservableList.cs (2)
Review comments generated (12)
- Review: 11
- LGTM: 1
Tips
Chat with CodeRabbit Bot (@coderabbitai)
- Reply on review comments left by this bot to ask follow-up questions. A review comment is a comment on a diff or a file.
- Invite the bot into a review comment chain by tagging
@coderabbitaiin a reply.
Code suggestions
- The bot may make code suggestions, but please review them carefully before committing since the line number ranges may be misaligned.
- You can edit the comment made by the bot and manually tweak the suggestion if it is slightly off.
Pausing incremental reviews
- Add
@coderabbitai: ignoreanywhere in the PR description to pause further reviews from the bot.
|
|
||
| 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); | ||
| } | ||
| } | ||
|
|
||
| /// <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(); | ||
| } | ||
|
|
||
| /// <inheritdoc /> |
There was a problem hiding this comment.
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.
|
|
||
| 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); | ||
| } | ||
| } | ||
|
|
||
| /// <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(); | ||
| } | ||
|
|
||
| /// <inheritdoc /> |
There was a problem hiding this comment.
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.
| 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; | ||
| } |
There was a problem hiding this comment.
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 -->
| 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); |
There was a problem hiding this comment.
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.
| 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(); |
There was a problem hiding this comment.
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;| 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); |
There was a problem hiding this comment.
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.
| 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(); |
There was a problem hiding this comment.
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.
|
|
||
| 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; | ||
| } |
There was a problem hiding this comment.
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.
| 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); |
There was a problem hiding this comment.
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.
| 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(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
Runtime/ObservableDictionary.cs (1)
309-334: LGTM with a minor optimization suggestionThe changes correctly prevent modification issues during iteration by using copied collections. However, we can optimize memory usage by reusing the list copies.
Consider this optimization:
if (ObservableUpdateFlag != ObservableUpdateFlag.UpdateOnly) { // Create a copy in case that one of the callbacks modifies the list var copy = new Dictionary<TKey, IList<Action<TKey, TValue, TValue, ObservableUpdateType>>>(_keyUpdateActions); + var reusableListCopy = new List<Action<TKey, TValue, TValue, ObservableUpdateType>>(); foreach (var data in copy) { - var listCopy = data.Value.ToList(); + reusableListCopy.Clear(); + reusableListCopy.AddRange(data.Value); - for (var i = 0; i < listCopy.Count; i++) + for (var i = 0; i < reusableListCopy.Count; i++) { - listCopy[i](data.Key, Dictionary[data.Key], default, ObservableUpdateType.Removed); + reusableListCopy[i](data.Key, Dictionary[data.Key], default, ObservableUpdateType.Removed); } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
Runtime/ObservableDictionary.cs(6 hunks)Runtime/ObservableList.cs(2 hunks)
🔇 Additional comments (4)
Runtime/ObservableDictionary.cs (1)
387-387: LGTM! Optimization correctly handles unique actions
The addition of break statements optimizes the method by stopping iteration once an action is found and removed, which is correct since each action should only appear once in each list.
Also applies to: 397-397
Runtime/ObservableList.cs (3)
235-243: Inconsistent observer notification order across methods
The RemoveAt and Clear methods iterate over _updateActions in reverse order, whereas the Add and InvokeUpdate methods iterate in forward order. This inconsistency can lead to observers receiving notifications in different orders depending on the operation, which may cause confusion or unintended side effects.
Consider standardizing the iteration order in all methods to ensure consistent observer notification sequencing.
Also applies to: 281-293
235-243:
Potential issue with index adjustment in RemoveAt method
The AdjustIndex method may not correctly adjust the index when observers unsubscribe during iteration in the RemoveAt method. This can lead to skipped actions or IndexOutOfRangeException due to incorrect indexing.
This issue was previously noted, but it appears that the AdjustIndex method may still have logical errors. Please review and ensure that index adjustments correctly handle modifications to _updateActions during iteration.
252-258: 🛠️ Refactor suggestion
Inefficient notification logic in Clear method
The nested loops in the Clear method can lead to redundant notifications. Each observer is notified for every element, which may result in performance issues, especially with large lists and multiple observers.
Consider refactoring the method to optimize notifications. For example, notify each observer once with a summary of changes or adjust the logic to minimize the number of callbacks.
| 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; | ||
| } |
There was a problem hiding this comment.
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.
| 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); |
There was a problem hiding this comment.
🛠️ 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.
| 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; |
| 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; | ||
| } |
There was a problem hiding this comment.
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.
Fix:
Summary by CodeRabbit
Release Notes for Version 0.6.5
Bug Fixes
ObservableDictionaryandObservableList, enhancing reliability during element addition/removal.New Features
ObservableListand its origin list, ensuring accurate observer notifications during modifications.Tests
ObservableListto verify correct behavior when stopping multiple observers.Documentation
Summary by CodeRabbit
ObservableDictionaryandObservableListrelated to subscribing/unsubscribing actions during element addition/removal.ObservableDictionaryandObservableListclasses.Clearmethod to ensure proper handling of callbacks.StopObserve_MultipleCalls_StopsOnlyOneto test stopping observation for multiple calls.StopObservingAll_MultipleCalls_ChecktoStopObservingAll_MultipleCalls_StopsAll.UnityEngineimport in the code file.