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

Unable to implement classic COM interface on XAML component with C++/WinRT #3331

Open
sylveon opened this issue Sep 25, 2020 · 13 comments
Open
Labels
area-XamlCompiler feature proposal New feature proposal future needs-winui-3 Indicates that feature can only be done in WinUI 3.0 or beyond. (needs winui 3) team-Markup Issue for the Markup team

Comments

@sylveon
Copy link
Contributor

sylveon commented Sep 25, 2020

Describe the bug
When creating a XAML control/page, it's not possible to make it implement a classic COM interface (as C++/WinRT does it, by appending it to the base template arguments: Something : SomethingT<Something, IMyClassicInterface>) because the XAML compiler codegen does not account for this case.

Steps to reproduce the bug

  1. Create a new blank C++/WinRT UWP app in VS
  2. Open MainPage.h, and add the following code:
    • Include <windows.ui.xaml.media.dxinterop.h>.
    • Change the struct declaration to struct MainPage : MainPageT<MainPage, ISwapChainPanelNative>.
    • Add IFACEMETHOD(SetSwapChain)(IDXGISwapChain*) noexcept; to the struct members.
  3. In MainPage.cpp, add the implementation:
    IFACEMETHODIMP MainPage::SetSwapChain(IDXGISwapChain*) noexcept
    {
        // dummy implementation
        return S_OK;
    }
  4. Try building the app.
  5. Observe compiler errors.

Expected behavior
The app builds fine and a consumer is able to cast the XAML component to ISwapChainPanelNative and call the method provided by that interface.

Screenshots
image

Version Info

NuGet package version:
[Microsoft.Windows.CppWinRT 2.0.200921.6]

Windows app type:

UWP Win32
Yes Yes
Windows 10 version Saw the problem?
Insider Build (xxxxx)
May 2020 Update (19041) Yes
November 2019 Update (18363)
May 2019 Update (18362)
October 2018 Update (17763)
April 2018 Update (17134)
Fall Creators Update (16299)
Creators Update (15063)
Device form factor Saw the problem?
Desktop Yes
Xbox
Surface Hub
IoT

Additional context
I replied Yes to the app type being Win32 because this issue showed up in system XAML islands while implementing IInitializeWithWindow to pass on a window handle to MessageDialog from my hosting code to the XAML code. I'm using MessageDialog because of ContentDialog's limitation of one dialog open at a time per thread. Implementing threading proved to be a huge pain, so I've decided on using MessageDialog for now.

The bug is caused by this line in MainPage.xaml.g.hpp:

template struct MainPageT<struct MainPage>;

It tries to specialize MainPageT using different parameters than what I used in my declaration, and since the class uses CRTP, it tries to cast this back to MainPage, but because MainPage actually inherits from a different type (different template instantiations are effectively different types), the cast fails.

@ghost ghost added the needs-triage Issue needs to be triaged by the area owners label Sep 25, 2020
@StephenLPeters
Copy link
Contributor

@kennykerr or @jevansaks I feel like we do this already, I think one of you two probably knows the answer. If not this sounds like a xaml compiler bug? @fabiant3

@StephenLPeters StephenLPeters added the team-Markup Issue for the Markup team label Sep 25, 2020
@StephenLPeters
Copy link
Contributor

going with markup team for now.

@sylveon
Copy link
Contributor Author

sylveon commented Sep 25, 2020

Yeah, it is a XAML compiler bug. Without XAML (a normal WinRT runtime component) it does work, but the XAML compiler's explicit specialization in .xaml.g.hpp is what breaks it.

@fabiant3
Copy link
Member

@RealTommyKlein - can you take a look?

@sylveon
Copy link
Contributor Author

sylveon commented Oct 1, 2020

My workaround was implementing the functionality in a subclass that doesn't use the XAML compiler: TranslucentTB/TranslucentTB@8f68f3c

@stevenbrix stevenbrix added needs-winui-3 Indicates that feature can only be done in WinUI 3.0 or beyond. (needs winui 3) future and removed needs-triage Issue needs to be triaged by the area owners labels Nov 16, 2020
@DHowett
Copy link
Member

DHowett commented Nov 24, 2020

Guessing that there's no workaround for this one? Just bit Terminal :/

@sylveon
Copy link
Contributor Author

sylveon commented Nov 24, 2020

There's 2 options:

  • putting this very ugly macro at the bottom of .h and undefining it in the .cpp
    #define MyPage MyPage, IThing
  • Writing the code implementing the classic COM interface in a WinRT class not using the XAML Compiler (Page -> PageWithThing -> MyPage)

@kennykerr
Copy link

@Scottj1s any chance someone from the Xaml compiler can take a look at this seemingly trivial bug?

@DHowett
Copy link
Member

DHowett commented Nov 24, 2020

@sylveon There was a brief but real moment when I realized I was here because I was also implementing IInitializeWithWindow in a XAML islands application that I was quite crestfallen because these are the depths to which we must sink.

Why does the XAML generated header need to forcibly instantiate this template anyway? Does it not suffice to let the consumer do it?

@sylveon
Copy link
Contributor Author

sylveon commented Nov 24, 2020

The XAML codegen does this because some members are forward declared in the .g.h file, which you include, and then implemented + instantiated in .g.hpp, which you don't include (it's built as part of XamlTypeInfo.g.cpp).

@sylveon
Copy link
Contributor Author

sylveon commented Nov 24, 2020

Without this explicit instantiation, you get linker errors.

DHowett added a commit to microsoft/terminal that referenced this issue Nov 24, 2020
This fixes the issue with the settings UI where clicking the browse
buttons would cause an exception to be thrown when we tried to display a
picker without an originating HWND.

It turns out that pickers need a hosting/parent window, and Xaml Islands
doesn't furnish us with a CoreWindow that's set up for that use case.
Alas!

Raymond Chen's [blog post on the matter] suggests that we should
hand the HWND off through some classic COM interface. To do that
properly, Terminal's various components need to implement that interface
and propagate the HWND down where it's needed.

Thanks to a [Xaml compiler issue], we can't actually do that. To work
around that, we've begged and borrowed different methods for pushing
HWNDs around:

1. Using IInitializeWithWindow in secret
2. A member that takes a uint64
3. An interface that offers a function that will "wire up" the HWND.

I chose (1) because AppHost can implement IInitializeWithWindow, but
TerminalPage cannot. We're just pretending that TerminalPage _can_.

I chose (2) because none of the Xaml types in TerminalSettingsEditor can
implement the interface thanks to the aforementioned compiler issue, but
we don't have an escape hatch like AppHost that lives in the same module
and can help us do the propagation.

I chose (3) because I didn't want to commit the same sin as (2) _seven
times_ for every different type of settings page that exists. (3) is
backed by "IHostedInWindow", and anybody who knows they have to use
IInitializeWithWindow to tie an HWND to an object can call
IHostedInWindow.TryPropagateHostingWindow() on that object.

House of cards.

[Xaml compiler issue]: microsoft/microsoft-ui-xaml#3331
[blog post on the matter]: https://devblogs.microsoft.com/oldnewthing/20190412-00/?p=102413
DHowett added a commit to microsoft/terminal that referenced this issue Nov 30, 2020
This fixes the issue with the settings UI where clicking the browse
buttons would cause an exception to be thrown when we tried to display a
picker without an originating HWND.

It turns out that pickers need a hosting/parent window, and Xaml Islands
doesn't furnish us with a CoreWindow that's set up for that use case.
Alas!

Raymond Chen's [blog post on the matter] suggests that we should
hand the HWND off through some classic COM interface. To do that
properly, Terminal's various components need to implement that interface
and propagate the HWND down where it's needed.

Thanks to a [Xaml compiler issue], we can't actually do that. To work
around that, we've begged and borrowed different methods for pushing
HWNDs around:

1. Using IInitializeWithWindow in secret
2. A member that takes a uint64
3. An interface that offers a function that will "wire up" the HWND.

I chose (1) because AppHost can implement IInitializeWithWindow, but
TerminalPage cannot. We're just pretending that TerminalPage _can_.

I chose (2) because none of the Xaml types in TerminalSettingsEditor can
implement the interface thanks to the aforementioned compiler issue, but
we don't have an escape hatch like AppHost that lives in the same module
and can help us do the propagation.

I chose (3) because I didn't want to commit the same sin as (2) _seven
times_ for every different type of settings page that exists. (3) is
backed by "IHostedInWindow", and anybody who knows they have to use
IInitializeWithWindow to tie an HWND to an object can call
IHostedInWindow.TryPropagateHostingWindow() on that object.

House of cards.

[Xaml compiler issue]: microsoft/microsoft-ui-xaml#3331
[blog post on the matter]: https://devblogs.microsoft.com/oldnewthing/20190412-00/?p=102413
DHowett added a commit to microsoft/terminal that referenced this issue Nov 30, 2020
This fixes the issue with the settings UI where clicking the browse
buttons would cause an exception to be thrown when we tried to display a
picker without an originating HWND.

It turns out that pickers need a hosting/parent window, and Xaml Islands
doesn't furnish us with a CoreWindow that's set up for that use case.
Alas!

Raymond Chen's [blog post on the matter] suggests that we should
hand the HWND off through some classic COM interface. To do that
properly, Terminal's various components need to implement that interface
and propagate the HWND down where it's needed.

Thanks to a [Xaml compiler issue], we can't actually do that. To work
around that, we've begged and borrowed different methods for pushing
HWNDs around:

1. Using IInitializeWithWindow in secret
2. A member that takes a uint64
3. An interface that offers a function that will "wire up" the HWND.

I chose (1) because AppHost can implement IInitializeWithWindow, but
TerminalPage cannot. We're just pretending that TerminalPage _can_.

I chose (2) because none of the Xaml types in TerminalSettingsEditor can
implement the interface thanks to the aforementioned compiler issue, but
we don't have an escape hatch like AppHost that lives in the same module
and can help us do the propagation.

I chose (3) because I didn't want to commit the same sin as (2) _seven
times_ for every different type of settings page that exists. (3) is
backed by "IHostedInWindow", and anybody who knows they have to use
IInitializeWithWindow to tie an HWND to an object can call
IHostedInWindow.TryPropagateHostingWindow() on that object.

House of cards.

[Xaml compiler issue]: microsoft/microsoft-ui-xaml#3331
[blog post on the matter]: https://devblogs.microsoft.com/oldnewthing/20190412-00/?p=102413

(cherry picked from commit f9fc986)
@evelynwu-msft evelynwu-msft added bug Something isn't working area-XamlCompiler labels Nov 29, 2022
@sylveon
Copy link
Contributor Author

sylveon commented Jul 15, 2023

Bump, still an issue

@JesseCol JesseCol added feature proposal New feature proposal and removed bug Something isn't working labels Feb 6, 2024
@amtopel
Copy link

amtopel commented May 30, 2024

This bug also prevents you from adding any additional arguments whatsoever to the base class template--not just COM interfaces, but also marker structs, etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-XamlCompiler feature proposal New feature proposal future needs-winui-3 Indicates that feature can only be done in WinUI 3.0 or beyond. (needs winui 3) team-Markup Issue for the Markup team
Projects
None yet
Development

No branches or pull requests

9 participants