Skip to content
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

Fixed issue with IncrementalLoadingCollection within AdvancedCollecti… #643

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

k-g-nolan
Copy link

…onView

Fixes #642

Fixed issue #642 by changing how the ACV handles generic types. The change is not perfect, but I wanted to be as non-invasive as possible. Ideally, AdvancedCollectionView would be generically typed itself, but that would be a major change.

PR Type

What kind of change does this PR introduce?

Bugfix

What is the current behavior?

#642
The current behavior throws a NullReferenceException when IncrementalLoadingCollection is used as the source of the ACV. this happens because the first generic type argument of the IncrementalLoadingColleciton is not the type of the objects in the list, but rather the type of the IIncrementalSource used when constructing the ILC.

What is the new behavior?

The new behavior fixes this issue by removing the generic type check and replacing it with the default behavior, which is to use the type of the first object being compared. It's not the most elegant solution, but it's non-invasive.

PR Checklist

Please check if your PR fulfills the following requirements:

Other information

@k-g-nolan

This comment was marked as outdated.

{
type = x.GetType();
}
Type type = x.GetType();
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the PR here. I'm not sure of a .NET convention for the ordering of generic types like this with multiples. But since IncrementalLoadingCollection is also in this same component, we can just check if the type is the collection and grab the proper type, then fallback to the existing logic. I think that'd be a less disruptive change for this specific scenario?

Copy link
Author

Choose a reason for hiding this comment

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

@michael-hawker I added a clause to the if statement to check if the source is an IncrementalLoadingCollection. Let me know how it looks.

@lhak
Copy link
Contributor

lhak commented Feb 26, 2025

This would reintroduce the crashes seen here: #78. Maybe AdvancedCollectionView could be changed to AdvancedCollectionView<T> to explicitly provide the type. I guess, this might also help for AOT support?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants