Compiled binding path for subclasses - support for mixed collection#382
Open
DanielTrommel wants to merge 4 commits into
Open
Compiled binding path for subclasses - support for mixed collection#382DanielTrommel wants to merge 4 commits into
DanielTrommel wants to merge 4 commits into
Conversation
… in the BindingPath string). Included test cases for collection of items with the same parent class, on which shared properties and indexers are defined. Signed-off-by: Daniël Trommel <d.s.trommel@gmail.com>
…tial naming slip-up from when I first build it (focussed on properties) Signed-off-by: Daniël Trommel <d.s.trommel@gmail.com>
…s the original Getter flow; when navigating to the final segment, the code is exactly the same. Only the final bit of code (get versus set) is different.
This refactor also exposed a subtle bug; GetCompiledValueSetter_ShouldNotSet_ForOutOfBoundsNonGenericListIndex failed with ArgumentOutOfRangeException. This was a latent bug in the setter that the old over-specialization was masking:
- For a property typed IList (non-generic ArrayList), the shared navigation correctly leaves current.Type as the interface IList (no over-specialization).
- The setter's bounds-check did current.Type.GetProperty("Count"), but Count is declared on the base ICollection interface — and reflection doesn't surface inherited members on interface types. So countProperty was null, the bounds check was skipped, and the raw indexer assignment threw.
- The old setter only "worked" by accident: it had converted current to the concrete ArrayList, where Count is found directly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
@w-ahmad , back again with an fix/improvement on the Func/BindingPath code.
Ref this PR, #178, in which we worked on generating a Func at runtime once, instead of repeated reflection to get a cell's value.
In the comments on that PR (#178 (comment)), I already mentioned that the current Func is built just using the first instance for which we happen do a GetPropertyValue(), which might cause trouble.
The specific problem: If the instances used for the rows in the table is a mixture of subclasses, and the (first) binding (step) is to an element on the shared parent class, the current code will throw an InvalidCastException when sorting.
The reason is that the first instance that is used to built the Func, dictates the explicit type cast that is done;
An example: suppose the type of the instance is MySubClass and for a column our BindingPath is "MyProperty", the current code in main generates this:
However, if MyProperty is actually defined on MySuperClass, that code should use a cast to that type instead:
This way, if the CollectionView contains a mixture of instance of different subclasses for MySuperClass, we won't get an InvalidCastException.
Furthermore:
main, I noticed you added a func for the Setter variant, so I also hardened your code so it will handle subclassingThe above points are in order and in separate commits.