Make Profile a WinRT object by carlos-zamora · Pull Request #7283 · microsoft/terminal

carlos-zamora · 2020-08-13T21:06:49Z

Profile is now a WinRT object in the TerminalApp project.

As with ColorScheme, all of the serialization logic is not exposed via
the idl. TerminalSetingsModel will handle it when it's all moved over.

I removed the "Get" and "Set" prefixes from all of the Profile
functions. It just makes more sense to use the GETSET_PROPERTY macro
to do most of the work for us.

CloseOnExitMode is now an enum off of the Profile.idl.

std::optional<wstring> got converted to hstring (as opposed to
IReference<hstring>). IReference<hstring> is not valid to MIDL.

References

#7141 - Profile is a settings object
#885 - this new settings object will be moved to a new TerminalSettingsModel project

Validation Steps Performed

Tests passed
Deployment succeeded

Closes #7435

DHowett · 2020-08-13T23:26:26Z

It will be important to retarget this PR before you merge and delete the destination branch (!)

carlos-zamora

Found a few small typos with massive consequences. Fixed though :)

Now I just have to figure out this one:

src/cascadia/TerminalApp/DefaultProfileUtils.cpp

src/cascadia/TerminalApp/Profile.cpp

src/cascadia/TerminalApp/CascadiaSettingsSerialization.cpp

zadjii-msft

Okay I'm at 24/29, and i bet you can guess which 5 I haven't been to yet

src/cascadia/TerminalApp/TerminalSettingsSerializationHelpers.h

src/cascadia/TerminalApp/DefaultProfileUtils.cpp

zadjii-msft · 2020-08-17T21:24:36Z

src/cascadia/TerminalApp/DefaultProfileUtils.cpp

+winrt::TerminalApp::Profile CreateDefaultProfile(const std::wstring_view name)
 {
-    const auto profileGuid{ Microsoft::Console::Utils::CreateV5Uuid(TERMINAL_PROFILE_NAMESPACE_GUID,
+    const winrt::guid profileGuid{ Microsoft::Console::Utils::CreateV5Uuid(TERMINAL_PROFILE_NAMESPACE_GUID,


this could still be auto, right? There should be an magic GUID->winrt::guid conversion already

I wish. I think that the compiler gets confused with the winrt::Windows::Foundation::IReference<winrt::guid>. :(

1>D:\Terminal\src\cascadia\TerminalApp\DefaultProfileUtils.cpp(22,55): error C2440: 'initializing': cannot convert from 'initializer list' to 'winrt::TerminalApp::Profile' 1>D:\Terminal\src\cascadia\TerminalApp\DefaultProfileUtils.cpp(22,55): message : No constructor could take the source type, or constructor overload resolution was ambiguous 1>D:\Terminal\src\cascadia\TerminalApp\DefaultProfileUtils.cpp(24,32): error C2664: 'auto winrt::impl::consume_TerminalApp_IProfile<winrt::TerminalApp::IProfile>::Guid(const winrt::Windows::Foundation::IReference<winrt::guid> &) const': cannot convert argument 1 from 'const GUID' to 'const winrt::Windows::Foundation::IReference<winrt::guid> &' 1>D:\Terminal\src\cascadia\TerminalApp\DefaultProfileUtils.cpp(24,32): message : Reason: cannot convert from 'const GUID' to 'const winrt::Windows::Foundation::IReference<winrt::guid>' 1>D:\Terminal\src\cascadia\TerminalApp\DefaultProfileUtils.cpp(24,21): message : No user-defined-conversion operator available that can perform this conversion, or the operator cannot be called

This actually seems to be the source of a lot of the comments you made. I've been getting around it by just using winrt::guid instead of GUID, but if you have a better idea in mind, go for it.

oh that makes me unreasonably angry

something something type covariance

src/cascadia/TerminalApp/Profile.idl

src/cascadia/TerminalApp/PowershellCoreProfileGenerator.cpp

src/cascadia/TerminalApp/TerminalPage.cpp

src/cascadia/TerminalApp/CascadiaSettingsSerialization.cpp

src/cascadia/ut_app/DynamicProfileTests.cpp

src/cascadia/TerminalApp/Profile.cpp

carlos-zamora · 2020-08-18T22:01:58Z

@DHowett, you might be interested in 03f6231. Is there a cleaner way to handle this issue with JsonUtils? I made it look a bit messier than before.

carlos-zamora · 2020-08-19T16:42:00Z

@DHowett, you might be interested in 03f6231. Is there a cleaner way to handle this issue with JsonUtils? I made it look a bit messier than before.

Chatted offline. JsonUtils should be able to handle IReference as a std::optional and uses the same converter between GUID and winrt::guid.

But it looks like IReference<winrt::guid> still gets confused somehow. Check this out:

1>D:\Terminal\src\cascadia\TerminalApp\JsonUtils.h(450,1): error C2679: binary '=': no operator found which takes a right-hand operand of type 'GUID' (or there is no acceptable conversion)
1>D:\Terminal\src\cascadia\TerminalApp\lib\Generated Files\winrt\impl\Windows.Foundation.1.h(156,1): message : could be 'winrt::Windows::Foundation::IReference<winrt::guid> &winrt::Windows::Foundation::IReference<winrt::guid>::operator =(winrt::Windows::Foundation::IReference<winrt::guid> &&)'
1>D:\Terminal\src\cascadia\TerminalApp\lib\Generated Files\winrt\impl\Windows.Foundation.1.h(156,1): message : or       'winrt::Windows::Foundation::IReference<winrt::guid> &winrt::Windows::Foundation::IReference<winrt::guid>::operator =(const winrt::Windows::Foundation::IReference<winrt::guid> &)'
1>D:\Terminal\src\cascadia\TerminalApp\JsonUtils.h(450,1): message : while trying to match the argument list '(T, GUID)'
1>        with
1>        [
1>            T=winrt::Windows::Foundation::IReference<winrt::guid>
1>        ]
1>D:\Terminal\src\cascadia\TerminalApp\JsonUtils.h(473): message : see reference to function template instantiation 'bool TerminalApp::JsonUtils::GetValue<T,_Ty>(const Json::Value &,T &,Converter &&)' being compiled
1>        with
1>        [
1>            T=winrt::Windows::Foundation::IReference<winrt::guid>,
1>            _Ty=TerminalApp::JsonUtils::ConversionTrait<winrt::guid>,
1>            Converter=TerminalApp::JsonUtils::ConversionTrait<winrt::guid>
1>        ]
1>D:\Terminal\src\cascadia\TerminalApp\JsonUtils.h(513): message : see reference to function template instantiation 'bool TerminalApp::JsonUtils::GetValueForKey<T,TerminalApp::JsonUtils::ConversionTrait<winrt::guid>>(const Json::Value &,std::string_view,T &,Converter &&)' being compiled
1>        with
1>        [
1>            T=winrt::Windows::Foundation::IReference<winrt::guid>,
1>            Converter=TerminalApp::JsonUtils::ConversionTrait<winrt::guid>
1>        ]
1>D:\Terminal\src\cascadia\TerminalApp\Profile.cpp(322): message : see reference to function template instantiation 'bool TerminalApp::JsonUtils::GetValueForKey<winrt::Windows::Foundation::IReference<winrt::guid>>(const Json::Value &,std::string_view,T &)' being compiled
1>        with
1>        [
1>            T=winrt::Windows::Foundation::IReference<winrt::guid>
1>        ]

zadjii-msft

re:

I was considering that for a bit, actually. If we go that route, I think it would make sense to break up
GETSET_PROPERTY(Windows::Foundation::IReference<guid>, Guid, nullptr);
into
// return/accept guid instead of IReference<guid>
bool HasGuid();
guid Guid();
void Guid(guid);

Honestly, that feels better to me, but I know how much annoying work that is. IMO, it seems like external to the deserialization of the settings, a profile should always have a GUID. Kinda like how right now we just throw if a profile ever doesn't. That'll prevent the need for all the other classes to do the if (profile.Guid() != nullptr) { profile.Guid().Value(); } dance.

src/cascadia/ut_app/DynamicProfileTests.cpp

src/cascadia/TerminalApp/JsonUtils.h

zadjii-msft

Alright, these are all only nits, so I won't block over them.

src/cascadia/TerminalApp/Profile.h

src/cascadia/TerminalApp/Profile.cpp

miniksa

I don't feel confident enough in WinRT-isms to sign off here, but I didn't see a whole lot out of order. Just left a few small comments.

DHowett

NAK because of the null discussion and change in behavior

src/cascadia/TerminalApp/Profile.cpp

carlos-zamora · 2020-08-26T23:58:18Z

I still want/need to double check if there is similar behavior with making the other settings null. Assigning to myself to do that. So this PR is not ready to go yet.

carlos-zamora · 2020-08-27T23:33:15Z

The latest commit closes #7435 as well.

src/cascadia/TerminalApp/Profile.h

src/cascadia/TerminalApp/Profile.cpp

DHowett · 2020-08-28T01:07:43Z

@msftbot merge this in 1 minute

ghost · 2020-08-28T01:07:46Z

Hello @DHowett!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

I won't merge this pull request until after the UTC date Fri, 28 Aug 2020 01:08:45 GMT, which is in 1 minute

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

ghost · 2020-09-22T16:02:03Z

🎉Windows Terminal Preview v1.4.2652.0 has been released which incorporates this pull request.:tada:

Handy links:

This comment has been minimized.

Sign in to view

carlos-zamora force-pushed the dev/cazamor/set/winrt-app-obj branch from 07dfdb7 to 1fb018d Compare August 13, 2020 21:41

carlos-zamora force-pushed the dev/cazamor/set/winrt-app-obj-prof branch from fde1189 to 5d5ab10 Compare August 13, 2020 21:46

carlos-zamora added Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. labels Aug 13, 2020

carlos-zamora force-pushed the dev/cazamor/set/winrt-app-obj-prof branch from 7d81387 to 105183d Compare August 14, 2020 20:30

carlos-zamora commented Aug 14, 2020

View reviewed changes

src/cascadia/TerminalApp/DefaultProfileUtils.cpp Outdated Show resolved Hide resolved

src/cascadia/TerminalApp/Profile.cpp Show resolved Hide resolved

src/cascadia/TerminalApp/CascadiaSettingsSerialization.cpp Outdated Show resolved Hide resolved

Base automatically changed from dev/cazamor/set/winrt-app-obj to master August 15, 2020 00:54

carlos-zamora force-pushed the dev/cazamor/set/winrt-app-obj-prof branch from 227172f to 17648c7 Compare August 17, 2020 18:02

zadjii-msft requested changes Aug 17, 2020

View reviewed changes

ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Aug 17, 2020

carlos-zamora commented Aug 18, 2020

View reviewed changes

src/cascadia/TerminalApp/Profile.cpp Outdated Show resolved Hide resolved

carlos-zamora marked this pull request as ready for review August 18, 2020 21:59

carlos-zamora assigned zadjii-msft Aug 18, 2020

carlos-zamora requested a review from a team August 18, 2020 21:59

carlos-zamora assigned DHowett Aug 19, 2020

zadjii-msft reviewed Aug 19, 2020

View reviewed changes

src/cascadia/ut_app/DynamicProfileTests.cpp Show resolved Hide resolved

zadjii-msft assigned carlos-zamora and unassigned zadjii-msft Aug 19, 2020

carlos-zamora commented Aug 20, 2020

View reviewed changes

src/cascadia/TerminalApp/JsonUtils.h Show resolved Hide resolved

carlos-zamora force-pushed the dev/cazamor/set/winrt-app-obj-prof branch from b8f476b to 1a20cb3 Compare August 20, 2020 06:21

carlos-zamora assigned zadjii-msft and unassigned carlos-zamora Aug 20, 2020

carlos-zamora force-pushed the dev/cazamor/set/winrt-app-obj-prof branch from 6051822 to 8396b2d Compare August 21, 2020 03:59

zadjii-msft approved these changes Aug 21, 2020

View reviewed changes

src/cascadia/TerminalApp/Profile.h Outdated Show resolved Hide resolved

src/cascadia/TerminalApp/Profile.h Outdated Show resolved Hide resolved

src/cascadia/TerminalApp/Profile.h Outdated Show resolved Hide resolved

src/cascadia/TerminalApp/Profile.h Outdated Show resolved Hide resolved

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Aug 26, 2020

miniksa reviewed Aug 26, 2020

View reviewed changes

src/cascadia/TerminalApp/Profile.cpp Outdated Show resolved Hide resolved

miniksa reviewed Aug 26, 2020

View reviewed changes

src/cascadia/TerminalApp/Profile.cpp Outdated Show resolved Hide resolved

miniksa reviewed Aug 26, 2020

View reviewed changes

PR comments and other polish

2c21f77

carlos-zamora assigned DHowett and unassigned carlos-zamora Aug 26, 2020

DHowett requested changes Aug 26, 2020

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Aug 26, 2020

DHowett removed their assignment Aug 26, 2020

DHowett reviewed Aug 26, 2020

View reviewed changes

src/cascadia/TerminalApp/Profile.cpp Outdated Show resolved Hide resolved

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Aug 26, 2020

properly handle a null starting directory

0ed994a

carlos-zamora self-assigned this Aug 26, 2020

carlos-zamora mentioned this pull request Aug 27, 2020

Handling Nullable Profile Settings #7435

Closed

update String handling, and reduce optional settings

90de494

ghost added the Area-Settings Issues related to settings and customizability, for console or terminal label Aug 27, 2020

carlos-zamora assigned DHowett and unassigned carlos-zamora Aug 27, 2020

DHowett reviewed Aug 27, 2020

View reviewed changes

src/cascadia/TerminalApp/Profile.h Outdated Show resolved Hide resolved

src/cascadia/TerminalApp/Profile.cpp Show resolved Hide resolved

use {} not L''

45aed5e

ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label Aug 28, 2020

DHowett approved these changes Aug 28, 2020

View reviewed changes

ghost merged commit a51091c into master Aug 28, 2020

ghost deleted the dev/cazamor/set/winrt-app-obj-prof branch August 28, 2020 01:09

This pull request was closed.

Conversation

carlos-zamora commented Aug 13, 2020 • edited by DHowett Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

References

Validation Steps Performed

Uh oh!

This comment has been minimized.

DHowett commented Aug 13, 2020

Uh oh!

carlos-zamora left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zadjii-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

zadjii-msft Aug 17, 2020

Choose a reason for hiding this comment

Uh oh!

carlos-zamora Aug 17, 2020

Choose a reason for hiding this comment

Uh oh!

carlos-zamora Aug 17, 2020

Choose a reason for hiding this comment

Uh oh!

zadjii-msft Aug 18, 2020

Choose a reason for hiding this comment

Uh oh!

DHowett Aug 26, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

carlos-zamora commented Aug 18, 2020

Uh oh!

carlos-zamora commented Aug 19, 2020

Uh oh!

zadjii-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

zadjii-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

miniksa left a comment

Choose a reason for hiding this comment

Uh oh!

DHowett left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

carlos-zamora commented Aug 26, 2020

Uh oh!

carlos-zamora commented Aug 27, 2020

Uh oh!

Uh oh!

Uh oh!

DHowett commented Aug 28, 2020

Uh oh!

ghost commented Aug 28, 2020

Uh oh!

ghost commented Sep 22, 2020

Uh oh!

carlos-zamora commented Aug 13, 2020 •

edited by DHowett

Loading