-
Notifications
You must be signed in to change notification settings - Fork 325
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
base: main-old
Are you sure you want to change the base?
Conversation
dev/VSIX/ProjectTemplates/Desktop/CppWinRT/SingleProjectPackagedApp/App.cpp
Outdated
Show resolved
Hide resolved
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? |
Could you add filters to organize files shown in Solution Explorer? The C++ project template has many more files than C#. |
...mplates/Desktop/CppWinRT/RuntimeComponent/WinUI.Desktop.CppWinRT.RuntimeComponent.vstemplate
Outdated
Show resolved
Hide resolved
@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:
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. |
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>())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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$">
)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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]>
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)
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)
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:
using namespace
for consistency with the rest of the code. Also removedusing 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).