microsoft / cppwinrt

C++/WinRT
MIT License
1.65k stars 238 forks source link

GetChildrenInTabFocusOrder no longer a member of the generated code #1395

Closed martgoodall closed 6 months ago

martgoodall commented 7 months ago

Version

2.0.240111.5

Summary

after upgrading from 2.0.230706.1 to 2.0.240111.5 , GetChildrenInTabFocusOrder() is no longer a member of the generated code, so the winappsdk/winrt c++ app no longer compiles the exe.

WinUI.zip

Reproducible example

pre upgrade, the following would compile - see attachment for complete source containing the following code:-

c++
   StackPanel stackPanelItems = containerContentChangingEventArgs.ItemContainer().ContentTemplateRoot().as<StackPanel>();
   auto autoItems = stackPanelItems.GetChildrenInTabFocusOrder();

GetChildrenInTabFocusOrder is not generated as a member of the class.

Expected behavior

to still compile existing code under the new release

Actual behavior

GetChildrenInTabFocusOrder is not generated as a member of the class.

Additional comments

app is built successfully with the following:-

<?xml version="1.0" encoding="utf-8"?>

app fails to build by upgrading Microsoft.Windows.CppWinRT

dmachaj commented 7 months ago

@sylveon this appears to be another question related to #1319.

If I understand the description of that PR it should be considered invalid for something outside the StackPanel implementation to call GetChildrenInTabFocusOrder.

sylveon commented 7 months ago

It's an overridable method on UIElement, which means it is protected. So yeah, the proper way would be to inherit from StackPanel to use it, like one would in C#. Alternatively as a workaround, using sp.as<IUIElementOverridable>().GetChildrenInTabFocusOrder() should work (I may have the exact name of the interface wrong).

martgoodall commented 7 months ago

I really appreciate the feedback. I was able to get it to compile under 1.5.0 with the following change:-

auto autoItems = stackPanelItems.as().GetChildrenInTabFocusOrder(); (which is almost what you suggested but IUIElementOverrides instead of IUIElementOverridable).

using winrt c++/windowsappsdk/Xaml, I'm really not sure how to implement the other suggestion of "inherited StackPanel in a similar fashion to the C#".....if anyone had a code sample doing similar, it would be very appreciated, since there are so few decent winrt c++/windowsappsdk/Xaml examples on the web.

again many thanks for the feedback.

sylveon commented 7 months ago

It is not very obvious, but the way to do this is to:

torleifat commented 6 months ago

So I stumbled across this issue just recently. However I also discovered another issue which I think is related, and even if it is, it might be outside the scope of cppwinrt to fix(if it should be fixed at all).

Attempting to call an overridable function from a derived class in Xaml

Say you have an IDL like this:

    [default_interface]
    unsealed runtimeclass A
    {
        A();

        overridable void MyFancyFunction();
    }

    [default_interface]
    runtimeclass B : A
    {
        B();
    }

    [default_interface]
    runtimeclass MainWindow : Microsoft.UI.Xaml.Window
    {
        MainWindow();

        B Model{ get; };
    }

And in your MainWindow xaml you call on MyFancyFunction, something like this:

<Button x:Name="myButton" Click="{x:Bind Model.MyFancyFunction}">Click Me</Button>

In 2.0.230706.1 this will compile and work fine. In 2.0.240111.5 it will lead to a compile-error where it could not find the MyFancyFunction as a member of B. If this was in code I could do something like Model().as<IAOverrides>().MyFancyFunction(). However seeing as this is done in Xaml it complicates matters. Note that there's ways around this, by adding MyFancyFunction to the B's IDL, and not adding any actual override in the implementation it'll successfully call A's MyFancyFunction. In this specific case we could also just remove the override flag, but in a theoretical case there could be a derived class C, which needs to override MyFancyFunction.

Now there's a chance that there are good reasons why this should not be allowed to begin with. I am not very well versed in MIDL 3.0 or C++/Winrt, but I am wondering if this intended. For me it does seem a little counter-intuitive that this should not work.

sylveon commented 6 months ago

This is indeed intended. To quote the WinRT type system:

A composable class may declare zero or more of its member interfaces to be overridable. An overridable interface may only be queried for, and used within, a composition chain—similar to the rules about accessing protected interfaces detailed previously. However, where a protected interface may only be implemented by the class that originally declared it, overridable interfaces may be re-implemented by classes that compose the class that implemented the overridable interface.

martgoodall commented 6 months ago

Thanks for the suggestion.

I believe the new attachment is what you suggested and appears to work but could you please confirm it looks right to you as this stuff is all new to me and it seems to me to be a strange change to now protect GetChildrenInTabFocusOrder forcing a custom method requirement for basic functionality.

WinUI.zip

Xaml reads as follows ( changed to ) :-

============================

 <local:StackPanelViewModel 
             x:Name="StackPanelQueue"
             AllowDrop="True"
             DragOver="GridViewQueues_DragOver"
             Drop="GridViewQueues_Drop"
             Width="200"
             Height="220">

========================

the c++ file reads as follows :-

// old code // StackPanel stackPanelItems = containerContentChangingEventArgs.ItemContainer().ContentTemplateRoot().as(); // // old code // for (auto autoStackPanelItem : stackPanelItems.as().GetChildrenInTabFocusOrder()) // { // }

// new code auto autoStackPanelViewModel = containerContentChangingEventArgs.ItemContainer().ContentTemplateRoot().try_as(); if (autoStackPanelViewModel == nullptr) co_return;

for (auto autoStackPanelItem : autoStackPanelViewModel.get()->ViewModelGetChildrenInTabFocusOrder()) { }

===================

thanks Mart

It is not very obvious, but the way to do this is to:

  • Create a blank page from VS (right-click the project in the Solution Explorer > Add > New Item... > Blank Page (WinUI 3))
  • Open the IDL, change Microsoft.UI.Xaml.Controls.Page to Microsoft.UI.Xaml.Controls.StackPanel
  • Open the XAML, change <Page to <StackPanel and </Page> to </StackPanel>
  • Put some contents, or if you don't want to add XAML to that class and delegate setting the contents to consumers, you can use the View Model (C++/WinRT) template when creating a new item instead of Blank Page (WinUI 3)
  • You should now be able to access GetChildrenInTabFocusOrder within the class: image
sylveon commented 6 months ago

Seems fine to me - FWIW GetChildrenInTabFocusOrder has always been protected virtual in C#.

DefaultRyan commented 6 months ago

Say you have an IDL like this:

    [default_interface]
    unsealed runtimeclass A
    {
        A();

        overridable void MyFancyFunction();
    }

    [default_interface]
    runtimeclass B : A
    {
        B();
    }

    [default_interface]
    runtimeclass MainWindow : Microsoft.UI.Xaml.Window
    {
        MainWindow();

        B Model{ get; };
    }

And in your MainWindow xaml you call on MyFancyFunction, something like this:

<Button x:Name="myButton" Click="{x:Bind Model.MyFancyFunction}">Click Me</Button>

In 2.0.230706.1 this will compile and work fine. In 2.0.240111.5 it will lead to a compile-error where it could not find the MyFancyFunction as a member of B. If this was in code I could do something like Model().as<IAOverrides>().MyFancyFunction(). However seeing as this is done in Xaml it complicates matters. Note that there's ways around this, by adding MyFancyFunction to the B's IDL, and not adding any actual override in the implementation it'll successfully call A's MyFancyFunction. In this specific case we could also just remove the override flag, but in a theoretical case there could be a derived class C, which needs to override MyFancyFunction.

Now there's a chance that there are good reasons why this should not be allowed to begin with. I am not very well versed in MIDL 3.0 or C++/Winrt, but I am wondering if this intended. For me it does seem a little counter-intuitive that this should not work.

First, I'll note that you don't need overridable to customize the implementation within your component. You can do that by making the internal implementation method(s) virtual. You only need to mark a method overridable if your intent is to give component-external consumers the ability to customize some behavior in a way that is visible to the base type(s) in your [shipped] component, and so you needed some way of making this customizability part of the public-facing interface of your component for consumers to extend later on.

It's common practice (I'd even say "standard practice") to separate interface from implementation by making the interface public and the implementation non-public. When dealing with virtual functions, it's a common idiom (but not universal) to treat the public interface as stable and non-virtual, and to treat the customizeable/virtual behavior as implementation by making it non-public. This pattern can be especially useful when the overridable bit needs to be surrounded by pre- and post- action.

A familiar example of this is XAML's Measureand MeasureOverride. Measure is non-virtual, while subclasses can customize their behavior by overriding MeasureOverride. Only the base Measure is callable publicly, and it performs work both before and after calling the overridable MeasureOverride.

Coming back to your example, you need to decide if you truly need overridable in your IDL for external components to subclass and customize your type. You probably don't, and will be able to remove overridable from MyFancyFunction in your IDL. If you want custom behavior between A and B, just make your implementation function virtual. While you're at it, it's probably worth checking your design to verify that B truly "is-a" A - whether it's required that A and B be so tightly coupled in both interface and implementation, or if you'd be better served by separating the two and offloading this common behavior into an interface:

interface IFancyFunction
{
  void FancyFunction();
}

[default_interface]
runtimeclass A : IFancyFunction
{
  A();
}

[default_interface]
runtimeclass B : IFancyFunction
{
  B();
}

Finally, if you truly need B to derive from A, and you truly need a component-external overridable method. Declare a public non-overridable method in the base that calls the overridable method in subclasses. Here Be Dragons - the actual implementation is non-trivial and easy to get wrong. You've been warned. :)

unsealed runtimeclass A
{
  A();

  void FancyFunction();
  overridable void FancyFunctionOverride();
}

[default_interface]
runtimeclass B : A
{
  B();
}

(Fun trivia, C++ virtual function behavior can sometimes get non-intuitive in the face of hiding and overloading - https://isocpp.org/wiki/faq/strange-inheritance#protected-virtuals)

torleifat commented 6 months ago

Thanks for the insight @DefaultRyan. The original code where this came up does indeed not require overridable. Since it is not really customer exposed code to begin with, so it was a quick fix, and as you say if needed an override can be handled in the implementation instead.