contextfree / winrt-rust

Use and (eventually) make Windows Runtime APIs with Rust
Apache License 2.0
142 stars 10 forks source link

Hide ComPtr from public API #80

Closed Boddlnagg closed 4 years ago

Boddlnagg commented 5 years ago

Fixes #79

Boddlnagg commented 4 years ago

@contextfree Have you tried to use this? Any objections to merging?

contextfree commented 4 years ago

Yes. I'm not quite done with this exercise but my main concern is that I'm afraid keeping ComPtr private might make it harder to use interop interfaces like ICompositorDesktopInterop, which are used by QueryInterface-ing to them from WinRT objects (Compositor in this case). Though I'm not sure this is really necessary, right now my prototype that uses these interfaces only works through a forked version of winrt-rust that exposes ComPtr and some other functions. I know you weren't intending to provide general COM support with winrt-rust ( #81 ) but I think it might make sense to provide enough to support these interop patterns.

Boddlnagg commented 4 years ago

Thanks! So this interop scenario would be a similar thing like https://github.com/contextfree/winrt-rust/issues/70, right? I agree that it would be good to provide some convenience APIs there. Do you have a good idea how to expose these "internal" APIs? Wouldn't you need to add boilerplate like the following for any interface that you want to use:

pub struct IUnknown(ComPtr<w::um::unknwnbase::IUnknown>);
impl ComIid for IUnknown { #[inline] fn iid() -> &'static Guid { &IID_IUnknown } }
impl ComInterface for IUnknown { type Vtbl = IUnknownVtbl; }
impl ComInterfaceAbi for w::um::unknwnbase::IUnknown {
    type Vtbl = w::um::unknwnbase::IUnknownVtbl;
    #[inline]
    fn get_vtbl(&self) -> *const Self::Vtbl {
        self.lpVtbl
    }
}
impl ComInterface for IUnknown {
    type TAbi = w::um::unknwnbase::IUnknown;
    #[inline] unsafe fn wrap_com(ptr: *mut Self::TAbi) -> Self { IUnknown(ComPtr::wrap(ptr)) }
    #[inline] fn get_abi(&self) -> &Self::TAbi { self.0.as_abi() }
}

How did you modify your forked version to make it possible?

contextfree commented 4 years ago

Yes, I had to write some boilerplate very similar to what you posted. As for ideas for how it should be done, I haven't really thought this through yet so might be overlooking something, but here's a brain dump of my current half-formed thoughts ...

Ideally we might expose a macro similar to RT_INTERFACE! but for "classic" COM interfaces that use the subset of COM that WinRT uses (which I think these interop interfaces limit themselves to). (We'd scope our COM support to just that core subset, so none of the quasi-deprecated (?) older stuff that was built on top of it like IDispatch, OLE Automation, etc.) So this COMBASE_INTERFACE! (?) would be RT_INTERFACE! but generate an IUnknown instead of IInspectable, and the method parameters would be raw winapi types instead of WinRT ABI types.

It might make sense to implement support for some of these interfaces directly in the winrt-rust crate itself, but I think we probably need to support third party implementation as not sure we'd be able to manually implement everything ourselves in a timely manner. (The "layering" here is kind of weird because some of these interfaces reference WinRT types, despite being defined in Windows SDK headers like the APIs normally wrapped by the winapi crate, so they don't feel like quite a perfect fit for either the winapi or winrt crate. Related question is, if someone does make a crate supporting the extended functionality of "classic" COM, would it ideally be layered on top of the same core COM support as winrt?)

If you like I'll take a crack at implementing this myself and submit sometime this weekend to get your opinion.

Boddlnagg commented 4 years ago

It'd be great if you could provide an implementation (based on this branch).

I was wondering about the layering as well ... maybe we split it into two crates? One "base" crate that contains the plumbing, and that can be used for interop with classic COM, and a second one that's providing the actual language projection of the WinRT library?

contextfree commented 4 years ago

PR into this source branch implementing the COM interop macro: https://github.com/Boddlnagg/winrt-rust/pull/1

About the layering issue, I think we should punt on it until (1) this library is more complete and has been more extensively validated for its intended (WinRT) usage (for example, when we finally get around to figuring out how to support inheritance for UI, I'm afraid it might require some refactoring that premature layering could make more difficult) and (2) we have a partner who is actually interested in implementing extended non-WinRT COM support, so we can get their feedback on our base layer API design. So far, it looks to me like the existing com_rs and wio projects don't venture much (or any?) outside the same basic COM subset that's shared with WinRT, but support for things like IDispatch or OLE would likely require someone to write a whole additional mass of macros and implementation types and functions ...

Boddlnagg commented 4 years ago

I just built this locally and ran the tests, everything seems to work fine.