Skip to content

Fixed ArgumentOutOfRangeException in AdvancedCollectionView #4349

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

Closed
w-ahmad wants to merge 2 commits into from
Closed

Fixed ArgumentOutOfRangeException in AdvancedCollectionView #4349

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d0d4e97
Add Test_AdvancedCollectionView_Filterd_Using_Shaping_Changing_Proper…
w-ahmad Oct 28, 2021
0d1e7c7
Fixed ArgumentOutOfRangeException in AdvancedCollectionView
w-ahmad Oct 28, 2021
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
28 changes: 6 additions & 22 deletions Microsoft.Toolkit.Uwp.UI/AdvancedCollectionView/AdvancedCollectionView.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -677,33 +677,17 @@ private bool HandleItemAdded(int newStartingIndex, object newItem, int? viewInde
return false;
}

if (newStartingIndex == 0 || _view.Count == 0)
{
newViewIndex = 0;
}
else if (newStartingIndex == _source.Count - 1)
{
newViewIndex = _view.Count - 1;
}
else if (viewIndex.HasValue)
if (viewIndex.HasValue)
{
newViewIndex = viewIndex.Value;
}
else
{
for (int i = 0, j = 0; i < _source.Count; i++)
{
if (i == newStartingIndex)
{
newViewIndex = j;
break;
}

if (_view[j] == _source[i])
{
j++;
}
}
newViewIndex = _view.Select(x => _source.IndexOf(x)) // Get indexes of all items that are currently in view
Copy link
Member

@michael-hawker michael-hawker Nov 2, 2021

Choose a reason for hiding this comment

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

Definitely worried about the performance impact here, this seems like a lot of extra work and memory to determine this calculation.

Copy link
Author

@w-ahmad w-ahmad Nov 2, 2021

Choose a reason for hiding this comment

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

Yes it could impact on performance, but not sure about any other possible solutions right now. Someone should have to run benchmark stuff on this method to get a better overview.

Copy link
Member

@michael-hawker michael-hawker Nov 16, 2021

Choose a reason for hiding this comment

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

If the unit test you added shows the failure case at least, then it'd be good if we can figure out how to fix the original algorithm here. We wouldn't want to add extra overhead to this helper.

This class is relatively isolated, so you may be able to pull it out to run benchmarking on it in isolation vs. trying to add benchmarking within the project. @Sergio0694 any tips/resources/articles to point @w-ahmad to for benchmarking code like this?

.Concat(new int[] { newStartingIndex }) // Add index of item that need to be inserted in view
.OrderBy(x => x)
.ToList()
.IndexOf(newStartingIndex); // Get the index for new item in view
}
}

Expand Down
49 changes: 49 additions & 0 deletions UnitTests/UnitTests.UWP/UI/Test_AdvancedCollectionView.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1053,6 +1053,55 @@ public void Test_AdvancedCollectionView_Sorting_CustomComparable_With_Shaping()
Assert.AreEqual(42, ((Person)a.First()).Age);
}

[TestCategory("AdvancedCollectionView")]
[UITestMethod]
public void Test_AdvancedCollectionView_Filterd_Using_Shaping_Changing_Properties()
{
var l = new ObservableCollection<Person>
{
new Person()
{
Name = "lorem",
Age = 4
},
new Person()
{
Name = "imsum",
Age = 8
},
new Person()
{
Name = "dolor",
Age = 15
},
new Person()
{
Name = "sit",
Age = 16
},
new Person()
{
Name = "amet",
Age = 23
},
new Person()
{
Name = "consectetur",
Age = 42
},
};

var a = new AdvancedCollectionView(l, true);
a.ObserveFilterProperty(nameof(Person.Age));
a.Filter = p => ((Person)p).Age <= 5;

Assert.AreEqual(1, a.Count);

l[4].Age = 5;

Assert.AreEqual(2, a.Count);
}

private class DelegateComparable : IComparer
{
private Func<object, object, int> _func;
Expand Down