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

Pass through ComWrappers based objects to COM type descriptor #89887

Merged
merged 1 commit into from
Aug 3, 2023

Conversation

JeremyKuhne
Copy link
Member

WinForms hosts the the COM type descriptor. It has been updated to work with ComWrappers based objects- adding the check for them in TypeDescriptor.

There are hard dependencies on Windows in the current implementation. We can potentially update to conditionalize these. dotnet/winforms#9291 tracks.

There is no automated test as it would require adding a dependency upstream to WinForms.

cc: @ericstj @AaronRobinsonMSFT

WinForms hosts the the COM type descriptor. It has been updated to work with ComWrappers based objects- adding the check for them in TypeDescriptor.

There are hard dependencies on Windows in the current implementation. We can potentially update to conditionalize these. dotnet/winforms#9291 tracks.

There is no automated test as it would require adding a dependency upstream to WinForms.
@ghost
Copy link

ghost commented Aug 2, 2023

Tagging subscribers to this area: @dotnet/area-system-componentmodel
See info in area-owners.md if you want to be subscribed.

Issue Details

WinForms hosts the the COM type descriptor. It has been updated to work with ComWrappers based objects- adding the check for them in TypeDescriptor.

There are hard dependencies on Windows in the current implementation. We can potentially update to conditionalize these. dotnet/winforms#9291 tracks.

There is no automated test as it would require adding a dependency upstream to WinForms.

cc: @ericstj @AaronRobinsonMSFT

Author: JeremyKuhne
Assignees: JeremyKuhne
Labels:

area-System.ComponentModel

Milestone: -

@ericstj ericstj requested a review from a team August 3, 2023 16:45
@ericstj
Copy link
Member

ericstj commented Aug 3, 2023

There is no automated test as it would require adding a dependency upstream to WinForms.

Can we copy a smaller portion of that code here to test - less as the "real scenario" and more as just a mock - or would that require too much duplication?

@JeremyKuhne
Copy link
Member Author

Can we copy a smaller portion of that code here to test - less as the "real scenario" and more as just a mock - or would that require too much duplication?

@ericstj I think it would be more effort than is worth it. I've got tests for this already in WinForms, I just need to switch them over to get the converter from TypeConverter. I'll link that PR back here after everything flows.

@AaronRobinsonMSFT
Copy link
Member

AaronRobinsonMSFT commented Aug 3, 2023

@JeremyKuhne Wouldn't it be possible to write a simple test that calls TypeDescriptor.GetProvider() and pass in a ComWrappers created COM object and a normal built-in COM object and validate the same is returned? I agree this might not be interesting, just want to make sure we consider the cost.

@JeremyKuhne
Copy link
Member Author

built-in COM object and validate the same is returned?

Maybe? It throws when WinForms isn't available, I'd have to handle all cases gracefully. I don't think this is likely to break and it will be caught when WinForms tries to update dependencies.

@AaronRobinsonMSFT
Copy link
Member

Maybe? It throws when WinForms isn't available,

Oh. Nevermind. I agree adding tests is probably not worth the effort.

Copy link
Member

@ericstj ericstj left a comment

Choose a reason for hiding this comment

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

Thanks for considering tests. I'm OK with the resolution to only test in WinForms.

@ericstj ericstj merged commit f43b0c1 into dotnet:main Aug 3, 2023
99 of 103 checks passed
@ericstj
Copy link
Member

ericstj commented Aug 3, 2023

Failing tests are known issues.

@ghost ghost locked as resolved and limited conversation to collaborators Sep 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants