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

Project COM interop APIs #830

Merged
merged 24 commits into from
Jun 11, 2021
Merged

Project COM interop APIs #830

merged 24 commits into from
Jun 11, 2021

Conversation

angelazhangmsft
Copy link
Contributor

@angelazhangmsft angelazhangmsft commented Apr 29, 2021

  • Defines IInspectable-based interop interfaces in IDL and generate projections. Also defines the ComImport for IWindowNative and IInitiazlieWithWindow. The generated projections of the interfaces are internal.
  • Defines helper wrapper classes implementing the generated interfaces
  • Add unit tests for the interop classes

This work will provide definitions for several COM interop interfaces through the Windows SDK projection, including:
WinRT.Interop.IWindowNative, WinRT.Interop.IInitializeWithWindow, Windows.Storage.Streams.IBufferByteAccess and wrappers for all WinRT interop interfaces included in the Universal API Contract (e.g. Windows.Security.Credentials.UI.UserConsentVerifierInterop, Windows.UI.ApplicationSettings.AccountsSettingsPaneInterop)

@Scottj1s
Copy link
Member

Scottj1s commented Apr 29, 2021

responses to questions:

Naming convention (Should the interop APIs live under a "Windows.Interop" namespace?)
using the win32metadata package sidesteps this

Projections of interfaces and helper classes are both public. We could consider making the projected interfaces internal to avoid confusion.
We would need to change cswinrt.exe to support 'internal' projections, but I think this is a good idea. Aside from discoverability (which extension method wrappers would solve), there are too many sharp edges to use the projection directly - ensuring the RIIDs match return type casts, and even knowing what those return types are, etc.

Only supporting interop interfaces with UniversalApiContract (e.g. not Desktop extension APIs like IProtectionPolicyManagerInterop)
this makes sense to me - we can always generated extension interop assemblies, if necessary.

Versioning by Windows version: could add contract version attributes to IDL definitions, which will project SupportedOSPlatform attribute
Note that the win32metadata package includes a definition for Windows.Win32.Interop.SupportedOSPlatformAttribute, which we can of course project to System.Runtime.Versioning.SupportedOSPlatformAttribute as we already do for ContractVersion. That's another reason to use the win32metadata package - to avoid special solutions to projection requirements like these.

@angelazhangmsft
Copy link
Contributor Author

Thank you everyone for all the helpful feedback! I am closing this PR for now and we will investigate the win32metadata approach, and adding support in C#/WinRT for IUnknown interfaces. Will open a separate PR using this feedback.

@angelazhangmsft
Copy link
Contributor Author

Re-opening this PR, addressing the prior feedback and made the following updates:

  • Changed the namespaces to adding the interop-specific classes to the same namespace as the main object. For example: Windows.UI.ApplicationSettings.AccountSettingsPane Windows.UI.ApplicationSettings.AccountsSettingsPaneInterop.GetForWindow(hwnd)
  • Decided to continue defining interop interfaces in IDL, since the win32metadata package does not have winmd file with "windowsruntime" attributed metadata
  • Added support in C#/WinRT for internal-only projection of the interop interfaces, so only the wrapper classes (not the interfaces) are exposed to the developer.
  • Added custom mapping of WinRT.Interop.HWND to System.IntPtr
  • Versioning: Instead of using SupportedOSPlatform/ContractVersion, we are using build-time filtering of types (conditional compilation) to support different versions of the Windows SDK projection.

@Scottj1s
Copy link
Member

Scottj1s commented May 7, 2021

  • Versioning: Instead of using SupportedOSPlatform/ContractVersion, we are using build-time filtering of types (conditional compilation) to support different versions of the Windows SDK projection.

A couple points here -

  1. The developer selects a version of the Windows SDK projection via TFM (e.g., net5.0-windows10.0.18362.0), so later API additions are not visible. It makes sense for the interop interfaces to follow suit. The alternative would be confusing - having interop interfaces that are not available on the target platform version.
  2. SupportedOSPlatform does not support interfaces, so there's no good way to inform the developer if an interop interface were not available on the target.

@Scottj1s Scottj1s requested a review from manodasanW May 7, 2021 16:16
@Scottj1s
Copy link
Member

Scottj1s commented May 7, 2021

@manodasanW, adding you to review as this will be the pattern that we want to implement in Microsoft.Windows.SDK.NET.dll

Scottj1s
Scottj1s previously approved these changes May 10, 2021
Copy link
Member

@Scottj1s Scottj1s left a comment

Choose a reason for hiding this comment

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

Looks good enough now to provide as a template for the Windows SDK projection (which was the ultimate goal)

@angelazhangmsft angelazhangmsft added the AssemblyVersion change Requires assembly version change for WinRT.Runtime label May 19, 2021
@Scottj1s
Copy link
Member

@manodasanW, @angelazhangmsft - automatic support for Windows SDK projection implemented - please review

Copy link
Member

@Scottj1s Scottj1s left a comment

Choose a reason for hiding this comment

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

awaiting others' approval

@Scottj1s Scottj1s dismissed their stale review June 10, 2021 16:10

awaiting others' approval

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AssemblyVersion change Requires assembly version change for WinRT.Runtime
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants