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

CsWin32 usage sample #7929

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

Conversation

JeremyKuhne
Copy link
Member

@JeremyKuhne JeremyKuhne commented Jun 15, 2023

This is a draft to drive the conversation around utilizing CsWin32 in WPF. In it I've converted SplashScreen to use CsWin32 and directly utilize WIC pointers.

This is informed by WinForms transition and utilizes some of the code, notably ComScope and IID. There is more support code that WinForms uses that eases handling of COM situations, including providing ComWrapper based "CCWs" for managed classes.

WinForms also contains other patterns for scoping handles, handling GC keep-alive scenarios, and managing agiile COM pointers and VARIANT interop.

As CsWin32 types Win32 typedefs (such as HWND, and HRESULT) it can get very awkward to exchange these if you use CsWin32 directly in multiple assemblies in the assembly graph. As such I created a new shared assembly for the interop code.

I added a batch file to simplify Visual Studio in the right context (copied from WinForms).

Choices here were expedient and made with the goal of illustrating how to do this. I have not tested anything outside of ensuring compilation works.

Microsoft Reviewers: Open in CodeFlow

This is a draft to drive the conversation around utilizing CsWin32 in WPF. In it I've converted SplashScreen to use CsWin32 and directly utilize WIC pointers.

This is informed by WinForms transition and utilizes some of the code, notably ComScope and IID. There is more support code that WinForms uses that eases handling of COM situations, including providing ComWrapper based "CCWs" for managed classes.

WinForms also contains other patterns for scoping handles, handling GC keep-alive scenarios, and managing agiile COM pointers and VARIANT interop.

As CsWin32 types Win32 typedefs (such as HWND, and HRESULT) it can get very awkward to exchange these if you use CsWin32 directly in multiple assemblies in the assembly graph. As such I created a new shared assembly for the interop code.

I added a batch file to simplify Visual Studio in the right context (copied from WinForms).

Choices here were expedient and made with the goal of illustrating how to do this. I have not tested anything outside of ensuring compilation works.
@JeremyKuhne JeremyKuhne added this to the Future milestone Jun 15, 2023
@JeremyKuhne JeremyKuhne requested a review from a team as a code owner June 15, 2023 00:35
@ghost ghost assigned JeremyKuhne Jun 15, 2023
@ghost ghost added the PR metadata: Label to tag PRs, to facilitate with triage label Jun 15, 2023
@ghost ghost requested review from dipeshmsft and singhashish-wpf June 15, 2023 00:35
@JeremyKuhne
Copy link
Member Author

cc: @dipeshmsft, @AArnott, @lonitra

@JeremyKuhne
Copy link
Member Author

If the interop assembly can be built for x86, x64, and arm64 that avoids an AnyCpu issue with CsWin32 that we're working on. I don't really have a handle on how things are packaged/deployed so I don't know how easy that is to accomplish.

The basic issue is that some Win32 APIs are not technically AnyCPU friendly. More specifically this has to do with some struct definitions. The fields in the struct lay out the same way on x86/x64, but packing would cause the structs to not be aligned correctly when contained in other structs. In internal usage this doesn't really come up- we've always been able to write PInvokes in WinForms for AnyCPU. There is an ask on CsWin32 to be able to make an "unsafe" ask for a given platform projection for these cases.

None of this hits this initial PR. The notable place this comes up is in some of the shell APIs which define with a PACK=1.

@ghost ghost removed the draft label Jun 15, 2023
Copy link

@AArnott AArnott left a comment

Choose a reason for hiding this comment

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

Love it.

NuGet.config Outdated Show resolved Hide resolved
<!--
We don't care about CLS compliance since everything here is internal and we want to match native types.
-->
<NoWarn>$(NoWarn);CS3016</NoWarn>
Copy link
Member

Choose a reason for hiding this comment

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

💡 Applying [assembly: CLSCompliant(false)] will disable this warning and disable CLS compliance analysis which significantly improves compiler performance.

Comment on lines +28 to +31
<PackageReference Include="Microsoft.Windows.CsWin32" Version="0.3.5-beta">
<PrivateAssets>all</PrivateAssets>
<IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
</PackageReference>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<PackageReference Include="Microsoft.Windows.CsWin32" Version="0.3.5-beta">
<PrivateAssets>all</PrivateAssets>
<IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
</PackageReference>
<PackageReference Include="Microsoft.Windows.CsWin32" Version="0.3.5-beta" PrivateAssets="all" />

start-vs.cmd Show resolved Hide resolved
Comment on lines +23 to +25
/// <para>
/// Take care to NOT make copies of the struct to avoid accidental over-release.
/// </para>
Copy link
Member

Choose a reason for hiding this comment

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

💡 Roslyn's [NonCopyable] can be used for this

Copy link

Choose a reason for hiding this comment

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

Can you provide more information on where this attribute is defined and which analyzer package needs to be installed to leverage it?

Copy link
Member

@sharwell sharwell Jun 28, 2023

Choose a reason for hiding this comment

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

It's part of Roslyn.Diagnostics.Analyzers today:
https:/dotnet/roslyn-analyzers/blob/main/src/Roslyn.Diagnostics.Analyzers/Core/AbstractDoNotCopyValue.cs

It's fairly unforgiving. Any use of a non-copyable value which doesn't match a known non-copying value will produce a diagnostic. It also relies on IOperation, so I'm not sure if it handles all cases where a piece of syntax doesn't have IOperation support. In practice, it's worked well for us with minimal hassle.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll look into it, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR metadata: Label to tag PRs, to facilitate with triage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants