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

[UWP]: Renderer [WRL] -> [WinRT] #6794

Merged
merged 42 commits into from
Jan 28, 2022
Merged

Conversation

beervoley
Copy link
Contributor

@beervoley beervoley commented Nov 30, 2021

Related Issue

This PR closes #6323.

Description

I've converted UWP Renderer to WinRT.

How Verified

In the process of verification
I've introduced some regressions and still working on them.
There are a lot of spacing issues, not sure why clang formatter haven't dealt with them or may be I didn't run it on everything yet - I'm still working on it (I apologize for that).

Reviewing

I've left a lot of comments where I'm unsure of what I've done with the mark TODO.
Everybody who's reviewing the changes, could you please carefully look at those comments and respond if you know how to do it correctly :)

Branch with the history and comments if anybody is curious: https://github.com/microsoft/AdaptiveCards/tree/u/vsiliush/renderer-winrt

Compilation Warnings

I've turned off all Warnings for now as well as removed treating warnings as errors (/Wall and /WX are turned off). I'll restore it in the end when the PR is ready to merge.

Microsoft Reviewers: Open in CodeFlow

Copy link
Member

@jonwis jonwis left a comment

Choose a reason for hiding this comment

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

Looking like a really great start! A few things noted - bunch of TODOs that need addressing, one comment about converting an E_FAIL/return-out-to-caller to an access-violation, and a few nits on the change.

source/uwp/Renderer/idl/AdaptiveCards.Rendering.Uwp.idl Outdated Show resolved Hide resolved
source/uwp/Renderer/idl/AdaptiveCards.Rendering.Uwp.idl Outdated Show resolved Hide resolved
source/uwp/Renderer/idl/AdaptiveCards.Rendering.Uwp.idl Outdated Show resolved Hide resolved
source/uwp/Renderer/lib/AdaptiveActionEventArgs.h Outdated Show resolved Hide resolved
source/uwp/Renderer/lib/AdaptiveActionInvoker.h Outdated Show resolved Hide resolved
source/uwp/Renderer/lib/ImageLoadTracker.cpp Outdated Show resolved Hide resolved
source/uwp/Renderer/lib/ImageLoadTracker.cpp Outdated Show resolved Hide resolved
source/uwp/Renderer/lib/InputValue.cpp Outdated Show resolved Hide resolved
source/uwp/Renderer/lib/InputValue.cpp Show resolved Hide resolved
source/uwp/Renderer/lib/Vector.h Outdated Show resolved Hide resolved
source/uwp/Renderer/idl/AdaptiveCards.Rendering.Uwp.idl Outdated Show resolved Hide resolved
source/uwp/Renderer/idl/AdaptiveCards.Rendering.Uwp.idl Outdated Show resolved Hide resolved
source/uwp/Renderer/idl/AdaptiveCards.Rendering.Uwp.idl Outdated Show resolved Hide resolved
source/uwp/Renderer/lib/ActionHelpers.h Outdated Show resolved Hide resolved
source/uwp/Renderer/lib/AdaptiveActionEventArgs.h Outdated Show resolved Hide resolved
source/uwp/Renderer/lib/AdaptiveActionInvoker.h Outdated Show resolved Hide resolved
@licanhua
Copy link
Contributor

licanhua commented Nov 30, 2021

    rtxaml::Thickness buttonMargin{};

If it's set correctly. I don't think you need rtxaml anywhere. just winrt::Thickness.
For example,
rtxaml::Controls::TextBlock tooltipTextBlock can be winrt::TextBlock too


In reply to: 982875118


In reply to: 982875118


Refers to: source/uwp/Renderer/lib/ActionHelpers.cpp:15 in a74771b. [](commit_id = a74771b, deletion_comment = False)

source/uwp/Renderer/lib/ActionHelpers.cpp Outdated Show resolved Hide resolved
source/uwp/Renderer/lib/AdaptiveColumnRenderer.cpp Outdated Show resolved Hide resolved
source/uwp/Renderer/lib/XamlBuilder.h Outdated Show resolved Hide resolved
source/uwp/Renderer/lib/AdaptiveColumnRenderer.cpp Outdated Show resolved Hide resolved
@licanhua
Copy link
Contributor

    rtxaml::Thickness buttonMargin{};

in WinUI, some magic played in microsoft-ui-xaml\dev\inc\CppWinRTIncludes.h.
I believe it's this part:

namespace winrt
{
    using namespace ::winrt::Windows;
    using namespace ::winrt::Windows::ApplicationModel::Activation;
    using namespace ::winrt::Windows::ApplicationModel::Contacts;
    using namespace ::winrt::Windows::ApplicationModel::Core;

In reply to: 982875118


Refers to: source/uwp/Renderer/lib/ActionHelpers.cpp:15 in a74771b. [](commit_id = a74771b, deletion_comment = False)

Copy link
Contributor

@licanhua licanhua left a comment

Choose a reason for hiding this comment

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

🕐

@beervoley
Copy link
Contributor Author

    rtxaml::Thickness buttonMargin{};

in WinUI, some magic played in microsoft-ui-xaml\dev\inc\CppWinRTIncludes.h. I believe it's this part:

namespace winrt
{
    using namespace ::winrt::Windows;
    using namespace ::winrt::Windows::ApplicationModel::Activation;
    using namespace ::winrt::Windows::ApplicationModel::Contacts;
    using namespace ::winrt::Windows::ApplicationModel::Core;

In reply to: 982875118

Refers to: source/uwp/Renderer/lib/ActionHelpers.cpp:15 in a74771b. [](commit_id = a74771b, deletion_comment = False)

I'll have a look as to how organize namespaces before merging:)
In WinUI2 it was really convenient, you're right..

@ghost ghost removed the Needs: Author Feedback label Nov 30, 2021
source/uwp/Renderer/AdaptiveCardRenderer.vcxproj Outdated Show resolved Hide resolved
source/uwp/Renderer/lib/XamlHelpers.cpp Outdated Show resolved Hide resolved
source/uwp/Renderer/lib/XamlHelpers.cpp Outdated Show resolved Hide resolved
@licanhua
Copy link
Contributor

    button.Click([action, actionInvoker](winrt::IInspectable const&, winrt::RoutedEventArgs const&)

Can you double check this? I'm wondering it may cause memory leak. when we set callback handler, we may need to use auto_revoke or something.

possible call:

button.Click(winrt::auto_revoke, lampda)

Refers to: source/uwp/Renderer/lib/ActionHelpers.cpp:578 in 394589e. [](commit_id = 394589e, deletion_comment = False)

@licanhua
Copy link
Contributor

    button.Click([action, actionInvoker](winrt::IInspectable const&, winrt::RoutedEventArgs const&)

It applies to all event callback if you have


In reply to: 1021532853


Refers to: source/uwp/Renderer/lib/ActionHelpers.cpp:578 in 394589e. [](commit_id = 394589e, deletion_comment = False)

@licanhua
Copy link
Contributor

inline winrt::SolidColorBrush GetSolidColorBrush(winrt::Windows::UI::Color const& color)

maybe you don't need this function and use winrt::SolidColorBrush(color) directly


Refers to: source/uwp/Renderer/lib/XamlHelpers.h:8 in 394589e. [](commit_id = 394589e, deletion_comment = False)

@licanhua
Copy link
Contributor

            auto wholeItemsPanelImpl = peek_innards<winrt::implementation::WholeItemsPanel>(wholeItemsPanel);

What's peek_innards? if it's weak to strong ref convert, you may need nullptr check here


Refers to: source/uwp/Renderer/lib/XamlHelpers.h:58 in 394589e. [](commit_id = 394589e, deletion_comment = False)

@licanhua
Copy link
Contributor

licanhua commented Jan 25, 2022

    SetContent(item, contentString, false);

false /* wrap */


In reply to: 1021678384


Refers to: source/uwp/Renderer/lib/XamlHelpers.h:72 in 394589e. [](commit_id = 394589e, deletion_comment = False)

@licanhua
Copy link
Contributor

licanhua commented Jan 25, 2022

    if (const auto contentControl = item.try_as<winrt::ContentControl>())

move if (const auto contentControl = item.try_as<winrt::ContentControl>()) to the first line of the function

if (const auto contentControl = item.try_as<winrt::ContentControl>())
{
        winrt::TextBlock content{};
        content.Text(contentString);
...
}

In reply to: 1021681055


Refers to: source/uwp/Renderer/lib/XamlHelpers.h:85 in 394589e. [](commit_id = 394589e, deletion_comment = False)

@licanhua
Copy link
Contributor

        auto containerAsPanelImpl = peek_innards<winrt::implementation::WholeItemsPanel>(containerAsPanel);

will it need nullptr check?


Refers to: source/uwp/Renderer/lib/XamlHelpers.h:103 in 394589e. [](commit_id = 394589e, deletion_comment = False)

@licanhua
Copy link
Contributor

licanhua commented Jan 25, 2022

            if (tag)

nit: if (auto tag = childAsFrameworkElement.Tag())


In reply to: 1021683309


In reply to: 1021683309


Refers to: source/uwp/Renderer/lib/XamlHelpers.cpp:67 in 394589e. [](commit_id = 394589e, deletion_comment = False)

@licanhua
Copy link
Contributor

licanhua commented Jan 25, 2022

  			validationBorder = containerAsBorder;

nit: tab to space


In reply to: 1021694975


In reply to: 1021694975


Refers to: source/uwp/Renderer/lib/XamlHelpers.cpp:802 in 394589e. [](commit_id = 394589e, deletion_comment = False)

@licanhua
Copy link
Contributor

licanhua commented Jan 25, 2022

    double widthAsDouble = _wtof(adaptiveColumnWidth.data());

it's too hacky to convert "stretch" to double.
It may eat some exception like overflow, or upper case like "Stretch". Please clearly state your intention.
Also I prefer stod or similar function

for example:

if (width == L"stretch") {

} else ... {
try {
std::stod()
} catch() {
}
}


In reply to: 1021702420


Refers to: source/uwp/Renderer/lib/XamlHelpers.cpp:214 in 394589e. [](commit_id = 394589e, deletion_comment = False)

@licanhua
Copy link
Contributor

licanhua commented Jan 25, 2022

        if (fallbackControl == nullptr)

if (!fallbackControl)


In reply to: 1021705591


Refers to: source/uwp/Renderer/lib/XamlHelpers.cpp:323 in 394589e. [](commit_id = 394589e, deletion_comment = False)

@beervoley
Copy link
Contributor Author

            auto wholeItemsPanelImpl = peek_innards<winrt::implementation::WholeItemsPanel>(wholeItemsPanel);

What's peek_innards? if it's weak to strong ref convert, you may need nullptr check here

Refers to: source/uwp/Renderer/lib/XamlHelpers.h:58 in 394589e. [](commit_id = 394589e, deletion_comment = False)

peek_innards is the same as winrt:;get_self for our own implementations but with additional GUID check to make sure that the implementation is our own and not something coming from outside :)

@beervoley
Copy link
Contributor Author

    double widthAsDouble = _wtof(adaptiveColumnWidth.data());

it's too hacky to convert "stretch" to double. It may eat some exception like overflow, or upper case like "Stretch". Please clearly state your intention. Also I prefer stod or similar function

for example:

if (width == L"stretch") {

} else ... { try { std::stod() } catch() { } }

Refers to: source/uwp/Renderer/lib/XamlHelpers.cpp:214 in 394589e. [](commit_id = 394589e, deletion_comment = False)

I agree there is a better way to deal with this but CalculateColumnWidth will deal with these scenarios for now:)
I will come back to it on a later date once have time

@beervoley
Copy link
Contributor Author

    button.Click([action, actionInvoker](winrt::IInspectable const&, winrt::RoutedEventArgs const&)

Can you double check this? I'm wondering it may cause memory leak. when we set callback handler, we may need to use auto_revoke or something.

possible call:

button.Click(winrt::auto_revoke, lampda)

Refers to: source/uwp/Renderer/lib/ActionHelpers.cpp:578 in 394589e. [](commit_id = 394589e, deletion_comment = False)

leak shouldn't happen in any of these scenarios because both the subscriber (class to which method belongs, in this case lambda is most likely compiled to the member class method of the class that's subscribing) and publisher (button) have the same lifetimes. Once the rendered card is disposed, the button will be disposed of, and when doing so, it will release the ref on the subscriber :)

Current Sprint automation moved this from Review in progress to Reviewer approved Jan 28, 2022
@beervoley beervoley merged commit 1dcef8c into main Jan 28, 2022
Current Sprint automation moved this from Reviewer approved to Done Jan 28, 2022
@beervoley beervoley deleted the user/vsiliush/renderer-winrt-final branch January 28, 2022 21:52
michaelfarnsworth pushed a commit to michaelfarnsworth/AdaptiveCards that referenced this pull request Nov 10, 2022
@beervoley beervoley restored the user/vsiliush/renderer-winrt-final branch December 9, 2022 00:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

Convert input types to C++/WinRT
8 participants