-
Notifications
You must be signed in to change notification settings - Fork 1
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.
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
Release 0.6.5 #12
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the new code, the loop for unsubscribing actions in the
Comment on lines
+278
to
+297
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The loop for unsubscribing actions in the
Comment on lines
+278
to
+297
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Comment on lines
+278
to
+297
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+278
to
298
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 -i = AdjustIndex(i, action, actions);
+i = actions.Contains(action) ? i : i - 1; The same fix should be applied to the
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The 387: - actions.Value.RemoveAt(i);
+ actions.Value.RemoveAt(i);
+ break; 397: - _updateActions.RemoveAt(i);
+ _updateActions.RemoveAt(i);
+ break;
Comment on lines
307
to
+334
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The 387: 397:
Comment on lines
307
to
+334
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The 387: 397: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
/// <inheritdoc /> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The code for removing an action from the 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);
} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -380,6 +394,7 @@ public void StopObserving(Action<TKey, TValue, TValue, ObservableUpdateType> onU | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if (_updateActions[i] == onUpdate) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
_updateActions.RemoveAt(i); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
break; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -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[index] == action) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return i; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return index + 1; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Overall, the changes in 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
/// <inheritdoc /> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
|
||||||||||||||
} | ||||||||||||||
|
||||||||||||||
/// <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); | ||||||||||||||
} | ||||||||||||||
} | ||||||||||||||
CoderGamester marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||
|
||||||||||||||
List.Clear(); | ||||||||||||||
Comment on lines
246
to
+260
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The code block is responsible for clearing the list and notifying the observers. There are a couple of issues with the implementation:
|
||||||||||||||
} | ||||||||||||||
|
||||||||||||||
/// <inheritdoc /> | ||||||||||||||
Comment on lines
232
to
263
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Comment on lines
232
to
263
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The loop in the |
||||||||||||||
|
@@ -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[index] == action) | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Potential logic error in In the Apply this diff to correct the condition: - if (_updateActions[index] == action)
+ if (_updateActions[i] == action) 📝 Committable suggestion
Suggested change
|
||||||||||||||
{ | ||||||||||||||
return i; | ||||||||||||||
} | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
return index + 1; | ||||||||||||||
} | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The code block contains the 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 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. |
||||||||||||||
} | ||||||||||||||
|
||||||||||||||
/// <inheritdoc /> | ||||||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a syntax error in the
add
function. Thereturn
keyword is misspelled asretrn
. It should be corrected as follows: