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

Improved C++ item & templates #3619

Open
wants to merge 10 commits into
base: main-old
Choose a base branch
from

Conversation

JaiganeshKumaran
Copy link
Contributor

A microsoft employee must use /azp run to validate using the pipelines below.

WARNING:
Comments made by azure-pipelines bot maybe inaccurate.
Please see pipeline link to verify that the build is being ran.

For status checks on the develop branch, please use TransportPackage-Foundation-PR
(https://microsoft.visualstudio.com/ProjectReunion/_build?definitionId=81063&_a=summary)
and run the build against your PR branch with the default parameters.

For status checks on the main branch, please use microsoft.ProjectReunion
(https://dev.azure.com/ms/ProjectReunion/_build?definitionId=391&_a=summary)
and run the build against your PR branch with the default parameters.

Unfortunately, while C# item & project templates were cleaned up in 1.3.0, C++ item & project templates weren't.

Changes made in this pull request:

  • Removed unnecessary MyProperty property and myButton_Click handler (except for MainWindow in the project template).
  • Removed usage of InitializeComponent as it's no longer recommended by C++/WinRT (see https://github.com/microsoft/cppwinrt/tree/master/nuget#initializecomponent for more details).
  • Removed unnecessary include of g.cpp file. Almost every app-level XAML type must have a public constructor, which means the g.cpp file will get generated.
  • Removed Microsoft copyright notice.
  • Removed #include XAML headers in a template control header. This was in the C++/WinRT templates because a runtime component will not include XAML headers in pch.h, which is no longer the case with a WinUI 3 runtime component.
  • Replaced boxing a hardcoded type name string with the winrt::xaml_typename function. The previous one will actually crash at runtime because it isn't a TypeName.
  • Included Windows.UI.Xaml.Interop.h in pch.h because its TypeName struct is still used in WinUI 3 and is required to get the winrt::xaml_typename function.
  • Made the WinUI 3 runtime component "desktop-only" as WinUI 3 cannot be used by UWP apps. This is a fix for Win32 Desktop functions not available in WinUI 3 Runtime Component. #1780.
  • Removed App.idl because it's empty and not needed.
  • Indented App.xaml.cpp's content inside the implementation namespace, rather than using namespace for consistency with the rest of the code. Also removed using namespace for namespaces that aren't actually used (Controls and Navigation namespaces were needed for Frame navigation, which isn't performed in the WinUI 3 template. IInspectable is available as an alias in every WinRT implementation type, thus doesn't require prefixing it with Windows::Foundation).

@DrusTheAxe
Copy link
Member

DrusTheAxe commented Jun 4, 2023

@Scottj1s @bpulliam

@JaiganeshKumaran
Copy link
Contributor Author

JaiganeshKumaran commented Jun 4, 2023

There are a few other things that could be done. Currently, the default templates enable dot prefix for C++/WinRT generated files in a sub-namespace. For example, if you have a runtime class under the View namespace, the generated file you'll need to include will be Views.RuntimeClass.g.h - however due to a long-standing XAML Compiler bug (microsoft/microsoft-ui-xaml#7652), it will not compile since the file generated by the XAML Compiler always seems to be under a subfolder.

Another thing is if you add one of these to a project whose root namespace itself is nested, you'll see the templates emit dots in the C++ header and source instead of double colons. I would also really appreciate being able to create an item in a subfolder and it's automatically put into a sub-namespace.

Lastly, I see that the WinUI 3 item templates show up even in a WinUI 2 project. Should we move them all from neutral to desktop?

@sotanakamura
Copy link

Could you add filters to organize files shown in Solution Explorer? The C++ project template has many more files than C#.

@evelynwu-msft
Copy link
Contributor

@JaiganeshKumaran thank you so much for taking the time to make these improvements to our project templates. We reviewed this PR internally and we like it a lot although there are some things that we believe merit further discussion. Unfortunately, because of internal scheduling constraints we don't have sufficient time to have that additional discussion and include your work here in the WinAppSDK 1.4 release and resolve merge conflicts with other in-flight work that we know will occur. So here's the plan we devised to fast-track the uncontroversial parts of your PR:

  1. I will create a new PR into the develop branch containing the portions of this PR that we know for certain we want to accept. This will allow for expedited code flow into the release branch.
  2. Once things have settled down and you have had a chance to merge main into your topic branch we will pick up this PR where we left off.
  3. The portions of this PR that weren't taken into the separate PR will target WinAppSDK 1.5.

Please rest assured that we're not trying to take credit for your work. I would much prefer it if you could drive the process from beginning to end but unfortunately our schedule won't permit that and this is what we came up with as a compromise to allow most of your changes in this PR to ship in WinAppSDK 1.4.

@sylveon
Copy link

sylveon commented Jul 17, 2023

You can make the commits in your PR as authored by Jaiganesh but committed by you - GitHub will keep the credits to him.

void $safeitemname$::MyProperty(int32_t /* value */)
{
throw hresult_not_implemented();
DefaultStyleKey(winrt::box_value(xaml_typename<class_type>()));
Copy link
Contributor

Choose a reason for hiding this comment

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

xaml_typename<class_type>()

XAML accepts boxed strings so this change isn't necessary. Can you provide a min repro project for the crash you were encountering with the previous incarnation?

Copy link

Choose a reason for hiding this comment

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

It's not a crash, it's a compile error. C++/WinRT hstring does not implicitly convert to IInspectable. It needs to be explicitly done (via winrt::box_value). The template won't compile by default.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's correct. However, the change being proposed here is simply from hstring to xaml_typename; the enclosing winrt::box_value() call isn't being touched.

Copy link

Choose a reason for hiding this comment

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

Ah, I see. xaml_typename is a C++/WinRT helper that gives a Windows::UI::Xaml::Interop::TypeName for the type. This matches more closely to the C# template (which uses typeof instead of a string), but the runtime behavior is the same (with both the new code and the old code, the runtime will look for <Style TargetType="$rootnamespace$.$safeitemname$">)

Copy link
Contributor

Choose a reason for hiding this comment

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

The primary benefit of using xaml_typename<class_type>() over 'hstring' that I'm aware of is that it simplifies renaming the class. But the argument being made here instead is that there's an error associated with the hstring variant so that's why I want to dig deeper.

Copy link
Contributor Author

@JaiganeshKumaran JaiganeshKumaran Jul 19, 2023

Choose a reason for hiding this comment

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

You're right DefaultStyleKey can be set to a boxed hstring as well, it doesn't have to be a TypeName. As an alternative, winrt::name_of could be used instead of winrt::xaml_typename. There are a few differences between the name_of and xaml_typename, for example, name_of<Windows::Foundation::Point>() returns "Windows.Foundation.Point" while xaml_typename<Windows::Foundation::Point>().Name is "Point". This doesn't matter in this case though.

evelynwu-msft added a commit that referenced this pull request Jul 20, 2023
This is a partial adoption of #3619 by @JaiganeshKumaran in order to expedite the merging of the changes from that PR which do not need additional time for discussion. The individual changes that were extracted from the aforementioned PR are as follows:

- Removed usage of InitializeComponent as it's no longer recommended by C++/WinRT (see https://github.com/microsoft/cppwinrt/tree/master/nuget#initializecomponent for more details).
- Removed Microsoft copyright notice.
- Removed #include XAML headers in a template control header. This was in the C++/WinRT templates because a runtime component will not include XAML headers in pch.h, which is no longer the case with a WinUI 3 runtime component.
- Made the WinUI 3 runtime component "desktop-only" as WinUI 3 cannot be used by UWP apps. This is a fix for #1780. (Note that unlike the original PR the source directory structure is not being changed in order to minimize potential merge conflicts.)
- Removed App.idl because it's empty and not needed.
- Indented App.xaml.cpp's content inside the implementation namespace, rather than `using namespace` for consistency with the rest of the code. Also removed `using namespace` for namespaces that aren't actually used (Controls and Navigation namespaces were needed for Frame navigation, which isn't performed in the WinUI 3 template. IInspectable is available as an alias in every WinRT implementation type, thus doesn't require prefixing it with Windows::Foundation).

Additionally, this PR replaces `<ProjectTypeTag>uwp</ProjectTypeTag>` with `<ProjectTypeTag>WinUI</ProjectTypeTag>` (or just adds the latter if the former was not present at all) in the item templates. I don't believe this tag is surfaced anywhere in the Visual Studio UI but the consistency makes me happier.

---------

Co-authored-by: Jaiganésh Kumaran <[email protected]>
evelynwu-msft added a commit that referenced this pull request Jul 21, 2023
This is a partial adoption of #3619 by @JaiganeshKumaran in order to expedite the merging of the changes from that PR which do not need additional time for discussion. The individual changes that were extracted from the aforementioned PR are as follows:

- Removed usage of InitializeComponent as it's no longer recommended by C++/WinRT (see https://github.com/microsoft/cppwinrt/tree/master/nuget#initializecomponent for more details).
- Removed Microsoft copyright notice.
- Removed #include XAML headers in a template control header. This was in the C++/WinRT templates because a runtime component will not include XAML headers in pch.h, which is no longer the case with a WinUI 3 runtime component.
- Made the WinUI 3 runtime component "desktop-only" as WinUI 3 cannot be used by UWP apps. This is a fix for #1780. (Note that unlike the original PR the source directory structure is not being changed in order to minimize potential merge conflicts.)
- Removed App.idl because it's empty and not needed.
- Indented App.xaml.cpp's content inside the implementation namespace, rather than `using namespace` for consistency with the rest of the code. Also removed `using namespace` for namespaces that aren't actually used (Controls and Navigation namespaces were needed for Frame navigation, which isn't performed in the WinUI 3 template. IInspectable is available as an alias in every WinRT implementation type, thus doesn't require prefixing it with Windows::Foundation).

Additionally, this PR replaces `<ProjectTypeTag>uwp</ProjectTypeTag>` with `<ProjectTypeTag>WinUI</ProjectTypeTag>` (or just adds the latter if the former was not present at all) in the item templates. I don't believe this tag is surfaced anywhere in the Visual Studio UI but the consistency makes me happier.

---------

Co-authored-by: Jaiganésh Kumaran <[email protected]>
(cherry picked from commit 18ada2f)
evelynwu-msft added a commit that referenced this pull request Jul 21, 2023
This is a partial adoption of #3619 by @JaiganeshKumaran in order to expedite the merging of the changes from that PR which do not need additional time for discussion. The individual changes that were extracted from the aforementioned PR are as follows:

- Removed usage of InitializeComponent as it's no longer recommended by C++/WinRT (see https://github.com/microsoft/cppwinrt/tree/master/nuget#initializecomponent for more details).
- Removed Microsoft copyright notice.
- Removed #include XAML headers in a template control header. This was in the C++/WinRT templates because a runtime component will not include XAML headers in pch.h, which is no longer the case with a WinUI 3 runtime component.
- Made the WinUI 3 runtime component "desktop-only" as WinUI 3 cannot be used by UWP apps. This is a fix for #1780. (Note that unlike the original PR the source directory structure is not being changed in order to minimize potential merge conflicts.)
- Removed App.idl because it's empty and not needed.
- Indented App.xaml.cpp's content inside the implementation namespace, rather than `using namespace` for consistency with the rest of the code. Also removed `using namespace` for namespaces that aren't actually used (Controls and Navigation namespaces were needed for Frame navigation, which isn't performed in the WinUI 3 template. IInspectable is available as an alias in every WinRT implementation type, thus doesn't require prefixing it with Windows::Foundation).

Additionally, this PR replaces `<ProjectTypeTag>uwp</ProjectTypeTag>` with `<ProjectTypeTag>WinUI</ProjectTypeTag>` (or just adds the latter if the former was not present at all) in the item templates. I don't believe this tag is surfaced anywhere in the Visual Studio UI but the consistency makes me happier.

---------

Co-authored-by: Jaiganésh Kumaran <[email protected]>
(cherry picked from commit 18ada2f)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants