InspectingDataSource attaches the wrong event handler type to IBindableObservableVector. #883
Conversation
…ched to IBindableVector causing a crash.
| } | ||
| else if (auto observableCollection = m_observableVector.safe_get()) | ||
| { | ||
| observableCollection.VectorChanged(m_eventToken); | ||
| } | ||
| else if (auto bindableObservableCollection = m_bindableObservableVector.safe_get()) |
There was a problem hiding this comment.
Thanks! Please ensure that all the tests pass in the PR pipeline
There was a problem hiding this comment.
We also must not have test coverage for this. @ranjeshj can you help add tests in the NugetRelease test? If it's CX only we have a CX test app there.
|
@jevansaks @ranjeshj build shows an error running tests, because the branch is from a fork. Can you take a look? Error mentions you might have to enable something for this to work. |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Sorry! We just merged the build and test pipelines and the settings for forks hadn't been carried over. I kicked another run, hopefully that works. I'll keep an eye on it. |
|
Looks like all the tests passed. What else is left to get this merged? |
|
Johan, can you add a quick test to the release cx app ? you add the cx code to this app - \test\MUXControlsReleaseTest\NugetPackageTestAppCX and add a test to test\MUXControlsReleaseTest\MUXControls.ReleaseTest\NugetTestsCX.cs. During the pipeline, the test will run against the nuget package that gets built. It does not look like there are any tests in that app to follow. To make it simple, perhaps you could just add a repeater to the mainpage for now and file an issue to create infra for CX tests. |
|
@ranjeshj Sure, I'll give that a shot. |
|
@ranjeshj I added some tests. Let me know what you think. |
|
looks fine. will merge once the pipeline completes |
|
/azp run |
|
Commenter does not have sufficient privileges for PR 883 in repo microsoft/microsoft-ui-xaml |
|
Doesn't look like the pipeline actually runs after I pushed changes. |
|
I believe it is running. the results will be updated once the run completes. |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
jevansaks
added
the
release note
|
Thanks for fixing this @jlaanstra! Do you need this pulled into the 2.1 framework package? |
|
🎉 Handy links: |
This PR fixes a bug in InspectingDataSource where the wrong event handler type is attached to an IBindableObservableVector causing an InvalidCastException when a change notification is raised by the Platform::Collections::Vector class in collection.h.
When attaching to a IBindableObservableVector, the event handler passed is of type VectorChangedEventHandler which is incorrect and should be of type BindableVectorChangedEventHandler.
I also swapped the order of IBindableObservableVector and IObervableVector as the former seems much more common.