Integrate Shmueli's ComServer into CsWinRT #1995
Conversation
|
I don't get why we would need yet another standalone WinRT server implementation and additional ComWrappers when it can already be achieved by passing the generated |
My guess is that it has to do with App Actions, announced in Build.
Separation of concerns? |
The goal behind this PR is to provide a higher-level abstraction around this and also add features to it such as lifetime management as there are going to be more scenarios relying on this support. The lifetime management piece also sounds like it is going to need to be able to keep track of the instances for which CCWs are created for in the server, so it is going to achieve that by deriving from |
| using System; | ||
| using System.Runtime.Versioning; | ||
|
|
||
| namespace ComServerHelpers; |
There was a problem hiding this comment.
We should really think about what actual namespace names to use. And type names.
src/ComServerHelpers/Internal/Windows/Com/Marshalling/TrustLevelMarshaller.cs
Outdated
Show resolved
Hide resolved
| /// </returns> | ||
| /// <seealso href="https://learn.microsoft.com/en-us/previous-versions/br205771(v=vs.85)">DllGetActivationFactory entry point</seealso> | ||
| [UnmanagedFunctionPointer(CallingConvention.StdCall)] | ||
| internal unsafe delegate HRESULT DllGetActivationFactory(HSTRING activatableClassId, IActivationFactory** factory); |
There was a problem hiding this comment.
We should not be marshalling function pointers from delegates. Can we just use [UnmanagedCallersOnly]?
There was a problem hiding this comment.
I was going to comment this, but they got local state that they are maintaining.
There was a problem hiding this comment.
@Sergio0694 any way around this while maintaining local state?
There was a problem hiding this comment.
Can you have more than one instance of a given COM server class at the same time?
| /// <threadsafety static="true" instance="false"/> | ||
| [SupportedOSPlatform("windows8.0")] | ||
| [System.Diagnostics.CodeAnalysis.SuppressMessage("Naming", "CA1724", Justification = "No better idea")] | ||
| public sealed class WinRtServer : IDisposable |
There was a problem hiding this comment.
Definitely let's not use "WinRt" as a type name. Should at the very least be "WinRT".
Ideally this should eventually be "WindowsRuntimeServer" in 3.0. We can do that now or later,
There was a problem hiding this comment.
I'm leaving typenames/namespace changing to be very last changes.
| public unsafe WinRtServer() | ||
| { | ||
| activationFactoryCallbackWrapper = ActivationFactoryCallback; | ||
| activationFactoryCallbackPointer = (delegate* unmanaged[Stdcall]<HSTRING, IActivationFactory**, HRESULT>)Marshal.GetFunctionPointerForDelegate(activationFactoryCallbackWrapper); |
There was a problem hiding this comment.
Yeah definitely let's not do this. Use a function pointer directly instead.
There was a problem hiding this comment.
The callback here relies on state, so I'm not sure about the feasibility of this.
src/ComServerHelpers/Internal/Windows/Com/Marshalling/TrustLevelMarshaller.cs
Outdated
Show resolved
Hide resolved
|
Closing this -- we discussed this offline and we're going to move this to a separate repo and NuGet package, rather than merging it into the CsWinRT repo and shipping it as part of the CsWinRT package. This follows a similar approach to what WindowsAppSDK has also done with the new package split, making the various components more modular and a-la-carte. In particular:
We'll be moving the code from this PR to continue that work, so thank you everyone for the reviews, those still helped! 😄 |
Moving https://github.com/shmuelie/Shmuelie.WinRTServer to CsWinRT repo.